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

Throw dedicated error for attempting EC2 auth not on EC2 #983

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pbrisbin
Copy link
Contributor

@pbrisbin pbrisbin commented Apr 9, 2024

As discussed in #981:

Each link in the discover chain should throw an AuthError to trigger
moving onto the next link in the chain. If all links fail, then a
CredentialChainExhausted error is thrown.

In the case of the final link, attempting an EC2 instance profile, the
code was throwing CredentialChainExhausted if run not on EC2. Throwing
when not on EC2 is important (to avoid an unnecessary and slow network
request) but throwing this particular exception is surprising and only
functions because this is the last link in said chain. If any other
authentication mechanisms were added after it, they would never be
reached.

To address this, a new constructor was added to AuthError to use
instead. The name is very much up for debate. This is also the first
case of a nullary constructor besides CredentialChainExhausted, so I
wasn't sure what to do in AsAuthError -- please advise.

The original discussion mentioned introduce two new errors:

One to say "we don't think we're on a network with an IMDS" and one
for "IMDS fetch failed"?

I did think about this and found that the second case is already
throwing a specific error (RetrievalError), so I only changed the
first case.

As discussed in brendanhay#981:

Each link in the discover chain should throw an `AuthError` to trigger
moving onto the next link in the chain. If all links fail, then a
`CredentialChainExhausted` error is thrown.

In the case of the final link, attempting an EC2 instance profile, the
code was throwing `CredentialChainExhausted` if run not on EC2. Throwing
when not on EC2 is important (to avoid an unnecessary and slow network
request) but throwing this particular exception is surprising and only
functions because this is the last link in said chain. If any other
authentication mechanisms were added after it, they would never be
reached.

To address this, a new constructor was added to `AuthError` to use
instead. The name is very much up for debate. This is also the first
case of a nullary constructor besides `CredentialChainExhausted`, so I
wasn't sure what to do in `AsAuthError` -- please advise.

The original discussion mentioned introduce two new errors:

> One to say "we don't think we're on a network with an IMDS" and one
> for "IMDS fetch failed"?

I did think about this and found that the second case is already
throwing a specific error (`RetrievalError`), so I only changed the
first case.
@endgame endgame added this to the 2.1 milestone Apr 17, 2024
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.

None yet

2 participants