IAM authentication method for redshift adapter #769
Conversation
There was a problem hiding this comment.
This is super cool, thanks for opening this PR @danielchalef! I dropped some comments in here, happy to discuss!
dbt/contracts/connection.py
Outdated
| @@ -24,6 +23,18 @@ | |||
| Required('schema'): basestring, | |||
| }) | |||
|
|
|||
| redshift_auth_methods = ['database', 'iam'] | |||
| redshift_credentials_contract = Schema({ | |||
| Required('method'): Any(*redshift_auth_methods), | |||
There was a problem hiding this comment.
I think this should be optional, and the default should be database. It would be good to preserve backwards compatibility here
There was a problem hiding this comment.
Roger that. It would be great to require a password if the method keyword is missing or the database method is selected. Similarly, for cluster_id if the iam method is used. How do I go about doing so with voluptuous?
There was a problem hiding this comment.
We're in the process of ripping out voluptuous and replacing it with json schemas as a part of building out dbt's API. I think you can just leave the voluptuous code here and we'll convert it to the new style contract along with everything else
dbt/adapters/redshift/impl.py
Outdated
| return (result) | ||
|
|
||
| elif method == 'iam': | ||
| cluster_id = result.get('cluster_id') |
There was a problem hiding this comment.
can you slide the logic in this else branch into a different function? I think get_redshift_credentials should return a dict that looks like
{
"dbname": "...",
"user": "...",
"pass": "...",
"port": "..."
}
regardless of if the auth method is credentials or IAM
| result = connection.copy() | ||
|
|
||
| try: | ||
| credentials = cls.get_redshift_credentials(connection.get('credentials', {})) |
There was a problem hiding this comment.
the suggestion above about get_redshift_credentials should clean up the if/else logic here I believe!
|
hey @danielchalef - thanks for your patience here. We changed some aspects of dbt's API since you opened this PR. Do you mind if I branch off of here and get it up-to-date with |
|
@drewbanin No worries. Go for it. Thanks. |
|
Closing this in favor of #818, which integrates development and adds an |
Per #757 :
A
methodvalue is now required in a profile using theredshiftadapter.databasemethod supports username / password as database auth credentials.iammethod support IAM authentication using boto3'sget_cluster_credentials. See AWS docs Using IAM Authentication to Generate Database User Credentials for required setup.