resource/aws_mq_broker: security group changes not require broker reboot or recreation #10442

albertoal · 2019-10-09T16:16:43Z

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" comments, they generate extra noise for pull request followers and do not help prioritize the request

Background:

As of Aug 30th, you can update the security groups of an AWS MQ broker without replacing the broker. Link to the source.

Changes proposed in this pull request:

aws_mq_broker.security_groups is not marked as ForceNew anymore.
Update resourceAwsMqBrokerUpdate() so it handles Security Group updates
Added rebootMarker to determine if changes require a broker reboot. Prior to this change, any update operation will trigger a reboot if apply_immediately is set to true. After this change, broker changes such as tags and security group updates are applied without a reboot.

Release note for CHANGELOG:

Security group updates on `aws_mq_broker` are applied without the broker instance being recreated or rebooted.

Output from acceptance testing (ran the entire TestAccAWSMqBroker suite):

make testacc TEST=./aws TESTARGS='-run=TestAccAWSMq'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSMq -timeout 120m
(...)
--- PASS: TestAccAWSMqConfiguration_withData (17.90s)
--- PASS: TestAccAWSMqConfiguration_basic (31.44s)
--- PASS: TestAccAWSMqConfiguration_updateTags (45.70s)
--- PASS: TestAccAWSMqBroker_basic (1074.68s)
--- PASS: TestAccAWSMqBroker_EncryptionOptions_UseAwsOwnedKey_Disabled (1136.15s)
--- PASS: TestAccAWSMqBroker_EncryptionOptions_UseAwsOwnedKey_Enabled (1142.81s)
--- PASS: TestAccAWSMqBroker_updateTags (1180.18s)
--- PASS: TestAccAWSMqBroker_EncryptionOptions_KmsKeyId (1238.39s)
--- PASS: TestAccAWSMqBroker_updateSecurityGroup (1390.94s)
--- PASS: TestAccAWSMqBroker_updateUsers (1618.90s)
--- PASS: TestAccAWSMqBroker_allFieldsCustomVpc (1722.76s)
--- PASS: TestAccAWSMqBroker_allFieldsDefaultVpc (1858.83s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1859.057s

albertoal · 2019-10-09T16:19:12Z

aws/resource_aws_mq_broker.go

 	}

 	if d.HasChange("user") {
 		o, n := d.GetChange("user")
-		err := updateAwsMqBrokerUsers(conn, d.Id(),
+		var err error


This approach is a bit brittle, but it was the only way to reliably detect user changes without refactoring the MQ Broker user functions which seems a bit out of the scope of this PR. I'd be happy to look into alternative suggestions though!

We're not familiar with an issue with d.HasChange("user") like this:

d.HasChange("user") always reports a change

Because we would expect the acceptance testing to previously have reported a difference after apply testing (ExpectNonEmptyPlan defaults to false in the testing framework terms). The only thing I can think of here would be the groups attribute inside user potentially showing as a difference not reported in the plan output but still listed as a difference in Terraform's graph when it makes it to the Update function. That would be quite a bug find! Are you sure this wasn't related the previous logic always calling RebootBroker when apply_immediately = true before the reboot variable was added?

aws/resource_aws_mq_broker_test.go

albertoal · 2019-10-09T16:21:52Z

aws/resource_aws_mq_broker_test.go

-  security_groups    = ["${aws_security_group.test.id}"]
+	security_groups    = ["${aws_security_group.test.id}"]
+
+	user {


Had to add user on testAccMqBrokerConfig_updateTags3 as the test was throwing the error "user": required field is not set - I assume this is because user is actually a required attribute: https://github.com/terraform-providers/terraform-provider-aws/blob/e94c64a/aws/resource_aws_mq_broker.go#L177

Hmmm looking on master, testAccMqBrokerConfig_updateTags3 has a user configuration block and the test has been passing in our daily acceptance testing.

https://github.com/terraform-providers/terraform-provider-aws/blob/abd539e22bd25ac33d225ba18a6f6146bb9d9821/aws/resource_aws_mq_broker_test.go#L1106-L1128

Maybe if the whitespace in this test configuration is set to spaces instead of tabs, the difference between this branch and master will be a little more clear 👍

yeah sorry my bad. My editor keeps defaulting to tabs as the code is in go but the literals are terraform which uses spaces. Will fix!

This has been fixed. It was indeed my editor. No diff once I sorted out or tests faling anymore. Thanks!

bflad

Hi @albertoal 👋 Thanks for submitting this -- overall its coming together well. Left a few initial review items below. Please reach out if you have any questions or if you do not have time to implement the feedback. Thanks!

bflad · 2019-10-12T00:38:43Z

aws/resource_aws_mq_broker_test.go

-  security_groups    = ["${aws_security_group.test.id}"]
+	security_groups    = ["${aws_security_group.test.id}"]
+
+	user {


Hmmm looking on master, testAccMqBrokerConfig_updateTags3 has a user configuration block and the test has been passing in our daily acceptance testing.

https://github.com/terraform-providers/terraform-provider-aws/blob/abd539e22bd25ac33d225ba18a6f6146bb9d9821/aws/resource_aws_mq_broker_test.go#L1106-L1128

Maybe if the whitespace in this test configuration is set to spaces instead of tabs, the difference between this branch and master will be a little more clear 👍

aws/resource_aws_mq_broker_test.go

bflad · 2019-10-12T00:40:28Z

aws/resource_aws_mq_broker.go

@@ -382,6 +381,19 @@ func resourceAwsMqBrokerRead(d *schema.ResourceData, meta interface{}) error {
 func resourceAwsMqBrokerUpdate(d *schema.ResourceData, meta interface{}) error {
 	conn := meta.(*AWSClient).mqconn

+	// rebootMarker is used to determine if changes require a broker reboot
+	rebootMarker := 0


Can you please use a boolean for this? We can probably also drop the comment if its named something like requiresReboot 😄

Suggested change

rebootMarker := 0

var rebootMarker bool

Edit: This seems like another symptom of the d.HasChange("user") behaviour - will dig into this and move back to bool. Thanks!

Fixed! Using bool, renamed to requiresReboot and added more detail in the PR thread about the changes to the user block.

bflad · 2019-10-12T00:41:32Z

aws/resource_aws_mq_broker.go

+			SecurityGroups: expandStringSet(d.Get("security_groups").(*schema.Set)),
+		})
+		if err != nil {
+			return err


Its generally helpful to provide context for operators and code maintainers when receiving an error, e.g.

Suggested change

return err

return fmt.Errorf("error updating MQ Broker (%s) security groups: %s", d.Id(), err)

Oops! Fixed - also enriched similar error returns on the other update blocks.

bflad · 2019-10-12T01:13:45Z

aws/resource_aws_mq_broker.go

 	}

 	if d.HasChange("user") {
 		o, n := d.GetChange("user")
-		err := updateAwsMqBrokerUsers(conn, d.Id(),
+		var err error


We're not familiar with an issue with d.HasChange("user") like this:

d.HasChange("user") always reports a change

Because we would expect the acceptance testing to previously have reported a difference after apply testing (ExpectNonEmptyPlan defaults to false in the testing framework terms). The only thing I can think of here would be the groups attribute inside user potentially showing as a difference not reported in the plan output but still listed as a difference in Terraform's graph when it makes it to the Update function. That would be quite a bug find! Are you sure this wasn't related the previous logic always calling RebootBroker when apply_immediately = true before the reboot variable was added?

albertoal · 2019-10-12T02:03:58Z

Hey @bflad thanks for looking into this! Your comments make a lot of sense and I will look into addressing your suggestions in the coming days. Will also dig a bit more into the user logic and report back. Cheers!

…at tf string literals with spaces

albertoal · 2019-10-12T23:09:36Z

Hi @bflad, I just addressed the minor suggestions with the latest commit. In terms of d.HasChange("user") evaluating to true, this is what I found:

Using the following terraform config as an example:

provider "aws" {
  region  = "us-west-2"
}

resource "aws_security_group" "one" {
  name = "one"
}

resource "aws_security_group" "two" {
  name = "two"
}

resource "aws_mq_configuration" "test" {
  name           = "test-config-aa"
  engine_type    = "ActiveMQ"
  engine_version = "5.15.0"

  data = <<DATA
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<broker xmlns="http://activemq.apache.org/schema/core">
</broker>
DATA
}

resource "aws_mq_broker" "example" {
  broker_name        = "example"
  apply_immediately  = true
  engine_type        = "ActiveMQ"
  engine_version     = "5.15.0"
  host_instance_type = "mq.t2.micro"
  security_groups    = ["${aws_security_group.one.id}", "${aws_security_group.two.id}"]

  configuration {
    id       = "${aws_mq_configuration.test.id}"
    revision = "${aws_mq_configuration.test.latest_revision}"
  }

  user {
    username = "Test"
    password = "TestTest9999"
  }

  tags = {
    env  = "test221"
    role = "test-role"
  }
}

Then I added some debug output on aws/resource_aws_mq_broker_test.go, inside the if statement where d.HasChange("user") gets evaluated:

if d.HasChange("user") {
    o, n := d.GetChange("user")
    log.Println("[INFO XXX ] Old:", o.(*schema.Set).List(), "New:", n.(*schema.Set).List())
    ....
}

Then I made a simple security group change to ensure the next terraform apply triggers a resourceAwsMqBrokerUpdate.

terraform plan output:

~ security_groups            = [
      "sg-07c349708a1db3561",
    - "sg-0e549389c02555d5a",
  ]
  subnet_ids                 = [
      "subnet-18c0d961",
  ]
...
(redacted - no changes)

  user {
      console_access = false
      groups         = []
      password       = (sensitive value)
      username       = "Test"
  }
}

Plan: 0 to add, 1 to change, 0 to destroy.

No changes to user{} reported in the plan, just to the security_groups

Then I saw the following when running TF_LOG=debug terraform apply -auto-approve:

Old: [map[console_access:false groups:0xc0009099a0 password:TestTest9999 username:Test]]
New: [map[console_access:false groups:0xc000909800 password:TestTest9999 username:Test]]

Notice a slight difference in the memory address of groups. I repeated the same change a few times to ensure this was consistent and observed the same behaviour:

Old: [map[console_access:false groups:0xc000a40c20 password:TestTest9999 username:Test]]
New: [map[console_access:false groups:0xc000a40a80 password:TestTest9999 username:Test]]

Digging into the actual values of groups, I added the following to the output:

oldGroupI, _ := o.(*schema.Set).List()[0].(map[string]interface{})
newGroupI, _ := n.(*schema.Set).List()[0].(map[string]interface{})
oldGroup := oldGroupI["groups"].(*schema.Set).List()
newGroup := newGroupI["groups"].(*schema.Set).List()
log.Println("[INFO XXX ] ", oldGroup, newGroup)

And the output shows that the two slices are the same, just empty ones as the user is not on any groups:

[DEBUG] plugin.terraform-provider-aws: 2019/10/12 15:41:01 [INFO XXX ]  [] []

So it seems that your assumption might be right regarding groups reporting a diff. Do you want me to look into fixing it in this PR or open a new bug? At first glance I don't see anything wrong in the users data structure but I'm not super familiar with this code so I would need to look into it a bit more. Any pointers on how to solve this would be appreciated!

I tested this behavior using the code on the latest commit of this PR 7a6b5fc but I'm happy to give it a go on master to ensure this is not something I have introduced, although I think is unlikely as all this debug output is being printed before updateAwsMqBrokerUsers() or d.Get("apply_immediately").(bool) are called where most of my changes are.

Thanks!

albertoal · 2019-10-24T20:15:26Z

Hey @bflad did you get a chance to take another look? I believe the current changes would solve the enhancement request and at this point we just need to decide if we want to open up a new bug for d.HasChange("user") always reporting a groups diff or try to fix it as part of this PR. Thanks!

bflad

Thanks so much for the updates, @albertoal 🚀 Let's treat anything with the users configuration blocks separately. 👍

--- PASS: TestAccAWSMqBroker_basic (1059.92s)
--- PASS: TestAccAWSMqBroker_EncryptionOptions_UseAwsOwnedKey_Enabled (1100.47s)
--- PASS: TestAccAWSMqBroker_EncryptionOptions_UseAwsOwnedKey_Disabled (1152.79s)
--- PASS: TestAccAWSMqBroker_updateTags (1223.38s)
--- PASS: TestAccAWSMqBroker_EncryptionOptions_KmsKeyId (1235.39s)
--- PASS: TestAccAWSMqBroker_updateSecurityGroup (1446.46s)
--- PASS: TestAccAWSMqBroker_updateUsers (1597.68s)
--- PASS: TestAccAWSMqBroker_allFieldsCustomVpc (1801.50s)
--- PASS: TestAccAWSMqBroker_allFieldsDefaultVpc (1861.13s)

albertoal · 2019-11-02T02:43:08Z

Thanks @bflad ! I'll open a new issue related to the diff on the user block and will also look into it a bit more now that this one is out of the way. Thanks again for the review

ghost · 2019-11-07T22:01:59Z

This has been released in version 2.35.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 · 2020-03-29T17:31:12Z

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!

albertoal added 4 commits October 8, 2019 10:13

enhancement: MQ broker sg updates don't recreate the broker

7047da8

Refactor resourceAwsMqBrokerUpdate to not reboot broker on sg changes

59851b6

rebootMarker cannot be boolean as user/config updates work differently

57a3720

use right hashcode for user resource attribute

e0c5bee

albertoal requested a review from a team October 9, 2019 16:16

ghost added size/L Managed by automation to categorize the size of a PR. service/mq Issues and PRs that pertain to the mq service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Oct 9, 2019

albertoal commented Oct 9, 2019

View reviewed changes

aws/resource_aws_mq_broker_test.go Show resolved Hide resolved

albertoal commented Oct 9, 2019

View reviewed changes

Fix indent and trigger GitHub job again to fix unrelated workflow error

13999d6

albertoal changed the title ~~resource/aws_mq_broker security group changes not require broker reboot or recreation~~ resource/aws_mq_broker: security group changes not require broker reboot or recreation Oct 11, 2019

bflad suggested changes Oct 12, 2019

View reviewed changes

bflad added enhancement Requests to existing resources that expand the functionality or scope. waiting-response Maintainers are waiting on response from community or contributor. labels Oct 12, 2019

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 12, 2019

Costmetic changes: Richer error messages, add requiresReboot and form…

7a6b5fc

…at tf string literals with spaces

albertoal requested a review from bflad October 17, 2019 19:23

bflad added this to the v2.35.0 milestone Nov 2, 2019

bflad approved these changes Nov 2, 2019

View reviewed changes

bflad merged commit d26e9b6 into hashicorp:master Nov 2, 2019

bflad added a commit that referenced this pull request Nov 2, 2019

Update CHANGELOG for #10442

823d8fa

ghost locked and limited conversation to collaborators Mar 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_mq_broker: security group changes not require broker reboot or recreation #10442

resource/aws_mq_broker: security group changes not require broker reboot or recreation #10442

albertoal commented Oct 9, 2019

albertoal Oct 9, 2019

bflad Oct 12, 2019

albertoal Oct 9, 2019

bflad Oct 12, 2019

albertoal Oct 12, 2019

albertoal Oct 12, 2019

bflad left a comment

bflad Oct 12, 2019

bflad Oct 12, 2019

albertoal Oct 12, 2019 •

edited

Loading

albertoal Oct 12, 2019

bflad Oct 12, 2019

albertoal Oct 12, 2019

bflad Oct 12, 2019

albertoal commented Oct 12, 2019 •

edited

Loading

albertoal commented Oct 12, 2019

albertoal commented Oct 24, 2019

bflad left a comment

albertoal commented Nov 2, 2019

ghost commented Nov 7, 2019

ghost commented Mar 29, 2020

	return err
	return fmt.Errorf("error updating MQ Broker (%s) security groups: %s", d.Id(), err)

resource/aws_mq_broker: security group changes not require broker reboot or recreation #10442

resource/aws_mq_broker: security group changes not require broker reboot or recreation #10442

Conversation

albertoal commented Oct 9, 2019

Community Note

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertoal Oct 12, 2019 • edited Loading

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

albertoal commented Oct 12, 2019 • edited Loading

albertoal commented Oct 12, 2019

albertoal commented Oct 24, 2019

bflad left a comment

Choose a reason for hiding this comment

albertoal commented Nov 2, 2019

ghost commented Nov 7, 2019

ghost commented Mar 29, 2020

albertoal Oct 12, 2019 •

edited

Loading

albertoal commented Oct 12, 2019 •

edited

Loading