feat: Add cross-namespace reference validation to generated code by sapphirew · Pull Request #699 · aws-controllers-k8s/code-generator

sapphirew · 2026-05-08T22:39:16Z

Issue #, if available:

Description of changes:

Replace the legacy unconditional namespace-override block in ResolveReferencesForField with a call to the shared runtime helper ackrt.ValidateCrossNamespaceReference. This centralizes cross-namespace reference validation so that when --enable-cross-namespace is disabled, references targeting a different namespace are rejected with a terminal error.

The generated code captures three return values (namespace, isCrossNs, error) from the helper. The isCrossNs signal is suppressed in generated code since warning conditions are handled centrally by the reconciler.

Helm chart templates are updated to expose the --enable-cross-namespace flag (default: true for Phase 1 warning behavior) covering resource references, secret references, and field exports.

Changes:

resource_reference.go: emit ValidateCrossNamespaceReference call with EnableCrossNamespace field and three return values
resource_reference_test.go: update all expected output strings to match the new helper call shape
deployment.yaml.tpl: add --enable-cross-namespace flag to container args
values.yaml.tpl: add enableCrossNamespace: true with description
values.schema.json.tpl: add enableCrossNamespace schema entry

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-prow · 2026-05-08T22:39:22Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sapphirew
Once this PR has been reviewed and has the lgtm label, please assign knottnt for approval by writing /assign @knottnt in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Replace the legacy unconditional namespace-override block in ResolveReferencesForField with a call to the shared runtime helper ackrt.ValidateCrossNamespaceReference. This centralizes cross-namespace reference validation so that when --enable-cross-namespace is disabled, references targeting a different namespace are rejected with a terminal error. The generated code captures three return values (namespace, isCrossNs, error) from the helper. The isCrossNs signal is suppressed in generated code since warning conditions are handled centrally by the reconciler. Helm chart templates are updated to expose the --enable-cross-namespace flag (default: true for Phase 1 warning behavior) covering resource references, secret references, and field exports. Changes: - resource_reference.go: emit ValidateCrossNamespaceReference call with EnableCrossNamespace field and three return values - resource_reference_test.go: update all expected output strings to match the new helper call shape - deployment.yaml.tpl: add --enable-cross-namespace flag to container args - values.yaml.tpl: add enableCrossNamespace: true with description - values.schema.json.tpl: add enableCrossNamespace schema entry

kprahulraj · 2026-05-21T23:09:21Z

Doc changes is added - aws-controllers-k8s/docs#37

knottnt · 2026-05-22T17:59:25Z

-			outPrefix += fmt.Sprintf("%snamespace := ko.ObjectMeta.GetNamespace()\n", innerIndent)
-			outPrefix += fmt.Sprintf("%sif arr.Namespace != nil && *arr.Namespace != \"\" {\n", innerIndent)
-			outPrefix += fmt.Sprintf("%s\tnamespace = *arr.Namespace\n", innerIndent)
+			outPrefix += fmt.Sprintf("%snamespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReference(\n", innerIndent)


Q: does the function comment need to be updated to reflect the new output?

Good catch — updated the function comment to reflect the new generated output (ValidateCrossNamespaceReference call with three return values, error handling, and isCrossNs warning/condition logic).

knottnt · 2026-05-22T19:14:54Z

 {{ "{{- end }}" }}
        - {{ "--enable-carm={{ .Values.enableCARM }}" }}
+        - {{ "--enable-cross-namespace={{ .Values.enableCrossNamespace }}" }}
+        - {{ "--enable-cross-namespace-references={{ .Values.enableCrossNamespaceReferences }}" }}


I don't see this flag in the associated runtime changes.

You're right — this was a leftover from the previous spec. The --enable-cross-namespace-references flag was superseded by --enable-cross-namespace in the runtime. Removed it.

knottnt · 2026-05-22T19:15:15Z

+# When false (default), the controller rejects any reference whose
+# Namespace field differs from the namespace of the resource containing
+# the reference.
+enableCrossNamespaceReferences: false


I don't see this flag in the associated runtime changes or in the values.schema.json.tpl.

Same issue — removed the stale enableCrossNamespaceReferences entry. Only enableCrossNamespace: true remains, which matches the runtime flag and is already in values.schema.json.tpl.

