✨ 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 <krishnankaruvattu@gmail.com>

* address review: add TODO for hash-label removal

Signed-off-by: Krishnan K M <krishnankaruvattu@gmail.com>

---------

Signed-off-by: Krishnan K M <krishnankaruvattu@gmail.com>

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 {

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