New Resource: aws_kms_grant #3038

jammerful · 2018-01-17T22:10:50Z

Hey,

Basically I think this is close to being ready to merge, but I see two flaws with it:

Dealing with the InvalidArnException upon creation of a grant that depends on a newly created role. I have a retry in my code, but it will just retry in the case that the arn is actually invalid. I don't see a way to distinguish between eventual consistency and a truly invalid arn.
- I think this maybe okay behavior.
In a similar vein, if you refresh the state quickly after creating a role and grant (as is the case with the acceptance tests currently). The read function will sometimes get an internal identifier (AROAXXXXX instad of arn:aws:XXX) to the grantee principal instead of an ARN, which would if you naively take the value be written to the state file and cause a new resource to be created since grants have no way of being updated.
- What I did, which I saw done in another resource file, is if the read doesn't return valid grant or retiree principals then don't update the state file for those attributes and log a warning. This will only happen in two cases ~1 minute after iam principal creation (aws acknoleged eventual consistency problem) and if one of the principals has been deleted (in cases where it is managed outside of terraform).

If merged this resolves #641

I have run the acceptance tests in their current form, and they pass:

$ make testacc TESTARGS='-run=TestAWSKmsGrant'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAWSKmsGrant -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAWSKmsGrant_Basic
--- PASS: TestAWSKmsGrant_Basic (39.08s)
=== RUN   TestAWSKmsGrant_withConstraints
--- PASS: TestAWSKmsGrant_withConstraints (30.75s)
=== RUN   TestAWSKmsGrant_withRetiringPrincipal
--- PASS: TestAWSKmsGrant_withRetiringPrincipal (39.39s)
=== RUN   TestAWSKmsGrant_bare
--- PASS: TestAWSKmsGrant_bare (38.95s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	148.205s

I have also run the unit tests and they pass:

$ make test
==> Checking that code complies with gofmt requirements...
go test ./... -timeout=30s -parallel=4
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1.765s

jammerful · 2018-01-19T21:24:34Z

aws/resource_aws_kms_grant.go

+				// an InvalidArnException to be thrown. TODO: Possibly change the aws_iam_role code?
+				if awsErr.Code() == "DependencyTimeoutException" ||
+					awsErr.Code() == "InternalException" ||
+					awsErr.Code() == "InvalidArnException" {


This is the first issue I mentioned.

jammerful · 2018-01-19T21:25:02Z

aws/resource_aws_kms_grant.go

+		// Confirmed with AWS that this is expected behavior.
+		// TODO: Handle upstream or here?
+		if grant != nil {
+			if !strings.HasPrefix(*grant.GranteePrincipal, "arn:aws") {


This is the second issue I mentioned.

jammerful · 2018-02-09T17:21:53Z

@radeksimko @bflad @paddycarver @Ninir @Puneeth-n Can you please take a look at this soon?

kikitux · 2018-02-27T23:01:38Z

Sorry for the delay, finding someone for a review

paddycarver

Sorry for the delay in reviewing this. Thanks for opening the PR! It looks nice overall, but I had some feedback.

paddycarver · 2018-02-27T23:44:31Z

aws/resource_aws_kms_grant.go

+				ValidateFunc: validateArn,
+			},
+			"operations": {
+				Type: schema.TypeList,


Does order matter here? Can the API return items in a different order without breaking its contract? And does it make sense to make the elements of this list available for interpolation?

If the answer to all of these is "no", then this should probably be a Set. If the answer to the first two is no but the third is yes, that should probably be a conversation we have and come to a decision about how to make this work.

The answer to all of these is no. Will make a set, I think the real benefit here is the set avoids duplication which could cause errors upon creation.

paddycarver · 2018-02-27T23:44:55Z

aws/resource_aws_kms_grant.go

+				ForceNew: true,
+			},
+			"constraints": {
+				Type:     schema.TypeList,


Same questions here, re: making it a Set possibly.

Will make a set.

I'm almost done with my revision, so I'll update the PR tomorrow. Sorry for the delay, the hash function for constraints now that it is a set is causing me a little heartache.

paddycarver · 2018-02-27T23:45:55Z

aws/resource_aws_kms_grant.go

+							Type:          schema.TypeMap,
+							Optional:      true,
+							Elem:          schema.TypeString,
+							ConflictsWith: []string{"constraints.0.encryption_context_subset"},


I am not 100% sure that ConflictsWith works this way. E.g., I don't know if setting the encryption context subset on the first element will allow the second element to have encryption context equals set.

It worked it my testing, I'm trying not allow both encryption_context_subset and encryption_context_equals both set, since the AWS KMS Grant API does not allow that. I'll have to change how that looks with the move to sets. Make sense?

Yeah, I think I get what you were trying to do here, the syntax is just a bit :/ It may be worthwhile to just implement that logic manually to throw an error in the Create function.

paddycarver · 2018-02-27T23:46:10Z

aws/resource_aws_kms_grant.go

+				ValidateFunc: validateArn,
+			},
+			"grant_creation_tokens": {
+				Type:     schema.TypeList,


Same questions here about whether this should be a Set or not.

Will move to a set, as the tokens should be unique. I don't see this a generating a lot of value, but I'll move it to a set.

The main value is that if the API ever changes the order, or the user changes the order, the grant doesn't need to be recreated for no real purpose.

paddycarver · 2018-02-27T23:48:19Z

aws/resource_aws_kms_grant.go

+	}
+	if grant != nil {
+		// The grant sometimes contains principals that identified by their unique id: "AROAJYCVIVUZIMTXXXXX"
+		// instead of "arn:aws:...", in this case don't update the state file


This logic worries me, as not setting things in state allows for drift between state and reality, which can be the source of some gnarly bugs. What conditions make principals return as their unique IDs? Is there any way to translate the unique IDs into arn:aws:... format?

Oh, I see, you acknowledged this in the PR body. In this case, I think it'd be better to retry until we get an ID in the arn:aws: format (or a max threshold is hit) than to not set state.

So this is one of the big question that I had.

I tried putting it in the retry earlier, the real problems is that if the IAM Principal/Role is managed outside of terraform and it is deleted before the grant is deleted the refresh step will fail, and if the person wants to delete the grant even if we let the error through it will be stuck on refreshing the state for a few minutes for nothing which seems like pretty un-optimal behavior.

So there are two times the internal ID will be not be resolved:

The IAM role is newly created and hasn't propagated through the system (eventual consistency)

The IAM role has been deleted, and no amount of retries will help here.

That mapping from internal ID to arn:aws:... is exactly what AWS itself is trying to do on the backend, I wouldn't try to re-implement that here.

Let me know what you think is the best path, what I've written above is what makes the most sense to me.

Hm, the second case does give me pause. Another silly question: is it worth storing the internal ID? What's the bad stuff that happens, in that case? That would protect us from server drift while side-stepping the eventual consistency problem. And if the IAM role has been deleted, I think it'd be better for Terraform to make some noise about that, rather than pretending everything is fine, right?

We could store the internal ID.
But think of this case: the only thing is that typically Create returns the Read function, which will in cases of new IAM roles cause internal IDs to be stored. Then when lets say some just applies terraform again, the change from internal ID to ARN will cause new resources to be created. So I think it may cause unnecessary resource churn.

Let me ensure that there isn't something clever I'm missing, I'll ask a few people I know that have dealt with these eventual consistency issues. Would it be possible for you to ask a few co-workers as well?

paddycarver · 2018-02-28T00:11:45Z

aws/resource_aws_kms_grant.go

+		m["encryption_context_subset"] = pointersMapToStringList(constraint.EncryptionContextSubset)
+	}
+
+	return []interface{}{m}


Can we only have one constraint? Why does the expand version loop over it then?

Only one of encryption_context_subset or encrytpion_context_equals can be set, but that itself can be a map. Will have to refactor this as part of the move to sets.

paddycarver · 2018-02-28T00:12:11Z

aws/resource_aws_kms_grant.go

+
+// Custom error, so we don't have to rely on
+// the content of an error message
+type KmsGrantMissingError struct {


This could just be a string. type KmsGrantMissingError string.

paddycarver · 2018-02-28T00:13:24Z

aws/resource_aws_kms_grant_test.go

+}
+
+resource "aws_kms_grant" "%s" {
+	name = "%s"


does this need to be unique?

No grant names don't need to be unique, they are optional. I just put names incase of orphaned resources it will be easier to identify them.

Awesome, thanks! I just wanted to be sure we wouldn't run into an error if the resource got orphaned, which we usually try to avoid using randomized names. Sounds like we don't need a randomized name, here, though.

paddycarver · 2018-02-28T00:14:16Z

aws/validators.go

+
+	if _, ok := validTypes[value]; !ok {
+		es = append(es, fmt.Errorf(
+			"%s must be one of the following: [ Decrypt, Encrypt, GenerateDataKey, GenerateDataKeyWithoutPlaintext, ReEncryptFrom, ReEncryptTo, CreateGrant, RetireGrant, DescribeKey ]", k))


Could we programmatically generate this list, to prevent drift?

Will pull from the above map above thanks. By the way this kind of duplication is pretty common in this file, is probably worth cleaning up.

lets follow the suggestion in this PR, your suggestion is welcome but lets focus the conversation on the PR it self

alvaro

No problem, was just bringing a small possible issue to your attention.

paddycarver · 2018-02-28T00:14:43Z

aws/validators.go

+func validateAwsKmsGrantOperation(v interface{}, k string) (ws []string, es []error) {
+	value := v.(string)
+
+	validTypes := map[string]bool{


This could be a map[string]struct{} with each value set to struct{}{}, just to denote that the value of the map has no meaning.

Will fix thanks.

jammerful · 2018-02-28T19:13:44Z

@paddycarver Thanks for the review. I left some comments, most of your concerns will be fixed. Except for the one about not loading in IAM principals into terraform state if it doesn't resolve to an ARN--I left some reasons on why retrying isn't optimal (maybe if we do something like if d.isNewResource but even then you really want that to be on the IAM principal not if the grant is new).

I did have one other concern, about the scalability of this approach. I was stress testing my code, and it can handle about ~200 grants managed in one resource file, if it is more the resource refreshing takes longer than the read timeout. The reason for the scale limit, is that for each resource/grant we are calling list grants, which will get throttled by AWS. Would it make sense to have a global list of grants? Have you seen other resources implement this (resources that don't have a Describe* call)? I could see this getting messy.

I will upload a new commit when finished tonight or tomorrow.

Will squash commits when ready to merge.

paddycarver · 2018-03-01T04:28:07Z

Thanks for the work on this and the careful explanations! For the scale part... The only solution I can think of at the moment is to introduce user-specified timeouts, like this: https://github.com/terraform-providers/terraform-provider-aws/blob/177a19851fbbf8ec6a54f6733312ae205d7327d3/aws/resource_aws_ami_from_instance.go#L30-L34

But I'm toying with the idea of just waiting to see if people hit that timeout case, and then adding the custom timeouts if that turns into an issue people have. I don't know how common it is, unfortunately, to create 200 grants.

There were murmurs and talks of eventually adding some sort of request batching capabilities to Terraform, but I don't think that's something people were talking about for the near future. Though that would ideally be the "real" solution to this, reusing a single List call across resources.

Unfortunately, I think trying to build batching in using a single List call at the moment would be buggy and error-prone, so I'd advise against it unless we discover we absolutely must implement it.

jammerful · 2018-03-02T02:12:59Z

I may add user specific timeouts, is that fine? I think I'll have a use for them myself. I'll update my PR tomorrow, sorry for the delay.

Will squash commits when ready to merge.

log.Print -> log.Printf Will squash

jammerful · 2018-03-02T19:12:53Z

@paddycarver Addressed your comments. Please take a look.

Regarding sometimes not setting the roles in state, the only thing I could think of is to always store the role id in the state file. However, I this won't work if the role is from another account since the caller won't have GetRole permissions on that role from another account.

jammerful · 2018-03-05T17:24:03Z

aws/resource_aws_kms_grant.go

+
+// The hash needs to encapsulate what type of constraint it is
+// as well as the keys and values of the constraint.
+func resourceKmsGrantConstraintsHash(v interface{}) int {


I need to fix this up. As of now, I know I at least need to sort the maps so that the constraint gets consistent hashes. I couldn't find any common utilities for this.

Will squash

jammerful · 2018-03-07T00:01:08Z

@paddycarver I think all of the initial comments have been resolved. Please take another look.

I still don't have a sense if not setting the roles in state is a non-starter/blocker. Keep in mind there is no way to modify a grant, so I wouldn't be extremely concerned about drift between the state file and what is in AWS.

jammerful · 2018-03-07T19:15:35Z

aws/resource_aws_kms_grant.go

+		input.Name = aws.String(v.(string))
+	}
+	if v, ok := d.GetOk("constraints"); ok {
+		if !kmsGrantConstraintsIsValid(v.(*schema.Set)) {


Not sure how much value this is delivering, I really liked when we used conflicts with in the schema because it validates this before going out to AWS. Right now this is caught right before AWS would let us know of the conflict, after other resources have already been created.

I'd love this to fail at plan time, too, but in the interest of moving this forward, I wanted to keep our scope here simple and not introduce complicated things like CustomDiff or ConflictsWith on a Set, which I've never used on a Set before, and don't know the intricacies of how it works. @bflad or @paultyng may have more insight on that.

It's almost certainly possible to have this fail at plan time. I wouldn't consider making that happen a blocker, but if you'd like it to be in the first pass, I'm happy to have it included.

Will address in a follow up PR, as I mentioned in another comment. Might make the most sense to move this back to a list? The set adds a lot of additional code, as well as not adding much value I think given that there can only be one element.

jammerful · 2018-03-08T19:55:03Z

@paddycarver @kikitux If you can't look at this soon could I please get an indication of whether you are inclined to merge this, or are there major issues preventing this from being merged?

copumpkin · 2018-03-09T17:15:21Z

Unfortunately, I think trying to build batching in using a single List call at the moment would be buggy and error-prone, so I'd advise against it unless we discover we absolutely must implement it.

@paddycarver if I understand correctly, without batching we get basically quadratic (actually O(m * n) where m is number of TF grant resources and n is number of grants in the account, which after the first time is always >= m). My concern here is that unlike other resources with similar issues (I think KMS aliases), grants are often quite numerous. One of my keys today has several hundred grants on it, the default limit per CMK is 2500 (and I've had it raised by AWS support in the past), and I'd like to manage many of those grants with KMS. That seems like it'll become untenable pretty quickly, unless you have a small handful of grants. But with a small number of grants, you might as well just encode it in the key policy.

I get that Terraform doesn't have a proper generic solution for this problem yet, but it doesn't seem awful to maintain a bit of state across grant resource calls so they don't each have to list grants from scratch.

bflad

Hi @jammerful! 👋 I'm just leaving a drive-by code review for a few things since this PR popped up on my radar, minus looking at the eventual consistency code. Architecturally, I do think this resource fits into the Terraform model if we can figure out the eventual consistency issues (I'll let @paddycarver keep working with you on that angle unless it gets handed off).

Please reach out with any questions!

bflad · 2018-03-09T17:08:19Z

aws/resource_aws_kms_grant.go

+				Set:  schema.HashString,
+				Elem: &schema.Schema{
+					Type:         schema.TypeString,
+					ValidateFunc: validateAwsKmsGrantOperation,


We can avoid a custom validation function and associated unit test by using Terraform core's validation.StringInSlice() 😄 (it should also use SDK provided constants)

ValidateFunc: validation.StringInSlice([]string{ kms.GrantOperationCreateGrant, kms.GrantOperationDecrypt, kms.GrantOperationDescribeKey, kms.GrantOperationEncrypt, kms.GrantOperationGenerateDataKey, kms.GrantOperationGenerateDataKeyWithoutPlaintext, kms.GrantOperationReEncryptFrom, kms.GrantOperationReEncryptTo, kms.GrantOperationRetireGrant, }, false),

bflad · 2018-03-09T17:12:10Z

aws/resource_aws_kms_grant.go

+		out, err = conn.CreateGrant(&input)
+
+		if err != nil {
+			if awsErr, ok := err.(awserr.Error); ok {


You can avoid dealing with awserr directly by using the isAWSErr() helper function we have. 😄 (similarly the SDK usually provides constants for the error codes)

if isAWSErr(err, kms.ErrCodeDependencyTimeoutException, "") {

bflad · 2018-03-09T17:14:49Z

aws/resource_aws_kms_grant.go

+	if grant == nil {
+		log.Printf("[WARN] %s KMS grant id not found for key id %s, removing from state file", grantId, keyId)
+		d.SetId("")
+	}


This should return nil so the if grant != nil { below (and its nesting) is unnecessary.

bflad · 2018-03-09T17:18:15Z

aws/resource_aws_kms_grant.go

+	return nil
+}
+
+// Retiring grants requires special permissions (i.e. the


Seems like this behavior should be noted in the documentation. Better yet, it would probably be best to provide an attribute flag to control the behavior (e.g. retire_on_delete) that defaults to true so we can always return an error if an expected deletion (retire or just revoke) is not possible.

Done. However, I inverted the default to false. Functionally retiring and revoking are the same, they don't show up in the ListGrants call, however retiring requires special permissions. It seemed to me that defaulting to that would be noisy and cause confusion.

bflad · 2018-03-09T17:18:45Z

aws/resource_aws_kms_grant.go

+		return err
+	}
+
+	d.SetId("")


Nitpick: d.SetId("") is no longer required in delete functions

Done. Didn't realize that.

bflad · 2018-03-09T17:31:22Z

aws/resource_aws_kms_grant.go

+	_, err := conn.RevokeGrant(&input)
+
+	if err != nil {
+		return err


We should allow non-existent grants to skip returning an error so its not required to do a terraform state rm:

if isAWSErr(err, kms.ErrCodeNotFoundException, "") { return nil }

Great catch. Done.

bflad · 2018-03-09T17:33:32Z

aws/resource_aws_kms_grant.go

+
+// Custom error, so we don't have to rely on
+// the content of an error message
+type KmsGrantMissingError string


Instead of creating our own error class, can we instead utilize the upstream SDK's awserr.Error and set to a code like kms.ErrCodeNotFoundException?

kms.ErrCodeNotFoundException is a string, and I wanted a type or code to use so I don't have to key off the error message. Might be missing something here?

bflad · 2018-03-09T17:47:14Z

aws/resource_aws_kms_grant.go

+	}
+
+	log.Printf("[DEBUG] Created new KMS Grant: %s", *out.GrantId)
+	d.SetId(*out.GrantId)


Given that grant IDs are really under a key ID, we might want to consider making the resource ID in Terraform contain both, e.g. KeyID:GrantID. This will allow us to import them.

Saving as part of the resource identifier:

// Using keyID := d.Get("key_id").(string) above input d.SetId(fmt.Sprintf("%s:%s", keyID, aws.StringValue(out.GrantId)))

Parsing anytime necessary:

keyID, grantID, err := decodeKmsGrantId(d.Id()) if err != nil { return err } // ... func decodeKmsGrantId(id string) (string, string, error) { parts := strings.Split(id, ":") if len(parts) != 2 { return "", "", fmt.Errorf("Unexpected format of ID (%q), expected KeyID:GrantID", id) } return parts[0], parts[1], nil }

paultyng · 2018-03-09T21:18:20Z

@copumpkin I think the issue is what if there is a grant that depends on a grant, etc, things like that? If you needed the token to create another grant for example then that cache starts to work against you, which is why it hasn't been used in other places.

I think since the grants are listed by key you could just create an aws_kms_grants resource that worked in bulk potentially where the ID was the key? And it could just share code here? Just a thought at least, I'm not very familiar with KMS, and that may also not be able to leverage count as effectively, etc.

Edit: here are some example bulk resources:

copumpkin · 2018-03-09T21:24:44Z

@paultyng that's an idea that occurred to us, but is there precedent for it in Terraform? Edit: I see you've amended your comment to give some examples, thanks!

Also, is there a good place (like a TF contributors Slack) to chat live about this sort of thing? Might help sort through these concerns more quickly. There's a few options that have occurred to us but it's hard to factor in what might work vs. trying to guess what HashiCorp might be willing to merge.

paddycarver

Hey @copumpkin / @jammerful, apologies for not getting back to you last week--my week got away from me with other providers. :/ I'm glad @paultyng and @bflad were able to jump in, because they know way more about this than I do. I took another pass on this, but I defer to either @bflad or @paultyng about the batching question. My instinct is to not borrow trouble, and address it if it pops up, but I'm not the most familiar with KMS, so that may be a sub-optimal stance. I do agree with @paultyng that the separate, plural resource has been a successful pattern for batching things in the past where there are too many resources to handle 1-by-1. I'm, unfortunately, not enough of a domain expert to be of much use in deciding whether it will be useful in this case or not. :(

As for the eventual consistency, I'm willing to accept the WARN log, and conditionally not setting it if we have an issue there. I don't like it, I'm not super happy with it, but the risk seems relatively small, and I think it's probably premature optimisation on my part to try and get rid of it. If @bflad disagrees, however, and wants it handled, I will happily propose workarounds and try to make that work.

I left a couple minor comments, but I don't think there's anything I see here that I'd hold the merge over. I'll circle up with @bflad in a couple of hours and make sure he and I are on the same page and get this merged, or report back if necessary.

As for a Slack, we only have a Slack channel for maintainers, at the moment, unfortunately. I apologise for the slow-motion code review and guesswork involved here. My general rule of thumb about what I'll accept (@bflad may have a different rule of thumb, and as he's a lead for the AWS provider, I'll defer to him) is:

Will this work in unexpected ways for users or break on users?
Does this use the framework in unusual or unorthodox ways, which may trigger an edge case in the future? If so, can we avoid that for a more mainstream approach? I'm generally more of a "follows the semantics" than a "technically works" reviewer, personally.
Is there an example of a similar resource that's already in Terraform? Does it do something differently? Why?
If we need to change this in the future, can we do it without breaking backwards compatibility? If not, how likely is it that we'll need to change this in the future?

I know that's not an enormously helpful list, but a good test is: does something like this already exist in Terraform? If yes, that's a good indicator it'll probably be acceptable. If no, then there may be some back and forth about it.

I'll report back in a few hours when the Eastern US is awake and I can make sure you're not going to get three separate answers on this.

paddycarver · 2018-03-12T10:32:55Z

aws/resource_aws_kms_grant.go

+		input.Name = aws.String(v.(string))
+	}
+	if v, ok := d.GetOk("constraints"); ok {
+		if !kmsGrantConstraintsIsValid(v.(*schema.Set)) {


I'd love this to fail at plan time, too, but in the interest of moving this forward, I wanted to keep our scope here simple and not introduce complicated things like CustomDiff or ConflictsWith on a Set, which I've never used on a Set before, and don't know the intricacies of how it works. @bflad or @paultyng may have more insight on that.

It's almost certainly possible to have this fail at plan time. I wouldn't consider making that happen a blocker, but if you'd like it to be in the first pass, I'm happy to have it included.

paddycarver · 2018-03-12T10:38:22Z

aws/resource_aws_kms_grant.go

+	return nil, NewKmsGrantMissingError(fmt.Sprintf("[DEBUG] Grant %s not found for key id: %s", grantId, keyId))
+}
+
+// Can't have both constraint options set:


I'll admit I'm confused by this part. You're only allowed to have 1 constraint, right? If you're only allowed 1 constraint, is the only reason constraints is a TypeSet for the block syntax? If that's the case, we could use MaxItems: 1 on constraints to make sure no more than 1 block is specified. Doesn't obviate the need for this function, but can simplify it a little.

Note: This is not a blocker for merge.

Setting MaxItems won't work given the way the constraint set is being hashed, it ends up being one item with two different sub-attributes. Maybe I should have left it as a list, mostly had moved it on your suggestion.

paddycarver

I talked with @bflad about this, and we got on the same page. This is totally good to go and ready to merge, with one caveat: it'd be great to see the ARN eventual consistency behavior documented. Feel free to ping me when you do that (or if you don't have time, reply here and I'll do it) and we'll get this merged.

paddycarver · 2018-03-12T14:40:51Z

website/docs/r/kms_grant.html.markdown

+
+* `name` - (Optional, Forces new resources) A friendly name for identifying the grant.
+* `key_id` - (Required, Forces new resources) The unique identifier for the customer master key (CMK) that the grant applies to.
+* `grantee_principal` - (Required, Forces new resources) The principal that is given permission to perform the operations that the grant permits.


Could we document here the eventual consistency problem and workaround we have in place, with the ARN weirdness?

jammerful · 2018-03-12T15:52:29Z

@paddycarver Thank you for all the help and comments. I updated the documentation, and replied to your comments. Should be ready to merge. Let me know if you want me to squash the commits.

paddycarver · 2018-03-12T16:04:00Z

Looks great! Thanks for contributing to Terraform's AWS provider!

copumpkin · 2018-03-12T16:05:21Z

Sweet, thanks! I also have an idea for how to get rid of the O(m * n) behavior that I'll propose in a separate issue and tag you on, if you don't mind.

paddycarver · 2018-03-12T16:07:13Z

Feel free to tag me, and I'll do my best to get to it, but to set expectations appropriately: I am responsible for other providers, so it may take me a bit to get to it; this week is already looking unlikely. There's no harm in tagging me on it, but your best bet for getting eyes on it is to open it up to the maximum number of maintainers. :)

bflad · 2018-03-23T20:50:15Z

This has been released in version 1.12.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-07T17:23:37Z

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!

bflad added new-resource Introduces a new resource. service/kms Issues and PRs that pertain to the kms service. labels Jan 17, 2018

jammerful mentioned this pull request Jan 18, 2018

Support management of AWS KMS grants #641

Closed

radeksimko changed the title ~~Add AWS KMS Grant Resource~~ New Resource: aws_kms_grant Jan 18, 2018

jammerful commented Jan 19, 2018

View reviewed changes

Add AWS KMS Grant Resource

6dc1bf7

jammerful force-pushed the master branch from 36ed029 to 6dc1bf7 Compare February 9, 2018 17:13

ghost added the size/XL Managed by automation to categorize the size of a PR. label Feb 9, 2018

paddycarver suggested changes Feb 28, 2018

View reviewed changes

aws_kms_grant: Address PR Concerns

ca73eaf

Will squash commits when ready to merge.

aws_kms_grant: Move constraints from List to Set

90b2c39

Will squash commits when ready to merge.

ghost added the size/XL Managed by automation to categorize the size of a PR. label Mar 2, 2018

aws_kms_grant: fix typo

b7c714a

log.Print -> log.Printf Will squash

jammerful commented Mar 5, 2018

View reviewed changes

aws_kms_grant: Make Constraint Hash Function Determinstic

c15b2fc

Will squash

ghost added the size/XL Managed by automation to categorize the size of a PR. label Mar 6, 2018

jammerful commented Mar 7, 2018

View reviewed changes

bflad reviewed Mar 9, 2018

View reviewed changes

aws_kms_grant: Address bflad PR comments

04f0b83

ghost added the size/XL Managed by automation to categorize the size of a PR. label Mar 9, 2018

paddycarver approved these changes Mar 12, 2018

View reviewed changes

paddycarver suggested changes Mar 12, 2018

View reviewed changes

bflad added this to the v1.12.0 milestone Mar 12, 2018

aws_kms_grant: Document Eventual Consistency Issue

5448f70

ghost added the size/XL Managed by automation to categorize the size of a PR. label Mar 12, 2018

paddycarver approved these changes Mar 12, 2018

View reviewed changes

paddycarver merged commit 525fd78 into hashicorp:master Mar 12, 2018

copumpkin mentioned this pull request Mar 12, 2018

Improve asymptotic efficiency of aws_kms_grant #3746

Closed

ghost locked and limited conversation to collaborators Apr 7, 2020

New Resource: aws_kms_grant #3038

New Resource: aws_kms_grant #3038

Conversation

jammerful commented Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jammerful commented Feb 9, 2018 • edited Loading

kikitux commented Feb 27, 2018

paddycarver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jammerful Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jammerful Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

jammerful commented Feb 28, 2018 • edited Loading

paddycarver commented Mar 1, 2018

jammerful commented Mar 2, 2018 • edited Loading

jammerful commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jammerful commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jammerful commented Mar 8, 2018

copumpkin commented Mar 9, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paultyng commented Mar 9, 2018 • edited Loading

copumpkin commented Mar 9, 2018 • edited Loading

paddycarver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paddycarver left a comment

Choose a reason for hiding this comment

jammerful commented Jan 17, 2018 •

edited

Loading

jammerful commented Feb 9, 2018 •

edited

Loading

jammerful Mar 2, 2018 •

edited

Loading

jammerful Feb 28, 2018 •

edited

Loading

jammerful commented Feb 28, 2018 •

edited

Loading

jammerful commented Mar 2, 2018 •

edited

Loading

paultyng commented Mar 9, 2018 •

edited

Loading

copumpkin commented Mar 9, 2018 •

edited

Loading