resource/aws_security_group_rule: Prevent sg rule recreation when source_security_group_id has accountId prefix #11809

ktham · 2020-01-30T05:30:42Z

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

Closes #159 and #141

Release note for CHANGELOG:

resource/aws_security_group_rule: Prevent sg rule recreation when source_security_group_id has accountId prefix

Output from acceptance testing (with code change):

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

Output from acceptance test prior to code change, which reproduces #159/#141 (account id is redacted in the output):

make testacc TEST=./aws TESTARGS='-run=TestAccAWSSecurityGroupRule_Ingress_Source_With_Account_Id'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSecurityGroupRule_Ingress_Source_With_Account_Id -timeout 120m
=== RUN   TestAccAWSSecurityGroupRule_Ingress_Source_With_Account_Id
=== PAUSE TestAccAWSSecurityGroupRule_Ingress_Source_With_Account_Id
=== CONT  TestAccAWSSecurityGroupRule_Ingress_Source_With_Account_Id
--- FAIL: TestAccAWSSecurityGroupRule_Ingress_Source_With_Account_Id (30.98s)
    testing.go:640: Step 0 error: After applying this step and refreshing, the plan was not empty:

        DIFF:

        DESTROY/CREATE: aws_security_group_rule.allow_self
          cidr_blocks.#:            "0" => ""
          description:              "" => ""
          from_port:                "0" => "0"
          id:                       "sgrule-2081816794" => "<computed>"
          ipv6_cidr_blocks.#:       "0" => ""
          prefix_list_ids.#:        "0" => ""
          protocol:                 "-1" => "-1"
          security_group_id:        "sg-0752efed0ad3f0d34" => "sg-0752efed0ad3f0d34"
          self:                     "false" => "false"
          source_security_group_id: "sg-0752efed0ad3f0d34" => "[REDACTED]/sg-0752efed0ad3f0d34" (forces new resource)
          to_port:                  "0" => "0"
          type:                     "ingress" => "ingress"

…ource sg id is specified with account id prefix

ketzacoatl · 2020-01-30T13:13:46Z

Is there an easy way for someone to plug this update into a stack for testing?

I have only used upstream/stable/released versions of Terraform and it's providers, but I'd be happy to assist with testing if that's useful.

ktham · 2020-01-30T16:30:50Z

Is there an easy way for someone to plug this update into a stack for testing?

What you can do, is make a local build of the provider using my branch and place it in your local terraform plugins directory at YOUR_WORKSPACE_DIR/.terraform.d/plugins/linux_amd64/terraform-provider-aws_vX.Y.Z (or YOUR_WORKSPACE_DIR/.terraform.d/plugins/darwin_amd64/terraform-provider-aws_vX.Y.Z for Macs). You can name the version however you like.

Instructions for making a local build is here: https://github.com/terraform-providers/terraform-provider-aws#developing-the-provider

Note: If you're not running the latest version of the provider, you may want to just cherry pick my changes and apply them on top of the version of the provider you are using.

ktham · 2020-02-24T17:45:45Z

Sorry to mention you @bflad - since I saw you participated in the referenced issues - would it be possible for you to take a quick look at this PR and share what you think? Hopefully this is small enough such that it doesn't take too much of your time

We've been experiencing slight downtime each time we apply Terraform because of this issue and am hoping in the next provider release that it can be addressed.

…vider-aws into hashicorpgh-159

anGie44 · 2020-05-08T21:58:02Z

aws/resource_aws_security_group_rule.go

