Skip to content
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 redshift copy support for role-based credential string #1962

Merged
merged 4 commits into from
Jan 26, 2017

Conversation

dlstadther
Copy link
Collaborator

Redshift copy supports key-based (with optional session token) or role-based credentials. This PR adds support for role-based credentials, making them the default choice if both role- and key-based are provided (since they are more restrictive and thus secure), and refactors implementation of optional session token for the key-based credential string.

Description

  • Added two new properties aws_account_id and aws_arn_role_name
  • Changed aws_access_key_id and aws_secret_access_key from @abc.abstractproperty to @property
    • Added possible NotImplementedError when no pair of credentials are provided to account for the lack of @abc.abstractproperty pair of credentials
  • Defaults to role-based creds if both pairs are provided.
  • Added test to ensure that NotImplementedError raises if no pair of creds are provided.

Motivation and Context

Transitioning to the use of role-based cred string for redshift copy. Maintain key-based creds. Refactor key-based session token.

Have you tested this? If so, how?

Single unit test for ensuring exception is raised if no pair of creds are provided.

Actual success of use has been verified for the 4 (most likely) cases:

  • role-based and key-based are provided (role is used)
  • key-based only
  • role-based only
  • neither (exception is raised)

@mention-bot
Copy link

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

@dlstadther
Copy link
Collaborator Author

dlstadther commented Dec 19, 2016

These commits can and should be squashed prior to merge (assuming all tests pass).

@dlstadther
Copy link
Collaborator Author

Review request: @fordb @rantav @steenzout @ddaniels888 @moandcompany

Thanks

@dlstadther
Copy link
Collaborator Author

@Tarrasch I'm not sure what to do here. No one with Luigi Redshift experience seems to want to review. Suggestions?

Copy link
Contributor

@Tarrasch Tarrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A coworker can review, if that's possible. If not, I'm fine with merging this after 2 weeks of no volunteered reviewing. :)

@dlstadther
Copy link
Collaborator Author

I'm a bit of a one-man band. So i'll wait a bit longer before merging. Thanks!

@dlstadther
Copy link
Collaborator Author

It's now been three week with no reviews. I'll be merging now!

@dlstadther dlstadther merged commit b86166c into spotify:master Jan 26, 2017
@dlstadther dlstadther deleted the redshift_arn branch January 26, 2017 12:02
kreczko pushed a commit to kreczko/luigi that referenced this pull request Mar 28, 2017
)

* Add support for role-based credentials and refactor key-based creds

* Add test to ensure error is raised when no redshift creds are provided
This was referenced Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants