diff --git a/manifests/0000_30_cluster-api_09_admission-policies.yaml b/manifests/0000_30_cluster-api_09_admission-policies.yaml index c652936e6..549db86b7 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,54 @@ 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"] + 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: '!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 + 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/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 fc790b918..4d9e409fe 100644 --- a/pkg/controllers/machinesync/machine_sync_controller_test.go +++ b/pkg/controllers/machinesync/machine_sync_controller_test.go @@ -1364,6 +1364,144 @@ var _ = Describe("With a running MachineSync Reconciler", 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{} + 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()) + + var err error + warnClient, warnSink, err = admissiontestutils.SetupClientWithWarningCollector(cfg, testScheme) + Expect(err).To(Not(HaveOccurred())) + + 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()) + + }) + + 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{ + { + Type: consts.SynchronizedCondition, + Status: corev1.ConditionFalse, + 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 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()) + }) + }) + }) })