diff --git a/rest-api/api/pkg/api/handler/instance.go b/rest-api/api/pkg/api/handler/instance.go index a7f351cb54..0e3c3d5445 100644 --- a/rest-api/api/pkg/api/handler/instance.go +++ b/rest-api/api/pkg/api/handler/instance.go @@ -2581,8 +2581,8 @@ func (uih UpdateInstanceHandler) Handle(c echo.Context) error { // Collect all Subnet and VPC Prefix IDs for batch query subnetIDs := []uuid.UUID{} vpcPrefixIDs := []uuid.UUID{} - subnetIfcMap := map[uuid.UUID]int{} - vpcPrefixIfcMap := map[uuid.UUID]int{} + subnetIfcMap := map[uuid.UUID]uint64{} + vpcPrefixIfcMap := map[uuid.UUID]uint64{} for _, ifc := range apiRequest.Interfaces { if ifc.SubnetID != nil { @@ -2638,8 +2638,8 @@ func (uih UpdateInstanceHandler) Handle(c echo.Context) error { return cutil.NewAPIErrorResponse(c, interfaceVpcErr.Code, interfaceVpcErr.Message, interfaceVpcErr.Data) } - existingSubnetIfcMap := map[uuid.UUID]int{} - existingVpcPrefixIfcMap := map[uuid.UUID]int{} + existingSubnetIfcMap := map[uuid.UUID]uint64{} + existingVpcPrefixIfcMap := map[uuid.UUID]uint64{} if len(apiRequest.Interfaces) > 0 { ifcDAO := cdbm.NewInterfaceDAO(uih.dbSession) existingIfcsForCapacity, _, err := ifcDAO.GetAll(ctx, nil, cdbm.InterfaceFilterInput{InstanceIDs: []uuid.UUID{instance.ID}}, cdbp.PageInput{Limit: cutil.GetPtr(cdbp.TotalLimit)}, nil) @@ -2649,6 +2649,10 @@ func (uih UpdateInstanceHandler) Handle(c echo.Context) error { } for i := range existingIfcsForCapacity { eifc := &existingIfcsForCapacity[i] + if eifc.Status == cdbm.InterfaceStatusDeleting { + continue + } + if eifc.SubnetID != nil { existingSubnetIfcMap[*eifc.SubnetID]++ } @@ -2742,9 +2746,13 @@ func (uih UpdateInstanceHandler) Handle(c echo.Context) error { } // Check if Subnet is exhausted - incomingInterfaceIPs := subnetIfcMap[subnetID] - existingSubnetIfcMap[subnetID] + incomingInterfaceIPs := uint64(0) + if subnetIfcMap[subnetID] > existingSubnetIfcMap[subnetID] { + incomingInterfaceIPs = subnetIfcMap[subnetID] - existingSubnetIfcMap[subnetID] + } + subnetUsage := subnetUsageMap[subnetID] - if subnetUsage != nil && subnetUsage.AvailableIPs > 0 && subnetUsage.AcquiredIPs+uint64(incomingInterfaceIPs) > subnetUsage.AvailableIPs { + if incomingInterfaceIPs > 0 && subnetUsage != nil && subnetUsage.AvailableIPs > 0 && subnetUsage.AcquiredIPs+incomingInterfaceIPs > subnetUsage.AvailableIPs { msg := fmt.Sprintf( "Subnet %v does not have enough IP addresses: %d of %d IP addresses remain available, but the %d additional interface(s) in this request require %d IP address(es)", subnetID, subnetUsage.AvailableIPs-subnetUsage.AcquiredIPs, subnetUsage.AvailableIPs, incomingInterfaceIPs, incomingInterfaceIPs, @@ -2826,9 +2834,13 @@ func (uih UpdateInstanceHandler) Handle(c echo.Context) error { } // Check if VPC Prefix is exhausted - incomingInterfaceIPs := max(vpcPrefixIfcMap[vpcPrefixID]-existingVpcPrefixIfcMap[vpcPrefixID], 0) + incomingInterfaceIPs := uint64(0) + if vpcPrefixIfcMap[vpcPrefixID] > existingVpcPrefixIfcMap[vpcPrefixID] { + incomingInterfaceIPs = vpcPrefixIfcMap[vpcPrefixID] - existingVpcPrefixIfcMap[vpcPrefixID] + } + vpUsage := vpcPrefixUsageMap[vpcPrefixID] - if vpUsage != nil && vpUsage.AvailableIPs > 0 && vpUsage.AcquiredIPs+uint64(incomingInterfaceIPs)*2 > vpUsage.AvailableIPs { + if incomingInterfaceIPs > 0 && vpUsage != nil && vpUsage.AvailableIPs > 0 && vpUsage.AcquiredIPs+incomingInterfaceIPs*2 > vpUsage.AvailableIPs { msg := fmt.Sprintf( "VPC Prefix %v does not have enough IP addresses: %d of %d IP addresses remain available, but the %d additional interface(s) in this request require %d IP addresses", vpcPrefixID, vpUsage.AvailableIPs-vpUsage.AcquiredIPs, vpUsage.AvailableIPs, incomingInterfaceIPs, incomingInterfaceIPs*2, @@ -3460,8 +3472,8 @@ func (uih UpdateInstanceHandler) Handle(c echo.Context) error { // return an empty list. Reads after this should reflect the // auto contract (no explicit interfaces) rather than the // stale rows that pre-dated the mode switch. - // - Explicit interfaces in the request: create the new rows - // and mark the previous rows as Deleting (existing behavior). + // - Explicit interfaces in the request: reuse matching rows, + // create new rows, and mark only removed rows as Deleting. // - Neither (no interface change, not switching to auto): // carry the existing rows forward. switch { @@ -3476,7 +3488,36 @@ func (uih UpdateInstanceHandler) Handle(c echo.Context) error { } newdbIfcs = []cdbm.Interface{} case len(apiRequest.Interfaces) > 0: + existingIfcMap := make(map[cdbm.EthernetInterfaceKey][]cdbm.Interface) + + for existingIfcIndex := range existingIfcs { + if existingIfcs[existingIfcIndex].Status == cdbm.InterfaceStatusDeleting { + continue + } + + key := existingIfcs[existingIfcIndex].EthernetKey() + existingIfcMap[key] = append(existingIfcMap[key], existingIfcs[existingIfcIndex]) + } + + reusedIfcIDs := make(map[uuid.UUID]struct{}) for _, dbifc := range dbInterfaces { + key := dbifc.EthernetKey() + + existingIfcsForKey := existingIfcMap[key] + if len(existingIfcsForKey) > 0 { + reusedIfc := existingIfcsForKey[0] + if len(existingIfcsForKey) == 1 { + delete(existingIfcMap, key) + } else { + existingIfcMap[key] = existingIfcsForKey[1:] + } + + reusedIfcIDs[reusedIfc.ID] = struct{}{} + newdbIfcs = append(newdbIfcs, reusedIfc) + + continue + } + input := cdbm.InterfaceCreateInput{ InstanceID: instance.ID, SubnetID: dbifc.SubnetID, @@ -3502,19 +3543,36 @@ func (uih UpdateInstanceHandler) Handle(c echo.Context) error { ifc := *newDbifc ifc.Vpc = dbifc.Vpc ifc.VpcPrefix = dbifc.VpcPrefix // We created the interface in the DB based on the values in dbifc, so we can populate this as well. + ifc.Subnet = dbifc.Subnet // Add the new Interface to the list of new Interfaces newdbIfcs = append(newdbIfcs, ifc) } - // Update status of existing Interfaces to Deleting - for i := range existingIfcs { - existingIfcs[i].Status = cdbm.InterfaceStatusDeleting - _, err := ifcDAO.Update(ctx, tx, cdbm.InterfaceUpdateInput{InterfaceID: existingIfcs[i].ID, Status: cutil.GetPtr(cdbm.InterfaceStatusDeleting)}) - if err != nil { - logger.Error().Err(err).Msg("failed to update Interface record in DB") - return cutil.NewAPIError(http.StatusInternalServerError, "Failed to update Interface for Instance, DB error", nil) + unmatchedIfcs := make([]cdbm.Interface, 0, len(existingIfcs)-len(reusedIfcIDs)) + for existingIfcIndex := range existingIfcs { + if _, reused := reusedIfcIDs[existingIfcs[existingIfcIndex].ID]; reused { + continue + } + + if existingIfcs[existingIfcIndex].Status != cdbm.InterfaceStatusDeleting { + existingIfcs[existingIfcIndex].Status = cdbm.InterfaceStatusDeleting + + // Deleting rows retain their associations and allocated addresses until Site cleanup releases them. + _, err := ifcDAO.Update(ctx, tx, cdbm.InterfaceUpdateInput{ + InterfaceID: existingIfcs[existingIfcIndex].ID, + Status: cutil.GetPtr(cdbm.InterfaceStatusDeleting), + }) //nolint:exhaustruct // Only the lifecycle status changes; associations remain held. + if err != nil { + logger.Error().Err(err).Msg("failed to update Interface record in DB") + + return cutil.NewAPIError(http.StatusInternalServerError, "Failed to update Interface for Instance, DB error", nil) + } } + + unmatchedIfcs = append(unmatchedIfcs, existingIfcs[existingIfcIndex]) } + + existingIfcs = unmatchedIfcs default: newdbIfcs = existingIfcs } diff --git a/rest-api/api/pkg/api/handler/instance_test.go b/rest-api/api/pkg/api/handler/instance_test.go index eeb5e4efa1..ee6456a124 100644 --- a/rest-api/api/pkg/api/handler/instance_test.go +++ b/rest-api/api/pkg/api/handler/instance_test.go @@ -560,6 +560,15 @@ func testUpdateInterfaceWithIPs(t *testing.T, dbSession *cdb.Session, ifc *cdbm. return ifc } +type ethernetReconciliationExpectation struct { + rowCount int + readyIDs []uuid.UUID + deletingIDs []uuid.UUID + pendingCount int + uniqueIPAddress *string + usagePrefix *cdbm.VpcPrefix +} + func testUpdateMachineToUnhealthy(t *testing.T, dbSession *cdb.Session, m *cdbm.Machine) *cdbm.Machine { m.Status = cdbm.MachineStatusError _, err := dbSession.DB.NewUpdate().Where("id = ?", m.ID).Model(m).Exec(context.Background()) @@ -4390,6 +4399,24 @@ func TestUpdateInstanceHandler_Handle(t *testing.T) { // Add Network DPU capability to Instance Type common.TestBuildMachineCapability(t, dbSession, nil, &ist4.ID, cdbm.MachineCapabilityTypeNetwork, "MT42822 BlueField-2 integrated ConnectX-6 Dx network controller", nil, nil, cutil.GetPtr("Mellanox Technologies"), cutil.GetPtr(2), cutil.GetPtr(cdbm.MachineCapabilityDeviceTypeDPU), nil) + issue4908Device := cutil.GetPtr("MT42822 BlueField-2 integrated ConnectX-6 Dx network controller") + issue4908DeviceInstance := cutil.GetPtr(0) + issue4908VFID := cutil.GetPtr(1) + + issue4908AddVFMachine := testInstanceBuildMachine(t, dbSession, ip.ID, st3.ID, cutil.GetPtr(false), nil) + assert.NotNil(t, testInstanceBuildMachineInstanceType(t, dbSession, issue4908AddVFMachine, ist4)) + issue4908AddVFInstance := testInstanceBuildInstance(t, dbSession, "issue-4908-add-vf", tn1.ID, ip.ID, st3.ID, &ist4.ID, vpc4.ID, cutil.GetPtr(issue4908AddVFMachine.ID), &os2.ID, nil, cdbm.InstanceStatusReady) + issue4908AddVFPF := testInstanceBuildInterface(t, dbSession, issue4908AddVFInstance.ID, nil, &vpcPrefix1.ID, issue4908Device, issue4908DeviceInstance, nil, true, cdbm.InterfaceStatusReady, tnu1) + testUpdateInterfaceWithIPs(t, dbSession, issue4908AddVFPF, []string{"192.168.0.1"}) + + issue4908RemoveMachine := testInstanceBuildMachine(t, dbSession, ip.ID, st3.ID, cutil.GetPtr(false), nil) + assert.NotNil(t, testInstanceBuildMachineInstanceType(t, dbSession, issue4908RemoveMachine, ist4)) + issue4908RemoveInstance := testInstanceBuildInstance(t, dbSession, "issue-4908-remove-vf", tn1.ID, ip.ID, st3.ID, &ist4.ID, vpc4.ID, cutil.GetPtr(issue4908RemoveMachine.ID), &os2.ID, nil, cdbm.InstanceStatusReady) + issue4908RemovePF := testInstanceBuildInterface(t, dbSession, issue4908RemoveInstance.ID, nil, &vpcPrefix1.ID, issue4908Device, issue4908DeviceInstance, nil, true, cdbm.InterfaceStatusReady, tnu1) + testUpdateInterfaceWithIPs(t, dbSession, issue4908RemovePF, []string{"192.168.0.3"}) + issue4908RemoveVF := testInstanceBuildInterface(t, dbSession, issue4908RemoveInstance.ID, nil, &vpcPrefixSite3Secondary.ID, issue4908Device, issue4908DeviceInstance, issue4908VFID, false, cdbm.InterfaceStatusReady, tnu1) + testUpdateInterfaceWithIPs(t, dbSession, issue4908RemoveVF, []string{"192.174.0.1"}) + inst13 := testInstanceBuildInstance(t, dbSession, "test-instance-nvlink-update", tn1.ID, ip.ID, st3.ID, &ist4.ID, vpc4.ID, cutil.GetPtr(mc5.ID), &os2.ID, nil, cdbm.InstanceStatusReady) // Add NVLink GPU capability to Machine @@ -4749,7 +4776,8 @@ func TestUpdateInstanceHandler_Handle(t *testing.T) { // When true with nvlinkInterfacesToDelete, still assert those rows are Deleting but skip Pending-row count/order checks. nvLinkSkipPendingDBAssertions bool // Optional hook after building the echo context and before Handle (e.g. adjust DB timestamps for time-sensitive branches). - beforeHandle func(t *testing.T) + beforeHandle func(t *testing.T) + ethernetReconciliation *ethernetReconciliationExpectation } tests := []struct { @@ -6146,6 +6174,105 @@ func TestUpdateInstanceHandler_Handle(t *testing.T) { verifySiteControllerRequest: true, verifyChildSpanner: true, }, + { + name: "test UpdateInstance adding VF reuses unchanged PF issue 4908", + fields: fields{ + dbSession: dbSession, + tc: tc, + scp: scp, + cfg: cfg, + }, + args: args{ //nolint:exhaustruct // This case leaves unrelated response assertions unset. + reqData: &model.APIInstanceUpdateRequest{ //nolint:exhaustruct // The request changes only Ethernet fields. + Name: cutil.GetPtr("Issue 4908 Add VF"), + IpxeScript: os2.IpxeScript, + SecondaryVpcIDs: []string{ + vpc4Site3Secondary.ID.String(), + }, + Interfaces: []model.APIInterfaceCreateOrUpdateRequest{ + { + SubnetID: nil, + VpcPrefixID: cutil.GetPtr(vpcPrefix1.ID.String()), + IPAddress: nil, + InlineRoutingProfile: nil, + Device: issue4908Device, + DeviceInstance: issue4908DeviceInstance, + VirtualFunctionID: nil, + IsPhysical: true, + }, + { + SubnetID: nil, + VpcPrefixID: cutil.GetPtr(vpcPrefixSite3Secondary.ID.String()), + IPAddress: nil, + InlineRoutingProfile: nil, + Device: issue4908Device, + DeviceInstance: issue4908DeviceInstance, + VirtualFunctionID: issue4908VFID, + IsPhysical: false, + }, + }, + }, + reqOrg: tnOrg1, + reqUser: tnu1, + reqInstance: issue4908AddVFInstance.ID.String(), + cleanInstanceToStatus: issue4908AddVFInstance.Status, + respCode: http.StatusOK, + ethernetReconciliation: ðernetReconciliationExpectation{ + rowCount: 2, + readyIDs: []uuid.UUID{issue4908AddVFPF.ID}, + deletingIDs: []uuid.UUID{}, + pendingCount: 1, + uniqueIPAddress: cutil.GetPtr("192.168.0.1"), + usagePrefix: vpcPrefix1, + }, + }, + wantErr: false, + verifySiteControllerRequest: true, + verifyChildSpanner: true, + }, + { + name: "test UpdateInstance marks only omitted Ethernet interface deleting issue 4908", + fields: fields{ + dbSession: dbSession, + tc: tc, + scp: scp, + cfg: cfg, + }, + args: args{ //nolint:exhaustruct // This case leaves unrelated response assertions unset. + reqData: &model.APIInstanceUpdateRequest{ //nolint:exhaustruct // The request changes only Ethernet fields. + Name: cutil.GetPtr("Issue 4908 Remove VF"), + IpxeScript: os2.IpxeScript, + Interfaces: []model.APIInterfaceCreateOrUpdateRequest{ + { + SubnetID: nil, + VpcPrefixID: cutil.GetPtr(vpcPrefix1.ID.String()), + IPAddress: nil, + InlineRoutingProfile: nil, + Device: issue4908Device, + DeviceInstance: issue4908DeviceInstance, + VirtualFunctionID: nil, + IsPhysical: true, + }, + }, + }, + reqOrg: tnOrg1, + reqUser: tnu1, + reqInstance: issue4908RemoveInstance.ID.String(), + cleanInstanceToStatus: issue4908RemoveInstance.Status, + respCode: http.StatusOK, + ethernetReconciliation: ðernetReconciliationExpectation{ + rowCount: 2, + readyIDs: []uuid.UUID{issue4908RemovePF.ID}, + deletingIDs: []uuid.UUID{issue4908RemoveVF.ID}, + pendingCount: 0, + uniqueIPAddress: nil, + usagePrefix: nil, + }, + }, + wantErr: false, + verifySiteControllerRequest: true, + verifyChildSpanner: true, + }, { name: "test Instance update API endpoint success with interface update", fields: fields{ @@ -7239,6 +7366,75 @@ func TestUpdateInstanceHandler_Handle(t *testing.T) { assert.ElementsMatch(t, tt.args.reqData.SecondaryVpcIDs, rst.SecondaryVpcIDs) } + if expected := tt.args.ethernetReconciliation; expected != nil { + reconciledIfcs, _, reconciliationErr := ifcDAO.GetAll( + ctx, + nil, + cdbm.InterfaceFilterInput{ + InstanceIDs: []uuid.UUID{reqIns.ID}, + SubnetID: nil, + VpcPrefixID: nil, + Device: nil, + DeviceInstance: nil, + IsPhysical: nil, + Statuses: nil, + IPAddresses: nil, + }, + cdbp.PageInput{Offset: nil, Limit: cutil.GetPtr(cdbp.TotalLimit), OrderBy: nil}, + nil, + ) + require.NoError(t, reconciliationErr) + assert.Len(t, reconciledIfcs, expected.rowCount) + assert.Len(t, rst.Interfaces, expected.rowCount) + + pendingCount := 0 + deletingCount := 0 + rowsWithExpectedIP := 0 + + for _, ifc := range reconciledIfcs { + switch ifc.Status { + case cdbm.InterfaceStatusPending: + pendingCount++ + case cdbm.InterfaceStatusDeleting: + deletingCount++ + } + + for _, ipAddress := range ifc.IPAddresses { + if expected.uniqueIPAddress != nil && ipAddress == *expected.uniqueIPAddress { + rowsWithExpectedIP++ + } + } + } + + assert.Equal(t, expected.pendingCount, pendingCount) + assert.Equal(t, len(expected.deletingIDs), deletingCount) + + if expected.uniqueIPAddress != nil { + assert.Equal(t, 1, rowsWithExpectedIP) + } + + for _, interfaceID := range expected.readyIDs { + ifc, getErr := ifcDAO.GetByID(ctx, nil, interfaceID, nil) + require.NoError(t, getErr) + assert.Equal(t, cdbm.InterfaceStatusReady, ifc.Status) + } + + for _, interfaceID := range expected.deletingIDs { + ifc, getErr := ifcDAO.GetByID(ctx, nil, interfaceID, nil) + require.NoError(t, getErr) + assert.Equal(t, cdbm.InterfaceStatusDeleting, ifc.Status) + } + + if expected.usagePrefix != nil { + usageByID, usageErr := cdbm.NewVpcPrefixDAO(tt.fields.dbSession).GetPrefixUsage(ctx, nil, expected.usagePrefix) + require.NoError(t, usageErr) + + usage := usageByID[expected.usagePrefix.ID] + require.NotNil(t, usage) + assert.LessOrEqual(t, usage.AcquiredIPs+uint64(2), usage.AvailableIPs) + } + } + if tt.args.expectedNetworkSecurityGroupInherited != nil { assert.Equal(t, *tt.args.expectedNetworkSecurityGroupInherited, rst.NetworkSecurityGroupInherited) } @@ -7358,7 +7554,21 @@ func TestUpdateInstanceHandler_Handle(t *testing.T) { if tt.args.respNoOfInterfaces != nil { reqInsIfcs, _, _ = ifcDAO.GetAll(ec.Request().Context(), nil, cdbm.InterfaceFilterInput{InstanceIDs: []uuid.UUID{reqIns.ID}, Statuses: []string{cdbm.InterfaceStatusPending}}, cdbp.PageInput{OrderBy: &cdbp.OrderBy{Field: cdbm.InterfaceOrderByCreated, Order: cdbp.OrderAscending}}, nil) } else { - reqInsIfcs, _, _ = ifcDAO.GetAll(ec.Request().Context(), nil, cdbm.InterfaceFilterInput{InstanceIDs: []uuid.UUID{reqIns.ID}}, cdbp.PageInput{OrderBy: &cdbp.OrderBy{Field: cdbm.InterfaceOrderByCreated, Order: cdbp.OrderAscending}}, nil) + reqInsIfcs, _, _ = ifcDAO.GetAll(ec.Request().Context(), nil, cdbm.InterfaceFilterInput{ + InstanceIDs: []uuid.UUID{reqIns.ID}, + SubnetID: nil, + VpcPrefixID: nil, + Device: nil, + DeviceInstance: nil, + IsPhysical: nil, + Statuses: []string{ + cdbm.InterfaceStatusPending, + cdbm.InterfaceStatusProvisioning, + cdbm.InterfaceStatusReady, + cdbm.InterfaceStatusError, + }, + IPAddresses: nil, + }, cdbp.PageInput{Offset: nil, Limit: nil, OrderBy: &cdbp.OrderBy{Field: cdbm.InterfaceOrderByCreated, Order: cdbp.OrderAscending}}, nil) } assert.Equal(t, len(reqInsIfcs), len(siteReq.Config.Network.Interfaces)) @@ -7409,7 +7619,7 @@ func TestUpdateInstanceHandler_Handle(t *testing.T) { // Check if VirtualFunctionId is present if reqInsIfcs[i].VirtualFunctionID != nil { - assert.Equal(t, siteIfc.VirtualFunctionId, reqInsIfcs[i].VirtualFunctionID) + assert.Equal(t, uint32(*reqInsIfcs[i].VirtualFunctionID), siteIfc.GetVirtualFunctionId()) } if reqInsIfcs[i].RequestedIpAddress != nil { diff --git a/rest-api/db/pkg/db/model/interface.go b/rest-api/db/pkg/db/model/interface.go index 81f83b05a2..e6f5d8af0d 100644 --- a/rest-api/db/pkg/db/model/interface.go +++ b/rest-api/db/pkg/db/model/interface.go @@ -7,6 +7,8 @@ import ( "context" "database/sql" "fmt" + "net/netip" + "strings" "time" "github.com/NVIDIA/infra-controller/rest-api/db/pkg/db" @@ -131,6 +133,97 @@ type Interface struct { CreatedBy uuid.UUID `bun:"type:uuid,notnull"` } +// EthernetInterfaceKey identifies an Ethernet interface configuration for update reconciliation. +type EthernetInterfaceKey struct { + SubnetID uuid.UUID + VpcPrefixID uuid.UUID + VpcID uuid.UUID + VpcIPFamilyMode InterfaceVpcIPFamilyMode + HasVpcIPFamilyMode bool + VirtualFunctionID int + HasVirtualFunctionID bool + Device string + HasDevice bool + DeviceInstance int + HasDeviceInstance bool + IsPhysical bool + RequestedIPAddress string + HasRequestedIPAddress bool + InlineRoutingProfile string + HasInlineRoutingProfile bool +} + +// EthernetKey returns the comparable fields that identify an Interface during update reconciliation. +func (ifc Interface) EthernetKey() EthernetInterfaceKey { + vpcID := uuid.Nil + if ifc.VpcID != nil { + vpcID = *ifc.VpcID + } else if ifc.VpcPrefix != nil { + vpcID = ifc.VpcPrefix.VpcID + } else if ifc.Subnet != nil { + vpcID = ifc.Subnet.VpcID + } + + key := EthernetInterfaceKey{ + SubnetID: uuid.Nil, + VpcPrefixID: uuid.Nil, + VpcID: vpcID, + VpcIPFamilyMode: "", + HasVpcIPFamilyMode: false, + VirtualFunctionID: 0, + HasVirtualFunctionID: false, + Device: "", + HasDevice: false, + DeviceInstance: 0, + HasDeviceInstance: false, + IsPhysical: ifc.IsPhysical, + RequestedIPAddress: "", + HasRequestedIPAddress: false, + InlineRoutingProfile: "", + HasInlineRoutingProfile: false, + } + + if ifc.SubnetID != nil { + key.SubnetID = *ifc.SubnetID + } + + if ifc.VpcPrefixID != nil { + key.VpcPrefixID = *ifc.VpcPrefixID + } + + if ifc.VpcIPFamilyMode != nil { + key.VpcIPFamilyMode = *ifc.VpcIPFamilyMode + key.HasVpcIPFamilyMode = true + } + + if ifc.VirtualFunctionID != nil { + key.VirtualFunctionID = *ifc.VirtualFunctionID + key.HasVirtualFunctionID = true + } + + if ifc.Device != nil { + key.Device = *ifc.Device + key.HasDevice = true + } + + if ifc.DeviceInstance != nil { + key.DeviceInstance = *ifc.DeviceInstance + key.HasDeviceInstance = true + } + + if ifc.RequestedIpAddress != nil { + key.RequestedIPAddress = *ifc.RequestedIpAddress + key.HasRequestedIPAddress = true + } + + if ifc.InlineRoutingProfile != nil { + key.InlineRoutingProfile = strings.Join(ifc.InlineRoutingProfile.AllowedAnycastPrefixes, "\x00") + key.HasInlineRoutingProfile = true + } + + return key +} + // InterfaceCreateInput input parameters for Create method type InterfaceCreateInput struct { InstanceID uuid.UUID @@ -503,6 +596,13 @@ func (ifcd InterfaceSQLDAO) Update(ctx context.Context, tx *db.Tx, input Interfa } } if input.IpAddresses != nil { + for _, ipAddress := range input.IpAddresses { + _, parseErr := netip.ParseAddr(ipAddress) + if parseErr != nil { + return nil, fmt.Errorf("invalid Interface IP address %q: %w", ipAddress, parseErr) + } + } + is.IPAddresses = input.IpAddresses updatedFields = append(updatedFields, "ip_addresses") diff --git a/rest-api/db/pkg/db/model/interface_test.go b/rest-api/db/pkg/db/model/interface_test.go index 5eedfdbffb..3ccf926687 100644 --- a/rest-api/db/pkg/db/model/interface_test.go +++ b/rest-api/db/pkg/db/model/interface_test.go @@ -1306,7 +1306,7 @@ func TestInterfaceSQLDAO_Update(t *testing.T) { vfID := 10 macAddress := "21-41-A7-A6-40-76" - ipAddresses := []string{"192.0.2.3", "2001:db8:abcd:0018"} + ipAddresses := []string{"192.0.2.3", "2001:db8:abcd::18"} routingProfile := &InterfaceInlineRoutingProfile{ AllowedAnycastPrefixes: []string{"192.0.2.0/24", "2001:db8::/64"}, } @@ -1444,6 +1444,18 @@ func TestInterfaceSQLDAO_Update(t *testing.T) { paramStatus: cutil.GetPtr(InterfaceStatusProvisioning), expectError: true, }, + { + desc: "failed with malformed IP address", + id: ifc1.ID, + paramIPAddresses: []string{"not-an-ip"}, + expectError: true, + }, + { + desc: "failed with CIDR-form IP address", + id: ifc1.ID, + paramIPAddresses: []string{"192.0.2.1/31"}, + expectError: true, + }, } for _, tc := range tests { t.Run(tc.desc, func(t *testing.T) { diff --git a/rest-api/db/pkg/db/model/vpcprefix.go b/rest-api/db/pkg/db/model/vpcprefix.go index 7947c021d3..608cd7532d 100644 --- a/rest-api/db/pkg/db/model/vpcprefix.go +++ b/rest-api/db/pkg/db/model/vpcprefix.go @@ -41,6 +41,9 @@ const ( // VpcPrefixOrderByDefault default field to be used for ordering when none specified VpcPrefixOrderByDefault = "created" + + vpcPrefixInterfaceBits = 31 + vpcPrefixIPsPerInterface uint64 = 2 ) var ( @@ -540,7 +543,8 @@ func (vpsd VpcPrefixSQLDAO) Delete(ctx context.Context, tx *db.Tx, id uuid.UUID) return nil } -func vpcPrefixUsageFromInterfaces(ctx context.Context, cidr string, ifcCount int64, ips []string) (*cipam.Usage, error) { +//nolint:cyclop,funlen // Sequential guards intentionally keep address handling inline. +func vpcPrefixUsageFromInterfaces(ctx context.Context, cidr string, ifcCountWithoutIPs uint64, ips []string) (*cipam.Usage, error) { ipamer := cipam.New(ctx) ipamPrefix, err := ipamer.NewPrefix(ctx, cidr) if err != nil { @@ -555,25 +559,31 @@ func vpcPrefixUsageFromInterfaces(ctx context.Context, cidr string, ifcCount int acquiredPrefixes := make(map[string]struct{}) for _, ipStr := range ips { - netIpAddr, ierr := netip.ParseAddr(strings.TrimSpace(ipStr)) - if ierr != nil || !netIpAddr.Is4() { + ipAddress, parseErr := netip.ParseAddr(strings.TrimSpace(ipStr)) + if parseErr != nil || !ipAddress.Is4() { continue } - if !netIpPrefix.Contains(netIpAddr) { + + if !netIpPrefix.Contains(ipAddress) { continue } - contained31Prefix, perr := netIpAddr.Prefix(31) - if perr != nil { + + containedPrefix, prefixErr := ipAddress.Prefix(vpcPrefixInterfaceBits) + if prefixErr != nil { continue } - k := contained31Prefix.Masked().String() - if _, dup := acquiredPrefixes[k]; dup { + + prefix := containedPrefix.Masked().String() + if _, dup := acquiredPrefixes[prefix]; dup { continue } - if _, ierr := ipamer.AcquireSpecificChildPrefix(ctx, validatedCidr, k); ierr != nil { - continue + + _, acquireErr := ipamer.AcquireSpecificChildPrefix(ctx, validatedCidr, prefix) + if acquireErr != nil { + return nil, fmt.Errorf("failed to acquire Interface prefix %q from %q: %w", prefix, validatedCidr, acquireErr) } - acquiredPrefixes[k] = struct{}{} + + acquiredPrefixes[prefix] = struct{}{} } ipamPrefix = ipamer.PrefixFrom(ctx, validatedCidr) @@ -583,7 +593,8 @@ func vpcPrefixUsageFromInterfaces(ctx context.Context, cidr string, ifcCount int usage := ipamPrefix.Usage() - acquiredIPs := uint64(ifcCount) * 2 + acquiredIPs := uint64(len(acquiredPrefixes))*vpcPrefixIPsPerInterface + + ifcCountWithoutIPs*vpcPrefixIPsPerInterface if acquiredIPs > usage.AvailableIPs { acquiredIPs = usage.AvailableIPs } @@ -622,10 +633,10 @@ func (vpsd VpcPrefixSQLDAO) GetPrefixUsage(ctx context.Context, tx *db.Tx, vpcPr idb := db.GetIDB(tx, vpsd.dbSession) - ifcCounts := make(map[uuid.UUID]int64, len(vpcPrefixIDs)) + ifcCountsWithoutIPs := make(map[uuid.UUID]uint64, len(vpcPrefixIDs)) ifcIPs := make(map[uuid.UUID][]string, len(vpcPrefixIDs)) for _, id := range vpcPrefixIDs { - ifcCounts[id] = 0 + ifcCountsWithoutIPs[id] = 0 ifcIPs[id] = nil } @@ -644,15 +655,18 @@ func (vpsd VpcPrefixSQLDAO) GetPrefixUsage(ctx context.Context, tx *db.Tx, vpcPr return nil, err } for _, r := range rows { - ifcCounts[r.VpcPrefixID]++ - if len(r.IPAddresses) > 0 { - ifcIPs[r.VpcPrefixID] = append(ifcIPs[r.VpcPrefixID], r.IPAddresses...) + if len(r.IPAddresses) == 0 { + ifcCountsWithoutIPs[r.VpcPrefixID]++ + + continue } + + ifcIPs[r.VpcPrefixID] = append(ifcIPs[r.VpcPrefixID], r.IPAddresses...) } usageByID := make(map[uuid.UUID]*cipam.Usage, len(vpcPrefixIDs)) for _, vpcPrefixID := range vpcPrefixIDs { - usage, uerr := vpcPrefixUsageFromInterfaces(ctx, vpcPrefixCIDRs[vpcPrefixID], ifcCounts[vpcPrefixID], ifcIPs[vpcPrefixID]) + usage, uerr := vpcPrefixUsageFromInterfaces(ctx, vpcPrefixCIDRs[vpcPrefixID], ifcCountsWithoutIPs[vpcPrefixID], ifcIPs[vpcPrefixID]) if uerr != nil { return nil, uerr } diff --git a/rest-api/db/pkg/db/model/vpcprefix_test.go b/rest-api/db/pkg/db/model/vpcprefix_test.go index 99f3adbd24..26d7f4e70f 100644 --- a/rest-api/db/pkg/db/model/vpcprefix_test.go +++ b/rest-api/db/pkg/db/model/vpcprefix_test.go @@ -926,3 +926,258 @@ func testVpcPrefixSQLDAO_Delete(t *testing.T) { }) } } + +//nolint:funlen // Cases stay inline so each usage invariant is visible at the call site. +func TestVpcPrefixUsageFromInterfaces(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + cidr string + ifcCountWithoutIPs uint64 + ips []string + expectedAvailableIPs uint64 + expectedAcquiredIPs uint64 + expectedAvailableSmallestPrefixes uint64 + expectedAcquiredPrefixes uint64 + }{ + { + name: "pending interfaces reserve one /31 each", + cidr: "10.0.0.0/28", + ifcCountWithoutIPs: 3, + ips: nil, + expectedAvailableIPs: 16, + expectedAcquiredIPs: 6, + expectedAvailableSmallestPrefixes: 4, + expectedAcquiredPrefixes: 0, + }, + { + name: "duplicate acquired prefix and pending interfaces are counted once each", + cidr: "10.0.0.0/28", + ifcCountWithoutIPs: 2, + ips: []string{"10.0.0.1", "10.0.0.3", "10.0.0.1"}, + expectedAvailableIPs: 16, + expectedAcquiredIPs: 8, + expectedAvailableSmallestPrefixes: 3, + expectedAcquiredPrefixes: 2, + }, + { + name: "prefix without interfaces is fully available", + cidr: "10.0.0.0/28", + ifcCountWithoutIPs: 0, + ips: nil, + expectedAvailableIPs: 16, + expectedAcquiredIPs: 0, + expectedAvailableSmallestPrefixes: 4, + expectedAcquiredPrefixes: 0, + }, + { + name: "acquired IPs clamp to prefix capacity", + cidr: "10.0.0.0/30", + ifcCountWithoutIPs: 1, + ips: []string{"10.0.0.1", "10.0.0.3"}, + expectedAvailableIPs: 4, + expectedAcquiredIPs: 4, + expectedAvailableSmallestPrefixes: 0, + expectedAcquiredPrefixes: 2, + }, + { + name: "non-empty invalid or inapplicable addresses are not pending reservations", + cidr: "10.0.0.0/28", + ifcCountWithoutIPs: 0, + ips: []string{"invalid", "10.0.0.1/31", "2001:db8::1", "192.0.2.1"}, + expectedAvailableIPs: 16, + expectedAcquiredIPs: 0, + expectedAvailableSmallestPrefixes: 4, + expectedAcquiredPrefixes: 0, + }, + } + + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + usage, err := vpcPrefixUsageFromInterfaces(context.Background(), testCase.cidr, testCase.ifcCountWithoutIPs, testCase.ips) + require.NoError(t, err) + require.NotNil(t, usage) + assert.Equal(t, testCase.expectedAvailableIPs, usage.AvailableIPs) + assert.Equal(t, testCase.expectedAcquiredIPs, usage.AcquiredIPs) + assert.Equal(t, testCase.expectedAvailableSmallestPrefixes, usage.AvailableSmallestPrefixes) + assert.Equal(t, testCase.expectedAcquiredPrefixes, usage.AcquiredPrefixes) + }) + } + + t.Run("unexpected child prefix acquisition error is propagated", func(t *testing.T) { + t.Parallel() + + usage, err := vpcPrefixUsageFromInterfaces(context.Background(), "10.0.0.1/32", 0, []string{"10.0.0.1"}) + require.Error(t, err) + assert.Nil(t, usage) + }) +} + +//nolint:funlen,paralleltest // Cases and fixtures stay inline; model tests share a PostgreSQL schema. +func TestVpcPrefixSQLDAO_GetPrefixUsage(t *testing.T) { + type interfaceFixture struct { + status string + ipAddress *string + } + + tests := []struct { + name string + interfaces []interfaceFixture + expectedAvailableIPs uint64 + expectedAcquiredIPs uint64 + expectedAcquiredPrefixes uint64 + expectedAvailableSmallest uint64 + expectedFreeInterfaceSlots uint64 + expectedAdmissionAllowed bool + }{ + { + name: "stale deleting rows do not exhaust prefix issue 4908", + // Deleting rows still hold capacity; duplicate /31 addresses are de-duplicated by prefix. + interfaces: []interfaceFixture{ + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.1")}, + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.3")}, + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.5")}, + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.7")}, + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.9")}, + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.13")}, + {status: InterfaceStatusDeleting, ipAddress: cutil.GetPtr("10.0.0.1")}, + {status: InterfaceStatusDeleting, ipAddress: cutil.GetPtr("10.0.0.3")}, + {status: InterfaceStatusDeleting, ipAddress: cutil.GetPtr("10.0.0.9")}, + {status: InterfaceStatusDeleting, ipAddress: cutil.GetPtr("10.0.0.13")}, + }, + expectedAvailableIPs: 16, + expectedAcquiredIPs: 12, + expectedAcquiredPrefixes: 6, + expectedAvailableSmallest: 0, + expectedFreeInterfaceSlots: 2, + expectedAdmissionAllowed: true, + }, + { + name: "deleting interface with a distinct IP still consumes capacity", + interfaces: []interfaceFixture{ + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.1")}, + {status: InterfaceStatusDeleting, ipAddress: cutil.GetPtr("10.0.0.3")}, + }, + expectedAvailableIPs: 16, + expectedAcquiredIPs: 4, + expectedAcquiredPrefixes: 2, + expectedAvailableSmallest: 3, + expectedFreeInterfaceSlots: 6, + expectedAdmissionAllowed: true, + }, + { + name: "pending interfaces without IPs reserve one /31 each", + interfaces: []interfaceFixture{ + {status: InterfaceStatusPending, ipAddress: nil}, + {status: InterfaceStatusPending, ipAddress: nil}, + {status: InterfaceStatusPending, ipAddress: nil}, + }, + expectedAvailableIPs: 16, + expectedAcquiredIPs: 6, + expectedAcquiredPrefixes: 0, + expectedAvailableSmallest: 4, + expectedFreeInterfaceSlots: 5, + expectedAdmissionAllowed: true, + }, + { + name: "mixed duplicate and pending interfaces reserve unique /31s", + interfaces: []interfaceFixture{ + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.1")}, + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.3")}, + {status: InterfaceStatusDeleting, ipAddress: cutil.GetPtr("10.0.0.1")}, + {status: InterfaceStatusPending, ipAddress: nil}, + {status: InterfaceStatusPending, ipAddress: nil}, + }, + expectedAvailableIPs: 16, + expectedAcquiredIPs: 8, + expectedAcquiredPrefixes: 2, + expectedAvailableSmallest: 3, + expectedFreeInterfaceSlots: 4, + expectedAdmissionAllowed: true, + }, + { + name: "usage clamps when acquired and pending interfaces exceed capacity", + interfaces: []interfaceFixture{ + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.1")}, + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.3")}, + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.5")}, + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.7")}, + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.9")}, + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.11")}, + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.13")}, + {status: InterfaceStatusReady, ipAddress: cutil.GetPtr("10.0.0.15")}, + {status: InterfaceStatusPending, ipAddress: nil}, + }, + expectedAvailableIPs: 16, + expectedAcquiredIPs: 16, + expectedAcquiredPrefixes: 8, + expectedAvailableSmallest: 0, + expectedFreeInterfaceSlots: 0, + expectedAdmissionAllowed: false, + }, + } + + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + dbSession := testVpcPrefixInitDB(t) + t.Cleanup(func() { + dbSession.Close() + }) + testInterfaceSetupSchema(t, dbSession) + + infrastructureProvider := testInstanceBuildInfrastructureProvider(t, dbSession, "issue-4908-provider") + site := testInstanceBuildSite(t, dbSession, infrastructureProvider, "issue-4908-site") + tenant := testInstanceBuildTenant(t, dbSession, "issue-4908-tenant") + vpc := testInstanceBuildVpc(t, dbSession, infrastructureProvider, site, tenant, "issue-4908-vpc") + user := testInstanceBuildUser(t, dbSession, "issue-4908-user") + instanceType := testInstanceBuildInstanceType(t, dbSession, infrastructureProvider, "issue-4908-instance-type") + machine := testMachineBuildMachine(t, dbSession, infrastructureProvider.ID, site.ID, &instanceType.ID, cutil.GetPtr("issue-4908-machine-type")) + operatingSystem := testInstanceBuildOperatingSystem(t, dbSession, "issue-4908-os") + instance := TestBuildInstance(t, dbSession, "issue-4908-instance", tenant, infrastructureProvider, site, instanceType, vpc, machine, operatingSystem) + instance.Status = InstanceStatusConfiguring + _, err := dbSession.DB.NewUpdate().Model(instance).Column("status").Where("id = ?", instance.ID).Exec(context.Background()) + require.NoError(t, err) + + vpcPrefix, err := NewVpcPrefixDAO(dbSession).Create(context.Background(), nil, VpcPrefixCreateInput{ + VpcPrefixID: nil, + Name: "issue-4908-prefix", + TenantOrg: tenant.Org, + SiteID: site.ID, + VpcID: vpc.ID, + TenantID: tenant.ID, + IpBlockID: nil, + Prefix: "10.0.0.0/28", + PrefixLength: 28, + Status: VpcPrefixStatusReady, + CreatedBy: user.ID, + }) + require.NoError(t, err) + + for _, interfaceFixture := range testCase.interfaces { + ifc := TestBuildInterface(t, dbSession, instance, nil, &vpcPrefix.ID, true, interfaceFixture.status) + if interfaceFixture.ipAddress != nil { + ifc.IPAddresses = []string{*interfaceFixture.ipAddress} + _, err = dbSession.DB.NewUpdate().Model(ifc).Column("ip_addresses").Where("id = ?", ifc.ID).Exec(context.Background()) + require.NoError(t, err) + } + } + + usageByID, err := NewVpcPrefixDAO(dbSession).GetPrefixUsage(context.Background(), nil, vpcPrefix) + require.NoError(t, err) + + usage := usageByID[vpcPrefix.ID] + require.NotNil(t, usage) + assert.Equal(t, testCase.expectedAvailableIPs, usage.AvailableIPs) + assert.Equal(t, testCase.expectedAcquiredIPs, usage.AcquiredIPs) + assert.Equal(t, testCase.expectedAcquiredPrefixes, usage.AcquiredPrefixes) + assert.Equal(t, testCase.expectedAvailableSmallest, usage.AvailableSmallestPrefixes) + assert.Equal(t, testCase.expectedFreeInterfaceSlots, (usage.AvailableIPs-usage.AcquiredIPs)/vpcPrefixIPsPerInterface) + + admissionAllowed := usage.AcquiredIPs+vpcPrefixIPsPerInterface <= usage.AvailableIPs + assert.Equal(t, testCase.expectedAdmissionAllowed, admissionAllowed) + }) + } +}