fix(webhook): validate pool-related inputs

Signed-off-by: Zespre Chang <zespre.chang@suse.com>
harvester · Feb 2, 2024 · 9698ffc · 9698ffc
1 parent d7609f9
commit 9698ffc
Show file tree

Hide file tree

Showing 6 changed files with 533 additions and 55 deletions.
diff --git a/pkg/controller/ippool/common.go b/pkg/controller/ippool/common.go
@@ -24,7 +24,7 @@ func prepareAgentPod(
 	clusterNetwork string,
 	agentServiceAccountName string,
 	agentImage *config.Image,
-) *corev1.Pod {
+) (*corev1.Pod, error) {
 	name := fmt.Sprintf("%s-%s-agent", ipPool.Namespace, ipPool.Name)
 
 	nadNamespace, nadName := kv.RSplit(ipPool.Spec.NetworkName, "/")
@@ -35,9 +35,15 @@ func prepareAgentPod(
 			InterfaceName: "eth1",
 		},
 	}
-	networksStr, _ := json.Marshal(networks)
+	networksStr, err := json.Marshal(networks)
+	if err != nil {
+		return nil, err
+	}
 
-	_, ipNet, _ := net.ParseCIDR(ipPool.Spec.IPv4Config.CIDR)
+	_, ipNet, err := net.ParseCIDR(ipPool.Spec.IPv4Config.CIDR)
+	if err != nil {
+		return nil, err
+	}
 	prefixLength, _ := ipNet.Mask.Size()
 
 	args := []string{
@@ -141,7 +147,7 @@ func prepareAgentPod(
 				},
 			},
 		},
