r/aws_instance: fix perpetual source_dest_check diff with primary_network_interface - #49198
Open
zoispag wants to merge 2 commits into
Open
r/aws_instance: fix perpetual source_dest_check diff with primary_network_interface#49198zoispag wants to merge 2 commits into
zoispag wants to merge 2 commits into
Conversation
…work_interface
When an ENI with `source_dest_check = false` is attached through a
`primary_network_interface` block, consecutive applies toggle the value
forever:
- `aws_instance` plans `source_dest_check` false -> true
- applying that issues ModifyInstanceAttribute, which writes through to
the ENI
- `aws_network_interface` then plans `source_dest_check` true -> false
- applying that puts us back at the first step
`source_dest_check` defaults to `true`, Read populates it from the primary
ENI, and its DiffSuppressFunc plus the ModifyInstanceAttribute guard in
Update both only accounted for the deprecated `network_interface` block.
Practitioners cannot work around this by setting the correct value on the
instance either, because `source_dest_check` is in `ConflictsWith` for
`primary_network_interface`.
Extend both checks to `primary_network_interface` so the attribute is owned
solely by the `aws_network_interface` resource, as it already is for
`network_interface`.
`primary_network_interface` is Optional+Computed and Read always populates
it, so `d.GetOk` cannot tell a configured block from a computed one --
using it would suppress the diff for every instance and break
`source_dest_check` on instances that do not manage their own ENI. The new
`hasConfiguredPrimaryNetworkInterface` helper consults the raw
configuration instead.
Fixes hashicorp#44768
Contributor
|
✅ Thank you for correcting the previously detected issues! The maintainers appreciate your efforts to make the review process as smooth as possible. |
Contributor
Community GuidelinesThis comment is added to every new Pull Request to provide quick reference to how the Terraform AWS Provider is maintained. Please review the information below, and thank you for contributing to the community that keeps the provider thriving! 🚀 Voting for Prioritization
Pull Request Authors
|
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
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.
Description
When an ENI with
source_dest_check = falseis attached to an instance through aprimary_network_interfaceblock, consecutive applies toggle the value forever:aws_instanceplanssource_dest_checkfalse -> trueModifyInstanceAttribute, which writes straight through to the primary ENIaws_network_interfacethen planssource_dest_checktrue -> falsesource_dest_checkhasDefault: true,Readpopulates it from the primary ENI, and both itsDiffSuppressFuncand theModifyInstanceAttributeguard inUpdateonly accounted for the deprecatednetwork_interfaceblock. Practitioners can't sidestep it by setting the correct value on the instance either —source_dest_checkis inConflictsWithforprimary_network_interface.This extends both checks to
primary_network_interface, so the attribute is owned solely by theaws_network_interfaceresource, exactly as it already is fornetwork_interface.One wrinkle worth calling out for review:
primary_network_interfaceisOptional+ComputedandReadalways populates it, sod.GetOkcannot distinguish a configured block from a computed one. UsingGetOkhere would suppress the diff for every instance and silently breaksource_dest_checkon instances that don't manage their own ENI. The newhasConfiguredPrimaryNetworkInterfacehelper consults the raw configuration instead. (network_interfaceavoids this becauseReadonly writes back the device indexes present in configuration, leaving it empty when unconfigured.)Docs for
source_dest_checkare updated to state theConflictsWithbehavior and point ataws_network_interface.Relations
Fixes #44768
References
SourceDestCheckattribute is the primary ENI's attribute, which is why theUpdatewrite is observable as drift on theaws_network_interfaceresource.Output from Acceptance Testing
Run in
eu-central-1.To confirm the new test is genuine regression coverage rather than something that would pass either way, I also ran it against unmodified
mainwith only the test cherry-picked (i.e. theec2_instance.gochange reverted). It fails with exactly the behaviour reported in #44768:I have not run the full
TestAccEC2Instance_suite, so a maintainer run of the neighbouringTestAccEC2Instance_PrimaryNetworkInterface_*,TestAccEC2Instance_NetworkInterface_*andTestAccEC2Instance_sourceDestChecktests would still be worthwhile to confirm nothing regressed on the paths this touches.