From 0eaa18680a982deb58f96ee29c09dcc31de9c094 Mon Sep 17 00:00:00 2001 From: Krishnan K M <86150100+krishnankm@users.noreply.github.com> Date: Wed, 8 Jul 2026 08:49:24 +0530 Subject: [PATCH] :sparkles: feat: surface invalid ManifestWork owner label on MWRS status (#1602) * feat: surface invalid ManifestWork owner label on MWRS status Signed-off-by: Krishnan K M * address review: add TODO for hash-label removal Signed-off-by: Krishnan K M --------- Signed-off-by: Krishnan K M --- ...manifestworkreplicaset_deploy_reconcile.go | 17 +++++ .../manifestworkreplicaset_deploy_test.go | 63 +++++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go b/pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go index 248fe8768..44a9e3ea6 100644 --- a/pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go +++ b/pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go @@ -3,12 +3,14 @@ package manifestworkreplicasetcontroller import ( "context" "fmt" + "strings" "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/validation" clusterlister "open-cluster-management.io/api/client/cluster/listers/cluster/v1beta1" worklisterv1 "open-cluster-management.io/api/client/work/listers/work/v1" @@ -21,6 +23,9 @@ import ( "open-cluster-management.io/ocm/pkg/work/helper" ) +// ReasonInvalidManifestWorkName indicates an invalid ManifestWork owner label. +const ReasonInvalidManifestWorkName = "InvalidManifestWorkName" + // deployReconciler is to manage ManifestWork based on the placement. type deployReconciler struct { workApplier *workapplier.WorkApplier @@ -37,6 +42,18 @@ func (d *deployReconciler) reconcile( minRequeue := maxRequeueTime count, total, succeededCount := 0, 0, 0 + // Report invalid ManifestWork owner labels in status. + // TODO: remove this once the owner label uses a hash value instead of + // namespace.name (#1596); the 63-char label limit no longer applies then. + ownerValue := manifestWorkReplicaSetKey(mwrSet) + if verrs := validation.IsValidLabelValue(ownerValue); len(verrs) > 0 { + message := fmt.Sprintf("ManifestWork owner reference %q (namespace.name) is not a valid label value: %s; "+ + "recreate the ManifestWorkReplicaSet with a shorter namespace and/or name so the combined length is at most 63 characters", + ownerValue, strings.Join(verrs, "; ")) + apimeta.SetStatusCondition(&mwrSet.Status.Conditions, getManifestworkApplied(ReasonInvalidManifestWorkName, message)) + return mwrSet, reconcileStop, nil + } + // Clean up ManifestWorks from placements no longer in the spec currentPlacementNames := sets.New[string]() for _, placementRef := range mwrSet.Spec.PlacementRefs { diff --git a/pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go b/pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go index 391c6ed9d..fcdb61a13 100644 --- a/pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go +++ b/pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "strings" "testing" "time" @@ -1750,3 +1751,65 @@ func TestIsConditionReady(t *testing.T) { }) } } + +func TestDeployReconcileOwnerLabelTooLong(t *testing.T) { + // "default." + longName exceeds the 63-char label-value limit, so the owner + // label value is invalid and the reconciler should report it on status. + longName := "mwrset-" + strings.Repeat("x", 60) + mwrSet := helpertest.CreateTestManifestWorkReplicaSet(longName, "default", "place-test") + + fWorkClient := fakeworkclient.NewSimpleClientset(mwrSet) + workInformerFactory := workinformers.NewSharedInformerFactoryWithOptions(fWorkClient, 1*time.Second) + if err := workInformerFactory.Work().V1alpha1().ManifestWorkReplicaSets().Informer().GetStore().Add(mwrSet); err != nil { + t.Fatal(err) + } + mwLister := workInformerFactory.Work().V1().ManifestWorks().Lister() + + placement, placementDecision := helpertest.CreateTestPlacement("place-test", "default", "cls1") + fClusterClient := fakeclusterclient.NewSimpleClientset(placement, placementDecision) + clusterInformerFactory := clusterinformers.NewSharedInformerFactoryWithOptions(fClusterClient, 1*time.Second) + if err := clusterInformerFactory.Cluster().V1beta1().Placements().Informer().GetStore().Add(placement); err != nil { + t.Fatal(err) + } + if err := clusterInformerFactory.Cluster().V1beta1().PlacementDecisions().Informer().GetStore().Add(placementDecision); err != nil { + t.Fatal(err) + } + + pmwDeployController := deployReconciler{ + workApplier: workapplier.NewWorkApplierWithTypedClient(fWorkClient, mwLister), + manifestWorkLister: mwLister, + placeDecisionLister: clusterInformerFactory.Cluster().V1beta1().PlacementDecisions().Lister(), + placementLister: clusterInformerFactory.Cluster().V1beta1().Placements().Lister(), + } + + mwrSet, state, err := pmwDeployController.reconcile(context.TODO(), mwrSet) + if err != nil { + t.Fatal("expected no error so the invalid name is reported via status, got ", err) + } + if state != reconcileStop { + t.Fatal("expected reconcileStop for a permanently invalid owner label, got ", state) + } + + cond := apimeta.FindStatusCondition(mwrSet.Status.Conditions, workapiv1alpha1.ManifestWorkReplicaSetConditionManifestworkApplied) + if cond == nil { + t.Fatal("ManifestworkApplied condition not found ", mwrSet.Status.Conditions) + } + if cond.Status != metav1.ConditionFalse { + t.Fatal("expected ManifestworkApplied=False, got ", cond) + } + if cond.Reason != ReasonInvalidManifestWorkName { + t.Fatal("expected Reason ReasonInvalidManifestWorkName, got ", cond.Reason) + } + if !strings.Contains(cond.Message, "63") { + t.Fatal("expected message to reference the 63-char limit, got ", cond.Message) + } + + // No ManifestWork should have been applied for the invalid owner value. + works, err := fWorkClient.WorkV1().ManifestWorks("cls1").List(context.TODO(), metav1.ListOptions{}) + if err != nil { + t.Fatal(err) + } + if len(works.Items) != 0 { + t.Fatal("expected no ManifestWork to be applied, got ", len(works.Items)) + } +}