proxmox_webhook: check routing policy tables (netplan lied more)

ionos-cloud · Apr 16, 2024 · 736a064 · 736a064
1 parent 76f1061
commit 736a064
Show file tree

Hide file tree

Showing 6 changed files with 87 additions and 5 deletions.
diff --git a/api/v1alpha1/proxmoxmachine_types.go b/api/v1alpha1/proxmoxmachine_types.go
@@ -281,7 +281,7 @@ type RoutingPolicySpec struct {
 
 	// Table is the routing table ID.
 	// +optional
-	Table uint32 `json:"table,omitempty"`
+	Table *uint32 `json:"table,omitempty"`
 
 	// Priority is the position in the ip rule FIB table.
 	// +kubebuilder:validation:Maximum=4294967295

diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go
diff --git a/internal/service/vmservice/bootstrap.go b/internal/service/vmservice/bootstrap.go
@@ -168,7 +168,9 @@ func getRoutingPolicyData(rules []infrav1alpha1.RoutingPolicySpec) *[]cloudinit.
 		ruleSpec.To = rule.To
 		ruleSpec.From = rule.From
 		ruleSpec.Priority = rule.Priority
-		ruleSpec.Table = rule.Table
+		if rule.Table != nil {
+			ruleSpec.Table = *rule.Table
+		}
 		routingPolicyData = append(routingPolicyData, ruleSpec)
 	}
 

diff --git a/internal/service/vmservice/bootstrap_test.go b/internal/service/vmservice/bootstrap_test.go
@@ -205,8 +205,8 @@ func TestGetCommonInterfaceConfig(t *testing.T) {
 							{To: "172.24.16.0/24", Via: "192.168.178.1", Table: 100},
 						},
 						RoutingPolicy: []infrav1alpha1.RoutingPolicySpec{
-							{To: "10.10.10.0/24", Table: 100},
-							{From: "172.24.16.0/24", Table: 100},
+							{To: "10.10.10.0/24", Table: ptr.To(uint32(100))},
+							{From: "172.24.16.0/24", Table: ptr.To(uint32(100))},
 						},
 					},
 				},

diff --git a/internal/webhook/proxmoxmachine_webhook.go b/internal/webhook/proxmoxmachine_webhook.go
@@ -120,8 +120,53 @@ func validateNetworks(machine *infrav1.ProxmoxMachine) error {
 						field.NewPath("spec", "network", "additionalDevices", fmt.Sprint(i), "linkMtu"), machine.Spec.Network.AdditionalDevices[i], err.Error()),
 				})
 		}
+		err = validateRoutingPolicy(&machine.Spec.Network.AdditionalDevices[i].InterfaceConfig.RoutingPolicy)
+		if err != nil {
+			return apierrors.NewInvalid(
+				gk,
+				name,
+				field.ErrorList{
+					field.Invalid(
+						field.NewPath("spec", "network", "additionalDevices", fmt.Sprint(i), "routingPolicy"), machine.Spec.Network.AdditionalDevices[i], err.Error()),
+				})
+		}
+	}
+
+	for i := range machine.Spec.Network.VirtualNetworkDevices.VRFs {
+		err := validateVRFConfigRoutingPolicy(&machine.Spec.Network.VirtualNetworkDevices.VRFs[i])
+		if err != nil {
+			return apierrors.NewInvalid(
+				gk,
+				name,
+				field.ErrorList{
+					field.Invalid(
+						field.NewPath("spec", "network", "VirtualNetworkDevices", "VRFs", fmt.Sprint(i), "Table"), machine.Spec.Network.VirtualNetworkDevices.VRFs[i], err.Error()),
+				})
+		}
+	}
+
+	return nil
+}
+
+func validateRoutingPolicy(policies *[]infrav1.RoutingPolicySpec) error {
+	for i, policy := range *policies {
+		if policy.Table == nil {
+			return fmt.Errorf("routing policy [%d] requires a table", i)
+		}
 	}
+	return nil
+}
 
+func validateVRFConfigRoutingPolicy(vrf *infrav1.VRFDevice) error {
+	for _, policy := range vrf.Routing.RoutingPolicy {
+		// Netplan will not accept rules not matching the l3mdev table, although
+		// there is no technical reason for this limitation.
+		if policy.Table != nil {
+			if *policy.Table != vrf.Table {
+				return fmt.Errorf("VRF %s: device/rule routing table mismatch %d != %d", vrf.Name, vrf.Table, *policy.Table)
+			}
+		}
+	}
 	return nil
 }
 

diff --git a/internal/webhook/proxmoxmachine_webhook_test.go b/internal/webhook/proxmoxmachine_webhook_test.go
@@ -65,6 +65,18 @@ var _ = Describe("Controller Test", func() {
 			machine.Spec.Network.AdditionalDevices[0].LinkMTU = ptr.To(uint16(1000))
 			g.Expect(k8sClient.Create(testEnv.GetContext(), &machine)).To(MatchError(ContainSubstring("mtu must be at least 1280, but was 1000")))
 		})
+
+		It("should disallow conflicting l3mdev/routing policy table", func() {
+			machine := validProxmoxMachine("test-machine")
+			*machine.Spec.Network.VirtualNetworkDevices.VRFs[0].Routing.RoutingPolicy[0].Table = 667
+			g.Expect(k8sClient.Create(testEnv.GetContext(), &machine)).To(MatchError(ContainSubstring("VRF vrf-green: device/rule routing table mismatch 665 != 667")))
+		})
+
+		It("should disallow routing policy without table", func() {
+			machine := validProxmoxMachine("test-machine")
+			machine.Spec.Network.AdditionalDevices[0].InterfaceConfig.Routing.RoutingPolicy[0].Table = nil
+			g.Expect(k8sClient.Create(testEnv.GetContext(), &machine)).To(MatchError(ContainSubstring("routing policy [0] requires a table")))
+		})
 	})
 
 	Context("update proxmox cluster", func() {
@@ -131,9 +143,25 @@ func validProxmoxMachine(name string) infrav1.ProxmoxMachine {
 								Kind:     "InClusterIPPool",
 								APIGroup: ptr.To("ipam.cluster.x-k8s.io"),
 							},
+							Routing: infrav1.Routing{
+								RoutingPolicy: []infrav1.RoutingPolicySpec{{
+									Table: ptr.To(uint32(665)),
+								}},
+							},
 						},
 					},
 				},
+				VirtualNetworkDevices: infrav1.VirtualNetworkDevices{
+					VRFs: []infrav1.VRFDevice{{
+						Table: 665,
+						Name:  "vrf-green",
+						Routing: infrav1.Routing{
+							RoutingPolicy: []infrav1.RoutingPolicySpec{{
+								Table: ptr.To(uint32(665)),
+							}},
+						}},
+					},
+				},
 			},
 		},
 	}