fix(webhook): check nad labels for associated nad #77

starbops · 2025-12-12T10:06:25Z

IMPORTANT: Please do not create a Pull Request without creating an issue first.

Problem:

If the name of the IPPool CR is not the same as its associated NetworkAttachmentDefinition (NAD) CR, all the VirtualMachineNetworkConfig (vmnetcfg) CR referencing the IPPool cannot be created automatically by the controller.

Solution:

Fix the vmnetcfg validating webhook logic.

Related Issue:

harvester/harvester#9710

Test plan:

Create a single-node v1.7.0 Harvester cluster

Enable the Managed DHCP add-on using the following manifest:

apiVersion: harvesterhci.io/v1beta1
kind: Addon
metadata:
  name: harvester-vm-dhcp-controller
  namespace: harvester-system
  labels:
    addon.harvesterhci.io/experimental: "true"
spec:
  enabled: true
  repo: https://charts.harvesterhci.io
  version: 1.6.0
  chart: harvester-vm-dhcp-controller
  valuesContent: |
    image:
      repository: starbops/harvester-vm-dhcp-controller
      tag: pool-name-hot-fix-head
    webhook:
      image:
        repository: starbops/harvester-vm-dhcp-webhook
        tag: pool-name-hot-fix-head
    agent:
      image:
        repository: starbops/harvester-vm-dhcp-agent
        tag: pool-name-hot-fix-head

Create a Virtual Machine Network (NAD) called test-net

