REDSHIFT: standardize credential usage #2068

zsalzbank · 2017-03-20T22:10:50Z

Description

Obtaining the credentials for a redshift connection is now uniform
accross all of the redshift tasks. This has the added benefit of
allowing role-based credentials in all of the tasks as well.

Motivation and Context

I need to use role-based credentials for all of my redshift tasks, not just some of them.

Have you tested this? If so, how?

I ran this against the current unit tests for the redshift files and everything passed. If anything else is needed, please let me know.

mention-bot · 2017-03-20T22:10:51Z

@zsalzbank, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dlstadther, @ddaniels888 and @rantav to be potential reviewers.

Obtaining the credentials for a redshift connection is now uniform accross all of the redshift tasks. This has the added benefit of allowing role-based credentials in all of the tasks as well.

dlstadther · 2017-03-22T11:23:27Z

luigi/contrib/redshift.py

+        """
+        Override to return the account id.
+        """
+        return None


If aws_access_key_id and aws_secret_access_key auto fill from config, why doesn't aws_account_id and aws_arn_role_name?

I was really trying to mimic the existing functionality for those fields, but would be happy to add that in if you think it is the right thing to do.

Right now, I'm not to sure that the implementation (current and previous) was even really so correct, because it is using the information from the s3 section as opposed to a redshift section in the configuration file.

I definitely think both role-based and key-based creds should be capable of pulling from config.

I agree that there is likely a better way to approach this implementation. I'll keep thinking on it.

dlstadther · 2017-03-22T11:25:10Z

luigi/contrib/redshift.py

+        config = luigi.configuration.get_config()
+
+        try:
+            value = config.get('s3', attribute)


Since these are general AWS creds, I'm not sure it makes sense to restrict their config location to s3, particularly if they're being used here for Redshift.

Haha - just mentioned that above, but kept the current implementation. Happy to change to a redshift section, which I think makes more sense, but it wouldn't be backwards compatible.

S3 gets its config here. So if we were to change this value (in the cred mixin), someone who uses both s3 and redshift (which is most redshift users) would need to include their creds two places in their config. This isn't ideal.

What are your thoughts on having a named section parameter somewhere with a default value of 'redshift' (where the user can override) which gets passed to _get_s3_configuration_attribute()?

Sounds doable.

dlstadther · 2017-03-22T11:27:16Z

luigi/contrib/redshift.py

+        except NoSectionError:
+            value = None
+
+        if value is None or value == '':


This can just be if not value: because None and '' equate to False

I learned something new!

dlstadther · 2017-03-22T11:28:26Z

luigi/contrib/redshift.py

+
+    def _credentials(self):
+        """
+        Return a credentials string for the provided task. If no valid


Return a credential string (not credentials)

dlstadther · 2017-03-22T11:44:14Z

I neglected to write a summary review, but in general this is very much something I've wanted to implement myself (just never had the time). All my comments are minor.

Thanks for contributing!

zsalzbank · 2017-03-22T12:01:58Z

Made a couple of changes. Let me know what you'd like to do regarding s3/redshift section and auto-loading the credentials, and I'll be happy to change it as well.

dlstadther · 2017-03-22T12:37:53Z

luigi/contrib/redshift.py

+        """
+        return None
+
+    def _get_s3_configuration_attribute(self, attribute):


Also just noticed that the name of this method includes s3. Seem odd given explicit redshift usage.

zsalzbank · 2017-03-22T13:18:06Z

Made some more changes.

dlstadther · 2017-03-22T14:42:26Z

@zsalzbank Can you confirm that this code works for you?

zsalzbank · 2017-03-22T15:02:43Z

I tested getting the values from the environment variables. I don't use the luigi configuration files in my setup, so I didn't really test that part. But I can if you point me in the right direction for how to use them.

dlstadther · 2017-03-22T15:07:35Z

You can create a luigi.cfg file in the same directory as your .py luigi files.

It would look like

[redshift]
aws_access_key_id: <your id here>
aws_secret_access_key: <your secret here>

[s3]
<same as redshift above just to check that you can change the section to something other than the default>

zsalzbank · 2017-03-22T15:29:39Z

Works for me. I made one minor change to take advantage of the configuration object supporting a default that I noticed when debugging.

If this all looks good, I'll squash all my fixups.

zsalzbank · 2017-03-22T15:32:28Z

I also updated the error message to say where the variables can be set when there are no valid pairs of credentials.

dlstadther · 2017-03-22T15:34:20Z

Let's get at least one more review before worrying about squashing or merging just yet.

If no one else reviews, @Tarrasch are you familiar enough with S3's use of accessing config variables to be able to provide a review here?

dlstadther · 2017-03-22T15:34:42Z

Also, @zsalzbank thanks for such quick turn around edits!

zsalzbank · 2017-03-22T15:36:06Z

No problem at all. I like giving back.

dmohns · 2017-03-24T10:48:15Z

I like this a lot!
Two questions:
1.) Just double-checking as I am not 100% familiar with Mixins: Will backwards compatibility be preserved? E.g. will we still be able to provide Redshift credentials by overriding not using the Mixin or cfg?
2.) Isn't the comment You must also override the attributes provided by the CredentialsMixin. a bit misleading? As far as I understand the main benefit of this PR is that we do not have to override attributes but can simply provide entries in the cfg.

zsalzbank · 2017-03-24T11:53:42Z

Yes, at least as I understand your question. You can still do aws_access_key_id = '...' in the class definition or:

@property
def aws_access_key_id(self):
    return '...'

