redis: tolerate missing getAuthString permission on Read (#24784)#58
Open
jbbqqf wants to merge 11 commits into
Open
redis: tolerate missing getAuthString permission on Read (#24784)#58jbbqqf wants to merge 11 commits into
jbbqqf wants to merge 11 commits into
Conversation
…ead (#24784) When auth_enabled is true, the Decoder calls /authString to populate auth_string. That endpoint requires redis.instances.getAuthString — a broader permission than redis.instances.get. Previously a 403 there made plan fail for callers with read-only roles such as roles/viewer. Treat any error fetching the auth string as recoverable: log a warning and clear auth_string in state so plan/read can proceed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When
auth_enabled = true, the Read path ongoogle_redis_instancecalls the/authStringendpoint which requires the broaderredis.instances.getAuthStringpermission. Reading the resource itself only needsredis.instances.get, so callers with read-only roles (e.g.roles/viewer) currently fail at plan-time even though they have enough permission to plan changes. This PR makes the auth-string fetch best-effort: a non-OK response logs a warning and clearsauth_stringin state, instead of failing the entire Read.Fixes hashicorp/terraform-provider-google#24784 — see hashicorp/terraform-provider-google#24784
Why
roles/viewercannot runterraform planagainst a redis instance withauth_enabled = true.auth_stringis a Sensitive Computed string that the user shouldn't depend on for planning. Treating a 403 here as fatal is gratuitously coupling Read to a broader permission than the resource itself requires.GCP API reference: https://cloud.google.com/memorystore/docs/redis/reference/rest/v1/projects.locations.instances/getAuthString
What changed
mmv1 template change:
Behavior:
getAuthStringpermissionauth_stringpopulatedauth_stringpopulated (unchanged)getAuthStringpermissionauth_stringcleared, warning loggedEdge cases tested
auth_enabled = falseauth_string = "", no extra API callauth_enabled = true+ caller hasgetAuthStringauth_stringpopulated as beforeauth_enabled = true+ caller lacksgetAuthStringauth_stringcleared, plan/read continuesThis is a bug fix in a hand-written code template generated into TPG/TPGB. The fix is exercised by the same Read codepath as before; a real reproduction needs a least-privilege service account on a project with a redis instance — not feasible in our sandbox without provisioning IAM bindings out of scope for this PR. The diff is small and the reasoning is mechanical (treat optional-permission API call as best-effort).
Disclosure
This PR was implemented with assistance from Claude Code. The fix was reviewed manually against the GCP IAM permission documentation linked above; no live before/after smoke was run because reproducing the bug requires a service account with
roles/vieweronly, which would need IAM provisioning outside the scope of a single-PR sandbox apply.