@@ -759,7 +759,19 @@ func setFromIPPerm(d *schema.ResourceData, sg *ec2.SecurityGroup, rule *ec2.IpPe
 		s := rule.UserIdGroupPairs[0]

 		if isVPC {
-			d.Set("source_security_group_id", s.GroupId)
+			if existingSourceSgId, ok := d.GetOk("source_security_group_id"); ok {


looks good 👍
as an alternative, I believe you could even use the value of the s.UserId to handle the if/else as it'll be an empty string for security groups not tied to a specific accountId prefix

Suggested change

if existingSourceSgId, ok := d.GetOk("source_security_group_id"); ok {

if userId := aws.StringValue(s.UserId); len(userId) > 0 {

d.Set("source_security_group_id", fmt.Sprintf("%s/%s", userId, aws.StringValue(s.GroupId)))

} else {

d.Set("source_security_group_id", s.GroupId)

}

Thanks for the review!

as it'll be an empty string for security groups not tied to a specific accountId prefix

I believe that is not true. I believe s.UserId always has a value defined as the AWS API always returns a value for it whether or not we have a accoundId prefix in the source_security_group_id string

😅 Using d.Get() or d.GetOk() to influence d.Set() calls is generally discouraged since there are different semantics between resources created versus imported, however in this case, we don't have any options other than maybe #4454 (a DiffSuppressFunc) since the API seems to be very ambiguous about how it requires canonicalization. I'm hesitantly thinking this change to base the decision of d.Set() value is fine in principle since it should keep the existing resource behavior for previous configurations.

anGie44

hi @ktham, thank you for this PR and apologies for the wait!
the changes LGTM, only a small suggestion 👍

Output of acceptance tests:

--- PASS: TestAccAWSSecurityGroup_*
--- PASS: TestIpPermissionIDHash*
--- PASS: TestAccAWSSecurityGroupRule*

bflad

Thanks for submitting this, @ktham 👍 Please see the below and reach out with any questions.

bflad · 2020-05-12T00:32:05Z

aws/resource_aws_security_group_rule.go

@@ -759,7 +759,19 @@ func setFromIPPerm(d *schema.ResourceData, sg *ec2.SecurityGroup, rule *ec2.IpPe
 		s := rule.UserIdGroupPairs[0]

 		if isVPC {
-			d.Set("source_security_group_id", s.GroupId)
+			if existingSourceSgId, ok := d.GetOk("source_security_group_id"); ok {


😅 Using d.Get() or d.GetOk() to influence d.Set() calls is generally discouraged since there are different semantics between resources created versus imported, however in this case, we don't have any options other than maybe #4454 (a DiffSuppressFunc) since the API seems to be very ambiguous about how it requires canonicalization. I'm hesitantly thinking this change to base the decision of d.Set() value is fine in principle since it should keep the existing resource behavior for previous configurations.

bflad · 2020-05-12T00:36:48Z

aws/resource_aws_security_group_rule.go

+
+				if hasAccountIdPrefix {
+					// then ensure on refresh that we prefix the account id
+					d.Set("source_security_group_id", *s.UserId+"/"+*s.GroupId)


To prevent potential panics (as recommended above), we should use the AWS Go SDK conversion functions, in case the new requirement of s.UserId does happen to be nil in some environments:

Suggested change

d.Set("source_security_group_id", *s.UserId+"/"+*s.GroupId)

d.Set("source_security_group_id", fmt.Sprintf("%s/%s", aws.StringValue(s.UserId), aws.StringValue(s.GroupId))

Or the conditional above should check && s.UserId != nil

bflad · 2020-05-12T00:40:35Z

aws/resource_aws_security_group_rule_test.go

+			// Ensure plan shows no difference after state is refreshed
+			{
+				Config:             testAccAWSSecurityGroupRule_Ingress_Source_with_AccountId(rInt),
+				PlanOnly:           true,
+				ExpectNonEmptyPlan: false,
+			},


This is extraneous as the acceptance testing framework automatically checks a refresh plan for differences after applying a Config 👍

Suggested change

// Ensure plan shows no difference after state is refreshed

{

Config: testAccAWSSecurityGroupRule_Ingress_Source_with_AccountId(rInt),

PlanOnly: true,

ExpectNonEmptyPlan: false,

},

ktham · 2020-05-12T03:16:27Z

Thanks @bflad ! I've updated the PR

bflad

Let's give this a go -- thank you @ktham for submitting this and @anGie44 for the initial review!

Output from acceptance testing:

--- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts (30.64s)
--- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts_NonZeroPorts (27.59s)
--- PASS: TestAccAWSSecurityGroupRule_Egress (26.17s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription (20.02s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription_updates (40.24s)
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidCIDR (5.11s)
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidTypeError (3.78s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Classic (23.37s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Ipv6 (45.69s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Protocol (40.01s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Source_With_Account_Id (32.57s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_VPC (19.53s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription (19.74s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription_updates (53.31s)
--- PASS: TestAccAWSSecurityGroupRule_Issue5310 (35.05s)
--- PASS: TestAccAWSSecurityGroupRule_MultiDescription (48.81s)
--- PASS: TestAccAWSSecurityGroupRule_MultiIngress (39.92s)
--- PASS: TestAccAWSSecurityGroupRule_MultipleRuleSearching_AllProtocolCrash (16.15s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_basic (39.64s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_Source (29.01s)
--- PASS: TestAccAWSSecurityGroupRule_PrefixListEgress (58.93s)
--- PASS: TestAccAWSSecurityGroupRule_Race (98.35s)
--- PASS: TestAccAWSSecurityGroupRule_SelfReference (36.32s)
--- PASS: TestAccAWSSecurityGroupRule_SelfSource (35.30s)

ghost · 2020-05-15T01:04:02Z

This has been released in version 2.62.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!

jnonino · 2020-06-08T11:51:33Z

Hi @ktham , I'm using version 2.65 of the AWS provider and the issue still present.

10:37:22  * provider.aws: version = "~> 2.65"
10:37:22  * provider.random: version = "~> 2.2"
10:37:22  * provider.template: version = "~> 2.1"

10:37:33    # module.<MY_MODULE>.aws_security_group_rule.<MY_RULE>[1] must be replaced
10:37:33  -/+ resource "aws_security_group_rule" "rule" {
10:37:33        - cidr_blocks              = [] -> null
10:37:33          description              = "ACCOUNT/sg-ID"
10:37:33          from_port                = 80
10:37:33        ~ id                       = "sgrule-ID" -> (known after apply)
10:37:33        - ipv6_cidr_blocks         = [] -> null
10:37:33        - prefix_list_ids          = [] -> null
10:37:33          protocol                 = "tcp"
10:37:33          security_group_id        = "sg-ID"
10:37:33          self                     = false
10:37:33        ~ source_security_group_id = "sg-ID" -> "ACCOUNT/sg-ID" # forces replacement
10:37:33          to_port                  = 80
10:37:33          type                     = "ingress"
10:37:33      }

ktham · 2020-06-08T23:06:08Z

Hi @jnonino , is that problem still repeatedly happening for you? Do note, that, right after upgrading to the provider version that contains the fix, depending on what your workaround beforehand was (e.g. if you had a lifecycle ignore rule on source_security_group_id) there may still be 1 more TF plan that will have the sg rule re-creation because the source_security_group_id got modified to not include "ACCOUNT/" when using a previous version of the provider.

jnonino · 2020-06-09T08:13:18Z

Thanks, I applied the change and seems the state was updated and it working as expected now. Thanks

ghost · 2020-06-13T17:10:01Z

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!

resource/aws_security_group_rule: Prevent resource replacement when s…

01d384b

…ource sg id is specified with account id prefix

ktham requested a review from a team January 30, 2020 05:30

ktham mentioned this pull request Jan 30, 2020

aws_security_group_rule doesn't seem to support security groups from multiple aws accounts. #159

Closed

ktham changed the title ~~resource/aws_security_group_rule: Prevent sg rule recreation accountId prefix is specified in source_security_group_id~~ resource/aws_security_group_rule: Prevent sg rule recreation when source_security_group_id has accountId prefix Jan 30, 2020

ktham added 2 commits February 25, 2020 16:53

Merge branch 'master' of github.com:terraform-providers/terraform-pro…

06bfec1

…vider-aws into hashicorpgh-159

Fix

ab807d6

anGie44 self-assigned this May 7, 2020

anGie44 removed the needs-triage Waiting for first response or review from a maintainer. label May 7, 2020

anGie44 reviewed May 8, 2020

View reviewed changes

anGie44 requested a review from bflad May 11, 2020 14:51

bflad suggested changes May 12, 2020

View reviewed changes

ktham added 2 commits May 11, 2020 18:23

Remove extraneous lines in acc test

db1de8d

Add nil check for s.UserId parameter

e9b33d4

bflad self-requested a review May 12, 2020 02:08

bflad added the bug Addresses a defect in current functionality. label May 12, 2020

bflad added this to the v2.62.0 milestone May 12, 2020

bflad approved these changes May 12, 2020

View reviewed changes

bflad mentioned this pull request May 12, 2020

r/aws_security_group_rule: prevent flapping of account prefixed references #4454

Closed

anGie44 merged commit cd05028 into hashicorp:master May 14, 2020

anGie44 added a commit that referenced this pull request May 14, 2020

Update CHANGELOG for #11809

7020ac0

ktham deleted the gh-159 branch May 15, 2020 04:22

ktham mentioned this pull request May 16, 2020

Fix aws_security_group_rule: description overwritten on state refresh #13364

Merged

ghost locked and limited conversation to collaborators Jun 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_security_group_rule: Prevent sg rule recreation when source_security_group_id has accountId prefix #11809

resource/aws_security_group_rule: Prevent sg rule recreation when source_security_group_id has accountId prefix #11809

ktham commented Jan 30, 2020 •

edited

Loading

ketzacoatl commented Jan 30, 2020

ktham commented Jan 30, 2020 •

edited

Loading

ktham commented Feb 24, 2020

anGie44 May 8, 2020 •

edited

Loading

ktham May 8, 2020 •

edited

Loading

bflad May 12, 2020

anGie44 left a comment

bflad left a comment

bflad May 12, 2020

bflad May 12, 2020 •

edited

Loading

bflad May 12, 2020

ktham commented May 12, 2020

bflad left a comment

ghost commented May 15, 2020

jnonino commented Jun 8, 2020 •

edited

Loading

ktham commented Jun 8, 2020 •

edited

Loading

jnonino commented Jun 9, 2020

ghost commented Jun 13, 2020

-			if existingSourceSgId, ok := d.GetOk("source_security_group_id"); ok {
+			if userId := aws.StringValue(s.UserId); len(userId) > 0 {
+				d.Set("source_security_group_id", fmt.Sprintf("%s/%s", userId, aws.StringValue(s.GroupId)))
+			} else {
+				d.Set("source_security_group_id", s.GroupId)
+			}

	d.Set("source_security_group_id", s.UserId+"/"+s.GroupId)
	d.Set("source_security_group_id", fmt.Sprintf("%s/%s", aws.StringValue(s.UserId), aws.StringValue(s.GroupId))

resource/aws_security_group_rule: Prevent sg rule recreation when source_security_group_id has accountId prefix #11809

resource/aws_security_group_rule: Prevent sg rule recreation when source_security_group_id has accountId prefix #11809

Conversation

ktham commented Jan 30, 2020 • edited Loading

Community Note

ketzacoatl commented Jan 30, 2020

ktham commented Jan 30, 2020 • edited Loading

ktham commented Feb 24, 2020

anGie44 May 8, 2020 • edited Loading

Choose a reason for hiding this comment

ktham May 8, 2020 • edited Loading

Choose a reason for hiding this comment

bflad May 12, 2020

Choose a reason for hiding this comment

anGie44 left a comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

bflad May 12, 2020

Choose a reason for hiding this comment

bflad May 12, 2020 • edited Loading

Choose a reason for hiding this comment

bflad May 12, 2020

Choose a reason for hiding this comment

ktham commented May 12, 2020

bflad left a comment

Choose a reason for hiding this comment

ghost commented May 15, 2020

jnonino commented Jun 8, 2020 • edited Loading

ktham commented Jun 8, 2020 • edited Loading

jnonino commented Jun 9, 2020

ghost commented Jun 13, 2020

ktham commented Jan 30, 2020 •

edited

Loading

ktham commented Jan 30, 2020 •

edited

Loading

anGie44 May 8, 2020 •

edited

Loading

ktham May 8, 2020 •

edited

Loading

bflad May 12, 2020 •

edited

Loading

jnonino commented Jun 8, 2020 •

edited

Loading

ktham commented Jun 8, 2020 •

edited

Loading