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 support to s3 for aws role assumption #28994

Merged
merged 1 commit into from Nov 30, 2015

Conversation

Projects
None yet
5 participants
@timcharper
Member

timcharper commented Nov 18, 2015

Addresses #28991

This patch adds support for role assumption to s3; it adds the configuration option s3.role_arn to the config options for s3, then enables support in the corresponding modules.

If role_arn is passed in to sig4, then an API call to http://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRole.html is made, and the session keyid, key, and token are returned and used, instead.

I made a guess and figured that my changes would break the test, but actually am not able to run them (i get the following output):

*** unit.modules.s3_test Tests  **********************************************************************************************************************************************************
 --------  Skipped Tests  ----------------------------------------------------------------------------------------------------------------------------------------------------------------
   -> unit.modules.s3_test.S3TestCase.test_delete  ->  mock python module is unavailable
   -> unit.modules.s3_test.S3TestCase.test_get     ->  mock python module is unavailable
   -> unit.modules.s3_test.S3TestCase.test_head    ->  mock python module is unavailable
   -> unit.modules.s3_test.S3TestCase.test_put     ->  mock python module is unavailable

Thank you for saltstack.

@timcharper

This comment has been minimized.

Show comment
Hide comment
@timcharper

timcharper Nov 18, 2015

Member

Note, the parameter will need to be added to the code modified by this patch:

6e2003e

Member

timcharper commented Nov 18, 2015

Note, the parameter will need to be added to the code modified by this patch:

6e2003e

@jfindlay

This comment has been minimized.

Show comment
Hide comment
@jfindlay

jfindlay Nov 18, 2015

Contributor

@timcharper, thanks for implementing this. There are a few test errors.

Contributor

jfindlay commented Nov 18, 2015

@timcharper, thanks for implementing this. There are a few test errors.

@jfindlay jfindlay added this to the Approved milestone Nov 18, 2015

@jfindlay jfindlay removed this from the Approved milestone Nov 18, 2015

@timcharper

This comment has been minimized.

Show comment
Hide comment
@timcharper

timcharper Nov 19, 2015

Member

Boy am I not surprised. I'll take a look at them.

Member

timcharper commented Nov 19, 2015

Boy am I not surprised. I'll take a look at them.

@timcharper

This comment has been minimized.

Show comment
Hide comment
@timcharper

timcharper Nov 19, 2015

Member

I'm sure some of the errors are my fault, but are all of them? This one seems unlikely to be, in particular: https://jenkins.saltstack.com/job/salt-pr-rs-cent7-n/9864/testReport/integration.netapi.rest_tornado.test_app/TestWebhookSaltAPIHandler/test_post/

Member

timcharper commented Nov 19, 2015

I'm sure some of the errors are my fault, but are all of them? This one seems unlikely to be, in particular: https://jenkins.saltstack.com/job/salt-pr-rs-cent7-n/9864/testReport/integration.netapi.rest_tornado.test_app/TestWebhookSaltAPIHandler/test_post/

@cachedout

This comment has been minimized.

Show comment
Hide comment
@cachedout

cachedout Nov 19, 2015

Contributor

@timcharper Just let us know when you've got those errors handled and we'll get this in. Thanks!

Contributor

cachedout commented Nov 19, 2015

@timcharper Just let us know when you've got those errors handled and we'll get this in. Thanks!

@timcharper

This comment has been minimized.

Show comment
Hide comment
@timcharper

timcharper Nov 20, 2015

Member

I can't look at the build failures anymore. It's telling me: "Access Denied: timcharper is missing the Overall/Read permission"

Member

timcharper commented Nov 20, 2015

I can't look at the build failures anymore. It's telling me: "Access Denied: timcharper is missing the Overall/Read permission"

@jfindlay

This comment has been minimized.

Show comment
Hide comment
@jfindlay

jfindlay Nov 20, 2015

Contributor

@timcharper, that should be fixed now.

Contributor

jfindlay commented Nov 20, 2015

