feat(runtime): Add --enable-cross-namespace flag by sapphirew · Pull Request #239 · aws-controllers-k8s/runtime

sapphirew · 2026-05-07T00:44:55Z

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.CrossNamespaceDeprecation condition type
Integrate cross-namespace check into SecretValueFromReference
Integrate cross-namespace check into field export reconciler
Update error messages to reference new flag name

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-07T00:45:00Z

[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 a-hilaly for approval by writing /assign @a-hilaly 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

Introduce a single CLI flag that controls all three categories of cross-namespace behavior: resource references, secret references, and field exports. Phase 1 defaults to true with deprecation warnings. Changes: - Rename --enable-cross-namespace-references to --enable-cross-namespace - Change default from false to true (Phase 1 warning behavior) - Update ValidateCrossNamespaceReference to return (string, bool, error) - Add ValidateCrossNamespaceReferenceString convenience wrapper - Add ACK.CrossNamespaceDeprecation condition type - Integrate cross-namespace check into SecretValueFromReference - Integrate cross-namespace check into field export reconciler - Add table-driven unit tests for both helper functions - Update error messages to reference new flag name

sapphirew · 2026-05-20T00:46:36Z

/retest

cheeseandcereal

First-pass review against the ACK rubric. Single squashed commit (rule I passes); no generator.yaml/helm/E2E surface in this PR (rules A–H, J don't apply — this is runtime). Most concerns are around scope alignment between the PR description and what actually ships on this branch.

cheeseandcereal · 2026-05-20T21:28:15Z


+	// Determine the namespace to use for fetching the secret.
+	// Cross-namespace validation, logging, and condition-setting are handled
+	// by the generated code before calling this method.


The PR description says this PR "Integrate[s] cross-namespace check into SecretValueFromReference", but this hunk does the opposite — it removes namespace logic and the comment defers validation to "the generated code before calling this method." That generated-side change isn't in this PR (and would live in code-generator, not runtime).

Net effect on this branch: --enable-cross-namespace=false does not block cross-namespace secret references — only field exports get gated, and resource references are gated indirectly by WithReferencesResolvedCondition. Two ways to close this:

Wire ValidateCrossNamespaceReferenceString(r.cfg.EnableCrossNamespace, ownerNamespace, ref.Namespace, ref.Name) into SecretValueFromReference here, parallel to field_export_reconciler.go. Keeps the runtime self-contained for tagged releases.

If you'd rather centralize in codegen, please trim the PR description to match scope and link the follow-up code-generator PR.

Addressed in #248 - updated PR description to clarify that cross-namespace validation for secrets is handled by generated code in aws-controllers-k8s/code-generator#699.

SecretValueFromReference here only extracts ownerNamespace; validation is deferred to the caller

cheeseandcereal · 2026-05-20T21:28:15Z

+			"targetNamespace", r.getTargetNamespace(&desired),
+			"targetName", *desired.Spec.To.Name,
+		)
+		r.setCrossNsOptInRequiredCondition(&desired)


*desired.Spec.To.Name is dereferenced unconditionally on the log line above — but validateFieldExportNamespace (line 204) explicitly guards desired.Spec.To.Name != nil before reading the same field. Pre-existing writeToConfigMap/writeToSecret paths already deref unguarded too, so this PR isn't introducing the panic, but the inconsistency within new code is worth resolving: either drop the nil-guard in validateFieldExportNamespace (callers downstream already assume non-nil), or thread the safe targetName through and use it here. Pick one and be consistent.

Fixed in #248 - using nil-safe targetName variable with guard check before deref, consistent with validateFieldExportNamespace.

cheeseandcereal · 2026-05-20T21:28:15Z

+		Status:  corev1.ConditionTrue,
+		Message: &message,
+	}
+	desired.Status.Conditions = append(desired.Status.Conditions, condition)


setCrossNsOptInRequiredCondition appends unconditionally. Safe today because resetConditions runs at the top of Sync (line 151), but it diverges from the lookup-or-create pattern used by patchRecoverableCondition (line 517) and patchTerminalCondition (line 551). If resetConditions is ever moved or skipped, this will accumulate duplicates silently. Prefer the existing pattern (search by type, update if present, append if not) so this code reads the same as its neighbors.

Fixed in #248 — now uses lookup-or-create pattern (search by type, update if present, append if not) matching patchRecoverableCondition/patchTerminalCondition.

cheeseandcereal · 2026-05-20T21:28:15Z

