r/aws_msk_sasl_scram_secret: New Resource

[dblooman]

Fixes #15237 and #15298

Looking for feedback on the approach here, should this be included in the resource_aws_msk_cluster resource?

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=TestAccAwsMskScramSecret*'           
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAwsMskScramSecret* -timeout 120m
=== RUN   TestAccAwsMskScramSecret_basic
=== PAUSE TestAccAwsMskScramSecret_basic
=== RUN   TestAccAwsMskScramSecret_Delete
=== PAUSE TestAccAwsMskScramSecret_Delete
=== RUN   TestAccAwsMskScramSecret_UpdateRemove
=== PAUSE TestAccAwsMskScramSecret_UpdateRemove
=== CONT  TestAccAwsMskScramSecret_basic
=== CONT  TestAccAwsMskScramSecret_UpdateRemove
=== CONT  TestAccAwsMskScramSecret_Delete
--- PASS: TestAccAwsMskScramSecret_basic (1190.10s)
--- PASS: TestAccAwsMskScramSecret_UpdateRemove (1216.69s)
--- PASS: TestAccAwsMskScramSecret_Delete (1240.62s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       1243.196s

...

[ant1441]

The output of the GetBootstrapBrokers API has been updated with a bootstrapBrokerStringSaslScram key.

For my use case I would need access to this, so ideally this would be added as a new attribute to the aws_msk_cluster Resource. Indeed, as that is the only response key I see, I think all users would want this:

$ aws kafka get-bootstrap-brokers --cluster-arn arn:aws:kafka:eu-west-1:<id>:cluster/<name>/<id>
{
    "BootstrapBrokerStringSaslScram": "<removed>"
}

I would assume this would go alongside the existing bootstrap_brokers & bootstrap_brokers_tls attributes?

• https://github.com/terraform-providers/terraform-provider-aws/blob/b787f355fe2953492a10d563bb5974171f040407/aws/resource_aws_msk_cluster.go#L440-L441
• https://github.com/terraform-providers/terraform-provider-aws/blob/3c5747e4a2bcbb6798d854fab62932f78911cc4b/aws/data_source_aws_msk_cluster.go#L102-L103

aws-sdk-go GetBootstrapBrokersOutput struct:
https://github.com/aws/aws-sdk-go/blob/v1.34.30/service/kafka/api.go#L5587-L5602

[dblooman]

@ant1441 Ok, I will add this to the changes, im still working on tests for the new resource, but this will be included in the PR

[dblooman]

Just as an update

The BatchAssociateScramSecret operation. Once that is run, a secret policy is attached to any secret associated with MSK, which includes the ARN of the secret that has just been created and attached. If the policy is left blank, then an AWS created policy will be attached, creating a diff in the terraform and meaning there will always be changes presented. My tests will not pass due to the fact there are changes after the apply, I can ignore this, but it would present as an undesirable result for the user, they create a custom policy, or empty policy, then create the resources, then changes are presented based on the MSK service changes.

[jurgenweber]

awesome work! Looking forward to seeing it soon.

[dblooman]

Further update:

After speaking with AWS, it seems that the resources policy for a secret for MSK will be generated regardless of whether a policy exists and is sufficient. However, to overcome this, I will use the https://docs.aws.amazon.com/sdk-for-go/api/service/secretsmanager/#SecretsManager.PutResourcePolicy PutResourcePolicy call and create a new resource called aws_secretsmanager_secret_policy which can use the newly created secret ARN to build the policy and apply before the association is made. As long as the policy is identical to the one provided by AWS, it will work as expected.

I will create a new PR with that resource in a new PR and once it is merged will update this PR to reflect the usage.

[haidaraM]

Looks like there is already a PR for the new ressource aws_secretsmanager_secret_policy : #14468.

[haidaraM]

I had an issue when trying to add multiple secrets to the same cluster:

Error: there were unprocessed secrets during association: [{
  ErrorCode: "InvalidSecretArn",
  ErrorMessage: "The requested resource doesn’t exist.",
  SecretArn: "arn:aws:secretsmanager:eu-west-1:xxxxxxxx:secret:AmazonMSK_ST_Test_2_20201016115903956600000001-ff4f52"
}]

I created a module inside which there is a aws_msk_sasl_scram_secret that attach a single secret to the cluster passed as parameter (and do other things). When calling this module twice, it fails to some time. Maybe a race condition with AWS API ?

Does the secrets can be managed in multiple Terraform project or they need to be managed by a single aws_msk_sasl_scram_secret ?

[haidaraM]

Update: same issue when removing and adding some secrets in the same plan (even with -parallelism=1). Need to plan/apply twice to make it work properly.

Not sure if it's related but looks like the params of filterSecrets are inverted:

• Declaration:
  https://github.com/terraform-providers/terraform-provider-aws/blob/09c04b3eb2fa552190e2e1b24c6a826c2e9d24d1/aws/resource_aws_msk_scram_secret.go#L157

• Usage:
  https://github.com/terraform-providers/terraform-provider-aws/blob/09c04b3eb2fa552190e2e1b24c6a826c2e9d24d1/aws/resource_aws_msk_scram_secret.go#L48

[dblooman]

Thanks for feedback, will add a test for this and update

[bill-rich]

Hi @dblooman! I'm trying to get caught up on your PR, and wanted to check that I understanding of the need for the secretsmanager_secret_policy resource is correct.

If the secret is applied before a policy is associated, a default policy will be applied, and that policy may not be as restrictive as what is needed. If you create the policy before applying the secret, then the desired policy will be in place from the start. So the idea would be that the secret would be created, then the policy would be associated with it then the cluster is created with the new secret (and policy)?

Please let me know if I have that wrong or missed any important points.

[jurgenweber]

Hi Team, a polite ping. This would be really cool to get into the next provider rls.

[dblooman]

@bill-rich Hi, agreed on the points, except where the API call to associate secret is concerned. If you don't create a policy, that is fine, once you associate the secret, a new policy is created for you on behalf of the kafka aws service, e.g

{
  "Version" : "2012-10-17",
  "Statement" : [ {
    "Effect" : "Allow",
    "Principal" : {
      "Service" : "kafka.amazonaws.com"
    },
    "Action" : "secretsmanager:getSecretValue",
    "Resource" : "arn:aws:secretsmanager:us-east-1:0000000:secret:AmazonMSK_testing-3KUWHE"
  } ]
}

Where the unique ID at the end after the name of the secret is random. In the same way that we wish to attach the policy, the kafka service will use the resource ARN to determine the policy, but you can't use the ARN before the resource exists and therefor, cannot create the resource and the policy at the same time. I asked the MSK product team about this and they suggested that we use an attachment, which is why I would like to use the AWS secrets manager secret policy resource.

@jurgenweber Im working on this all day today, writing tests and fixing issues seen by @haidaraM, hopefully will have something ready today or tommorrow.

[bill-rich]

@dblooman Thank you for the clarification. I'm focusing on getting these two resources approved and merged also, so please ping me when you're ready for me to take another look at msk_sasl_scram_secret and give it a review. Meanwhile, I'll be working on the code review for secretsmanager_secret_policy.

[jurgenweber]

@dblooman Thanks mate, really appreciate the effort. I Apologise that I am too time pressed to assist but always happy to cheer from the side lines. :)

