Merge pull request #405 from theobarberbany/tb/validate-creation-mapi-machinesets

OCPCLOUD-3188: Prevent MAPI MachineSet creation when CAPI MachineSet with same name exists

Author: openshift-merge-bot[bot]

manifests/0000_30_cluster-api_09_admission-policies.yaml

@@ -354,6 +354,53 @@ data:
     ---
     apiVersion: admissionregistration.k8s.io/v1
     kind: ValidatingAdmissionPolicy
+    metadata:
+      name: openshift-prevent-authoritative-mapi-machineset-create-when-capi-exists 
+    spec:
+      failurePolicy: Fail
+      paramKind:
+        apiVersion: cluster.x-k8s.io/v1beta1
+        kind: MachineSet
+      matchConstraints:
+        resourceRules:
+          - apiGroups: ["machine.openshift.io"]
+            apiVersions: ["*"]
+            operations: ["CREATE"]
+            resources: ["machinesets"]
+      # Requests must satisfy every matchCondition to reach the validations
+      matchConditions:
+        - name: check-param-match
+          expression: 'object.metadata.name == params.metadata.name'
+      # All validations must evaluate to true
+      validations:
+       # Only allow creation of a Machine API MachineSet with the same name as an
+       # existing CAPI one if spec.authoritativeAPI == ClusterAPI
+      - expression: 'object.spec.authoritativeAPI == "ClusterAPI"'
+        messageExpression: "'Can\\'t create Machine API MachineSet ' + object.metadata.name + ' with spec.authoritativeAPI: ' + object.spec.?authoritativeAPI.orValue('<unset>') + ' because a Cluster API MachineSet with the same name already exists.'"
+    ---
+    apiVersion: admissionregistration.k8s.io/v1
+    kind: ValidatingAdmissionPolicyBinding
+    metadata:
+      name: openshift-prevent-authoritative-mapi-machineset-create-when-capi-exists
+    spec:
+      matchResources:
+        namespaceSelector:
+          matchLabels:
+            kubernetes.io/metadata.name: openshift-machine-api
+      paramRef:
+        namespace: openshift-cluster-api
+        # We 'Allow' here as we don't want to block MAPI MachineSet
+        # functionality when no CAPI machineset (param) exists.
+        # This might happen when the synchronisation controller first
+        # is installed or when an unmigrateable machineset is encountered.
+        parameterNotFoundAction: Allow
+        selector: {}
+      policyName: openshift-prevent-authoritative-mapi-machineset-create-when-capi-exists
+      validationActions:
+          - Deny
+    ---
+    apiVersion: admissionregistration.k8s.io/v1
+    kind: ValidatingAdmissionPolicy
     metadata:
       name: openshift-prevent-migration-when-machine-updating
     spec:

pkg/controllers/machinesetsync/machineset_vap_test.go

@@ -23,6 +23,7 @@ import (
 	clusterv1resourcebuilder "github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/cluster-api/core/v1beta1"
 	awsv1resourcebuilder "github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/cluster-api/infrastructure/v1beta2"
 	corev1resourcebuilder "github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/core/v1"
+	machinev1resourcebuilder "github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/machine/v1beta1"
 	admissiontestutils "github.com/openshift/cluster-capi-operator/pkg/admissionpolicy/testutils"
 
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
@@ -234,4 +235,104 @@ var _ = Describe("MachineSet VAP Tests", func() {
 			}), timeout).Should(MatchError(ContainSubstring(".readinessGates is a forbidden field")))
 		})
 	})
