Skip to content

Conversation

@ramonvermeulen
Copy link
Contributor

@ramonvermeulen ramonvermeulen commented Oct 29, 2025

This PR introduces support for nested write-only fields and adds write-only arguments for the google_monitoring_notification_channel resource.

Closing: hashicorp/terraform-provider-google#21855

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.

monitoring: add write-only variants (`auth_token_wo`, `password_wo`, `service_key_wo`) for `google_monitoring_notification_channel`

@modular-magician modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Oct 29, 2025
@ramonvermeulen ramonvermeulen force-pushed the f/24327-write-only-pointer-constraint-groups branch from fd2c99f to 0d5ec68 Compare October 29, 2025 17:32
@ramonvermeulen
Copy link
Contributor Author

ramonvermeulen commented Oct 29, 2025

When I do a local build, the ExactlyOneOf seems to have all the correct fields now:

ExactlyOneOf: []string{"sensitive_labels.0.auth_token", "sensitive_labels.0.password", "sensitive_labels.0.service_key", "sensitive_labels.0.auth_token_wo", "sensitive_labels.0.password_wo", "sensitive_labels.0.service_key_wo"},

@melinath

Let's discuss this feature further on this draft, I would like to validate with you if this aligns a bit with the original idea you had in mind for this feature.

@melinath

This comment was marked as outdated.

@github-actions
Copy link

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.

@melinath, 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.

@github-actions github-actions bot requested a review from melinath October 29, 2025 18:04
@melinath melinath marked this pull request as ready for review October 29, 2025 18:05
@melinath

This comment was marked as outdated.

@modular-magician modular-magician added awaiting-approval Pull requests that need reviewer's approval to run presubmit tests service/terraform and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Oct 29, 2025
@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@ramonvermeulen

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this looks like what I was imagining. It looks like the downstream generation worked fine / came out as expected, even for the case where multiple write only fields are added to the same constraint group 👍

Should be good to go once we have tests for the new fields.

@ramonvermeulen
Copy link
Contributor Author

ramonvermeulen commented Nov 10, 2025

Should be good to go once we have tests for the new fields.

Great, I will try to add these tests today!

Do we also want to add some documentation about this logic somewhere? At least I can imagine without context reading this code for the first time might be a bit confusing, e.g. "why the ... do they apply this pointer logic?".

On the other hand it is also not that big of a change.

EDIT:
While adding the tests in c93f109 I also noticed the flatteners and expanders do not get generated, most likely because it is a "nested" field. I remember we never implemented the full support for nested fields (yet).

I will see what I can do to make it work. I think it is a unique case in general, because this use-case uses a custom encoder and url_param_only: true at https://github.com/ramonvermeulen/magic-modules/blob/03b0d07a70f6937bdbece2ecd47a367d449a809e/mmv1/products/monitoring/NotificationChannel.yaml#L104 which doesn't generate an expander for sensitive_labels to begin with.

@github-actions github-actions bot requested a review from melinath November 10, 2025 12:03
…to nested fields and `url_param_only: true`)
@ramonvermeulen ramonvermeulen force-pushed the f/24327-write-only-pointer-constraint-groups branch from 03b0d07 to c93f109 Compare November 10, 2025 15:07
@ramonvermeulen
Copy link
Contributor Author

ramonvermeulen commented Nov 12, 2025

I will see what I can do to make it work.

I think I managed to address most of this in 8a48fca and ac2487d, unfortunately it is still quite some custom code, but that is also because sensitiveLabels seems to be quite a unique case. Can you retrigger the CI?

Tests on latest commit:

make testacc TEST=./google/services/monitoring TESTARGS='-run=TestAccMonitoringNotificationChannel_'
sh -c "'/Users/ramon/go/src/github.com/hashicorp/terraform-provider-google/scripts/gofmtcheck.sh'"
==> Checking that code complies with gofmt requirements...
go vet
TF_ACC_REFRESH_AFTER_APPLY=1 TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google/services/monitoring -v -run=TestAccMonitoringNotificationChannel_ -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccMonitoringNotificationChannel_notificationChannelBasicExample
=== PAUSE TestAccMonitoringNotificationChannel_notificationChannelBasicExample
=== RUN   TestAccMonitoringNotificationChannel_update
=== PAUSE TestAccMonitoringNotificationChannel_update
=== RUN   TestAccMonitoringNotificationChannel_updateLabels_slack
    resource_monitoring_notification_channel_test.go:57:
--- SKIP: TestAccMonitoringNotificationChannel_updateLabels_slack (0.00s)
=== RUN   TestAccMonitoringNotificationChannel_updateLabels
=== PAUSE TestAccMonitoringNotificationChannel_updateLabels
=== RUN   TestAccMonitoringNotificationChannel_updateSensitiveLabels_slack
    resource_monitoring_notification_channel_test.go:107:
