provider/aws: Support tags for AWS redshift cluster

[stack72]

Needed to implement a model of marking that a redshift cluster needed modifying. Without this requestUpdate variable being set, I was getting the following error:

* aws_redshift_cluster.default: [WARN] Error modifying Redshift Cluster (tf-redshift-cluster-4700836719349831525): InvalidParameterCombination: No modifications were requested
            status code: 400, request id: b4cc33fc-dd58-11e5-9c3a-4bb3559cbdb2

The test results are as follows:

make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSRedshiftCluster' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSRedshiftCluster -timeout 120m
=== RUN   TestAccAWSRedshiftCluster_basic
--- PASS: TestAccAWSRedshiftCluster_basic (549.24s)
=== RUN   TestAccAWSRedshiftCluster_tags
--- PASS: TestAccAWSRedshiftCluster_tags (627.55s)
=== RUN   TestAccAWSRedshiftCluster_notPubliclyAccessible
--- PASS: TestAccAWSRedshiftCluster_notPubliclyAccessible (574.28s)
PASS
ok  github.com/hashicorp/terraform/builtin/providers/aws    1751.501s

[radeksimko]

Would this API call fail if you passed name instead of ARN?

[stack72]

Not sure actually TBH - looking now

[stack72]

Yes, it will fail. We need to pass the ARN here

[radeksimko]

Like we discussed in another issue, I'm not a big fan of making more resources dependent on iam:GetUser (effectively on IAM Users).

I'm personally inclined to hold off on merging this before #5030 is in place and then we can leverage that logic to get account ID reliably across multiple environments (EC2 instance, IAM User, federated IAM Role).

[stack72]

With regards to iam:GetUser - I am following a pattern used all over Elasticache and db instance etc

So if we change this, we should change those at the same time

[radeksimko]

#5030 is merged and account ID is available via meta.(*AWSClient).accountid.

#6503 is fixing the discussed issue for existing resources. Keep in mind that getting the account ID may fail and your function here should expect accountid to potentially be an empty string (and probably raise an error in such cases).

[stack72]

@radeksimko this has been updated with the new way of building the ARN - please let me know if you are happy with this so I can merge :)

Ideally, I'd like to get this into 0.6.16!

P.

[radeksimko]

Are you sure this is a good idea? Just silently ignore tags update? I think we should rather return this as an error to the user, so they're aware that tags have not been updated and they also know why.

[stack72]

This is how it is currently done in db_instance as well

https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_db_instance.go#L659

I just used the same layout here

[radeksimko]

Good spot!

87907e2#diff-0b21e21c91dc560c837c95806e2ba913R341
^ @catsby was there any specific intention in making tags update fail silently?

[josephholsten]

I'm in favor of returning an error for a failed tag update, but would be happy to let it slide so long as there's a tracking ticket to come back to it.

[radeksimko]

I left you some comments there. I can run related acceptance tests for Redshift tomorrow morning. Looking at the retry timeouts scares me away, but I'm happy to verify that before merging for you. 😉

[radeksimko]

This is looking ok except the two last things:

1. silent failure - pending @catsby 's response
2. two exact same test cases

[stack72]

@radeksimko if we are under pressure for 0.6.16 can this be a follow up request for these things are they already exist in db_instance and will need to be fixed there too?

[stack72]

Made the changes as per the review:

• Return an error if the tags fail
• Testing of the actual tags

The tags unit test is the same test used everywhere. We can make any changes to these later :)

[josephholsten]

👍 LGTM

[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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.