OCPBUGS-6075: nodelink: log and emit events for invalid labels

pkg/controller/nodelink/nodelink_controller.go

@@ -9,6 +9,9 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/client-go/tools/events"
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
@@ -39,6 +42,7 @@ type ReconcileNodeLink struct {
 	listNodesByFieldFunc    func(ctx context.Context, key, value string) ([]corev1.Node, error)
 	listMachinesByFieldFunc func(ctx context.Context, key, value string) ([]machinev1.Machine, error)
 	nodeReadinessCache      map[string]bool
+	eventRecorder           events.EventRecorder
 }
 
 // Add creates a new Nodelink Controller and adds it to the Manager. The Manager will set fields on the Controller
@@ -149,7 +153,8 @@ func newReconciler(mgr manager.Manager) (*ReconcileNodeLink, error) {
 	}
 
 	r := ReconcileNodeLink{
-		client: mgr.GetClient(),
+		client:        mgr.GetClient(),
+		eventRecorder: mgr.GetEventRecorder("nodelink-controller"),
 	}
 	r.nodeReadinessCache = make(map[string]bool)
 
@@ -248,8 +253,19 @@ func (r *ReconcileNodeLink) Reconcile(ctx context.Context, request reconcile.Req
 
 	if !reflect.DeepEqual(node, modNode) {
 		klog.V(3).Infof("Node %q has changed, updating", modNode.GetName())
+
+		if errs := validateNodeLabels(modNode.Labels); len(errs) > 0 {
+			klog.Errorf("invalid labels on node %q: %s", modNode.GetName(), errs.ToAggregate().Error())
+			r.eventRecorder.Eventf(machine, nil, corev1.EventTypeWarning, "InvalidNodeLabels", "Update",
+				"invalid labels on node %q: %s", modNode.GetName(), errs.ToAggregate().Error())
+			return reconcile.Result{}, fmt.Errorf("invalid labels on node %q: %s", modNode.GetName(), errs.ToAggregate().Error())
+		}
+
 		if err := r.client.Update(context.Background(), modNode); err != nil {
-			return reconcile.Result{}, fmt.Errorf("error updating node: %v", err)
+			klog.Errorf("error updating node %q: %v", modNode.GetName(), err)
+			r.eventRecorder.Eventf(machine, nil, corev1.EventTypeWarning, "FailedUpdateNode", "Update",
+				"error updating node %q: %v", modNode.GetName(), err)
+			return reconcile.Result{}, fmt.Errorf("error updating node %q: %v", modNode.GetName(), err)
 		}
 	}
 
@@ -536,3 +552,8 @@ func isNodeReady(node *corev1.Node) bool {
 	}
 	return false
 }
+
+// validateNodeLabels validates that all labels on a node conform to Kubernetes label requirements.
+func validateNodeLabels(labels map[string]string) field.ErrorList {
+	return metav1validation.ValidateLabels(labels, field.NewPath("metadata.labels"))
+}

pkg/controller/nodelink/nodelink_controller_test.go

@@ -13,6 +13,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes/scheme"
+	"k8s.io/client-go/tools/record"
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -118,7 +119,8 @@ type fakeReconciler struct {
 func newFakeReconciler(client client.Client, machine *machinev1.Machine, node *corev1.Node) fakeReconciler {
 	r := fakeReconciler{
 		ReconcileNodeLink: ReconcileNodeLink{
-			client: client,
+			client:        client,
+			eventRecorder: record.NewEventRecorderAdapter(record.NewFakeRecorder(32)),
 		},
 		fakeNodeIndexer:    make(map[string]corev1.Node),
 		fakeMachineIndexer: make(map[string]machinev1.Machine),
@@ -569,6 +571,77 @@ func TestReconcile(t *testing.T) {
 	}
 }
 
+func TestReconcileWithInvalidLabels(t *testing.T) {
+	testCases := []struct {
+		name        string
+		specLabels  map[string]string
+		expectError bool
+		errContains string
+	}{
+		{
+			name:        "valid labels succeed",
+			specLabels:  map[string]string{"valid-key": "valid-value"},
+			expectError: false,
+		},
+		{
+			name:        "invalid label value is rejected",
+			specLabels:  map[string]string{"foo": "bar/baz"},
+			expectError: true,
+			errContains: "invalid labels",
+		},
+		{
+			name:        "invalid label key is rejected",
+			specLabels:  map[string]string{"invalid key with spaces": "value"},
+			expectError: true,
+			errContains: "invalid labels",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			testMachine := machine("testMachine", "test-provider-id", nil, nil, nil)
+			testMachine.Spec.Labels = tc.specLabels
+			testNode := node("testNode", "test-provider-id", nil, nil)
+
+			rec := record.NewFakeRecorder(32)
+			r := newFakeReconciler(
+				fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(testNode, testMachine).WithStatusSubresource(&machinev1.Machine{}).Build(),
+				testMachine, testNode,
+			)
+			r.eventRecorder = record.NewEventRecorderAdapter(rec)
+
+			request := reconcile.Request{
+				NamespacedName: client.ObjectKey{
+					Namespace: metav1.NamespaceNone,
+					Name:      testNode.Name,
+				},
+			}
+
+			_, err := r.Reconcile(ctx, request)
+			if tc.expectError {
+				if err == nil {
+					t.Fatalf("expected error but got nil")
+				}
+				if !strings.Contains(err.Error(), tc.errContains) {
+					t.Errorf("expected error containing %q, got: %v", tc.errContains, err)
+				}
+				select {
+				case event := <-rec.Events:
+					if !strings.Contains(event, "InvalidNodeLabels") {
+						t.Errorf("expected InvalidNodeLabels event, got: %s", event)
+					}
+				default:
+					t.Error("expected a warning event to be emitted")
+				}
+			} else {
+				if err != nil {
+					t.Fatalf("unexpected error: %v", err)
+				}
+			}
+		})
+	}
+}
+
 func TestIndexNodeByProviderID(t *testing.T) {
 	testCases := []struct {
 		object   client.Object