-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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.
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
resource/aws_mq_broker: security group changes not require broker reboot or recreation #10442
Conversation
} | ||
|
||
if d.HasChange("user") { | ||
o, n := d.GetChange("user") | ||
err := updateAwsMqBrokerUsers(conn, d.Id(), | ||
var err error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
security_groups = ["${aws_security_group.test.id}"] | ||
security_groups = ["${aws_security_group.test.id}"] | ||
|
||
user { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm looking on master, testAccMqBrokerConfig_updateTags3
has a user
configuration block and the test has been passing in our daily acceptance testing.
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 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been fixed. It was indeed my editor. No diff once I sorted out or tests faling anymore. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
aws/resource_aws_mq_broker_test.go
Outdated
security_groups = ["${aws_security_group.test.id}"] | ||
security_groups = ["${aws_security_group.test.id}"] | ||
|
||
user { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm looking on master, testAccMqBrokerConfig_updateTags3
has a user
configuration block and the test has been passing in our daily acceptance testing.
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.go
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please use a boolean for this? We can probably also drop the comment if its named something like requiresReboot
😄
rebootMarker := 0 | |
var rebootMarker bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: This seems like another symptom of the d.HasChange("user")
behaviour - will dig into this and move back to bool. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! Using bool
, renamed to requiresReboot
and added more detail in the PR thread about the changes to the user block.
aws/resource_aws_mq_broker.go
Outdated
SecurityGroups: expandStringSet(d.Get("security_groups").(*schema.Set)), | ||
}) | ||
if err != nil { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its generally helpful to provide context for operators and code maintainers when receiving an error, e.g.
return err | |
return fmt.Errorf("error updating MQ Broker (%s) security groups: %s", d.Id(), err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Fixed - also enriched similar error returns on the other update blocks.
} | ||
|
||
if d.HasChange("user") { | ||
o, n := d.GetChange("user") | ||
err := updateAwsMqBrokerUsers(conn, d.Id(), | ||
var err error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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
Hi @bflad, I just addressed the minor suggestions with the latest commit. In terms of Using the following terraform config as an example:
Then I added some debug output on
Then I made a simple security group change to ensure the next
No changes to Then I saw the following when running
Notice a slight difference in the memory address of
Digging into the actual values of groups, I added the following to the output:
And the output shows that the two slices are the same, just empty ones as the user is not on any groups:
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 Thanks! |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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 |
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! |
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! |
Community Note
Closes #10209
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 asForceNew
anymore.resourceAwsMqBrokerUpdate()
so it handles Security Group updatesrebootMarker
to determine if changes require a broker reboot. Prior to this change, any update operation will trigger a reboot ifapply_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:
Output from acceptance testing (ran the entire
TestAccAWSMqBroker
suite):