From 023e47eb33a841dd35cca9f678ae23bd0439ec03 Mon Sep 17 00:00:00 2001 From: Rachel Ryan Date: Wed, 15 Oct 2025 15:44:40 +0100 Subject: [PATCH 1/2] Synchronization VAP and tests working on tests --- ..._30_cluster-api_09_admission-policies.yaml | 36 ++++++++- .../machine_sync_controller_test.go | 74 +++++++++++++++++++ 2 files changed, 108 insertions(+), 2 deletions(-) diff --git a/manifests/0000_30_cluster-api_09_admission-policies.yaml b/manifests/0000_30_cluster-api_09_admission-policies.yaml index c652936e6..d6fa011e4 100644 --- a/manifests/0000_30_cluster-api_09_admission-policies.yaml +++ b/manifests/0000_30_cluster-api_09_admission-policies.yaml @@ -243,14 +243,13 @@ data: name: openshift-prevent-migration-when-machine-updating spec: failurePolicy: Fail - matchConstraints: resourceRules: - apiGroups: ["machine.openshift.io"] apiVersions: ["*"] operations: ["UPDATE"] resources: ["machines"] - + # All validations must evaluate to true validations: - expression: '!(has(object.status) && has(object.status.phase) && object.status.phase == "Provisioning" && (oldObject.spec.authoritativeAPI != object.spec.authoritativeAPI))' @@ -270,6 +269,39 @@ data: policyName: openshift-prevent-migration-when-machine-updating validationActions: - Deny + --- + apiVersion: admissionregistration.k8s.io/v1 + kind: ValidatingAdmissionPolicy + metadata: + name: openshift-provide-warning-when-not-synchronized + spec: + failurePolicy: Ignore + + matchConstraints: + resourceRules: + - apiGroups: ["machine.openshift.io"] + apiVersions: ["*"] + operations: ["UPDATE"] + resources: ["machines"] + + # All validations must evaluate to true + validations: + - expression: '!(has(object.status) && has(object.status.conditions) && object.status.conditions.exists(c, c.type == "Synchronized" && c.status == "False") && (oldObject.spec.authoritativeAPI != object.spec.authoritativeAPI))' + message: 'Warning: updating .spec.authoritativeAPI when Synchronized=false on resource means changes may not take effect' + validationActions: [Warn] + --- + apiVersion: admissionregistration.k8s.io/v1 + kind: ValidatingAdmissionPolicyBinding + metadata: + name: openshift-provide-warning-when-not-synchronized + spec: + matchResources: + namespaceSelector: + matchLabels: + kubernetes.io/metadata.name: openshift-machine-api + policyName: openshift-provide-warning-when-not-synchronized + validationActions: + - Warn --- apiVersion: v1 kind: ConfigMap diff --git a/pkg/controllers/machinesync/machine_sync_controller_test.go b/pkg/controllers/machinesync/machine_sync_controller_test.go index fc790b918..1d5c12a07 100644 --- a/pkg/controllers/machinesync/machine_sync_controller_test.go +++ b/pkg/controllers/machinesync/machine_sync_controller_test.go @@ -1364,6 +1364,80 @@ var _ = Describe("With a running MachineSync Reconciler", func() { }) + FContext("Updates to MAPI machine warns user if the Synchronized condition is set to false", func() { + BeforeEach(func() { + By("Waiting for VAP to be ready") + machineVap = &admissionregistrationv1.ValidatingAdmissionPolicy{} + Eventually(k8sClient.Get(ctx, client.ObjectKey{Name: "openshift-provide-warning-when-not-synchronized"}, machineVap), timeout).Should(Succeed()) + Eventually(k.Update(machineVap, func() { + machineVap.Spec.Validations = append(machineVap.Spec.Validations, admissionregistrationv1.Validation{ + Expression: "!(has(object.metadata.labels) && \"test-sentinel\" in object.metadata.labels)", + Message: "policy in place", + }) + })).Should(Succeed()) + + Eventually(k.Object(machineVap), timeout).Should( + HaveField("Status.ObservedGeneration", BeNumerically(">=", 2)), + ) + + By("Updating the VAP binding") + policyBinding = &admissionregistrationv1.ValidatingAdmissionPolicyBinding{} + Eventually(k8sClient.Get(ctx, client.ObjectKey{ + Name: "openshift-provide-warning-when-not-synchronized"}, policyBinding), timeout).Should(Succeed()) + + Eventually(k.Update(policyBinding, func() { + // We need to update the namespace in our namespaceSelector, + // since also use `GenerateName` here + policyBinding.Spec.MatchResources.NamespaceSelector.MatchLabels = map[string]string{ + "kubernetes.io/metadata.name": 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 a throwaway MAPI machine") + sentinelMachine := mapiMachineBuilder.WithName("sentinel-machine").WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityClusterAPI).Build() + Eventually(k8sClient.Create(ctx, sentinelMachine), timeout).Should(Succeed()) + + capiSentinelMachine := clusterv1resourcebuilder.Machine().WithName("sentinel-machine").WithNamespace(capiNamespace.Name).Build() + Expect(k8sClient.Create(ctx, capiSentinelMachine)).To(Succeed()) + + Eventually(k.Get(capiSentinelMachine)).Should(Succeed()) + + Eventually(k.Update(sentinelMachine, func() { + sentinelMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"} + }), timeout).Should(Succeed()) + }) + + It("warns the user when the machine is still synchronzing", func() { + By("Setting the Synchronized condition to False") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.Conditions = []mapiv1beta1.Condition{ + { + Type: consts.SynchronizedCondition, + Status: corev1.ConditionFalse, + Reason: "ErrorReason", + Message: "Error message", + LastTransitionTime: metav1.Now(), + }, + } + })).Should(Succeed()) + + By("Attempting to update the authoritativeAPI should be blocked") + Eventually(k.Update(mapiMachine, func() { + mapiMachine.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + }), timeout).Should(Succeed()) // This should succeed but show a warning? + }) + + }) + }) }) From b065cc9d7dd317604146bed05b2ad19533f834ba Mon Sep 17 00:00:00 2001 From: Theo Barber-Bany Date: Fri, 24 Oct 2025 12:04:18 +0100 Subject: [PATCH 2/2] Capture warnings using custom client - Updates the VAP to emit a warning if the SynchronizedCondition is Unknown or unset. - Updates the test suite to capture warnings using a custom client. --- ..._30_cluster-api_09_admission-policies.yaml | 21 ++++- pkg/admissionpolicy/testutils/util.go | 55 ++++++++++++ .../machine_sync_controller_test.go | 88 ++++++++++++++++--- 3 files changed, 149 insertions(+), 15 deletions(-) diff --git a/manifests/0000_30_cluster-api_09_admission-policies.yaml b/manifests/0000_30_cluster-api_09_admission-policies.yaml index d6fa011e4..549db86b7 100644 --- a/manifests/0000_30_cluster-api_09_admission-policies.yaml +++ b/manifests/0000_30_cluster-api_09_admission-policies.yaml @@ -283,12 +283,27 @@ data: apiVersions: ["*"] operations: ["UPDATE"] resources: ["machines"] + variables: + - name: syncCond + expression: > + has(object.status.conditions) ? + object.status.conditions.exists(c, c.type == "Synchronized") : + false + - name: syncBad + expression: > + variables.syncCond && + object.status.conditions.exists(c, + c.type == "Synchronized" && + (c.status == "False" || c.status == "Unknown") + ) + - name: authAPIChanged + expression: > + oldObject.spec.authoritativeAPI != object.spec.authoritativeAPI # All validations must evaluate to true validations: - - expression: '!(has(object.status) && has(object.status.conditions) && object.status.conditions.exists(c, c.type == "Synchronized" && c.status == "False") && (oldObject.spec.authoritativeAPI != object.spec.authoritativeAPI))' - message: 'Warning: updating .spec.authoritativeAPI when Synchronized=false on resource means changes may not take effect' - validationActions: [Warn] + - expression: '!variables.authAPIChanged || (variables.syncCond && !variables.syncBad)' + message: 'Updating .spec.authoritativeAPI when the Synchronized condition is not true means changes may not take effect' --- apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingAdmissionPolicyBinding diff --git a/pkg/admissionpolicy/testutils/util.go b/pkg/admissionpolicy/testutils/util.go index de74d7c9d..c43b5e62c 100644 --- a/pkg/admissionpolicy/testutils/util.go +++ b/pkg/admissionpolicy/testutils/util.go @@ -17,11 +17,14 @@ limitations under the License. package testutils import ( + "context" "errors" + "fmt" "io" "os" "path/filepath" "strings" + "sync" corev1 "k8s.io/api/core/v1" @@ -32,6 +35,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/yaml" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -261,3 +265,54 @@ func LoadTransportConfigMaps() map[string][]client.Object { return mapObjs } + +// WarningCollector is to provide a way to collect +// kube client warnings, intended for testing VAPs that return warnings. +type WarningCollector struct { + sync.Mutex + messages []string +} + +// HandleWarningHeaderWithContext implements rest.WarningHandlerWithContext. +// For test simplicity, only the message is captured; code and agent are ignored. +func (w *WarningCollector) HandleWarningHeaderWithContext(_ context.Context, code int, _ string, message string) { + w.Lock() + w.messages = append(w.messages, message) + w.Unlock() +} + +// Messages returns messages collected by a warning collector. +func (w *WarningCollector) Messages() []string { + w.Lock() + defer w.Unlock() + + // return a copy for thread-safety + out := make([]string, len(w.messages)) + copy(out, w.messages) + + return out +} + +// Reset clears the messages, used between tests to reset state. +func (w *WarningCollector) Reset() { + w.Lock() + w.messages = nil + w.Unlock() +} + +// SetupClientWithWarningCollector creates a new client.Client, with a warning handler that writes to a returned WarningCollector. +func SetupClientWithWarningCollector(cfg *rest.Config, scheme *runtime.Scheme) (client.Client, *WarningCollector, error) { + warnSink := &WarningCollector{} + // copy to avoid mutating the passed-in config + newcfg := rest.CopyConfig(cfg) + + newcfg.WarningHandlerWithContext = warnSink + + // Build the client with this config + client, err := client.New(newcfg, client.Options{Scheme: scheme}) + if err != nil { + return nil, nil, fmt.Errorf("error creating new client: %w", err) + } + + return client, warnSink, nil +} diff --git a/pkg/controllers/machinesync/machine_sync_controller_test.go b/pkg/controllers/machinesync/machine_sync_controller_test.go index 1d5c12a07..4d9e409fe 100644 --- a/pkg/controllers/machinesync/machine_sync_controller_test.go +++ b/pkg/controllers/machinesync/machine_sync_controller_test.go @@ -1364,7 +1364,11 @@ var _ = Describe("With a running MachineSync Reconciler", func() { }) - FContext("Updates to MAPI machine warns user if the Synchronized condition is set to false", func() { + Context("Updates to MAPI machine warns user if the Synchronized condition is set to false", func() { + var warnClient client.Client + var warnSink *admissiontestutils.WarningCollector + var warnKomega komega.Komega + BeforeEach(func() { By("Waiting for VAP to be ready") machineVap = &admissionregistrationv1.ValidatingAdmissionPolicy{} @@ -1406,17 +1410,26 @@ var _ = Describe("With a running MachineSync Reconciler", func() { sentinelMachine := mapiMachineBuilder.WithName("sentinel-machine").WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityClusterAPI).Build() Eventually(k8sClient.Create(ctx, sentinelMachine), timeout).Should(Succeed()) - capiSentinelMachine := clusterv1resourcebuilder.Machine().WithName("sentinel-machine").WithNamespace(capiNamespace.Name).Build() - Expect(k8sClient.Create(ctx, capiSentinelMachine)).To(Succeed()) + var err error + warnClient, warnSink, err = admissiontestutils.SetupClientWithWarningCollector(cfg, testScheme) + Expect(err).To(Not(HaveOccurred())) - Eventually(k.Get(capiSentinelMachine)).Should(Succeed()) + warnKomega = komega.New(warnClient) + + Eventually(func(g Gomega) { + warnSink.Reset() // keep each probe self-contained + + err := warnKomega.Update(sentinelMachine, func() { + sentinelMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"} + })() + g.Expect(err).NotTo(HaveOccurred()) + + g.Expect(warnSink.Messages()).To(ContainElement(ContainSubstring("policy in place"))) + }, timeout).Should(Succeed()) - Eventually(k.Update(sentinelMachine, func() { - sentinelMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"} - }), timeout).Should(Succeed()) }) - It("warns the user when the machine is still synchronzing", func() { + It("warns the user when the machine is still synchronizing", func() { By("Setting the Synchronized condition to False") Eventually(k.UpdateStatus(mapiMachine, func() { mapiMachine.Status.Conditions = []mapiv1beta1.Condition{ @@ -1430,12 +1443,63 @@ var _ = Describe("With a running MachineSync Reconciler", func() { } })).Should(Succeed()) - By("Attempting to update the authoritativeAPI should be blocked") - Eventually(k.Update(mapiMachine, func() { - mapiMachine.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI - }), timeout).Should(Succeed()) // This should succeed but show a warning? + By("Attempting to update the authoritativeAPI should emit a warning") + Eventually(func(g Gomega) { + warnSink.Reset() // keep each probe self-contained + + err := warnKomega.Update(mapiMachine, func() { + mapiMachine.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + })() + g.Expect(err).NotTo(HaveOccurred()) + + g.Expect(warnSink.Messages()).To(ContainElement(ContainSubstring("Updating .spec.authoritativeAPI when the Synchronized condition is not true means changes may not take effect"))) + }, timeout).Should(Succeed()) }) + It("warns the user when the machine synchronisation is unknown", func() { + By("Setting the Synchronized condition to Unknown") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.Conditions = []mapiv1beta1.Condition{ + { + Type: consts.SynchronizedCondition, + Status: corev1.ConditionUnknown, + Reason: "ErrorReason", + Message: "Error message", + LastTransitionTime: metav1.Now(), + }, + } + })).Should(Succeed()) + + By("Attempting to update the authoritativeAPI should emit a warning") + Eventually(func(g Gomega) { + warnSink.Reset() // keep each probe self-contained + err := warnKomega.Update(mapiMachine, func() { + mapiMachine.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + })() + g.Expect(err).NotTo(HaveOccurred()) + + g.Expect(warnSink.Messages()).To(ContainElement(ContainSubstring("Updating .spec.authoritativeAPI when the Synchronized condition is not true means changes may not take effect"))) + }, timeout).Should(Succeed()) + }) + + It("warns the user when the machine has no synchronized condition", func() { + By("Setting the conditions to empty") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.Conditions = []mapiv1beta1.Condition{} + })).Should(Succeed()) + + By("Attempting to update the authoritativeAPI should emit a warning") + Eventually(func(g Gomega) { + warnSink.Reset() // keep each probe self-contained + + err := warnKomega.Update(mapiMachine, func() { + mapiMachine.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + })() + g.Expect(err).NotTo(HaveOccurred()) + + g.Expect(warnSink.Messages()).To(ContainElement(ContainSubstring("Updating .spec.authoritativeAPI when the Synchronized condition is not true means changes may not take effect"))) + }, timeout).Should(Succeed()) + }) }) })