fix: Dns_options for vpc-endpoints

taylorsilva commented 10 months ago

Description

While trying to setup an S3 VPC endpoint I found I could not set dns_options.private_dns_only_for_inbound_resolver_endpoint even though this was apparently resolved by https://github.com/terraform-aws-modules/terraform-aws-vpc/pull/1029

To see what was going on under the hood, I removed the try wrapping private_dns_only_for_inbound_resolver_endpoint and got this error:

Planning failed. Terraform encountered an error while generating this plan.

╷
│ Error: Unsupported attribute
│
│   on .terraform/modules/endpoints/modules/vpc-endpoints/main.tf line 42, in resource "aws_vpc_endpoint" "this":
│   42:       private_dns_only_for_inbound_resolver_endpoint = dns_options.value.private_dns_only_for_inbound_resolver_endpoint
│     ├────────────────
│     │ dns_options.value is false
│
│ Can't access attributes on a primitive-typed value (bool).

So this whole thing is already off. Idk why dns_options.value is a bool, all I know is it has something to do with the dynamic block being used here.

I noticed the dynamic block is not needed. You're only allowed one dns_options block for the aws_vpc_endpoint resource: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/vpc_endpoint#dns_options

So I removed the dynamic block and updated the value lookups to each.value.dns_options[] which now works.

For an S3 service, with dns_options.private_dns_only_for_inbound_resolver_endpoint set to false, this is the plan it generated:

+ resource "aws_vpc_endpoint" "this" {
   + arn                   = (known after apply)
   + cidr_blocks           = (known after apply)
   + dns_entry             = (known after apply)
   + id                    = (known after apply)
   + ip_address_type       = (known after apply)
   + network_interface_ids = (known after apply)
   + owner_id              = (known after apply)
   + policy                = jsonencode(
         {
           + Statement = [
               + {
                   + Action    = "*"
                   + Condition = {
                       + StringNotEquals = {
                           + "aws:SourceVpc" = "vpc-0518b6428b706516d"
                         }
                     }
                   + Effect    = "Deny"
                   + Principal = "*"
                   + Resource  = "*"
                 },
             ]
           + Version   = "2012-10-17"
         }
     )
   + prefix_list_id        = (known after apply)
   + private_dns_enabled   = true
   + requester_managed     = (known after apply)
   + route_table_ids       = (known after apply)
   + security_group_ids    = [
       + "sg-050ed3bb7b46fd681",
     ]
   + service_name          = "com.amazonaws.us-east-1.s3"
   + state                 = (known after apply)
   + subnet_ids            = [
       + "subnet-0af61b529ffb7940b",
       + "subnet-0dd27b92ffe0d02cd",
     ]
   + tags                  = {
       + "Name" = "s3"
     }
   + tags_all              = {
       + "Name" = "s3"
     }
   + vpc_endpoint_type     = "Interface"
   + vpc_id                = "vpc-0518b6428b706516d"

   + dns_options {
       + dns_record_ip_type                             = (known after apply)
       + private_dns_only_for_inbound_resolver_endpoint = false
     }

   + timeouts {
       + create = "10m"
       + delete = "10m"
       + update = "10m"
     }
 }

For a resource that did not pass in a dns_options, like this:

...
ecr_api = {
      service             = "ecr.api"
      private_dns_enabled = true
      subnet_ids          = module.vpc.private_subnets
      policy              = data.aws_iam_policy_document.generic_endpoint_policy.json
    },
...

I got this plan:

+ resource "aws_vpc_endpoint" "this" {
   + arn                   = (known after apply)
   + cidr_blocks           = (known after apply)
   + dns_entry             = (known after apply)
   + id                    = (known after apply)
   + ip_address_type       = (known after apply)
   + network_interface_ids = (known after apply)
   + owner_id              = (known after apply)
   + policy                = jsonencode(
         {
           + Statement = [
               + {
                   + Action    = "*"
                   + Condition = {
                       + StringNotEquals = {
                           + "aws:SourceVpc" = "vpc-0518b6428b706516d"
                         }
                     }
                   + Effect    = "Deny"
                   + Principal = "*"
                   + Resource  = "*"
                 },
             ]
           + Version   = "2012-10-17"
         }
     )
   + prefix_list_id        = (known after apply)
   + private_dns_enabled   = true
   + requester_managed     = (known after apply)
   + route_table_ids       = (known after apply)
   + security_group_ids    = [
       + "sg-050ed3bb7b46fd681",
     ]
   + service_name          = "com.amazonaws.us-east-1.ecr.api"
   + state                 = (known after apply)
   + subnet_ids            = [
       + "subnet-0af61b529ffb7940b",
       + "subnet-0dd27b92ffe0d02cd",
     ]
   + tags                  = {
       + "Name" = "ecr.api"
     }
   + tags_all              = {
       + "Name" = "ecr.api"
     }
   + vpc_endpoint_type     = "Interface"
   + vpc_id                = "vpc-0518b6428b706516d"

   + dns_options {
       + dns_record_ip_type = (known after apply)
     }

   + timeouts {
       + create = "10m"
       + delete = "10m"
       + update = "10m"
     }
 }

I think the removal of the dynamic block simplifies things now. It's easier to see what's going on. There should be no change for anyone that isn't setting dns_options as shown by my plan output above.

Breaking Changes

None. For VPCE's that I had beforehand, nothing changed for them. Existing setup was respected.

How Has This Been Tested?

[ ] I have updated at least one of the examples/* to demonstrate and validate my change(s)
[x] I have tested and validated these changes using one or more of the provided examples/* projects
[ ] I have executed pre-commit run -a on my pull request

taylorsilva commented 10 months ago

I looked at the pre-commit stuff and it looked like a lot to setup. If you really need me to do it to get the PR in then let me know and I'll make the time. Not feeling it right now because I've already spent a bunch of time troubleshooting this and it's late now 😭

bryantbiggs commented 10 months ago

its dynamic because its an optional argument, so removing the dynamic block is not correct.

lets start with an issue and a reproduction

erezo9 commented 10 months ago

@taylorsilva it should be like this private_dns_enabled = true dns_options = { private_dns_only_for_inbound_resolver_endpoint = true }

like @bryantbiggs said, its optional that is why i put dynamic in the original PR it is also mentioned in the examples https://github.com/terraform-aws-modules/terraform-aws-vpc/blob/master/examples/complete/main.tf#L107

github-actions[bot] commented 9 months ago

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.

terraform-aws-modules / terraform-aws-vpc