From 1a0059cb36c25221c3319daa8f31610e416560bd Mon Sep 17 00:00:00 2001
From: Malte Bauch <malte.bauch@stud.h-da.de>
Date: Tue, 29 Mar 2022 10:04:01 +0000
Subject: [PATCH] Resolve "Requesting a specific config path of an OND returns
always the whole config"
See merge request danet/gosdn!266
---
controller/northbound/server/pnd.go | 102 ++++++++++++++++-
controller/northbound/server/pnd_test.go | 134 ++++++++++++++++++++++-
2 files changed, 231 insertions(+), 5 deletions(-)
diff --git a/controller/northbound/server/pnd.go b/controller/northbound/server/pnd.go
index 10dad37f4..df50fd617 100644
--- a/controller/northbound/server/pnd.go
+++ b/controller/northbound/server/pnd.go
@@ -12,7 +12,9 @@ import (
"code.fbi.h-da.de/danet/gosdn/controller/nucleus"
"code.fbi.h-da.de/danet/gosdn/controller/nucleus/errors"
"github.com/google/uuid"
+ "github.com/openconfig/gnmi/proto/gnmi"
"github.com/openconfig/ygot/ygot"
+ "github.com/openconfig/ygot/ytypes"
"github.com/prometheus/client_golang/prometheus"
log "github.com/sirupsen/logrus"
"google.golang.org/grpc/codes"
@@ -139,6 +141,100 @@ func fillOnds(pnd networkdomain.NetworkDomain, all bool, did ...string) ([]*ppb.
return onds, nil
}
+func fillOndBySpecificPath(pnd networkdomain.NetworkDomain, did string, path string) (*ppb.OrchestratedNetworkingDevice, error) {
+ d, err := pnd.GetDevice(did)
+ if err != nil {
+ log.Error(err)
+ return nil, status.Errorf(codes.Aborted, "%v", err)
+ }
+
+ gnmiPath, err := ygot.StringToPath(path, ygot.StructuredPath)
+ if err != nil {
+ log.Error(err)
+ return nil, status.Errorf(codes.Aborted, "%v", err)
+ }
+
+ opts := []ytypes.GetNodeOpt{&ytypes.GetHandleWildcards{}}
+ nodes, err := ytypes.GetNode(d.SBI().Schema().RootSchema(), d.Model(), gnmiPath, opts...)
+ if err != nil {
+ log.Error(err)
+ return nil, status.Errorf(codes.Aborted, "%v", err)
+ }
+
+ // we should be fine to access nodes[0] here, since ytypes.GetNode() throws
+ // an error if there are no matches for the path.
+ nodeEntry := nodes[0].Schema
+ node := nodes[0].Data
+
+ var dev []*gnmi.Notification
+ if nodeEntry.IsDir() {
+ validatedNode, ok := nodes[0].Data.(ygot.ValidatedGoStruct)
+ if !ok {
+ return nil, status.Errorf(codes.Aborted, "%v", errors.ErrInvalidTypeAssertion{
+ Value: node,
+ Type: (ygot.ValidatedGoStruct)(nil),
+ })
+ }
+ cfg := ygot.GNMINotificationsConfig{}
+ dev, err = ygot.TogNMINotifications(validatedNode, time.Now().UnixNano(), cfg)
+ if err != nil {
+ log.Error(err)
+ return nil, status.Errorf(codes.Aborted, "%v", err)
+ }
+ } else if nodeEntry.IsLeaf() {
+ dev, err = genGnmiNotificationForLeaf(gnmiPath, node)
+ if err != nil {
+ log.Error(err)
+ return nil, status.Errorf(codes.Aborted, "%v", err)
+ }
+ }
+
+ sbi := spb.SouthboundInterface{}
+ if d.SBI() != nil {
+ sbi.Id = d.SBI().ID().String()
+ sbi.Type = d.SBI().Type()
+ }
+
+ ond := &ppb.OrchestratedNetworkingDevice{
+ Id: d.ID().String(),
+ Name: d.Name(),
+ Device: dev,
+ Sbi: &sbi,
+ }
+
+ return ond, nil
+}
+
+func genGnmiNotificationForLeaf(path *gnmi.Path, val any) ([]*gnmi.Notification, error) {
+ typedVal, err := ygot.EncodeTypedValue(val, gnmi.Encoding_PROTO)
+ if err != nil {
+ return nil, err
+ }
+ pathElem := path.GetElem()[len(path.GetElem())-1].GetName()
+ ppath := []string{pathElem}
+ return []*gnmi.Notification{
+ {
+ Timestamp: time.Now().UnixNano(),
+ Update: []*gnmi.Update{
+ {
+ Path: &gnmi.Path{
+ // NOTE: This is dirty and should be changed as soon as
+ // ygot.TogNMINotifications does not return
+ // notifications with `Element`!
+ // `Element` is marked as deprecated, but since
+ // ygot.TogNMINotifications returns `Element`, we will
+ // keep using it for now and to maintain consistency.
+ // Only returning the last element here, since we
+ // request a path node.
+ Element: ppath,
+ },
+ Val: typedVal,
+ },
+ },
+ },
+ }, nil
+}
+
func (p pndServer) GetSbi(ctx context.Context, request *ppb.GetSbiRequest) (*ppb.GetSbiResponse, error) {
labels := prometheus.Labels{"service": "pnd", "rpc": "get"}
start := metrics.StartHook(labels, grpcRequestsTotal)
@@ -276,11 +372,13 @@ func (p pndServer) GetPath(ctx context.Context, request *ppb.GetPathRequest) (*p
log.Error(err)
return nil, status.Errorf(codes.Aborted, "%v", err)
}
- ond, err := fillOnds(pnd, false, request.Did)
+
+ ond, err := fillOndBySpecificPath(pnd, request.Did, path)
if err != nil {
log.Error(err)
return nil, status.Errorf(codes.Aborted, "%v", err)
}
+
return &ppb.GetPathResponse{
Timestamp: time.Now().UnixNano(),
Pnd: &ppb.PrincipalNetworkDomain{
@@ -288,7 +386,7 @@ func (p pndServer) GetPath(ctx context.Context, request *ppb.GetPathRequest) (*p
Name: pnd.GetName(),
Description: pnd.GetDescription(),
},
- Device: ond[0].Device,
+ Device: ond.Device,
}, nil
}
diff --git a/controller/northbound/server/pnd_test.go b/controller/northbound/server/pnd_test.go
index 11e8e1259..3bd0af7e1 100644
--- a/controller/northbound/server/pnd_test.go
+++ b/controller/northbound/server/pnd_test.go
@@ -17,7 +17,10 @@ import (
"code.fbi.h-da.de/danet/gosdn/controller/nucleus"
"code.fbi.h-da.de/danet/gosdn/controller/store"
"code.fbi.h-da.de/danet/yang-models/generated/openconfig"
+ "github.com/golang/protobuf/proto"
+ "github.com/google/go-cmp/cmp"
"github.com/google/uuid"
+ "github.com/openconfig/gnmi/proto/gnmi"
log "github.com/sirupsen/logrus"
"github.com/stretchr/testify/mock"
"google.golang.org/grpc"
@@ -33,6 +36,7 @@ const sbiID = "f6fd4b35-f039-4111-9156-5e4501bb8a5a"
const ondID = "7e0ed8cc-ebf5-46fa-9794-741494914883"
var hostname = "manfred"
+var domainname = "uwe"
var pndUUID uuid.UUID
var sbiUUID uuid.UUID
var pendingChangeUUID uuid.UUID
@@ -122,7 +126,8 @@ func TestMain(m *testing.M) {
GoStruct: &openconfig.Device{
System: &openconfig.OpenconfigSystem_System{
Config: &openconfig.OpenconfigSystem_System_Config{
- Hostname: &hostname,
+ Hostname: &hostname,
+ DomainName: &domainname,
},
},
},
@@ -159,11 +164,16 @@ func TestMain(m *testing.M) {
mockPnd.On("Commit", mock.Anything).Return(nil)
mockPnd.On("Confirm", mock.Anything).Return(nil)
mockPnd.On("ChangeOND", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(uuid.Nil, nil)
+ mockPnd.On("Request", mock.Anything, mock.Anything).Return(nil, nil)
- newPnd := getMockPND()
+ //newPnd := getMockPND()
pndc = store.NewPndStore()
- if err := pndc.Add(newPnd); err != nil {
+ //if err := pndc.Add(newPnd); err != nil {
+ // log.Fatal(err)
+ //}
+
+ if err := pndc.Add(mockPnd); err != nil {
log.Fatal(err)
}
@@ -224,6 +234,124 @@ func Test_pnd_Get(t *testing.T) {
}
}
+func Test_pnd_GetPath(t *testing.T) {
+ removeExistingStores()
+ // options to apply to cmp.Equal
+ type args struct {
+ ctx context.Context
+ request *ppb.GetPathRequest
+ }
+ tests := []struct {
+ name string
+ args args
+ want []*gnmi.Notification
+ wantErr bool
+ }{
+ {
+ name: "get path: system/config/hostname",
+ args: args{
+ ctx: context.Background(),
+ request: &ppb.GetPathRequest{
+ Timestamp: 1648488947324854250,
+ Did: mockDevice.ID().String(),
+ Path: "system/config/hostname",
+ Pid: mockPnd.ID().String(),
+ },
+ },
+ want: []*gnmi.Notification{
+ {
+ Update: []*gnmi.Update{
+ {
+ Path: &gnmi.Path{Element: []string{"hostname"}},
+ Val: &gnmi.TypedValue{
+ Value: &gnmi.TypedValue_StringVal{
+ StringVal: "manfred",
+ },
+ },
+ },
+ }},
+ },
+ wantErr: false,
+ },
+ {
+ name: "get path: system",
+ args: args{
+ ctx: context.Background(),
+ request: &ppb.GetPathRequest{
+ Timestamp: 1648488947324854250,
+ Did: mockDevice.ID().String(),
+ Path: "system",
+ Pid: mockPnd.ID().String(),
+ },
+ },
+ want: []*gnmi.Notification{
+ {
+ Update: []*gnmi.Update{
+ {
+ Path: &gnmi.Path{Element: []string{"config", "domain-name"}},
+ Val: &gnmi.TypedValue{
+ Value: &gnmi.TypedValue_StringVal{
+ StringVal: "uwe",
+ },
+ },
+ },
+ {
+ Path: &gnmi.Path{Element: []string{"config", "hostname"}},
+ Val: &gnmi.TypedValue{
+ Value: &gnmi.TypedValue_StringVal{
+ StringVal: "manfred",
+ },
+ },
+ },
+ }},
+ },
+ wantErr: false,
+ },
+ {
+ name: "get path: this/path/is/not/valid",
+ args: args{
+ ctx: context.Background(),
+ request: &ppb.GetPathRequest{
+ Timestamp: 1648488947324854250,
+ Did: mockDevice.ID().String(),
+ Path: "this/path/is/not/valid",
+ Pid: mockPnd.ID().String(),
+ },
+ },
+ want: []*gnmi.Notification{},
+ wantErr: true,
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ s := pndServer{
+ UnimplementedPndServiceServer: ppb.UnimplementedPndServiceServer{},
+ }
+ resp, err := s.GetPath(tt.args.ctx, tt.args.request)
+ if (err != nil) != tt.wantErr {
+ t.Errorf("GetPath() error = %v, wantErr %v", err, tt.wantErr)
+ return
+ }
+
+ got := resp.GetDevice()
+
+ // with the current implementation we should only get a
+ // notification set with length one. For the case that we do not
+ // get anything this makes sure we do not trigger a panic.
+ if len(got) >= 1 {
+ // set timestamp for equality; not the prettiest solution
+ tt.want[0].Timestamp = got[0].Timestamp
+ }
+
+ for i, n := range got {
+ if diff := cmp.Diff(n, tt.want[i], cmp.Comparer(proto.Equal)); diff != "" {
+ t.Errorf("GetPath() diff in the received notification %d: \n%s", i+1, diff)
+ }
+ }
+ })
+ }
+}
+
func Test_pnd_Set(t *testing.T) {
removeExistingStores()
--
GitLab