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

Support AWS partitions with role-based credentials #210

Merged
merged 1 commit into from
Jul 15, 2020

Conversation

bbayles
Copy link
Contributor

@bbayles bbayles commented Jul 15, 2020

#88 added support for IAM Role-based authentication. This allows the user to supply an aws_account_id and iam_role_name to use with the COPY and UNLOAD commands.

The existing code constructs an ARN for the IAM role from these parameters. However, it assumes that the account exists in the aws partition.

This assumption is right for most AWS regions - e.g. us-east-1, eu-central-1. But it's not right for the AWS GovCloud (US) regions or the AWS China regions.

This PR adds an aws_partition parameter for both the COPY and UNLOAD commands that allows those other regions to be used. It defaults to 'aws' such that the default behavior is unchanged.

This is a common mistake when dealing with ARNs. See the AWS docs for information about the format, including the partition.

  • MIT compatible
  • Tests
  • Documentation
  • Updated CHANGES.rst

@@ -26,13 +26,14 @@
ACCESS_KEY_ID_RE = re.compile('[A-Z0-9]{20}')
SECRET_ACCESS_KEY_RE = re.compile('[A-Za-z0-9/+=]{40}')
TOKEN_RE = re.compile('[A-Za-z0-9/+=]+')
AWS_PARTITIONS = frozenset({'aws', 'aws-cn', 'aws-us-gov'})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The list of valid partitions is here.

Copy link
Member

@jklukas jklukas left a comment

Choose a reason for hiding this comment

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

Looks great! Integration tests are passing. Thanks for the contribution.

@jklukas jklukas merged commit d91b025 into sqlalchemy-redshift:master Jul 15, 2020
@bbayles
Copy link
Contributor Author

bbayles commented Jul 15, 2020

Excellent - many thanks for the quick response. Might I request a release with this change incorporated? Thanks again.

@jklukas
Copy link
Member

jklukas commented Jul 15, 2020

Done. 0.8.1 is released.

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.

2 participants