Clarified.

dmohns · 2017-03-27T11:21:01Z

luigi/contrib/redshift.py

@@ -52,7 +138,7 @@ class RedshiftTarget(postgres.PostgresTarget):
    use_db_timestamps = False


-class S3CopyToTable(rdbms.CopyToTable):
+class S3CopyToTable(rdbms.CopyToTable, _CredentialsMixin):


Is this the correct order of inheritance? Based on this http://stackoverflow.com/questions/825945/abstract-class-mixin-multiple-inheritance-in-python it should be ordered the other way around. I couldn't test it yet.

Works for me as it is, but I'm happy to change it if you think that is the right way to go.

Unfortunately I don't have the time to test it. I was doing something similar some time back and remember it failed because the @abc.abstractpropertys from the base class were still abstract on instantiation.
As this is just based on my memory your code might be correct already. Maybe someone else can help out?

So the issue identified in that SO question seems to be around the Mixin implementing an abstractmethod which is supplied by an earlier inherited class.

Perhaps it's worth swapping the order of inheritance for a future case where _CredentialsMixin overrides an abstractmethod which is added to rdbms.CopyToTable.

So long as the overall functionality still works, it doesn't hurt anything.

THanks,

Let me re-phrase my concern: From my understanding the current implementation should not work unless you override the abstract properties in sub class. (Which is exactly what you not want to do, when you try to read credentials from luigi.cfg.)

Currently this is not covered by any unittest, as all the test subclass and override. So, my pragmatic suggestion would be: Can we create a test for reading credentials from luigi.cfg (or even ENV) ? If this test goes through successfully my comment is decrepit.

Sounds doable. Might take me a little bit though, as I have to focus on some other stuff right now.

Where did we land on this thread? Do your tests cover this (potential) issue?

This is what my most recent commit is for. I test loading the credentials from the different available sources. Let me know if you want to see other tests.

This looks good. Just to clarify: What I missed earlier is the fact that within this PR the aws credentials are no longer abstract. Potentially missing credentials are now handled explicitly in the mixin. Therefore order of inheritance is indeed irrelevant here. 👍

dlstadther · 2017-08-25T11:46:24Z

@zsalzbank Any update here? Really looking forward to merging and using this PR!

zsalzbank · 2017-08-25T11:52:02Z

Sorry, been busy, but I'll try and get to it next week.

zsalzbank · 2017-08-28T14:50:56Z

Added test cases - pass locally with the existing mixin order, but will confirm that is the case with the full CI suite.

dlstadther · 2017-08-28T15:58:07Z

@dmohns Could you also review again? If approved, i'll merge.

zsalzbank · 2017-08-28T18:35:09Z

Thanks!

zsalzbank force-pushed the common-redshift-credentials branch from bdeff7b to e8cfb63 Compare March 20, 2017 22:27

REDSHIFT: standardize credential usage

2f2ef70

Obtaining the credentials for a redshift connection is now uniform accross all of the redshift tasks. This has the added benefit of allowing role-based credentials in all of the tasks as well.

zsalzbank force-pushed the common-redshift-credentials branch from e8cfb63 to 2f2ef70 Compare March 20, 2017 22:28

dlstadther requested changes Mar 22, 2017

View reviewed changes

fixup! REDSHIFT: standardize credential usage

073a1e3

dlstadther reviewed Mar 22, 2017

View reviewed changes

fixup! REDSHIFT: standardize credential usage

48f48f9

fixup! REDSHIFT: standardize credential usage

d8e406d

fixup! REDSHIFT: standardize credential usage

2cbb60a

zsalzbank force-pushed the common-redshift-credentials branch from 615d8ff to 2cbb60a Compare March 22, 2017 15:31

fixup! REDSHIFT: standardize credential usage

f777cbf

dmohns reviewed Mar 27, 2017

View reviewed changes

fixup! REDSHIFT: standardize credential usage

1d2b7a7

dlstadther approved these changes Aug 28, 2017

View reviewed changes

dmohns approved these changes Aug 28, 2017

View reviewed changes

dlstadther merged commit b47b267 into spotify:master Aug 28, 2017

This was referenced Jun 29, 2022

no mo enum 34 #3180

Closed

enum34 be gone #3181

Closed

mdragilev mentioned this pull request Jun 28, 2024

for S3 contrib package move to boto3 Affirm/luigi#26

Merged

REDSHIFT: standardize credential usage #2068

REDSHIFT: standardize credential usage #2068

Conversation

zsalzbank commented Mar 20, 2017

Description

Motivation and Context

Have you tested this? If so, how?

mention-bot commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlstadther commented Mar 22, 2017

zsalzbank commented Mar 22, 2017

Choose a reason for hiding this comment

zsalzbank commented Mar 22, 2017

dlstadther commented Mar 22, 2017

zsalzbank commented Mar 22, 2017

dlstadther commented Mar 22, 2017

zsalzbank commented Mar 22, 2017

zsalzbank commented Mar 22, 2017

dlstadther commented Mar 22, 2017

dlstadther commented Mar 22, 2017

zsalzbank commented Mar 22, 2017

dmohns commented Mar 24, 2017

zsalzbank commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmohns Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmohns Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlstadther commented Aug 25, 2017

zsalzbank commented Aug 25, 2017

zsalzbank commented Aug 28, 2017

dlstadther commented Aug 28, 2017

zsalzbank commented Aug 28, 2017

dmohns Mar 30, 2017 •

edited

Loading

dmohns Apr 5, 2017 •

edited

Loading