Skip to content

fix: Reject reserved IP addresses on interface update #223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fabiendupont wants to merge 1 commit into
base: main
Choose a base branch
from
+363 −1
Open

fix: Reject reserved IP addresses on interface update #223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions db/pkg/db/ipam/ipam.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (

cipam "github.com/nvidia/bare-metal-manager-rest/ipam"
"github.com/uptrace/bun"
"go4.org/netipx"

cdb "github.com/nvidia/bare-metal-manager-rest/db/pkg/db"
cdbm "github.com/nvidia/bare-metal-manager-rest/db/pkg/db/model"
Expand Down Expand Up @@ -242,3 +243,87 @@ func DeleteChildIpamEntryFromCidr(ctx context.Context, tx *cdb.Tx, dbSession *cd
}
return nil
}

// ValidateIPAddresses checks that none of the given IP addresses are reserved
// network addresses (network address, broadcast address, or gateway) for the
// specified CIDR prefix. It returns the list of valid IPs and the list of
// rejected IPs with reasons.
func ValidateIPAddresses(ipAddresses []string, cidr string, gateway string) (valid []string, rejected []string) {
if len(ipAddresses) == 0 {
return ipAddresses, nil
}

prefix, err := netip.ParsePrefix(cidr)
if err != nil {
// If we can't parse the prefix, we can't validate — pass through
return ipAddresses, nil
}

iprange := netipx.RangeOfPrefix(prefix)
networkAddr := iprange.From()
broadcastAddr := iprange.To()

var gatewayAddr netip.Addr
if gateway != "" {
gatewayAddr, _ = netip.ParseAddr(gateway)
}

for _, ipStr := range ipAddresses {
addr, err := netip.ParseAddr(ipStr)
if err != nil {
rejected = append(rejected, fmt.Sprintf("%s (invalid IP)", ipStr))
continue
}

if !prefix.Contains(addr) {
rejected = append(rejected, fmt.Sprintf("%s (not in prefix %s)", ipStr, cidr))
continue
}

if addr == networkAddr {
rejected = append(rejected, fmt.Sprintf("%s (network address of %s)", ipStr, cidr))
continue
}

if prefix.Addr().Is4() && addr == broadcastAddr {
rejected = append(rejected, fmt.Sprintf("%s (broadcast address of %s)", ipStr, cidr))
continue
}

if gatewayAddr.IsValid() && addr == gatewayAddr {
rejected = append(rejected, fmt.Sprintf("%s (gateway address of %s)", ipStr, cidr))
continue
}

valid = append(valid, ipStr)
}

return valid, rejected
}

// GetInterfaceCIDRAndGateway returns the CIDR and gateway for an interface
// based on its associated Subnet or VpcPrefix.
func GetInterfaceCIDRAndGateway(ifc *cdbm.Interface) (cidr string, gateway string) {
if ifc.Subnet != nil {
if ifc.Subnet.IPv4Prefix != nil {
cidr = fmt.Sprintf("%s/%d", *ifc.Subnet.IPv4Prefix, ifc.Subnet.PrefixLength)
if ifc.Subnet.IPv4Gateway != nil {
gateway = *ifc.Subnet.IPv4Gateway
}
} else if ifc.Subnet.IPv6Prefix != nil {
cidr = fmt.Sprintf("%s/%d", *ifc.Subnet.IPv6Prefix, ifc.Subnet.PrefixLength)
if ifc.Subnet.IPv6Gateway != nil {
gateway = *ifc.Subnet.IPv6Gateway
}
}
} else if ifc.VpcPrefix != nil {
// VpcPrefix.Prefix may already contain the prefix length (e.g. "192.172.0.0/24")
// from the IPAM library's Cidr format. Parse it to extract just the address.
if p, err := netip.ParsePrefix(ifc.VpcPrefix.Prefix); err == nil {
cidr = p.String()
} else {
cidr = fmt.Sprintf("%s/%d", ifc.VpcPrefix.Prefix, ifc.VpcPrefix.PrefixLength)
}
}
return cidr, gateway
}
183 changes: 183 additions & 0 deletions db/pkg/db/ipam/ipam_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,3 +846,186 @@ func TestGetFirstIPFromCidr(t *testing.T) {
})
}
}

