Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Addresses persistent diff with manage_default_network_acl #737

Merged
merged 4 commits into from
Jan 28, 2022

Conversation

lorengordon
Copy link
Contributor

@lorengordon lorengordon commented Jan 25, 2022

As noted in the terraform docs, subnets using the default network acl
will generate a persistent diff if they are not specified to the aws_default_network_acl
resource. This module was handling subnets created by the module, but
of course is not aware of subnets created externally to the module.

The docs suggest using lifecycle ignore_changes as an option to avoid
the persistent diff, which is the approach implemented in this patch.

Fixes #736

Breaking Changes

None

How Has This Been Tested?

  • I have tested and validated these changes using one or more of the provided examples/* projects
  1. cd examples/network-acls
  2. run terraform apply
  3. manually create a new subnet in the vpc
  4. run terraform apply
  5. view that no changes are planned

As noted in the [terraform docs][0], subnets using the default network acl
will generate a persistent diff if they are not specified to the aws_default_network_acl
resource. This module was handling subnets created by the module, but
of course is not aware of subnets created externally to the module.

The docs suggest using lifecycle ignore_changes as an option to avoid
the persistence diff, which is the approach implemented in this patch.

Fixes terraform-aws-modules#736

[0]: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/default_network_acl#managing-subnets-in-a-default-network-acl
Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@bryantbiggs What do you think?

@antonbabenko
Copy link
Member

Could you also fix the issue introduced by #742 here? :)

@lorengordon
Copy link
Contributor Author

Could you also fix the issue introduced by #742 here? :)

Haha, sure, on my phone, gimme 30 min too get back to a computer

@lorengordon
Copy link
Contributor Author

@antonbabenko Ok, should be good now!

@lorengordon
Copy link
Contributor Author

Also, not sure if you are tracking this already, but there is some kind of issue going on with the cidr_blocks outputs for unused features. After first apply, they are lists of nulls, but the second apply they are reported as a planned change, becoming lists of empty strings...

Changes to Outputs:
  ~ database_subnets_ipv6_cidr_blocks    = [
      - null,
      - null,
      - null,
      + "",
      + "",
      + "",
    ]
  ~ elasticache_subnets_ipv6_cidr_blocks = [
      - null,
      - null,
      - null,
      + "",
      + "",
      + "",
    ]
  ~ intra_subnets_ipv6_cidr_blocks       = [
      - null,
      - null,
      - null,
      + "",
      + "",
      + "",
    ]
  ~ private_subnets_ipv6_cidr_blocks     = [
      - null,
      - null,
      - null,
      + "",
      + "",
      + "",
    ]
  ~ public_subnets_ipv6_cidr_blocks      = [
      - null,
      - null,
      - null,
      + "",
      + "",
      + "",
    ]
  ~ redshift_subnets_ipv6_cidr_blocks    = [
      - null,
      - null,
      - null,
      + "",
      + "",
      + "",
    ]

You can apply this plan to save these new output values to the Terraform state, without changing any real infrastructure.

@antonbabenko antonbabenko merged commit d247d8e into terraform-aws-modules:master Jan 28, 2022
antonbabenko pushed a commit that referenced this pull request Jan 28, 2022
### [3.11.5](v3.11.4...v3.11.5) (2022-01-28)

### Bug Fixes

* Addresses persistent diff with manage_default_network_acl ([#737](#737)) ([d247d8e](d247d8e))
@antonbabenko
Copy link
Member

This PR is included in version 3.11.5 🎉

@antonbabenko
Copy link
Member

Regarding the issue with outputs you are seeing, I don't think we have seen such behavior before.

@lorengordon
Copy link
Contributor Author

Regarding the issue with outputs you are seeing, I don't think we have seen such behavior before.

Roger, I'll create a separate issue for that and see if I can track it down.

@@ -208,7 +202,7 @@ data "aws_iam_policy_document" "dynamodb_endpoint_policy" {
test = "StringNotEquals"
variable = "aws:sourceVpce"

values = [data.vpc.vpc_id]
values = [module.vpc.vpc_id]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oy, this is still wrong! it should be

Suggested change
values = [module.vpc.vpc_id]
values = [data.aws_vpc_endpoint.dynamodb.id]

because the condition is "source VPC endpoint" not "source VPC"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what it was before #742 (attempted to) change it to the vpc id! Oi! Revert and revert and revert!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can leave it as is now, but the condition is wrong - should be "aws:SourceVpc"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at it again now, the data source is a race condition. The comment makes no sense. This whole thing is confusing to me. Why not just get the vpce id from an output of the vpc endpoint module?

harrythebot pushed a commit to lolocompany/terraform-aws-vpc that referenced this pull request May 11, 2022
harrythebot pushed a commit to lolocompany/terraform-aws-vpc that referenced this pull request May 11, 2022
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Persistent diff when using manage_default_network_acl with subnets created outside config
3 participants