New resource: aws_ec2_managed_prefix_list #14068

roberth-k · 2020-07-06T23:07:32Z

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

Release note for CHANGELOG:

FEATURES:
- **New Resource:** `aws_ec2_managed_prefix_list`

ENHANCEMENTS:
- resource/aws_security_group: Update documentation with managed prefix lists in mind
- resource/aws_security_group_rule: Update documentation with managed prefix lists in mind

Update to route tables expected with #14013.

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS=-run=TestAccAwsEc2ManagedPrefixList
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAwsEc2ManagedPrefixList -timeout 120m
--- PASS: TestAccAwsEc2ManagedPrefixList_disappears (20.64s)
--- PASS: TestAccAwsEc2ManagedPrefixList_exceedLimit (28.56s)
--- PASS: TestAccAwsEc2ManagedPrefixList_name (38.95s)
--- PASS: TestAccAwsEc2ManagedPrefixList_basic (41.02s)
--- PASS: TestAccAwsEc2ManagedPrefixList_tags (78.83s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       80.429s

ewbankkit · 2020-07-07T18:47:21Z

@roberth-k Thanks for the great contribution 👏.
A few high-level comments:

As the EC2 APIs are separate, I suggest having a separate aws_managed_prefix_list data source and calling out in the documentation that this is for customer-managed prefix lists and the existing aws_prefix_list data source is for AWS-managed prefix lists
Split the new data source into a separate PR (dependent on the resource PR as you'll need the corresponding resource for acceptance tests) - Helps with speed of review
The fixes to the current aws_prefix_list data source (thanks) can then be in another separate PR (once again, aids review)
Given that the same EC2 API updates both the PL name and the entries in the PL, are there use cases for having a separate aws_managed_prefix_list_entry resources?

Yes, I'm working towards support in the aws_route and aws_route_table resources with #14014 and #14013 respectively - There are a number of bugs in those 2 that need addressing to get to a solid implementation.

roberth-k · 2020-07-08T12:18:31Z

Thanks @ewbankkit.

As the EC2 APIs are separate, I suggest having a separate aws_managed_prefix_list data source and calling out in the documentation that this is for customer-managed prefix lists and the existing aws_prefix_list data source is for AWS-managed prefix lists

Split the new data source into a separate PR (dependent on the resource PR as you'll need the corresponding resource for acceptance tests) - Helps with speed of review

The fixes to the current aws_prefix_list data source (thanks) can then be in another separate PR (once again, aids review)

The DescribeManagedPrefixLists API is a superset of DescribePrefixLists, even returning AWS's own prefix lists by default. A separate aws_managed_prefix_list resource would have to artificially insert a filter by owner-id in order to produce only customer-managed prefix lists. It seemed appropriate to present an unambiguous data source, though I did at one point consider aliasing the two names.

Existing uses of data.aws_prefix_list would be affected only where no filters had been provided to the data source, in which case the current behavior is to produce an arbitrary AWS prefix list, but after this PR would be to raise an error.

However, it is true that the two API-s are physically separate, so I'm happy to split it out if that's the preferred practice. In any case, I'll move these changes to a separate PR.

Given that the same EC2 API updates both the PL name and the entries in the PL, are there use cases for having a separate aws_managed_prefix_list_entry resources?

aws_managed_prefix_list_entry follows the example of security groups and routes, allowing the prefix list to be populated from other Terraform modules.

It's true that there's no separate API for it, and implementing the _entry resource did necessitate trading off support for the version attribute on the main resource (as adding or removing entries would affect it). However: the versioning aspect of Managed Prefix Lists behaves similar to e.g. RevisionId in Lambda, in that it's mainly used as a race detector, and the "restore" functionality still increments it. Ultimately the practical value of _entry seemed worth it.

roberth-k · 2020-07-09T10:54:52Z

The data source changes have been split into #14109 and #14110.

dturnbu · 2020-07-15T14:00:32Z

Do we have any idea when this will be merged and made available for use?

kuperiudazn · 2020-07-17T08:16:04Z

Any ETA for the release ?

roberth-k · 2020-07-19T10:14:56Z

Per #12690 (comment) it may be a while before this is picked up.

In the meanwhile, I recommend using the aws_cloudformation_stack resource to wrap an AWS::EC2::PrefixList with DeletionPolicy Retain for eventual import into Terraform.

roberth-k · 2020-07-21T21:54:02Z

The resources have been renamed to aws_prefix_list and aws_prefix_list_entry, and the data source changes updated accordingly.

The new Managed Prefix List API is a superset of the regular Prefix List API. Customer and AWS prefix lists share the same ID format and namespace, and CloudFormation's AWS::EC2::PrefixList resource does not differentiate between managed and non-managed. I've therefore elected to drop most references to prefix lists being managed. The resource was originally named aws_managed_prefix_list, and this remains reflected in the branch name.

teamterraform · 2020-08-18T21:25:49Z

Notification of Recent and Upcoming Changes to Contributions

Thank you for this contribution! There have been a few recent development changes that affect this pull request. We apologize for the inconvenience, especially if there have been long review delays up until now. Please note that this is automated message from an unmonitored account. See the FAQ for additional information on the maintainer team and review prioritization.

If you are unable to complete these updates, please leave a comment for the community and maintainers so someone can potentially continue the work. The maintainers will encourage other contributors to use the existing contribution as the base for additional changes as appropriate. Otherwise, contributions that do not receive updated code or comments from the original contributor may be closed in the future so the maintainers can focus on active items.

For the most up to date information about Terraform AWS Provider development, see the Contributing Guide. Additional technical debt changes can be tracked with the technical-debt label on issues.

As part of updating a pull request with these changes, the most current unit testing and linting will run. These may report issues that were not previously reported.

Terraform 0.12 Syntax

Reference: #8950
Reference: #14417

Version 3 and later of the Terraform AWS Provider, which all existing contributions would potentially be added, only supports Terraform 0.12 and later. Certain syntax elements of Terraform 0.11 and earlier show deprecation warnings during runs with Terraform 0.12. Documentation and test configurations, such as those including deprecated string interpolations (some_attribute = "${aws_service_thing.example.id}") should be updated to the newer syntax (some_attribute = aws_service_thing.example.id). Contribution testing will automatically fail on older syntax in the near future. Please see the referenced issues for additional information.

Action Required: Terraform Plugin SDK Version 2

Reference: #14551

The Terraform AWS Provider has been upgraded to the latest version of the Terraform Plugin SDK. Generally, most changes to contributions should only involve updating Go import paths in source code files. Please see the referenced issue for additional information.

Removal of website/aws.erb File

Reference: #14712

Any changes to the website/aws.erb file are no longer necessary and should be removed from this contribution to prevent merge issues in the near future when the file is removed from the repository. Please see the referenced issue for additional information.

Upcoming Change of Git Branch Naming

Reference: #14292

Development environments will need their upstream Git branch updated from master to main in the near future. Please see the referenced issue for additional information and scheduling.

Upcoming Change of GitHub Organization

Reference: #14715

This repository will be migrating from https://github.com/terraform-providers/terraform-provider-aws to https://github.com/hashicorp/terraform-provider-aws. No practitioner or developer action is anticipated and most GitHub functionality will automatically redirect to the new location. Go import paths including terraform-providers can remain for now. Please see the referenced issue for additional information and scheduling.

t0rr3sp3dr0 · 2020-08-25T00:24:16Z

Any updates on this?

bflad · 2020-12-03T18:49:03Z

Is the data source implementation (#14110, #14109) slated for merge as well?

I will separately reach out in those PRs -- the functionality will be reviewed and handled in some fashion as part of including this new functionality as well.

bflad

Hi @roberth-k 👋 Thank you for updating this pull request. I have provided some initial feedback here, with the biggest item to remove the aws_ec2_managed_prefix_list_entry resource for now so we can focus on getting the aws_ec2_managed_prefix_list resource merged in. Please reach out with any questions or let us know if you will not have time to make these updates. 👍

bflad · 2020-12-10T15:20:48Z

aws/provider.go

@@ -599,6 +599,8 @@ func Provider() *schema.Provider {
 "aws_ec2_fleet": resourceAwsEc2Fleet(),
 "aws_ec2_local_gateway_route": resourceAwsEc2LocalGatewayRoute(),
 "aws_ec2_local_gateway_route_table_vpc_association": resourceAwsEc2LocalGatewayRouteTableVpcAssociation(),
+ "aws_ec2_managed_prefix_list": resourceAwsEc2ManagedPrefixList(),
+ "aws_ec2_managed_prefix_list_entry": resourceAwsEc2ManagedPrefixListEntry(),


Due to the complexity of the EC2 API using versioning on the parent resource and needing to handle the concurrency issues on the EC2 API which is ultimately eventually consistent, our preference would be to not introduce this separate resource at this time. That is not to say we would never introduce a new resource like this, however trying to work through all the concurrency issues in this PR seems counterproductive when we can introduce the standalone aws_ec2_managed_prefix_list resource more immediately. 👍

Suggested change

"aws_ec2_managed_prefix_list_entry": resourceAwsEc2ManagedPrefixListEntry(),

bflad · 2020-12-10T15:26:37Z

aws/resource_aws_ec2_managed_prefix_list.go

+ Type: schema.TypeSet,
+ Optional: true,
+ Computed: true,
+ ConfigMode: schema.SchemaConfigModeAttr,


New resources should avoid declaring ConfigMode for now. For more context, it exists on older resources where requiring dynamic in configurations was a large barrier to upgrading to Terraform CLI 0.12.

Suggested change

ConfigMode: schema.SchemaConfigModeAttr,

bflad · 2020-12-10T15:27:01Z

aws/resource_aws_ec2_managed_prefix_list.go

+ Computed: true,
+ ConfigMode: schema.SchemaConfigModeAttr,
+ Elem: prefixListEntrySchema(),
+ Set: awsPrefixListEntrySetHashFunc,


We should be able to avoid this custom Set function here:

Suggested change

Set: awsPrefixListEntrySetHashFunc,

bflad · 2020-12-10T15:27:46Z

aws/resource_aws_ec2_managed_prefix_list.go

+ }
+}
+
+func prefixListEntrySchema() *schema.Resource {


Given that we will not be including the _entry resource initially, let's move this inline above. 👍

bflad · 2020-12-10T15:28:50Z

aws/resource_aws_ec2_managed_prefix_list.go

+
+ output, err := conn.CreateManagedPrefixList(&input)
+ if err != nil {
+ return fmt.Errorf("failed to create managed prefix list: %v", err)


We have some pending linters to setup for this, but preferred to use %w for returning AWS Go SDK errors:

Suggested change

return fmt.Errorf("failed to create managed prefix list: %v", err)

return fmt.Errorf("error creating EC2 Managed Prefix List: %w", err)

bflad · 2020-12-10T16:00:32Z

aws/resource_aws_ec2_managed_prefix_list_test.go

+
+const testAccAwsEc2ManagedPrefixListConfig_basic_create = `
+resource "aws_ec2_managed_prefix_list" "test" {
+ name = "tf-test-basic-create"


We should randomize the naming of these resources -- please see the Contributing Guide for information about this (or searching for rName usage elsewhere).

bflad · 2020-12-10T16:02:04Z

aws/resource_aws_ec2_managed_prefix_list_test.go

+
+const testAccAwsEc2ManagedPrefixListConfig_disappears = `
+resource "aws_ec2_managed_prefix_list" "test" {
+ name = "tf-test-disappears"


Similarly here, regarding naming randomization.

bflad · 2020-12-10T16:03:21Z

aws/resource_aws_ec2_managed_prefix_list_test.go

+ resourceName := "aws_ec2_managed_prefix_list.test"
+ pl := ec2.ManagedPrefixList{}
+
+ checkName := func(name string) resource.TestCheckFunc {


Similarly here, regarding using ImportStateVerify instead.

bflad · 2020-12-10T16:03:30Z

aws/resource_aws_ec2_managed_prefix_list_test.go

+ resourceName := "aws_ec2_managed_prefix_list.test"
+ pl := ec2.ManagedPrefixList{}
+
+ checkTags := func(m map[string]string) resource.TestCheckFunc {


Similarly here, regarding using ImportStateVerify instead.

bflad · 2020-12-10T16:05:14Z

website/docs/r/ec2_managed_prefix_list.html.markdown

+~> **NOTE on Prefix Lists and Prefix List Entries:** Terraform currently
+provides both a standalone [Managed Prefix List Entry resource](ec2_managed_prefix_list_entry.html),
+and a Prefix List resource with an `entry` set defined in-line. At this time you
+cannot use a Prefix List with in-line rules in conjunction with any Prefix List Entry
+resources. Doing so will cause a conflict of rule settings and will unpredictably
+fail or overwrite rules.
+


Won't be needed:

Suggested change

~> **NOTE on Prefix Lists and Prefix List Entries:** Terraform currently

provides both a standalone [Managed Prefix List Entry resource](ec2_managed_prefix_list_entry.html),

and a Prefix List resource with an `entry` set defined in-line. At this time you

cannot use a Prefix List with in-line rules in conjunction with any Prefix List Entry

resources. Doing so will cause a conflict of rule settings and will unpredictably

fail or overwrite rules.

bflad · 2020-12-15T04:40:55Z

Please note that I've updated the milestone for this pull request to version 3.22.0, which we expect to release on Thursday as our last release before the new year so its not lingering over the long break. If we do not see the updates prior to Wednesday, we will add commits on top of the existing ones to ensure this is tested and merged in time. 👍

roberth-k · 2020-12-16T15:00:19Z

@bflad I'll get the changes in now.

bflad · 2020-12-17T03:29:06Z

Thank you for your work on this, @roberth-k, pulling in your commits now. 👍

ghost · 2020-12-18T22:26:01Z

This has been released in version 3.22.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2021-01-16T17:10:26Z

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!

roberth-k requested a review from a team July 6, 2020 23:07

This was referenced Jul 9, 2020

d/aws_prefix_list: fixes and enhancements #14109

Closed

d/aws_prefix_list: add support for managed prefix lists #14110

Closed

anGie44 force-pushed the master branch from 54db70d to 48ba2f7 Compare July 21, 2020 18:26

roberth-k force-pushed the f-aws_managed_prefix_list branch 2 times, most recently from 863752c to cbddc42 Compare July 21, 2020 21:25

ghost added service/apigateway Issues and PRs that pertain to the apigateway service. service/rds Issues and PRs that pertain to the rds service. labels Jul 21, 2020

roberth-k force-pushed the f-aws_managed_prefix_list branch from cbddc42 to 354559c Compare July 21, 2020 21:35

rafaelleonardocruz approved these changes Sep 15, 2020

View reviewed changes

bflad self-assigned this Dec 3, 2020

bflad added new-resource Introduces a new resource. and removed needs-triage Waiting for first response or review from a maintainer. service/apigateway Issues and PRs that pertain to the apigateway service. service/rds Issues and PRs that pertain to the rds service. labels Dec 3, 2020

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Dec 3, 2020

roberth-k force-pushed the f-aws_managed_prefix_list branch from 354559c to 45eeaf4 Compare December 5, 2020 14:26

roberth-k requested a review from a team as a code owner December 5, 2020 14:26

roberth-k changed the title ~~Add managed prefix list support~~ New resource: aws_ec2_managed_prefix_list & _entry Dec 5, 2020

r/aws_ec2_managed_prefix_list: new resource

56e912f

roberth-k force-pushed the f-aws_managed_prefix_list branch 3 times, most recently from 5a29531 to 9a3c7e8 Compare December 5, 2020 14:50

r/aws_ec2_managed_prefix_list_entry: new resource

efc5819

roberth-k force-pushed the f-aws_managed_prefix_list branch from 9a3c7e8 to efc5819 Compare December 5, 2020 14:56

bflad suggested changes Dec 10, 2020

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Dec 10, 2020

roberth-k mentioned this pull request Dec 13, 2020

New data source: aws_ec2_managed_prefix_list #16738

Merged

bflad modified the milestones: Roadmap, v3.22.0 Dec 15, 2020

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Dec 16, 2020

roberth-k added 2 commits December 16, 2020 20:06

r/aws_ec2_managed_prefix_list_entry: remove resource

0dcab47

r/aws_ec2_managed_prefix_list: code review updates

a78644e

roberth-k force-pushed the f-aws_managed_prefix_list branch from 1aa75e0 to a78644e Compare December 16, 2020 20:06

roberth-k changed the title ~~New resource: aws_ec2_managed_prefix_list & _entry~~ New resource: aws_ec2_managed_prefix_list Dec 16, 2020

bflad merged commit a98038d into hashicorp:master Dec 17, 2020

bflad added a commit that referenced this pull request Dec 17, 2020

Update CHANGELOG for #14068

dc57570

ghost locked as resolved and limited conversation to collaborators Jan 16, 2021

roberth-k deleted the f-aws_managed_prefix_list branch January 16, 2022 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New resource: aws_ec2_managed_prefix_list #14068

New resource: aws_ec2_managed_prefix_list #14068

roberth-k commented Jul 6, 2020 •

edited

Loading

ewbankkit commented Jul 7, 2020 •

edited

Loading

roberth-k commented Jul 8, 2020

roberth-k commented Jul 9, 2020

dturnbu commented Jul 15, 2020

kuperiudazn commented Jul 17, 2020

roberth-k commented Jul 19, 2020

roberth-k commented Jul 21, 2020

teamterraform commented Aug 18, 2020

t0rr3sp3dr0 commented Aug 25, 2020

bflad commented Dec 3, 2020

bflad left a comment

bflad Dec 10, 2020

bflad Dec 10, 2020

bflad Dec 10, 2020

bflad Dec 10, 2020

bflad Dec 10, 2020

bflad Dec 10, 2020

bflad Dec 10, 2020

bflad Dec 10, 2020

bflad Dec 10, 2020

bflad Dec 10, 2020

bflad commented Dec 15, 2020

roberth-k commented Dec 16, 2020

bflad commented Dec 17, 2020

ghost commented Dec 18, 2020

ghost commented Jan 16, 2021

	return fmt.Errorf("failed to create managed prefix list: %v", err)
	return fmt.Errorf("error creating EC2 Managed Prefix List: %w", err)

New resource: aws_ec2_managed_prefix_list #14068

New resource: aws_ec2_managed_prefix_list #14068

Conversation

roberth-k commented Jul 6, 2020 • edited Loading

Community Note

ewbankkit commented Jul 7, 2020 • edited Loading

roberth-k commented Jul 8, 2020

roberth-k commented Jul 9, 2020

dturnbu commented Jul 15, 2020

kuperiudazn commented Jul 17, 2020

roberth-k commented Jul 19, 2020

roberth-k commented Jul 21, 2020

teamterraform commented Aug 18, 2020

Notification of Recent and Upcoming Changes to Contributions

Terraform 0.12 Syntax

Action Required: Terraform Plugin SDK Version 2

Removal of website/aws.erb File

Upcoming Change of Git Branch Naming

Upcoming Change of GitHub Organization

t0rr3sp3dr0 commented Aug 25, 2020

bflad commented Dec 3, 2020

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Dec 15, 2020

roberth-k commented Dec 16, 2020

bflad commented Dec 17, 2020

ghost commented Dec 18, 2020

ghost commented Jan 16, 2021

roberth-k commented Jul 6, 2020 •

edited

Loading

ewbankkit commented Jul 7, 2020 •

edited

Loading