fix(webhook): more robust cidr check for ippool

Load the cluster-wide service CIDR from the following sources: - "rke2.io/node-args" annotation in management Node objects - Webhook command argument "--service-cidr" - Default value "10.53.0.0/16" Comparing the CIDR of the user-input IPPool object with the cluster-wide one. If overlap, reject the create/update requests. Signed-off-by: Zespre Chang <zespre.chang@suse.com>
harvester · Mar 12, 2024 · dd8eff9 · dd8eff9
1 parent 7e9159d
commit dd8eff9
Show file tree

Hide file tree

Showing 5 changed files with 106 additions and 26 deletions.
diff --git a/cmd/webhook/root.go b/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,11 +61,14 @@ 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(&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")
 	rootCmd.Flags().IntVar(&options.HTTPSListenPort, "https-port", 8443, "HTTPS listen port")
 	rootCmd.Flags().IntVar(&options.Threadiness, "threadiness", 5, "Specify controller threads")
+
 }
 
 // Execute adds all child commands to the root command and sets flags appropriately.

diff --git a/cmd/webhook/run.go b/cmd/webhook/run.go
@@ -76,7 +76,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.nodeCache, c.vmnetcfgCache),
 		vmnetcfg.NewValidator(c.ippoolCache),
 	); err != nil {
 		return err

diff --git a/pkg/util/common.go b/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 {

diff --git a/pkg/webhook/ippool/validator.go b/pkg/webhook/ippool/validator.go
@@ -2,13 +2,15 @@ package ippool
 
 import (
 	"fmt"
+	"net"
 	"net/netip"
 	"strings"
 
 	"github.com/harvester/webhook/pkg/server/admission"
 	"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"
 
@@ -23,17 +25,21 @@ 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 +167,21 @@ func (v *Validator) checkCIDR(cidr string) error {
 		return nil
 	}
 
-	nodes, err := v.nodeCache.List(labels.Everything())
+	sets := labels.Set{
+		util.ManagementNodeLabelKey: "true",
+	}
+	mgmtNodes, err := v.nodeCache.List(sets.AsSelector())
 	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
-		}
+	svcIPNet, err := consolidateServiceCIDRs(mgmtNodes, v.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
@@ -297,3 +299,28 @@ 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
@@ -290,7 +290,7 @@ func TestValidator_Create(t *testing.T) {
 			},
 		},
 		{
-			name: "cidr overlaps cluster's service cidr",
+			name: "cidr overlaps cluster's service cidr (retrieved from the node-args annotation)",
 			given: input{
 				ipPool: newTestIPPoolBuilder().
 					CIDR(testCIDROverlap).
@@ -301,12 +301,35 @@ func TestValidator_Create(t *testing.T) {
 						Annotations: map[string]string{
 							util.NodeArgsAnnotationKey: fmt.Sprintf("[\"%s\", \"%s\"]", util.ServiceCIDRFlag, testServiceCIDR),
 						},
+						Labels: map[string]string{
+							util.ManagementNodeLabelKey: "true",
+						},
+						Name: "node-0",
+					},
+				},
+			},
+			expected: output{
+				err: fmt.Errorf("could not create IPPool %s/%s because cidr %s overlaps cluster service cidr %s", testIPPoolNamespace, testIPPoolName, testCIDROverlap, testServiceCIDR),
+			},
+		},
+		{
+			name: "cidr overlaps cluster's service cidr (default service cidr)",
+			given: input{
+				ipPool: newTestIPPoolBuilder().
+					CIDR(testCIDROverlap).
+					NetworkName(testNetworkName).Build(),
+				nad: newTestNetworkAttachmentDefinitionBuilder().Build(),
+				node: &corev1.Node{
+					ObjectMeta: metav1.ObjectMeta{
+						Labels: map[string]string{
+							util.ManagementNodeLabelKey: "true",
+						},
 						Name: "node-0",
 					},
 				},
 			},
 			expected: output{
-				err: fmt.Errorf("could not create IPPool %s/%s because cidr %s overlaps service cidr %s", testIPPoolNamespace, testIPPoolName, testCIDROverlap, testServiceCIDR),
+				err: fmt.Errorf("could not create IPPool %s/%s because cidr %s overlaps cluster service cidr %s", testIPPoolNamespace, testIPPoolName, testCIDROverlap, testServiceCIDR),
 			},
 		},
 	}
@@ -331,7 +354,7 @@ 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(nadCache, nodeCache, vmnetCache)
+		validator := NewValidator(testServiceCIDR, nadCache, nodeCache, vmnetCache)
 
 		err = validator.Create(&admission.Request{}, tc.given.ipPool)
 
@@ -633,7 +656,7 @@ func TestValidator_Update(t *testing.T) {
 			},
 		},
 		{
-			name: "cidr overlaps cluster's service cidr",
+			name: "cidr overlaps cluster's service cidr (retrieved from the node-args annotation)",
 			given: input{
 				newIPPool: newTestIPPoolBuilder().
 					CIDR(testCIDROverlap).
@@ -644,12 +667,35 @@ func TestValidator_Update(t *testing.T) {
 						Annotations: map[string]string{
 							util.NodeArgsAnnotationKey: fmt.Sprintf("[\"%s\", \"%s\"]", util.ServiceCIDRFlag, testServiceCIDR),
 						},
+						Labels: map[string]string{
+							util.ManagementNodeLabelKey: "true",
+						},
+						Name: "node-0",
+					},
+				},
+			},
+			expected: output{
+				err: fmt.Errorf("could not update IPPool %s/%s because cidr %s overlaps cluster service cidr %s", testIPPoolNamespace, testIPPoolName, testCIDROverlap, testServiceCIDR),
+			},
+		},
+		{
+			name: "cidr overlaps cluster's service cidr (default service cidr)",
+			given: input{
+				newIPPool: newTestIPPoolBuilder().
+					CIDR(testCIDROverlap).
+					NetworkName(testNetworkName).Build(),
+				nad: newTestNetworkAttachmentDefinitionBuilder().Build(),
+				node: &corev1.Node{
+					ObjectMeta: metav1.ObjectMeta{
+						Labels: map[string]string{
+							util.ManagementNodeLabelKey: "true",
+						},
 						Name: "node-0",
 					},
 				},
 			},
 			expected: output{
-				err: fmt.Errorf("could not update IPPool %s/%s because cidr %s overlaps service cidr %s", testIPPoolNamespace, testIPPoolName, testCIDROverlap, testServiceCIDR),
+				err: fmt.Errorf("could not update IPPool %s/%s because cidr %s overlaps cluster service cidr %s", testIPPoolNamespace, testIPPoolName, testCIDROverlap, testServiceCIDR),
 			},
 		},
 		{
@@ -699,7 +745,7 @@ 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(nadCache, nodeCache, vmnetCache)
+		validator := NewValidator(testServiceCIDR, nadCache, nodeCache, vmnetCache)
 
 		err = validator.Update(&admission.Request{}, tc.given.oldIPPool, tc.given.newIPPool)