d/aws_prefix_list: fixes and enhancements

[roberth-k]

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" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates #14068, #13986

Contains bugfixes and enhancements to the aws_prefix_list data source discovered while implementing support for Managed Prefix Lists.

Users may be affected by the following changes in behavior:

• Where no criteria have been provided to the data source, the data source currently produces an arbitrary prefix list. After this PR, it will raise an error.
• Where a resource depends on the API ordering of CIDR blocks (such as by selecting cidr_blocks[0]), the result may now be a different CIDR block.

Release note for CHANGELOG:

ENHANCEMENTS:
- data-source/aws_prefix_list: The `cidr_blocks` attribute is now lexically sorted
- data-source/aws_prefix_list: Add documentation example about finding a prefix list by name

BUG FIXES:
- data-source/aws_prefix_list: Raise error when criteria match more than one prefix list
- data-source/aws_prefix_list: Specifying the `name` attribute no longer overwrites all filters

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS=-run=TestAccDataSourceAwsPrefixList_
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccDataSourceAwsPrefixList_ -timeout 120m
--- PASS: TestAccDataSourceAwsPrefixList_matchesTooMany (7.09s)
--- PASS: TestAccDataSourceAwsPrefixList_nameDoesNotOverrideFilter (7.10s)
--- PASS: TestAccDataSourceAwsPrefixList_basic (27.39s)
--- PASS: TestAccDataSourceAwsPrefixList_filter (28.37s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       29.752s
...

[bflad]

Hi @roberth-k 👋 Thank you for submitting this! Overall this is good stuff, but I marked this as breaking changes for now since we would want to hold off on purposefully introducing changes that effect practitioner experience until a major version update. We can split this up into fixing the name versus filter bug now and separately introducing the other changes.

Please reach out if you have any questions. 😄

[bflad]

The predominate coding style for the codebase is if conditionals with early returns rather than bare switch conditionals. Can you please change this back rather than introducing a different coding style? Thanks!

[bflad]

While this is a perfectly valid change for consistency with most other data sources, we should only perform this change during a major version upgrade to align with practitioner versioning expectations, which the next one will be a few months away. It would be best to remove this change here and create a separate GitHub bug report so we can mark it for that future milestone. 👍 You're more than welcome to submit this change as well separately.

[bflad]

Similarly here with respect to introducing a breaking change which should be removed for now.

In this case though, if the ordering of the CIDR Blocks is not significant (which it likely is not), the cidr_blocks attribute should be changed to TypeSet instead of attempting to add ordering that is inconsistent with the semantics of the upstream API. Terraform configurations can and should use built in language support such as resource for_each for handling all elements of an attribute like this to avoid ordering concerns.

At some point in the future we may go back through many of the TypeList attributes and convert them to TypeSet to ensure that Terraform configurations are not depending on ordering where its not guaranteed or semantically correct.

[bflad]

Some of our newer codebase linting will likely pick this up on rebase/merge for not being AWS Region agnostic. Similar to the website example you added (👍 ) we should use the aws_region data source here so the configuration can be applied in anywhere. This also means we'll need to remove the hardcoded EC2 Prefix List identifier. Something like this should get you close:

data "aws_region" "current" {}

data "aws_prefix_list" "dynamodb" {
  name = "com.amazonaws.${data.aws_region.current.name}.dynamodb"
}

data "aws_prefix_list" "test" {
  name = "com.amazonaws.${data.aws_region.current.name}.s3"

  filter {
    name = "prefix-list-id"
    values = [data.aws_prefix_list.dynamodb.id]
  }
}

[roberth-k]

@bflad The managed prefix list data source has been spun off as #16738, and the fixes for the original data source at #16739.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!