feat: Generate cross-namespace reference validation and --enable-cross-namespace flag#699
Conversation
6f2fb42 to
0511503
Compare
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
|
Doc changes is added - aws-controllers-k8s/docs#37 |
| 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) |
There was a problem hiding this comment.
Q: does the function comment need to be updated to reflect the new output?
There was a problem hiding this comment.
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).
| {{ "{{- end }}" }} | ||
| - {{ "--enable-carm={{ .Values.enableCARM }}" }} | ||
| - {{ "--enable-cross-namespace={{ .Values.enableCrossNamespace }}" }} | ||
| - {{ "--enable-cross-namespace-references={{ .Values.enableCrossNamespaceReferences }}" }} |
There was a problem hiding this comment.
I don't see this flag in the associated runtime changes.
There was a problem hiding this comment.
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.
| # When false (default), the controller rejects any reference whose | ||
| # Namespace field differs from the namespace of the resource containing | ||
| # the reference. | ||
| enableCrossNamespaceReferences: false |
There was a problem hiding this comment.
I don't see this flag in the associated runtime changes or in the values.schema.json.tpl.
There was a problem hiding this comment.
Same issue — removed the stale enableCrossNamespaceReferences entry. Only enableCrossNamespace: true remains, which matches the runtime flag and is already in values.schema.json.tpl.
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)
0511503 to
30bdd70
Compare
| // | ||
| // 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) | ||
| // } | ||
| // } | ||
| // } | ||
| // } | ||
| // } | ||
| // } |
There was a problem hiding this comment.
Good catch — restored. The new sample shows the simpler generated output that uses ackrt.HandleCrossNamespaceReference from runtime#249.
| 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) |
There was a problem hiding this comment.
can we make this block a helper function? maybe in runtime?
There was a problem hiding this comment.
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.
|
|
||
| // 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 | ||
| } |
There was a problem hiding this comment.
nit: this could also be a runtime function, similar to https://github.com/aws-controllers-k8s/runtime/blob/main/pkg/runtime/reconciler.go#L555
There was a problem hiding this comment.
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.
| {{ if .CRD.HasReferenceFields -}} | ||
| {{ end -}} | ||
|
|
There was a problem hiding this comment.
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).
| // 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, | ||
| // ) | ||
| // } |
There was a problem hiding this comment.
i was hoping this entire section would be inside the helper function..and we won't need to return isCrossNs
There was a problem hiding this comment.
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).
Issue #, if available: Description of changes: Extract the lookup-or-create condition pattern and the warning-log + condition combo into shared helpers so generated code can call a single runtime function instead of inlining ~14 lines per cross-namespace check. - Add SetCrossNamespaceOptInRequired(conditions, message): updates or appends the ACK.CrossNamespaceOptInRequired condition using a lookup-or-create pattern (avoids duplicate conditions on repeated reconciles) - Add HandleCrossNamespaceReference(ctx, conditions, refKind, ownerNs, targetNs, refName): emits the structured warning log and sets the condition; intended to be called from generated code after ValidateCrossNamespaceReference reports isCrossNamespace=true - Refactor fieldExportReconciler.setCrossNsOptInRequiredCondition to use SetCrossNamespaceOptInRequired so all condition handling shares one implementation Addresses review feedback on aws-controllers-k8s/code-generator#699. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Point at runtime pseudo-version v0.59.2-0.20260603224255-14be98880374 (commit 14be988, PR aws-controllers-k8s#249) which adds the ResolveCrossNamespaceReference and ResolveCrossNamespaceReferenceString helpers that generated code now calls. This is a temporary pin for testing; revert to the tagged release (v0.60.0) once it is published upstream.
|
/retest |
1 similar comment
|
/retest |
…espace notice Add a temporary replace directive pointing at sapphirew/ack-runtime@4ee0bc5 (runtime PR aws-controllers-k8s#254) which introduces the SecretValueFromReference signature change and ACK.Advisory cross-namespace condition. This resolves the dependency skew that was failing code-generator tests. Drop this replace and bump the require line once aws-controllers-k8s#254 merges and a tagged runtime release is published.
|
/retest |
1 similar comment
|
/retest |
|
/hold |
| sigs.k8s.io/structured-merge-diff/v6 v6.3.0 // indirect | ||
| ) | ||
|
|
||
| replace github.com/aws-controllers-k8s/runtime => github.com/sapphirew/ack-runtime v0.0.0-20260608212517-9bb3e738fe97 |
There was a problem hiding this comment.
This will need to be removed before merging.
|
/retest |
2 similar comments
|
/retest |
|
/retest |
…isory condition (#254) Issue #, if available: Description of changes: Surface the Phase 1 cross-namespace deprecation **notice** consistently across all three cross-namespace categories (resource references, field exports, and secret references), and move cross-namespace **validation** for secret references into the runtime so every caller is covered. ### Notice surface: `ACK.Advisory` condition The deprecation notice is set on the **existing `ACK.Advisory`** condition, carrying the stable reason `CrossNamespaceOptInRequired` (constant `CrossNamespaceOptInRequiredReason`). It is **not** a dedicated condition type. Rationale: advisory conditions carry no programmatic dependents, which sidesteps the Kubernetes API convention that *removing or repurposing a condition type is a breaking change*. This keeps the temporary notice cheap to add now and cheap to stop setting once the rollout completes, while staying greppable for support. An earlier revision of this PR introduced a dedicated `ACK.CrossNamespaceOptInRequired` type; that type has been removed in favor of `ACK.Advisory`. - New `condition.SetAdvisory` / `condition.AdvisoryWithReason` helpers — reason-keyed lookup-or-create so distinct advisories on the same resource coexist without clobbering each other. ### Secret references: validation in `SecretValueFromReference` Cross-namespace validation for secret references lives in `SecretValueFromReference` so every caller is covered, including hand-written custom update functions (e.g. RDS `db_cluster/custom_update.go`) and hooks that call it directly rather than going through generated `sdk.go`. - When `--enable-cross-namespace` is false and the secret targets a different namespace, return a terminal error (wrapping the `ResourceReferenceCrossNamespaceNotAllowed` sentinel). The caller's existing condition machinery sets `ACK.Terminal` from the message. - When the flag is true, log a deprecation warning, set the `ACK.Advisory` notice, and resolve the secret from the target namespace (Phase 1 behavior). - Same-namespace and empty-namespace refs are unaffected. ### Mechanism: context-stash (no signature change) `SecretValueFromReference` keeps its existing signature `(ctx, ref)`. To reach the owning resource (so the advisory can be set on it) without a breaking interface change, the reconciler stashes the resource in the context: - `Reconcile()` stashes the resource via `WithConditionManager(ctx, desired)`. - `SecretValueFromReference` retrieves it via `ConditionManagerFromContext(ctx)`. > An earlier revision instead added a `ConditionManager` argument to `SecretValueFromReference` (a cleaner signature), but that is a **breaking change** for the hand-written controller callers above and would force every such controller to be updated in lockstep with the runtime release. The context-stash keeps the change additive and non-breaking; mocks are regenerated for the unchanged 2-arg signature. This lets the code-generator drop the per-call secret validation block it previously emitted in `set_sdk.go` (which only covered generated call sites). Companion change: aws-controllers-k8s/code-generator#699. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
The runtime change reverted the SecretValueFromReference signature back to (ctx, ref) and restored the context-stash mechanism, so a breaking signature change is no longer forced on hand-written controller callers (e.g. rds-controller db_cluster/custom_update.go). Accordingly, revert the generated-code change: setSDKForSecret again emits rm.rr.SecretValueFromReference(ctx, <source>) with no resource-var threading, and the setSDKForContainer/Struct/Slice/Map/Union helpers drop the resourceVarName parameter. Re-pin the runtime replace directive to the amended fork commit (9bb3e73).
e0bcddb to
328841e
Compare
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
1 similar comment
|
/retest |
|
/hold cancel |
|
/retest |
|
/lgtm |
|
@sapphirew: you cannot LGTM your own PR. DetailsIn response to this:
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-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: knottnt, michaelhtm, sapphirew The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issue #, if available:
Description of changes:
Generate cross-namespace reference validation into controller code and expose the
--enable-cross-namespaceflag, aligned with the runtime changes in aws-controllers-k8s/runtime#254.Resource references
Replace the legacy unconditional namespace-override block in
ResolveReferencesForFieldwith a call to the shared runtime helperackrt.ResolveCrossNamespaceReference. This centralizes cross-namespace reference validation: when--enable-cross-namespaceis disabled, references targeting a different namespace are rejected with a terminal error; when enabled (Phase 1), the runtime sets theACK.Advisorydeprecation notice. Generated code passes&ko.Status.Conditionsto the helper; it does not manage the notice itself.Secret references
Cross-namespace validation for secrets now lives entirely in the runtime's
SecretValueFromReference(runtime#254), so the generator emits the unchanged call:No per-call validation block and no resource-handle argument are generated. The runtime reaches the owning resource via a context-stash (set in
Reconcile()), which keepsSecretValueFromReferencesignature-compatible and avoids breaking hand-written callers (e.g. RDSdb_cluster/custom_update.go).Helm chart
Expose the
--enable-cross-namespaceflag (defaulttruefor Phase 1 warning behavior), covering resource references, secret references, and field exports.Changes
resource_reference.go: emitResolveCrossNamespaceReferencecall withEnableCrossNamespaceand conditions pointerset_sdk.go: secret fields emit the unchangedSecretValueFromReference(ctx, source)(validation handled in runtime)*_test.go: expected output strings updated to match the helper/call shapesdeployment.yaml.tpl: add--enable-cross-namespaceflag to container argsvalues.yaml.tpl: addenableCrossNamespace: truewith descriptionvalues.schema.json.tpl: addenableCrossNamespaceschema entrygo.mod: temporaryreplacepinning the runtime to the runtime#254 branch commit until it merges and a release is taggedDependency / merge order
This PR depends on runtime#254 (the
ACK.Advisorynotice + secret validation). Thereplacedirective must be dropped and therequireline bumped to the tagged runtime release once #254 merges.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.