From 56af41f15535736d5d126bdf594f3f5c99e45271 Mon Sep 17 00:00:00 2001
From: Malte Bauch <malte.bauch@tbnet.works>
Date: Mon, 13 Jun 2022 09:17:35 +0000
Subject: [PATCH] Change gnmiNotifications sent via applyUpdate to use
IETF_JSON
See merge request danet/gosdn!331
Co-authored-by: Malte Bauch <malte.bauch@extern.h-da.de>
---
.gitlab/ci/.test.yml | 2 +-
cli/cmd/deviceSet.go | 2 +-
controller/interfaces/transport/transport.go | 2 +-
controller/mocks/Transport.go | 10 +-
controller/nucleus/errors/errors.go | 25 +++++
controller/nucleus/gnmi_transport.go | 93 +++++++++++++++++--
controller/nucleus/gnmi_transport_test.go | 92 +++++++++++++++++-
controller/nucleus/initialise_test.go | 2 +-
controller/nucleus/principalNetworkDomain.go | 7 +-
.../nucleus/principalNetworkDomain_test.go | 2 +-
.../integration/nucleusIntegration_test.go | 11 ++-
11 files changed, 221 insertions(+), 27 deletions(-)
diff --git a/.gitlab/ci/.test.yml b/.gitlab/ci/.test.yml
index 60fea0a97..4d033d2ea 100644
--- a/.gitlab/ci/.test.yml
+++ b/.gitlab/ci/.test.yml
@@ -5,7 +5,7 @@
- when: on_success
variables:
GOSDN_LOG: "nolog"
- GOSDN_CHANGE_TIMEOUT: "100ms"
+ GOSDN_CHANGE_TIMEOUT: "5000ms"
artifacts:
when: always
reports:
diff --git a/cli/cmd/deviceSet.go b/cli/cmd/deviceSet.go
index 1c18443b5..15806d356 100644
--- a/cli/cmd/deviceSet.go
+++ b/cli/cmd/deviceSet.go
@@ -119,6 +119,6 @@ func fileContentToString(path string) (string, error) {
func init() {
deviceCmd.AddCommand(deviceSetCmd)
deviceSetCmd.Flags().BoolVarP(&replace, "replace", "r", false, "enables replace behaviour")
- deviceSetCmd.Flags().StringVar(&file, "json file", "", "reference the path to a file containing your changes as JSON")
+ deviceSetCmd.Flags().StringVar(&file, "file", "", "reference the path to a file containing your changes as JSON")
deviceSetCmd.Flags().BoolVar(&forcePush, "force-push", false, "enables the possibility to instantly push the set without commit/confirm")
}
diff --git a/controller/interfaces/transport/transport.go b/controller/interfaces/transport/transport.go
index a0209d9b7..9d732c370 100644
--- a/controller/interfaces/transport/transport.go
+++ b/controller/interfaces/transport/transport.go
@@ -12,7 +12,7 @@ import (
// like RESTCONF or gnmi
type Transport interface {
Get(ctx context.Context, params ...string) (interface{}, error)
- Set(ctx context.Context, payload change.Payload) error
+ Set(ctx context.Context, payload change.Payload, path string, schema *ytypes.Schema) error
Subscribe(ctx context.Context, params ...string) error
Type() string
ProcessResponse(resp interface{}, root interface{}, models *ytypes.Schema) error
diff --git a/controller/mocks/Transport.go b/controller/mocks/Transport.go
index ae80536a4..10fa882e3 100644
--- a/controller/mocks/Transport.go
+++ b/controller/mocks/Transport.go
@@ -63,13 +63,13 @@ func (_m *Transport) ProcessResponse(resp interface{}, root interface{}, models
return r0
}
-// Set provides a mock function with given fields: ctx, payload
-func (_m *Transport) Set(ctx context.Context, payload change.Payload) error {
- ret := _m.Called(ctx, payload)
+// Set provides a mock function with given fields: ctx, payload, path, schema
+func (_m *Transport) Set(ctx context.Context, payload change.Payload, path string, schema *ytypes.Schema) error {
+ ret := _m.Called(ctx, payload, path, schema)
var r0 error
- if rf, ok := ret.Get(0).(func(context.Context, change.Payload) error); ok {
- r0 = rf(ctx, payload)
+ if rf, ok := ret.Get(0).(func(context.Context, change.Payload, string, *ytypes.Schema) error); ok {
+ r0 = rf(ctx, payload, path, schema)
} else {
r0 = ret.Error(0)
}
diff --git a/controller/nucleus/errors/errors.go b/controller/nucleus/errors/errors.go
index 7883c3598..8c30a9f42 100644
--- a/controller/nucleus/errors/errors.go
+++ b/controller/nucleus/errors/errors.go
@@ -3,6 +3,8 @@ package errors
import (
"fmt"
"reflect"
+
+ "github.com/openconfig/ygot/ygot"
)
// ErrNilClient implements the Error interface and is called if a GNMI Client is nil.
@@ -72,6 +74,17 @@ func (e ErrUnsupportedPath) Error() string {
return fmt.Sprintf("path %v is not supported", e.Path)
}
+// ErrPathNotFound implements the Error interface and is called if the
+// given path is not supported.
+type ErrPathNotFound struct {
+ Path interface{}
+ Err error
+}
+
+func (e ErrPathNotFound) Error() string {
+ return fmt.Sprintf("path %v not found: %v", e.Path, e.Err)
+}
+
// ErrNotYetImplemented implements the Error interface and is called if a function
// is not implemented yet.
type ErrNotYetImplemented struct{}
@@ -206,3 +219,15 @@ type ErrCouldNotDelete struct {
func (e ErrCouldNotDelete) Error() string {
return fmt.Sprintf("could not delete %s", e.StoreName)
}
+
+// ErrNoNewChanges implements the Error interface and is called if a the
+// gNMI-Notification created from ygot.Diff does not contain any `updates` or
+// `deletes`.
+type ErrNoNewChanges struct {
+ Original ygot.GoStruct
+ Modified ygot.GoStruct
+}
+
+func (e ErrNoNewChanges) Error() string {
+ return fmt.Sprintf("There are no changes between %v and %v", e.Original, e.Modified)
+}
diff --git a/controller/nucleus/gnmi_transport.go b/controller/nucleus/gnmi_transport.go
index ff19f96a6..84b9e3b3d 100644
--- a/controller/nucleus/gnmi_transport.go
+++ b/controller/nucleus/gnmi_transport.go
@@ -15,6 +15,7 @@ import (
"code.fbi.h-da.de/danet/gosdn/forks/goarista/gnmi"
gpb "github.com/openconfig/gnmi/proto/gnmi"
"github.com/openconfig/goyang/pkg/yang"
+ "github.com/openconfig/ygot/util"
"github.com/openconfig/ygot/ygot"
"github.com/openconfig/ygot/ytypes"
log "github.com/sirupsen/logrus"
@@ -85,37 +86,109 @@ func (g *Gnmi) Get(ctx context.Context, params ...string) (interface{}, error) {
}
// Set takes a change.Payload struct.
-func (g *Gnmi) Set(ctx context.Context, payload change.Payload) error {
+func (g *Gnmi) Set(ctx context.Context, payload change.Payload, path string, schema *ytypes.Schema) error {
+ p, err := ygot.StringToStructuredPath(path)
+ if err != nil {
+ return err
+ }
if g.client == nil {
return &errors.ErrNilClient{}
}
ctx = gnmi.NewContext(ctx, g.config)
- return g.applyDiff(ctx, payload)
+ return g.applyDiff(ctx, payload, p, schema)
}
-func (g *Gnmi) applyDiff(ctx context.Context, payload change.Payload) error {
- op := ctx.Value(types.CtxKeyOperation)
+// isGNMINotificationEmpty checks if the given gnmi.Notification does not
+// contain any updates or deletes.
+func isGNMINotificationEmpty(n *gpb.Notification) bool {
+ if n.Update == nil || len(n.Update) == 0 {
+ if n.Delete == nil || len(n.Delete) == 0 {
+ return true
+ }
+ }
+ return false
+}
+func (g *Gnmi) applyDiff(ctx context.Context, payload change.Payload, path *gpb.Path, schema *ytypes.Schema) error {
diff, err := ygot.Diff(payload.Original, payload.Modified)
if err != nil {
return err
}
+
+ if isGNMINotificationEmpty(diff) {
+ return errors.ErrNoNewChanges{Original: payload.Original, Modified: payload.Modified}
+ }
+
+ var json []byte
+ if op := ctx.Value(types.CtxKeyOperation); op == ppb.ApiOperation_API_OPERATION_UPDATE || op == ppb.ApiOperation_API_OPERATION_REPLACE {
+ rootCopy, err := ygot.DeepCopy(schema.Root)
+ if err != nil {
+ return err
+ }
+
+ for _, u := range diff.Update {
+ opts := []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}, &ytypes.TolerateJSONInconsistencies{}}
+ if err := g.SetNode(schema.RootSchema(), rootCopy, u.GetPath(), u.GetVal(), opts...); err != nil {
+ return err
+ }
+ }
+
+ ygot.PruneEmptyBranches(rootCopy)
+
+ opts := []ytypes.GetNodeOpt{
+ &ytypes.GetHandleWildcards{},
+ }
+ nodes, err := ytypes.GetNode(schema.RootSchema(), rootCopy, path, opts...)
+ if err != nil {
+ return err
+ }
+
+ if len(nodes) == 0 || err != nil || util.IsValueNil(nodes[0].Data) {
+ return errors.ErrPathNotFound{Path: path, Err: err}
+ }
+
+ json, err = ygot.Marshal7951(nodes[0].Data, &ygot.RFC7951JSONConfig{AppendModuleName: true})
+ if err != nil {
+ return err
+ }
+ }
+
+ req, err := createSetRequest(ctx, diff, json, path)
+ if err != nil {
+ return err
+ }
+
+ resp, err := g.client.Set(ctx, req)
+ log.Info(resp)
+ return err
+}
+
+func createSetRequest(ctx context.Context, diff *gpb.Notification, json []byte, path *gpb.Path) (*gpb.SetRequest, error) {
+ op := ctx.Value(types.CtxKeyOperation)
req := &gpb.SetRequest{}
if diff.Update != nil {
switch op {
case ppb.ApiOperation_API_OPERATION_UPDATE:
- req.Update = diff.Update
+ req.Update = []*gpb.Update{{
+ Path: path,
+ Val: &gpb.TypedValue{
+ Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: json},
+ },
+ }}
case ppb.ApiOperation_API_OPERATION_REPLACE:
- req.Replace = diff.Update
+ req.Replace = []*gpb.Update{{
+ Path: path,
+ Val: &gpb.TypedValue{
+ Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: json},
+ },
+ }}
default:
- return &errors.ErrOperationNotSupported{Op: op}
+ return nil, &errors.ErrOperationNotSupported{Op: op}
}
} else if diff.Delete != nil {
req.Delete = diff.Delete
}
- resp, err := g.client.Set(ctx, req)
- log.Info(resp)
- return err
+ return req, nil
}
//Subscribe subscribes to a gNMI target
diff --git a/controller/nucleus/gnmi_transport_test.go b/controller/nucleus/gnmi_transport_test.go
index e7715e7fd..deb6742db 100644
--- a/controller/nucleus/gnmi_transport_test.go
+++ b/controller/nucleus/gnmi_transport_test.go
@@ -7,9 +7,11 @@ import (
"testing"
"code.fbi.h-da.de/danet/gosdn/controller/interfaces/change"
+ "code.fbi.h-da.de/danet/gosdn/controller/nucleus/types"
"code.fbi.h-da.de/danet/gosdn/controller/interfaces/southbound"
+ ppb "code.fbi.h-da.de/danet/gosdn/api/go/gosdn/pnd"
spb "code.fbi.h-da.de/danet/gosdn/api/go/gosdn/southbound"
tpb "code.fbi.h-da.de/danet/gosdn/api/go/gosdn/transport"
@@ -18,6 +20,7 @@ import (
"code.fbi.h-da.de/danet/gosdn/models/generated/openconfig"
gpb "github.com/openconfig/gnmi/proto/gnmi"
"github.com/openconfig/goyang/pkg/yang"
+ "github.com/openconfig/ygot/ygot"
"github.com/openconfig/ygot/ytypes"
"github.com/stretchr/testify/mock"
)
@@ -271,11 +274,21 @@ func TestGnmi_ProcessResponse(t *testing.T) {
}
func TestGnmi_Set(t *testing.T) {
+ schema, err := openconfig.Schema()
+ if err != nil {
+ t.Errorf("Set() error = %v", err)
+ }
+
+ setResponse := &gpb.SetResponse{}
+
type fields struct {
- transport *Gnmi
+ transport Gnmi
+ mockArgumentMatcher any
}
type args struct {
payload change.Payload
+ path string
+ ctx context.Context
}
tests := []struct {
name string
@@ -285,17 +298,88 @@ func TestGnmi_Set(t *testing.T) {
}{
{
name: "uninitialised",
- fields: fields{&Gnmi{}},
+ fields: fields{transport: mockTransport()},
args: args{
payload: change.Payload{},
+ path: "/",
+ ctx: context.WithValue(context.Background(), types.CtxKeyOperation, ppb.ApiOperation_API_OPERATION_UPDATE), // nolint
},
wantErr: true,
},
- // TODO: Positive test cases
+ {
+ name: "updateValue",
+ fields: fields{
+ transport: mockTransport(),
+ mockArgumentMatcher: mock.MatchedBy(func(input *gpb.SetRequest) bool {
+ if len(input.Update) == 0 {
+ return false
+ }
+ test, _ := ygot.PathToString(input.Update[0].Path)
+ return test == "/system/config/hostname"
+ }),
+ },
+ args: args{
+ payload: change.Payload{
+ Original: &openconfig.Device{
+ System: &openconfig.OpenconfigSystem_System{
+ Config: &openconfig.OpenconfigSystem_System_Config{
+ Hostname: ygot.String("oldName"),
+ },
+ },
+ },
+ Modified: &openconfig.Device{
+ System: &openconfig.OpenconfigSystem_System{
+ Config: &openconfig.OpenconfigSystem_System_Config{
+ Hostname: ygot.String("newName"),
+ },
+ },
+ },
+ },
+ path: "/system/config/hostname",
+ ctx: context.WithValue(context.Background(), types.CtxKeyOperation, ppb.ApiOperation_API_OPERATION_UPDATE), // nolint
+ },
+ wantErr: false,
+ },
+ {
+ name: "removeValue",
+ fields: fields{
+ transport: mockTransport(),
+ mockArgumentMatcher: mock.MatchedBy(func(input *gpb.SetRequest) bool {
+ if len(input.Delete) == 0 {
+ return false
+ }
+ test, _ := ygot.PathToString(input.Delete[0])
+ return test == "/system/config/hostname"
+ }),
+ },
+ args: args{
+ payload: change.Payload{
+ Original: &openconfig.Device{
+ System: &openconfig.OpenconfigSystem_System{
+ Config: &openconfig.OpenconfigSystem_System_Config{
+ Hostname: ygot.String("oldName"),
+ },
+ },
+ },
+ Modified: &openconfig.Device{
+ System: &openconfig.OpenconfigSystem_System{
+ Config: &openconfig.OpenconfigSystem_System_Config{
+ Hostname: nil,
+ },
+ },
+ },
+ },
+ path: "/system/config/hostname",
+ ctx: context.WithValue(context.Background(), types.CtxKeyOperation, ppb.ApiOperation_API_OPERATION_DELETE), // nolint
+ },
+ wantErr: false,
+ },
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
- err := tt.fields.transport.Set(context.Background(), tt.args.payload)
+ tt.fields.transport.client.(*mocks.GNMIClient).
+ On("Set", mockContext, tt.fields.mockArgumentMatcher).Return(setResponse, nil)
+ err := tt.fields.transport.Set(tt.args.ctx, tt.args.payload, tt.args.path, schema)
if (err != nil) != tt.wantErr {
t.Errorf("Set() error = %v, wantErr %v", err, tt.wantErr)
}
diff --git a/controller/nucleus/initialise_test.go b/controller/nucleus/initialise_test.go
index ed1d2b16c..958991f27 100644
--- a/controller/nucleus/initialise_test.go
+++ b/controller/nucleus/initialise_test.go
@@ -87,7 +87,7 @@ func targetRunner() {
func mockTransport() Gnmi {
return Gnmi{
- SetNode: nil,
+ SetNode: getMockSbi(defaultSbiID).SetNode,
RespChan: make(chan *gpb.SubscribeResponse),
Options: newGnmiTransportOptions(),
client: &mocks.GNMIClient{},
diff --git a/controller/nucleus/principalNetworkDomain.go b/controller/nucleus/principalNetworkDomain.go
index 3c75a1ae1..ad53c9694 100644
--- a/controller/nucleus/principalNetworkDomain.go
+++ b/controller/nucleus/principalNetworkDomain.go
@@ -472,7 +472,8 @@ func (pnd *pndImplementation) ChangeOND(duid uuid.UUID, operation ppb.ApiOperati
if err != nil {
return uuid.Nil, err
}
- if err := ytypes.SetNode(d.SBI().Schema().RootSchema(), validatedCpy, p, typedValue); err != nil {
+ opts := []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}, &ytypes.TolerateJSONInconsistencies{}}
+ if err := ytypes.SetNode(d.SBI().Schema().RootSchema(), validatedCpy, p, typedValue, opts...); err != nil {
return uuid.Nil, err
}
}
@@ -488,7 +489,9 @@ func (pnd *pndImplementation) ChangeOND(duid uuid.UUID, operation ppb.ApiOperati
callback := func(original ygot.GoStruct, modified ygot.GoStruct) error {
ctx := context.WithValue(context.Background(), types.CtxKeyOperation, operation) // nolint
payload := change.Payload{Original: original, Modified: modified}
- return d.Transport().Set(ctx, payload)
+ pathToSet := path
+ schema := d.SBI().Schema()
+ return d.Transport().Set(ctx, payload, pathToSet, schema)
}
ch := NewChange(duid, d.GetModel(), validatedCpy, callback)
diff --git a/controller/nucleus/principalNetworkDomain_test.go b/controller/nucleus/principalNetworkDomain_test.go
index 907e027ab..ac28816d9 100644
--- a/controller/nucleus/principalNetworkDomain_test.go
+++ b/controller/nucleus/principalNetworkDomain_test.go
@@ -959,7 +959,7 @@ func Test_pndImplementation_Confirm(t *testing.T) {
d := mockDevice()
tr := d.Transport().(*mocks.Transport)
- tr.On("Set", mockContext, mock.Anything, mock.Anything).Return(nil)
+ tr.On("Set", mockContext, mock.Anything, mock.Anything, mock.Anything).Return(nil)
_, err := pnd.addDevice(d)
if err != nil {
t.Error(err)
diff --git a/controller/test/integration/nucleusIntegration_test.go b/controller/test/integration/nucleusIntegration_test.go
index 333efb177..b85058a46 100644
--- a/controller/test/integration/nucleusIntegration_test.go
+++ b/controller/test/integration/nucleusIntegration_test.go
@@ -14,6 +14,7 @@ import (
spb "code.fbi.h-da.de/danet/gosdn/api/go/gosdn/southbound"
tpb "code.fbi.h-da.de/danet/gosdn/api/go/gosdn/transport"
"code.fbi.h-da.de/danet/gosdn/controller/interfaces/change"
+ "code.fbi.h-da.de/danet/gosdn/models/generated/openconfig"
"code.fbi.h-da.de/danet/gosdn/controller/nucleus"
"code.fbi.h-da.de/danet/gosdn/controller/nucleus/errors"
@@ -92,6 +93,11 @@ func testSetupIntegration() {
}
func TestGnmi_SetInvalidIntegration(t *testing.T) {
+ schema, err := openconfig.Schema()
+ if err != nil {
+ t.Errorf("Set ")
+
+ }
if testing.Short() {
t.Skip("skipping integration test")
}
@@ -101,6 +107,7 @@ func TestGnmi_SetInvalidIntegration(t *testing.T) {
type args struct {
ctx context.Context
payload change.Payload
+ path string
}
tests := []struct {
name string
@@ -120,6 +127,7 @@ func TestGnmi_SetInvalidIntegration(t *testing.T) {
args: args{
ctx: context.Background(),
payload: change.Payload{},
+ path: "/",
},
wantErr: true,
},
@@ -129,6 +137,7 @@ func TestGnmi_SetInvalidIntegration(t *testing.T) {
args: args{
ctx: context.Background(),
payload: change.Payload{},
+ path: "/",
},
wantErr: true,
},
@@ -145,7 +154,7 @@ func TestGnmi_SetInvalidIntegration(t *testing.T) {
t.Errorf("SetInvalidIntegration() error = %v, wantErr %v", err, tt.wantErr)
return
}
- err = g.Set(tt.args.ctx, tt.args.payload)
+ err = g.Set(tt.args.ctx, tt.args.payload, tt.args.path, schema)
if (err != nil) != tt.wantErr {
t.Errorf("SetInvalidIntegration() error = %v, wantErr %v", err, tt.wantErr)
return
--
GitLab