fix: harden formatOverrideTarget fallback and align IsClusterMatched log

Address review feedback on PR #686:

- formatOverrideTarget: fall back to "Unknown" when the unstructured
  target has an empty Kind, so a malformed snapshot doesn't render as
  `"" "name"` in user-facing error messages.
- applyOverrideRules: drop the NewUnexpectedBehaviorError log wrapper on
  the IsClusterMatched failure path. The outer applyOverrides wrap
  already tags this as ErrUserError, so classifying the log line as
  "unexpected" contradicted the user-error classification.
- applyOverrides: switch the outer wrap from the inline
  fmt.Errorf("%w: ...", controller.ErrUserError, ...) form to
  controller.NewUserError(fmt.Errorf(...)) to match the form used by
  every other call site in the codebase. Behaviorally identical
  (same Error() string, same errors.Is, same trim output at
  workgenerator/controller.go:188-194).
- Add TestFormatOverrideTarget covering namespaced, cluster-scoped, and
  the new empty-Kind fallback (both scopes).
- Trim the verbose doc and test comments introduced by this PR.

Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>

Author: ytimocin

pkg/controllers/workgenerator/override.go

@@ -35,10 +35,9 @@ import (
 	"github.com/kubefleet-dev/kubefleet/pkg/utils/overrider"
 )
 
-// fetchClusterResourceOverrideSnapshots returns the ClusterResourceOverrideSnapshots referenced
-// by the given binding, keyed by the ResourceIdentifier each snapshot's selectors target. The
-// returned map preserves the order in which the binding lists the snapshots, so the caller can
-// apply them in a deterministic sequence.
+// fetchClusterResourceOverrideSnapshots returns the binding's CRO snapshots keyed by the
+// ResourceIdentifier their selectors target. Order matches the binding's snapshot list so
+// callers can apply deterministically.
 //
 // TODO: combine the following two functions into one, as they are very similar.
 func (r *Reconciler) fetchClusterResourceOverrideSnapshots(ctx context.Context, resourceBinding placementv1beta1.BindingObj) (map[placementv1beta1.ResourceIdentifier][]*placementv1beta1.ClusterResourceOverrideSnapshot, error) {
@@ -75,10 +74,9 @@ func (r *Reconciler) fetchClusterResourceOverrideSnapshots(ctx context.Context,
 	return croMap, nil
 }
 
-// fetchResourceOverrideSnapshots returns the ResourceOverrideSnapshots referenced by the given
-// binding, keyed by the ResourceIdentifier each snapshot's selectors target. The returned map
-// preserves the order in which the binding lists the snapshots, so the caller can apply them in
-// a deterministic sequence.
+// fetchResourceOverrideSnapshots returns the binding's RO snapshots keyed by the
+// ResourceIdentifier their selectors target. Order matches the binding's snapshot list so
+// callers can apply deterministically.
 func (r *Reconciler) fetchResourceOverrideSnapshots(ctx context.Context, resourceBinding placementv1beta1.BindingObj) (map[placementv1beta1.ResourceIdentifier][]*placementv1beta1.ResourceOverrideSnapshot, error) {
 	roMap := make(map[placementv1beta1.ResourceIdentifier][]*placementv1beta1.ResourceOverrideSnapshot)
 
@@ -158,8 +156,8 @@ func (r *Reconciler) applyOverrides(resource *placementv1beta1.ResourceContent,
 		}
 		if err := applyOverrideRules(resource, cluster, snapshot.Spec.OverrideSpec.Policy.OverrideRules); err != nil {
 			klog.ErrorS(err, "Failed to apply the override rules", "clusterResourceOverrideSnapshot", klog.KObj(snapshot))
-			return false, fmt.Errorf("%w: ClusterResourceOverrideSnapshot %q failed to apply on %s: %s",
-				controller.ErrUserError, snapshot.Name, formatOverrideTarget(&uResource), err.Error())
+			return false, controller.NewUserError(fmt.Errorf("ClusterResourceOverrideSnapshot %q failed to apply on %s: %s",
+				snapshot.Name, formatOverrideTarget(&uResource), err.Error()))
 		}
 	}
 	klog.V(2).InfoS("Applied clusterResourceOverrideSnapshots", "resource", klog.KObj(&uResource), "numberOfOverrides", len(croMap[key]))
@@ -182,36 +180,38 @@ func (r *Reconciler) applyOverrides(resource *placementv1beta1.ResourceContent,
 			}
 			if err := applyOverrideRules(resource, cluster, snapshot.Spec.OverrideSpec.Policy.OverrideRules); err != nil {
 				klog.ErrorS(err, "Failed to apply the override rules", "resourceOverrideSnapshot", klog.KObj(snapshot))
-				return false, fmt.Errorf("%w: ResourceOverrideSnapshot %q failed to apply on %s: %s",
-					controller.ErrUserError, snapshot.Name, formatOverrideTarget(&uResource), err.Error())
+				return false, controller.NewUserError(fmt.Errorf("ResourceOverrideSnapshot %q failed to apply on %s: %s",
+					snapshot.Name, formatOverrideTarget(&uResource), err.Error()))
 			}
 		}
 		klog.V(2).InfoS("Applied resourceOverrideSnapshots", "resource", klog.KObj(&uResource), "numberOfOverrides", len(roMap[key]))
 	}
 	return resource.Raw == nil, nil
 }
 
-// formatOverrideTarget returns a human-readable identifier for the resource being overridden,
-// suitable for inclusion in a user-facing error message (e.g. `Deployment "my-app" in namespace "default"`).
+// formatOverrideTarget renders the target as e.g. `Deployment "my-app" in namespace "default"`
+// for inclusion in a user-facing error message.
 func formatOverrideTarget(target *unstructured.Unstructured) string {
+	// Fall back to "Unknown" so a snapshot missing `kind` doesn't render as `"" "name"`.
 	kind := target.GetObjectKind().GroupVersionKind().Kind
+	if kind == "" {
+		kind = "Unknown"
+	}
 	if ns := target.GetNamespace(); ns != "" {
 		return fmt.Sprintf("%s %q in namespace %q", kind, target.GetName(), ns)
 	}
 	return fmt.Sprintf("%s %q", kind, target.GetName())
 }
 
-// applyOverrideRules applies the given override rules to the resource for the cluster. Rules
-// whose cluster selector does not match the cluster are skipped. A DeleteOverrideType rule
-// clears the resource and ends rule application; otherwise the rule's JSON patches are applied
-// in order. The first rule that fails returns its raw error to the caller, which is responsible
-// for tagging the failure as a user error.
+// applyOverrideRules applies matching rules to the resource. A DeleteOverrideType rule clears
+// the resource and stops; otherwise JSON patches apply in order. Errors are returned raw — the
+// caller (applyOverrides) tags them as user errors so we don't double-wrap the sentinel.
 func applyOverrideRules(resource *placementv1beta1.ResourceContent, cluster *clusterv1beta1.MemberCluster, rules []placementv1beta1.OverrideRule) error {
 	for _, rule := range rules {
 		matched, err := overrider.IsClusterMatched(cluster, rule)
 		if err != nil {
-			klog.ErrorS(controller.NewUnexpectedBehaviorError(err), "Found an invalid override rule")
-			return err // should not happen though and should be rejected by the webhook
+			klog.ErrorS(err, "Found an invalid override rule")
+			return err
 		}
 		if !matched {
 			continue

pkg/controllers/workgenerator/override_test.go

@@ -429,10 +429,9 @@ func TestFetchResourceOverrideSnapshot(t *testing.T) {
 }
 
 func TestApplyOverrides_clusterScopedResource(t *testing.T) {
-	// FakeManager.IsClusterScopedResources returns (m.APIResources[gvk] == m.IsClusterScopedResource),
-	// so a `false` flag combined with a map of namespace-scoped GVKs makes any GVK NOT in the map
-	// (here: ClusterRole) correctly report as cluster-scoped, while listed GVKs (Deployment) report
-	// as namespace-scoped. Despite the field name, `false` is the right value for these tests.
+	// FakeManager.IsClusterScopedResources returns (APIResources[gvk] == IsClusterScopedResource).
+	// With the flag set to false and APIResources listing namespace-scoped GVKs, listed GVKs
+	// report as namespace-scoped and unlisted GVKs (ClusterRole) report as cluster-scoped.
 	fakeInformer := informer.FakeManager{
 		APIResources: map[schema.GroupVersionKind]bool{
 			{
@@ -455,8 +454,7 @@ func TestApplyOverrides_clusterScopedResource(t *testing.T) {
 		croMap          map[placementv1beta1.ResourceIdentifier][]*placementv1beta1.ClusterResourceOverrideSnapshot
 		wantClusterRole rbacv1.ClusterRole
 		wantErr         error
-		// wantErrSubstr asserts substrings in the returned error message — used to verify
-		// that the per-resource failure identifies the failing override snapshot and target object.
+		// wantErrSubstr asserts the failure message names the failing snapshot and target.
 		wantErrSubstr []string
 		wantDeleted   bool
 	}{
@@ -977,9 +975,8 @@ func TestApplyOverrides_clusterScopedResource(t *testing.T) {
 			},
 		},
 		{
-			// Covers the applyOverrideRules → IsClusterMatched error path. An invalid LabelSelector
-			// Operator causes metav1.LabelSelectorAsSelector to fail; applyOverrideRules returns
-			// the raw error, and applyOverrides wraps it with the snapshot/target identity.
+			// Invalid LabelSelector Operator makes IsClusterMatched fail — exercises the
+			// applyOverrideRules raw-error return that applyOverrides wraps.
 			name: "invalid cluster label selector in clusterResourceOverride",
 			clusterRole: rbacv1.ClusterRole{
 				TypeMeta: clusterRoleType,
@@ -1231,8 +1228,7 @@ func TestApplyOverrides_clusterScopedResource(t *testing.T) {
 }
 
 func TestApplyOverrides_namespaceScopedResource(t *testing.T) {
-	// See the same setup in TestApplyOverrides_clusterScopedResource above for the rationale
-	// behind IsClusterScopedResource: false combined with a map of namespace-scoped GVKs.
+	// See TestApplyOverrides_clusterScopedResource for the IsClusterScopedResource: false rationale.
 	fakeInformer := informer.FakeManager{
 		APIResources: map[schema.GroupVersionKind]bool{
 			{
@@ -1256,8 +1252,7 @@ func TestApplyOverrides_namespaceScopedResource(t *testing.T) {
 		roMap          map[placementv1beta1.ResourceIdentifier][]*placementv1beta1.ResourceOverrideSnapshot
 		wantDeployment appsv1.Deployment
 		wantErr        error
-		// wantErrSubstr asserts substrings in the returned error message — used to verify
-		// that the per-resource failure identifies the failing override snapshot and target object.
+		// wantErrSubstr asserts the failure message names the failing snapshot and target.
 		wantErrSubstr []string
 		wantDeleted   bool
 	}{
@@ -3003,3 +2998,45 @@ func TestReplaceClusterLabelKeyVariables(t *testing.T) {
 		})
 	}
 }
+
+func TestFormatOverrideTarget(t *testing.T) {
+	tests := map[string]struct {
+		kind      string
+		name      string
+		namespace string
+		want      string
+	}{
+		"namespaced resource": {
+			kind:      "Deployment",
+			name:      "my-app",
+			namespace: "default",
+			want:      `Deployment "my-app" in namespace "default"`,
+		},
+		"cluster-scoped resource": {
+			kind: "Namespace",
+			name: "app",
+			want: `Namespace "app"`,
+		},
+		"empty kind falls back to Unknown": {
+			name:      "my-app",
+			namespace: "default",
+			want:      `Unknown "my-app" in namespace "default"`,
+		},
+		"empty kind, cluster-scoped": {
+			name: "app",
+			want: `Unknown "app"`,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			target := &unstructured.Unstructured{}
+			target.SetGroupVersionKind(schema.GroupVersionKind{Kind: tc.kind})
+			target.SetName(tc.name)
+			target.SetNamespace(tc.namespace)
+			if got := formatOverrideTarget(target); got != tc.want {
+				t.Errorf("formatOverrideTarget() = %q, want %q", got, tc.want)
+			}
+		})
+	}
+}