-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
chore: do not show sensitive certificate details in Terraform cmd out… #9684
Conversation
Hello! I am a robot. It looks like you are a: @NickElliot, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 8 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataprocClusterIamPolicy |
Rerun these tests in REPLAYING mode to catch issues
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 7 insertions(+), 3 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packages
|
Gentle ping. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
marked a couple needed changes for this implementation
// Returned from API on all replicas | ||
Computed: true, | ||
Computed: true, | ||
Sensitive: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The individual sub fields that specifically contain the encrypted data should be flagged sensitive for this update, not the entire list object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Nick. Unfortunately this is a limitation of TF for these nested messages. Please see hashicorp/terraform-plugin-sdk#201. So I have to mark the whole message as sensitive.
I've tested the version which only marks the sensitive fields in the nested message. TF failed to hide them
Computed: true, | ||
Type: schema.TypeList, | ||
Computed: true, | ||
Sensitive: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Nick. Unfortunately this is a limitation of TF for these nested messages. Please see this issue. So I have to mark the whole message as sensitive.
I've tested the version which only marks the sensitive fields in the nested message. TF failed to hide them.
Friendly ping. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM then!
…oogleCloudPlatform#9684) * chore: do not show sensitive certificate details in Terraform cmd output. * Mark the whole replica_configuration as sensitive due to the private keys.
…oogleCloudPlatform#9684) * chore: do not show sensitive certificate details in Terraform cmd output. * Mark the whole replica_configuration as sensitive due to the private keys.
…oogleCloudPlatform#9684) * chore: do not show sensitive certificate details in Terraform cmd output. * Mark the whole replica_configuration as sensitive due to the private keys.
…oogleCloudPlatform#9684) * chore: do not show sensitive certificate details in Terraform cmd output. * Mark the whole replica_configuration as sensitive due to the private keys.
Mark some certificate-related information as sensitive so that they wouldn't be shown in the Terraform cmd output like
terraform destroy
.Release Note Template for Downstream PRs (will be copied)