Dualstack Network Support #549

majst01 · 2024-07-17T13:57:51Z

Closes #164

Actions Required

The ChildPrefixLength property was removed from the Partition Entity. Every TenantSupernetwork must now be configured with ChildPrefixLength accordingly.
This is done during startup in the database migration step and set to the same value as in the `Partition`.

It is required to add a defaultchildprefixlength property to the tenant super network in you configuration because otherwise the initial network creation will fail:
Sample:
  defaultchildprefixlength:
    IPv4: 22

Also ensure that all firewall-controllers are up-to-date in all firewalls before adding ipv6 prefixes to any of the exiting networks.

Depends on:

replaces #544

TODO:

add possibility to create machines in different networks like internet and tenant-network, but only from tenant-network IPs from both AF are acquired, but only ipv4 is possible from internet even if dualstack

…o ipv6-support

…o dualstack-support

cmd/metal-api/internal/service/network-service.go

…o dualstack-support

cmd/metal-api/internal/service/network-service.go

Gerrit91 · 2025-01-28T14:13:48Z

cmd/metal-api/internal/datastore/ip.go

@@ -79,6 +80,21 @@ func (p *IPSearchQuery) generateTerm(rs *RethinkStore) *r.Term {
 		})
 	}

+	if p.AddressFamily != nil {
+		separator := "."


Suggested change

separator := "."

var separator string

Gerrit91 · 2025-01-28T14:17:58Z

cmd/metal-api/internal/datastore/migrations/08_childprefixlength.go

+				if parsed.Addr().Is4() {
+					af = metal.IPv4AddressFamily
+					defaultChildPrefixLength[af] = 22
+				}
+				if parsed.Addr().Is6() {
+					af = metal.IPv6AddressFamily
+					defaultChildPrefixLength[af] = 64
+				}
+
+				if new.AddressFamilies == nil {
+					new.AddressFamilies = metal.AddressFamilies{}
+				}
+				new.AddressFamilies = metal.AddressFamilies{af}


This is only required for private super networks and for those it's probably enough just to take the value from the partition for IPv4 and 64 for IPv6.

Gerrit91 · 2025-01-28T14:18:25Z

cmd/metal-api/internal/datastore/network.go

@@ -154,6 +155,25 @@ func (p *NetworkSearchQuery) generateTerm(rs *RethinkStore) (*r.Term, error) {
 		})
 	}

+	if p.AddressFamily != nil {
+		separator := "."


Suggested change

separator := "."

var separator string

Gerrit91 · 2025-01-28T14:20:21Z

cmd/metal-api/internal/datastore/network.go

+			separator = ":"
+		}
+
+		fmt.Printf("Separator:%s\n", separator)


Suggested change

fmt.Printf("Separator:%s\n", separator)

Gerrit91 · 2025-01-28T14:24:09Z

cmd/metal-api/internal/ipam/ipam.go

+	if pfx.Addr().Is6() {
+		af = metal.IPv6AddressFamily
+	}
+	availableIPs := metal.AddressFamilyUsage{
+		af: usage.Msg.AvailableIps,
+	}
+	usedIPs := metal.AddressFamilyUsage{
+		af: usage.Msg.AcquiredIps,
+	}
+	availablePrefixes := metal.AddressFamilyUsage{
+		af: usage.Msg.AvailableSmallestPrefixes,
+	}
+	usedPrefixes := metal.AddressFamilyUsage{
+		af: usage.Msg.AcquiredPrefixes,
+	}


As this is always the same address family and the map will never have different ones it might be okay to just return the usage without address family and handle in the caller where to put it

Gerrit91 · 2025-01-28T14:49:52Z

cmd/metal-api/internal/service/ip-service_test.go

@@ -258,6 +286,49 @@ func TestAllocateIP(t *testing.T) {
 			wantedStatus: http.StatusUnprocessableEntity,
 			wantErr:      errors.New("specific ip not contained in any of the defined prefixes"),
 		},
+		{
+			name: "allocate a IPv4 address",


Suggested change

name: "allocate a IPv4 address",

name: "allocate an IPv4 address",

Gerrit91 · 2025-01-28T14:50:08Z

cmd/metal-api/internal/service/ip-service_test.go

+			wantedStatus: http.StatusCreated,
+		},
+		{
+			name: "allocate a IPv6 address",


Suggested change

name: "allocate a IPv6 address",

name: "allocate an IPv6 address",

Gerrit91 · 2025-01-28T14:50:26Z

cmd/metal-api/internal/service/ip-service_test.go

+			wantErr:      errors.New("there is no prefix for the given addressfamily:IPv6 present in network:4"),
+		},
+		{
+			name: "allocate a IPv4 (no addressfamily specified) address from a IPv6 Only network",


Suggested change

name: "allocate a IPv4 (no addressfamily specified) address from a IPv6 Only network",

name: "allocate an IPv4 (no addressfamily specified) address from an IPv6 only network",

Gerrit91 · 2025-01-28T14:54:26Z

cmd/metal-api/internal/service/machine-service.go

+		if n == nil || n.network == nil {
+			continue
+		}
+		if len(n.network.AddressFamilies) == 0 {


I don't know if this is possible but can this be dynamically evaluated where the network was retrieved?

Gerrit91 · 2025-01-28T15:10:50Z

cmd/metal-api/internal/service/network-service.go

+		// We should support two private super per partition, one per addressfamily
+		// the network allocate request must be configurable with addressfamily


Suggested change

// We should support two private super per partition, one per addressfamily

// the network allocate request must be configurable with addressfamily

Gerrit91 · 2025-01-28T15:11:40Z

cmd/metal-api/internal/service/network-service.go

+				PartitionID:  &partition.ID,
+				PrivateSuper: pointer.Pointer(true),
+			}, &nw)
+			r.log.Info("createnetwork", "found", nw)


Suggested change

r.log.Info("createnetwork", "found", nw)

Gerrit91 · 2025-01-28T15:13:07Z

cmd/metal-api/internal/service/network-service.go

@@ -439,6 +456,79 @@ func validateAdditionalAnnouncableCIDRs(additionalCidrs []string, privateSuper b
 	return nil
 }

