aws_kms_secret is using private SDK feature that will no longer work after Core 0.12

[apparentlymart]

This issue is intended to start a discussion about what we're going to do about an unexpected compatibility situation for the Core 0.12 release.

The aws_kms_secret resource is currently using the special __has_dynamic_attributes attribute and the ResourceData.UnsafeSetFieldRaw method, neither of which are allowed to be used outside the terraform_remote_state resource per the documentation:

// UnsafeSetFieldRaw allows setting arbitrary values in state to arbitrary
// values, bypassing schema. This MUST NOT be used in normal circumstances -
// it exists only to support the remote_state data source.
func (d *ResourceData) UnsafeSetFieldRaw(key string, value string) {
     // ...
}

This was added as a hack to allow the terraform_remote_state data source to return dynamically-typed attributes based on remote state outputs even though helper/schema doesn't actually support dynamic schema. This hack is no longer supported for Core 0.12 because Core now has support for dynamically-typed attributes.

Under the assumption that this was used only in terraform_remote_state as required by that comment, we have a plan for migrating terraform_remote_state that relies on the fact that the terraform provider is built into Terraform Core and so it is able to directly implement the internal provider interface rather than running in a child process via RPC. This solution will not work for aws_kms_secret because it does run as a child process plugin and so it is constrained by the featureset of the provider SDK.

---

The reason this feature has to go away for 0.12 is because Terraform Core now needs to know the schema of each resource type in order to decode and validate resource and data blocks and validate references to resource attributes.

The Core-level representation of schema requires all of the resource attributes to be enumerated. There is no support for arbitrary extra attributes being added dynamically by the provider, since it would then be impossible to run the language interpreter's semantic check to validate the configuration.

For the terraform_remote_state resource we will be addressing this by re-introducing its original design of having all of the outputs returned in a single attribute called outputs which a nested object with an attribute for each output. Relying on the fact that terraform_remote_state can code directly against the Core provider interface, and is thus not subject to provider SDK limitations, it is able to mark the output attribute itself as being dynamically-typed, which then allows Terraform Core to type check the top-level attributes of the data source itself but suspends type checking of the outputs attribute until its final value is known.

The effect of this is that data.terraform_remote_state.example.foo can be detected as invalid during the validation step, but data.terraform_remote_state.example.outputs.foo will be detected as invalid only after the data source has been read, since the type of data.terraform_remote_state.example.outputs is not known during validation.

Because aws_kms_secret is implemented in terms of the provider SDK so that it can work as a plugin, this strategy will not work there: the provider SDK at the time of writing has no way to express that a particular attribute has a dynamic type.

---

From inspecting the aws_kms_secret code, it appears that secret values are always strings:

https://github.com/terraform-providers/terraform-provider-aws/blob/083bccb553beb03195534e0824b8b607ec58eb11/aws/data_source_aws_kms_secret.go#L95

This assumption would give us a solution here that would work within the existing constraints of the provider SDK, allowing us to defer introducing dynamic types there until some later point.

My suggestion is to define an attribute called secrets that is typed as a map of strings, and then return the secrets in that map rather than at the top level. Users would then access secrets via that map:

aws_kms_secret.example.secrets.foo

This can be implemented using the normal ResourceData methods, and thus we can stop calling the to-be-removed UnsafeSetFieldRaw method.

Unfortunately this will be a breaking change for users of this resource, so would need to happen in a major release with a state migration implementation and a configuration migration guide.

In principle if we can do this soon we could do a gradual migration where for a while both the new secrets map and the hacky dynamic attributes are supported together, allowing for a deprecation cycle, and then drop support for the dynamic attributes in a major release that coincides approximately with the Core 0.12 release.

However, we have no way to generate a deprecation warning for the use of dynamic attributes (Core doesn't know anything about them in 0.11 and prior) and so this deprecation cycle would have to rely only on documentation in the CHANGELOG and the website.

[bflad]

We really should provide some sort of upgrade path as soon as possible before the end of the Terraform 0.12 countdown timer, definitely don't want this to hold up anything there. 😄

Every time I keep thinking about reworking the existing resource and its state, I worry that any change to it could potentially be breaking to someone's configuration. 😖 I guess that technically, we could mark the existing secret attribute as deprecated with the friendly message saying to move references or read the documentation, but that's pretty bad user experience as you wouldn't be able to fix it until the next major version is released.

Instead, I am wondering if it might be best to use the "new" resource deprecation functionality here to switch to a pluralized aws_kms_secrets data source using the TypeMap approach you mention.

We do get some fringe benefits:

• Actually provides a fixable deprecation notice to operators. We can link it to a section of the new data source documentation page that explicitly shows how to migrate configurations.
• Aligns this data source to our current naming conventions of singular versus plural handling. We can (re-)introduce a singular data source (that would be easier to use with only top level attributes) after the next major version.

[apparentlymart]

That seems like a fine approach to me, @bflad. Since it's a data source (rather than a managed resource type) the migration here would still be config-only even though it's a change of name. (If it had been a managed resource type then some terraform state mv would've been required.)

In that case, I suppose the plan would be:

• Add this new data source ASAP alongside the old one, leaving the old one unchanged apart from adding the deprecation notice.
• In the AWS provider major release that will coincide with the Terraform Core 0.12 release (introducing the new plugin protocol) we'd also remove the deprecated aws_kms_secret data source, ending its deprecation period.

[bflad]

Sounds great @apparentlymart -- I am submitting the pull request to upgrade our vendoring to the hashicorp/terraform SHA with DeprecationMessage and should be able to get this handled sometime in the next week or so.

[apparentlymart]

Awesome! Thanks, @bflad.

[bflad]

Pull request submitted based on the implementation above: #5195

[iancward]

this is probably related: #5040

[bflad]

Thanks @iancward! That issue is certainly relevant here. 😄

[bflad]

A new aws_kms_secrets data source and deprecation of the existing aws_kms_secret data source has been merged into master via #5195. Instructions for configuration migration are provided in a new "AWS Provider Version 2 Upgrade" guide are deep linked in the old data source deprecation message and old data source documentation page.

This will release tomorrow with version 1.29.0 of the AWS provider. 👍

[bflad]

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

[bflad]

Soft removal of the aws_kms_secret data source can be found here: #7657

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