[dblooman]

@haidaraM I think my latest commit should take care of the bug you found.

Fun fact, if you associate a secret, then delete the secret before disassociating it, the secret still stays associated, but doesn't present in the console.

Test to be completed tomorrow and will submit for review. @bill-rich For now i have added the secrets manager code to test, but once that is merge it will be rebased out.

[anGie44]

totally understand @dblooman, thank you for letting me know👍 happy to continue the work here and get it into an upcoming release

[anGie44]

Output of acceptance tests (Commercial):

--- FAIL: TestAccAWSMskCluster_OpenMonitoring (2513.23s) -- failing in TC, passing locally
--- PASS: TestAccAWSMskClusterDataSource_Name (2516.08s)
--- PASS: TestAccAWSMskCluster_BrokerNodeGroupInfo_EbsVolumeSize (2784.61s)
--- PASS: TestAccAWSMskCluster_ClientAuthentication_Sasl_Scram (2783.68s)
--- PASS: TestAccAWSMskCluster_ConfigurationInfo_Revision (3286.33s)
--- PASS: TestAccAWSMskCluster_EncryptionInfo_EncryptionAtRestKmsKeyArn (2570.84s)
--- PASS: TestAccAWSMskCluster_EncryptionInfo_EncryptionInTransit_ClientBroker (2802.29s)
--- PASS: TestAccAWSMskCluster_EncryptionInfo_EncryptionInTransit_InCluster (2664.71s)
--- PASS: TestAccAWSMskCluster_EnhancedMonitoring (2829.92s)
--- PASS: TestAccAWSMskCluster_KafkaVersionDowngrade (3886.85s)
--- PASS: TestAccAWSMskCluster_KafkaVersionUpgrade (6678.27s)
--- PASS: TestAccAWSMskCluster_KafkaVersionUpgradeWithConfigurationInfo (6677.91s)
--- PASS: TestAccAWSMskCluster_NumberOfBrokerNodes (2965.33s)
--- PASS: TestAccAWSMskCluster_Tags (2824.15s)
--- PASS: TestAccAWSMskCluster_basic (2847.43s)
--- PASS: TestAccAwsMskScramSecretAssociation_basic (2517.21s)
--- PASS: TestAccAwsMskScramSecretAssociation_clusterDisappears (2767.39s)
--- PASS: TestAccAwsMskScramSecretAssociation_disappears (2842.44s)
--- PASS: TestAccAwsMskScramSecretAssociation_update (2625.98s)
--- SKIP: TestAccAWSMskCluster_ClientAuthentication_Tls_CertificateAuthorityArns (0.00s)