func TestValidateIPAddresses(t *testing.T) {
tests := []struct {
name string
ipAddresses []string
cidr string
gateway string
expectedValid []string
expectedRejected []string
}{
{
name: "all valid IPs in /24",
ipAddresses: []string{"192.168.1.10", "192.168.1.20"},
cidr: "192.168.1.0/24",
gateway: "192.168.1.1",
expectedValid: []string{"192.168.1.10", "192.168.1.20"},
expectedRejected: nil,
},
{
name: "reject network address",
ipAddresses: []string{"192.168.1.0", "192.168.1.10"},
cidr: "192.168.1.0/24",
gateway: "",
expectedValid: []string{"192.168.1.10"},
expectedRejected: []string{"192.168.1.0 (network address of 192.168.1.0/24)"},
},
{
name: "reject broadcast address",
ipAddresses: []string{"192.168.1.255", "192.168.1.10"},
cidr: "192.168.1.0/24",
gateway: "",
expectedValid: []string{"192.168.1.10"},
expectedRejected: []string{"192.168.1.255 (broadcast address of 192.168.1.0/24)"},
},
{
name: "reject gateway address",
ipAddresses: []string{"192.168.1.1", "192.168.1.10"},
cidr: "192.168.1.0/24",
gateway: "192.168.1.1",
expectedValid: []string{"192.168.1.10"},
expectedRejected: []string{"192.168.1.1 (gateway address of 192.168.1.0/24)"},
},
{
name: "reject all reserved addresses at once",
ipAddresses: []string{"192.168.1.0", "192.168.1.1", "192.168.1.255", "192.168.1.100"},
cidr: "192.168.1.0/24",
gateway: "192.168.1.1",
expectedValid: []string{"192.168.1.100"},
expectedRejected: []string{"192.168.1.0 (network address of 192.168.1.0/24)", "192.168.1.1 (gateway address of 192.168.1.0/24)", "192.168.1.255 (broadcast address of 192.168.1.0/24)"},
},
{
name: "reject IP outside prefix",
ipAddresses: []string{"10.0.0.1", "192.168.1.10"},
cidr: "192.168.1.0/24",
gateway: "",
expectedValid: []string{"192.168.1.10"},
expectedRejected: []string{"10.0.0.1 (not in prefix 192.168.1.0/24)"},
},
{
name: "reject invalid IP string",
ipAddresses: []string{"not-an-ip", "192.168.1.10"},
cidr: "192.168.1.0/24",
gateway: "",
expectedValid: []string{"192.168.1.10"},
expectedRejected: []string{"not-an-ip (invalid IP)"},
},
{
name: "small /30 prefix - reject network and broadcast",
ipAddresses: []string{"10.0.0.0", "10.0.0.1", "10.0.0.2", "10.0.0.3"},
cidr: "10.0.0.0/30",
gateway: "10.0.0.1",
expectedValid: []string{"10.0.0.2"},
expectedRejected: []string{"10.0.0.0 (network address of 10.0.0.0/30)", "10.0.0.1 (gateway address of 10.0.0.0/30)", "10.0.0.3 (broadcast address of 10.0.0.0/30)"},
},
{
name: "IPv6 - reject network address only (no broadcast in IPv6)",
ipAddresses: []string{"2001:db8::", "2001:db8::1", "2001:db8::f"},
cidr: "2001:db8::/124",
gateway: "",
expectedValid: []string{"2001:db8::1", "2001:db8::f"},
expectedRejected: []string{"2001:db8:: (network address of 2001:db8::/124)"},
},
{
name: "empty IP list",
ipAddresses: []string{},
cidr: "192.168.1.0/24",
gateway: "",
expectedValid: []string{},
expectedRejected: nil,
},
{
name: "invalid CIDR passes through",
ipAddresses: []string{"192.168.1.10"},
cidr: "bad-cidr",
gateway: "",
expectedValid: []string{"192.168.1.10"},
expectedRejected: nil,
},
{
name: "no gateway - only network and broadcast rejected",
ipAddresses: []string{"192.168.1.0", "192.168.1.1", "192.168.1.255"},
cidr: "192.168.1.0/24",
gateway: "",
expectedValid: []string{"192.168.1.1"},
expectedRejected: []string{"192.168.1.0 (network address of 192.168.1.0/24)", "192.168.1.255 (broadcast address of 192.168.1.0/24)"},
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
valid, rejected := ValidateIPAddresses(tc.ipAddresses, tc.cidr, tc.gateway)
assert.Equal(t, tc.expectedValid, valid)
assert.Equal(t, tc.expectedRejected, rejected)
})
}
}

func TestGetInterfaceCIDRAndGateway(t *testing.T) {
ipv4Prefix := "192.168.1.0"
ipv4Gateway := "192.168.1.1"
ipv6Prefix := "2001:db8::"
ipv6Gateway := "2001:db8::1"

t.Run("subnet interface with IPv4", func(t *testing.T) {
ifc := &cdbm.Interface{
Subnet: &cdbm.Subnet{
IPv4Prefix: &ipv4Prefix,
PrefixLength: 24,
IPv4Gateway: &ipv4Gateway,
},
}
cidr, gw := GetInterfaceCIDRAndGateway(ifc)
assert.Equal(t, "192.168.1.0/24", cidr)
assert.Equal(t, "192.168.1.1", gw)
})

t.Run("subnet interface with IPv6 only", func(t *testing.T) {
ifc := &cdbm.Interface{
Subnet: &cdbm.Subnet{
IPv6Prefix: &ipv6Prefix,
PrefixLength: 64,
IPv6Gateway: &ipv6Gateway,
},
}
cidr, gw := GetInterfaceCIDRAndGateway(ifc)
assert.Equal(t, "2001:db8::/64", cidr)
assert.Equal(t, "2001:db8::1", gw)
})

t.Run("vpc prefix interface with CIDR format", func(t *testing.T) {
// In production, VpcPrefix.Prefix is set from childPrefix.Cidr which
// includes the prefix length (e.g. "10.0.0.0/16")
ifc := &cdbm.Interface{
VpcPrefix: &cdbm.VpcPrefix{
Prefix: "10.0.0.0/16",
PrefixLength: 16,
},
}
cidr, gw := GetInterfaceCIDRAndGateway(ifc)
assert.Equal(t, "10.0.0.0/16", cidr)
assert.Equal(t, "", gw)
})

t.Run("vpc prefix interface with bare IP format", func(t *testing.T) {
// Fallback: if Prefix is just an IP without prefix length
ifc := &cdbm.Interface{
VpcPrefix: &cdbm.VpcPrefix{
Prefix: "10.0.0.0",
PrefixLength: 16,
},
}
cidr, gw := GetInterfaceCIDRAndGateway(ifc)
assert.Equal(t, "10.0.0.0/16", cidr)
assert.Equal(t, "", gw)
})

t.Run("no subnet or vpc prefix", func(t *testing.T) {
ifc := &cdbm.Interface{}
cidr, gw := GetInterfaceCIDRAndGateway(ifc)
assert.Equal(t, "", cidr)
assert.Equal(t, "", gw)
})
}
18 changes: 17 additions & 1 deletion workflow/pkg/activity/instance/instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"go.temporal.io/sdk/client"

cdb "github.com/nvidia/bare-metal-manager-rest/db/pkg/db"
"github.com/nvidia/bare-metal-manager-rest/db/pkg/db/ipam"
cdbm "github.com/nvidia/bare-metal-manager-rest/db/pkg/db/model"
"github.com/nvidia/bare-metal-manager-rest/db/pkg/db/paginator"
cdbp "github.com/nvidia/bare-metal-manager-rest/db/pkg/db/paginator"
Expand Down Expand Up @@ -1041,6 +1042,17 @@ func (mi ManageInstance) UpdateInstancesInDB(ctx context.Context, siteID uuid.UU
ipAddresses := []string{}
ipAddresses = append(ipAddresses, interfaceStatus.Addresses...)

// Validate IP addresses against the interface's prefix to reject
// network, broadcast, and gateway addresses
var ipValidationFailed bool
if cidr, gateway := ipam.GetInterfaceCIDRAndGateway(ifc); cidr != "" {
_, rejectedIPs := ipam.ValidateIPAddresses(ipAddresses, cidr, gateway)
if len(rejectedIPs) > 0 {
slogger.Error().Strs("rejected_ips", rejectedIPs).Str("Interface ID", ifc.ID.String()).Msg("interface has reserved IP addresses assigned by site controller")
ipValidationFailed = true
}
}

// Update device and device_instance if specified in the inventory
var device *string
var deviceInstance *int
Expand All @@ -1052,7 +1064,11 @@ func (mi ManageInstance) UpdateInstancesInDB(ctx context.Context, siteID uuid.UU
}

var status *string
if controllerInstance.Status.Network.ConfigsSynced == cwsv1.SyncState_SYNCED {
if ipValidationFailed {
status = cdb.GetStrPtr(cdbm.InterfaceStatusError)
// Do not persist invalid IP addresses
ipAddresses = nil
Comment on lines +1069 to +1070
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code sets ipAddresses = nil at line 1070 to prevent persisting invalid IP addresses, then passes it as IpAddresses: ipAddresses (i.e., nil) to interfaceDAO.Update. However, in db/pkg/db/model/interface.go (around line 422), the update logic only writes the ip_addresses column when input.IpAddresses != nil. Therefore, when a previously valid interface later receives a reserved IP, any IP addresses already stored in the database will remain — only the status changes to Error. This contradicts the comment // Do not persist invalid IP addresses and the test assertion's intent.

To actually clear the stored IPs, the update logic needs a way to explicitly set ip_addresses to an empty/null value. One approach is to use an empty slice ([]string{}) instead of nil and update the DAO to treat an empty (non-nil) slice as a clear operation, or add a separate ClearIPAddresses bool field to InterfaceUpdateInput. As-is, the self-healing behavior on the next cycle may overwrite the stale IPs with a valid set anyway, but the current cycle leaves stale IPs in an Error-status interface.

Suggested change
// Do not persist invalid IP addresses
ipAddresses = nil
// Do not persist invalid IP addresses: clear any stored IPs
ipAddresses = []string{}

Copilot uses AI. Check for mistakes.
} else if controllerInstance.Status.Network.ConfigsSynced == cwsv1.SyncState_SYNCED {
status = cdb.GetStrPtr(cdbm.InterfaceStatusReady)
}

Expand Down
Loading
Loading