feat(rt): ec2 credentials provider #348
Conversation
| client.value.get(CREDENTIALS_BASE_PATH) | ||
| } catch (ex: EC2MetadataError) { | ||
| if (ex.statusCode == 404) { | ||
| logger.info { "received 404 from IMDS when loading profile information. Hint: This instance may not have an IAM role associated" } |
There was a problem hiding this comment.
Nit: "received" → "Received"
| return try { | ||
| client.value.get(CREDENTIALS_BASE_PATH) | ||
| } catch (ex: EC2MetadataError) { | ||
| if (ex.statusCode == 404) { |
There was a problem hiding this comment.
Style: Suggest using a constant.
| public sealed class CredentialsError(public val message: String) { | ||
| /** | ||
| * No credentials were available from this provider | ||
| */ | ||
| public class CredentialsNotLoaded(message: String?) : CredentialsError(message ?: "The provider could not provide credentials or required configuration was not set") | ||
|
|
||
| /** | ||
| * The provider was given an invalid configuration (e.g. invalid aws configuration file, invalid IMDS endpoint, etc) | ||
| */ | ||
| public class InvalidConfiguration(message: String) : CredentialsError(message) | ||
|
|
||
| /** | ||
| * The provider experienced an error during credentials resolution | ||
| */ | ||
| public class ProviderError(message: String) : CredentialsError(message) | ||
|
|
||
| /** | ||
| * An unexpected error occurred during credential resolution. | ||
| * If the error is something that can occur during expected usage of a provider, [ProviderError] should be returned | ||
| * instead. Unknown is for exceptional cases, for example data this is missing required fields. | ||
| */ | ||
| public class Unknown(message: String) : CredentialsError(message) | ||
|
|
||
| override fun toString(): String = "${this::class.simpleName}: $message" | ||
| } | ||
|
|
||
| /** | ||
| * Exception base class for credential provider errors | ||
| */ | ||
| public open class CredentialsException(public val error: CredentialsError, cause: Throwable? = null) : ClientException(error.toString(), cause) |
There was a problem hiding this comment.
Comment: Given that these are mutually exclusive and represent distinct types of error, I think these subtypes may work better in the exception hierarchy itself. Kotlin makes it easy to catch exceptions by type with a simple catch block whereas call-site filtering for these would require if/when inside the catch.
kggilmer
left a comment
kggilmer
left a comment
There was a problem hiding this comment.
Interested in discussing the embedded Error type inside of the Exception
|
|
||
| import aws.sdk.kotlin.runtime.ClientException | ||
|
|
||
| public sealed class CredentialsError(public val message: String) { |
There was a problem hiding this comment.
style
IMO the least surprising way of modeling this would be that the members of CredentialsError would be concrete exception types with CredentialsException being the parent. What advantage(s) do you see with an error type as a member of a general exception?
| throwCredentialsError(CredentialsError.CredentialsNotLoaded("AWS EC2 metadata is explicitly disabled; credentials not loaded")) | ||
| } | ||
|
|
||
| val profileName = runCatching { |
There was a problem hiding this comment.
question
what is this offering over
val profile = try { ... } catch (e: Ex) { throw CredentialsException(.., e) }There was a problem hiding this comment.
Nothing. Just style. Can update and change to that.
| client.value.get(CREDENTIALS_BASE_PATH) | ||
| } catch (ex: EC2MetadataError) { | ||
| if (ex.statusCode == 404) { | ||
| logger.info { "received 404 from IMDS when loading profile information. Hint: This instance may not have an IAM role associated" } |
There was a problem hiding this comment.
nit
terminate last sentence w/ period
|
|
||
| override fun close() { | ||
| if (client.isInitialized()) { | ||
| client.value.close() |
There was a problem hiding this comment.
comment
it would be nice to get lazy eval without having the additional value access. I don't see a way to do it offhand but 🤷
There was a problem hiding this comment.
Ya you can't have properties/delegated properties be suspend so I don't see a way around it
| val expiration: Instant, | ||
| ) : JsonCredentialsResponse() | ||
|
|
||
| // TODO - add support for static credentials |
There was a problem hiding this comment.
question
these will come in subsequent PR associated w/ the imds client?
There was a problem hiding this comment.
Not as much to do with IMDS so much as other JSON based credential responses. IMDS, STS, ECS all have the same JSON response structure (or rather are subsets of one another)
|
|
||
| return when (code) { | ||
| // IMDS does not appear to reply with `Code` missing but documentation indicates it may be possible | ||
| "Success", null -> { |
There was a problem hiding this comment.
question
it's guaranteed no variations in case?
There was a problem hiding this comment.
As far as I know there is not
There was a problem hiding this comment.
Then probably makes sense to do the string check without regard to case.
3 participants
🚨 Speculative PR alert! 🚨
Interested to hear about the use of
CredentialsErrorw.r.tCredentialsException. The goal was/is to provide more meaningful exceptions and error messages to customers. There are other ways of modeling this though.Issue #
closes #17
Description of changes
CredentialsProviderNOTE: This has the same issue with loading the active profile per/provider. Eventually we may want to thread through an active profile as a
LazyAsyncValue, that way the first to need it will load it, subsequent callers will get the cached version.Scope
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.