-
Notifications
You must be signed in to change notification settings - Fork 2.1k
monitoring: add write-only support for notification_channel sensitive_labels #15524
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
monitoring: add write-only support for notification_channel sensitive_labels #15524
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. Googlers: For automatic test runs see go/terraform-auto-test-runs. @c2thorn, 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. |
da203e2 to
1c5381c
Compare
|
Hey 👋 we're getting the CLA sorted out sorry! |
|
Hi @GraceAtwood, Thanks a lot for working on this implementation! Today I started working on a first implementation for hashicorp/terraform-provider-google#24327 in #15538, making it also possible to do this via Hopefully we can get hashicorp/terraform-provider-google#24327 resolved soon! If this feature is really high-prio, please go ahead and use |
|
@c2thorn This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
|
@GoogleCloudPlatform/terraform-team @c2thorn This PR has been waiting for review for 1 week. Please take a look! Use the label |
|
pausing reminders while waiting on #15538 and CLAs |
|
@ramonvermeulen @c2thorn hey yall! Wrangled the lawyers to get our CCLA sorted out and signed with google -> anthropic! Thanks for the responses! I made this PR because this limitation is causing us to emit a secret into tf state in an otherwise pretty secure set up. I'd prefer to send this fix sooner rather than later if possible, but I promise not to leave it at that and I'll subscribe to that other PR and come back to remove the Also, looks like the CLA check doesn't like claude on the co-authored line so I'll drop that. |
97c044e to
f8f10e8
Compare
|
@BBBmau do you mind taking an initial pass at this new WO support? I'm not very familiar with the feature yet. |
| - 'sensitive_labels.0.auth_token' | ||
| - 'sensitive_labels.0.password' | ||
| - 'sensitive_labels.0.service_key' | ||
| - name: 'authTokenWo' |
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.
explcitily adding the write-only isn't needed. You can add write-only support by adding write_only: true on the desired field in this case: authToken
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.
@BBBmau @melinath see #15538 (comment), unfortunately this isn't fully supported yet for fields with constraint groups. The PR I linked solves the constraint group issues, but while writing tests I encountered some other issues, because it is a nested field and url_param_only is set to true for this specific sensetive_labels implementation. I will see what I can do, because the goal is to make write_only: true generation work for any type of write-only use-case.
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.
Am I following correctly then that there's no change I need to make 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.
We'll only want to move forward with one of the PRs, and we'll definitely want to use the new write_only rather than write_only_legacy.
I'd ideally like to separate the general "handling of nested write_only fields" changes from "handling of notification_channel sensitive labels" to make things easier to review / less risky.
Given these are both community contributions (thanks both of you!) we could go either way. Currently I'd be inclined to stick with @ramonvermeulen's PR since it's pretty far along, but I could be convinced otherwise.
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.
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.
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.
@GraceAtwood appreciate your patience and efforts on this PR. At this point this can be closed in favor of #15538.
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.
Sounds good thanks! closed this one
|
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_monitoring_notification_channel" "primary" {
sensitive_labels {
auth_token_wo = # value needed
auth_token_wo_version = # value needed
password_wo = # value needed
password_wo_version = # value needed
service_key_wo = # value needed
service_key_wo_version = # value needed
}
}
Missing doc report (experimental)The following resources have fields missing in documents.
|
Tests analyticsTotal tests: 58 Click here to see the affected service packages
🟢 All tests passed! View the build log |
…labels Implements write-only (_wo) variants for all three sensitive label fields: - auth_token_wo/auth_token_wo_version (Slack) - password_wo/password_wo_version (webhook_basicauth) - service_key_wo/service_key_wo_version (PagerDuty) These fields use WriteOnly: true which prevents secrets from being stored in Terraform state files, addressing a long-standing security concern. Users can now choose between: - sensitive_labels.service_key (hidden from plan, stored in state) - sensitive_labels.service_key_wo (hidden from plan, NOT in state) Implementation follows the pattern from SecretVersion resource using write_only_legacy for manual control over field generation. Fixes: hashicorp/terraform-provider-google#21855 Tested-by: Code generation successful for both GA and beta providers
f8f10e8 to
c1ed183
Compare
|
Just rebased main since this is kind of old now |
- Add acceptance tests for write-only sensitive_labels fields (service_key_wo, password_wo, auth_token_wo) - Fix version field attributes: replace url_param_only with immutable/ignore_read to match UptimeCheckConfig pattern - Add required_with back-references for version fields
|
Added acceptance tests and fixed documentation generation for the write-only fields per the Modular Magician's feedback. |
Fixes hashicorp/terraform-provider-google#21855
Summary
Adds write-only (
_wo) variants for all three sensitive label fields ingoogle_monitoring_notification_channel:auth_token_wo/auth_token_wo_version(Slack)password_wo/password_wo_version(webhook_basicauth)service_key_wo/service_key_wo_version(PagerDuty)These fields use
WriteOnly: truewhich prevents secrets from being stored in Terraform state files, addressing a long-standing security concern.Implementation
Users can now choose between:
sensitive_labels.service_key- Hidden from plan output, but stored in state (backwards compatible)sensitive_labels.service_key_wo- Hidden from plan output AND not stored in state (Terraform >= 1.11)This implementation follows the pattern from
google_secret_manager_secret_versionusingwrite_only_legacyfor manual field generation to avoid conflicts with the existingexactly_one_ofconstraints (see hashicorp/terraform-provider-google#24327).Changes
write_only_legacy: true_wofield values to API_wofields from state on readTesting
WriteOnly: trueon_wofieldsRelease Note Template for Downstream PRs (will be copied)
🤖 Generated with Claude Code