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

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

Merged
merged 6 commits into from
Nov 2, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7047da8
enhancement: MQ broker sg updates don't recreate the broker
albertoal Oct 8, 2019
59851b6
Refactor resourceAwsMqBrokerUpdate to not reboot broker on sg changes
albertoal Oct 9, 2019
57a3720
rebootMarker cannot be boolean as user/config updates work differently
albertoal Oct 9, 2019
e0c5bee
use right hashcode for user resource attribute
albertoal Oct 9, 2019
13999d6
Fix indent and trigger GitHub job again to fix unrelated workflow error
albertoal Oct 11, 2019
7a6b5fc
Costmetic changes: Richer error messages, add requiresReboot and form…
albertoal Oct 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 38 additions & 12 deletions aws/resource_aws_mq_broker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,6 @@ func resourceAwsMqBroker() *schema.Resource {
Type: schema.TypeSet,
Elem: &schema.Schema{Type: schema.TypeString},
Required: true,
ForceNew: true,
},
"subnet_ids": {
Type: schema.TypeSet,
Expand Down Expand Up @@ -382,32 +381,53 @@ func resourceAwsMqBrokerRead(d *schema.ResourceData, meta interface{}) error {
func resourceAwsMqBrokerUpdate(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).mqconn

requiresReboot := false

if d.HasChange("security_groups") {
_, err := conn.UpdateBroker(&mq.UpdateBrokerRequest{
BrokerId: aws.String(d.Id()),
SecurityGroups: expandStringSet(d.Get("security_groups").(*schema.Set)),
})
if err != nil {
return fmt.Errorf("error updating MQ Broker (%s) security groups: %s", d.Id(), err)
}
}

if d.HasChange("configuration") || d.HasChange("logs") {
_, err := conn.UpdateBroker(&mq.UpdateBrokerRequest{
BrokerId: aws.String(d.Id()),
Configuration: expandMqConfigurationId(d.Get("configuration").([]interface{})),
Logs: expandMqLogs(d.Get("logs").([]interface{})),
})
if err != nil {
return err
return fmt.Errorf("error updating MQ Broker (%s) configuration: %s", d.Id(), err)
}
requiresReboot = true
}

if d.HasChange("user") {
o, n := d.GetChange("user")
err := updateAwsMqBrokerUsers(conn, d.Id(),
var err error
Copy link
Contributor Author

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!

Copy link
Contributor

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?

// d.HasChange("user") always reports a change when running resourceAwsMqBrokerUpdate
// updateAwsMqBrokerUsers needs to be called to know if changes to user are actually made
var usersUpdated bool
usersUpdated, err = updateAwsMqBrokerUsers(conn, d.Id(),
o.(*schema.Set).List(), n.(*schema.Set).List())
if err != nil {
return err
return fmt.Errorf("error updating MQ Broker (%s) user: %s", d.Id(), err)
}

if usersUpdated {
requiresReboot = true
}
}

if d.Get("apply_immediately").(bool) {
if d.Get("apply_immediately").(bool) && requiresReboot {
_, err := conn.RebootBroker(&mq.RebootBrokerInput{
BrokerId: aws.String(d.Id()),
})
if err != nil {
return err
return fmt.Errorf("error rebooting MQ Broker (%s): %s", d.Id(), err)
}

stateConf := resource.StateChangeConf{
Expand Down Expand Up @@ -502,32 +522,38 @@ func waitForMqBrokerDeletion(conn *mq.MQ, id string) error {
return err
}

func updateAwsMqBrokerUsers(conn *mq.MQ, bId string, oldUsers, newUsers []interface{}) error {
func updateAwsMqBrokerUsers(conn *mq.MQ, bId string, oldUsers, newUsers []interface{}) (bool, error) {
// If there are any user creates/deletes/updates, updatedUsers will be set to true
updatedUsers := false

createL, deleteL, updateL, err := diffAwsMqBrokerUsers(bId, oldUsers, newUsers)
if err != nil {
return err
return updatedUsers, err
}

for _, c := range createL {
_, err := conn.CreateUser(c)
updatedUsers = true
if err != nil {
return err
return updatedUsers, err
}
}
for _, d := range deleteL {
_, err := conn.DeleteUser(d)
updatedUsers = true
if err != nil {
return err
return updatedUsers, err
}
}
for _, u := range updateL {
_, err := conn.UpdateUser(u)
updatedUsers = true
if err != nil {
return err
return updatedUsers, err
}
}

return nil
return updatedUsers, nil
}

func diffAwsMqBrokerUsers(bId string, oldUsers, newUsers []interface{}) (
Expand Down
103 changes: 101 additions & 2 deletions aws/resource_aws_mq_broker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,7 @@ func TestAccAWSMqBroker_updateTags(t *testing.T) {
resource.TestCheckResourceAttr("aws_mq_broker.test", "tags.env", "test"),
),
},
// Adding new user + modify existing
bflad marked this conversation as resolved.
Show resolved Hide resolved
// Modify existing tags
{
Config: testAccMqBrokerConfig_updateTags2(sgName, brokerName),
Check: resource.ComposeTestCheckFunc(
Expand All @@ -654,7 +654,7 @@ func TestAccAWSMqBroker_updateTags(t *testing.T) {
resource.TestCheckResourceAttr("aws_mq_broker.test", "tags.role", "test-role"),
),
},
// Deleting user + modify existing
// Deleting tags
{
Config: testAccMqBrokerConfig_updateTags3(sgName, brokerName),
Check: resource.ComposeTestCheckFunc(
Expand All @@ -667,6 +667,45 @@ func TestAccAWSMqBroker_updateTags(t *testing.T) {
})
}

func TestAccAWSMqBroker_updateSecurityGroup(t *testing.T) {
sgName := fmt.Sprintf("tf-acc-test-%s", acctest.RandString(5))
brokerName := fmt.Sprintf("tf-acc-test-%s", acctest.RandString(5))

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t); testAccPreCheckAWSMq(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAwsMqBrokerDestroy,
Steps: []resource.TestStep{
{
Config: testAccMqBrokerConfig(sgName, brokerName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAwsMqBrokerExists("aws_mq_broker.test"),
resource.TestCheckResourceAttr("aws_mq_broker.test", "security_groups.#", "1"),
),
},
{
Config: testAccMqBrokerConfig_updateSecurityGroups(sgName, brokerName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAwsMqBrokerExists("aws_mq_broker.test"),
resource.TestCheckResourceAttr("aws_mq_broker.test", "security_groups.#", "2"),
),
},
// Trigger a reboot and ensure the password change was applied
// User hashcode can be retrieved by calling resourceAwsMqUserHash
{
Config: testAccMqBrokerConfig_updateUsersSecurityGroups(sgName, brokerName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAwsMqBrokerExists("aws_mq_broker.test"),
resource.TestCheckResourceAttr("aws_mq_broker.test", "security_groups.#", "1"),
resource.TestCheckResourceAttr("aws_mq_broker.test", "user.#", "1"),
resource.TestCheckResourceAttr("aws_mq_broker.test", "user.2209734970.username", "Test"),
resource.TestCheckResourceAttr("aws_mq_broker.test", "user.2209734970.password", "TestTest9999"),
),
},
},
})
}

func testAccCheckAwsMqBrokerDestroy(s *terraform.State) error {
conn := testAccProvider.Meta().(*AWSClient).mqconn

Expand Down Expand Up @@ -1128,3 +1167,63 @@ resource "aws_mq_broker" "test" {
}
`, sgName, brokerName)
}

func testAccMqBrokerConfig_updateSecurityGroups(sgName, brokerName string) string {
return fmt.Sprintf(`
resource "aws_security_group" "test" {
name = "%s"
}

resource "aws_security_group" "test2" {
name = "%s-2"
}

resource "aws_mq_broker" "test" {
apply_immediately = true
broker_name = "%s"
engine_type = "ActiveMQ"
engine_version = "5.15.0"
host_instance_type = "mq.t2.micro"
security_groups = ["${aws_security_group.test.id}", "${aws_security_group.test2.id}"]

logs {
general = true
}

user {
username = "Test"
password = "TestTest1234"
}
}
`, sgName, sgName, brokerName)
}

func testAccMqBrokerConfig_updateUsersSecurityGroups(sgName, brokerName string) string {
return fmt.Sprintf(`
resource "aws_security_group" "test" {
name = "%s"
}

resource "aws_security_group" "test2" {
name = "%s-2"
}

resource "aws_mq_broker" "test" {
apply_immediately = true
broker_name = "%s"
engine_type = "ActiveMQ"
engine_version = "5.15.0"
host_instance_type = "mq.t2.micro"
security_groups = ["${aws_security_group.test2.id}"]

logs {
general = true
}

user {
username = "Test"
password = "TestTest9999"
}
}
`, sgName, sgName, brokerName)
}