Skip to content

Commit

Permalink
Support empty vs nil lists and error for unset attrs (#278)
Browse files Browse the repository at this point in the history 
* Support empty vs nil lists

* test fix

* feedback
  • Loading branch information
@DanG100
DanG100 authored Sep 28, 2023
1 parent 442f947 commit c133d13
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 35 deletions.
1 change: 1 addition & 0 deletions dataplane/standalone/saiserver/attrmgr/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
65 changes: 47 additions & 18 deletions dataplane/standalone/saiserver/attrmgr/attrmgr.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand All @@ -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
}
Expand Down Expand Up @@ -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") {
Expand Down Expand Up @@ -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
}
Expand All @@ -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)
}
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down
36 changes: 21 additions & 15 deletions dataplane/standalone/saiserver/attrmgr/attrmgr_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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),
},
Expand All @@ -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),
},
Expand All @@ -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),
},
Expand All @@ -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,
Expand All @@ -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{
Expand Down Expand Up @@ -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",
Expand All @@ -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,
},
},
}}
Expand Down
13 changes: 11 additions & 2 deletions dataplane/standalone/saiserver/ports_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
},
}}
Expand All @@ -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 != "" {
Expand Down Expand Up @@ -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 {
Expand All @@ -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 != "" {
Expand Down

0 comments on commit c133d13

Please sign in to comment.