fix: correct two custom_sync behaviors found while adopting it - #734
Open
gustavodiaz7722 wants to merge 2 commits into
Open
fix: correct two custom_sync behaviors found while adopting it#734gustavodiaz7722 wants to merge 2 commits into
gustavodiaz7722 wants to merge 2 commits into
Conversation
The check rejecting `custom_sync` on a `compare.is_ignored` field claimed the field would never appear in the delta, so the sync would never run and the generated DifferentExcept short-circuit would fire on every reconcile. That is not true. `compare.is_ignored` suppresses only the GENERATED comparison. A resource with a `delta_pre_compare` hook adds the same path by hand, and that is precisely how the out-of-band field pattern `custom_sync` generalizes is written in the controllers that have it today - eventbridge Rule ignores both Spec.Tags and Spec.Targets and adds each in customPreCompare. So the check fired on the feature's most likely consumers, and the only way to adopt `custom_sync` was to drop `is_ignored` and rework the resource's comparison, which is unrelated to the field being synced out of band. Remove it rather than narrow it. Whether a path reaches the delta is a property of hand-written code the generator cannot see, so every variant of this check is a guess: "ignored AND no delta_pre_compare hook" would still be one, since a hook's existence does not prove it adds THIS path. The runtime symptom of getting it wrong is local and visible - the field stops being applied - which is the same failure mode as an unimplemented sync method, and that is not pre-validated either. The three remaining validations are unaffected: nested field, is_read_only, and no Update operation all stay rejected, since each is decidable from the config alone. The fixture that asserted the rejection now asserts the config is honored, so the field is still collected and still yields sync<Field>.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gustavodiaz7722 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 |
The post-create marker assumes a `custom_sync` field is applied only in the
update path, so after a successful create it sets Synced=false to make the
runtime requeue and apply the field.
That is wrong for a field the Create input shape carries, and it is wrong
for both known consumers of the feature. `newCreateRequestPayload` writes
Tags into the request for eventbridge Rule (PutRule) and for autoscaling
AutoScalingGroup (CreateAutoScalingGroup) alike, so tags are live the
moment create returns. The marker costs a needless reconcile and, worse,
reports the resource as not synced while it is - for up to
requeue.DefaultRequeueAfterDuration.
Tags still need `custom_sync`, because tag UPDATES go through
TagResource/UntagResource rather than the Update operation. So the two
facts have to be expressible at once:
Tags:
custom_sync:
applied_on_create: true
The option governs the post-create marker ONLY. The field keeps its place
in the update path: sdkUpdate still calls its sync function when the delta
reports it, and still counts it in the DifferentExcept short-circuit. A
resource whose every custom_sync field is applied_on_create emits no marker
at all.
Stated by the author rather than derived from the Create input shape. The
generator can see that a field was written into the request struct but not
whether AWS acted on it, so an API that accepts a field on create and
ignores it would silently lose its marker and report Synced on a field that
was never applied. ACK already treats this class of claim as an assertion:
is_immutable, late_initialize and tags.ignore are none of them verifiable
either.
A bool rather than an operation selector, because the axis that varies is
only whether create leaves the field pending - the sync function is always
called from update. CustomSyncConfig stays a struct, so an `operations:`
key remains addable later without breaking the files adopting this now.
Rejected at generation time: `applied_on_create` on a resource with no
Create operation, which would drop the marker on the claim of an operation
that does not exist.
gustavodiaz7722
changed the title
Member
Author
|
/retest |
|
@gustavodiaz7722: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two corrections to
custom_sync(#732), found while adopting it in eventbridge-controller.1. Allow
custom_syncwithcompare.is_ignored#732 rejected the combination:
The premise is wrong.
compare.is_ignoredsuppresses only the generated comparison. A resource with adelta_pre_comparehook adds the same path by hand, and that is exactly how the out-of-band field patterncustom_syncgeneralizes is written in the controllers that have it today. eventbridgeRuleignores bothSpec.TagsandSpec.Targetsand adds each incustomPreCompare:Both paths are in the delta, so
DifferentAtfires andDifferentExceptbehaves. The check fired on the feature's most likely consumers, and the only way to adoptcustom_syncwas to dropis_ignoredand rework the resource's whole comparison — which has nothing to do with the field being synced out of band. For a set-like list of structs that also means hand-writing an ordering normalization, since the generated comparison for that shape is a whole-sliceDeepEqual.Removed rather than narrowed. Whether a path reaches the delta is a property of hand-written code the generator cannot see, so every variant of the check is a guess: "ignored and no
delta_pre_comparehook" would still be one, because a hook's existence does not prove it adds this path. The failure mode it guarded is local and visible — the field stops being applied — which is the same failure mode as a missingsync<Field>method, and that is not pre-validated either.2. Add
custom_sync.applied_on_createThe post-create marker assumes a
custom_syncfield is applied only in the update path, so it setsSynced=falseafter create to force a requeue.That is wrong for a field the Create input shape carries — and it is wrong for both known consumers.
newCreateRequestPayloadwrites Tags into the request for eventbridgeRule(PutRule) and autoscalingAutoScalingGroup(CreateAutoScalingGroup) alike:So tags are live the moment create returns. The marker costs a needless reconcile and reports the resource as not synced while it is, for up to
requeue.DefaultRequeueAfterDuration.Tags still need
custom_sync, because tag updates go throughTagResource/UntagResourcerather than the Update operation. Both facts now coexist:The option governs the post-create marker only. The field keeps its place in the update path —
sdkUpdatestill calls its sync function when the delta reports it, and still counts it in theDifferentExceptshort-circuit. A resource whose everycustom_syncfield isapplied_on_createemits no marker at all.Why a bool, not an operation selector
The axis that varies is only whether create leaves the field pending; the sync function is always called from update. An
operations: [Create, Update]shape would imply you can request a create-path sync the generator does not emit, and would inheritset.method's unvalidated-free-string behavior (method: Readmatches nothing, silently).CustomSyncConfigstays a struct, so anoperations:key remains addable later without breaking the files adopting this now — which matters, because eventbridgeTargetsneeds delete-path teardown and that is the case that would genuinely justify a selector.Why author-stated rather than derived
The generator can see whether
SetSDKputs a field in the create input, so it could derive this with no config at all. It should not: the marker is a claim about AWS runtime behavior ("this field isn't live yet") while the generator only knows wire shape ("the field was in the request struct"). Those diverge exactly in the interesting case — an API that accepts a field on create and ignores it — and there derivation silently drops a marker that was needed. ACK already treats this class of claim as an assertion;is_immutable,late_initializeandtags.ignoreare none of them verifiable either.Naming:
applied_on_createstates the API fact.skip_create_markerwould name the emitted code, and go stale if the marker's implementation changes.Validation
custom_syncon a nested fieldcustom_syncwithis_read_onlycustom_syncwith no Update operationcustom_syncwithcompare.is_ignoredapplied_on_createwith no Create operationTesting
make testpasses — 15 packages, no failures.TestCustomSyncInvalid_CompareIgnoredbecomesTestCustomSync_CompareIgnored, asserting the config is accepted and honored (field still collected, still yieldssyncTags). Keeping the fixture and inverting the assertion pins the new behavior instead of deleting the coverage.TestCustomSyncCreate_AppliedOnCreateasserts the marker narrows to the still-pending field, andTestCustomSyncUpdate_AppliedOnCreateasserts the update path is untouched — that second one is the regression that would actually hurt, since a tags-only change falling through to the Update operation cannot be applied by it.custom_sync: {}stays pending after create).generator-with-custom-sync-applied-on-create.yaml.Verified end to end by regenerating eventbridge-controller with
Tags: custom_sync: {applied_on_create: true}andTargets: custom_sync: {}. The marker narrows to the one field create does not apply, while both fields keep their update-path sync:By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.