diff --git a/dataplane/standalone/saiserver/attrmgr/BUILD b/dataplane/standalone/saiserver/attrmgr/BUILD index 71c1fd24..5f2b3727 100644 --- a/dataplane/standalone/saiserver/attrmgr/BUILD +++ b/dataplane/standalone/saiserver/attrmgr/BUILD @@ -7,6 +7,7 @@ go_library( visibility = ["//visibility:public"], deps = [ "//dataplane/standalone/proto:sai", + "@com_github_golang_glog//:glog", "@org_golang_google_grpc//:go_default_library", "@org_golang_google_grpc//codes", "@org_golang_google_grpc//status", diff --git a/dataplane/standalone/saiserver/attrmgr/attrmgr.go b/dataplane/standalone/saiserver/attrmgr/attrmgr.go index e869dd72..83d1647c 100644 --- a/dataplane/standalone/saiserver/attrmgr/attrmgr.go +++ b/dataplane/standalone/saiserver/attrmgr/attrmgr.go @@ -25,6 +25,7 @@ import ( "sync" "sync/atomic" + log "github.com/golang/glog" "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -40,7 +41,7 @@ import ( type AttrMgr struct { mu sync.Mutex // attrs is a map of object id (string) to a map of attributes (key: attr id, some enum value). - attrs map[string]map[int32]protoreflect.Value + attrs map[string]map[int32]*protoreflect.Value nextOid atomic.Uint64 // idToType maps an object id to its SAI type. idToType map[string]saipb.ObjectType @@ -52,16 +53,16 @@ type AttrMgr struct { // New returns a new AttrMgr. func New() *AttrMgr { mgr := &AttrMgr{ - attrs: make(map[string]map[int32]protoreflect.Value), + attrs: make(map[string]map[int32]*protoreflect.Value), idToType: make(map[string]saipb.ObjectType), msgEnumToFieldNum: make(map[string]map[int32]int), } return mgr } -func (mgr *AttrMgr) set(id string, attr int32, val protoreflect.Value) { +func (mgr *AttrMgr) set(id string, attr int32, val *protoreflect.Value) { if _, ok := mgr.attrs[id]; !ok { - mgr.attrs[id] = make(map[int32]protoreflect.Value) + mgr.attrs[id] = make(map[int32]*protoreflect.Value) } mgr.attrs[id][attr] = val } @@ -108,7 +109,8 @@ func (mgr *AttrMgr) Interceptor(ctx context.Context, req any, info *grpc.UnarySe if strings.Contains(info.FullMethod, "Create") || strings.Contains(info.FullMethod, "Set") { id, err := mgr.getID(reqMsg, respMsg) if err != nil { - return nil, err + log.Warningf("failed to get id %v", err) + return respMsg, nil } mgr.storeAttributes(id, reqMsg) } else if strings.Contains(info.FullMethod, "Get") { @@ -173,9 +175,12 @@ func (mgr *AttrMgr) PopulateAttributes(req, resp proto.Message) error { enumVal := reqList.Get(i).Enum() val, ok := mgr.attrs[id][int32(enumVal)] if !ok { - continue + return fmt.Errorf("requested attribute not set: %v", reqList.Get(i)) + } + // Empty lists exist so they are not errors, but are not settable. + if val != nil { + attrs.Set(attrs.Descriptor().Fields().Get(enumToFields[int32(enumVal)]), *val) } - attrs.Set(attrs.Descriptor().Fields().Get(enumToFields[int32(enumVal)]), val) } return nil } @@ -197,16 +202,17 @@ func (mgr *AttrMgr) PopulateAllAttributes(id string, msg proto.Message) error { continue } val, ok := mgr.attrs[id][enumVal] - if !ok { + if !ok || val == nil { continue } - msg.ProtoReflect().Set(desc.Fields().Get(i), val) + msg.ProtoReflect().Set(desc.Fields().Get(i), *val) } - return nil } -// storeAttributes stores all the attributes in the message. +// StoreAttributes stores all the attributes in the message. +// Note: for lists, a nil lists is an unset attribute, but a non-nil empty list is set. +// so querying a nil list returns an error, even though they look the same on the wire. func (mgr *AttrMgr) StoreAttributes(id uint64, msg proto.Message) { mgr.storeAttributes(fmt.Sprint(id), msg) } @@ -235,17 +241,36 @@ func (mgr *AttrMgr) storeAttributes(id string, msg proto.Message) { mgr.SetType(id, ty) } - msg.ProtoReflect().Range(func(fd protoreflect.FieldDescriptor, v protoreflect.Value) bool { + // Protoreflect treats nil lists and empty lists as the same. However We want to store the value of empty lists, but not nil lists. + // So use regular go reflect for that case. + rv := reflect.ValueOf(msg).Elem() + rt := reflect.TypeOf(msg).Elem() + for i := 0; i < rt.NumField(); i++ { + tag := rt.Field(i).Tag.Get("protobuf") + if tag == "" { + continue + } + var fName string + for _, v := range strings.Split(tag, ",") { + if strings.HasPrefix(v, "name=") { + fName = strings.TrimPrefix(v, "name=") + } + } + fd := msg.ProtoReflect().Descriptor().Fields().ByTextName(fName) opt, ok := fd.Options().(*descriptorpb.FieldOptions) if !ok { - return true + continue } enumValue := proto.GetExtension(opt, saipb.E_AttrEnumValue).(int32) - if enumValue != 0 { - mgr.set(id, enumValue, v) + if enumValue == 0 { + continue } - return true - }) + if v := msg.ProtoReflect().Get(fd); msg.ProtoReflect().Has(fd) { + mgr.set(id, enumValue, &v) + } else if fd.IsList() && !rv.Field(i).IsNil() { + mgr.set(id, enumValue, nil) + } + } } // NextID returns the next available object id. @@ -267,7 +292,11 @@ func (mgr *AttrMgr) getID(req, resp proto.Message) (string, error) { return fmt.Sprint(v), nil } } - entry := req.ProtoReflect().Get(req.ProtoReflect().Descriptor().Fields().ByTextName("entry")).Message().Interface() + fd := req.ProtoReflect().Descriptor().Fields().ByTextName("entry") + if fd == nil { + return "", fmt.Errorf("no id found in message") + } + entry := req.ProtoReflect().Get(fd).Message().Interface() pBytes, err := proto.Marshal(entry) if err != nil { return "", err diff --git a/dataplane/standalone/saiserver/attrmgr/attrmgr_test.go b/dataplane/standalone/saiserver/attrmgr/attrmgr_test.go index 38cbeb60..1c9bb11a 100644 --- a/dataplane/standalone/saiserver/attrmgr/attrmgr_test.go +++ b/dataplane/standalone/saiserver/attrmgr/attrmgr_test.go @@ -31,11 +31,15 @@ import ( saipb "github.com/openconfig/lemming/dataplane/standalone/proto" ) +func ptrToValue(v protoreflect.Value) *protoreflect.Value { + return &v +} + func TestInterceptor(t *testing.T) { tests := []struct { desc string req any - attrs map[string]map[int32]protoreflect.Value + attrs map[string]map[int32]*protoreflect.Value handlerResp any handlerErr error info *grpc.UnaryServerInfo @@ -44,17 +48,17 @@ func TestInterceptor(t *testing.T) { }{{ desc: "not sai", info: &grpc.UnaryServerInfo{FullMethod: "foo"}, - attrs: map[string]map[int32]protoreflect.Value{}, + attrs: map[string]map[int32]*protoreflect.Value{}, }, { desc: "handler error", info: &grpc.UnaryServerInfo{FullMethod: "/lemming.dataplane.sai.Switch/CreateSwitch"}, - attrs: map[string]map[int32]protoreflect.Value{}, + attrs: map[string]map[int32]*protoreflect.Value{}, handlerErr: fmt.Errorf("foo"), wantErr: "foo", }, { desc: "create request", info: &grpc.UnaryServerInfo{FullMethod: "/lemming.dataplane.sai.Switch/CreateSwitch"}, - attrs: map[string]map[int32]protoreflect.Value{}, + attrs: map[string]map[int32]*protoreflect.Value{}, req: &saipb.CreateSwitchRequest{ RestartWarm: proto.Bool(true), }, @@ -65,7 +69,7 @@ func TestInterceptor(t *testing.T) { }, { desc: "create request unimplemented", info: &grpc.UnaryServerInfo{FullMethod: "/lemming.dataplane.sai.Switch/CreateSwitch"}, - attrs: map[string]map[int32]protoreflect.Value{}, + attrs: map[string]map[int32]*protoreflect.Value{}, req: &saipb.CreateSwitchRequest{ RestartWarm: proto.Bool(true), }, @@ -76,7 +80,7 @@ func TestInterceptor(t *testing.T) { }, { desc: "create request unimplemented typed nil", info: &grpc.UnaryServerInfo{FullMethod: "/lemming.dataplane.sai.Switch/CreateSwitch"}, - attrs: map[string]map[int32]protoreflect.Value{}, + attrs: map[string]map[int32]*protoreflect.Value{}, req: &saipb.CreateSwitchRequest{ RestartWarm: proto.Bool(true), }, @@ -88,7 +92,7 @@ func TestInterceptor(t *testing.T) { }, { desc: "create request entry", info: &grpc.UnaryServerInfo{FullMethod: "/lemming.dataplane.sai.Route/CreateRoute"}, - attrs: map[string]map[int32]protoreflect.Value{}, + attrs: map[string]map[int32]*protoreflect.Value{}, req: &saipb.CreateRouteEntryRequest{ Entry: &saipb.RouteEntry{ SwitchId: 12, @@ -99,10 +103,10 @@ func TestInterceptor(t *testing.T) { }, { desc: "get request", info: &grpc.UnaryServerInfo{FullMethod: "/lemming.dataplane.sai.Switch/GetSwitchAttribute"}, - attrs: map[string]map[int32]protoreflect.Value{ + attrs: map[string]map[int32]*protoreflect.Value{ "10": { - int32(saipb.SwitchAttr_SWITCH_ATTR_CPU_PORT): protoreflect.ValueOfUint64(100), - int32(saipb.SwitchAttr_SWITCH_ATTR_PRE_INGRESS_ACL): protoreflect.ValueOfUint64(300), + int32(saipb.SwitchAttr_SWITCH_ATTR_CPU_PORT): ptrToValue(protoreflect.ValueOfUint64(100)), + int32(saipb.SwitchAttr_SWITCH_ATTR_PRE_INGRESS_ACL): ptrToValue(protoreflect.ValueOfUint64(300)), }, }, req: &saipb.GetSwitchAttributeRequest{ @@ -144,7 +148,7 @@ func TestInvokeAndSave(t *testing.T) { desc string req proto.Message rpc func(context.Context, proto.Message) (proto.Message, error) - wantAttrs map[string]map[int32]protoreflect.Value + wantAttrs map[string]map[int32]*protoreflect.Value wantErr string }{{ desc: "rpc error", @@ -158,12 +162,14 @@ func TestInvokeAndSave(t *testing.T) { return nil, nil }, req: &saipb.SetPortAttributeRequest{ - Oid: 1, - AdminState: proto.Bool(true), + Oid: 1, + AdminState: proto.Bool(true), + AdvertisedSpeed: []uint32{}, }, - wantAttrs: map[string]map[int32]protoreflect.Value{ + wantAttrs: map[string]map[int32]*protoreflect.Value{ "1": { - int32(saipb.PortAttr_PORT_ATTR_ADMIN_STATE): protoreflect.ValueOfBool(true), + int32(saipb.PortAttr_PORT_ATTR_ADMIN_STATE): ptrToValue(protoreflect.ValueOfBool(true)), + int32(saipb.PortAttr_PORT_ATTR_ADVERTISED_SPEED): nil, }, }, }} diff --git a/dataplane/standalone/saiserver/ports_test.go b/dataplane/standalone/saiserver/ports_test.go index 0459094c..0fcadf8c 100644 --- a/dataplane/standalone/saiserver/ports_test.go +++ b/dataplane/standalone/saiserver/ports_test.go @@ -123,6 +123,7 @@ func TestSetPortAttribute(t *testing.T) { ContextId: &fwdpb.ContextId{}, }, wantAttr: &saipb.PortAttribute{ + OperStatus: saipb.PortOperStatus_PORT_OPER_STATUS_DOWN.Enum(), AdminState: proto.Bool(false), }, }} @@ -133,6 +134,9 @@ func TestSetPortAttribute(t *testing.T) { } dplane := &fakePortDataplaneAPI{} c, mgr, stopFn := newTestPort(t, dplane) + mgr.StoreAttributes(1, &saipb.PortAttribute{ + OperStatus: saipb.PortOperStatus_PORT_OPER_STATUS_DOWN.Enum(), + }) defer stopFn() _, gotErr := c.SetPortAttribute(context.TODO(), tt.req) if diff := errdiff.Check(gotErr, tt.wantErr); diff != "" { @@ -173,13 +177,15 @@ func TestCreateHostif(t *testing.T) { }, { desc: "success netdev", req: &saipb.CreateHostifRequest{ - Type: saipb.HostifType_HOSTIF_TYPE_NETDEV.Enum(), + Type: saipb.HostifType_HOSTIF_TYPE_NETDEV.Enum(), + ObjId: proto.Uint64(2), }, want: &saipb.CreateHostifResponse{ Oid: 1, }, wantAttr: &saipb.HostifAttribute{ - Type: saipb.HostifType_HOSTIF_TYPE_NETDEV.Enum(), + Type: saipb.HostifType_HOSTIF_TYPE_NETDEV.Enum(), + ObjId: proto.Uint64(2), }, }} for _, tt := range tests { @@ -189,6 +195,9 @@ func TestCreateHostif(t *testing.T) { } dplane := &fakePortDataplaneAPI{} c, mgr, stopFn := newTestHostif(t, dplane) + mgr.StoreAttributes(2, &saipb.PortAttribute{ + OperStatus: saipb.PortOperStatus_PORT_OPER_STATUS_DOWN.Enum(), + }) defer stopFn() got, gotErr := c.CreateHostif(context.TODO(), tt.req) if diff := errdiff.Check(gotErr, tt.wantErr); diff != "" {