feat(runtime): add ignore-field-drift annotation for selective field reconciliation#256
feat(runtime): add ignore-field-drift annotation for selective field reconciliation#256sapphirew wants to merge 9 commits into
Conversation
|
Companion code-generator PR: aws-controllers-k8s/code-generator#714 — it adds the call to |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sapphirew The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
4864f40 to
d494844
Compare
sapphirew
changed the title
|
Updated: the annotation is now |
d494844 to
bffae2b
Compare
Adds an opt-in, runtime-level mechanism for telling ACK to stop reconciling drift on named fields of a custom resource, gated behind the SelectiveReconciliation feature gate (Alpha, disabled by default). A field listed in the services.k8s.aws/ignore-field-drift annotation is still created from Spec and late-initialized normally, but ACK no longer reconciles drift on it: the field is excluded from the reconcile delta, so an out-of-band change on the AWS side does not trigger an Update and is not reset to Spec. The user's declared value is retained in the CR Spec (never overwritten with the observed AWS value), so removing the annotation cleanly resumes full reconciliation. - New annotation services.k8s.aws/ignore-field-drift (comma-separated JSON-style field paths). - applyIgnoredFields merges the observed value into a deep copy of desired before the delta (suppression + anti-clobber); restoreIgnoredFields puts the declared value back before the spec write-back (retain). - FilterIgnoredDeltaDifferences removes ignored-path differences from a computed delta, for the requeue/IsSynced paths (called from generated per-resource delta code; see companion code-generator change). - Process-wide SetGlobalFeatureGates/GetGlobalFeatureGates (set once at startup) so generated package-level delta code can consult the gate. - Drift on ignored fields is surfaced via a log line only. Refs aws-controllers-k8s/community#2367
53b4b6c to
aac0dc8
Compare
The log-only drift detection in logIgnoredFieldDrift compared each ignored field byte-for-byte. For fields the code-generator compares semantically -- is_iam_policy (IAMPolicyDocumentEqual) and is_document (DocumentEqual) -- AWS canonicalizes the value on read (re-orders keys, collapses single-element arrays, URL-encodes, reindents), so the stored declared string is never byte-equal to the value read back. This produced a spurious "skipping drifted ignore-field-drift fields" log line on every reconcile for any ignored policy/document field, even when it never actually drifted. Replace the byte comparison with driftedIgnoredPaths, which runs the generated per-resource Delta against a copy of desired with the annotation removed (so the delta's own FilterIgnoredDeltaDifferences is a no-op and ignored-path diffs stay visible). This inherits each field's real comparator, so the log fires only on genuine semantic drift. The stored CR is never mutated. Verified end-to-end against the IAM Role AssumeRolePolicyDocument field: no spurious log across 10 steady-state reconciles; the log now appears only on a real out-of-band policy change.
aac0dc8 to
6b72758
Compare
The reconciler-level ignore-field-drift tests were named by their position in the design-doc behavior matrix (Row1..Row6), which is opaque out of context. Rename them after the behavior they verify (e.g. Ignored_UpdateAntiClobberAndRetain, Unannotated_DriftReconciled) and add a single table comment mapping each (annotation, Spec, late-init) combination to its test. No test logic changes.
Add a syntactic well-formedness check for the paths in the services.k8s.aws/ignore-field-drift annotation. A path with illegal characters, empty segments, an array index, or a segment not starting with a letter (e.g. "spec/tags", "spec..tags", "spec.tags[0]") is a silent no-op at every consumer -- it matches no field in the merge, filter, or drift log -- so the field the user meant to ignore keeps being reconciled with no signal. warnMalformedIgnorePaths now emits a log line naming such paths on reconcile. This is a SYNTACTIC check only (regex over each dotted segment); it does not verify that a well-formed path resolves to a real field on the resource, which would need the CRD schema the runtime does not have (tracked as the fuller validation follow-up). No behavior change to suppression/merge; malformed paths were already harmless no-ops. Adds isValidFieldPath / malformedIgnorePaths unit tests.
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
…rift Add two reconciler-level tests that model a custom sdkUpdate which builds its own SDK request (the pattern every audited controller uses), to verify the 'custom update could clobber' concern is a non-issue: - CustomUpdateBuildsRequestFromDesired: a custom update that sources the ignored field from the passed-in desired sends the OBSERVED value (a no-op), never the declared value; the non-ignored field is reconciled normally. - CustomUpdateFromLatestIsAlsoSafe: even sourcing from latest is safe, because the merge makes desired == latest for an ignored field. Pins the boundary that the only clobbering value is one that is neither declared nor observed. Full pkg/runtime suite green.
|
/retest |
knottnt
left a comment
knottnt
left a comment
There was a problem hiding this comment.
@sapphirew left a few initial comments.
| // reconciliation via the services.k8s.aws/ignore-field-drift annotation. It | ||
| // alters reconciliation behavior, so it is disabled by default and must be | ||
| // explicitly enabled by the controller operator. | ||
| SelectiveReconciliation = "SelectiveReconciliation" |
There was a problem hiding this comment.
nit: From the name alone it isn't immediately clear that this is related to the the ignored field annotations.
There was a problem hiding this comment.
Renamed to IgnoreFieldDrift in d3456f7 to match the annotation. The gate never shipped, so no compatibility impact.
| // per-resource newResourceDelta function). It defaults to the ACK default | ||
| // feature gates so that code reading it before initialization sees sane, | ||
| // disabled-by-default values. |
There was a problem hiding this comment.
Q: When the feature flag is set to a non-default value is there a race condition here where some controller code would still see the default value?
There was a problem hiding this comment.
See comment on the code-generator PR. I'm wondering if we can drop this entirely by bringing the FilterIgnoredDeltaDifferences into the runtime framework.
There was a problem hiding this comment.
Gone with your wrapper suggestion: the filter now reads cfg.FeatureGates and the global accessors are deleted in 8e9e706.
There was a problem hiding this comment.
Done in 8e9e706. Verified nothing bypasses it: the generated delta's only caller is the descriptor's Delta, invoked only by the runtime; no hooks or IsSynced use it. New filteredDelta wraps r.rd.Delta at the reconciler's decision points. Closed aws-controllers-k8s/code-generator#714 as no longer needed.
| // Selective reconciliation: rm.IsSynced (generated per-resource code) | ||
| // derives the in-sync determination from the per-resource | ||
| // newResourceDelta. That generated delta now calls |
There was a problem hiding this comment.
Is this the case? I believe the latest resource is what we use for the IsSynced call. I would expect Delta filtering to impact this.
There was a problem hiding this comment.
You were right, the comment was wrong. Fixed in 8e9e706. Generated IsSynced never uses the delta, it returns true or checks synced.when conditions. Ignored drift stays out of ResourceSynced upstream: the filtered delta means rm.Update is never called.
| // Warn about any ignore-field-drift paths that are not well-formed field | ||
| // paths (typos / illegal characters); they silently have no effect. |
There was a problem hiding this comment.
Should we apply a terminal error here? If there's an invalid field path and we continue the reconcile loop we could end up mutating a field that the user intended for us to ignore. In that case failing fast and throwing an error seems safer.
There was a problem hiding this comment.
We chose warn only for the MVP. A terminal error stops managing the whole resource over one typo, and recovery needs a spec edit since annotation changes alone do not trigger a reconcile. A syntax check also is not the real safety line: a well formed typo like spec.tagz passes it. The real fix is schema validation at admission time, per your other comment.
There was a problem hiding this comment.
IMO it would be good to fail fast here instead of preceding. Agree that the current check doesn't cover every malformed input and validating that the path matches a valid field in the resource schema would be a good add. However, we do know that if this check fails something is wrong with the user input and we may be mutating something the user doesn't want us to.
As for the reconcile not being triggered by an annotation change. I believe we do have the option of adding a custom predicate here that could allow us to trigger on more than just spec/generation changes.
There was a problem hiding this comment.
Agreed — changed to fail fast with a TerminalError in f4841cb. The reconciler now checks malformedIgnorePaths before any merge/delta logic and returns immediately if the annotation is syntactically invalid. This puts the resource into an error condition, making it visible to the user that their annotation needs fixing.
Also added AnnotationChangedPredicate to the controller builder (via predicate.Or(GenerationChangedPredicate{}, AnnotationChangedPredicate{})) as you suggested, so annotation changes now trigger an immediate reconcile rather than waiting for the next resync or spec change. This means the terminal error surfaces right away when a user sets a bad annotation.
Added TestIgnoreFieldDrift_MalformedPath_FailsFast to verify the behavior end-to-end (terminal error returned, Update never called).
| // characters (e.g. "spec/tags", "spec.tags!"), empty segments (e.g. "spec..tags", | ||
| // ".spec", "spec."), array indices (e.g. "spec.tags[0]" -- sub-element ignore is | ||
| // out of v1 scope), and the empty string. | ||
| func isValidFieldPath(p string) bool { |
There was a problem hiding this comment.
Would we be able to validate this against the actual fields available in the Resource Kind's type?
There was a problem hiding this comment.
Yes, deferred as a follow up. The runtime lacks the CRD schema at reconcile time, so this belongs in an admission webhook or a codegen emitted allowlist. Tracked in the PR description.
…p global feature gates Address review feedback: move FilterIgnoredDeltaDifferences out of the generated per-resource delta and into a runtime-side wrapper (resourceReconciler.filteredDelta) around r.rd.Delta. Every production consumer of the generated delta routes through the descriptor's Delta method, which only the runtime invokes, so wrapping the call sites in the reconciler covers all delta-driven decisions (update, read-only skip, requeue/patch gate) without any generated-code change. Consequences: - Delete Set/GetGlobalFeatureGates: the filter now reads the controller's own cfg.FeatureGates, so no process-global state (and no data-race question) is needed. - Unexport the filter as filterIgnoredDeltaDifferences. - driftedIgnoredPaths no longer needs the strip-the-annotation-on-a- copy trick: the raw r.rd.Delta is unfiltered by construction. - Fix an incorrect comment at the IsSynced call site: generated IsSynced checks synced.when status conditions only and never consults the resource delta.
…oreFieldDrift The gate now matches the services.k8s.aws/ignore-field-drift annotation it controls, per review feedback. The gate has never shipped in a release, so there is no compatibility impact. Also renames selective_reconciliation.go -> ignore_field_drift.go (and the test file), HasSelectiveReconciliation -> HasIgnoreFieldDrift, and the 'selective reconciliation:' log prefixes to 'ignore-field-drift:'.
sapphirew
changed the title
| // distinguishes "late-init wins" (persist, matrix Row 6) from drift | ||
| // suppression on a non-late-init ignored field (do NOT persist, Row 5); AND |
There was a problem hiding this comment.
nit: Row 5/6 doesn't refer to anything in the codebase and can be removed.
There was a problem hiding this comment.
Done — removed the Row 5/6 references in f4841cb.
There was a problem hiding this comment.
Would we be able to add test cases for nest fields like spec.a.b.c ? It looks like most of the tests cover only top level fields.
There was a problem hiding this comment.
Done — added four tests exercising deeply nested paths (spec.a.b.c style) in f4841cb:
TestApplyIgnoredFields_NestedPath— mergesspec.network.vpc.cidrfrom latest, preserves sibling fields at same depthTestApplyIgnoredFields_NestedPathAbsentInLatest— handles nested path absent in latestTestRestoreIgnoredFields_NestedPath— restores declared value for a nested path after updateTestFilterIgnoredDeltaDifferences_NestedPaths— strips a nested ignored delta entry while preserving siblings at the same nesting depth
| // Note: rm.IsSynced (generated per-resource code) checks | ||
| // `synced.when` status conditions only and never consults the | ||
| // resource delta, so ignore-field-drift needs no suppression here. | ||
| // Drift on ignored fields is kept out of the synced determination | ||
| // upstream, where the reconciler's delta-driven decisions | ||
| // (update/requeue) go through filteredDelta. |
There was a problem hiding this comment.
nit: we can likely drop this comment since it isn't covering new behavior. Generally, synced behavior is based on status and not user set spec as well.
| // Warn about any ignore-field-drift paths that are not well-formed field | ||
| // paths (typos / illegal characters); they silently have no effect. |
There was a problem hiding this comment.
IMO it would be good to fail fast here instead of preceding. Agree that the current check doesn't cover every malformed input and validating that the path matches a valid field in the resource schema would be a good add. However, we do know that if this check fails something is wrong with the user input and we may be mutating something the user doesn't want us to.
As for the reconcile not being triggered by an annotation change. I believe we do have the option of adding a custom predicate here that could allow us to trigger on more than just spec/generation changes.
…notation predicate Address review feedback: - Return a TerminalError when the ignore-field-drift annotation contains malformed paths instead of warning and continuing. This prevents the reconciler from proceeding when user intent is unclear. - Add AnnotationChangedPredicate so the controller triggers a reconcile immediately when annotations change (not just on spec/generation changes). - Remove "Row 5/6" references from comments (they referenced nothing in the codebase). - Drop the IsSynced comment that restated existing behavior. - Add unit tests for nested field paths (spec.a.b.c) covering apply, restore, and delta filter. - Add a reconciler-level test verifying the malformed-path terminal error.
3 participants
Description
Adds an opt-in, runtime-level mechanism that lets a user tell ACK to stop reconciling drift on specific fields of a custom resource, via a new annotation. This is useful when a field is legitimately managed outside of ACK — for example, organization tooling that applies dynamic tags to a resource that ACK would otherwise try to remove on every reconcile.
The behavior is gated behind a new
IgnoreFieldDriftfeature gate (Alpha, disabled by default), so there is no change to existing controllers unless an operator explicitly enables it.Behavior
A field listed in the
services.k8s.aws/ignore-field-driftannotation is still created fromSpecand late-initialized normally — ACK simply stops reconciling drift on it:Specas usual (this establishes the baseline).Spec.Spec; ACK never overwrites it with the observed AWS value. Removing the annotation therefore cleanly resumes full reconciliation against the declared value — it is non-destructive.ACK.ResourceSyncedstaysTrue(the user opted out of managing those fields).The annotation value is a comma-separated list of dotted, JSON-style field paths.
Implementation
AnnotationIgnoreFieldDrift(services.k8s.aws/ignore-field-drift).IgnoreFieldDriftfeature gate (Alpha, disabled by default).resourceReconciler.filteredDelta(a, b)wrapsr.rd.Deltaand strips ignored-path differences (filterIgnoredDeltaDifferences) before the reconciler consumes the result. This is sufficient because the generated per-resourcenewResourceDeltahas exactly one production caller fleet-wide — the generated descriptor'sDeltamethod, which only the runtime invokes (no controller hooks or customsdkUpdatecode call it directly, and no generatedIsSyncedconsults a delta — it checkssynced.whenstatus conditions only). Wrapping the reconciler's delta call sites (the update decision, the read-only skip, and the requeue/patch gate) therefore covers every delta-driven decision without any generated-code change and without process-global feature gates — the filter reads the controller's owncfg.FeatureGates.applyIgnoredFieldsmerges the observed value into a deep copy ofdesiredbefore the delta is computed (suppresses drift; avoids clobbering the external value when an unrelated field triggers an Update).restoreIgnoredFieldsputs the user's declared value back before the spec write-back (retain).filterIgnoredDeltaDifferencesremoves ignored-path differences from a computed delta (applied via thefilteredDeltawrapper above).applyIgnoredFieldsputs the AWS-observed value into thedesiredthe update code is handed, so re-sending it is a no-op; (2) the filtered delta makes an ignored-only drift an empty delta →rm.Updateis never called, and anydelta.DifferentAt-gated send is skipped for ignored fields; (3)restoreIgnoredFieldsresets ignored paths to the declared value before the spec write-back. A real clobber would require a custom update that both sends an ignored field ungated and sources the value from something other than the mergeddesired(a freshReadOne,latest, or the AWS response). A trace of the 64 controllers with custom update logic (incl.opensearchservice/Domain,memorydb/Cluster+ACL,rds,lambda,documentdb,eks,sagemaker, …) found no controller that does both — every generatednewUpdateRequestPayloadbuilds from the mergeddesired, and theresp → ko.Speccopies some controllers do are output-mapping thatrestoreIgnoredFieldscleans up, not AWS writes.Delta(the rawr.rd.Delta, which is unfiltered by construction since the filter lives in the runtime wrapper). This means fields the code-generator compares semantically —is_iam_policy(IAMPolicyDocumentEqual) andis_document(DocumentEqual) — are judged with their real comparator. AWS canonicalizes such values on read (re-orders keys, collapses single-element arrays, URL-encodes, reindents), so a naive byte compare would report perpetual, spurious drift on every reconcile for an ignored policy/document field; delegating to the generated comparator makes the log fire only on genuine drift.warnMalformedIgnorePathsruns a syntactic well-formedness check (a regex over each dotted segment) and logs any path with illegal characters, empty segments, an array index, or a segment not starting with a letter (e.g.spec/tags,spec..tags,spec.tags[0]). Such a path is otherwise a silent no-op, so this warns the user that the field they meant to ignore will keep being reconciled. This does not verify a well-formed path resolves to a real field on the resource (that needs the CRD schema — see follow-ups).Testing
is_iam_policyfields, malformed-path detection, and a reconciler-level behavior suite covering create / drift-suppression / anti-clobber / retain / late-initialization / annotation-removal across the annotated and unannotated cases.ignore-field-drift: "spec.tags"— an externally-added tag survives reconciliation, the resource staysSynced, and the declared value is retained.ignore-field-drift: "spec.assumeRolePolicyDocument"(theis_iam_policyfield) — an out-of-band trust-policy change is not reverted while ignored; no spurious drift log across steady-state reconciles; removing the annotation reconciles the policy back to the declared value.No code-generator change is required: the delta filtering is applied entirely by the runtime wrapper around the generated
Delta.Known risks / limitations in this MVP
Scope / follow-ups (not in this PR)
This is the MVP for the first iteration. Deliberately deferred:
spec.tagzfor a resource whose field isspec.tags) — that resolves to no field and is still a silent no-op. Validating each path against the CRD schema (and warning on an unknown-but-well-formed path) is deferred; the runtime does not carry the OpenAPI schema at reconcile time, so it is best done in an admission webhook or via agenerator.yamlallowlist.additive-members) and sub-field ignore inside list entries.sdkUpdatebuilds its request from the mergeddesired. That is not enforced, though — a future custom update that sends an ignored field ungated from a value that is neither the passed-indesirednor the observed state (e.g. a synthesized/stale value from a freshReadOneor the API response) would clobber. A code-generator lint could flag that pattern so the anti-clobber contract can't silently regress; an optional stronger form re-applies the observed value onto the built request payload before the SDK call.IgnoreFieldDriftfrom Alpha to Beta after soak.Refs aws-controllers-k8s/community#2367