Add redshift role use option #4532
Conversation
|
Looking at this I think the failing test has nothing to do with the code in the commit - I assume there is another failure on master. Is that likely? |
|
Yes, this failure is not related to your code. Sorry about this. |
redash/query_runner/pg.py
Outdated
| @@ -284,6 +349,27 @@ def configuration_schema(cls): | |||
| "required": ["dbname", "user", "password", "host", "port"], | |||
| "secret": ["password"], | |||
| } | |||
| if redshift_role_enabled: | |||
There was a problem hiding this comment.
Having all the options under one data source will make the configuration a bit confusing. Also it makes it hard to enforce required fields.
How about we create a subclass for the IAM based authentication?
There was a problem hiding this comment.
Sure - that's a sensible option. So we end up with something like Redshift - User Login and Redshift - IAM Login.
There was a problem hiding this comment.
I would go with Redshift and Redshift (IAM Login).
|
I'm now making the logo work for the new version. Then it should all work. |
|
This should now work. I've tested it using AWS Keys. I'll make sure with roles as well. |
|
I've tested this using roles as well now. |
requirements.txt
Outdated
| @@ -60,6 +60,7 @@ disposable-email-domains>=0.0.52 | |||
| gevent==1.4.0 | |||
| supervisor==4.1.0 | |||
| supervisor_checks==0.8.1 | |||
| urllib3==1.25.7 | |||
There was a problem hiding this comment.
Are you sure we need this as an explicit dependency?
There was a problem hiding this comment.
Hmm - I think it addresses a bug raised with in a couple of places. I'll try it with just specification of the pyopenssl version.
There was a problem hiding this comment.
OK tested it and specify of pyopenssl is enough so I have requirement requirement for specified urllib.
There was a problem hiding this comment.
The cryptography upgrade looks safe, but pyopenssl has some deprecations which I'm not sure if they are supported by all the other libs we depend on.
There was a problem hiding this comment.
I'll try rerunning with just cryptography tomorrow morning - Berlin time.
There was a problem hiding this comment.
It worked with cryptography 2.8 and pyopenssl 19.0.0 so I've made that commit.
There was a problem hiding this comment.
Great. I fixed a merge conflict and once the build finishes will merge 👍
Co-Authored-By: Arik Fraimovich <[email protected]>
Co-Authored-By: Arik Fraimovich <[email protected]>
|
Thanks! |
What type of PR is this? (check all applicable)
Description
This is to add the ability to use IAM Roles and Users for Redshift access. It supports specifying a role and/or keys or using a role or keys already attached to the instance.
Related Tickets & Documents
Mobile & Desktop Screenshots/Recordings (if there are UI changes)