GoogleCloudPlatform
feat: support for nested write-only arguments + write-only arguments for `google_monitoring_notification_channel`
This PR introduces support for nested write-only fields and adds write-only arguments for the google_monitoring_notification_channel resource.
Closing: https://github.com/hashicorp/terraform-provider-google/issues/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`
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.
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[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
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 ( 7 files changed, 81 insertions(+), 13 deletions(-))
google-beta provider: Diff ( 7 files changed, 81 insertions(+), 13 deletions(-))
Missing test report
Your PR includes resource fields which are not covered by any test.
Resource: google_monitoring_notification_channel (14 total tests)
Please add an acceptance test which includes these fields. The test should include the following:
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.
google_monitoring_notification_channel- Expected Document Path:
/website/docs/r/monitoring_notification_channel.html.markdown - Fields:
[sensitive_labels.auth_token_wo sensitive_labels.auth_token_wo_version sensitive_labels.password_wo sensitive_labels.password_wo_version sensitive_labels.service_key_wo sensitive_labels.service_key_wo_version]
- Expected Document Path:
Tests analytics
Total tests: 121 Passed tests: 116 Skipped tests: 5 Affected tests: 0
Click here to see the affected service packages
- bigquerydatatransfer
- cloudbuild
- firestore
- monitoring
🟢 All tests passed!
View the build log
Resource:
google_monitoring_notification_channel(14 total tests) Please add an acceptance test which includes these fields. The test should include the following: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 } }
Will focus on these later, once we are happy with the implementation of the constraint group registry itself.
Small FYI, I am out-of-office until next Monday so I won't be able to work on this feature for the rest of this week.
@melinath This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.
@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.
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 https://github.com/GoogleCloudPlatform/magic-modules/pull/15538/commits/c93f1091fa5e09f961c3eca3bb24360d8241666f 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.
I will see what I can do to make it work.
I think I managed to address most of this in https://github.com/GoogleCloudPlatform/magic-modules/pull/15538/commits/8a48fca54f22dcab98e16bb8ebce2326d7bf531b and https://github.com/GoogleCloudPlatform/magic-modules/pull/15538/commits/ac2487d67d0f67c3347a2c69d774e3cce0bbfb06, 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 https://github.com/GoogleCloudPlatform/magic-modules/pull/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 This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.
basically if we could avoid solving the nested write-only arguments problem for now that would be great.
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, 483 insertions(+), 33 deletions(-))
google-beta provider: Diff ( 9 files changed, 483 insertions(+), 33 deletions(-))
terraform-google-conversion: Diff ( 6 files changed, 384 insertions(+), 30 deletions(-))
Missing doc report (experimental)
The following resources have fields missing in documents.
google_monitoring_notification_channel- Expected Document Path:
/website/docs/r/monitoring_notification_channel.html.markdown - Fields:
[sensitive_labels.auth_token_wo sensitive_labels.auth_token_wo_version sensitive_labels.password_wo sensitive_labels.password_wo_version sensitive_labels.service_key_wo sensitive_labels.service_key_wo_version]
- Expected Document Path:
Non-exercised tests
🔴 Tests were added that are skipped in VCR:
- TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo_slack
Tests analytics
Total tests: 123 Passed tests: 117 Skipped tests: 6 Affected tests: 0
Click here to see the affected service packages
- bigquerydatatransfer
- cloudbuild
- firestore
- monitoring
🟢 All tests passed!
View the build log
basically if we could avoid solving the nested write-only arguments problem for now that would be great.
Yes, I agree that it is quite a lot of changes in one PR, which might not be the best idea. Unfortunately, I am not sure if there is another use-case on which we can test the constraint group registry apart from the monitoring notification channel without arguments. I might spend some time searching through the issue tracker to see if there are any new write-only requests.
I also forgot that this wasn't working yet for nested arguments and figured that out while implementing the tests. How would you suggest moving forward? I am fine with splitting up the PR into multiple more subject-focused PRs. In my opinion, there are four things:
- Constraint group registry
- Templating changes so that write-only arguments are properly generated for nested fields
- Templating changes to the markdown generation for nested fields (still to do)
- Specific encoder and decoder changes for the monitoring notification channel to make the write-only argument generation work for this use-case.
The last point has quite a few edge cases because sensitiveLabels is not an actual API field and it needs to target the labels API field. This requires some complex decoder logic because during decoding you need to know if labels, sensitiveLabels, or the write-only variant was used.
That seems like a reasonable split. I'd be fine with reviewing the constraint group logic without a specific relevant write-only field being added, since it seems to work as expected here. We'd expect to see zero diff on that PR, and if there were any bugs we could fix them separately in a more targeted PR.
The templating changes for nested changes (go + markdown) might make sense to do in the same PR, but with some other nested sensitive field that's not as complicated as google_monitoring_notification_channel's.
The templating changes for nested changes (go + markdown) might make sense to do in the same PR, but with some other nested sensitive field that's not as complicated as google_monitoring_notification_channel's.
Check, for this PR I really focussed on getting the acceptance tests to pass, but I still get the point that it is a lot of changes at the same time potentially breaking something.
I opened a PR for the constraint group registry at: https://github.com/GoogleCloudPlatform/magic-modules/pull/15741
When we have a good use-case I can make a separate PR for the nested write-only field and documentation generation. Shall I close this PR?
Thanks, I know it's more effort to split it into PRs but it will make it easier/safer to review. I want to be extra cautious since we've had some issues with write-only field conversions in the past.
This PR isn't that long; if you'd like to refocus it that would be fine, but if you'd prefer to close it that's fine too.
@melinath This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.
This PR isn't that long; if you'd like to refocus it that would be fine, but if you'd prefer to close it that's fine too.
I leave this up to you, I just rebased and changed the description. Since the constraint group registry is merged (apart from one required change in https://github.com/GoogleCloudPlatform/magic-modules/pull/15743#issuecomment-3551013595), this PR now only includes the nested write-only generation + the write-only argument support for thegoogle_monitoring_notification_channel.
The markdown generation still needs to be implemented for the nested write-only arguments.
@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.
mm.... I guess let's just keep it all in this PR and add the nested write-only markdown generation handling changes here. It's going to be a bit of a pain to review but it'll be fine.
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 ( 6 files changed, 463 insertions(+), 41 deletions(-))
google-beta provider: Diff ( 6 files changed, 463 insertions(+), 41 deletions(-))
terraform-google-conversion: Diff ( 6 files changed, 381 insertions(+), 30 deletions(-))
Missing doc report (experimental)
The following resources have fields missing in documents.
google_monitoring_notification_channel- Expected Document Path:
/website/docs/r/monitoring_notification_channel.html.markdown - Fields:
[sensitive_labels.auth_token_wo sensitive_labels.auth_token_wo_version sensitive_labels.password_wo sensitive_labels.password_wo_version sensitive_labels.service_key_wo sensitive_labels.service_key_wo_version]
- Expected Document Path:
Non-exercised tests
🔴 Tests were added that are skipped in VCR:
- TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo_slack
Tests analytics
Total tests: 60 Passed tests: 56 Skipped tests: 4 Affected tests: 0
Click here to see the affected service packages
- monitoring
🟢 All tests passed!
View the build log
It's possible I'm missing something, but it looks like previously, we weren't storing sensitiveLabels in state, and after this change, we will be. Is that correct to your understanding? If so, why the change?
It might be okay, since sensitive fields end up in state in general. However, we'll need to make sure that if wo_version is set, we're not setting a value for the original field.
I'm concerned that this might not be possible to implement properly, though, similar to https://github.com/GoogleCloudPlatform/magic-modules/pull/15443#pullrequestreview-3462032583. It's difficult to prevent setting a sensitive field (that is returned from the API) based on wo_version being set reliably enough for security assurances (because we can't tell on import).
It's complex enough that I'm not 100% sure; you might have a better idea than I do at the moment. I can take a closer look tomorrow hopefully.
It's possible I'm missing something, but it looks like previously, we weren't storing sensitiveLabels in state, and after this change, we will be. Is that correct to your understanding? If so, why the change?
It is indeed unfortunately quite a complex use-case, especially because sensitiveLabels itself already uses a different API field (labels) under-the-hood, and is not a concept in the API itself. So the write-only variants would also need to map to that. This makes it possible to configure the labels API argument in 3 different ways (normal, sensitive, and write-only).
In the old scenario sensitive labels were hidden from logs (therefore sensitive) but still written to state. I just tested this by applying this config using the latest provider version:
Config (used provider version v7.5.0, but nothing changed in latest)
resource "google_monitoring_notification_channel" "default" {
display_name = "Test Webhook Basci Auth"
type = "webhook_basicauth"
labels = {
"url" = "https://example.com"
"username" = "foo"
}
sensitive_labels {
password = "bar"
}
}
tfstate after apply:
{
"version": 4,
"terraform_version": "1.14.0",
"serial": 5,
"lineage": "89e632fd-afde-ed71-e732-ac60b27cd956",
"outputs": {},
"resources": [
{
"mode": "managed",
"type": "google_monitoring_notification_channel",
"name": "default",
"provider": "provider[\"registry.terraform.io/hashicorp/google\"]",
"instances": [
{
"schema_version": 0,
"attributes": {
"description": "",
"display_name": "Test Webhook Basci Auth",
"enabled": true,
"force_delete": false,
"id": "projects/<masked>/notificationChannels/4469459461553005592",
"labels": {
"url": "https://example.com",
"username": "foo"
},
"name": "projects/<masked>/notificationChannels/4469459461553005592",
"project": "<masked>",
"sensitive_labels": [
{
"auth_token": "",
"password": "bar",
"service_key": ""
}
],
"timeouts": null,
"type": "webhook_basicauth",
"user_labels": {},
"verification_status": ""
},
"sensitive_attributes": [
[
{
"type": "get_attr",
"value": "sensitive_labels"
},
{
"type": "index",
"value": {
"value": 0,
"type": "number"
}
},
{
"type": "get_attr",
"value": "auth_token"
}
],
[
{
"type": "get_attr",
"value": "sensitive_labels"
},
{
"type": "index",
"value": {
"value": 0,
"type": "number"
}
},
{
"type": "get_attr",
"value": "password"
}
],
[
{
"type": "get_attr",
"value": "sensitive_labels"
},
{
"type": "index",
"value": {
"value": 0,
"type": "number"
}
},
{
"type": "get_attr",
"value": "service_key"
}
]
],
"identity_schema_version": 0,
"private": "xxxxx"
}
]
}
],
"check_results": null
}
The pain point in the implementation is especially in the decoding, because esentially all you get from the API is the labels property. The decoder must be "smart enougth" to be able to determine if it was configured via the labels, sensitive_labels or sensitive_labels write-only variant. Hopefully this explanation helps a bit with understanding the use-case.
I will try to work on the documentation generation over the next days, to get that in a better shape as well.
@melinath
In https://github.com/GoogleCloudPlatform/magic-modules/pull/15538/commits/7afa9fefec1704d66f63d91fbfcd336141bb86b9 I implemented the documentation generation, and for me it seems to generate the write-only argument documentation for nested fields correct. In https://github.com/GoogleCloudPlatform/magic-modules/pull/15538/commits/eae8f1885566ead8822099ae32ce5e5ae95ebdf8 I removed the alphabetical sorting that I had in the first implementation, because with the sorting it affects almost all resources (because non-write only resources would also be affected by sorting all nested properties).
Let's run the CI so that we can review the generated output?
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 ( 8 files changed, 536 insertions(+), 24 deletions(-))
google-beta provider: Diff ( 8 files changed, 536 insertions(+), 24 deletions(-))
terraform-google-conversion: Diff ( 6 files changed, 381 insertions(+), 30 deletions(-))