-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add support for IAM Auth for Google CloudSQL DBs #22445
Conversation
Build Results: |
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! Great work ya'll!
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.
Left a couple of small comments around formatting
@@ -29,6 +35,8 @@ type mySQLConnectionProducer struct { | |||
MaxConnectionLifetimeRaw interface{} `json:"max_connection_lifetime" mapstructure:"max_connection_lifetime" structs:"max_connection_lifetime"` | |||
Username string `json:"username" mapstructure:"username" structs:"username"` | |||
Password string `json:"password" mapstructure:"password" structs:"password"` | |||
AuthType string `json:"auth_type" mapstructure:"auth_type" structs:"auth_type"` | |||
ServiceAccountJSON string `json:"service_account_json" mapstructure:"service_account_json" structs:"service_account_json"` |
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.
nit: I was going to suggest adding a comment to this field to document that it is only for GCP. But maybe we should consider renaming it instead? I think in Vault-land we usually use GOOGLE_APPLICATION_CREDENTIALS
for service account JSON.
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.
Interesting point! This field was originally named credentials
, and was renamed to service_account_json
with the same intent of making it GCP specific. I would say service_account_json
suffices enough to make it explicit that this is a GCP related field because in the IAM context, I think only GCP uses the term 'service accounts' as far as I know 🤔 (I believe AWS just calls them IAM users). If it is not clear enough I can add in a comment like you suggested!
Here's the original renaming discussion for context: #22445 (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.
We're still making use of the GOOGLE_APPLICATION_CREDENTIALS
under the hood as the environment variable though, since that is what the gcloud
sdk also expects (similar to Vault)
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! Great job on this 🎉
@@ -29,8 +37,15 @@ type SQLConnectionProducer struct { | |||
MaxConnectionLifetimeRaw interface{} `json:"max_connection_lifetime" mapstructure:"max_connection_lifetime" structs:"max_connection_lifetime"` | |||
Username string `json:"username" mapstructure:"username" structs:"username"` | |||
Password string `json:"password" mapstructure:"password" structs:"password"` | |||
AuthType string `json:"auth_type" mapstructure:"auth_type" structs:"auth_type"` |
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.
Not blocking but we could validate that the given auth_type
is one we support and provide an error message indicating misconfiguration. Right now I think it would accept anything as input.
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.
Added auth type validation with some basic tests in f9c5f7f
This pull requests adds Google Cloud IAM-based auto-authorization capability to the MySQL and PostgreSQL database types.
Specifically, it adds an
auth_type
(with supported valuegcp_iam
) andcredentials
field to the database/config request. When set, the database backend will use GCP IAM authorization (supplied either via the environment or via the credentials field), and can understand the GCP location triple in theconnection_url
field:Example:
Note that MSSQL on GCP doesn't support auto IAM connections, and so isn't supported here.
In technical implementation, the new auth_type field signals the switch into the IAM auth path, which in GCPs case involves using their custom dialer/driver implementation.