Implement DHCP for machine network #53

mcbenjemaa · 2024-01-08T16:08:42Z

THis PR adds:

Support DHCP

api/v1alpha1/helpers.go

* add provider manifests and docs * Update local-providers docs --------- Co-authored-by: Ludwig Bedacht <ludwig.bedacht@ionos.com> Co-authored-by: lubedacht <132355999+lubedacht@users.noreply.github.com>

avorima

First batch of comments. Didn't look at vmservice yet

api/v1alpha1/helpers.go

api/v1alpha1/proxmoxcluster_types.go

api/v1alpha1/proxmoxmachine_types.go

internal/controller/proxmoxcluster_controller.go

api/v1alpha1/proxmoxcluster_types.go

wikkyk · 2024-01-26T14:01:05Z

Rebased and updated copyright headers.

api/v1alpha1/proxmoxcluster_types.go

internal/service/vmservice/bootstrap.go

wikkyk · 2024-01-26T14:11:32Z

This PR should also update the docs as appropriate.

wikkyk

First round of reviews.

.github/workflows/lint.yml

wikkyk · 2024-01-31T10:07:18Z

internal/service/vmservice/ip.go

-func machineHasIPAddress(machine *infrav1alpha1.ProxmoxMachine) bool {
-	return machine.Status.IPAddresses[infrav1alpha1.DefaultNetworkDevice] != (infrav1alpha1.IPAddress{})
+func machineHasIPAddress(machine *infrav1.ProxmoxMachine) bool {
+	return machine.Status.IPAddresses[infrav1.DefaultNetworkDevice] != (infrav1.IPAddress{})
 }

 func handleIPAddressForDevice(ctx context.Context, machineScope *scope.MachineScope, device, format string, ipamRef *corev1.TypedLocalObjectReference) (string, error) {


Can this function be refactored so that it always returns an error when it also returns an empty string? To avoid all the if err != nil || ip == "" {.

It's very confusing to return an error when creating a new IP claim:

cluster-api-provider-proxmox/internal/service/vmservice/ip.go

Lines 94 to 98 in 0cd195b

err = machineScope.IPAMHelper.CreateIPAddressClaim(ctx, machineScope.ProxmoxMachine, device, format, machineScope.InfraCluster.Cluster.GetName(), ipamRef)

if err != nil {

return "", errors.Wrapf(err, "unable to create Ip address claim for machine %s", machineScope.Name())

}

return "", nil

also when trying to add a tag:

cluster-api-provider-proxmox/internal/service/vmservice/ip.go

Lines 112 to 117 in 0cd195b

t, err := machineScope.InfraCluster.ProxmoxClient.TagVM(ctx, vm, ipTag)

if err != nil {

return "", errors.Wrapf(err, "unable to add Ip tag to VirtualMachine %s", machineScope.Name())

}

machineScope.ProxmoxMachine.Status.TaskRef = ptr.To(string(t.UPID))

return "", nil

Another choice is to add a third return as bool, but it's actually the same.

Maybe there should be an error for the "" case that handleIPAddressForDevice would return. Then, the usages where this is an error (i.e. if err != nil || ip == "" {s) could be reduced to the usual if err != nil {. Of course, usages that can handle "" would then need to handle this new error but at least they would have the right context instead of relying on a magic return value.

internal/service/vmservice/vm.go

sonarcloud · 2024-02-02T13:44:10Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

4 New issues
0 Security Hotspots
85.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mcbenjemaa · 2024-02-06T15:08:09Z

I'm ready

avorima

Sorry, for the long wait.

avorima · 2024-02-08T18:14:21Z

docs/Usage.md

-| calico         | templates/cluster-template-calico.yaml          | templates/crs/cni/calico.yaml |
-| multiple-vlans | templates/cluster-template-multiple-vlans.yaml  | -                             |
-| default        | templates/cluster-template.yaml                 | -                             |
+| Flavor         | Tepmlate File                                  | CRS File                      |


typo: Template

avorima · 2024-02-08T18:26:33Z

api/v1alpha1/proxmoxcluster_types_test.go

Ensure that DHCP test cases don't set IPAM fields

avorima · 2024-02-08T18:28:10Z

api/v1alpha1/helpers.go

+// DHCPEnabled returns whether DHCP is enabled.
+func (c ClusterNetworkConfig) DHCPEnabled() bool {
+	switch {
+	case (c.IPv6Config != nil && ptr.Deref(c.IPv6Config.DHCP, false)) && (c.IPv4Config != nil && ptr.Deref(c.IPv4Config.DHCP, false)):
+		return true
+	case (c.IPv6Config != nil && ptr.Deref(c.IPv6Config.DHCP, false)) && c.IPv4Config == nil:
+		return true
+	case (c.IPv4Config != nil && ptr.Deref(c.IPv4Config.DHCP, false)) && c.IPv6Config == nil:
+		return true
+	default:
+		return false
+	}
+}


Isn't this just supposed to do this?

Suggested change

// DHCPEnabled returns whether DHCP is enabled.

func (c ClusterNetworkConfig) DHCPEnabled() bool {

switch {

case (c.IPv6Config != nil && ptr.Deref(c.IPv6Config.DHCP, false)) && (c.IPv4Config != nil && ptr.Deref(c.IPv4Config.DHCP, false)):

return true

case (c.IPv6Config != nil && ptr.Deref(c.IPv6Config.DHCP, false)) && c.IPv4Config == nil:

return true

case (c.IPv4Config != nil && ptr.Deref(c.IPv4Config.DHCP, false)) && c.IPv6Config == nil:

return true

default:

return false

}

}

// DHCPEnabled returns true when DHCP is enabled.

func (c ClusterNetworkConfig) DHCPEnabled() bool {

return c.IPv4Config.DHCPEnabled() || c.IPv6Config.DHCPEnabled()

}

// DHCPEnabled returns true when DHCP is enabled.

func (c *IPConfig) DHCPEnabled() bool {

return c != nil && ptr.Deref(c.DHCP, false)

}

I think this way it's easier to understand and you can also use the IPConfig.DHCPEnabled in the controller and vmservice.

The problem here is we need to check if the cluster has dhcp so we can skip the IPAM pool.

Then, in the machine, we also need to check if the machine has DHCP.

This doesn't contradict my proposition

avorima · 2024-02-08T18:31:10Z

internal/controller/proxmoxcluster_controller_test.go

Please add a test case for DHCP

avorima · 2024-02-08T18:42:32Z

internal/service/vmservice/bootstrap.go

-			if len(nic.DNSServers) != 0 {
-				config.DNSServers = nic.DNSServers
+			if len(nics[i].DNSServers) != 0 {
+				config.DNSServers = nics[i].DNSServers


Either do this:

Suggested change

config.DNSServers = nics[i].DNSServers

conf.DNSServers = nics[i].DNSServers

or pass in the whole AdditionalNetworkDevice into getNetworkConfigDataForDevice to do the override there. Same goes for ipv6 a few lines down.

avorima · 2024-02-08T19:13:03Z

internal/service/vmservice/vm.go

+	if hasDHCPEnabled(scope.InfraCluster.ProxmoxCluster.Spec.IPv4Config,
+		networkSpec.Default,
+		infrav1alpha1.DefaultNetworkDevice,
+		infrav1alpha1.IPV4Format) ||
+		hasDHCPEnabled(scope.InfraCluster.ProxmoxCluster.Spec.IPv6Config,
+			networkSpec.Default,
+			infrav1alpha1.DefaultNetworkDevice,
+			infrav1alpha1.IPV6Format) {


It would actually be less code if you move the default interface handling out of hasDHCPEnabled and use a few intermediate variables, e.g.

if dev := networkSpec.Default; scope.InfraCluster.ProxmoxCluster.Spec.DHCPEnabled() || (dev != nil && dev.DHCP4 || dev.DHCP6) {

But more importantly, why is the default device DHCP setting even considered when DHCP is enabled on the cluster?

avorima · 2024-02-08T19:17:09Z

internal/service/vmservice/vm.go

+				for _, ip := range net.IPAddresses {
+					if ip.IPAddressType == "ipv4" && ip.IPAddress != scope.InfraCluster.ProxmoxCluster.Spec.ControlPlaneEndpoint.Host {
+						ipv4 = ip.IPAddress
+					} else if ip.IPAddressType == "ipv6" && !strings.Contains(ip.IPAddress, "fe80:") {


Please use netip.Addr.IsLinkLocalUnicast. Substring match for IPv6 is always less accurate than comparing the bits.

avorima · 2024-02-08T19:21:56Z

api/v1alpha1/proxmoxmachine_types.go

+	// DHCP4 indicates that if DHCP should be used to assign IPv4 addresses.
+	// DHCP4 enforce device to use DHCP despite the config set in cluster.spec.ipv4Config.
+	// +optional
+	DHCP4 bool `json:"dhcp4,omitempty"`


Why is in the base network device? All permuations of DHCP and IPAM on the default device should be configured in the cluster network config.

This also has the same pointer issue as the cluster dhcp setting.

avorima · 2024-02-08T19:25:47Z

api/v1alpha1/proxmoxcluster_types.go

+	// DHCP indicates that DHCP should be used to assign IP addresses.
+	// mutually exclusive with Addresses.
+	// +optional
+	// +kubebuilder:default=false


This default isn't needed. The behavior stays the same whether it's set to false or null. My pointer request was about making the user-facing API explicit.

avorima · 2024-02-08T19:29:54Z

pkg/proxmox/goproxmox/api_client.go

@@ -245,3 +245,13 @@ func (c *APIClient) StartVM(ctx context.Context, vm *proxmox.VirtualMachine) (*p
 func (c *APIClient) TagVM(ctx context.Context, vm *proxmox.VirtualMachine, tag string) (*proxmox.Task, error) {
 	return vm.AddTag(ctx, tag)
 }
+
+// GetVMNetwork returns a VM network interfaces based on nodeName and vmID.
+func (c *APIClient) GetVMNetwork(ctx context.Context, vm *proxmox.VirtualMachine) (iFaces []*proxmox.AgentNetworkIface, err error) {


nit: named return values are not used and don't provide any additional context

mcbenjemaa self-assigned this Jan 8, 2024

mcbenjemaa requested a review from avorima January 8, 2024 16:09

mcbenjemaa force-pushed the dhcp branch from ebe242e to 9103990 Compare January 9, 2024 11:53

mcbenjemaa requested review from wikkyk and pborn-ionos January 9, 2024 12:08

pborn-ionos reviewed Jan 9, 2024

View reviewed changes

api/v1alpha1/helpers.go Outdated Show resolved Hide resolved

avorima requested changes Jan 18, 2024

View reviewed changes

mcbenjemaa force-pushed the dhcp branch 3 times, most recently from d6ecf9e to 2ca1686 Compare January 23, 2024 15:09

wikkyk force-pushed the main branch from 8cd9d0b to 314477e Compare January 25, 2024 13:48

wikkyk force-pushed the dhcp branch from 2ca1686 to e9d6f82 Compare January 26, 2024 14:00

wikkyk requested changes Jan 26, 2024

View reviewed changes

api/v1alpha1/proxmoxcluster_types.go Outdated Show resolved Hide resolved

internal/service/vmservice/bootstrap.go Outdated Show resolved Hide resolved

mcbenjemaa force-pushed the dhcp branch from e875a35 to ed9dd83 Compare January 29, 2024 12:27

mcbenjemaa had a problem deploying to e2e January 30, 2024 12:21 — with GitHub Actions Failure

mcbenjemaa had a problem deploying to e2e January 30, 2024 12:33 — with GitHub Actions Error

wikkyk added this to the v0.3.0 milestone Jan 30, 2024

mcbenjemaa temporarily deployed to e2e January 30, 2024 12:44 — with GitHub Actions Inactive

mcbenjemaa force-pushed the dhcp branch from 95412df to 9a88f24 Compare January 30, 2024 12:46

mcbenjemaa temporarily deployed to e2e January 30, 2024 12:46 — with GitHub Actions Inactive

mcbenjemaa had a problem deploying to e2e January 30, 2024 14:44 — with GitHub Actions Failure

mcbenjemaa had a problem deploying to e2e January 30, 2024 15:06 — with GitHub Actions Failure

mcbenjemaa force-pushed the dhcp branch from 712adf8 to 8ac9bcc Compare January 30, 2024 15:20

mcbenjemaa temporarily deployed to e2e January 30, 2024 15:20 — with GitHub Actions Inactive

wikkyk requested changes Jan 31, 2024

View reviewed changes

mcbenjemaa temporarily deployed to e2e February 1, 2024 20:22 — with GitHub Actions Inactive

Implement DHCP for machine network

a5d1c3c

mcbenjemaa and others added 14 commits February 2, 2024 14:41

Rebase

6c39227

Fix spell and license

092a8c3

Lint

b2df16d

Address reviews

cca9248

regenerate api deepcopy

049953b

update copyright years

c7f1a89

Fix typos

994d53e

Update manifests

95ada52

Update docs

32fb6c7

Fix gosec linting

29c004e

Add TestReconcileMachineAddresses_DHCP

8fe5237

Format file

5f6a4e2

Change dhcp to pointer

3d4b6c1

Fix linting

9b929d1

mcbenjemaa force-pushed the dhcp branch from 2eddfa8 to 9b929d1 Compare February 2, 2024 13:42

mcbenjemaa temporarily deployed to e2e February 2, 2024 13:42 — with GitHub Actions Inactive

avorima reviewed Feb 8, 2024

View reviewed changes

wikkyk modified the milestones: v0.3.0, v0.4.0 Feb 20, 2024

wikkyk force-pushed the main branch from fd38117 to 72d5796 Compare April 3, 2024 13:22

wikkyk modified the milestones: v0.4.0, v0.5.0 Apr 18, 2024

wikkyk force-pushed the main branch from f16632d to e057b80 Compare April 29, 2024 10:42

wikkyk modified the milestones: v0.5.0, v0.6.0 Jun 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement DHCP for machine network #53

Implement DHCP for machine network #53

mcbenjemaa commented Jan 8, 2024 •

edited

avorima left a comment

wikkyk commented Jan 26, 2024

wikkyk commented Jan 26, 2024

wikkyk left a comment

wikkyk Jan 31, 2024

mcbenjemaa Feb 1, 2024

wikkyk Feb 8, 2024

sonarcloud bot commented Feb 2, 2024

mcbenjemaa commented Feb 6, 2024

avorima left a comment

avorima Feb 8, 2024

avorima Feb 8, 2024

avorima Feb 8, 2024

mcbenjemaa Feb 9, 2024

avorima Feb 9, 2024

avorima Feb 8, 2024

avorima Feb 8, 2024

avorima Feb 8, 2024

avorima Feb 8, 2024

avorima Feb 8, 2024

avorima Feb 8, 2024 •

edited

avorima Feb 8, 2024

	err = machineScope.IPAMHelper.CreateIPAddressClaim(ctx, machineScope.ProxmoxMachine, device, format, machineScope.InfraCluster.Cluster.GetName(), ipamRef)
	if err != nil {
	return "", errors.Wrapf(err, "unable to create Ip address claim for machine %s", machineScope.Name())
	}
	return "", nil

	t, err := machineScope.InfraCluster.ProxmoxClient.TagVM(ctx, vm, ipTag)
	if err != nil {
	return "", errors.Wrapf(err, "unable to add Ip tag to VirtualMachine %s", machineScope.Name())
	}
	machineScope.ProxmoxMachine.Status.TaskRef = ptr.To(string(t.UPID))
	return "", nil

	config.DNSServers = nics[i].DNSServers
	conf.DNSServers = nics[i].DNSServers

Implement DHCP for machine network #53

Are you sure you want to change the base?

Implement DHCP for machine network #53

Conversation

mcbenjemaa commented Jan 8, 2024 • edited

avorima left a comment

Choose a reason for hiding this comment

wikkyk commented Jan 26, 2024

wikkyk commented Jan 26, 2024

wikkyk left a 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

sonarcloud bot commented Feb 2, 2024

Quality Gate passed

mcbenjemaa commented Feb 6, 2024

avorima left a 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

Choose a reason for hiding this comment

avorima Feb 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcbenjemaa commented Jan 8, 2024 •

edited

avorima Feb 8, 2024 •

edited