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: Properly handle updating description when protocol is -1/ALL #6407

Merged
merged 2 commits into from
Nov 9, 2018
Merged

resource/aws_security_group_rule: Properly handle updating description when protocol is -1/ALL #6407

merged 2 commits into from
Nov 9, 2018

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Nov 9, 2018

Fixes #1920

Previously:

--- FAIL: TestAccAWSSecurityGroupRule_Description_AllPorts (21.74s)
    testing.go:538: Step 2 error: Error applying: 1 error occurred:
        	* aws_security_group_rule.test: 1 error occurred:
        	* aws_security_group_rule.test: Error updating security group sg-0b6f5a54297754be3 rule description: InvalidParameterValue: When protocol is ALL, you cannot specify from-port.

Output from acceptance testing:

--- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts (36.84s)
--- PASS: TestAccAWSSecurityGroupRule_Egress (21.08s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription (21.13s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription_updates (33.23s)
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidCIDR (1.77s)
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidTypeError (1.72s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Classic (20.29s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Ipv6 (42.93s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Protocol (41.24s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_VPC (20.04s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription (20.98s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription_updates (35.99s)
--- PASS: TestAccAWSSecurityGroupRule_Issue5310 (20.12s)
--- PASS: TestAccAWSSecurityGroupRule_MultiDescription (82.07s)
--- PASS: TestAccAWSSecurityGroupRule_MultiIngress (22.57s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_basic (47.79s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_Source (44.23s)
--- PASS: TestAccAWSSecurityGroupRule_PrefixListEgress (51.97s)
--- PASS: TestAccAWSSecurityGroupRule_Race (274.72s)
--- PASS: TestAccAWSSecurityGroupRule_SelfReference (40.94s)
--- PASS: TestAccAWSSecurityGroupRule_SelfSource (41.11s)

@bflad
resource/aws_security_group_rule: Properly handle updating descriptio…
524d6e8 
…n when protocol is -1/ALL

Previously:

```
--- FAIL: TestAccAWSSecurityGroupRule_Description_AllPorts (21.74s)
    testing.go:538: Step 2 error: Error applying: 1 error occurred:
        	* aws_security_group_rule.test: 1 error occurred:
        	* aws_security_group_rule.test: Error updating security group sg-0b6f5a54297754be3 rule description: InvalidParameterValue: When protocol is ALL, you cannot specify from-port.
```

Output from acceptance testing:

```
--- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts (36.84s)
--- PASS: TestAccAWSSecurityGroupRule_Egress (21.08s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription (21.13s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription_updates (33.23s)
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidCIDR (1.77s)
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidTypeError (1.72s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Classic (20.29s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Ipv6 (42.93s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Protocol (41.24s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_VPC (20.04s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription (20.98s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription_updates (35.99s)
--- PASS: TestAccAWSSecurityGroupRule_Issue5310 (20.12s)
--- PASS: TestAccAWSSecurityGroupRule_MultiDescription (82.07s)
--- PASS: TestAccAWSSecurityGroupRule_MultiIngress (22.57s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_basic (47.79s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_Source (44.23s)
--- PASS: TestAccAWSSecurityGroupRule_PrefixListEgress (51.97s)
--- PASS: TestAccAWSSecurityGroupRule_Race (274.72s)
--- PASS: TestAccAWSSecurityGroupRule_SelfReference (40.94s)
--- PASS: TestAccAWSSecurityGroupRule_SelfSource (41.11s)
```
@bflad bflad added bug Addresses a defect in current functionality. service/ec2 Issues and PRs that pertain to the ec2 service. labels Nov 9, 2018
@bflad bflad added this to the v1.43.1 milestone Nov 9, 2018
@bflad bflad requested a review from a team November 9, 2018 00:26
@ghost ghost added size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Nov 9, 2018
@bflad bflad mentioned this pull request Nov 9, 2018
Closed
tombuildsstuff
tombuildsstuff approved these changes Nov 9, 2018
View reviewed changes
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

one minor 🤔 but this otherwise LGTM 👍

aws/resource_aws_security_group_rule_test.go
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSSecurityGroupRuleExists(securityGroupResourceName, &group),
testAccCheckAWSSecurityGroupRuleAttributes(resourceName, &group, &rule2, "ingress"),
resource.TestCheckResourceAttr(resourceName, "description", "description2"),
Copy link
Contributor

Choose a reason for hiding this comment

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

do we also want to ensure the from_port / to_port values are set to empty values in the state on the way back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added these:

resource.TestCheckResourceAttr(resourceName, "from_port", "0"),
resource.TestCheckResourceAttr(resourceName, "protocol", "-1"),
resource.TestCheckResourceAttr(resourceName, "to_port", "0"),

Still passes. 😄

--- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts (25.78s)

@bflad
tests/resource/aws_security_group_rule: Add from_port/protocol/to_por…
6ffadc9 
…t attribute checks in TestAccAWSSecurityGroupRule_Description_AllPorts

```
--- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts (25.78s)
```
@bflad bflad merged commit fb4e02b into master Nov 9, 2018
@bflad bflad deleted the b-aws_security_rule_rule-all-update-description branch November 9, 2018 18:59
bflad added a commit that referenced this pull request Nov 9, 2018
@bflad
Update CHANGELOG for #6407
ecc1c85
@bflad
Copy link
Contributor Author

bflad commented Nov 9, 2018

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

@analytically
Copy link

This change keeps flagging my security groups as needing to be recreated:

-/+ module.nomad_sg.module.sg.aws_security_group_rule.ingress_with_self (new resource required)
      id:                       "sgrule-3623521784" => <computed> (forces new resource)
      description:              "Ingress Rule" => "Ingress Rule"
      from_port:                "0" => "-1" (forces new resource)
      protocol:                 "-1" => "-1"
      security_group_id:        "sg-052a15203f66daa75" => "sg-052a15203f66daa75"
      self:                     "true" => "true"
      source_security_group_id: "sg-052a15203f66daa75" => <computed>
      to_port:                  "0" => "-1" (forces new resource)
      type:                     "ingress" => "ingress"

At every apply.

@bflad bflad mentioned this pull request Nov 10, 2018
Merged
@bflad
Copy link
Contributor Author

bflad commented Nov 10, 2018

Thanks for the heads up, @analytically. I have submitted #6423 to fix that regression, which was not previously covered by an acceptance test.

@analytically
Copy link

Awesome, thanks!!

@mdlavin mdlavin mentioned this pull request Nov 21, 2018
Closed
@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

@tombuildsstuff tombuildsstuff tombuildsstuff approved these changes

Assignees
No one assigned
Labels
bug Addresses a defect in current functionality. service/ec2 Issues and PRs that pertain to the ec2 service. size/M 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.43.1
Development

Successfully merging this pull request may close these issues.

aws_security_group_rule: InvalidParameterValue: When protocol is ALL, you cannot specify from-port.
3 participants
@bflad @analytically @tombuildsstuff