provider/aws: Add support for KMS

[radeksimko]

This is a polished version of #3209

@psytale would you mind giving a look at this? Just wanted to know whether you're happy with the modifications I made and whether all make sense to you.

Test plan

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=Kms' 2>~/tf.log

Eventual consistency

There's just one concern I have - KMS has apparently similar problem as IAM policies - that is eventual consistency. Creation/deletion doesn't seem to be a problem, but disabling/enabling and key rotation are fields that may take time to propagate.

The way we usually deal with such problem is that we call Describe/Get operations repeatedly until we have a confirmation that status has been changed. The problem with such approach is that KMS is literally inconsistent which means that a single confirmation often doesn't mean propagation:

KEY_ID=$(aws kms create-key --description "aws-eventually-consistent-kms-3" | jq -r .KeyMetadata.KeyId)
echo "Disabling $KEY_ID"
aws kms disable-key --key-id $KEY_ID
echo "$KEY_ID should be now disabled"
aws kms describe-key --key-id $KEY_ID | jq .KeyMetadata.Enabled
aws kms describe-key --key-id $KEY_ID | jq .KeyMetadata.Enabled
aws kms describe-key --key-id $KEY_ID | jq .KeyMetadata.Enabled
aws kms describe-key --key-id $KEY_ID | jq .KeyMetadata.Enabled
aws kms describe-key --key-id $KEY_ID | jq .KeyMetadata.Enabled
aws kms describe-key --key-id $KEY_ID | jq .KeyMetadata.Enabled

Disabling 128169fc-a5ac-481f-99f5-730c6980d924
128169fc-a5ac-481f-99f5-730c6980d924 should be now disabled
false
true
false
false
true
true

^ that is why I set the MinTimeout fairly high - 60 secs, but it's naive solution anyway. I'm thinking of creating a special version or modifying the WaitForState helper so that it's counting occurrences of target state in a given time frame and maybe wait for defined percentage (majority) of target status.

Are we silently ignoring this edge case for IAM policies too?

Reported to AWS

I have submitted two feature requests to AWS while I was working on this:

1. ListAliases to support listing by KeyId, so we don't have to waste so much effort on listing through all
2. Make KMS read-after-write consistent (which I believe was just an upvote for an existing feature request)

[apparentlymart]

Should the start of this paragraph be "Provides an alias for a KMS customer master key"?

[radeksimko]

fixed & squashed into the original commit with docs, so I'm not cluttering the git log even more.

[philwitty]

Looks great to me. Really appreciate the help

[boxrick]

So is KMS supported within Terraform now ? Or some elements?

[radeksimko]

I wanted to merge this yesterday, so I reran the acceptance tests before doing so.

There are still issues with the eventual consistency - TestAccAWSKmsKey_isEnabled is intermittently failing due to that.

I'm not sure what the best solution is - read my first post above. Even though we wait 1 minute until checking the status, it does not guarantee consistency. Better solution may include waiting for x number of subsequent success statuses.

I will probably try to implement the mentioned solution.

cc @Felivel

[boxrick]

Interesting, I am currently playing around with your dev build. Would be particularly handy if IAM Administrators could be specified. Makes it a little difficult to use this without it.

[catsby]

Hey @radeksimko sorry for the delay – how do you feel about this PR right now?

[radeksimko]

@catsby Feature-wise this is ready, but it would make me feel more confident if we first get thru #4447 and implement it here afterwards to prevent the intermittent test failures.

The other linked issue would help making the initial creation a lot faster, but that's not really something I'd consider to be a blocker for KMS generally.

[radeksimko]

Just vendor-ed KMS to make godep happy and make the tests green again.

[boxrick]

Do we have any updates with this getting merged back in? Waiting for this functionality!

[radeksimko]

This is now ready for final review as I managed to implement the enhanced retry logic from #4447 that made enabling/disabling keys a lot more stable which made me much more confident that this resource will actually work well.

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=AWSKms'

TF_ACC=1 go test ./builtin/providers/aws -v -run=AWSKms -timeout 120m
=== RUN   TestAccAWSKmsAlias_basic
--- PASS: TestAccAWSKmsAlias_basic (368.99s)
=== RUN   TestAccAWSKmsAlias_multiple
--- PASS: TestAccAWSKmsAlias_multiple (83.41s)
=== RUN   TestAccAWSKmsKey_basic
--- PASS: TestAccAWSKmsKey_basic (230.95s)
=== RUN   TestAccAWSKmsKey_isEnabled
--- PASS: TestAccAWSKmsKey_isEnabled (657.69s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    1341.056s

[bigkraig]

@psytale @catsby hey guys does this cut the mustard now? It looks like everything has been wrapped up. I just sent in pull request #5453 to implement KMS server side encryption on S3 objects and would really like to be able to get everything into the next release.

[phinze]

This 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.