[WIP] Add a new EngineTypeRabbitmq AWS messaging engine type

[bonza]

See https://docs.aws.amazon.com/amazon-mq/latest/developer-guide/getting-started-rabbitmq.html

Not sure what else needs to be changed

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

Relates OR Closes #0000

Release note for CHANGELOG:

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

[github-actions]

Welcome @bonza 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTING guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

[mattburgess]

Hi @bonza - note that my PR at https://github.com/hashicorp/terraform-provider-aws/pull/16732/files#diff-c8149cac5dcb3d1ae9b02795a3824d9ac8ae1e87fedce8529dede6d884987665R102 also covers this but in a slightly different way.

From a quick look at the Getting Started Guide and the API docs it looks like it might just be as simple as specifying an engine_type of RABBITMQ & a correct engine_version value 🤞

[bonza]

Thanks, @mattburgess !
Unfortunately, I have no idea how to test it, as we are using v12 of the terraform and I couldn't find any docs on how to replace an existing data provider with a custom one for v12. I was hoping to get some help here or I to be lucky enough so it will be merged =)

[mattburgess]

For testing, I'd suggest you start by copying the current "basic" test from https://github.com/hashicorp/terraform-provider-aws/blob/master/aws/resource_aws_mq_broker_test.go#L236-L301 and call it something like TestAccAWSMqBroker_rabbitmq. Then take a copy of that test's config from

terraform-provider-aws/aws/resource_aws_mq_broker_test.go

Lines 897 to 920 in 4ca17d1

	func testAccMqBrokerConfig(sgName, brokerName string) string {
	return fmt.Sprintf(`
	resource "aws_security_group" "test" {
	name = "%s"
	}
	resource "aws_mq_broker" "test" {
	broker_name = "%s"
	engine_type = "ActiveMQ"
	engine_version = "5.15.0"
	host_instance_type = "mq.t2.micro"
	security_groups = [aws_security_group.test.id]
	logs {
	general = true
	}
	user {
	username = "Test"
	password = "TestTest1234"
	}
	}
	`, sgName, brokerName)
	}

and call it something like testAccMqBrokerConfig_rabbitmq. Change the parameters there to match what is required for a basic RabbitMQ-based broker then change your copy of the basic test case so that it calls that function (i.e. your equivalent of line 248).

The documentation at https://github.com/hashicorp/terraform-provider-aws/blob/master/docs/contributing/running-and-writing-acceptance-tests.md will walk you through how to run those tests against your branch-built version of the provider.

If you run it at this stage, it will fail because of the test assertions (the resource.TestCheckResourceAttr() calls) so update those to match your engine_type and engine_version values from your config function and they should pass (assuming that changing those values don't require any other changes as well).

You'll also want to update the documentation at https://github.com/hashicorp/terraform-provider-aws/blob/4ca17d1f5bc5f1c5bf21b0ab6ee8ee79ccb7701e/website/docs/r/mq_broker.html.markdown to mention that engine_version takes a second valid string now.

Hope this helps!

[bonza]

@mattburgess , it seems like it is not that easy as I was hoping. The issue I'm trying to fix is there is no such thing as configuration for RabbitMQ engine, so acceptance test is failing on a call of flattenMqConfigurationId with scary

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x4656b52]

goroutine 360 [running]:
github.com/terraform-providers/terraform-provider-aws/aws.resourceAwsMqBrokerRead(0xc002c89500, 0x658b580, 0xc000676000, 0x71af540, 0xc000622dc0)
        /home/damir/work/go/terraform-providers/terraform-provider-aws/aws/resource_aws_mq_broker.go:379 +0xbf2

Unfortunately, I'm not a Go expert, I'm a total noob :) Do you know any resource code I can take a look to find a way how to deal with similar differences between resource types?