+func validatePrefixesAndAddressFamilies(prefixes, destinationPrefixes []string, defaultChildPrefixLength metal.ChildPrefixLength, privateSuper bool) (metal.Prefixes, metal.Prefixes, metal.AddressFamilies, error) {


Function signature does not imply the return values. Could be better to split validation functions from parsing functions to increase reusability.

Gerrit91 · 2025-01-28T15:14:36Z

cmd/metal-api/internal/service/network-service.go

+	if err != nil {
+		return nil, nil, nil, err
+	}
+	// all DestinationPrefixes must be valid and from the same addressfamily


Is this still correct? Because I see different address families in the destination prefixes in our test environment.

Gerrit91 · 2025-01-28T15:16:05Z

cmd/metal-api/internal/service/network-service.go

+	if err != nil {
+		return nil, nil, nil, err
+	}
+	if len(destinationAddressFamilies) > len(addressFamilies) {


Maybe this check is insufficient, e.g. there are IPv6 destination prefixes but only IPv4 prefixes.

Gerrit91 · 2025-01-28T15:17:35Z

cmd/metal-api/internal/service/network-service.go

+	}
+
+	for af, length := range defaultChildPrefixLength {
+		fmt.Printf("af %s length:%d addressfamilies:%v", af, length, addressFamilies)


Suggested change

fmt.Printf("af %s length:%d addressfamilies:%v", af, length, addressFamilies)

Gerrit91 · 2025-01-28T15:18:43Z

cmd/metal-api/internal/service/network-service.go

-
-		prefixes = append(prefixes, *prefix)
+	var childPrefixLength = metal.ChildPrefixLength{}
+	for af, length := range requestPayload.DefaultChildPrefixLength {


Either this needs only to be done for private super or the validation must be extended to validate this field for non-private super networks, too.

Gerrit91 · 2025-01-28T15:26:59Z

cmd/metal-api/internal/service/network-service.go

+		return
+	}
+	if len(superNetwork.DefaultChildPrefixLength) == 0 {
+		r.sendError(request, response, httperrors.BadRequest(fmt.Errorf("supernetwork %s has no defaultchildprefixlength specified", superNetwork.ID)))


Should be an internal server error but I am not sure if we need these validation because we ensure this somewhere else in the code?

Gerrit91 · 2025-01-28T15:43:29Z

cmd/metal-api/internal/service/network-service.go

+		return nil, nil, nil, fmt.Errorf("private super network must always contain a defaultchildprefixlength")
+	}
+
+	for af, length := range defaultChildPrefixLength {


It would also be good to check if the address family is actually IPv4 or IPv6.

Gerrit91 · 2025-01-28T15:45:35Z

cmd/metal-api/internal/service/network-service.go

+		r.sendError(request, response, httperrors.BadRequest(fmt.Errorf("supernetwork %s has no defaultchildprefixlength specified", superNetwork.ID)))
+		return
+	}
+	r.log.Info("network allocate", "supernetwork", superNetwork.ID)


Log appears later in the code again

Gerrit91 · 2025-01-28T15:47:16Z

cmd/metal-api/internal/service/network-service.go

+			return nil, err
+		}
+		if childPrefix == nil {
+			return nil, fmt.Errorf("could not allocate child prefix in parent network: %s for addressfamily: %s length:%d", parent.ID, af, childLength)


Hard to mitigate this but this is a potential leakage in case one child prefix could already be allocated.

cmd/metal-api/internal/service/network-service.go

Gerrit91 · 2025-01-29T14:46:00Z

cmd/metal-api/internal/metal/network.go

+)
+
+// ToAddressFamily will convert a string af to a AddressFamily
+func ToAddressFamily(af string) AddressFamily {


When returning an error with this function it would be helpful for validations because currently users can often try to send random stuff to the API and try to do something weird.

Implemented ValidateAddressfamily as well

majst01 added 6 commits July 4, 2024 09:08

IPv6 Support

5ea7ea6

More tests

22d916e

Validate Prefixes and DestinationPrefixes on create and update network

e8571c9

Constify defaultChildPrefixlength

59c7511

Merge branch 'master' of https://github.com/metal-stack/metal-api int…

9f52dfd

…o ipv6-support

Merge branch 'master' of https://github.com/metal-stack/metal-api int…

89a0857

…o ipv6-support

majst01 force-pushed the dualstack-support branch 4 times, most recently from 5919c8f to 0c53831 Compare July 23, 2024 07:47

Support DualStack Networks

aae9274

majst01 force-pushed the dualstack-support branch from 0c53831 to 2389965 Compare July 23, 2024 11:48

Gerrit91 mentioned this pull request Jul 23, 2024

IPv6 Support with single stack networks #544

Draft

8 tasks

Gerrit91 assigned majst01 Jul 23, 2024

Merge branch 'master' of https://github.com/metal-stack/metal-api int…

1b09d5d

…o dualstack-support

majst01 force-pushed the dualstack-support branch 7 times, most recently from d49d2c0 to b65c835 Compare July 25, 2024 09:50

majst01 commented Aug 1, 2024

View reviewed changes

cmd/metal-api/internal/service/network-service.go Outdated Show resolved Hide resolved

majst01 force-pushed the dualstack-support branch 3 times, most recently from 23a3d1e to 89096c0 Compare August 2, 2024 12:40

majst01 mentioned this pull request Aug 2, 2024

Remove hardcoded /32 bitmask for some switch entities #555

Merged

Merge branch 'master' of https://github.com/metal-stack/metal-api int…

87a2a98

…o dualstack-support

majst01 force-pushed the dualstack-support branch 2 times, most recently from a3571a2 to d9667d3 Compare August 5, 2024 07:13

iljarotar reviewed Jan 24, 2025

View reviewed changes

cmd/metal-api/internal/service/network-service.go Outdated Show resolved Hide resolved

majst01 added 2 commits January 24, 2025 13:08

Fix typo

9ee694a

Update deps

d964c1b

Gerrit91 reviewed Jan 28, 2025

View reviewed changes

cmd/metal-api/internal/service/network-service.go Outdated Show resolved Hide resolved

Gerrit91 reviewed Jan 29, 2025

View reviewed changes

Gerrit91 and others added 5 commits January 29, 2025 16:13

Introduce helper function.

04a84e5

Fix ip query

e38db83

First bunch of review findings

60c9d8a

Small fix

0d64c98

Validate Addressfamily

ff87efa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dualstack Network Support #549

Dualstack Network Support #549

majst01 commented Jul 17, 2024 •

edited

Loading

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025 •

edited

Loading

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 28, 2025

Gerrit91 Jan 29, 2025

majst01 Jan 30, 2025

	name: "allocate a IPv4 address",
	name: "allocate an IPv4 address",

	name: "allocate a IPv6 address",
	name: "allocate an IPv6 address",

	name: "allocate a IPv4 (no addressfamily specified) address from a IPv6 Only network",
	name: "allocate an IPv4 (no addressfamily specified) address from an IPv6 only network",

		// We should support two private super per partition, one per addressfamily
		// the network allocate request must be configurable with addressfamily

Dualstack Network Support #549

Are you sure you want to change the base?

Dualstack Network Support #549

Conversation

majst01 commented Jul 17, 2024 • edited Loading

Actions Required

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gerrit91 Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majst01 commented Jul 17, 2024 •

edited

Loading

Gerrit91 Jan 28, 2025 •

edited

Loading