Better sensitive value handling in Google pub/sub logging provider #376
Conversation
There was a problem hiding this comment.
Thanks @robwil for opening this PR.
The code looks fine to me. I left one comment which is an external conversation that I thought might be interesting to share. Otherwise it would be good if we could validate this change set with a test. Is that something you can add please.
| - **topic** (String) The Google Cloud Pub/Sub topic to which logs will be published | ||
| - **user** (String) Your Google Cloud Platform service account email address. The `client_email` field in your service account authentication JSON | ||
|
|
||
| Optional: |
There was a problem hiding this comment.
So this was actually quite interesting to me, so I raised a query with the official terraform documentation maintainers:
hashicorp/terraform-plugin-docs#42
NOTE: this isn't a blocker to the PR getting approved/merged (as it's outside the control of both the contributor and ourselves as maintainers of this terraform provider), it's just something I noticed and thought I'd share.
There was a problem hiding this comment.
One thing worth noting here is that the field does in fact become optional from the perspective of the user, since if you don't explicitly pass it then it will default to "" (empty string). In practice, we can leave secret_key out of the TF config block entirely.
But I do see your point that since it will always have a value, and is required, it is a little strange to call it "optional" in the documentation.
|
The reason I left a test out is because none of the existing envvar overrides are covered with tests, so there wasn't a clear example to follow (as someone unfamiliar with the test setup here). But I can take a shot at it! |
|
I just added an attempt at a unit test. I didn't see too many other examples of this, but the way I did it seemed like the most straight-forward way to test that |
| defer func() { | ||
| os.Setenv(envVarKey, originalEnvValue) | ||
| }() |
There was a problem hiding this comment.
I appreciate this detail 🙂
This allows users to provide secret values for the Google pub/sub log integration, in a similar way that is already implemented for BigQuery and GCS (via an optional environment variable). Importantly, it also marks the private key as a sensitive value so it won't be logged during plan / apply.
cc @jw2340