Merge pull request #398 from theobarberbany/tb/sync-warn

OCPCLOUD-2636: Warn users when attempting to change Authoritative API but Synchronized condition is false

Author: openshift-merge-bot[bot]

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

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
+}

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())
+			})
+		})
+
 	})
 })