@sapphirew

Issue #, if available: Description of changes: Introduce a single CLI flag that controls all three categories of cross-namespace behavior: resource references, secret references, and field exports. It defaults to true with deprecation warnings to start with. It will default to false later in order to follow best security practice. Changes: - Add `--enable-cross-namespace` CLI flag with default value `true` - Update ValidateCrossNamespaceReference to return (string, bool, error) - Add ValidateCrossNamespaceReferenceString convenience wrapper - Add `ACK.CrossNamespaceOptInRequired` condition type - Integrate cross-namespace check into field export reconciler - Refactor SecretValueFromReference to extract ownerNamespace (cross-namespace validation for secrets is handled by generated code in aws-controllers-k8s/code-generator#699) - Add table-driven unit tests for both helper functions - Update error messages to reference new flag name Note: This PR builds on @sapphirew work in [#239](#239) and addresses review feedback from @cheeseandcereal : - Use nil-safe targetName in field export log line - Use lookup-or-create pattern in setCrossNsOptInRequiredCondition to avoid duplicate conditions - Return ownerNamespace (not empty string) on error for defensive fail-closed behavior By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

- Update ResolveReferencesForField function comment to reflect new generated output with ValidateCrossNamespaceReference call, three return values, and cross-namespace warning/condition logic - Remove stale --enable-cross-namespace-references flag from deployment.yaml.tpl (superseded by --enable-cross-namespace) - Remove stale enableCrossNamespaceReferences value from values.yaml.tpl (superseded by enableCrossNamespace)

michaelhtm · 2026-05-28T19:25:29Z

-//
-// Sample output (resolving nested lists of structs containing references):
-//
-//	if ko.Spec.Notification != nil {
-//		for f0idx, f0iter := range ko.Spec.Notification.LambdaFunctionConfigurations {
-//			if f0iter.Filter != nil {
-//				if f0iter.Filter.Key != nil {
-//					for f1idx, f1iter := range f0iter.Filter.Key.FilterRules {
-//						if f1iter.ValueRef != nil && f1iter.ValueRef.From != nil {
-//							hasReferences = true
-//							arr := f1iter.ValueRef.From
-//							if arr.Name == nil || *arr.Name == "" {
-//								return hasReferences, fmt.Errorf("provided resource reference is nil or empty: Notification.LambdaFunctionConfigurations.Filter.Key.FilterRules.ValueRef")
-//							}
-//							obj := &svcapitypes.Bucket{}
-//							if err := getReferencedResourceState_Bucket(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
-//								return hasReferences, err
-//							}
-//							ko.Spec.Notification.LambdaFunctionConfigurations[f0idx].Filter.Key.FilterRules[f1idx].Value = (*string)(obj.Spec.Name)
-//						}
-//					}
-//				}
-//			}
-//		}
-//	}


nit: why is this removed?

Good catch — restored. The new sample shows the simpler generated output that uses ackrt.HandleCrossNamespaceReference from runtime#249.

michaelhtm · 2026-05-28T20:48:20Z

