Fix aws elb hosted zone id add nlb ids

[jukie]

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #7988

Changes proposed in this pull request:

• Add elb_type argument to support Network Load Balancers
• Update acceptance tests for new argument
• Update web docs

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSElbHostedZoneId_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSElbHostedZoneId_basic -timeout 120m
=== RUN   TestAccAWSElbHostedZoneId_basic
=== PAUSE TestAccAWSElbHostedZoneId_basic
=== CONT  TestAccAWSElbHostedZoneId_basic
--- PASS: TestAccAWSElbHostedZoneId_basic (11.49s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	11.612s

...

[jukie]

Updated my branch to let elb_type be an optional argument which defaults to application/classic if not set. I think that's the best option in order to avoid breaking existing templates while also allowing users to get network hz id's.

[carinadigital]

@jukie could you fix the conflicts ?

[jukie]

Sure I'll fix them now, any ideas why us-gov-west-1's elb hosted zone id was removed? 2954561#diff-e47acbc3ed755feb1ae98b9f5b6a7115L29

[jukie]

Updated with correct NLB id's as well as upstream changes to drop us-gov-west-1 due to what appears to be a lack of documentation.

[jukie]

Updated test run below:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSElbHostedZoneId_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSElbHostedZoneId_basic -timeout 120m
=== RUN   TestAccAWSElbHostedZoneId_basic
=== PAUSE TestAccAWSElbHostedZoneId_basic
=== CONT  TestAccAWSElbHostedZoneId_basic
--- PASS: TestAccAWSElbHostedZoneId_basic (28.06s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	28.161s

[jukie]

Should be ready now @carinadigital

[carinadigital]

LGTM. Once comment about variable naming.

[jukie]

Updated acctest after variable changes:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSElbHostedZoneId_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSElbHostedZoneId_basic -timeout 120m
=== RUN   TestAccAWSElbHostedZoneId_basic
=== PAUSE TestAccAWSElbHostedZoneId_basic
=== CONT  TestAccAWSElbHostedZoneId_basic
--- PASS: TestAccAWSElbHostedZoneId_basic (27.70s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	27.759s

[jukie]

Updated acctest:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSElbHostedZoneId_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSElbHostedZoneId_basic -timeout 120m
=== RUN   TestAccAWSElbHostedZoneId_basic
=== PAUSE TestAccAWSElbHostedZoneId_basic
=== CONT  TestAccAWSElbHostedZoneId_basic
--- PASS: TestAccAWSElbHostedZoneId_basic (27.06s)
PASS

[bflad]

Hey @jukie 👋 Thanks for working on this! It would be really nice to get the NLB hosted zone information somewhere available in the provider.

I'm personally very torn on whether we should expand the existing data source (and potentially adding confusion with "ELB" in its naming) versus just adding a new data source (call it aws_nlb_hosted_zone or aws_nlb_hosted_zone_id although we've been shying away from _id naming since data sources may have other attributes in the future, poor aws_subnet_ids 😅). If we go the new data source route, we can also create a aws_alb_hosted_zone data source that points to the same underlying information as aws_elb_hosted_zone_id.

How do you feel about leaving the existing data source and just adding this as a new aws_nlb_hosted_zone data source?

[jukie]

Well "ELB" technically encompasses Classic, ALB, and NLB if I'm not mistaken but I also recognize the possible confusion because when people say ELB they're usually referring to the "Classic" type.
I'd be fine with splitting them. Would you also happen to know if there's any api call or managed endpoint that would return the list of id's at https://docs.aws.amazon.com/general/latest/gr/rande.html? Unless these are guaranteed to stay the same, we should probably be validating the values we're hardcoding in the data source as well.

[shmick]

I went looking for aws_nlb_hosted_zone_idand came across this PR. Is there any update on this?

[jukie]

@bflad In my opinion splitting them will be a duplication of code so adding a type attribute with a default value of classic/alb to avoid breaking existing user's config seems like a good option but I'm happy to hear alternatives.

• The term ELB encompasses ALB, Classic, and NLB so it seems like only splitting off NLB could create confusion.

• This data source has a single purpose of returning an elb hosted zone id so I'd expect that if there's any feature/attribute expansion it would happen in the aws_lb or aws_elb data sources and if needed we'd make the determination about splitting the "main" lb data sources.

[jukie]

Updated with missing regions and an upstream merge.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSElbHostedZoneId_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSElbHostedZoneId_basic -timeout 120m
=== RUN   TestAccAWSElbHostedZoneId_basic
=== PAUSE TestAccAWSElbHostedZoneId_basic
=== CONT  TestAccAWSElbHostedZoneId_basic
--- PASS: TestAccAWSElbHostedZoneId_basic (38.50s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	40.879s

[jukie]

Any new thoughts on this? Can rework if necessary.

[zhelding]

Pull request #21306 has significantly refactored the AWS Provider codebase. As a result, most PRs opened prior to the refactor now have merge conflicts that must be resolved before proceeding.

Specifically, PR #21306 relocated the code for all AWS resources and data sources from a single aws directory to a large number of separate directories in internal/service, each corresponding to a particular AWS service. This separation of code has also allowed for us to simplify the names of underlying functions -- while still avoiding namespace collisions.

We recognize that many pull requests have been open for some time without yet being addressed by our maintainers. Therefore, we want to make it clear that resolving these conflicts in no way affects the prioritization of a particular pull request. Once a pull request has been prioritized for review, the necessary changes will be made by a maintainer -- either directly or in collaboration with the pull request author.

For a more complete description of this refactor, including examples of how old filepaths and function names correspond to their new counterparts: please refer to issue #20000.

For a quick guide on how to amend your pull request to resolve the merge conflicts resulting from this refactor and bring it in line with our new code patterns: please refer to our Service Package Refactor Pull Request Guide.

[ewbankkit]

Superseded by #24749.
@jukie Thanks for the contribution 🎉 👏.

[github-actions]

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.