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

Treat 'RoleArn' as an optional field when fetching ECS credentials #139

Merged
merged 2 commits into from
Mar 10, 2022

Conversation

cole-p
Copy link
Contributor

@cole-p cole-p commented Mar 10, 2022

The ECS developers guide implies that the RoleArn field should always be returned when querying the container's credential endpoint, however this isn't always the case for Amazon SageMaker jobs.

Amazon avoids this in botocore by not accessing the RoleArn field.

Copy link
Member

@mattBrzezinski mattBrzezinski left a comment

Choose a reason for hiding this comment

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

LGTM, I took a look about writing a test case for this but with it's current layout it's a bit tricky.

Also we could port this change into AWS.jl, and internally stop using this package as well.

But for now we can merge this in as it's a safe change.

@mattBrzezinski
Copy link
Member

I cannot suggest the change, but in the Project.toml file can you do a patch version bump and just add that to this PR so we don't need to open another?

@iamed2
Copy link
Member

iamed2 commented Mar 10, 2022

Also we could port this change into AWS.jl

Yes this should be ported no matter what

@mattBrzezinski
Copy link
Member

Also we could port this change into AWS.jl

Yes this should be ported no matter what

Yes. I typed the wrong word there. This definitely should be included in AWS.jl.

@mattBrzezinski
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Mar 10, 2022
139: Treat 'RoleArn' as an optional field when fetching ECS credentials r=mattBrzezinski a=cole-p

The [ECS developers guide](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-iam-roles.html) implies that the `RoleArn` field should always be returned when querying the container's credential endpoint, however this isn't always the case for Amazon SageMaker jobs.

Amazon avoids this in [botocore](https://github.com/boto/botocore/blob/21b06efb39da61fe4101ab9709140dc1e51dc33d/botocore/credentials.py#L1886-L1889) by not accessing the `RoleArn` field.

Co-authored-by: Cole Peters <[email protected]>
bors bot added a commit to JuliaCloud/AWS.jl that referenced this pull request Mar 10, 2022
535: Treat 'RoleArn' as an optional field when fetching ECS credentials r=mattBrzezinski a=cole-p

Port the following changes made to AWSCore.jl: JuliaCloud/AWSCore.jl#139

> The [ECS developers guide](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-iam-roles.html) implies that the `RoleArn` field should always be returned when querying the container's credential endpoint, however this isn't always the case for Amazon SageMaker jobs.
>
>Amazon avoids this in [botocore](https://github.com/boto/botocore/blob/21b06efb39da61fe4101ab9709140dc1e51dc33d/botocore/credentials.py#L1886-L1889) by not accessing the `RoleArn` field.

Co-authored-by: Cole Peters <[email protected]>
@mattBrzezinski
Copy link
Member

CI is going to time out because it's still setup for Travis CI and not GHA. Just going to merge this manually...

@mattBrzezinski mattBrzezinski merged commit 7098e9a into JuliaCloud:master Mar 10, 2022
@bors
Copy link
Contributor

bors bot commented Mar 10, 2022

Timed out.

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