Output of failed acctest when run locally:

--- PASS: TestAccAWSMskCluster_OpenMonitoring (1825.88s)

[PavelDemyanenko]

@anGie44 great work! Thank you! Hopefuly we'll see it in upcoming release

[dblooman]

Hi, thanks for the work @anGie44. Do we know which release this will be in?

[anGie44]

Thanks again @dblooman for the great work and your (and other's following this feature) patience! Getting this work into the upcoming 3.18.0 release 👍

Confirming output of acceptance tests (commercial):

--- SKIP: TestAccAWSMskCluster_ClientAuthentication_Tls_CertificateAuthorityArns (0.00s)
--- PASS: TestAccAWSMskCluster_EncryptionInfo_EncryptionAtRestKmsKeyArn (1812.73s)
--- PASS: TestAccAWSMskCluster_basic (1953.64s)
--- PASS: TestAccAwsMskScramSecretAssociation_clusterDisappears (2002.34s)
--- PASS: TestAccAWSMskCluster_EncryptionInfo_EncryptionInTransit_ClientBroker (2017.27s)
--- PASS: TestAccAwsMskScramSecretAssociation_disappears (2083.90s)
--- PASS: TestAccAWSMskCluster_EnhancedMonitoring (2181.96s)
--- PASS: TestAccAWSMskCluster_LoggingInfo (2190.62s)
--- PASS: TestAccAWSMskCluster_OpenMonitoring (2201.28s)
--- PASS: TestAccAWSMskCluster_EncryptionInfo_EncryptionInTransit_InCluster (2221.78s)
--- PASS: TestAccAwsMskScramSecretAssociation_basic (2287.63s)
--- PASS: TestAccAWSMskClusterDataSource_Name (2301.83s)
--- PASS: TestAccAWSMskCluster_Tags (2322.30s)
--- PASS: TestAccAwsMskScramSecretAssociation_update (2373.08s)
--- PASS: TestAccAWSMskCluster_BrokerNodeGroupInfo_EbsVolumeSize (2530.99s)
--- PASS: TestAccAWSMskCluster_NumberOfBrokerNodes (2740.42s)
--- PASS: TestAccAWSMskCluster_ConfigurationInfo_Revision (2987.14s)
--- PASS: TestAccAWSMskCluster_ClientAuthentication_Sasl_Scram (3303.68s)
--- PASS: TestAccAWSMskCluster_KafkaVersionDowngrade (3363.70s)
--- PASS: TestAccAWSMskCluster_KafkaVersionUpgrade (5974.84s)
--- PASS: TestAccAWSMskCluster_KafkaVersionUpgradeWithConfigurationInfo (5983.46s)

[ghost]

This has been released in version 3.18.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]

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!