[bonza]

    resource_aws_mq_broker_test.go:309: Step 1/2 error: After applying this test step and performing a `terraform refresh`, the plan was not empty.
        stdout


        An execution plan has been generated and is shown below.
        Resource actions are indicated with the following symbols:
          ~ update in-place

        Terraform will perform the following actions:

          # aws_mq_broker.test will be updated in-place
          ~ resource "aws_mq_broker" "test" {
                apply_immediately          = false
                arn                        = "arn:aws:mq:eu-west-1:<censored>:broker:tf-acc-test-fqf9v:b-17032f34-01c1-4cae-a82d-d328ef7efa04"
                auto_minor_version_upgrade = false
                broker_name                = "tf-acc-test-fqf9v"
                deployment_mode            = "SINGLE_INSTANCE"
                engine_type                = "RabbitMQ"
                engine_version             = "3.8.6"
                host_instance_type         = "mq.t3.micro"
                id                         = "b-17032f34-01c1-4cae-a82d-d328ef7efa04"
                instances                  = [
                    {
                        console_url = "https://b-17032f34-01c1-4cae-a82d-d328ef7efa04.mq.eu-west-1.amazonaws.com"
                        endpoints   = [
                            "amqps://b-17032f34-01c1-4cae-a82d-d328ef7efa04.mq.eu-west-1.amazonaws.com:5671",
                        ]
                        ip_address  = ""
                    },
                ]
                publicly_accessible        = false
                security_groups            = [
                    "sg-0dcb61105bb86e342",
                ]
                subnet_ids                 = [
                    "subnet-06fd0ef1f0679e503",
                ]
                tags                       = {}

                encryption_options {
                    use_aws_owned_key = true
                }

                logs {
                    audit   = false
                    general = false
                }

                maintenance_window_start_time {
                    day_of_week = "FRIDAY"
                    time_of_day = "23:00"
                    time_zone   = "UTC"
                }

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

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

Have no idea what is going on here 😢

[mattburgess]

This took a bit of digging! https://github.com/aws/aws-sdk-go/blob/v1.36.13/service/mq/api.go#L5751 - ConsoleAccess is only applicable to ActiveMQ clusters, not RabbitMQ. Groups isn't explicitly called out as not applying to RabbitMQ brokers but its comment only explicitly calls out ActiveMQ.

[mattburgess]

Well, that seems to be a bit of a false alarm. @bonza - if you run the test like this: TF_LOG=DEBUG make testacc TESTARGS='-run=TestAccAWSMqBroker_basic_rabbitmq' you'll see all of the API requests made to AWS and the responses it returns.

You'll see a CreateBroker call, then a number of DescribeBroker calls as it waits for the broker to reach a RUNNING state. Interestingly, when that final DescribeBroker call returns, this is what I see:

  "brokerArn" : "arn:aws:mq:us-west-2:xxx:broker:tf-acc-test-cfrdd:b-5c8b471e-cb4b-4c16-ac49-a480e5685fea",
  "brokerId" : "b-5c8b471e-cb4b-4c16-ac49-a480e5685fea",
  "brokerName" : "tf-acc-test-cfrdd",
  "brokerState" : "RUNNING",
  "engineType" : "RabbitMQ",
  "engineVersion" : "3.8.6",
  "hostInstanceType" : "mq.t3.micro",
  "publiclyAccessible" : false,
  "autoMinorVersionUpgrade" : false,
  "deploymentMode" : "SINGLE_INSTANCE",
  "subnetIds" : [ "subnet-0d09a732b68bd3467" ],
  "securityGroups" : [ "sg-0cd5f517bdcac6e73" ],
  "brokerInstances" : [ {
    "consoleURL" : "https://b-5c8b471e-cb4b-4c16-ac49-a480e5685fea.mq.us-west-2.amazonaws.com",
    "endpoints" : [ "amqps://b-5c8b471e-cb4b-4c16-ac49-a480e5685fea.mq.us-west-2.amazonaws.com:5671" ]
  } ],
  "maintenanceWindowStartTime" : {
    "timeOfDay" : "19:00",
    "dayOfWeek" : "MONDAY",
    "timeZone" : "UTC"
  },
  "logs" : {
    "general" : false,
    "generalLogGroup" : "/aws/amazonmq/broker/b-5c8b471e-cb4b-4c16-ac49-a480e5685fea/general"
  },
  "users" : [ ],
  "created" : "2020-12-22T16:17:23.891Z",
  "tags" : { },
  "encryptionOptions" : {
    "useAwsOwnedKey" : true
  },
  "storageType" : "ebs",
  "authenticationStrategy" : "simple"
}

Note how the users array is empty there. That's what's causing the test to fail, because it doesn't actually match what was requested, so the post-apply plan step shows differences. My next idea is to try to create a RabbitMQ broker using the AWS CLI and see what the describe-broker command gives back.

[mattburgess]

Yup, sure enough, this looks to be a bug in the AWS API:

Given an input.json of:

{
    "BrokerName": "mybroker",
    "DeploymentMode": "SINGLE_INSTANCE",
    "EngineType": "RABBITMQ",
    "EngineVersion": "3.8.6",
    "HostInstanceType": "mq.t3.micro",
    "StorageType": "EBS",
    "Users": [
        {
            "Password": "TestTest1234",
            "Username": "Test"
        }
    ]
}

The cluster can be created using: aws mq create-broker --cli-input-json file://input.json

And then described using: aws mq describe-broker --broker-id b-some-id

This shows:

{
    "AuthenticationStrategy": "simple",
    "AutoMinorVersionUpgrade": true,
    "BrokerArn": "arn:aws:mq:us-west-2:xxx:broker:mybroker:b-6f570b6d-319c-4ce6-abb0-4d2068c57cf9",
    "BrokerId": "b-6f570b6d-319c-4ce6-abb0-4d2068c57cf9",
    "BrokerInstances": [
        {
            "ConsoleURL": "https://b-6f570b6d-319c-4ce6-abb0-4d2068c57cf9.mq.us-west-2.amazonaws.com",
            "Endpoints": [
                "amqps://b-6f570b6d-319c-4ce6-abb0-4d2068c57cf9.mq.us-west-2.amazonaws.com:5671"
            ]
        }
    ],
    "BrokerName": "mybroker",
    "BrokerState": "RUNNING",
    "Created": "2020-12-22T16:53:38.703Z",
    "DeploymentMode": "SINGLE_INSTANCE",
    "EncryptionOptions": {
        "UseAwsOwnedKey": true
    },
    "EngineType": "RabbitMQ",
    "EngineVersion": "3.8.6",
    "HostInstanceType": "mq.t3.micro",
    "Logs": {
        "General": false,
        "GeneralLogGroup": "/aws/amazonmq/broker/b-6f570b6d-319c-4ce6-abb0-4d2068c57cf9/general"
    },
    "MaintenanceWindowStartTime": {
        "DayOfWeek": "TUESDAY",
        "TimeOfDay": "03:00",
        "TimeZone": "UTC"
    },
    "PubliclyAccessible": false,
    "SecurityGroups": [
        "sg-0e3470cde0f9f4f73"
    ],
    "StorageType": "ebs",
    "SubnetIds": [
        "subnet-0d09a732b68bd3467"
    ],
    "Tags": {},
    "Users": []
}

[mattburgess]

aws/aws-sdk-go#3625 is interesting! Looks like the current thinking is that users will always be empty when the engine is RABBITMQ. There's nothing at https://github.com/aws/aws-sdk-go/blob/v1.36.13/service/mq/api.go#L3548-L3623 to indicate that's the intended behaviour, so this might want to go back through AWS' support channels to confirm that it'll always be emty.

[bonza]

Thanks a lot, @mattburgess! It would take days for me to get through all of this

[mattburgess]

@bonza - I've raised aws/aws-sdk#32 seeking confirmation of if this is indeed expected behaviour. There are ways and means of suppressing that diff if that's the case but I worry about how this resource should handle it; we'll probably want to add some extra tests to ensure we do the right thing if a user updates the users attribute post-creation. This may require forcing a recreation of the cluster, or it may be able to be updated in place. Again, testing using the AWS CLI would likely be the quickest way of confirming AWS' intended behaviour. I'll see what I can do over the next couple of days!

[YakDriver]

@bonza Thanks for your work on this PR! I recognize this is a draft PR. However, we've reworked MQ in #16108. Please rebase on that and see if there's anything you can add. Thanks again for your help!

[YakDriver]

I believe that this functionality has been added with #16108 and may no longer be necessary. If you have objections, please respond in this thread. Otherwise, this will be closed at a future time.

[bonza]

Thanks a lot for you work, @YakDriver !

[ghost]

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!