feat: add support for impersonation #1460
Conversation
|
~361 lines of this PR are just cleaning up our e2e authentication tests to be table tests, and adding a matching impersonation test for each authentication type. |
|
Also, I've updated our CI pipeline to support impersonation as well. |
internal/proxy/proxy.go
Outdated
| TargetPrincipal: c.ImpersonateTarget, | ||
| Delegates: c.ImpersonateDelegates, | ||
| Scopes: []string{ | ||
| sqladmin.CloudPlatformScope, |
There was a problem hiding this comment.
Do we need CloudPlatform here?
There was a problem hiding this comment.
Let me check on that -- we did for the e2e tests using the token.
There was a problem hiding this comment.
We don't need it here. It's the token source that gets the impersonated token that needs this scope. Removed it here, and removed the SQL Admin scope from the token source in the tests.
| func (c *Config) DialerOptions(l cloudsql.Logger) ([]cloudsqlconn.Option, error) { | ||
| opts := []cloudsqlconn.Option{ | ||
| cloudsqlconn.WithUserAgent(c.UserAgent), | ||
| func (c *Config) credentialsOpt(l cloudsql.Logger) (cloudsqlconn.Option, error) { |
There was a problem hiding this comment.
This doesn't look like it modifies c in anyway. Would it be better to take c as an arg instead?
There was a problem hiding this comment.
Yes, I'm in favor of that idea.
There was a problem hiding this comment.
Looking at this again, I see we're reading a bunch of data from c. So even though we're not modifying it, it's still common to define c as a receiver. Passing it as an argument would basically be a de-sugared version of the same code here.
There was a problem hiding this comment.
downgrade to nit: but it does seem like a receiver indicates we are doing something to c, where a read only parameter makes clear in the intent is only to create something from c.
There was a problem hiding this comment.
Let me try that in a separate PR and we can apply the pattern across the board if we like it.
|
I need to verify the delegation logic here. I've matched gcloud's behavior, but need to check if the Go library does the same (back to front). |
| func (c *Config) DialerOptions(l cloudsql.Logger) ([]cloudsqlconn.Option, error) { | ||
| opts := []cloudsqlconn.Option{ | ||
| cloudsqlconn.WithUserAgent(c.UserAgent), | ||
| func (c *Config) credentialsOpt(l cloudsql.Logger) (cloudsqlconn.Option, error) { |
There was a problem hiding this comment.
downgrade to nit: but it does seem like a receiver indicates we are doing something to c, where a read only parameter makes clear in the intent is only to create something from c.
| @@ -35,7 +34,9 @@ const connTestTimeout = time.Minute | |||
| // and then unsets GOOGLE_APPLICATION_CREDENTIALS. It returns a cleanup function | |||
| // that restores the original setup. | |||
| func removeAuthEnvVar(t *testing.T) (*oauth2.Token, string, func()) { | |||
| ts, err := google.DefaultTokenSource(context.Background(), sqladmin.SqlserviceAdminScope) | |||
| ts, err := google.DefaultTokenSource(context.Background(), | |||
| "https://www.googleapis.com/auth/cloud-platform", | |||
There was a problem hiding this comment.
Why do we need cloud-platform here?
There was a problem hiding this comment.
Short answer: the impersonation fails without this scope. Longer answer: let me figure out if we can limit this scope still further.
There was a problem hiding this comment.
After some digging, I found that this is the required OAuth2 scope to call generateAccessToken.
See:
Note: the CLI flag matches the gcloud UI.
Fixes #417