-
Notifications
You must be signed in to change notification settings - Fork 415
[datadog_synthetics_global_variable] feat: add write-only value support #3382
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
Conversation
42142e3 to
25fe8b0
Compare
6314d5b to
75163e8
Compare
25fe8b0 to
b2a5d4b
Compare
75163e8 to
8581175
Compare
| stringvalidator.AlsoRequires(frameworkPath.Expressions{ | ||
| frameworkPath.MatchRoot(valueConfig.WriteOnlyAttr), | ||
| }...), | ||
| }, |
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.
ℹ️ info: we can't use CreateWriteOnlySecretAttributes because of secure conditional, which is unique to this resource
8581175 to
a0542d7
Compare
a0542d7 to
4fc53db
Compare
Matt-Cam
left a comment
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.
Overall code LGTM. Two main questions:
1. Testing
Were these changes validated against a locally spun up terraform-provider hitting staging?
2. Why are we doing this?
Does this provide any benefit over the existing secure attribute behavior, which already keeps secret values out of state? It seems like this mainly gives users option to hide from state a non-secure variable? Is this being done because it's best practice or for demonstration purposes?
|
thank you for the review @Matt-Cam 🙏 about your questions
Validated it locally hitting frog prod org.
Not sure I understand "existing secure attribute behavior, which already keeps secret values out of state" part. The current behavior saves secret to the terraform state file even if the option is And so the main benefit of this change is that we are not going to store it in state anymore and we're going to use native terraform framework behavior that ensures this. |
|
@LiuVII thanks for the response 🙏🏼
Ahh, okay I didn't realize that we were storing in state even when |
Matt-Cam
left a comment
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 with one last question
The main issue is perpetual drift. Here's what happens if you don't save value to state when
This means every single plan would show: ~ resource "datadog_synthetics_global_variable" "example" {
~ value = null -> "my-secret" # forces replacement or update
}The user would have to either:
Why write-only solves this: |
Makes sense, thanks for this! All good on my side 🚀 |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
Follow-up #3210 to show-case how WO argument can be implemented using some WO helpers.
APIR-2446
Summary
Adds write-only attribute support to
datadog_synthetics_global_variableresource for Terraform 1.11+, enabling secure secret handling without state file exposure.Changes
value_wo(write-only) andvalue_wo_version(trigger) attributes alongside existingvalueWriteOnlySecretHandlerhelpers fromfwutilspackage for secret retrieval logicValidateConfigto handle mutual exclusion betweenvalue/value_wowhile allowing neither for FIDO variablesPreferWriteOnlyAttributewarning for secure variables still using plaintextvalueTest Plan
TestAccDatadogSyntheticsGlobalVariableWriteOnly_Basic- create with write-only valueTestAccDatadogSyntheticsGlobalVariableWriteOnly_Updated- update via version triggerTestAccDatadogSyntheticsGlobalVariableWriteOnlySecure_Basic- secure + write-only