Skip to content
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

2995 check if storage network attached before cluster network and vlan config config deletion #115

Merged
merged 2 commits into from
Sep 19, 2024
+21 −3
Merged

2995 check if storage network attached before cluster network and vlan config config deletion #115

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ce5bb5
2995 check if storage network attached before cluster network or vlan…
rrajendran17 Sep 9, 2024
487f555
2995 remove cluster network validation
rrajendran17 Sep 11, 2024
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
2 changes: 1 addition & 1 deletion cmd/webhook/main.go
View file
Original file line number Diff line number Diff line change
@@ -117,7 +117,7 @@ func run(ctx context.Context, cfg *rest.Config, options *config.Options) error {
}

if err := webhookServer.RegisterValidators(
clusternetwork.NewCnValidator(c.vcCache),
clusternetwork.NewCnValidator(c.vcCache, c.nadCache),
nad.NewNadValidator(c.vmiCache),
vlanconfig.NewVlanConfigValidator(c.nadCache, c.vcCache, c.vsCache, c.vmiCache),
); err != nil {
25 changes: 20 additions & 5 deletions pkg/webhook/clusternetwork/validator.go
View file
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@ package clusternetwork
import (
"fmt"

ctlcniv1 "github.com/harvester/harvester/pkg/generated/controllers/k8s.cni.cncf.io/v1"
"github.com/harvester/webhook/pkg/server/admission"
admissionregv1 "k8s.io/api/admissionregistration/v1"
"k8s.io/apimachinery/pkg/labels"
@@ -15,20 +16,23 @@ import (
)

const (
createErr = "could not create cluster network %s because %w"
deleteErr = "could not delete cluster network %s because %w"
createErr = "could not create cluster network %s because %w"
deleteErr = "could not delete cluster network %s because %w"
StorageNetworkNetAttachDefNamespace = "harvester-system"
)

type CnValidator struct {
admission.DefaultValidator
vcCache ctlnetworkv1.VlanConfigCache
vcCache ctlnetworkv1.VlanConfigCache
nadCache ctlcniv1.NetworkAttachmentDefinitionCache
}

var _ admission.Validator = &CnValidator{}

func NewCnValidator(vcCache ctlnetworkv1.VlanConfigCache) *CnValidator {
func NewCnValidator(vcCache ctlnetworkv1.VlanConfigCache, nadCache ctlcniv1.NetworkAttachmentDefinitionCache) *CnValidator {
validator := &CnValidator{
vcCache: vcCache,
vcCache: vcCache,
nadCache: nadCache,
}
return validator
}
@@ -53,6 +57,17 @@ func (c *CnValidator) Delete(_ *admission.Request, oldObj runtime.Object) error
return fmt.Errorf(deleteErr, cn.Name, fmt.Errorf("it's not allowed"))
}

nads, err := c.nadCache.List(StorageNetworkNetAttachDefNamespace, labels.Set(map[string]string{
Copy link
Member

Choose a reason for hiding this comment

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

This may be redundant.

The following vlanconfig checks if any vc on this cluster is still existing, then block it. The storagenetwork also relys on one vlanconfig. Checking vlanconfig seems to be enough here.

On the other hand, below function on Harvester needs to check VlanConfig is existing when utilize this vlanconfig to carry the storagenetwork.

func (v *settingValidator) validateStorageNetworkHelper(value string) error {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@w13915984028 yes makes sense, if storage network is validated for vlan config to be present then validating at cluster network will be redundant.I will remove that check.Thanks.

Regarding storage network validating if vlan config is present, I have another pull request (testing still in progress)
where I am checking if cluster network is ready (cluster network becomes ready only if at least one vlan config attached) before configuring storage networks.
harvester/harvester#6538

utils.KeyClusterNetworkLabel: cn.Name,
}).AsSelector())
if err != nil {
return fmt.Errorf(deleteErr, cn.Name, err)
}

if len(nads) > 0 {
return fmt.Errorf(deleteErr, cn.Name, fmt.Errorf("storage network is still attached"))
}

vcs, err := c.vcCache.List(labels.Set{
utils.KeyClusterNetworkLabel: cn.Name,
}.AsSelector())
18 changes: 15 additions & 3 deletions pkg/webhook/vlanconfig/validator.go
View file
Original file line number Diff line number Diff line change
@@ -23,9 +23,10 @@ import (
)

const (
createErr = "could not create vlanConfig %s because %w"
updateErr = "could not update vlanConfig %s because %w"
deleteErr = "could not delete vlanConfig %s because %w"
createErr = "could not create vlanConfig %s because %w"
updateErr = "could not update vlanConfig %s because %w"
deleteErr = "could not delete vlanConfig %s because %w"
StorageNetworkNetAttachDefNamespace = "harvester-system"
)

type Validator struct {
@@ -143,6 +144,17 @@ func (v *Validator) Delete(_ *admission.Request, oldObj runtime.Object) error {
return fmt.Errorf(deleteErr, vc.Name, err)
}

nads, err := v.nadCache.List(StorageNetworkNetAttachDefNamespace, labels.Set(map[string]string{
utils.KeyClusterNetworkLabel: vc.Spec.ClusterNetwork,
}).AsSelector())
if err != nil {
return fmt.Errorf(deleteErr, vc.Name, err)
}

if len(nads) > 0 {
return fmt.Errorf(deleteErr, vc.Name, fmt.Errorf("storage network is still attached"))
}

return nil
}