--- SKIP: TestAccMonitoringNotificationChannel_updateSensitiveLabels_slack (0.00s)
=== RUN   TestAccMonitoringNotificationChannel_updateSensitiveLabels
=== PAUSE TestAccMonitoringNotificationChannel_updateSensitiveLabels
=== RUN   TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
=== PAUSE TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
=== RUN   TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo_slack
    resource_monitoring_notification_channel_test.go:216:
--- SKIP: TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo_slack (0.00s)
=== CONT  TestAccMonitoringNotificationChannel_notificationChannelBasicExample
=== CONT  TestAccMonitoringNotificationChannel_updateSensitiveLabels
=== CONT  TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
=== CONT  TestAccMonitoringNotificationChannel_updateLabels
=== CONT  TestAccMonitoringNotificationChannel_update
--- PASS: TestAccMonitoringNotificationChannel_notificationChannelBasicExample (6.64s)
--- PASS: TestAccMonitoringNotificationChannel_updateLabels (8.15s)
--- PASS: TestAccMonitoringNotificationChannel_updateSensitiveLabels (10.79s)
--- PASS: TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo (14.25s)
--- PASS: TestAccMonitoringNotificationChannel_update (14.40s)
PASS
ok      github.com/hashicorp/terraform-provider-google/google/services/monitoring       15.182s

I only think the documentation for write-only arguments is still not generated fully correct. I think it makes sense to wait a little bit with that before #15385 is merged?

We most likely need to add some changes to this file to enable support for the nested write-only docs: https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/templates/terraform/nested_property_documentation.html.markdown.tmpl.

@melinath
Copy link
Member

/gcbrun

@modular-magician modular-magician added awaiting-approval Pull requests that need reviewer's approval to run presubmit tests and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Jan 13, 2026
@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, went through and made some suggestions. Thanks again for your patience!

var sensitiveLabels = []string{"auth_token", "service_key", "password", "auth_token_wo", "service_key_wo", "password_wo", "auth_token_wo_version", "service_key_wo_version", "password_wo_version"}

