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.

Sign up for GitHub

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

Jump to bottom

resource/aws_security_group_rule: Add import functionality for security group rules #6027

Merged
merged 4 commits into from
Oct 8, 2018
Merged

resource/aws_security_group_rule: Add import functionality for security group rules #6027

merged 4 commits into from
Oct 8, 2018

Conversation

YakDriver
Copy link
Member

@YakDriver YakDriver commented Sep 29, 2018 •
edited
Loading

Fixes #2895

Changes proposed in this pull request:

  • Add functionality to the existing resource aws_security_group_rule to allow for importing rules
  • Add importing and import verification to the acceptance tests
  • Add import documentation

How it works

Import string/ID

The import string/ID can be complex but manageable. It follows this format:

SECURITYGROUPID_TYPE_PROTOCOL_FROMPORT_TOPORT_SOURCE[_SOURCE]*

These are valid examples of import strings:

  • sg-09a093729ef9382a6_ingress_tcp_8000_8000_10.0.3.0/24
  • sg-09a093729ef9382a6_ingress_92_0_65535_10.0.3.0/24_10.0.4.0/24 (multiple CIDR blocks)
  • sg-09a093729ef9382a6_egress_tcp_100_8000_10.0.3.0/24 (port range)
  • sg-09a093729ef9382a6_egress_tcp_8000_8000_pl-34800000 (prefix list id destination)
  • sg-09a093729ef9382a6_ingress_all_0_65535_sg-08123412342323
  • sg-09a093729ef9382a6_ingress_tcp_100_121_10.1.0.0/16_2001:db8::/48_10.2.0.0/16_2002:db8::/48 (mixture of IPv4 and IPv6 CIDRs)
  • sg-09a093729ef9382a6_ingress_tcp_0_65535_self_2001:db8::/48 (self + IPv6 CIDR block)
  • sg-09a093729ef9382a6_ingress_92_0_65535_self (if protocol is not TCP/UDP/ICMP/ALL then it must be a protocol number)

All tricky functionality

This captures all security group rule functionality including the tricky bits:

  • as many CIDR blocks as someone wants to type
  • self
  • all protocols
  • all ports
  • port ranges
  • multiple IPv4/6 CIDR blocks, prefix list IDs
  • source/destination security groups

Flexible: Config / import ID don't have to match

AWS is not particular about needing to modify an entire rule. Thus, you can partially import (i.e., some CIDR blocks but not others) and then update those within Terraform. AWS gracefully adjusts rules accordingly.

Acceptance tests:

