fix(webhook): more robust cidr check for ippool

cmd/webhook/root.go

@@ -13,12 +13,15 @@ import (
 	"github.com/harvester/webhook/pkg/config"
 )
 
+const defaultServiceCIDR = "10.53.0.0/16"
+
 var (
 	logDebug bool
 	logTrace bool
 
-	name    string
-	options config.Options
+	name        string
+	serviceCIDR string
+	options     config.Options
 )
 
 // rootCmd represents the base command when called without any subcommands
@@ -58,6 +61,8 @@ 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, "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")
 	rootCmd.Flags().StringVar(&options.Namespace, "namespace", os.Getenv("NAMESPACE"), "The harvester namespace")

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(c.nadCache, c.nodeCache, c.vmnetcfgCache),
+		ippool.NewValidator(serviceCIDR, c.nadCache, c.vmnetcfgCache),
 		vmnetcfg.NewValidator(c.ippoolCache),
 	); err != nil {
 		return err

pkg/util/common.go

@@ -10,9 +10,10 @@ const (
 	ExcludedMark = "EXCLUDED"
 	ReservedMark = "RESERVED"
 
-	AgentSuffixName       = "agent"
-	NodeArgsAnnotationKey = "rke2.io/node-args"
-	ServiceCIDRFlag       = "--service-cidr"
+	AgentSuffixName        = "agent"
+	NodeArgsAnnotationKey  = "rke2.io/node-args"
+	ServiceCIDRFlag        = "--service-cidr"
+	ManagementNodeLabelKey = "node-role.kubernetes.io/control-plane"
 )
 
 func agentConcatName(name ...string) string {

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"
 )

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),
 			},
 		},
 		{

pkg/webhook/ippool/validator.go

@@ -9,11 +9,9 @@ import (
 	"github.com/rancher/wrangler/pkg/kv"
 	"github.com/sirupsen/logrus"
 	admissionregv1 "k8s.io/api/admissionregistration/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"
@@ -23,19 +21,20 @@ import (
 type Validator struct {
 	admission.DefaultValidator
 
+	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,
 	}
 }
@@ -161,25 +160,13 @@ func (v *Validator) checkCIDR(cidr string) error {
 		return nil
 	}
 
-	nodes, err := v.nodeCache.List(labels.Everything())
+	svcIPNet, _, _, err := util.LoadCIDR(v.serviceCIDR)
 	if err != nil {
 		return err
 	}
 
-	for _, node := range nodes {
-		serviceCIDR, err := util.GetServiceCIDRFromNode(node)
-		if err != nil {
-			return err
-		}
-
-		svcIPNet, _, _, err := util.LoadCIDR(serviceCIDR)
-		if err != nil {
-			return err
-		}
-
-		if ipNet.Contains(svcIPNet.IP) || svcIPNet.Contains(ipNet.IP) {
-			return fmt.Errorf("cidr %s overlaps service cidr %s", cidr, serviceCIDR)
-		}
+	if ipNet.Contains(svcIPNet.IP) || svcIPNet.Contains(ipNet.IP) {
+		return fmt.Errorf("cidr %s overlaps cluster service cidr %s", cidr, svcIPNet)
 	}
 
 	return nil
@@ -237,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 {