-
Notifications
You must be signed in to change notification settings - Fork 49
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
misc: credentials business metrics #1442
Changes from 19 commits
faee8bb
a50e692
8d61e07
18c80e7
8244764
9c3967c
92e091e
a4e5eca
1a51409
d7ba2b9
668ed18
1bdaa80
03ca91f
6a7b812
02db28d
b74a50b
864dc65
82c7a3f
573facd
85b78d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ package aws.sdk.kotlin.runtime.auth.credentials | |
|
||
import aws.sdk.kotlin.runtime.config.AwsSdkSetting | ||
import aws.sdk.kotlin.runtime.config.imds.ImdsClient | ||
import aws.sdk.kotlin.runtime.http.interceptors.businessmetrics.AwsBusinessMetric | ||
import aws.smithy.kotlin.runtime.auth.awscredentials.* | ||
import aws.smithy.kotlin.runtime.collections.Attributes | ||
import aws.smithy.kotlin.runtime.http.engine.DefaultHttpEngine | ||
|
@@ -52,7 +53,10 @@ public class DefaultChainCredentialsProvider constructor( | |
private val chain = CredentialsProviderChain( | ||
SystemPropertyCredentialsProvider(platformProvider::getProperty), | ||
EnvironmentCredentialsProvider(platformProvider::getenv), | ||
LazilyInitializedCredentialsProvider("EnvironmentStsWebIdentityCredentialsProvider") { | ||
LazilyInitializedCredentialsProvider( | ||
"EnvironmentStsWebIdentityCredentialsProvider", | ||
AwsBusinessMetric.Credentials.CREDENTIALS_ENV_VARS_STS_WEB_ID_TOKEN, | ||
) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Why should the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was originally what I had, you can see it here. There's some conversation you can follow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't really address my initial concern of the business metrics seeming to be hacked into the credentials provider implementation. I agree with Ian, |
||
// STS web identity provider can be constructed from either the profile OR 100% from the environment | ||
StsWebIdentityCredentialsProvider.fromEnvironment( | ||
platformProvider = platformProvider, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,16 +9,18 @@ import aws.sdk.kotlin.runtime.InternalSdkApi | |
import aws.sdk.kotlin.runtime.auth.credentials.profile.LeafProvider | ||
import aws.sdk.kotlin.runtime.auth.credentials.profile.ProfileChain | ||
import aws.sdk.kotlin.runtime.auth.credentials.profile.RoleArn | ||
import aws.sdk.kotlin.runtime.auth.credentials.profile.RoleArnSource | ||
import aws.sdk.kotlin.runtime.client.AwsClientOption | ||
import aws.sdk.kotlin.runtime.config.AwsSdkSetting | ||
import aws.sdk.kotlin.runtime.config.imds.ImdsClient | ||
import aws.sdk.kotlin.runtime.config.profile.AwsConfigurationSource | ||
import aws.sdk.kotlin.runtime.config.profile.loadAwsSharedConfig | ||
import aws.sdk.kotlin.runtime.http.interceptors.businessmetrics.AwsBusinessMetric | ||
import aws.sdk.kotlin.runtime.http.interceptors.businessmetrics.emitBusinessMetrics | ||
import aws.sdk.kotlin.runtime.region.resolveRegion | ||
import aws.smithy.kotlin.runtime.auth.awscredentials.CloseableCredentialsProvider | ||
import aws.smithy.kotlin.runtime.auth.awscredentials.Credentials | ||
import aws.smithy.kotlin.runtime.auth.awscredentials.CredentialsProvider | ||
import aws.smithy.kotlin.runtime.auth.awscredentials.simpleClassName | ||
import aws.smithy.kotlin.runtime.auth.awscredentials.* | ||
import aws.smithy.kotlin.runtime.businessmetrics.BusinessMetric | ||
import aws.smithy.kotlin.runtime.businessmetrics.BusinessMetrics | ||
import aws.smithy.kotlin.runtime.collections.Attributes | ||
import aws.smithy.kotlin.runtime.http.engine.HttpClientEngine | ||
import aws.smithy.kotlin.runtime.io.closeIfCloseable | ||
|
@@ -86,6 +88,7 @@ public class ProfileCredentialsProvider @InternalSdkApi constructor( | |
public val httpClient: HttpClientEngine? = null, | ||
public val configurationSource: AwsConfigurationSource? = null, | ||
) : CloseableCredentialsProvider { | ||
private val credentialsBusinessMetrics: MutableSet<BusinessMetric> = mutableSetOf() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correctness:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It actually does From
From
You can double check we're using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's the current implementation, but it's not guaranteed and could change in a future version of Kotlin. It's unlikely, and I'm open to keeping this |
||
|
||
public constructor( | ||
profileName: String? = null, | ||
|
@@ -131,12 +134,21 @@ public class ProfileCredentialsProvider @InternalSdkApi constructor( | |
|
||
chain.roles.forEach { roleArn -> | ||
logger.debug { "Assuming role `${roleArn.roleArn}`" } | ||
if (roleArn.source == RoleArnSource.SOURCE_PROFILE) { | ||
credentialsBusinessMetrics.add(AwsBusinessMetric.Credentials.CREDENTIALS_PROFILE_SOURCE_PROFILE) | ||
} | ||
|
||
val assumeProvider = roleArn.toCredentialsProvider(creds, region) | ||
|
||
creds.attributes.getOrNull(BusinessMetrics)?.forEach { metric -> | ||
credentialsBusinessMetrics.add(metric) | ||
} | ||
|
||
creds = assumeProvider.resolve(attributes) | ||
} | ||
|
||
logger.debug { "Obtained credentials from profile; expiration=${creds.expiration?.format(TimestampFormat.ISO_8601)}" } | ||
return creds | ||
return creds.emitBusinessMetrics(credentialsBusinessMetrics) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: We're calling |
||
} | ||
|
||
override fun close() { | ||
|
@@ -147,10 +159,13 @@ public class ProfileCredentialsProvider @InternalSdkApi constructor( | |
|
||
private suspend fun LeafProvider.toCredentialsProvider(region: LazyAsyncValue<String?>): CredentialsProvider = | ||
when (this) { | ||
is LeafProvider.NamedSource -> namedProviders[name] | ||
?: throw ProviderConfigurationException("unknown credentials source: $name") | ||
is LeafProvider.NamedSource -> namedProviders[name].also { | ||
credentialsBusinessMetrics.add(AwsBusinessMetric.Credentials.CREDENTIALS_PROFILE_NAMED_PROVIDER) | ||
} ?: throw ProviderConfigurationException("unknown credentials source: $name") | ||
|
||
is LeafProvider.AccessKey -> StaticCredentialsProvider(credentials) | ||
is LeafProvider.AccessKey -> StaticCredentialsProvider(credentials).also { | ||
credentialsBusinessMetrics.add(AwsBusinessMetric.Credentials.CREDENTIALS_PROFILE) | ||
} | ||
|
||
is LeafProvider.WebIdentityTokenRole -> StsWebIdentityCredentialsProvider( | ||
roleArn, | ||
|
@@ -159,7 +174,9 @@ public class ProfileCredentialsProvider @InternalSdkApi constructor( | |
roleSessionName = sessionName, | ||
platformProvider = platformProvider, | ||
httpClient = httpClient, | ||
) | ||
).also { | ||
credentialsBusinessMetrics.add(AwsBusinessMetric.Credentials.CREDENTIALS_PROFILE_STS_WEB_ID_TOKEN) | ||
} | ||
|
||
is LeafProvider.SsoSession -> SsoCredentialsProvider( | ||
accountId = ssoAccountId, | ||
|
@@ -169,7 +186,9 @@ public class ProfileCredentialsProvider @InternalSdkApi constructor( | |
ssoSessionName = ssoSessionName, | ||
httpClient = httpClient, | ||
platformProvider = platformProvider, | ||
) | ||
).also { | ||
credentialsBusinessMetrics.add(AwsBusinessMetric.Credentials.CREDENTIALS_PROFILE_SSO) | ||
} | ||
|
||
is LeafProvider.LegacySso -> SsoCredentialsProvider( | ||
accountId = ssoAccountId, | ||
|
@@ -178,9 +197,13 @@ public class ProfileCredentialsProvider @InternalSdkApi constructor( | |
ssoRegion = ssoRegion, | ||
httpClient = httpClient, | ||
platformProvider = platformProvider, | ||
) | ||
).also { | ||
credentialsBusinessMetrics.add(AwsBusinessMetric.Credentials.CREDENTIALS_PROFILE_SSO_LEGACY) | ||
} | ||
|
||
is LeafProvider.Process -> ProcessCredentialsProvider(command) | ||
is LeafProvider.Process -> ProcessCredentialsProvider(command).also { | ||
credentialsBusinessMetrics.add(AwsBusinessMetric.Credentials.CREDENTIALS_PROFILE_PROCESS) | ||
} | ||
} | ||
|
||
private suspend fun RoleArn.toCredentialsProvider( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correctness: Backwards-incompatible changes