test(vpc): add e2e coverage for ignore-field-drift - #361
Conversation
|
Heads up: I pushed a temporary commit ( Why: the Merge plan: runtime #256 merges & releases first → ec2-controller bumps to a runtime version containing the gate → I revert |
|
/hold |
ack-prow
Bot
added
the
do-not-merge/hold
41ed2d8 to
14e9fde
Compare
sapphirew
changed the title
aa14cd9 to
378f688
Compare
|
/unhold |
ack-prow
Bot
removed
the
do-not-merge/hold
|
/retest |
2 similar comments
|
/retest |
|
/retest |
3214995 to
b2cb92a
Compare
|
/retest |
2 similar comments
|
/retest |
|
/retest |
| def test_nested_field_under_ignored_parent( | ||
| self, ec2_client, ignore_field_drift_enabled, ignore_field_drift_vpc, | ||
| ): | ||
| """Drift on a nested child of an ignored parent: the declared tag's | ||
| value (Delta path Spec.Tags.N.Value) changes externally while only the | ||
| parent spec.tags is ignored. The runtime must match this by path prefix, | ||
| so the child drift is ignored too.""" |
There was a problem hiding this comment.
Gen AI generated comment.
- Issue: The test claims to exercise the runtime's path-prefix match with a Delta path of
Spec.Tags.N.Valueunder an ignoredspec.tags. No such path exists.pkg/resource/vpc/delta.gocompares tags as a whole viaconvertToOrderedACKTags+MapStringStringEqualand emits exactly one difference,delta.Add("Spec.Tags", ...). SoPath.ContainsFold("spec.tags")matches by exact segment equality here — the same branchtest_tags_drift_ignoredalready covers. Two independent confirmations:runtime/pkg/compare/path_test.goassertsPath("Spec.KMSKeyID").ContainsFold("spec.kmsKeyID.extra")is False, i.e.Containsrequires the ignored path to be a prefix of the Delta path, so prefix matching needs a Delta path deeper than the annotation; andisValidFieldPathinignore_field_drift.gorejects indices outright, soSpec.Tags.N.Valueis not even expressible as an annotation. - Fix: Either (a) pick a field whose generated delta genuinely emits a nested path — a nested struct member where code-gen produces a
compareXhelper with aSpec.Parent.Childsubject — and annotate the parent; or (b) drop the prefix-match claim. Option (b) is fine: tag-value drift on a declared key is a materially different case from a new externally-added key (it is the one thattags.Syncwould actively revert), so keep the test but rename it totest_declared_tag_value_drift_ignoredand correct the docstring plus the PR description. Do not leave a test asserting coverage of a branch it never reaches.
There was a problem hiding this comment.
Confirmed, and taken option (b). pkg/resource/vpc/delta.go:143-146 emits a single delta.Add("Spec.Tags", ...) via convertToOrderedACKTags + MapStringStringEqual, and Path.Contains (runtime/pkg/compare/path.go:63-77) bails when the subject has more segments than the path, so spec.tags against Spec.Tags is segment equality — the branch test_tags_drift_ignored already reaches.
Renamed to test_declared_tag_value_drift_ignored in 13e3586. The docstring now states what it covers (a changed value on a declared key, which tags.Sync would actively revert, unlike an added key) and explicitly what it does not (prefix matching needs a Delta subject deeper than the annotation; per-element paths are not expressible since isValidFieldPath rejects indices). Corrected the class docstring, which repeated the same claim, and the PR description.
| # Editing the ignored scalar in the spec is retained but NOT pushed to | ||
| # AWS: patch to a new value and confirm the live attribute is unchanged. | ||
| k8s.patch_custom_resource(ref, {"spec": {"enableDNSSupport": True}}) | ||
| time.sleep(MODIFY_WAIT_AFTER_SECONDS) | ||
|
|
||
| assert _dns_support_enabled(ec2_client, vpc_id) is True | ||
| latest = k8s.get_resource(ref) | ||
| assert latest["spec"].get("enableDNSSupport") is True |
There was a problem hiding this comment.
Gen AI generated comment.
- Issue: The "edit retained but not pushed" assertions are vacuous. The external actor has already set the live attribute to
Trueviamodify_vpc_attribute; the test then patchesspec.enableDNSSupporttoTrueand asserts the live attribute isTrueand the spec isTrue. Both hold whether or not the edit was pushed — spec and AWS agree, so there is no delta to suppress and nothing being tested. Contrasttest_tags_drift_ignored, which patches toteam=changedwhile AWS holdspayments, and is therefore sound. - Fix: Reorder so the edit is made against a differing live value. Create with
enableDNSSupport: False(the controller applies it at create), patch the spec toTrue, assert the live attribute is stillFalseandspec.enableDNSSupport is True— that is the real retain/not-pushed assertion. Then do the external-flip half. Because the field is boolean there is no third value, so ordering is the only way to make both halves non-vacuous.
There was a problem hiding this comment.
Confirmed vacuous — the external flip sets the live value to True before the spec is patched to True, so the assertion holds either way.
One correction to the proposed fix: reordering within a single test does not resolve it, because the field is boolean. After create False → patch spec True, asserting the live value is still False is sound; but then flipping live to True makes spec and AWS agree, so the external-drift half asserts nothing. Whichever half runs second finds no delta to suppress, and there is no third value to escape with.
Split into two tests in 13e3586, each with its own VPC:
test_scalar_external_drift_ignored— createFalse, external flip toTrue, assert it survives and stays Synced.test_scalar_spec_edit_not_pushed— createFalse, patch spec toTrue, assert the live value is stillFalseand the spec retainsTrue.
The edit case also earns a separate test on its own merits: enableDNSSupport reaches AWS through ModifyVpcAttribute in the custom update path rather than tags.Sync, so suppression has to hold for that path too.
| try: | ||
| _, deleted = k8s.delete_custom_resource(ref, 3, 10) | ||
| assert deleted | ||
| except: | ||
| pass |
There was a problem hiding this comment.
Gen AI generated comment.
- Issue:
except: passaroundassert deletedsilently discards teardown failure, so a leaked VPC or CR passes unnoticed — which directly undercuts the PR description's claim that teardown was verified clean. The bare form also swallowsKeyboardInterruptandSystemExit. - Fix: Remove the try/except and let
assert deletedfail the teardown, or at minimum narrow toexcept Exception:, log, and re-raise. Separately, move thedelete_tagscleanup of the out-of-bandexternaltag out of the test body into the fixture teardown (or afinally), so a mid-test assertion failure does not leave it behind.
There was a problem hiding this comment.
Both points fixed in 13e3586. The try/except: pass is gone, so a failed delete fails the teardown, and the out-of-band external tag cleanup moved into a finally — as a trailing statement it was skipped whenever an assertion above it failed.
One addition: the delete budget goes from 30s to 60s in the same change. delete_custom_resource(ref, 3, 10) gives a VPC CR 30s to clear its finalizer, and ec2's own test_vpc.py::TestVpc::test_crud_tags failed on exactly assert deleted is True in a run on this PR earlier this week. Asserting on the old budget would have traded a silent leak for a flake, so the assertion now runs against a budget that reflects observed deletion times.
|
/retest |
|
/LGTM |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gustavodiaz7722, 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 |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
Adds an integration test exercising the services.k8s.aws/ignore-field-drift annotation (aws-controllers-k8s/runtime#256) against the EC2 VPC resource. The IgnoreFieldDrift feature gate is Alpha and disabled by default, so the test self-enables it on the deployed controller for the module and restores the prior FEATURE_GATES value afterwards. EC2 is one of the controllers the runtime presubmit regenerates and e2e-tests, so this coverage runs on runtime PRs. The test asserts, on a VPC annotated to ignore spec.tags: - the declared tag is applied at create; - an externally-added tag survives (controller does not call DeleteTags); - the resource stays Synced despite tag drift; - an edit to spec.tags is retained in the spec but not pushed to AWS.
Extends the ignore-field-drift e2e coverage beyond the list-of-objects case (spec.tags) to the two other field shapes the runtime treats differently: - test_scalar_field_drift_ignored: ignore a scalar leaf (spec.enableDNSSupport) whose Delta path matches the ignored path exactly. Asserts an external modify_vpc_attribute flip is not reverted and a spec edit is retained but not pushed. - test_nested_field_under_ignored_parent: drift on a nested child (a tag's value, Delta path Spec.Tags.N.Value) while only the parent spec.tags is ignored, exercising the runtime's path-prefix match rather than exact equality. The VPC fixture is now parametrizable (indirect) on the ignored paths and the baseline enableDNSSupport value; the existing test keeps its default behaviour.
The suite runs 32 pytest-xdist workers with LoadScheduling, which spreads
individual tests across worker PROCESSES. A fixture is therefore created
once in every worker that picks up a test from this file, at any scope --
so the module-scoped enable/restore pair ran three times in one run (the
three drift tests landed on gw31, gw9 and gw30), rolling the shared
ack-ec2-controller Deployment up to six times.
Each rollout stops reconciliation cluster-wide for a few seconds, which
is enough to fail an unrelated worker mid-assertion. Build 2092380219
lost three tests that way while all three drift tests passed:
TestTGW::test_create_delete KeyError: 'transitGatewayID'
TestTGW::test_crud_tags KeyError: 'transitGatewayID'
TestSecurityGroup::
test_self_ref_rule_no_perpetual_diff ACK.ResourceSynced timed out
Both shapes are "the controller was not reconciling during my window":
a status that never got populated, and a synced-condition wait that
expired.
Turn the gate on once and leave it on:
- Check-then-set, so a worker that finds it already on skips the patch.
Concurrent workers observing it off compute the same FEATURE_GATES
value from the same starting point, so the second patch leaves the pod
template identical, does not bump the Deployment generation and does
not roll a second time. No cross-process lock needed.
- No restore, which would cost a second rollout and could disable the
gate underneath a drift test still running in another worker. Safe to
leave on: the gate is inert unless a resource carries the
ignore-field-drift annotation, and only this file's resources do.
Still one rollout per run. Removing it entirely means setting the gate at
controller setup time (FEATURE_GATES in test-infra's controller-setup.sh,
alongside the existing IAMRoleSelector case), after which this fixture
degrades to a no-op check. Filed as a follow-up.
Three review findings, all confirmed against the code.
1. test_nested_field_under_ignored_parent claimed to exercise the runtime's
path-PREFIX match via a Delta path of Spec.Tags.N.Value. No such path
exists. pkg/resource/vpc/delta.go compares tags as a whole
(convertToOrderedACKTags + MapStringStringEqual) and emits a single
delta.Add("Spec.Tags", ...), and Path.Contains requires the ignored path
to be a prefix of the Delta path, so spec.tags vs Spec.Tags is segment
equality -- the branch test_tags_drift_ignored already covers.
Per-element paths are not expressible as an annotation either, since
isValidFieldPath rejects indices.
The scenario is still worth covering: a changed value on a DECLARED key
is what tags.Sync would actively revert, unlike an externally-added key.
Renamed to test_declared_tag_value_drift_ignored and the docstring now
says what it does and does not cover. Also corrected the class docstring,
which repeated the prefix-match claim.
2. The "edit retained but not pushed" half of test_scalar_field_drift_ignored
was vacuous. The external actor had already set the live attribute to
True, so patching spec.enableDNSSupport to True and asserting the live
value is True holds whether or not the edit was pushed.
Split into test_scalar_external_drift_ignored (external flip survives)
and test_scalar_spec_edit_not_pushed (edit made while AWS still holds
False, so declining to push it is observable). Reordering inside one test
does not work here: the field is boolean, so whichever half runs second
finds spec and AWS in agreement and asserts nothing. The edit case also
earns its own test because enableDNSSupport is pushed by
ModifyVpcAttribute in the custom update path, not by tags.Sync.
3. `except: pass` around `assert deleted` in the fixture teardown hid leaked
VPCs and CRs, and swallowed KeyboardInterrupt/SystemExit. Removed, so a
failed delete fails the teardown. The budget goes from 30s to 60s at the
same time: a VPC CR queued behind a busy controller regularly needs more
than 30s to clear its finalizer, and ec2's own test_vpc.py has flaked on
that exact assertion, so asserting on a 30s budget would trade a silent
leak for a flake.
The out-of-band `external` tag cleanup also moved into a finally, since as
a trailing statement it was skipped whenever an assertion above it failed.
Same two defects @gustavodiaz7722 found in elbv2-controller#90, which are generic to ACK drift e2e tests rather than specific to that repo. Verified present here against this controller's own config. 1. The drift-survives and stays-Synced assertions could not fail. An out-of-band AWS change produces no watch event, and this controller's resync period is the runtime default of 10 hours: config/controller/deployment.yaml (what controller-setup.sh deploys via kustomize) passes no --reconcile-default-resync-seconds or --reconcile-resource-resync-seconds, and pkg/resource/vpc/manager_factory.go returns 0 from RequeueOnSuccessSeconds(), so getResyncPeriod falls through to defaultResyncPeriod. No reconcile occurred during MODIFY_WAIT_AFTER_SECONDS, so asserting the external change survived observed that nobody had looked -- it held identically with the feature removed. The Synced checks were worse: wait_on_condition returned on its first poll off the ACK.ResourceSynced condition written back at create. Note this is a different vacuity from the one 5b177cf addressed. That was spec and AWS agreeing so no delta existed to suppress; this is no reconcile happening at all. Fixing the first did not touch the second. Affected: test_tags_drift_ignored, test_scalar_external_drift_ignored and test_declared_tag_value_drift_ignored, all of which drift out-of-band via CreateTags / ModifyVpcAttribute. Each now captures condition.get_synced_last_transition_time before the drift, forces a reconcile, and requires via wait_on_condition_after that a reconcile which started after the drift completed with ACK.ResourceSynced=True. That gate carries both original claims, so the separate (vacuous) Synced waits are gone rather than kept. A precondition assert also confirms AWS reports the drift before the reconcile is forced, so a race with the out-of-band call cannot make a test pass for an unrelated reason. The reconcile is forced by patching an inert annotation rather than a spec field, because the runtime adds AnnotationChangedPredicate to the event filter whenever the IgnoreFieldDrift gate is on (reconciler.go SetupWithManager; the default is GenerationChangedPredicate alone). Keeping the probe off the spec means the only delta the reconcile sees is the external drift itself. The probe value is a timestamp, not a constant, so the second call within test_tags_drift_ignored really changes the annotation -- re-patching an identical value would fire no event and silently degrade the wait to a no-op. test_scalar_spec_edit_not_pushed and the spec-edit half of test_tags_drift_ignored get the gate too, via _await_reconcile_after. Those patches do bump metadata.generation so a reconcile is queued, but a fixed sleep never established that it had finished. MODIFY_WAIT_AFTER_SECONDS is now unused and removed. 2. The fixture read status.vpcID before it existed. wait_resource_consumed_by_controller returns as soon as the resource has any .status, which is the first status write and can predate status.vpcID. Every test reads vpcID out of the CR ignore_field_drift_vpc yields, so that snapshot could hand them a KeyError they cannot recover from. The fixture now waits for ACK.ResourceSynced, then re-reads and asserts vpcID is present before yielding. Signed-off-by: Hao Wang <rhaowang@amazon.com>
e7ca069 to
bdbb4b0
Compare
|
New changes are detected. LGTM label has been removed. |
|
@sapphirew: 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. |
3 participants
Description
Adds e2e integration tests exercising the
services.k8s.aws/ignore-field-driftannotation (aws-controllers-k8s/runtime#256, released in runtimev0.62.0) against the EC2 VPC resource.EC2 is one of the controllers the runtime presubmit (
runtime_presubmit_services) regenerates and e2e-tests, so this coverage runs on runtime PRs and guards the feature against regressions.The
IgnoreFieldDriftfeature gate is Alpha and disabled by default, so the tests self-enable it on the deployed controller by patching the deployment'sFEATURE_GATESenv var and waiting for rollout.That patch rolls the shared controller Deployment, which is a cluster-wide side effect in a suite running 32 xdist workers — a restart mid-run can time out any other worker waiting on
ACK.ResourceSynced. The gate is therefore turned on once per run and never turned back off (check-then-set, so concurrent workers produce a byte-identical pod template and no second rollout; no restore, so a teardown in one worker can't disable the gate underneath a drift test still running in another). Leaving it on is safe because the gate is inert unless a resource carries the annotation, and only this file's resources do.Follow-up to remove the remaining rollout entirely: set
IgnoreFieldDriftinFEATURE_GATESat controller setup time in test-infrascripts/controller-setup.sh, alongside the existingIAMRoleSelectorcase, after which this fixture degrades to a no-op check.What the tests assert
Four cases, covering the field shapes and drift sources that matter:
test_tags_drift_ignored— collection field,spec.tagsignored:DeleteTagsfor drift on the ignored field;Syncedeven thoughspec.tagsdiffers from the live AWS tag set;spec.tagsis retained in the CR spec but is not pushed to AWS.test_scalar_external_drift_ignored— scalar leaf,spec.enableDNSSupportignored. An externalModifyVpcAttributeflip survives and the resource staysSynced.test_scalar_spec_edit_not_pushed— the same scalar, with the edit made while AWS still holds the other value, so declining to push it is observable. Separate test rather than a second half of the one above: the field is boolean, so after an external flip spec and AWS agree and the assertion would be vacuous. It also covers a different code path —enableDNSSupportis pushed byModifyVpcAttributein the custom update path, not bytags.Sync.test_declared_tag_value_drift_ignored— an externally-changed value on a tag key the CR declares. Distinct from an added key: this is the casetags.Syncwould actively revert.Note none of these exercise the runtime's path-prefix match. Every Delta path here equals its annotation path —
pkg/resource/vpc/delta.gocompares tags as a whole and emits a singledelta.Add("Spec.Tags", ...), sospec.tagsmatches by segment equality. Prefix matching needs a Delta subject deeper than the annotation, which none of VPC's ignorable fields produce.Testing
Ran locally via
make kind-test SERVICE=ec2against released runtimev0.62.0— no leaked VPCs or CRs (teardown fixture verified clean).Files
test/e2e/tests/test_vpc_ignore_field_drift.pytest/e2e/resources/vpc_ignore_field_drift.yamlNote on the third commit
aa14cd9 chore: regenerate apis/v1alpha1/types.gois unrelated to the tests and is here to getec2-verify-code-gengreen.code-generator#733 (
d13ed56, Aug 15) removed the forcedaws.JSONValueimport fromtemplates/apis/types.go.tpl. It is not in a release yet — latest isv0.62.1, cut two days earlier — but the presubmit regenerates with code-generatormain, so the committedtypes.go(generated atv0.62.1, perapis/v1alpha1/ack-generate-metadata.yaml) reads as drifted:The commit applies exactly that.
aws-sdk-gov1 stays ingo.mod—pkg/resource/*/hooks.gostill uses it — so nothing else changes. This is not specific to this PR:maincarries the stale hack, so any ec2-controller PR whose presubmit runs now hits it (s3-controllerandiam-controllerare in the same state;rds-controllernever had the hack, which is why its PRs stay green). Happy to split this into its own PR if you'd rather.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.