fix: addressed MR feedback

Author: spydaNVIDIA

docker/production/Dockerfile.carbide-nsm

@@ -39,6 +39,7 @@ COPY sdk/standard/go.mod ./sdk/standard/
 # Download dependencies
 RUN go mod download
 
+COPY common/ ./common/
 COPY nvswitch-manager/ ./nvswitch-manager/
 
 WORKDIR /workspace/nvswitch-manager

go.mod

@@ -37,6 +37,7 @@ require (
 	github.com/gogo/status v1.1.1
 	github.com/golang-jwt/jwt/v5 v5.3.0
 	github.com/golang/mock v1.6.0
+	github.com/golang/protobuf v1.5.4
 	github.com/google/uuid v1.6.0
 	github.com/gorilla/mux v1.8.1
 	github.com/grpc-ecosystem/go-grpc-middleware v1.4.0
@@ -150,7 +151,6 @@ require (
 	github.com/go-viper/mapstructure/v2 v2.4.0 // indirect
 	github.com/gogo/googleapis v0.0.0-20180223154316-0cd9801be74a // indirect
 	github.com/gogo/protobuf v1.3.2 // indirect
-	github.com/golang/protobuf v1.5.4 // indirect
 	github.com/golang/snappy v0.0.4 // indirect
 	github.com/google/gnostic-models v0.7.0 // indirect
 	github.com/google/go-cmp v0.7.0 // indirect

nvswitch-manager/internal/proto/v1/nvswitch-manager.pb.go

@@ -278,25 +278,45 @@ message QueueUpdateResponse {
 
 // QueueUpdatesRequest queues firmware updates for multiple switches.
 // Registered switches are identified by UUID; unregistered devices use FirmwareTarget.
+//
+// `switch_uuids` and `targets` are mutually exclusive: a single request must
+// populate exactly one of them. The handler does not deduplicate across the
+// two lists, so allowing both would risk queuing duplicate updates for any
+// device whose UUID and FirmwareTarget are both supplied. Callers with mixed
+// inventory should issue one RPC per registration state. A request that
+// populates neither is rejected for the same reason it would be a no-op.
+//
+// Whichever list is populated is processed best-effort, per item: a failure
+// for one switch (invalid UUID, validation, auto-registration, queue
+// rejection) does not abort processing of the remaining items and does not
+// produce a non-OK gRPC status. Callers must inspect each
+// `QueueUpdatesResponse.results[i].status` to determine per-item outcome.
+// The RPC only returns a top-level error on global failures (firmware
+// manager uninitialized, malformed `components` enum value, or violation of
+// the mutual-exclusivity contract above).
 message QueueUpdatesRequest {
-    repeated string switch_uuids = 1;          // UUIDs of registered NV-Switches to update
+    repeated string switch_uuids = 1;          // UUIDs of registered NV-Switches to update; mutually exclusive with `targets`. Processed best-effort.
     string bundle_version = 2;                 // Version of the firmware bundle
     repeated NVSwitchComponent components = 3; // Components to update (empty = all)
-    repeated FirmwareTarget targets = 4;       // Unregistered switches (auto-registered before update)
+    repeated FirmwareTarget targets = 4;       // Unregistered switches (auto-registered before update); mutually exclusive with `switch_uuids`. Processed best-effort.
 }
 
-// QueueUpdatesResponse returns the results for each switch.
+// QueueUpdatesResponse returns the results for each switch. Length and
+// ordering correspond to whichever of `switch_uuids` or `targets` was
+// populated in the originating request (the two are mutually exclusive).
+// Each entry reports its own outcome; see QueueUpdatesRequest for the
+// best-effort contract.
 message QueueUpdatesResponse {
     repeated QueueUpdateResult results = 1;
 }
 
 // QueueUpdateResult contains the result for a single switch update.
 message QueueUpdateResult {
-    string switch_uuid = 1;               // UUID of the switch
-    StatusCode status = 2;                // SUCCESS or error
-    string error = 3;                     // Error message if status != SUCCESS
+    string switch_uuid = 1;                  // UUID of the switch
+    StatusCode status = 2;                   // SUCCESS or error; per-item outcome under QueueUpdatesRequest's best-effort contract
+    string error = 3;                        // Error message if status != SUCCESS
     repeated FirmwareUpdateInfo updates = 4; // Queued updates if successful
-    string bmc_ip = 5;                    // Set for FirmwareTarget responses; empty for UUID-based
+    string bmc_ip = 5;                       // Set for FirmwareTarget responses; empty for UUID-based
 }
 
 // GetUpdateRequest gets a specific update by ID.

nvswitch-manager/internal/proto/v1/nvswitch-manager.proto

@@ -422,6 +422,22 @@ func (s *NVSwitchManagerServerImpl) QueueUpdates(ctx context.Context, req *pb.Qu
 		return nil, status.Error(codes.Unavailable, "firmware manager not initialized")
 	}
 
+	// switch_uuids and targets are mutually exclusive: a request must populate
+	// exactly one. Allowing both would risk queuing duplicate updates for any
+	// device whose UUID is in switch_uuids and whose FirmwareTarget is in
+	// targets, since the handler does not deduplicate across the two lists.
+	// Empty requests are rejected as well — almost certainly a client bug.
+	hasUUIDs := len(req.SwitchUuids) > 0
+	hasTargets := len(req.Targets) > 0
+	switch {
+	case !hasUUIDs && !hasTargets:
+		return nil, status.Error(codes.InvalidArgument,
+			"request must populate either switch_uuids or targets")
+	case hasUUIDs && hasTargets:
+		return nil, status.Error(codes.InvalidArgument,
+			"request must populate only one of switch_uuids or targets, not both")
+	}
+
 	// Convert proto components to domain components
 	var components []nvswitch.Component
 	for _, c := range req.Components {
@@ -454,6 +470,14 @@ func (s *NVSwitchManagerServerImpl) QueueUpdates(ctx context.Context, req *pb.Qu
 
 	// Direct path: auto-register unregistered targets, then queue updates
 	for _, target := range req.Targets {
+		if target == nil {
+			results = append(results, &pb.QueueUpdateResult{
+				Status: pb.StatusCode_INVALID_ARGUMENT,
+				Error:  "target is nil",
+			})
+			continue
+		}
+
 		result := &pb.QueueUpdateResult{
 			BmcIp: target.BmcIp,
 		}

nvswitch-manager/internal/service/server_impl.go

@@ -22,10 +22,13 @@ import (
 	"testing"
 
 	pb "github.com/NVIDIA/ncx-infra-controller-rest/nvswitch-manager/internal/proto/v1"
+	"github.com/NVIDIA/ncx-infra-controller-rest/nvswitch-manager/pkg/firmwaremanager"
 	"github.com/NVIDIA/ncx-infra-controller-rest/nvswitch-manager/pkg/redfish"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 func TestResetTarget_InvalidIP(t *testing.T) {
@@ -206,18 +209,63 @@ func TestFirmwareTargetToRegisterRequest(t *testing.T) {
 	assert.Equal(t, int32(22), req.Nvos.Port)
 }
 
-func TestQueueUpdates_InvalidTarget(t *testing.T) {
+// TestQueueUpdates_NilFirmwareManager guards the early-return in QueueUpdates
+// that protects against being called before the firmware manager has been
+// initialized (e.g. when persistent mode is configured but the firmware
+// packages directory is unset). Per-target validation is exercised by the
+// TestValidateFirmwareTarget_* tests; driving end-to-end validation through
+// the handler would require a non-nil fwm and is left for handler-level
+// tests with a mocked FirmwareManager.
+func TestQueueUpdates_NilFirmwareManager(t *testing.T) {
 	srv := &NVSwitchManagerServerImpl{}
-	// fwm is nil, but the target validation should fail before reaching fwm
-	// We can't test the full flow without a fwm, but we can test validation errors
-	// by checking that the handler returns results (not a gRPC error) for invalid targets.
 
-	// Without fwm, QueueUpdates returns "firmware manager not initialized"
-	// So we only test validateFirmwareTarget directly here.
-	target := validFirmwareTarget()
-	target.BmcIp = "invalid"
-	err := validateFirmwareTarget(target)
-	assert.Error(t, err)
+	resp, err := srv.QueueUpdates(context.Background(), &pb.QueueUpdatesRequest{})
+
+	require.Error(t, err)
+	assert.Nil(t, resp)
+	st, ok := status.FromError(err)
+	require.True(t, ok, "expected a gRPC status error, got %T", err)
+	assert.Equal(t, codes.Unavailable, st.Code())
+	assert.Contains(t, st.Message(), "firmware manager not initialized")
+}
+
+// TestQueueUpdates_MutualExclusivity locks in the contract that
+// QueueUpdates rejects requests violating the switch_uuids/targets
+// mutual-exclusivity constraint. See QueueUpdatesRequest in
+// nvswitch-manager.proto for the full contract documentation.
+func TestQueueUpdates_MutualExclusivity(t *testing.T) {
+	cases := map[string]struct {
+		req         *pb.QueueUpdatesRequest
+		wantMessage string
+	}{
+		"empty request rejected (no work to do)": {
+			req:         &pb.QueueUpdatesRequest{},
+			wantMessage: "either switch_uuids or targets",
+		},
+		"both populated rejected (would risk duplicate updates)": {
+			req: &pb.QueueUpdatesRequest{
+				SwitchUuids: []string{"00000000-0000-0000-0000-000000000001"},
+				Targets:     []*pb.FirmwareTarget{validFirmwareTarget()},
+			},
+			wantMessage: "not both",
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			// Non-nil fwm so we get past the Unavailable guard. The
+			// mutual-exclusivity validation rejects before any fwm method
+			// is invoked, so a zero-value pointer is sufficient.
+			srv := &NVSwitchManagerServerImpl{fwm: &firmwaremanager.FirmwareManager{}}
+
+			resp, err := srv.QueueUpdates(context.Background(), tc.req)
 
-	_ = srv // suppress unused
+			require.Error(t, err)
+			assert.Nil(t, resp)
+			st, ok := status.FromError(err)
+			require.True(t, ok, "expected a gRPC status error, got %T", err)
+			assert.Equal(t, codes.InvalidArgument, st.Code())
+			assert.Contains(t, st.Message(), tc.wantMessage)
+		})
+	}
 }

nvswitch-manager/internal/service/server_impl_test.go

@@ -56,7 +56,10 @@ type Service struct {
 func New(ctx context.Context, c Config) (*Service, error) {
 	// Connect to database first if configured (needed for persistent registry)
 	var db *bun.DB
-	if c.DataStoreType == nvswitchmanager.DatastoreTypePersistent && c.DBConf.Host != "" {
+	if c.DataStoreType == nvswitchmanager.DatastoreTypePersistent {
+		if c.DBConf.Host == "" {
+			return nil, fmt.Errorf("DB host is required for persistent mode")
+		}
 		pg, err := postgres.New(ctx, c.DBConf)
 		if err != nil {
 			return nil, fmt.Errorf("database connection required for persistent mode: %w", err)

nvswitch-manager/internal/service/service.go

@@ -20,6 +20,7 @@ package credentials
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net"
 	"sync"
 
@@ -86,6 +87,10 @@ func (m *InMemoryCredentialManager) GetBMC(ctx context.Context, mac net.Hardware
 // PutBMC stores the BMC credential for mac. If an identical entry exists, this is a no-op.
 // If a different entry exists, the new value overwrites (with a warning log).
 func (m *InMemoryCredentialManager) PutBMC(ctx context.Context, mac net.HardwareAddr, cred *credential.Credential) error {
+	if cred == nil {
+		return fmt.Errorf("BMC credential for %s is nil", mac)
+	}
+
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	key := m.bmcKey(mac)
@@ -102,6 +107,10 @@ func (m *InMemoryCredentialManager) PutBMC(ctx context.Context, mac net.Hardware
 
 // PatchBMC updates the BMC credential for mac (replaces current value).
 func (m *InMemoryCredentialManager) PatchBMC(ctx context.Context, mac net.HardwareAddr, cred *credential.Credential) error {
+	if cred == nil {
+		return fmt.Errorf("BMC credential for %s is nil", mac)
+	}
+
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	key := m.bmcKey(mac)
@@ -144,6 +153,10 @@ func (m *InMemoryCredentialManager) GetNVOS(ctx context.Context, mac net.Hardwar
 // PutNVOS stores the NVOS credential for mac. If an identical entry exists, this is a no-op.
 // If a different entry exists, the new value overwrites (with a warning log).
 func (m *InMemoryCredentialManager) PutNVOS(ctx context.Context, mac net.HardwareAddr, cred *credential.Credential) error {
+	if cred == nil {
+		return fmt.Errorf("NVOS credential for %s is nil", mac)
+	}
+
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	key := m.nvosKey(mac)
@@ -160,6 +173,10 @@ func (m *InMemoryCredentialManager) PutNVOS(ctx context.Context, mac net.Hardwar
 
 // PatchNVOS updates the NVOS credential for mac (replaces current value).
 func (m *InMemoryCredentialManager) PatchNVOS(ctx context.Context, mac net.HardwareAddr, cred *credential.Credential) error {
+	if cred == nil {
+		return fmt.Errorf("NVOS credential for %s is nil", mac)
+	}
+
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	key := m.nvosKey(mac)

nvswitch-manager/pkg/credentials/inmemory.go

@@ -61,14 +61,15 @@ func TestInMemoryStartStop(t *testing.T) {
 
 func TestInMemoryBMCPutGet(t *testing.T) {
 	testCases := map[string]struct {
-		initialPut bool
-		putMAC     string
-		putCred    *credential.Credential
-		getMAC     string
-		wantErr    bool
-		wantUser   string
-		wantPass   string
-		samePtr    bool
+		initialPut   bool
+		putMAC       string
+		putCred      *credential.Credential
+		putSameAgain bool // if true, immediately re-put a fresh-but-equal credential to exercise the idempotent skip path
+		getMAC       string
+		wantErr      bool
+		wantUser     string
+		wantPass     string
+		samePtr      bool
 	}{
 		"get existing valid BMC credential": {
 			initialPut: true,
@@ -93,14 +94,15 @@ func TestInMemoryBMCPutGet(t *testing.T) {
 			wantErr:    true,
 		},
 		"put same credential is no-op": {
-			initialPut: true,
-			putMAC:     "aa:bb:cc:dd:ee:ff",
-			putCred:    newCredential("user1", "p1"),
-			getMAC:     "aa:bb:cc:dd:ee:ff",
-			wantErr:    false,
-			wantUser:   "user1",
-			wantPass:   "p1",
-			samePtr:    true,
+			initialPut:   true,
+			putMAC:       "aa:bb:cc:dd:ee:ff",
+			putCred:      newCredential("user1", "p1"),
+			putSameAgain: true,
+			getMAC:       "aa:bb:cc:dd:ee:ff",
+			wantErr:      false,
+			wantUser:     "user1",
+			wantPass:     "p1",
+			samePtr:      true, // second put with equal-but-fresh pointer must be skipped, leaving original pointer in place
 		},
 	}
 
@@ -113,9 +115,12 @@ func TestInMemoryBMCPutGet(t *testing.T) {
 			if tc.initialPut {
 				mac := parseMAC(t, tc.putMAC)
 				assert.NoError(t, mgr.PutBMC(ctx, mac, tc.putCred))
-				// For the idempotent scenario, put the same credential again
-				if name == "put same credential is no-op" {
-					assert.NoError(t, mgr.PutBMC(ctx, mac, newCredential("user1", "p1")))
+				// Exercise the idempotent skip path with a fresh-but-equal
+				// credential pointer. samePtr below verifies the original
+				// pointer survived (i.e. the second Put was actually skipped,
+				// not just rewritten with the same values).
+				if tc.putSameAgain {
+					assert.NoError(t, mgr.PutBMC(ctx, mac, newCredential(tc.putCred.User, tc.putCred.Password.Value)))
 				}
 			}