-	}
+	}, nil
 }
 
 func setRegisteredCondition(ipPool *networkv1.IPPool, status corev1.ConditionStatus, reason, message string) {

diff --git a/pkg/controller/ippool/controller.go b/pkg/controller/ippool/controller.go
@@ -280,7 +280,10 @@ func (h *Handler) DeployAgent(ipPool *networkv1.IPPool, status networkv1.IPPoolS
 		return status, fmt.Errorf("could not find clusternetwork for nad %s", ipPool.Spec.NetworkName)
 	}
 
-	agent := prepareAgentPod(ipPool, h.noDHCP, h.agentNamespace, clusterNetwork, h.agentServiceAccountName, h.agentImage)
+	agent, err := prepareAgentPod(ipPool, h.noDHCP, h.agentNamespace, clusterNetwork, h.agentServiceAccountName, h.agentImage)
+	if err != nil {
+		return status, err
+	}
 
 	agentPod, err := h.podClient.Create(agent)
 	if err != nil {

diff --git a/pkg/controller/ippool/controller_test.go b/pkg/controller/ippool/controller_test.go
@@ -226,7 +226,7 @@ func TestHandler_DeployAgent(t *testing.T) {
 
 		expectedStatus := newTestIPPoolStatusBuilder().
 			AgentPodRef(testPodNamespace, testPodName).Build()
-		expectedPod := prepareAgentPod(
+		expectedPod, _ := prepareAgentPod(
 			NewIPPoolBuilder(testIPPoolNamespace, testIPPoolName).
 				ServerIP(testServerIP).
 				CIDR(testCIDR).
@@ -322,7 +322,7 @@ func TestHandler_DeployAgent(t *testing.T) {
 			AgentPodRef(testPodNamespace, testPodName).Build()
 		givenNAD := newTestNetworkAttachmentDefinitionBuilder().
 			Label(clusterNetworkLabelKey, testClusterNetwork).Build()
-		givenPod := prepareAgentPod(
+		givenPod, _ := prepareAgentPod(
 			NewIPPoolBuilder(testIPPoolNamespace, testIPPoolName).
 				ServerIP(testServerIP).
 				CIDR(testCIDR).
@@ -339,7 +339,7 @@ func TestHandler_DeployAgent(t *testing.T) {
 
 		expectedStatus := newTestIPPoolStatusBuilder().
 			AgentPodRef(testPodNamespace, testPodName).Build()
-		expectedPod := prepareAgentPod(
+		expectedPod, _ := prepareAgentPod(
 			NewIPPoolBuilder(testIPPoolNamespace, testIPPoolName).
 				ServerIP(testServerIP).
 				CIDR(testCIDR).

diff --git a/pkg/util/network.go b/pkg/util/network.go
@@ -4,9 +4,21 @@ import (
 	"fmt"
 	"net"
 	"net/netip"
+
+	networkv1 "github.com/harvester/vm-dhcp-controller/pkg/apis/network.harvesterhci.io/v1alpha1"
 )
 
-func LoadCIDR(cidr string) (ipNet *net.IPNet, networkIPAddr netip.Addr, broadcastIPAddr netip.Addr, err error) {
+type PoolInfo struct {
+	IPNet           *net.IPNet
+	NetworkIPAddr   netip.Addr
+	BroadcastIPAddr netip.Addr
+	StartIPAddr     netip.Addr
+	EndIPAddr       netip.Addr
+	ServerIPAddr    netip.Addr
+	RouterIPAddr    netip.Addr
+}
+
+func loadCIDR(cidr string) (ipNet *net.IPNet, networkIPAddr netip.Addr, broadcastIPAddr netip.Addr, err error) {
 	_, ipNet, err = net.ParseCIDR(cidr)
 	if err != nil {
 		return
@@ -31,3 +43,51 @@ func LoadCIDR(cidr string) (ipNet *net.IPNet, networkIPAddr netip.Addr, broadcas
 
 	return
 }
+
+func LoadPool(ipPool *networkv1.IPPool) (pi PoolInfo, err error) {
+	pi.IPNet, pi.NetworkIPAddr, pi.BroadcastIPAddr, err = loadCIDR(ipPool.Spec.IPv4Config.CIDR)
+	if err != nil {
+		return
+	}
+
+	if ipPool.Spec.IPv4Config.Pool.Start != "" {
+		pi.StartIPAddr, err = netip.ParseAddr(ipPool.Spec.IPv4Config.Pool.Start)
+		if err != nil {
+			return
+		}
+	}
+
+	if ipPool.Spec.IPv4Config.Pool.End != "" {
+		pi.EndIPAddr, err = netip.ParseAddr(ipPool.Spec.IPv4Config.Pool.End)
+		if err != nil {
+			return
+		}
+	}
+
+	if ipPool.Spec.IPv4Config.ServerIP != "" {
+		pi.ServerIPAddr, err = netip.ParseAddr(ipPool.Spec.IPv4Config.ServerIP)
+		if err != nil {
+			return
+		}
+	}
+
+	if ipPool.Spec.IPv4Config.Router != "" {
+		pi.RouterIPAddr, err = netip.ParseAddr(ipPool.Spec.IPv4Config.Router)
+		if err != nil {
+			return
+		}
+	}
+
+	return
+}
+
+func LoadAllocated(allocated map[string]string) (ipAddrList []netip.Addr) {
+	for ip := range allocated {
+		ipAddr, err := netip.ParseAddr(ip)
+		if err != nil {
+			continue
+		}
+		ipAddrList = append(ipAddrList, ipAddr)
+	}
+	return
+}
diff --git a/pkg/webhook/ippool/validator.go b/pkg/webhook/ippool/validator.go
@@ -36,11 +36,25 @@ func (v *Validator) Create(_ *admission.Request, newObj runtime.Object) error {
 	ipPool := newObj.(*networkv1.IPPool)
 	logrus.Infof("create ippool %s/%s", ipPool.Namespace, ipPool.Name)
 
-	if err := v.checkNAD(ipPool); err != nil {
+	// sanity check
+	poolInfo, err := util.LoadPool(ipPool)
+	if err != nil {
+		return fmt.Errorf(webhook.CreateErr, "IPPool", ipPool.Namespace, ipPool.Name, err)
+	}
+
+	if err := v.checkNAD(ipPool.Spec.NetworkName); err != nil {
+		return fmt.Errorf(webhook.CreateErr, "IPPool", ipPool.Namespace, ipPool.Name, err)
+	}
+
+	if err := v.checkPoolRange(poolInfo); err != nil {
 		return fmt.Errorf(webhook.CreateErr, "IPPool", ipPool.Namespace, ipPool.Name, err)
 	}
 
-	if err := v.checkServerIP(ipPool); err != nil {
+	if err := v.checkServerIP(poolInfo); err != nil {
+		return fmt.Errorf(webhook.CreateErr, "IPPool", ipPool.Namespace, ipPool.Name, err)
+	}
+
+	if err := v.checkRouter(poolInfo); err != nil {
 		return fmt.Errorf(webhook.CreateErr, "IPPool", ipPool.Namespace, ipPool.Name, err)
 	}
 
@@ -56,11 +70,30 @@ func (v *Validator) Update(_ *admission.Request, _, newObj runtime.Object) error
 
 	logrus.Infof("update ippool %s/%s", ipPool.Namespace, ipPool.Name)
 
-	if err := v.checkNAD(ipPool); err != nil {
+	// sanity check
+	poolInfo, err := util.LoadPool(ipPool)
+	if err != nil {
 		return fmt.Errorf(webhook.CreateErr, "IPPool", ipPool.Namespace, ipPool.Name, err)
 	}
 
-	if err := v.checkServerIP(ipPool); err != nil {
+	var allocatedIPAddrList []netip.Addr
+	if ipPool.Status.IPv4 != nil {
+		allocatedIPAddrList = util.LoadAllocated(ipPool.Status.IPv4.Allocated)
+	}
+
+	if err := v.checkNAD(ipPool.Spec.NetworkName); err != nil {
+		return fmt.Errorf(webhook.CreateErr, "IPPool", ipPool.Namespace, ipPool.Name, err)
+	}
+
+	if err := v.checkPoolRange(poolInfo); err != nil {
+		return fmt.Errorf(webhook.CreateErr, "IPPool", ipPool.Namespace, ipPool.Name, err)
+	}
+
+	if err := v.checkServerIP(poolInfo, allocatedIPAddrList...); err != nil {
+		return fmt.Errorf(webhook.CreateErr, "IPPool", ipPool.Namespace, ipPool.Name, err)
+	}
+
+	if err := v.checkRouter(poolInfo); err != nil {
 		return fmt.Errorf(webhook.CreateErr, "IPPool", ipPool.Namespace, ipPool.Name, err)
 	}
 
@@ -93,8 +126,8 @@ func (v *Validator) Resource() admission.Resource {
 	}
 }
 
-func (v *Validator) checkNAD(ipPool *networkv1.IPPool) error {
-	nadNamespace, nadName := kv.RSplit(ipPool.Spec.NetworkName, "/")
+func (v *Validator) checkNAD(namespacedName string) error {
+	nadNamespace, nadName := kv.RSplit(namespacedName, "/")
 	if nadNamespace == "" {
 		nadNamespace = "default"
 	}
@@ -103,49 +136,87 @@ func (v *Validator) checkNAD(ipPool *networkv1.IPPool) error {
 	return err
 }
 
-func (v *Validator) checkServerIP(ipPool *networkv1.IPPool) error {
-	ipNet, networkIPAddr, broadcastIPAddr, err := util.LoadCIDR(ipPool.Spec.IPv4Config.CIDR)
-	if err != nil {
-		return err
+func (v *Validator) checkPoolRange(pi util.PoolInfo) error {
+	if pi.StartIPAddr.IsValid() {
+		if !pi.IPNet.Contains(pi.StartIPAddr.AsSlice()) {
+			return fmt.Errorf("start ip %s is not within subnet", pi.StartIPAddr)
+		}
+
+		if pi.StartIPAddr.As4() == pi.NetworkIPAddr.As4() {
+			return fmt.Errorf("start ip %s is the same as network ip", pi.StartIPAddr)
+		}
+
+		if pi.StartIPAddr.As4() == pi.BroadcastIPAddr.As4() {
+			return fmt.Errorf("start ip %s is the same as broadcast ip", pi.StartIPAddr)
+		}
 	}
 
-	routerIPAddr, err := netip.ParseAddr(ipPool.Spec.IPv4Config.Router)
-	if err != nil {
-		routerIPAddr = netip.Addr{}
+	if pi.EndIPAddr.IsValid() {
+		if !pi.IPNet.Contains(pi.EndIPAddr.AsSlice()) {
+			return fmt.Errorf("end ip %s is not within subnet", pi.EndIPAddr)
+		}
+
+		if pi.EndIPAddr.As4() == pi.NetworkIPAddr.As4() {
+			return fmt.Errorf("end ip %s is the same as network ip", pi.EndIPAddr)
+		}
+
+		if pi.EndIPAddr.As4() == pi.BroadcastIPAddr.As4() {
+			return fmt.Errorf("end ip %s is the same as broadcast ip", pi.EndIPAddr)
+		}
 	}
+	return nil
+}
 
-	serverIPAddr, err := netip.ParseAddr(ipPool.Spec.IPv4Config.ServerIP)
-	if err != nil {
-		return err
+func (v *Validator) checkServerIP(pi util.PoolInfo, allocatedIPs ...netip.Addr) error {
+	if !pi.ServerIPAddr.IsValid() {
+		return nil
 	}
 
-	if !ipNet.Contains(serverIPAddr.AsSlice()) {
-		return fmt.Errorf("server ip %s is not within subnet", serverIPAddr)
+	if !pi.IPNet.Contains(pi.ServerIPAddr.AsSlice()) {
+		return fmt.Errorf("server ip %s is not within subnet", pi.ServerIPAddr)
 	}
 
-	if serverIPAddr.As4() == networkIPAddr.As4() {
-		return fmt.Errorf("server ip %s cannot be the same as network ip", serverIPAddr)
+	if pi.ServerIPAddr.As4() == pi.NetworkIPAddr.As4() {
+		return fmt.Errorf("server ip %s cannot be the same as network ip", pi.ServerIPAddr)
 	}
 
-	if serverIPAddr.As4() == broadcastIPAddr.As4() {
-		return fmt.Errorf("server ip %s cannot be the same as broadcast ip", serverIPAddr)
+	if pi.ServerIPAddr.As4() == pi.BroadcastIPAddr.As4() {
+		return fmt.Errorf("server ip %s cannot be the same as broadcast ip", pi.ServerIPAddr)
 	}
 
-	if routerIPAddr.IsValid() && serverIPAddr.As4() == routerIPAddr.As4() {
-		return fmt.Errorf("server ip %s cannot be the same as router ip", 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)
 	}
 
-	if ipPool.Status.IPv4 != nil {
-		for ip, mac := range ipPool.Status.IPv4.Allocated {
-			if serverIPAddr.String() == ip {
-				return fmt.Errorf("server ip %s is already allocated by mac %s", serverIPAddr, mac)
-			}
+	for _, ip := range allocatedIPs {
+		if pi.ServerIPAddr == ip {
+			return fmt.Errorf("server ip %s is already allocated", pi.ServerIPAddr)
 		}
 	}
 
 	return nil
 }
 
+func (v *Validator) checkRouter(pi util.PoolInfo) error {
+	if !pi.RouterIPAddr.IsValid() {
+		return nil
+	}
+
+	if !pi.IPNet.Contains(pi.RouterIPAddr.AsSlice()) {
+		return fmt.Errorf("router ip %s is not within subnet", pi.RouterIPAddr)
+	}
+
+	if pi.RouterIPAddr.As4() == pi.NetworkIPAddr.As4() {
+		return fmt.Errorf("router ip %s is the same as network ip", pi.RouterIPAddr)
+	}
+
+	if pi.RouterIPAddr.As4() == pi.BroadcastIPAddr.As4() {
+		return fmt.Errorf("router ip %s is the same as broadcast ip", pi.BroadcastIPAddr)
+	}
+
+	return nil
+}
+
 func (v *Validator) checkVmNetCfgs(ipPool *networkv1.IPPool) error {
 	vmnetcfgGetter := util.VmnetcfgGetter{
 		VmnetcfgCache: v.vmnetcfgCache,