I added imports/state verification to 17 of 20 existing tests. Import didn't make sense for the other 3 -- 2 test error conditions and 1 tests a race condition.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSSecurityGroupRule_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSSecurityGroupRule_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSSecurityGroupRule_Ingress_VPC
--- PASS: TestAccAWSSecurityGroupRule_Ingress_VPC (21.20s)
=== RUN   TestAccAWSSecurityGroupRule_Ingress_Protocol
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Protocol (35.63s)
=== RUN   TestAccAWSSecurityGroupRule_Ingress_Ipv6
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Ipv6 (35.64s)
=== RUN   TestAccAWSSecurityGroupRule_Ingress_Classic
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Classic (20.46s)
=== RUN   TestAccAWSSecurityGroupRule_MultiIngress
--- PASS: TestAccAWSSecurityGroupRule_MultiIngress (23.22s)
=== RUN   TestAccAWSSecurityGroupRule_Egress
--- PASS: TestAccAWSSecurityGroupRule_Egress (18.58s)
=== RUN   TestAccAWSSecurityGroupRule_SelfReference
--- PASS: TestAccAWSSecurityGroupRule_SelfReference (35.63s)
=== RUN   TestAccAWSSecurityGroupRule_ExpectInvalidTypeError
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidTypeError (0.85s)
=== RUN   TestAccAWSSecurityGroupRule_ExpectInvalidCIDR
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidCIDR (0.98s)
=== RUN   TestAccAWSSecurityGroupRule_PartialMatching_basic
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_basic (41.64s)
=== RUN   TestAccAWSSecurityGroupRule_PartialMatching_Source
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_Source (37.77s)
=== RUN   TestAccAWSSecurityGroupRule_Issue5310
--- PASS: TestAccAWSSecurityGroupRule_Issue5310 (18.91s)
=== RUN   TestAccAWSSecurityGroupRule_Race
--- PASS: TestAccAWSSecurityGroupRule_Race (318.46s)
=== RUN   TestAccAWSSecurityGroupRule_SelfSource
--- PASS: TestAccAWSSecurityGroupRule_SelfSource (35.23s)
=== RUN   TestAccAWSSecurityGroupRule_PrefixListEgress
--- PASS: TestAccAWSSecurityGroupRule_PrefixListEgress (50.37s)
=== RUN   TestAccAWSSecurityGroupRule_IngressDescription
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription (21.09s)
=== RUN   TestAccAWSSecurityGroupRule_EgressDescription
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription (20.27s)
=== RUN   TestAccAWSSecurityGroupRule_IngressDescription_updates
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription_updates (30.31s)
=== RUN   TestAccAWSSecurityGroupRule_EgressDescription_updates
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription_updates (29.49s)
=== RUN   TestAccAWSSecurityGroupRule_MultiDescription
--- PASS: TestAccAWSSecurityGroupRule_MultiDescription (87.41s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	866.721s

@ghost ghost added size/XXL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Sep 29, 2018
@YakDriver YakDriver mentioned this pull request Sep 29, 2018
Closed
@ghost ghost added the documentation Introduces or discusses updates to documentation. label Oct 1, 2018
@YakDriver
Copy link
Member Author

@bflad code, acceptance tests, docs ready for review & adjust!

@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. labels Oct 1, 2018
@YakDriver YakDriver mentioned this pull request Oct 1, 2018
Closed
@bflad bflad mentioned this pull request Oct 2, 2018
Closed
bflad
bflad reviewed Oct 2, 2018
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

This is going to be huge help for a lot of people, thanks @YakDriver! Sorry for the drive-by comments, but I left some initial thoughts below. What do you think?

aws/resource_aws_security_group_rule.go Show resolved Hide resolved
aws/resource_aws_security_group_rule.go Outdated Show resolved Hide resolved
website/docs/r/security_group_rule.html.markdown Outdated Show resolved Hide resolved
@YakDriver
Copy link
Member Author

@bflad I made the changes you requested, added more examples to the docs, updated the examples above in this PR, and have re-run the acceptance tests:

$ make testacc TESTARGS='-run=TestAccAWSSecurityGroupRule_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSSecurityGroupRule_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSSecurityGroupRule_Ingress_VPC
--- PASS: TestAccAWSSecurityGroupRule_Ingress_VPC (21.82s)
=== RUN   TestAccAWSSecurityGroupRule_Ingress_Protocol
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Protocol (35.72s)
=== RUN   TestAccAWSSecurityGroupRule_Ingress_Ipv6
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Ipv6 (34.94s)
=== RUN   TestAccAWSSecurityGroupRule_Ingress_Classic
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Classic (20.69s)
=== RUN   TestAccAWSSecurityGroupRule_MultiIngress
--- PASS: TestAccAWSSecurityGroupRule_MultiIngress (23.07s)
=== RUN   TestAccAWSSecurityGroupRule_Egress
--- PASS: TestAccAWSSecurityGroupRule_Egress (20.14s)
=== RUN   TestAccAWSSecurityGroupRule_SelfReference
--- PASS: TestAccAWSSecurityGroupRule_SelfReference (36.87s)
=== RUN   TestAccAWSSecurityGroupRule_ExpectInvalidTypeError
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidTypeError (0.92s)
=== RUN   TestAccAWSSecurityGroupRule_ExpectInvalidCIDR
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidCIDR (0.87s)
=== RUN   TestAccAWSSecurityGroupRule_PartialMatching_basic
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_basic (42.00s)
=== RUN   TestAccAWSSecurityGroupRule_PartialMatching_Source
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_Source (41.27s)
=== RUN   TestAccAWSSecurityGroupRule_Issue5310
--- PASS: TestAccAWSSecurityGroupRule_Issue5310 (20.27s)
=== RUN   TestAccAWSSecurityGroupRule_Race
--- PASS: TestAccAWSSecurityGroupRule_Race (323.01s)
=== RUN   TestAccAWSSecurityGroupRule_SelfSource
--- PASS: TestAccAWSSecurityGroupRule_SelfSource (35.96s)
=== RUN   TestAccAWSSecurityGroupRule_PrefixListEgress
--- PASS: TestAccAWSSecurityGroupRule_PrefixListEgress (48.83s)
=== RUN   TestAccAWSSecurityGroupRule_IngressDescription
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription (20.90s)
=== RUN   TestAccAWSSecurityGroupRule_EgressDescription
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription (20.36s)
=== RUN   TestAccAWSSecurityGroupRule_IngressDescription_updates
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription_updates (29.80s)
=== RUN   TestAccAWSSecurityGroupRule_EgressDescription_updates
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription_updates (29.29s)
=== RUN   TestAccAWSSecurityGroupRule_MultiDescription
--- PASS: TestAccAWSSecurityGroupRule_MultiDescription (126.77s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	933.525s

bflad
bflad approved these changes Oct 8, 2018
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Awesome work, @YakDriver! Let's get this in (with one minor simplification on merge) 🚀

--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidTypeError (1.96s)
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidCIDR (2.32s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription (17.19s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Classic (20.66s)
--- PASS: TestAccAWSSecurityGroupRule_Egress (23.80s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription_updates (24.18s)
--- PASS: TestAccAWSSecurityGroupRule_Issue5310 (26.44s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Ipv6 (28.50s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription (31.30s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_VPC (31.40s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription_updates (36.44s)
--- PASS: TestAccAWSSecurityGroupRule_SelfSource (36.49s)
--- PASS: TestAccAWSSecurityGroupRule_SelfReference (39.08s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Protocol (40.50s)
--- PASS: TestAccAWSSecurityGroupRule_PrefixListEgress (40.64s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_Source (41.02s)
--- PASS: TestAccAWSSecurityGroupRule_MultiIngress (41.96s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_basic (44.00s)
--- PASS: TestAccAWSSecurityGroupRule_MultiDescription (53.58s)
--- PASS: TestAccAWSSecurityGroupRule_Race (432.57s)

aws/resource_aws_security_group_rule.go Show resolved Hide resolved
@bflad bflad added this to the v1.40.0 milestone Oct 8, 2018
@bflad bflad merged commit cb6ec49 into hashicorp:master Oct 8, 2018
bflad added a commit that referenced this pull request Oct 8, 2018
@bflad
Update CHANGELOG for #6027
4d7d210
@bflad
Copy link
Contributor

bflad commented Oct 10, 2018

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

This was referenced Oct 22, 2018
Closed
Merged
@ghost
Copy link

ghost commented Apr 2, 2020

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!

@ghost ghost locked and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. size/XXL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v1.40.0
Development

Successfully merging this pull request may close these issues.

Can't import security_group_rule
2 participants
@YakDriver @bflad