Skip to content

Conversation

@votdev
Copy link
Member

@votdev votdev commented Feb 11, 2026

Problem:
Storage-network NAD can be deleted from kubectl directly.

Solution:
Deny deletion of a NAD if it is used for storage-network.

Related Issue:
harvester/harvester#9623

Test plan:
Case 1

  • Go to the Harvester settings page and edit storage-network. You may use the following settings:
Enabled: Yes
Type: L2VLanNetwork
VLAN ID: 100
Cluster Network: mgmt
IP Range: 192.168.0.0/24
Exclude IPs: 192.168.0.100/32

Note, all VMs need to be stopped to apply the settings.

  • List existing NAD. In our case there is only the new created storage-network NAD.
$ k get network-attachment-definitions.k8s.cni.cncf.io -n harvester-system                           
NAME                   AGE
storagenetwork-5g5mn   18h
  • Try to delete it:
$ k delete network-attachment-definitions.k8s.cni.cncf.io -n harvester-system storagenetwork-5g5mn
Error from server (InternalError): admission webhook "validator.harvester-system.harvester-network-webhook" denied the request: Internal error occurred: can't delete nad harvester-system/storagenetwork-5g5mn because it is used by storagenetwork

The resource is not allowed to be deleted.
Case 2

  • Make sure a storage network is configured (see case 1).
  • Go to the Harvester settings page and edit storage-network.
  • Click the Use the default value button. The Disabled checkbox is selected. Click Save.
  • Delete the NAD:
$ k delete network-attachment-definitions.k8s.cni.cncf.io -n harvester-system storagenetwork-5g5mn

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the NAD validating webhook to prevent deletion of NetworkAttachmentDefinitions that are used for Harvester’s storage network, addressing harvester/harvester#9623.

Changes:

  • Add a storage-network check to the NAD delete admission path to deny deletion of storage-network NADs.
  • Fix the delete path to wrap NAD config decode errors with the correct delete-specific error format.
  • Extend delete validator unit tests to cover storage-network NAD deletion denial (annotation-based and prefix-based).

Reviewed changes

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

File Description
pkg/webhook/nad/validator.go Adds storage-network deletion protection and corrects delete error wrapping for config decode failures.
pkg/webhook/nad/validator_test.go Adds new delete test cases asserting storage-network NADs cannot be deleted.

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

Comment on lines 125 to 127
if err := v.checkStorageNetwork(nad); err != nil {
return fmt.Errorf(deleteErr, nad.Namespace, nad.Name, err)
}
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The delete path now surfaces the storage-network protection error to end users. The current message text (via storageNetworkErr) says "storagenetwork" which is a bit opaque/inconsistent with the user-facing term "storage-network" used elsewhere; consider updating the underlying error text to be more explicit (e.g., mention "storage-network NAD" and/or use "storage network" wording) so kubectl users understand why deletion is denied.

Copilot uses AI. Check for mistakes.
Copy link
Member

@starbops starbops left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I have a question: does the change affect how the storage network setting behaves? For instance, restore the setting to the default value, i.e., no dedicated storage network.

Deny deletion of a NAD if it is used for storage-network.

Related to: harvester/harvester#9623

Signed-off-by: Volker Theile <[email protected]>
@votdev votdev force-pushed the issue_9623_del_nad_webhook branch from f682c42 to d3e9230 Compare February 11, 2026 10:04
@votdev
Copy link
Member Author

votdev commented Feb 11, 2026

OK, I think we need to take the following approach, as we certainly don't want to import Harvester into network-controller-harvester just to be able to access the settings. Therefore, the bug in Harvester must first be fixed so that the annotation storage-network.settings.harvesterhci.io is not removed or set to false when the storage-network setting is reset to default (disabled). Then we just need to check here in storage-network.settings.harvesterhci.io in the NAD Delete validator whether the annotation exists and contains true and simply omit the validation of the name (storagenetwork-XXX). I think that's the most pragmatic approach.

Alternatively relocate the validation into Harvester where we have full access to the settings: harvester/harvester#10042

Signed-off-by: Volker Theile <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants