Move aws_kms_ciphertext to resource

[JoelSpeed]

Fixes #960

As a resource, the encryption is performed on create only.

Changing any of the input forces a new resource.

[Ninir]

Hi @JoelSpeed

Thanks for the proposal here!

Unfortunately, we can't accept such work as it is a major breaking change: people relying on the data source would not be able to use it anymore, while just upgrading from something to 1.6.0 for instance.

I'm not sure to see how having a resource would fix it here: it's just me trying to better understand it.
Thanks!

[JoelSpeed]

The argument for making it into a resource was based on reading this page https://www.terraform.io/docs/configuration/data-sources.html and my general understanding of terraform idoms.

My understanding is that we are creating the ciphertext from the input. We only need to recreate this when the input changes which sounds to me, more like a resource than a datasource.

By making this a resource, we can use the ForceNew property on the input schema to only perform the encryption when one of the input properties changes using the Create function. A Read operation should actually not do anything to the ciphertext_blob as it is stored in the state and this is usually used to read remote properties.

My thoughts were that you could wrap the creation of the ciphertext_blob like the below within the data resource, but this seemed like a bit of an abuse of the data source type. I believe this would fix the issue but this logic shouldn't really be within a read function.
https://github.com/pusher/terraform-provider-aws/blob/df4c73b9b61962162673810d0351a8630ef9e2a5/aws/resource_aws_kms_ciphertext.go#L81-L83

[JoelSpeed]

@Ninir Assuming you won't look at this again with the waiting-response label, I guess I should have @'ed you in the first reply, sorry

[Ninir]

@JoelSpeed No worries 😄

What I meant is, if we were to do such thing, we could not remove the data source for now, but first deprecate it, and in a coming future remove it. So we should instead ADD a new resource rather than moving the data source to a resource.

Having a resource might be the way to go, even though I think this is resolvable at the data source level.

[JoelSpeed]

@Ninir What would you prefer me to do then? I see two options, try and add some logic to the data type and fix it that way.

Or put the data type back into this PR and follow a deprecation path to move it to a resource?

[Ninir]

@JoelSpeed In an utopian world, I would go for your 1st suggestion, fixing it as is. Let us know If we can help!

[JoelSpeed]

@Ninir I've had a go at this over the last few hours but have run into a few problems.

I was hoping to be able to use the d.HasChange method to detect the changes, but this does not work as data resources do not get passed the current state, only the new input.

This means that you cannot check whether the input to the Read method has changed or not since the last run.

I then thought, maybe you could try decrypting the old ciphertext, the Decrypt method in the aws sdk returns the key and the plaintext if the decrypt is successful which would give you the information to compare. But this is also not possible as the old ciphertext_blob is not in the state passed to the Read method so you cannot attempt to decrypt it.

The only input the Read method gets are the current values of the key_id, ciphertext_blob and context.

Without access to the old state or the old ciphertext blob, I don't think this bug can be fixed while keeping the ciphertext as a data resource.

[johnjelinek]

It sounds like this PR is almost ready if you add it the existing behavior and add the resource.

[charles-at-geospock]

I've just encountered this problem. We would like to have idempotent deployments, and have just introduced the use of kms ciphertext into our workflow. Now, however, each execution results in a change to the ciphertext, which means that resources get updated with the new data and the deployment is no longer idempotent.

My understanding of the discussion so far is...

• Removing the data resource isn't reasonable as people will be using it. It must be deprecated first.
• Adding logic for changes in state to the data resource won't work, because the data resources don't get to know about the current state.
• The implementation to make a resource (from the existing data resource) exists in the change supplied.

So my understanding would be that if the data resource were left alone, and the new (regular) resource were created with the logic proposed here, that would allow the original data resource to continue working as it does (and nobody is any worse off who uses it), and give an easy migration path for them to move to the new resource which avoids this change in state on each invocation.

Would that be a correct understanding of the situation?

If it is, I may have a go at updating this change to implement that, if I get an opportunity in the next few weeks.

[JoelSpeed]

@charles-at-geospock If that's the case, I can just amend my commits to remove the deletion of the data type.

If the maintainers will accept having both a data and resource and start the migration process then I can make the PR resemble that, won't take too long

[jbergknoff]

Would it also be feasible to store a hash of the plaintext instead of the plaintext itself?

[cperilla-rival]

@Ninir I have implemented the changes you requested on this PR #6993

[bflad]

Hi @JoelSpeed 👋 Thanks for your contribution here. As noted above, this pull request would have required creating a separate resource, which looks like was taken care of after a few months in a new pull request: #6993. Since #6993 does not introduce breaking changes and implements the functionality as expected, opting to merge that one. Thanks again for submitting this.

[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!