@timcharper, that should be fixed now.

@rallytime

This comment has been minimized.

Show comment
Hide comment
@rallytime

rallytime Nov 21, 2015

Contributor

@techhat You might want to know about this one, too.

Contributor

rallytime commented Nov 21, 2015

@techhat You might want to know about this one, too.

@timcharper

This comment has been minimized.

Show comment
Hide comment
@timcharper

timcharper Nov 22, 2015

Member

Okay, I pushed a new patch, looks like a single omitted argument in the s3-delete function was causing all of the s3-related test case failures.

There will still be some failures; The FileTestCase failures aren't a result of my changes, either.

Member

timcharper commented Nov 22, 2015

Okay, I pushed a new patch, looks like a single omitted argument in the s3-delete function was causing all of the s3-related test case failures.

There will still be some failures; The FileTestCase failures aren't a result of my changes, either.

Show outdated Hide outdated salt/modules/s3.py Outdated
@timcharper

This comment has been minimized.

Show comment
Hide comment
@timcharper

timcharper Nov 23, 2015

Member

That was embarrassing, @lorengordon . Removed the print.

Member

timcharper commented Nov 23, 2015

That was embarrassing, @lorengordon . Removed the print.

@lorengordon

This comment has been minimized.

Show comment
Hide comment
@lorengordon

lorengordon Nov 23, 2015

Contributor

Heh, @timcharper, it's all good, I've been there too. Better caught now than after the merge!

Contributor

lorengordon commented Nov 23, 2015

Heh, @timcharper, it's all good, I've been there too. Better caught now than after the merge!

@cachedout

This comment has been minimized.

Show comment
Hide comment
@cachedout

cachedout Nov 24, 2015

Contributor

@timcharper Looks like some lint failures are still present. Would you mind having another look? Then we can definietely get this one in. Thanks!

Contributor

cachedout commented Nov 24, 2015

@timcharper Looks like some lint failures are still present. Would you mind having another look? Then we can definietely get this one in. Thanks!

@timcharper

This comment has been minimized.

Show comment
Hide comment
@timcharper

timcharper Nov 28, 2015

Member

@cachedout lint errors are all fixed!

Member

timcharper commented Nov 28, 2015

@cachedout lint errors are all fixed!

@timcharper

This comment has been minimized.

Show comment
Hide comment
@timcharper

timcharper Nov 28, 2015

Member

I just rebased against the latest 2015.8

Member

timcharper commented Nov 28, 2015

I just rebased against the latest 2015.8

cachedout added a commit that referenced this pull request Nov 30, 2015

Merge pull request #28994 from timcharper/2015.8.1-dev
add support to s3 for aws role assumption

@cachedout cachedout merged commit 87e4aa4 into saltstack:2015.8 Nov 30, 2015

@cachedout

This comment has been minimized.

Show comment
Hide comment
@cachedout

cachedout Nov 30, 2015

Contributor

Sweet! Merged. Thanks, @timcharper

Contributor

cachedout commented Nov 30, 2015

Sweet! Merged. Thanks, @timcharper

@timcharper

This comment has been minimized.

Show comment
Hide comment
@timcharper

timcharper Nov 30, 2015

Member

\\ ٩( ᐛ )و //

Member

timcharper commented Nov 30, 2015

\\ ٩( ᐛ )و //

@timcharper timcharper deleted the timcharper:2015.8.1-dev branch Jan 19, 2016

@timcharper

This comment has been minimized.

Show comment
Hide comment
@timcharper

timcharper Jan 19, 2016

Member

@cachedout would you like me to update release notes for 2015.8.4 ?

Member

timcharper commented Jan 19, 2016

@cachedout would you like me to update release notes for 2015.8.4 ?

@jfindlay

This comment has been minimized.

Show comment
Hide comment
@jfindlay

jfindlay Jan 19, 2016

Contributor

@timcharper, yes that would be awesome, thanks.

Contributor

jfindlay commented Jan 19, 2016

@timcharper, yes that would be awesome, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment