From 69c6fd2d4c586b0f4cf7fd5df85c16eb6f0631c9 Mon Sep 17 00:00:00 2001 From: Zespre Chang Date: Mon, 11 Mar 2024 14:44:59 +0800 Subject: [PATCH] fix(webhook): revert runtime decision of service cidr It's overkill to retrieve the cluster's service CIDR in runtime since it's rarely changed and almost the same in every Harvester deployment. Revert the relevant code and let users to input the service CIDR string from the webhook's command line argument to remain flexibility. The default value is still `10.53.0.0/16`. Signed-off-by: Zespre Chang --- cmd/webhook/root.go | 2 +- cmd/webhook/run.go | 9 +-- pkg/webhook/common.go | 6 +- pkg/webhook/ippool/mutator_test.go | 6 +- pkg/webhook/ippool/validator.go | 48 +----------- pkg/webhook/ippool/validator_test.go | 111 ++++++++++++++++----------- 6 files changed, 81 insertions(+), 101 deletions(-) diff --git a/cmd/webhook/root.go b/cmd/webhook/root.go index 8ee8a69..9f33c0e 100644 --- a/cmd/webhook/root.go +++ b/cmd/webhook/root.go @@ -61,7 +61,7 @@ func init() { rootCmd.PersistentFlags().BoolVar(&logTrace, "trace", trace, "set logging level to trace") rootCmd.Flags().StringVar(&name, "name", os.Getenv("VM_DHCP_AGENT_NAME"), "The name of the vm-dhcp-webhook instance") - rootCmd.Flags().StringVar(&serviceCIDR, "service-cidr", defaultServiceCIDR, "") + rootCmd.Flags().StringVar(&serviceCIDR, "service-cidr", defaultServiceCIDR, "The service CIDR that the cluster is currently using") rootCmd.Flags().StringVar(&options.ControllerUsername, "controller-user", "harvester-vm-dhcp-controller", "The harvester controller username") rootCmd.Flags().StringVar(&options.GarbageCollectionUsername, "gc-user", "system:serviceaccount:kube-system:generic-garbage-collector", "The system username that performs garbage collection") diff --git a/cmd/webhook/run.go b/cmd/webhook/run.go index 3406745..064d1fb 100644 --- a/cmd/webhook/run.go +++ b/cmd/webhook/run.go @@ -10,7 +10,6 @@ import ( "k8s.io/client-go/rest" ctlcore "github.com/harvester/vm-dhcp-controller/pkg/generated/controllers/core" - ctlcorev1 "github.com/harvester/vm-dhcp-controller/pkg/generated/controllers/core/v1" ctlcni "github.com/harvester/vm-dhcp-controller/pkg/generated/controllers/k8s.cni.cncf.io" ctlcniv1 "github.com/harvester/vm-dhcp-controller/pkg/generated/controllers/k8s.cni.cncf.io/v1" ctlkubevirt "github.com/harvester/vm-dhcp-controller/pkg/generated/controllers/kubevirt.io" @@ -26,9 +25,8 @@ type caches struct { ippoolCache ctlnetworkv1.IPPoolCache vmnetcfgCache ctlnetworkv1.VirtualMachineNetworkConfigCache - nadCache ctlcniv1.NetworkAttachmentDefinitionCache - nodeCache ctlcorev1.NodeCache - vmCache ctlkubevirtv1.VirtualMachineCache + nadCache ctlcniv1.NetworkAttachmentDefinitionCache + vmCache ctlkubevirtv1.VirtualMachineCache } func newCaches(ctx context.Context, cfg *rest.Config, threadiness int) (*caches, error) { @@ -51,7 +49,6 @@ func newCaches(ctx context.Context, cfg *rest.Config, threadiness int) (*caches, ippoolCache: networkFactory.Network().V1alpha1().IPPool().Cache(), vmnetcfgCache: networkFactory.Network().V1alpha1().VirtualMachineNetworkConfig().Cache(), nadCache: cniFactory.K8s().V1().NetworkAttachmentDefinition().Cache(), - nodeCache: coreFactory.Core().V1().Node().Cache(), vmCache: kubevirtFactory.Kubevirt().V1().VirtualMachine().Cache(), } @@ -76,7 +73,7 @@ func run(ctx context.Context, cfg *rest.Config, options *config.Options) error { webhookServer := server.NewWebhookServer(ctx, cfg, name, options) if err := webhookServer.RegisterValidators( - ippool.NewValidator(serviceCIDR, c.nadCache, c.nodeCache, c.vmnetcfgCache), + ippool.NewValidator(serviceCIDR, c.nadCache, c.vmnetcfgCache), vmnetcfg.NewValidator(c.ippoolCache), ); err != nil { return err diff --git a/pkg/webhook/common.go b/pkg/webhook/common.go index 108e099..895a05d 100644 --- a/pkg/webhook/common.go +++ b/pkg/webhook/common.go @@ -1,7 +1,7 @@ package webhook const ( - CreateErr = "could not create %s %s/%s because %w" - UpdateErr = "could not update %s %s/%s because %w" - DeleteErr = "could not delete %s %s/%s because %w" + CreateErr = "cannot create %s %s/%s because %w" + UpdateErr = "cannot update %s %s/%s because %w" + DeleteErr = "cannot delete %s %s/%s because %w" ) diff --git a/pkg/webhook/ippool/mutator_test.go b/pkg/webhook/ippool/mutator_test.go index 33d3804..98cbc1d 100644 --- a/pkg/webhook/ippool/mutator_test.go +++ b/pkg/webhook/ippool/mutator_test.go @@ -194,7 +194,7 @@ func TestMutator_Create(t *testing.T) { Exclude("172.19.64.129", "172.19.64.131", "172.19.64.132", "172.19.64.133", "172.19.64.134").Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because fail to assign ip for dhcp server", testIPPoolNamespace, testIPPoolName), + err: fmt.Errorf("cannot create IPPool %s/%s because fail to assign ip for dhcp server", testIPPoolNamespace, testIPPoolName), }, }, { @@ -204,7 +204,7 @@ func TestMutator_Create(t *testing.T) { CIDR("172.19.64.128/32").Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because fail to assign ip for dhcp server", testIPPoolNamespace, testIPPoolName), + err: fmt.Errorf("cannot create IPPool %s/%s because fail to assign ip for dhcp server", testIPPoolNamespace, testIPPoolName), }, }, { @@ -214,7 +214,7 @@ func TestMutator_Create(t *testing.T) { CIDR("172.19.64.128/31").Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because fail to assign ip for dhcp server", testIPPoolNamespace, testIPPoolName), + err: fmt.Errorf("cannot create IPPool %s/%s because fail to assign ip for dhcp server", testIPPoolNamespace, testIPPoolName), }, }, { diff --git a/pkg/webhook/ippool/validator.go b/pkg/webhook/ippool/validator.go index d4eb3b9..6220f87 100644 --- a/pkg/webhook/ippool/validator.go +++ b/pkg/webhook/ippool/validator.go @@ -2,7 +2,6 @@ package ippool import ( "fmt" - "net" "net/netip" "strings" @@ -10,12 +9,9 @@ import ( "github.com/rancher/wrangler/pkg/kv" "github.com/sirupsen/logrus" admissionregv1 "k8s.io/api/admissionregistration/v1" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" networkv1 "github.com/harvester/vm-dhcp-controller/pkg/apis/network.harvesterhci.io/v1alpha1" - ctlcorev1 "github.com/harvester/vm-dhcp-controller/pkg/generated/controllers/core/v1" ctlcniv1 "github.com/harvester/vm-dhcp-controller/pkg/generated/controllers/k8s.cni.cncf.io/v1" ctlnetworkv1 "github.com/harvester/vm-dhcp-controller/pkg/generated/controllers/network.harvesterhci.io/v1alpha1" "github.com/harvester/vm-dhcp-controller/pkg/util" @@ -28,20 +24,17 @@ type Validator struct { serviceCIDR string nadCache ctlcniv1.NetworkAttachmentDefinitionCache - nodeCache ctlcorev1.NodeCache vmnetcfgCache ctlnetworkv1.VirtualMachineNetworkConfigCache } func NewValidator( serviceCIDR string, nadCache ctlcniv1.NetworkAttachmentDefinitionCache, - nodeCache ctlcorev1.NodeCache, vmnetcfgCache ctlnetworkv1.VirtualMachineNetworkConfigCache, ) *Validator { return &Validator{ serviceCIDR: serviceCIDR, nadCache: nadCache, - nodeCache: nodeCache, vmnetcfgCache: vmnetcfgCache, } } @@ -167,15 +160,7 @@ func (v *Validator) checkCIDR(cidr string) error { return nil } - sets := labels.Set{ - util.ManagementNodeLabelKey: "true", - } - mgmtNodes, err := v.nodeCache.List(sets.AsSelector()) - if err != nil { - return err - } - - svcIPNet, err := consolidateServiceCIDRs(mgmtNodes, v.serviceCIDR) + svcIPNet, _, _, err := util.LoadCIDR(v.serviceCIDR) if err != nil { return err } @@ -239,15 +224,15 @@ func (v *Validator) checkServerIP(pi util.PoolInfo, unallocatables ...netip.Addr } if pi.ServerIPAddr.As4() == pi.NetworkIPAddr.As4() { - return fmt.Errorf("server ip %s cannot be the same as network ip", pi.ServerIPAddr) + return fmt.Errorf("server ip %s is the same as network ip", pi.ServerIPAddr) } if pi.ServerIPAddr.As4() == pi.BroadcastIPAddr.As4() { - return fmt.Errorf("server ip %s cannot be the same as broadcast ip", pi.ServerIPAddr) + return fmt.Errorf("server ip %s is the same as broadcast ip", pi.ServerIPAddr) } if pi.RouterIPAddr.IsValid() && pi.ServerIPAddr.As4() == pi.RouterIPAddr.As4() { - return fmt.Errorf("server ip %s cannot be the same as router ip", pi.ServerIPAddr) + return fmt.Errorf("server ip %s is the same as router ip", pi.ServerIPAddr) } for _, ip := range unallocatables { @@ -299,28 +284,3 @@ func (v *Validator) checkVmNetCfgs(ipPool *networkv1.IPPool) error { } return nil } - -func consolidateServiceCIDRs(nodes []*corev1.Node, cidr string) (svcIPNet *net.IPNet, err error) { - for _, node := range nodes { - var serviceCIDR string - serviceCIDR, err = util.GetServiceCIDRFromNode(node) - if err != nil { - logrus.Warningf("could not find service cidr from node annoatation") - continue - } - - svcIPNet, _, _, err = util.LoadCIDR(serviceCIDR) - if err != nil { - return - } - } - - if svcIPNet == nil { - svcIPNet, _, _, err = util.LoadCIDR(cidr) - if err != nil { - return - } - } - - return -} diff --git a/pkg/webhook/ippool/validator_test.go b/pkg/webhook/ippool/validator_test.go index 11e5faa..e44e5f2 100644 --- a/pkg/webhook/ippool/validator_test.go +++ b/pkg/webhook/ippool/validator_test.go @@ -78,7 +78,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because server ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, testServerIPOutOfRange), + err: fmt.Errorf("cannot create IPPool %s/%s because server ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, testServerIPOutOfRange), }, }, { @@ -91,7 +91,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because server ip %s cannot be the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.128"), + err: fmt.Errorf("cannot create IPPool %s/%s because server ip %s is the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.128"), }, }, { @@ -104,7 +104,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because server ip %s cannot be the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.127"), + err: fmt.Errorf("cannot create IPPool %s/%s because server ip %s is the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.127"), }, }, { @@ -118,7 +118,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because server ip %s cannot be the same as router ip", testIPPoolNamespace, testIPPoolName, "192.168.0.254"), + err: fmt.Errorf("cannot create IPPool %s/%s because server ip %s is the same as router ip", testIPPoolNamespace, testIPPoolName, "192.168.0.254"), }, }, { @@ -131,7 +131,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because ParseAddr(\"%s\"): IPv4 field has value >255", testIPPoolNamespace, testIPPoolName, "192.168.0.1000"), + err: fmt.Errorf("cannot create IPPool %s/%s because ParseAddr(\"%s\"): IPv4 field has value >255", testIPPoolNamespace, testIPPoolName, "192.168.0.1000"), }, }, { @@ -144,7 +144,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because router ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, "192.168.1.1"), + err: fmt.Errorf("cannot create IPPool %s/%s because router ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, "192.168.1.1"), }, }, { @@ -157,7 +157,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because router ip %s is the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.0"), + err: fmt.Errorf("cannot create IPPool %s/%s because router ip %s is the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.0"), }, }, { @@ -170,7 +170,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because router ip %s is the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.255"), + err: fmt.Errorf("cannot create IPPool %s/%s because router ip %s is the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.255"), }, }, { @@ -183,7 +183,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because ParseAddr(\"%s\"): IPv4 field has value >255", testIPPoolNamespace, testIPPoolName, "192.168.0.1000"), + err: fmt.Errorf("cannot create IPPool %s/%s because ParseAddr(\"%s\"): IPv4 field has value >255", testIPPoolNamespace, testIPPoolName, "192.168.0.1000"), }, }, { @@ -196,7 +196,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because start ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, "192.168.1.100"), + err: fmt.Errorf("cannot create IPPool %s/%s because start ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, "192.168.1.100"), }, }, { @@ -209,7 +209,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because start ip %s is the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.0"), + err: fmt.Errorf("cannot create IPPool %s/%s because start ip %s is the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.0"), }, }, { @@ -222,7 +222,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because start ip %s is the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.255"), + err: fmt.Errorf("cannot create IPPool %s/%s because start ip %s is the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.255"), }, }, { @@ -235,7 +235,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because ParseAddr(\"%s\"): IPv4 field has value >255", testIPPoolNamespace, testIPPoolName, "192.168.0.1000"), + err: fmt.Errorf("cannot create IPPool %s/%s because ParseAddr(\"%s\"): IPv4 field has value >255", testIPPoolNamespace, testIPPoolName, "192.168.0.1000"), }, }, { @@ -248,7 +248,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because end ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, "192.168.1.100"), + err: fmt.Errorf("cannot create IPPool %s/%s because end ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, "192.168.1.100"), }, }, { @@ -261,7 +261,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because end ip %s is the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.0"), + err: fmt.Errorf("cannot create IPPool %s/%s because end ip %s is the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.0"), }, }, { @@ -274,7 +274,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because end ip %s is the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.255"), + err: fmt.Errorf("cannot create IPPool %s/%s because end ip %s is the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.255"), }, }, { @@ -286,7 +286,7 @@ func TestValidator_Create(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not create IPPool default/net-1 because network-attachment-definitions.k8s.cni.cncf.io \"%s\" not found", "nonexist"), + err: fmt.Errorf("cannot create IPPool default/net-1 because network-attachment-definitions.k8s.cni.cncf.io \"%s\" not found", "nonexist"), }, }, { @@ -309,7 +309,7 @@ func TestValidator_Create(t *testing.T) { }, }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because cidr %s overlaps cluster service cidr %s", testIPPoolNamespace, testIPPoolName, testCIDROverlap, testServiceCIDR), + err: fmt.Errorf("cannot create IPPool %s/%s because cidr %s overlaps cluster service cidr %s", testIPPoolNamespace, testIPPoolName, testCIDROverlap, testServiceCIDR), }, }, { @@ -329,7 +329,7 @@ func TestValidator_Create(t *testing.T) { }, }, expected: output{ - err: fmt.Errorf("could not create IPPool %s/%s because cidr %s overlaps cluster service cidr %s", testIPPoolNamespace, testIPPoolName, testCIDROverlap, testServiceCIDR), + err: fmt.Errorf("cannot create IPPool %s/%s because cidr %s overlaps cluster service cidr %s", testIPPoolNamespace, testIPPoolName, testCIDROverlap, testServiceCIDR), }, }, } @@ -352,9 +352,8 @@ func TestValidator_Create(t *testing.T) { } nadCache := fakeclient.NetworkAttachmentDefinitionCache(clientset.K8sCniCncfIoV1().NetworkAttachmentDefinitions) - nodeCache := fakeclient.NodeCache(k8sclientset.CoreV1().Nodes) vmnetCache := fakeclient.VirtualMachineNetworkConfigCache(clientset.NetworkV1alpha1().VirtualMachineNetworkConfigs) - validator := NewValidator(testServiceCIDR, nadCache, nodeCache, vmnetCache) + validator := NewValidator(testServiceCIDR, nadCache, vmnetCache) err = validator.Create(&admission.Request{}, tc.given.ipPool) @@ -411,7 +410,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because server ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, testServerIPOutOfRange), + err: fmt.Errorf("cannot update IPPool %s/%s because server ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, testServerIPOutOfRange), }, }, { @@ -428,7 +427,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because server ip %s cannot be the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.0"), + err: fmt.Errorf("cannot update IPPool %s/%s because server ip %s is the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.0"), }, }, { @@ -445,7 +444,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because server ip %s cannot be the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.255"), + err: fmt.Errorf("cannot update IPPool %s/%s because server ip %s is the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.255"), }, }, { @@ -464,7 +463,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because server ip %s cannot be the same as router ip", testIPPoolNamespace, testIPPoolName, "192.168.0.254"), + err: fmt.Errorf("cannot update IPPool %s/%s because server ip %s is the same as router ip", testIPPoolNamespace, testIPPoolName, "192.168.0.254"), }, }, { @@ -484,7 +483,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because server ip %s is already occupied", testIPPoolNamespace, testIPPoolName, "192.168.0.100"), + err: fmt.Errorf("cannot update IPPool %s/%s because server ip %s is already occupied", testIPPoolNamespace, testIPPoolName, "192.168.0.100"), }, }, { @@ -497,7 +496,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because ParseAddr(\"%s\"): IPv4 field has value >255", testIPPoolNamespace, testIPPoolName, "192.168.0.1000"), + err: fmt.Errorf("cannot update IPPool %s/%s because ParseAddr(\"%s\"): IPv4 field has value >255", testIPPoolNamespace, testIPPoolName, "192.168.0.1000"), }, }, { @@ -510,7 +509,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because router ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, "192.168.1.1"), + err: fmt.Errorf("cannot update IPPool %s/%s because router ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, "192.168.1.1"), }, }, { @@ -523,7 +522,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because router ip %s is the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.0"), + err: fmt.Errorf("cannot update IPPool %s/%s because router ip %s is the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.0"), }, }, { @@ -536,7 +535,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because router ip %s is the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.255"), + err: fmt.Errorf("cannot update IPPool %s/%s because router ip %s is the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.255"), }, }, { @@ -549,7 +548,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because ParseAddr(\"%s\"): IPv4 field has value >255", testIPPoolNamespace, testIPPoolName, "192.168.0.1000"), + err: fmt.Errorf("cannot update IPPool %s/%s because ParseAddr(\"%s\"): IPv4 field has value >255", testIPPoolNamespace, testIPPoolName, "192.168.0.1000"), }, }, { @@ -562,7 +561,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because start ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, "192.168.1.100"), + err: fmt.Errorf("cannot update IPPool %s/%s because start ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, "192.168.1.100"), }, }, { @@ -575,7 +574,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because start ip %s is the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.0"), + err: fmt.Errorf("cannot update IPPool %s/%s because start ip %s is the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.0"), }, }, { @@ -588,7 +587,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because start ip %s is the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.255"), + err: fmt.Errorf("cannot update IPPool %s/%s because start ip %s is the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.255"), }, }, { @@ -601,7 +600,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because ParseAddr(\"%s\"): IPv4 field has value >255", testIPPoolNamespace, testIPPoolName, "192.168.0.1000"), + err: fmt.Errorf("cannot update IPPool %s/%s because ParseAddr(\"%s\"): IPv4 field has value >255", testIPPoolNamespace, testIPPoolName, "192.168.0.1000"), }, }, { @@ -614,7 +613,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because end ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, "192.168.1.100"), + err: fmt.Errorf("cannot update IPPool %s/%s because end ip %s is not within subnet", testIPPoolNamespace, testIPPoolName, "192.168.1.100"), }, }, { @@ -627,7 +626,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because end ip %s is the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.0"), + err: fmt.Errorf("cannot update IPPool %s/%s because end ip %s is the same as network ip", testIPPoolNamespace, testIPPoolName, "192.168.0.0"), }, }, { @@ -640,7 +639,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because end ip %s is the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.255"), + err: fmt.Errorf("cannot update IPPool %s/%s because end ip %s is the same as broadcast ip", testIPPoolNamespace, testIPPoolName, "192.168.0.255"), }, }, { @@ -652,7 +651,7 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool default/net-1 because network-attachment-definitions.k8s.cni.cncf.io \"%s\" not found", "nonexist"), + err: fmt.Errorf("cannot update IPPool default/net-1 because network-attachment-definitions.k8s.cni.cncf.io \"%s\" not found", "nonexist"), }, }, { @@ -675,7 +674,7 @@ func TestValidator_Update(t *testing.T) { }, }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because cidr %s overlaps cluster service cidr %s", testIPPoolNamespace, testIPPoolName, testCIDROverlap, testServiceCIDR), + err: fmt.Errorf("cannot update IPPool %s/%s because cidr %s overlaps cluster service cidr %s", testIPPoolNamespace, testIPPoolName, testCIDROverlap, testServiceCIDR), }, }, { @@ -695,7 +694,7 @@ func TestValidator_Update(t *testing.T) { }, }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because cidr %s overlaps cluster service cidr %s", testIPPoolNamespace, testIPPoolName, testCIDROverlap, testServiceCIDR), + err: fmt.Errorf("cannot update IPPool %s/%s because cidr %s overlaps cluster service cidr %s", testIPPoolNamespace, testIPPoolName, testCIDROverlap, testServiceCIDR), }, }, { @@ -709,7 +708,32 @@ func TestValidator_Update(t *testing.T) { nad: newTestNetworkAttachmentDefinitionBuilder().Build(), }, expected: output{ - err: fmt.Errorf("could not update IPPool %s/%s because server ip %s is already occupied", testIPPoolNamespace, testIPPoolName, testExcludedIP), + err: fmt.Errorf("cannot update IPPool %s/%s because server ip %s is already occupied", testIPPoolNamespace, testIPPoolName, testExcludedIP), + }, + }, + { + name: "server ip within the pool range", + given: input{ + newIPPool: newTestIPPoolBuilder(). + CIDR(testCIDR). + ServerIP(testServerIPWithinRange). + NetworkName(testNetworkName). + Allocated(testServerIPWithinRange, util.ReservedMark).Build(), + nad: newTestNetworkAttachmentDefinitionBuilder().Build(), + }, + }, + { + name: "server ip collides with excluded ip", + given: input{ + newIPPool: newTestIPPoolBuilder(). + CIDR(testCIDR). + ServerIP(testExcludedIP). + NetworkName(testNetworkName). + Allocated(testExcludedIP, util.ExcludedMark).Build(), + nad: newTestNetworkAttachmentDefinitionBuilder().Build(), + }, + expected: output{ + err: fmt.Errorf("cannot update IPPool %s/%s because server ip %s is already occupied", testIPPoolNamespace, testIPPoolName, testExcludedIP), }, }, { @@ -743,9 +767,8 @@ func TestValidator_Update(t *testing.T) { } nadCache := fakeclient.NetworkAttachmentDefinitionCache(clientset.K8sCniCncfIoV1().NetworkAttachmentDefinitions) - nodeCache := fakeclient.NodeCache(k8sclientset.CoreV1().Nodes) vmnetCache := fakeclient.VirtualMachineNetworkConfigCache(clientset.NetworkV1alpha1().VirtualMachineNetworkConfigs) - validator := NewValidator(testServiceCIDR, nadCache, nodeCache, vmnetCache) + validator := NewValidator(testServiceCIDR, nadCache, vmnetCache) err = validator.Update(&admission.Request{}, tc.given.oldIPPool, tc.given.newIPPool)