+	// reference specifies a Namespace that differs from the namespace of the
+	// resource containing the reference, and the
+	// --enable-cross-namespace flag is not enabled.
+	ResourceReferenceCrossNamespaceNotAllowed = fmt.Errorf(


--enable-cross-namespace is a bool flag; Set --enable-cross-namespace=true to allow it. is what operators actually need to type. Worth flipping the message text. Also: while the flag still defaults to true, this terminal error will only ever fire when an operator explicitly opted out — so the wording is fine. When the default flips, consider also referencing the ACK.CrossNamespaceOptInRequired condition so users can audit their fleet before the flip lands.

Fixed in #248 — error message now says Set --enable-cross-namespace=true to allow it

cheeseandcereal · 2026-05-20T21:28:15Z

+	if enableCrossNamespace {
+		return *refNamespace, true, nil
+	}
+	return "", false, ackerr.ResourceReferenceCrossNamespaceNotAllowedFor(


Minor: returning "" for resolvedNamespace alongside the error is technically fine — the contract says callers must check err first — but it's a foot-gun if a future caller ignores the error and falls through to an empty-namespace Get. Either return ownerNamespace defensively (so a misuse fails closed on the owner ns rather than cluster-scoped), or return *refNamespace so the rejected value is at least diagnostic.

Fixed in #248 - returns ownerNamespace instead of "" so a misuse fails closed on the owner namespace.

cheeseandcereal · 2026-05-20T21:28:15Z

+	rm.AssertNotCalled(t, "ReadOne", ctx, desired)
+	// No downstream operations should occur
+	rm.AssertNotCalled(t, "Create", ctx, desired)
+	rm.AssertNotCalled(t, "Update")


Good coverage on the reference-resolution rejection path. Three gaps still uncovered for the new code paths most likely to regress later:

field_export_reconciler.Sync rejection branch (line 158–159) when EnableCrossNamespace=false and the FieldExport target is a different namespace.

setCrossNsOptInRequiredCondition is set on the FieldExport when flag=true and namespaces differ (the deprecation-warning happy path).

WithReferencesResolvedCondition (pkg/condition/condition.go:317-318): a unit test asserting the new ResourceReferenceCrossNamespaceNotAllowed branch resolves to ConditionFalse (not ConditionUnknown).

Non-blocking, but easy adds while the helper signature is fresh.

Will add as a follow up

cheeseandcereal · 2026-05-20T21:28:15Z

 	// and we'll use the custom role to manage the AWSResource
 	ConditionTypeIAMRoleSelected ConditionType = "ACK.IAMRoleSelected"
+	// ConditionTypeCrossNamespaceOptInRequired indicates that the resource uses
+	// cross-namespace behavior (resource references, secret references, or


Naming inconsistency: PR description calls this ACK.CrossNamespaceDeprecation, the type added here is ACK.CrossNamespaceOptInRequired. The latter is the better of the two (describes the action operators must take), but please update the PR body so downstream release notes don't spread the wrong name — and be aware that the condition Type string is part of the user-visible API.

Fixed in #248 - PR description updated to use ACK.CrossNamespaceOptInRequired.

kprahulraj · 2026-05-20T22:20:32Z

The ec2-controller-test failure is unrelated to this PR. It's a known issue where system tags (services.k8s.aws/*) leak into spec.tags. A possible fix should be #246

sapphirew · 2026-05-21T18:43:10Z

/retest

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

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

kprahulraj · 2026-05-22T07:41:23Z

Addressed the comments in #248

ack-prow Bot requested review from jlbutler and michaelhtm May 7, 2026 00:44

sapphirew marked this pull request as draft May 7, 2026 00:45

ack-prow Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 7, 2026

sapphirew force-pushed the feat/enable-cross-namespace branch from 4a9648c to 4eb656a Compare May 20, 2026 00:18

sapphirew marked this pull request as ready for review May 20, 2026 00:18

ack-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 20, 2026

ack-prow Bot requested a review from knottnt May 20, 2026 00:18

sapphirew changed the title ~~feat(runtime): Add unified --enable-cross-namespace flag~~ feat(runtime): Add --enable-cross-namespace flag May 20, 2026

cheeseandcereal reviewed May 20, 2026

View reviewed changes

kprahulraj mentioned this pull request May 21, 2026

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

Open

kprahulraj mentioned this pull request May 22, 2026

feat(runtime): Add --enable-cross-namespace flag #248

Open

Conversation

sapphirew commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ack-prow Bot commented May 7, 2026

Uh oh!

sapphirew commented May 20, 2026

Uh oh!

cheeseandcereal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kprahulraj May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kprahulraj commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sapphirew commented May 21, 2026

Uh oh!

kprahulraj commented May 21, 2026

Uh oh!

kprahulraj commented May 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sapphirew commented May 7, 2026 •

edited

Loading

kprahulraj May 22, 2026 •

edited

Loading

kprahulraj commented May 20, 2026 •

edited

Loading