From 94c24a879841243a38584926296947ac4f7b8598 Mon Sep 17 00:00:00 2001 From: Chet Nichols III Date: Thu, 27 Aug 2026 17:20:57 -0700 Subject: [PATCH] fix(rest-api): reject heterogeneous ExpectedMachine batch updates ExpectedMachine batch updates already require every item to provide the same fields, except `bmcIpAddress`. The handler validated each item independently, so mismatched field sets could reach `ExpectedMachine.UpdateMultiple` and write zero values through its shared column list. Compare effective field presence before any database or workflow work begins. Items may still use different values, empty collections remain explicit clears, and `bmcIpAddress` keeps its existing set, clear, omit, and `null` behavior. This supports https://github.com/NVIDIA/infra-controller/issues/5458 Signed-off-by: Chet Nichols III --- .../api/pkg/api/handler/expectedmachine.go | 53 ++++++ .../pkg/api/handler/expectedmachine_test.go | 180 +++++++++++++++++- 2 files changed, 228 insertions(+), 5 deletions(-) diff --git a/rest-api/api/pkg/api/handler/expectedmachine.go b/rest-api/api/pkg/api/handler/expectedmachine.go index b5678ae3d7..5fc345319a 100644 --- a/rest-api/api/pkg/api/handler/expectedmachine.go +++ b/rest-api/api/pkg/api/handler/expectedmachine.go @@ -1259,6 +1259,52 @@ func NewUpdateExpectedMachinesHandler(dbSession *cdb.Session, scp *sc.ClientPool } } +type expectedMachineUpdateFieldSet struct { + bmcMacAddress bool + defaultBmcUsername bool + defaultBmcPassword bool + chassisSerialNumber bool + fallbackDPUSerialNumbers bool + skuID bool + rackID bool + name bool + manufacturer bool + model bool + description bool + slotID bool + trayIdx bool + hostID bool + isDpfEnabled bool + labels bool + hostLifecycleProfile bool +} + +// expectedMachineUpdateFields records field presence across the complete update +// request without comparing values. Keep it in sync with +// `model.APIExpectedMachineUpdateRequest`: `ID` selects the row, and +// `bmcIpAddress` is the sole documented per-machine exception. +func expectedMachineUpdateFields(req model.APIExpectedMachineUpdateRequest) expectedMachineUpdateFieldSet { + return expectedMachineUpdateFieldSet{ + bmcMacAddress: req.BmcMacAddress != nil, + defaultBmcUsername: req.DefaultBmcUsername != nil, + defaultBmcPassword: req.DefaultBmcPassword != nil, + chassisSerialNumber: req.ChassisSerialNumber != nil, + fallbackDPUSerialNumbers: req.FallbackDPUSerialNumbers != nil, + skuID: req.SkuID != nil, + rackID: req.RackID != nil, + name: req.Name != nil, + manufacturer: req.Manufacturer != nil, + model: req.Model != nil, + description: req.Description != nil, + slotID: req.SlotID != nil, + trayIdx: req.TrayIdx != nil, + hostID: req.HostID != nil, + isDpfEnabled: req.IsDpfEnabled != nil, + labels: req.Labels != nil, + hostLifecycleProfile: req.HostLifecycleProfile.ToDBModelPtr() != nil, + } +} + // Handle godoc // @Summary Batch update ExpectedMachines // @Description Update multiple ExpectedMachines in a single request. All machines must belong to the same site. @@ -1307,6 +1353,9 @@ func (uemh UpdateExpectedMachinesHandler) Handle(c echo.Context) error { idMap := make(map[uuid.UUID]int) // Map Expected Machine ID to its index in the request array serialMap := make(map[string]int) requestedSkuIDs := make(map[string]bool) + // The public batch contract requires one update field set. Enforce it before + // `UpdateMultiple` can apply its shared column list to rows that omitted a field. + batchFields := expectedMachineUpdateFields(apiRequests[0]) for i, req := range apiRequests { strIndex := strconv.Itoa(i) // index/key as string for validation errors map itemErrors := validation.Errors{} @@ -1347,6 +1396,10 @@ func (uemh UpdateExpectedMachinesHandler) Handle(c echo.Context) error { requestedSkuIDs[*req.SkuID] = true } + if expectedMachineUpdateFields(req) != batchFields { + common.AddToValidationErrors(itemErrors, "fields", errors.New("must provide the same set of fields as batch item 0, except bmcIpAddress")) + } + if len(itemErrors) > 0 { validationErrors[strIndex] = itemErrors } diff --git a/rest-api/api/pkg/api/handler/expectedmachine_test.go b/rest-api/api/pkg/api/handler/expectedmachine_test.go index 554fce50b8..c3043647d8 100644 --- a/rest-api/api/pkg/api/handler/expectedmachine_test.go +++ b/rest-api/api/pkg/api/handler/expectedmachine_test.go @@ -10,6 +10,8 @@ import ( "fmt" "net/http" "net/http/httptest" + "reflect" + "strings" "testing" "github.com/NVIDIA/infra-controller/rest-api/api/internal/config" @@ -2714,17 +2716,181 @@ func TestUpdateExpectedMachineHandler_BmcCredentialsForwardedToWorkflow(t *testi } } -// TestUpdateExpectedMachinesHandler_Handle tests the batch update handler +func TestExpectedMachineUpdateFields(t *testing.T) { + t.Run("tracks every request field except ID and BMC IP address", func(t *testing.T) { + requestFieldCount := reflect.TypeOf(model.APIExpectedMachineUpdateRequest{}).NumField() + updateFieldCount := reflect.TypeOf(expectedMachineUpdateFieldSet{}).NumField() + assert.Equal(t, requestFieldCount-2, updateFieldCount) + }) + + fieldPresenceTests := []struct { + name string + setField func(*model.APIExpectedMachineUpdateRequest) + wantEqual bool + }{ + { + name: "ID identifies the target instead of an update field", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.ID = cutil.GetPtr(uuid.NewString()) }, + wantEqual: true, + }, + { + name: "BMC IP address may vary per machine", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.BmcIpAddress = cutil.GetPtr("192.0.2.10") }, + wantEqual: true, + }, + { + name: "empty host lifecycle profile is an omitted update", + setField: func(req *model.APIExpectedMachineUpdateRequest) { + req.HostLifecycleProfile = &model.APIHostLifecycleProfile{} + }, + wantEqual: true, + }, + { + name: "BMC MAC address", + setField: func(req *model.APIExpectedMachineUpdateRequest) { + req.BmcMacAddress = cutil.GetPtr("00:11:22:33:44:55") + }, + }, + { + name: "default BMC username", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.DefaultBmcUsername = cutil.GetPtr("admin") }, + }, + { + name: "default BMC password", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.DefaultBmcPassword = cutil.GetPtr("password") }, + }, + { + name: "chassis serial number", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.ChassisSerialNumber = cutil.GetPtr("SERIAL") }, + }, + { + name: "empty fallback DPU serial numbers", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.FallbackDPUSerialNumbers = []string{} }, + }, + { + name: "SKU ID", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.SkuID = cutil.GetPtr(uuid.NewString()) }, + }, + { + name: "rack ID", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.RackID = cutil.GetPtr("rack-1") }, + }, + { + name: "name", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.Name = cutil.GetPtr("machine") }, + }, + { + name: "manufacturer", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.Manufacturer = cutil.GetPtr("NVIDIA") }, + }, + { + name: "model", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.Model = cutil.GetPtr("test") }, + }, + { + name: "description", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.Description = cutil.GetPtr("test") }, + }, + { + name: "slot ID", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.SlotID = cutil.GetPtr(int32(1)) }, + }, + { + name: "tray index", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.TrayIdx = cutil.GetPtr(int32(1)) }, + }, + { + name: "host ID", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.HostID = cutil.GetPtr(int32(1)) }, + }, + { + name: "DPF enablement", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.IsDpfEnabled = cutil.GetPtr(true) }, + }, + { + name: "empty labels", + setField: func(req *model.APIExpectedMachineUpdateRequest) { req.Labels = map[string]string{} }, + }, + { + name: "host lifecycle profile setting", + setField: func(req *model.APIExpectedMachineUpdateRequest) { + req.HostLifecycleProfile = &model.APIHostLifecycleProfile{DisableLockdown: cutil.GetPtr(true)} + }, + }, + } + + emptyFields := expectedMachineUpdateFields(model.APIExpectedMachineUpdateRequest{}) + for _, tt := range fieldPresenceTests { + t.Run(tt.name, func(t *testing.T) { + request := model.APIExpectedMachineUpdateRequest{} + tt.setField(&request) + assert.Equal(t, tt.wantEqual, expectedMachineUpdateFields(request) == emptyFields) + }) + } + + t.Run("values may differ when field presence matches", func(t *testing.T) { + first := model.APIExpectedMachineUpdateRequest{ + Name: cutil.GetPtr("first"), + FallbackDPUSerialNumbers: []string{"DPU-1"}, + Labels: map[string]string{"machine": "first"}, + HostLifecycleProfile: &model.APIHostLifecycleProfile{DisableLockdown: cutil.GetPtr(true)}, + } + second := model.APIExpectedMachineUpdateRequest{ + Name: cutil.GetPtr("second"), + FallbackDPUSerialNumbers: []string{}, + Labels: map[string]string{}, + HostLifecycleProfile: &model.APIHostLifecycleProfile{DisableLockdown: cutil.GetPtr(false)}, + } + assert.Equal(t, expectedMachineUpdateFields(first), expectedMachineUpdateFields(second)) + }) +} + func TestUpdateExpectedMachinesHandler_Handle(t *testing.T) { // Setup e := echo.New() + cfg := common.GetTestConfig() + + fieldSetTests := []struct { + name string + requestBody string + }{ + { + name: "rejects differing ordinary field sets before database access", + requestBody: `[ + {"id":"00000000-0000-0000-0000-000000000001","fallbackDPUSerialNumbers":[]}, + {"id":"00000000-0000-0000-0000-000000000002","labels":{}} + ]`, + }, + { + name: "rejects differing host lifecycle profile field sets before database access", + requestBody: `[ + {"id":"00000000-0000-0000-0000-000000000001","name":"first"}, + {"id":"00000000-0000-0000-0000-000000000002","name":"second","hostLifecycleProfile":{"disableLockdown":true}} + ]`, + }, + } + for _, tt := range fieldSetTests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPatch, "/v2/org/test-org/nico/expected-machine/batch", strings.NewReader(tt.requestBody)) + req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + c.Set("user", &cdbm.User{}) + c.SetParamNames("orgName") + c.SetParamValues("test-org") + + handler := NewUpdateExpectedMachinesHandler(nil, nil, cfg) + err := handler.Handle(c) + require.NoError(t, err) + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Contains(t, rec.Body.String(), "same set of fields") + }) + } // Initialize test database dbSession := testExpectedMachineInitDB(t) defer dbSession.Close() - cfg := common.GetTestConfig() - // Prepare client pool for workflow calls tcfg, _ := cfg.GetTemporalConfig() scp := sc.NewClientPool(tcfg) @@ -2966,13 +3132,17 @@ func TestUpdateExpectedMachinesHandler_Handle(t *testing.T) { }, { name: "BMC MAC address change rejects the whole batch", + // Keep both requests on the same field set so this case reaches the + // BMC MAC immutability check. requestBody: []model.APIExpectedMachineUpdateRequest{ { - ID: cutil.GetPtr(testEM1.ID.String()), - BmcMacAddress: cutil.GetPtr("AA:BB:CC:DD:EE:FF"), + ID: cutil.GetPtr(testEM1.ID.String()), + BmcMacAddress: cutil.GetPtr("AA:BB:CC:DD:EE:FF"), + ChassisSerialNumber: cutil.GetPtr("REJECTED-BATCH-COMPANION"), }, { ID: cutil.GetPtr(testEM2.ID.String()), + BmcMacAddress: cutil.GetPtr(testEM2.BmcMacAddress), ChassisSerialNumber: cutil.GetPtr("REJECTED-BATCH-CHANGE"), }, },