Create an IPPool called test-pool (the name must be different from its associated NAD's name)

apiVersion: network.harvesterhci.io/v1alpha1
kind: IPPool
metadata:
  name: test-pool
spec:
 ipv4Config:
   cidr: 192.168.48.0/24   # please change the cidr accordingly to suit your network environment
 networkName: default/test-net

Create a virtual machine test-vm and attach it to the test-net NAD
The virtual machine should eventually get the allocated IP address (it should match the one reflected in the VirtualMachineNetworkConfig CR)

Signed-off-by: Zespre Chang <[email protected]>

Copilot

Pull request overview

This pull request fixes the VirtualMachineNetworkConfig (vmnetcfg) validating webhook to properly handle scenarios where the IPPool name differs from its associated NetworkAttachmentDefinition (NAD) name. The fix retrieves the IPPool reference from NAD labels instead of assuming the names match.

Key Changes

Modified the webhook validator to read IPPool namespace and name from NAD labels (network.harvesterhci.io/ippool-namespace and network.harvesterhci.io/ippool-name)
Added nadCache dependency to the validator to look up NAD resources
Added comprehensive unit tests covering different naming scenarios

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
pkg/webhook/vmnetcfg/validator.go	Updated validation logic to retrieve IPPool information from NAD labels instead of directly from network name, added NAD cache dependency
pkg/webhook/vmnetcfg/validator_test.go	Added new test file with test cases for IPPool/NAD name matching scenarios and error conditions
cmd/webhook/run.go	Updated validator constructor call to include nadCache parameter

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-29T06:52:15Z

pkg/webhook/vmnetcfg/validator_test.go

+			given: input{
+				vmNetCfg: newTestVirtualMachineNetworkConfigBuilder().
+					WithNetworkConfig("", "", testNetworkName).Build(),
+				ipPool: ippool.NewIPPoolBuilder(testNADNamespace, testNADName).Build(),


The IPPool created in this test is missing the NetworkName configuration. The IPPool should be associated with the network by setting its NetworkName to testNetworkName, similar to how it's done in other tests. Without this, the test doesn't properly reflect the real-world scenario where IPPool references the NAD through its NetworkName field.

Suggested change

ipPool: ippool.NewIPPoolBuilder(testNADNamespace, testNADName).Build(),

ipPool: ippool.NewIPPoolBuilder(testNADNamespace, testNADName).

WithNetworkName(testNetworkName).Build(),

Copilot · 2025-12-29T06:52:16Z

pkg/webhook/vmnetcfg/validator.go

+			return fmt.Errorf(
+				webhook.CreateErr,
+				vmNetCfg.Kind,
+				vmNetCfg.Namespace,
+				vmNetCfg.Name, fmt.Errorf("%s label not found", util.IPPoolNameLabelKey),


The error formatting here has an issue. The fmt.Errorf call wraps another fmt.Errorf as the last argument, but the format string webhook.CreateErr expects a %w verb, not a nested error. This should be simplified to pass the error message string directly or use a properly formatted error. The current code may not produce the expected error message format.

Suggested change

return fmt.Errorf(

webhook.CreateErr,

vmNetCfg.Kind,

vmNetCfg.Namespace,

vmNetCfg.Name, fmt.Errorf("%s label not found", util.IPPoolNameLabelKey),

err := fmt.Errorf("%s label not found", util.IPPoolNameLabelKey)

return fmt.Errorf(

webhook.CreateErr,

vmNetCfg.Kind,

vmNetCfg.Namespace,

vmNetCfg.Name,

err,

Copilot · 2025-12-29T06:52:16Z

pkg/webhook/vmnetcfg/validator.go

+		}
+		ipPoolNamespace, ok := nad.Labels[util.IPPoolNamespaceLabelKey]
+		if !ok {
+			ipPoolNamespace = nadNamespace


There's an inconsistency with how the controller handles the IPPoolNamespaceLabelKey. In the controller's getIPPoolFromNetworkName function, if the IPPoolNamespaceLabelKey label is missing, it returns an error. However, in this validator, if the label is missing, it defaults to nadNamespace. This inconsistency could lead to the validator accepting configurations that the controller will reject. The validator should either match the controller's strict behavior or the controller should adopt this more lenient approach.

Suggested change

ipPoolNamespace = nadNamespace

return fmt.Errorf(

webhook.CreateErr,

vmNetCfg.Kind,

vmNetCfg.Namespace,

vmNetCfg.Name, fmt.Errorf("%s label not found", util.IPPoolNamespaceLabelKey),

)

do we support nad and ipool in different namespaces or does it have to be in same namespace. Do we restrict this somewhere ? Any reason behind using nad namespace as ipool namespace in case of namespace does not exist.
Can we return error instead ?

do we support nad and ipool in different namespaces or does it have to be in same namespace. Do we restrict this somewhere ?

yes we currently support cross-namespace referencing between nads and ippools. but i gradually realize that this shouldn't be allowed. restricting ippools that can only reference the same namespace nads should be the norm. however, this will be a breaking change for users who have already done so (cross-namespace referencing), and need to be planned carefully.

Any reason behind using nad namespace as ipool namespace in case of namespace does not exist.
Can we return error instead ?

sure we can return an error here. i initially thought it could be handy to set it to the nad's namespace if the namespace label on the nad is missing.

Copilot · 2025-12-29T06:52:16Z

pkg/webhook/vmnetcfg/validator_test.go

+				shouldErr: true,
+			},
+		},
+	}


The test suite is missing a test case for when a NAD exists but lacks the required IPPoolNameLabelKey label. This is an important validation scenario that should be tested to ensure the validator properly rejects VirtualMachineNetworkConfig creation when the NAD is not properly labeled. Consider adding a test case similar to the existing ones that validates this error path.

Copilot · 2025-12-29T06:52:17Z

pkg/webhook/vmnetcfg/validator_test.go

+			given: input{
+				vmNetCfg: newTestVirtualMachineNetworkConfigBuilder().
+					WithNetworkConfig("", "", testNetworkName).Build(),
+				ipPool: newTestIPPoolBuilder().Build(),


The IPPool created in this test is missing the NetworkName configuration. The IPPool should be associated with the network by setting its NetworkName to testNetworkName, similar to how it's done in other tests. Without this, the test doesn't properly reflect the real-world scenario where IPPool references the NAD through its NetworkName field.

Copilot · 2025-12-29T06:52:17Z

pkg/webhook/vmnetcfg/validator_test.go

+			given: input{
+				vmNetCfg: newTestVirtualMachineNetworkConfigBuilder().
+					WithNetworkConfig("", "", testNetworkName).Build(),
+				ipPool: newTestIPPoolBuilder().Build(),


The IPPool created in this test is missing the NetworkName configuration. The IPPool should be associated with the network by setting its NetworkName to testNetworkName. Without this, the test doesn't properly reflect the real-world scenario where IPPool references the NAD through its NetworkName field.

Signed-off-by: Zespre Chang <[email protected]>

rrajendran17 · 2026-01-09T00:49:03Z

LGTM,Thanks

ibrokethecloud

lgtm. thanks for the fix.

starbops · 2026-01-13T10:10:32Z

@mergify backport v1.7

mergify · 2026-01-13T10:10:41Z

backport v1.7

✅ Backports have been created

Details

#80 fix(webhook): check nad labels for associated nad (backport #77) has been created for branch v1.7

fix(webhook): check nad labels for associated nad

f8def6b

Signed-off-by: Zespre Chang <[email protected]>

starbops marked this pull request as ready for review December 29, 2025 06:44

Copilot AI review requested due to automatic review settings December 29, 2025 06:44

Copilot started reviewing on behalf of starbops December 29, 2025 06:44 View session

starbops requested review from mingshuoqiu and rrajendran17 December 29, 2025 06:45

starbops self-assigned this Dec 29, 2025

Copilot AI reviewed Dec 29, 2025

View reviewed changes

starbops mentioned this pull request Dec 29, 2025

[BUG] VMs cannot get IPs from vm-dhcp-controller when names differ in IPPool and NAD harvester/harvester#9710

Open

starbops force-pushed the pool-name-hot-fix branch from cf14971 to c8f1130 Compare December 29, 2025 07:35

test(webhook): add vmnetcfg validator tests

ea6c375

Signed-off-by: Zespre Chang <[email protected]>

starbops force-pushed the pool-name-hot-fix branch from c8f1130 to ea6c375 Compare January 6, 2026 16:32

fix(webhook): return err when the ns label on nad is missing

0718ec5

Signed-off-by: Zespre Chang <[email protected]>

rrajendran17 approved these changes Jan 9, 2026

View reviewed changes

starbops requested a review from ibrokethecloud January 9, 2026 08:30

ibrokethecloud approved these changes Jan 13, 2026

View reviewed changes

starbops merged commit 54275d2 into harvester:main Jan 13, 2026
7 checks passed

mergify bot mentioned this pull request Jan 13, 2026

fix(webhook): check nad labels for associated nad (backport #77) #80

Merged

	ipPool: ippool.NewIPPoolBuilder(testNADNamespace, testNADName).Build(),
	ipPool: ippool.NewIPPoolBuilder(testNADNamespace, testNADName).
	WithNetworkName(testNetworkName).Build(),

fix(webhook): check nad labels for associated nad #77

fix(webhook): check nad labels for associated nad #77

Uh oh!

Conversation

starbops commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

rrajendran17 Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

starbops Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

rrajendran17 commented Jan 9, 2026

Uh oh!

ibrokethecloud left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

starbops commented Jan 13, 2026

Uh oh!

mergify bot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

starbops commented Dec 12, 2025 •

edited

Loading

mergify bot commented Jan 13, 2026 •

edited

Loading