+
+	Context("Prevent authoritative MAPI MachineSet creation when same-named CAPI MachineSet exists", func() {
+		var mapiMachineSetBuilder machinev1resourcebuilder.MachineSetBuilder
+		const vapName string = "openshift-prevent-authoritative-mapi-machineset-create-when-capi-exists"
+
+		BeforeEach(func() {
+			By("Waiting for VAP to be ready")
+			machineSetVap = &admissionregistrationv1.ValidatingAdmissionPolicy{}
+			Eventually(k8sClient.Get(ctx, client.ObjectKey{Name: vapName}, machineSetVap), timeout).Should(Succeed())
+
+			// Add UPDATE operation for easier testing (same as Machine tests)
+			resourceRules := machineSetVap.Spec.MatchConstraints.ResourceRules
+			Expect(resourceRules).To(HaveLen(1))
+			resourceRules[0].Operations = append(resourceRules[0].Operations, admissionregistrationv1.Update)
+
+			Eventually(k.Update(machineSetVap, func() {
+				admissiontestutils.AddSentinelValidation(machineSetVap)
+				machineSetVap.Spec.MatchConstraints.ResourceRules = resourceRules
+			})).Should(Succeed())
+
+			Eventually(k.Object(machineSetVap), timeout).Should(
+				HaveField("Status.ObservedGeneration", BeNumerically(">=", 2)),
+			)
+
+			By("Updating the VAP binding")
+			policyBinding = &admissionregistrationv1.ValidatingAdmissionPolicyBinding{}
+			Eventually(k8sClient.Get(ctx, client.ObjectKey{
+				Name: vapName}, policyBinding), timeout).Should(Succeed())
+
+			Eventually(k.Update(policyBinding, func() {
+				// paramNamespace=capiNamespace (CAPI resources are params)
+				// targetNamespace=mapiNamespace (MAPI resources are validated)
+				admissiontestutils.UpdateVAPBindingNamespaces(policyBinding, capiNamespace.GetName(), mapiNamespace.GetName())
+			}), timeout).Should(Succeed())
+
+			// Wait until the binding shows the patched values
+			Eventually(k.Object(policyBinding), timeout).Should(
+				SatisfyAll(
+					HaveField("Spec.MatchResources.NamespaceSelector.MatchLabels",
+						HaveKeyWithValue("kubernetes.io/metadata.name",
+							mapiNamespace.GetName())),
+				),
+			)
+
+			By("Creating throwaway MachineSet pair for sentinel validation")
+			mapiMachineSetBuilder = machinev1resourcebuilder.MachineSet().
+				WithNamespace(mapiNamespace.Name)
+
+			sentinelMachineSet := machinev1resourcebuilder.MachineSet().
+				WithNamespace(mapiNamespace.Name).
+				WithName("sentinel-machineset").
+				WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityClusterAPI).
+				Build()
+			Eventually(k8sClient.Create(ctx, sentinelMachineSet), timeout).Should(Succeed())
+
+			capiSentinelMachineSet := clusterv1resourcebuilder.MachineSet().
+				WithName("sentinel-machineset").
+				WithNamespace(capiNamespace.Name).
+				Build()
+			Eventually(k8sClient.Create(ctx, capiSentinelMachineSet)).Should(Succeed())
+
+			Eventually(k.Get(capiSentinelMachineSet)).Should(Succeed())
+
+			admissiontestutils.VerifySentinelValidation(k, sentinelMachineSet, timeout)
+		})
+
+		It("Does not allow creation of a MAPI MachineSet with spec.authoritativeAPI: MachineAPI and the same name", func() {
+			By("Create the CAPI MachineSet")
+			Eventually(k8sClient.Create(ctx, capiMachineSet)).Should(Succeed())
+
+			By("Create the MAPI MachineSet")
+			newMapiMachineSet := mapiMachineSetBuilder.
+				WithName("test-machineset").
+				WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityMachineAPI).
+				Build()
+			Eventually(k8sClient.Create(ctx, newMapiMachineSet), timeout).Should(
+				MatchError(ContainSubstring("with spec.authoritativeAPI: MachineAPI because a Cluster API MachineSet with the same name already exists.")))
+		})
+
+		It("Does allow creation of a MAPI machineset with authoritative API ClusterAPI and the same name", func() {
+			By("Create the CAPI MachineSet")
+			Eventually(k8sClient.Create(ctx, capiMachineSet)).Should(Succeed())
+
+			By("Create the MAPI MachineSet")
+			newMapiMachineSet := mapiMachineSetBuilder.
+				WithName("test-machineset").
+				WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityClusterAPI).
+				Build()
+			Eventually(k8sClient.Create(ctx, newMapiMachineSet), timeout).Should(Succeed())
+		})
+
+		It("Does allow creation of a MAPI MachineSet when no matching CAPI MachineSet exists (parameterNotFoundAction)", func() {
+			By("Create the MAPI MachineSet without creating a CAPI MachineSet first")
+			newMapiMachineSet := mapiMachineSetBuilder.
+				WithName("no-capi-equivalent").
+				WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityMachineAPI).
+				Build()
+			Eventually(k8sClient.Create(ctx, newMapiMachineSet), timeout).Should(Succeed())
+		})
+	})
 })