func sensitiveLabelCustomizeDiff(_ context.Context, diff *schema.ResourceDiff, v interface{}) error {
for _, sl := range sensitiveLabels {
Copy link
Member

@melinath melinath Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like diff.Get does have access to the write-only fields inside sensitive labels (which surprises me? but I guess it makes sense because it's plan-time?) but the comparison needs to be with the non-write-only version in the non-sensitive labels.

(I think renaming the vars might be useful as well)
(I don't see any unit tests for this - could be nice to add but I'm fine with skipping.)

Maybe something like this? The suggestion feature isn't working quite right so it's all getting put as replacing this line.

Suggested change
for _, sl := range sensitiveLabels {
for _, sl := range sensitiveLabels {
l := strings.TrimSuffix(sl, "_wo")
l := strings.TrimSuffix(l, "_wo_version")
val := diff.Get("labels." + l).(string)
sensitiveVal := diff.Get("sensitive_labels.0." + sl).(string)
if val != "" && sensitiveVal != "" {
return fmt.Errorf("Sensitive label %q cannot be set at the same time as label %q.", sl, l)
}
}


for configKey, configVal := range configuredSensitiveLabels {
if strings.HasSuffix(configKey, "_wo_version") {
// Convert: field_wo_version → fieldWoVersion
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will fail for auth_token - it should be authTokenWoVersion but this would make it auth_tokenWoVersion.

That said - I think we shouldn't be building this object here at all. We only need to set values in the decoder/flattener for fields that are read from the API - but none of these are. So, I think we can set ignore_read: true on the sensitiveLabels field and avoid generating a flattener for it at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could be an option, but wouldn't that introduce a breaking change? E.g. users who now depend on the value of the sensitive labels? And aside from that it will most likely produce a diff for existing state?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sensitiveLabels is currently ignore_read on main, so keeping that the case will maintain the existing behavior - that is, it won't be a breaking change. You can see in the generated diff that the flattener is getting added by this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er, slight correction, sorry, this is all a bit messy. sensitiveLabels is url_param_only on main.

Generation of flatteners is done by iterating over ReadProperties, which returns all GettableProperties except ones with IgnoreRead == true, and GettableProperties excludes UrlParamOnly.

So, the effect is the same in terms of flattening.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #16219 to make the flattener generation logic easier to understand.

Comment on lines +48 to +54
if configuredLabels, ok := d.Get("labels").(map[string]interface{}); ok {
for _, field := range []string{"auth_token", "password", "service_key"} {
if configVal, exists := configuredLabels[field]; exists && configVal != nil && configVal != "" {
labels[field] = configVal
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the logic here isn't quite right; if I set sensitive_labels.password I get a permadiff on labels.password. If the key is set in sensitive_labels, it should be removed from the labels. I think if that's changed, this will match the previous behavior.

Copy link
Contributor Author

@ramonvermeulen ramonvermeulen Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I test it locally I do not have that problem:

resource "google_monitoring_notification_channel" "slack" {
  display_name = "TFTest Basicauth Channel"
  type         = "webhook_basicauth"
  labels = {
    "username" = "foo"
    "url"      = "https://foo.com/bar"
  }
  sensitive_labels {
    password = "woop"
  }
}
╭─ ~/projects/personal/terraform_test_suite                                                                                                                                                                                      󰊭 xxxxxxx ─╮
╰─❮ TF_CLI_CONFIG_FILE="$(pwd)/tf-dev-override.tfrc" terraform apply -auto-approve                                                                                                                                                         ─╯


│ Warning: Provider development overrides are in effect

│ The following provider development overrides are set in the CLI configuration:
│  - hashicorp/google in /Users/ramon/go/bin
│  - hashicorp/google-beta in /Users/ramon/go/bin

│ The behavior may therefore not match any released version of the provider and applying changes may cause the state to become incompatible with published releases.


Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # google_monitoring_notification_channel.slack will be created
  + resource "google_monitoring_notification_channel" "slack" {
      + display_name        = "TFTest Basicauth Channel"
      + enabled             = true
      + force_delete        = false
      + id                  = (known after apply)
      + labels              = {
          + "url"      = "https://foo.com/bar"
          + "username" = "foo"
        }
      + name                = (known after apply)
      + project             = "<masked>"
      + type                = "webhook_basicauth"
      + verification_status = (known after apply)

      + sensitive_labels {
          + auth_token_wo  = (write-only attribute)
          + password       = (sensitive value)
          + password_wo    = (write-only attribute)
          + service_key_wo = (write-only attribute)
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.
google_monitoring_notification_channel.slack: Creating...
google_monitoring_notification_channel.slack: Creation complete after 1s [id=projects/<masked>/notificationChannels/14550561117501074655]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

╭─ ~/projects/personal/terraform_test_suite                                                                                                                                                                                      󰊭 xxxxxxx ─╮
╰─❮ TF_CLI_CONFIG_FILE="$(pwd)/tf-dev-override.tfrc" terraform plan                                                                                                                                                                        ─╯


│ Warning: Provider development overrides are in effect

│ The following provider development overrides are set in the CLI configuration:
│  - hashicorp/google in /Users/ramon/go/bin
│  - hashicorp/google-beta in /Users/ramon/go/bin

│ The behavior may therefore not match any released version of the provider and applying changes may cause the state to become incompatible with published releases.

google_monitoring_notification_channel.slack: Refreshing state... [id=projects/<masked>/notificationChannels/14550561117501074655]

No changes. Your infrastructure matches the configuration.

Terraform has compared your real infrastructure against your configuration and found no differences, so no changes are needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I must have been testing this with the previous section related to sensitive labels deleted.

@modular-magician modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Jan 14, 2026
@github-actions github-actions bot requested a review from melinath January 14, 2026 13:53
@ramonvermeulen
Copy link
Contributor Author

fix: apply quick suggestions from code review

Thanks for your review! I just committed the obvious code suggestions in e1eb47d. I will try to find some time in the next coming days (most likely Friday) to go in-depth over the rest of the comments.

@modular-magician modular-magician removed the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Jan 14, 2026
@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@ramonvermeulen ramonvermeulen force-pushed the f/24327-write-only-pointer-constraint-groups branch from e1eb47d to 6bb8dba Compare January 20, 2026 10:45
@modular-magician modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Jan 20, 2026
@ramonvermeulen ramonvermeulen force-pushed the f/24327-write-only-pointer-constraint-groups branch from 2fa8999 to bc39ae4 Compare January 20, 2026 12:14
@ramonvermeulen
Copy link
Contributor Author

ramonvermeulen commented Jan 20, 2026

@melinath

I applied some of the feedback, but not everything. I rely heavily on at least ensuring the existing acceptance tests keep succeeding with the changes.

─ ~/go/src/github.com/hashicorp/terraform-provider-google main *3 !9                                                                                                                                                 ▼  1.24.5 󰊭 xxxxxx ─╮
╰─❯ make testacc TEST=./google/services/monitoring TESTARGS='-run=TestAccMonitoringNotificationChannel_'                                                                                                                                   ─╯
sh -c "'/Users/ramon/go/src/github.com/hashicorp/terraform-provider-google/scripts/gofmtcheck.sh'"
==> Checking that code complies with gofmt requirements...
go vet
TF_ACC_REFRESH_AFTER_APPLY=1 TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google/services/monitoring -v -run=TestAccMonitoringNotificationChannel_ -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccMonitoringNotificationChannel_notificationChannelBasicExample
=== PAUSE TestAccMonitoringNotificationChannel_notificationChannelBasicExample
=== RUN   TestAccMonitoringNotificationChannel_update
=== PAUSE TestAccMonitoringNotificationChannel_update
=== RUN   TestAccMonitoringNotificationChannel_updateLabels_slack
    resource_monitoring_notification_channel_test.go:57:
--- SKIP: TestAccMonitoringNotificationChannel_updateLabels_slack (0.00s)
=== RUN   TestAccMonitoringNotificationChannel_updateLabels
=== PAUSE TestAccMonitoringNotificationChannel_updateLabels
=== RUN   TestAccMonitoringNotificationChannel_updateSensitiveLabels_slack
    resource_monitoring_notification_channel_test.go:107:
--- SKIP: TestAccMonitoringNotificationChannel_updateSensitiveLabels_slack (0.00s)
=== RUN   TestAccMonitoringNotificationChannel_updateSensitiveLabels
=== PAUSE TestAccMonitoringNotificationChannel_updateSensitiveLabels
=== RUN   TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
=== PAUSE TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
=== RUN   TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo_slack
    resource_monitoring_notification_channel_test.go:216:
--- SKIP: TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo_slack (0.00s)
=== CONT  TestAccMonitoringNotificationChannel_notificationChannelBasicExample
=== CONT  TestAccMonitoringNotificationChannel_updateSensitiveLabels
=== CONT  TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
=== CONT  TestAccMonitoringNotificationChannel_updateLabels
=== CONT  TestAccMonitoringNotificationChannel_update
--- PASS: TestAccMonitoringNotificationChannel_updateSensitiveLabels (9.99s)
--- PASS: TestAccMonitoringNotificationChannel_updateLabels (11.53s)
--- PASS: TestAccMonitoringNotificationChannel_notificationChannelBasicExample (11.75s)
--- PASS: TestAccMonitoringNotificationChannel_update (13.06s)
--- PASS: TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo (18.62s)
PASS
ok      github.com/hashicorp/terraform-provider-google/google/services/monitoring       19.420s

Let's generate a new diff and see what is left to change, at least all acceptance tests succeed on the latest commit.

@modular-magician modular-magician removed the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Jan 20, 2026
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 9 files changed, 522 insertions(+), 35 deletions(-))
google-beta provider: Diff ( 9 files changed, 522 insertions(+), 35 deletions(-))
terraform-google-conversion: Diff ( 7 files changed, 372 insertions(+), 52 deletions(-))

@modular-magician
Copy link
Collaborator

Non-exercised tests

🔴 Tests were added that are skipped in VCR:

  • TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo_slack

Tests analytics

Total tests: 5933
Passed tests: 5294
Skipped tests: 634
Affected tests: 5

Click here to see the affected service packages

All service packages are affected

Action taken

Found 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccAccessContextManager__service_perimeter_dry_run_egress_policy
  • TestAccAccessContextManager__service_perimeter_dry_run_ingress_policy
  • TestAccComputeNetwork_bgpModeAndMedInteractions
  • TestAccComputeNetwork_networkBgpStandardModeDeleteMed
  • TestAccComputeNetwork_networkBgpStandardModeDeleteMedExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccAccessContextManager__access_level [Debug log]
TestAccAccessContextManager__access_level_custom [Debug log]
TestAccAccessContextManager__access_level_full [Debug log]
TestAccAccessContextManager__access_levels [Debug log]
TestAccAccessContextManager__access_policy [Debug log]
TestAccAccessContextManager__access_policy_scoped [Debug log]
TestAccAccessContextManager__authorized_orgs_desc [Debug log]
TestAccAccessContextManager__service_perimeter [Debug log]
TestAccAccessContextManager__service_perimeter_dry_run_egress_policy [Debug log]
TestAccAccessContextManager__service_perimeter_dry_run_ingress_policy [Debug log]
TestAccAccessContextManager__service_perimeter_update [Debug log]
TestAccAccessContextManager__service_perimeters [Debug log]
TestAccComputeNetwork_bgpModeAndMedInteractions [Debug log]
TestAccComputeNetwork_networkBgpStandardModeDeleteMed [Debug log]
TestAccComputeNetwork_networkBgpStandardModeDeleteMedExample [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.


🔴 Tests failed during RECORDING mode:
TestAccAccessContextManager__access_level_condition [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

@github-actions
Copy link

@GoogleCloudPlatform/terraform-team @melinath This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

responded on the threads above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants