gatewayapi,ir,xds/translator: naming and comment cleanup from code review

- Rename btpClusterSettingsHasSettings to btpSpecHasClusterScopedFields
  (and its test) - the old name read as testing "ClusterSettings" the
  struct, when it actually checks the whole BackendTrafficPolicySpec for
  any CDS-scoped field.
- Trim ir.BackendCluster.Merged's doc comment to a one-liner, consistent
  with the struct's other fields.
- Rewrite singleResolvedClusterName's comment: frame the registry-lookup
  indirection as xds/translator defensively protecting itself against
  not-necessarily-well-formed data from an earlier layer, rather than
  tracing specific (and driftable) legit code paths that could produce a
  ref-without-registration.
- Rename mergeIncompatibleForRule/mergeIncompatibleForNonWeightedRule to
  mergeIncompatibleForWeightedRule/mergeIncompatibleForSingleClusterRule -
  the old names read as a general case and a negated special case of it,
  when they're actually independent implementations for two route-type
  families distinguished by whether their route action supports weighted
  clusters. Split mergeIncompatibleForSingleClusterRule's combined boolean
  into guard clauses with a comment per condition, matching the sibling
  function's established style.
- mergeIncompatibleForWeightedRule: add a comment to the len(backendRefs)
  <= 1 guard, and restructure the final ConsistentHash condition into an
  explicit if/return true with a trailing return false, for consistency
  with the other guard clauses. Its position (checked before session
  persistence, fallback backend, and ConsistentHash) is intentional and
  unchanged: those three checks protect a multi-backend pool from
  fragmenting, which doesn't apply when there's nothing to fragment.

Signed-off-by: Muhammad Waqar <waqar.hameed08@gmail.com>

Author: muwaqar

internal/gatewayapi/backendtrafficpolicy.go

@@ -211,10 +211,10 @@ func (idx *BTPRoutingTypeIndex) LookupGatewayBTRoutingType(gatewayNN types.Names
 	return nil
 }
 
-// btpClusterSettingsHasSettings reports whether spec sets any backend-cluster-scoped (CDS) field —
+// btpSpecHasClusterScopedFields reports whether spec sets any backend-cluster-scoped (CDS) field —
 // either directly inside its embedded ClusterSettings, or via a sibling field on the spec that also
 // affects the generated Cluster resource (UseClientProtocol feeds into HTTPRouteTranslator.asClusterArgs).
-func btpClusterSettingsHasSettings(spec *egv1a1.BackendTrafficPolicySpec) bool {
+func btpSpecHasClusterScopedFields(spec *egv1a1.BackendTrafficPolicySpec) bool {
 	if spec == nil {
 		return false
 	}
@@ -232,7 +232,7 @@ func btpClusterSettingsHasSettings(spec *egv1a1.BackendTrafficPolicySpec) bool {
 
 func hasBTPClusterSettings(btps []*egv1a1.BackendTrafficPolicy) bool {
 	for _, btp := range btps {
-		if btpClusterSettingsHasSettings(&btp.Spec) {
+		if btpSpecHasClusterScopedFields(&btp.Spec) {
 			return true
 		}
 	}
@@ -269,7 +269,7 @@ func BuildBTPClusterSettingsIndex(
 	}
 
 	for _, btp := range btps {
-		if !btpClusterSettingsHasSettings(&btp.Spec) {
+		if !btpSpecHasClusterScopedFields(&btp.Spec) {
 			continue
 		}
 		refs := resolvePolicyTargets(

internal/gatewayapi/backendtrafficpolicy_test.go

@@ -1988,7 +1988,7 @@ func TestBTPLoadBalancerIndexIsConsistentHash(t *testing.T) {
 	}
 }
 
-func TestBtpClusterSettingsHasSettings(t *testing.T) {
+func TestBtpSpecHasClusterScopedFields(t *testing.T) {
 	circuitBreakerSet := &egv1a1.ClusterSettings{CircuitBreaker: &egv1a1.CircuitBreaker{}}
 	useClientProtocolTrue := true
 
@@ -2020,7 +2020,7 @@ func TestBtpClusterSettingsHasSettings(t *testing.T) {
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			require.Equal(t, tc.want, btpClusterSettingsHasSettings(tc.spec))
+			require.Equal(t, tc.want, btpSpecHasClusterScopedFields(tc.spec))
 		})
 	}
 }

internal/gatewayapi/route.go

@@ -280,7 +280,7 @@ func (t *Translator) processHTTPRouteRules(httpRoute *HTTPRouteContext, parentRe
 		for i := range rule.BackendRefs {
 			backendRefs[i] = rule.BackendRefs[i].BackendObjectReference
 		}
-		mergeIncompatible := t.mergeIncompatibleForRule(httpRoute, parentRef, rule.Name, backendRefs, hasRouteLevelClusterSettings, rule.SessionPersistence != nil, gatewayCtx)
+		mergeIncompatible := t.mergeIncompatibleForWeightedRule(httpRoute, parentRef, rule.Name, backendRefs, hasRouteLevelClusterSettings, rule.SessionPersistence != nil, gatewayCtx)
 
 		for i := range rule.BackendRefs {
 			backendNamespace := NamespaceDerefOr(rule.BackendRefs[i].Namespace, httpRoute.GetNamespace())
@@ -698,10 +698,11 @@ func (t *Translator) isFallbackBackend(backendRef gwapiv1.BackendObjectReference
 	return backend != nil && ptr.Deref(backend.Spec.Fallback, false)
 }
 
-// mergeIncompatibleForRule reports whether a rule-level condition makes cluster deduplication
-// unsafe for any of this rule's backendRefs: route-level cluster settings, session persistence,
-// a fallback backend, or ConsistentHash load balancing.
-func (t *Translator) mergeIncompatibleForRule(
+// mergeIncompatibleForWeightedRule reports whether a rule-level condition makes cluster
+// deduplication unsafe for any of this rule's backendRefs: route-level cluster settings, session
+// persistence, a fallback backend, or ConsistentHash load balancing. For HTTP/GRPC, whose
+// weighted-clusters route action can represent multiple distinct clusters in one rule.
+func (t *Translator) mergeIncompatibleForWeightedRule(
 	route RouteContext,
 	parentRef *RouteParentContext,
 	ruleName *gwapiv1.SectionName,
@@ -715,6 +716,8 @@ func (t *Translator) mergeIncompatibleForRule(
 	if hasRouteLevelClusterSettings {
 		return true
 	}
+	// A single backendRef has no multi-backend pool for the checks below to protect —
+	// nothing to fragment, so it's always mergeable at this point.
 	if len(backendRefs) <= 1 {
 		return false
 	}
@@ -729,16 +732,29 @@ func (t *Translator) mergeIncompatibleForRule(
 		}
 	}
 	// ConsistentHash needs the full combined backend pool, not per-identity split clusters.
-	return gatewayCtx != nil && t.BTPLoadBalancerIndex.IsConsistentHash(route.GetRouteType(), utils.NamespacedName(route), utils.NamespacedName(gatewayCtx.Gateway), parentRef.SectionName, ruleName)
+	if gatewayCtx != nil && t.BTPLoadBalancerIndex.IsConsistentHash(route.GetRouteType(), utils.NamespacedName(route), utils.NamespacedName(gatewayCtx.Gateway), parentRef.SectionName, ruleName) {
+		return true
+	}
+	return false
 }
 
-// mergeIncompatibleForNonWeightedRule reports whether hasRouteLevelClusterSettings, or the rule
-// having more than one backendRef, makes cluster deduplication unsafe for a TCP/UDP/TLS rule.
-// Unlike HTTP/GRPC, these route types have no weighted-cluster mechanism at the listener layer,
-// so a rule's backendRefs must always land in a single cluster; letting them merge independently
-// would silently split the rule across clusters the listener can't reference together.
-func mergeIncompatibleForNonWeightedRule(hasRouteLevelClusterSettings bool, backendRefCount int) bool {
-	return hasRouteLevelClusterSettings || backendRefCount > 1
+// mergeIncompatibleForSingleClusterRule reports whether a rule-level condition makes cluster
+// deduplication unsafe for a TCP/UDP/TLS rule. Unlike HTTP/GRPC, these route types have no
+// weighted-cluster mechanism at the listener layer, so a rule's backendRefs must always resolve
+// to a single cluster.
+func mergeIncompatibleForSingleClusterRule(hasRouteLevelClusterSettings bool, backendRefCount int) bool {
+	// A route-level BackendTrafficPolicy's ClusterSettings would incorrectly apply to a
+	// cluster shared with other routes if merged.
+	if hasRouteLevelClusterSettings {
+		return true
+	}
+	// This route type has no weighted-cluster mechanism at the listener layer, so a rule's
+	// backendRefs must always resolve to a single cluster — letting them merge independently
+	// could split the rule across clusters the listener can't reference together.
+	if backendRefCount > 1 {
+		return true
+	}
+	return false
 }
 
 // backendClusterSettingName derives the DestinationSetting name for a backendRef: a merged
@@ -1324,7 +1340,7 @@ func (t *Translator) processGRPCRouteRules(grpcRoute *GRPCRouteContext, parentRe
 		for i := range rule.BackendRefs {
 			backendRefs[i] = rule.BackendRefs[i].BackendObjectReference
 		}
-		mergeIncompatible := t.mergeIncompatibleForRule(grpcRoute, parentRef, rule.Name, backendRefs, hasRouteLevelClusterSettings, false, gatewayCtx)
+		mergeIncompatible := t.mergeIncompatibleForWeightedRule(grpcRoute, parentRef, rule.Name, backendRefs, hasRouteLevelClusterSettings, false, gatewayCtx)
 
 		backendRefNames := make([]string, len(rule.BackendRefs))
 		for i := range rule.BackendRefs {
@@ -1729,7 +1745,7 @@ func (t *Translator) processTLSRouteParentRefs(tlsRoute *TLSRouteContext, resour
 		for _, rule := range tlsRoute.Spec.Rules {
 			gatewayCtx, btpRoutingType, hasRouteLevelClusterSettings := t.resolveRoutingContext(tlsRoute, parentRef, rule.Name)
 			gwIR := t.gatewayXdsIR(gatewayCtx, xdsIR)
-			mergeIncompatible := mergeIncompatibleForNonWeightedRule(hasRouteLevelClusterSettings, len(rule.BackendRefs))
+			mergeIncompatible := mergeIncompatibleForSingleClusterRule(hasRouteLevelClusterSettings, len(rule.BackendRefs))
 			for i := range rule.BackendRefs {
 				backendRefCtx := DirectBackendRef{BackendRef: &rule.BackendRefs[i]}
 				// ds will never be nil here because processDestination returns an empty DestinationSetting for invalid backendRefs.
@@ -1914,7 +1930,7 @@ func (t *Translator) processUDPRouteParentRefs(udpRoute *UDPRouteContext, resour
 
 		gatewayCtx, btpRoutingType, hasRouteLevelClusterSettings := t.resolveRoutingContext(udpRoute, parentRef, udpRoute.Spec.Rules[0].Name)
 		gwIR := t.gatewayXdsIR(gatewayCtx, xdsIR)
-		mergeIncompatible := mergeIncompatibleForNonWeightedRule(hasRouteLevelClusterSettings, len(udpRoute.Spec.Rules[0].BackendRefs))
+		mergeIncompatible := mergeIncompatibleForSingleClusterRule(hasRouteLevelClusterSettings, len(udpRoute.Spec.Rules[0].BackendRefs))
 		for i := range udpRoute.Spec.Rules[0].BackendRefs {
 			backendRefCtx := DirectBackendRef{BackendRef: &udpRoute.Spec.Rules[0].BackendRefs[i]}
 			// ds will never be nil here because processDestination returns an empty DestinationSetting for invalid backendRefs.
@@ -2070,7 +2086,7 @@ func (t *Translator) processTCPRouteParentRefs(tcpRoute *TCPRouteContext, resour
 
 		gatewayCtx, btpRoutingType, hasRouteLevelClusterSettings := t.resolveRoutingContext(tcpRoute, parentRef, tcpRoute.Spec.Rules[0].Name)
 		gwIR := t.gatewayXdsIR(gatewayCtx, xdsIR)
-		mergeIncompatible := mergeIncompatibleForNonWeightedRule(hasRouteLevelClusterSettings, len(tcpRoute.Spec.Rules[0].BackendRefs))
+		mergeIncompatible := mergeIncompatibleForSingleClusterRule(hasRouteLevelClusterSettings, len(tcpRoute.Spec.Rules[0].BackendRefs))
 		for i := range tcpRoute.Spec.Rules[0].BackendRefs {
 			backendRefCtx := DirectBackendRef{BackendRef: &tcpRoute.Spec.Rules[0].BackendRefs[i]}
 			ds, cluster, _, err := t.processBackendRef(destName, i, backendRefCtx, parentRef, tcpRoute, resources, gatewayCtx, btpRoutingType, xdsIR, mergeIncompatible)

internal/gatewayapi/route_test.go

@@ -890,7 +890,7 @@ func TestIsFallbackBackend(t *testing.T) {
 	}
 }
 
-func TestMergeIncompatibleForRule(t *testing.T) {
+func TestMergeIncompatibleForWeightedRule(t *testing.T) {
 	fallbackTrue := true
 	fallbackBackend := &egv1a1.Backend{
 		ObjectMeta: metav1.ObjectMeta{Name: "be-fallback", Namespace: "default"},
@@ -974,7 +974,7 @@ func TestMergeIncompatibleForRule(t *testing.T) {
 				BackendMap:           map[types.NamespacedName]*egv1a1.Backend{{Namespace: "default", Name: "be-fallback"}: fallbackBackend},
 				BTPLoadBalancerIndex: tc.lbIndex,
 			}}
-			got := tr.mergeIncompatibleForRule(route, parentRef, ruleName, tc.backendRefs, tc.hasRouteLevelClusterSettings, tc.sessionPersistent, tc.gatewayCtx)
+			got := tr.mergeIncompatibleForWeightedRule(route, parentRef, ruleName, tc.backendRefs, tc.hasRouteLevelClusterSettings, tc.sessionPersistent, tc.gatewayCtx)
 			require.Equal(t, tc.want, got)
 		})
 	}

internal/ir/xds.go

@@ -1923,10 +1923,7 @@ type BackendCluster struct {
 	Settings []*DestinationSetting `json:"settings,omitempty" yaml:"settings,omitempty"`
 	// Metadata describes the backend resource (Service, Backend, etc.)
 	Metadata *ResourceMetadata `json:"metadata,omitempty" yaml:"metadata,omitempty"`
-	// Merged is true when this cluster was created via backend-identity deduplication
-	// (MergeBackends) and so may be referenced by more than one route. A property derived from
-	// exactly one referencing route (e.g. that route's own hostname) is not reliable for a merged
-	// cluster and must not be used as if it uniquely belonged to this cluster.
+	// Merged is true when this cluster is shared across routes via backend-identity deduplication.
 	Merged bool `json:"merged,omitempty" yaml:"merged,omitempty"`
 }
 

internal/xds/translator/backend_registry.go

@@ -45,13 +45,11 @@ func (t *Translator) getBackendClusters(rd *ir.RouteDestination) []*ir.BackendCl
 	return []*ir.BackendCluster{{Name: rd.Name, Metadata: rd.Metadata}}
 }
 
-// singleResolvedClusterName returns the name of rd's one resolved BackendCluster, when it
-// resolves to exactly one: that's the actual Envoy cluster the backend registered under, which
-// differs from rd's own route-scoped Name once the backend merges. Callers use this in place of
-// rd.Name wherever they reference a cluster directly (not through a weighted-clusters
-// specifier), so the reference stays valid after a merge. Falls back to rd.Name (already correct
-// when nothing merged) whenever zero or more than one cluster resolves, since a direct reference
-// can't represent that anyway.
+// singleResolvedClusterName returns the name of rd's one resolved BackendCluster, when there
+// is exactly one. This layer doesn't assume data from earlier layers is always well-formed:
+// if the ref count isn't exactly one (should be impossible in this call path) or the ref
+// doesn't resolve to a registered BackendCluster, something unexpected has happened, and rd's
+// own route-scoped name is the best available fallback.
 func (t *Translator) singleResolvedClusterName(rd *ir.RouteDestination) string {
 	if bcs := t.getBackendClusters(rd); len(bcs) == 1 {
 		return bcs[0].Name