+							namespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReference(
+								rm.cfg.EnableCrossNamespace,
+								ko.ObjectMeta.GetNamespace(),
+								arr.Namespace,
+								*arr.Name,
+							)
+							if err != nil {
+								return hasReferences, err
+							}
+							if isCrossNs {
+								ackrtlog.FromContext(ctx).Info("cross-namespace resource reference detected; "+
+									"this behavior will be disabled by default in a future release. "+
+									"Set --enable-cross-namespace to preserve this behavior.",
+									"ownerNamespace", ko.ObjectMeta.GetNamespace(),
+									"targetNamespace", *arr.Namespace,
+									"referenceName", *arr.Name,
+								)
+								crossNsMsg := fmt.Sprintf("Cross-namespace resource reference detected: "+
+									"resource in namespace %q references %q in namespace %q. "+
+									"Cross-namespace behavior will be disabled by default in a future release. "+
+									"Set --enable-cross-namespace=true to preserve this behavior.",
+									ko.ObjectMeta.GetNamespace(), *arr.Name, *arr.Namespace)
+								setCrossNamespaceCondition(ko, crossNsMsg)


can we make this block a helper function? maybe in runtime?

Done — moved into runtime as ackrt.HandleCrossNamespaceReference (see aws-controllers-k8s/runtime#249). The block in generated code collapses from ~14 lines to a single helper call.

michaelhtm · 2026-05-28T20:51:24Z

+
+// setCrossNamespaceCondition sets the ACK.CrossNamespaceOptInRequired condition
+// on the resource to notify users that their cross-namespace usage will require
+// explicit opt-in in a future release.
+func setCrossNamespaceCondition(ko *svcapitypes.{{ .CRD.Names.Camel }}, message string) {
+	if ko.Status.Conditions == nil {
+		ko.Status.Conditions = []*ackv1alpha1.Condition{}
+	}
+	var crossNsCond *ackv1alpha1.Condition
+	for _, c := range ko.Status.Conditions {
+		if c.Type == ackv1alpha1.ConditionTypeCrossNamespaceOptInRequired {
+			crossNsCond = c
+			break
+		}
+	}
+	if crossNsCond == nil {
+		crossNsCond = &ackv1alpha1.Condition{
+			Type: ackv1alpha1.ConditionTypeCrossNamespaceOptInRequired,
+		}
+		ko.Status.Conditions = append(ko.Status.Conditions, crossNsCond)
+	}
+	now := metav1.Now()
+	crossNsCond.LastTransitionTime = &now
+	crossNsCond.Status = corev1.ConditionTrue
+	crossNsCond.Message = &message
+}


nit: this could also be a runtime function, similar to https://github.com/aws-controllers-k8s/runtime/blob/main/pkg/runtime/reconciler.go#L555

Good call — done. The lookup-or-create logic now lives in the runtime as ackrt.SetCrossNamespaceOptInRequired, mirroring the field export reconciler pattern you linked. The per-resource generated function is gone.

michaelhtm · 2026-05-28T20:53:03Z

+{{ if .CRD.HasReferenceFields -}}
+{{ end -}}
+


what does this part do?

Nothing — that was leftover dead code from when setCrossNamespaceCondition was briefly defined here before being moved. Removed it.

- Replace inlined cross-namespace warning + condition logic with calls to the new ackrt.HandleCrossNamespaceReference runtime helper. This collapses ~14 lines of generated code per cross-namespace check into a single helper call. Applies to both resource references (resource_reference.go) and secret references (set_sdk.go). - Remove the per-package setCrossNamespaceCondition function from the sdk.go.tpl template; the runtime helper now owns the lookup-or-create pattern, avoiding 26 lines of generated code per resource. - Drop the now-unused ackrtlog import and var-block from the references.go.tpl template since the runtime helper handles logging. - Remove the empty {{ if .CRD.HasReferenceFields -}}{{ end -}} block at the bottom of references.go.tpl that was left over from the previous refactor. - Restore the third sample output (resolving nested lists of structs containing references) in the ResolveReferencesForField doc comment, updated to reflect the new generated output. - Update set_sdk_test.go and resource_reference_test.go expected outputs to match the simplified generated code. Requires runtime helpers added in aws-controllers-k8s/runtime PR ( HandleCrossNamespaceReference, SetCrossNamespaceOptInRequired, CrossNamespaceRefKindResource, CrossNamespaceRefKindSecret).

michaelhtm · 2026-05-29T19:32:16Z

+//		namespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReference(
+//			rm.cfg.EnableCrossNamespace,
+//			ko.ObjectMeta.GetNamespace(),
+//			arr.Namespace,
+//			*arr.Name,
+//		)
+//		if err != nil {
+//			return hasReferences, err
+//		}
+//		if isCrossNs {
+//			ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
+//				ctx, ko.Status.Conditions,
+//				ackrt.CrossNamespaceRefKindResource,
+//				ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
+//			)
+//		}


i was hoping this entire section would be inside the helper function..and we won't need to return isCrossNs

sure, moved to the helper function

Address review feedback on aws-controllers-k8s/code-generator#699 to collapse the validate + handle flow into a single helper call so generated code does not need to inspect or branch on the isCrossNamespace return value. - Add ResolveCrossNamespaceReference(*string namespace): orchestrates ValidateCrossNamespaceReference + HandleCrossNamespaceReference, returning only (resolvedNamespace, error) - Add ResolveCrossNamespaceReferenceString(string namespace): the string-namespace counterpart for SecretKeyReference/FieldExportTarget callers - Both accept a *[]*Condition pointer so the conditions slice is updated in place; passing nil is safe and skips condition handling - Add unit tests covering same-namespace, nil/empty namespace, cross namespace with flag enabled, cross namespace with flag disabled, and nil conditions pointer for both helpers

Address review feedback from michaelhtm (aws-controllers-k8s#699) to fold the entire validate + handle flow into a single helper call. The generated code no longer needs to inspect or branch on isCrossNs. - resource_reference.go: replace ValidateCrossNamespaceReference + Handle block with single ResolveCrossNamespaceReference call (3 returns -> 2) - set_sdk.go: same simplification for secret references using ResolveCrossNamespaceReferenceString - Update doc comments in both generators to reflect the new output - Update 7 test expectations in resource_reference_test.go and 7 in set_sdk_test.go Requires runtime helpers added in aws-controllers-k8s/runtime#249 (ResolveCrossNamespaceReference, ResolveCrossNamespaceReferenceString).

ack-prow · 2026-05-29T20:04:16Z

@sapphirew: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
documentdb-controller-test	`77c3c71`	link	true	`/test documentdb-controller-test`
cloudfront-controller-test	`77c3c71`	link	true	`/test cloudfront-controller-test`
efs-controller-test	`77c3c71`	link	true	`/test efs-controller-test`
prometheusservice-controller-test	`77c3c71`	link	true	`/test prometheusservice-controller-test`
apigatewayv2-controller-test	`77c3c71`	link	true	`/test apigatewayv2-controller-test`
acm-controller-test	`77c3c71`	link	true	`/test acm-controller-test`
dynamodb-controller-test	`77c3c71`	link	true	`/test dynamodb-controller-test`
ecr-controller-test	`77c3c71`	link	true	`/test ecr-controller-test`
iam-controller-test	`77c3c71`	link	true	`/test iam-controller-test`
eventbridge-controller-test	`77c3c71`	link	true	`/test eventbridge-controller-test`
s3-controller-test	`77c3c71`	link	true	`/test s3-controller-test`
pipes-controller-test	`77c3c71`	link	true	`/test pipes-controller-test`
eks-controller-test	`77c3c71`	link	true	`/test eks-controller-test`
ec2-controller-test	`77c3c71`	link	true	`/test ec2-controller-test`
lambda-controller-test	`77c3c71`	link	true	`/test lambda-controller-test`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

ack-prow Bot requested review from a-hilaly and knottnt May 8, 2026 22:39

sapphirew changed the title ~~feat: Add cross-namespace reference validation to generated code~~ [Draft]feat: Add cross-namespace reference validation to generated code May 8, 2026

sapphirew force-pushed the cross-namespace-refs branch from 6f2fb42 to 0511503 Compare May 20, 2026 00:35

kprahulraj mentioned this pull request May 21, 2026

docs: add breaking changes page for cross-namespace default change aws-controllers-k8s/docs#37

Open

This was referenced May 22, 2026

feat(runtime): Add --enable-cross-namespace flag aws-controllers-k8s/runtime#239

Closed

feat(runtime): Add --enable-cross-namespace flag aws-controllers-k8s/runtime#248

Merged

knottnt reviewed May 22, 2026

View reviewed changes

sapphirew force-pushed the cross-namespace-refs branch from 0511503 to 30bdd70 Compare May 27, 2026 22:36

sapphirew changed the title ~~[Draft]feat: Add cross-namespace reference validation to generated code~~ feat: Add cross-namespace reference validation to generated code May 27, 2026

sapphirew requested a review from knottnt May 27, 2026 22:55

michaelhtm reviewed May 28, 2026

View reviewed changes

sapphirew mentioned this pull request May 28, 2026

feat(runtime): Add cross-namespace deprecation helpers aws-controllers-k8s/runtime#249

Open

sapphirew requested a review from michaelhtm May 29, 2026 17:58

michaelhtm reviewed May 29, 2026

View reviewed changes

sapphirew requested a review from michaelhtm May 29, 2026 21:24

Conversation

sapphirew commented May 8, 2026

Uh oh!

ack-prow Bot commented May 8, 2026

Uh oh!

kprahulraj commented May 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ack-prow Bot commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants