Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resource/aws_rds_cluster_instance: PerformanceInsightsKMSKeyId is not necessarily an ARN #3102

Closed
wants to merge 4 commits into from
Closed

resource/aws_rds_cluster_instance: PerformanceInsightsKMSKeyId is not necessarily an ARN #3102

wants to merge 4 commits into from

Conversation

kwent
Copy link
Contributor

@kwent kwent commented Jan 23, 2018

Fixing #3014

cc @ColinHebert

…manceInsightsKMSKeyId can be either a KMS key ARN, a KMS alias ARN or simply a KMS key ID.

In most cases, I would expect people to rely on either the KMS alias ARN or the key ARN, but because of the way data.aws_kms_key works, some users might end up with a raw key ID (not the ARN).

In this case the cluster instance creation will fail as the validation ensures that the performance_insights_kms_key_id is an ARN.

As a side note, the terraform documentation also mentions that we expect "The ARN for the KMS key to encrypt Performance Insights data.", it is probably worth specifying that an ARN to an alias is perfectly fine as well.

Fixing #3014
@ColinHebert
Copy link
Contributor

So depending on how we want to deal with that, we can either force the key ID ARN (which is what AWS is going to return later) or document the fact that "we know it supports other things, but really it's only ARNs". WDYT?

@kwent
Copy link
Contributor Author

kwent commented Jan 23, 2018

If i understand correctly right now possibilities are:

ARN: arn:aws:kms:us-east-1:0000000000:key/0f94ded2-7c3c-406f-8e09-7de1b29cddec
KMS KEY identifier: 0f94ded2-7c3c-406f-8e09-7de1b29cddec
Alias: my_alias

If this is correct the user should be able to use those 3 possibilities. I don't see any pattern between those where i can restrict input except allowing any string.

@radeksimko radeksimko added bug Addresses a defect in current functionality. service/rds Issues and PRs that pertain to the rds service. labels Jan 23, 2018
@kwent
Copy link
Contributor Author

kwent commented Jan 30, 2018

@ColinHebert Satisfied by my answer ?

@aeschright aeschright requested a review from a team June 25, 2019 18:50
@bflad
Copy link
Contributor

bflad commented Aug 12, 2019

Hi @kwent 👋 Thank you for submitting this. While various AWS APIs may support multiple input values for KMS Keys, usually they return the full KMS Key ARN in the response. For two reasons, we typically just require the KMS Key ARN:

  • Terraform Providers are expected to not have the attribute value change between a plan (the configuration value in this case) and apply (the API response value in this case). These surface as diffs didn't match errors in Terraform 0.11 and earlier or Provider produced inconsistent result after apply errors in Terraform 0.12 when attempting to reference attributes that change like this. This is a fundamental design principal in Terraform and the Terraform AWS Provider must follow suit to prevent those errors.
  • Even without downstream references to the attribute that would generate the above errors, operators configuring the KMS Alias ID/ARN or Key ID will see a difference in their next Terraform run for the resource unless they also configure the lifecycle configuration block ignore_changes meta-argument. We would prefer to skip the complexity of documenting this every place where multiple values are possible on input and instead only allow the canonical input that would not require this workaround and potential confusion.

The aws_kms_alias data source and resource were augmented since this pull request was filed with a target_key_arn attribute to make these types of configurations easier and the usage of that is the preferred configuration method in these cases.

Since we would prefer not to introduce the above problems and ambiguity into the resource, we are going to close this. Thanks again for this contribution and hope to see others in the future.

@bflad bflad closed this Aug 12, 2019
@ghost
Copy link

ghost commented Nov 1, 2019

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!

@ghost ghost locked and limited conversation to collaborators Nov 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. service/rds Issues and PRs that pertain to the rds service.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants