diff --git a/pkg/controller/nodelink/nodelink_controller.go b/pkg/controller/nodelink/nodelink_controller.go index 0c6b2e91e1..17d5e7b57c 100644 --- a/pkg/controller/nodelink/nodelink_controller.go +++ b/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")) +} diff --git a/pkg/controller/nodelink/nodelink_controller_test.go b/pkg/controller/nodelink/nodelink_controller_test.go index fced046aea..fb2b5fb9c3 100644 --- a/pkg/controller/nodelink/nodelink_controller_test.go +++ b/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