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.

Author: theobarberbany

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: >
+              has(object.status.conditions) ?
+              object.status.conditions.exists(c,
+                c.type == "Synchronized" &&
+                (c.status == "False" || c.status == "Unknown")
+              ) : true
+        - 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))'
+      - expression: '!(( !variables.syncCond || variables.syncBad ) && variables.authAPIChanged)'
         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

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,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 Synchronized=false on resource 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 Synchronized=false on resource 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 Synchronized=false on resource means changes may not take effect")))
+				}, timeout).Should(Succeed())
+			})
 		})
 
 	})