Skip to content

Conversation

@aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Dec 10, 2021

Issue #

closes #355
closes #298
closes #461
closes #419
closes #357
closes #252

Description of changes

  • Implement SSO provider and add to default chain
  • Refactor the credential providers to no longer require CRT

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-feat-cred-providers

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-feat-cred-providers

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-feat-cred-providers

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-feat-cred-providers

@github-actions
Copy link

github-actions bot commented Jan 5, 2022

A new generated diff is ready to view: __generated-main...__generated-feat-cred-providers

@github-actions
Copy link

github-actions bot commented Jan 5, 2022

A new generated diff is ready to view: __generated-main...__generated-feat-cred-providers

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-feat-cred-providers

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-feat-cred-providers

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-feat-cred-providers

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-feat-cred-providers

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-feat-cred-providers

@github-actions
Copy link

github-actions bot commented Feb 1, 2022

A new generated diff is ready to view: __generated-main...__generated-feat-cred-providers

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

A new generated diff is ready to view: __generated-main...__generated-feat-cred-providers

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-feat-cred-providers

@aajtodd aajtodd marked this pull request as ready for review February 16, 2022 20:33
@aajtodd aajtodd requested a review from a team as a code owner February 16, 2022 20:33
@aajtodd aajtodd requested a review from ianbotsf February 16, 2022 20:33
Comment on lines +165 to +179
/**
* Returns the current result if not null or computes it by invoking [fn]
*/
private inline fun LeafProviderResult?.unwrapOrElse(fn: () -> LeafProviderResult): LeafProviderResult = when (this) {
null -> fn()
else -> this
}

/**
* Return current result if not null, otherwise use the result from calling [fn]
*/
private inline fun LeafProviderResult?.orElse(fn: () -> LeafProviderResult?): LeafProviderResult? = when (this) {
null -> fn()
else -> this
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment: I don't quite understand why these functions are necessary over the built-in Elvis operator. Looking at the call site, I see:

return webIdentityTokenCreds()
    .orElse(::ssoCreds)
    .unwrapOrElse(::staticCreds)
    .unwrap()

Can't this be simplified to:

return (webIdentityTokenCreds() ?: ssoCreds() ?: staticCreds()).unwrap()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't recall other than it was actually you that liked this change originally. It's a matter of taste really as changing it back to elvis operator will require throwing the configuration exceptions in each function vs returning an error.

I'm going to leave it since it's already been tested and we've already reviewed. If we add more leaf providers at some point we can revisit

// FIXME - note this won't work until https://github.com/awslabs/aws-crt-java/issues/252 is resolved
source = builder.source?.let { CredentialsProviderCrtProxy(it) }
}
private var cachedCredentials = CachedValue<Credentials>(null, bufferTime = refreshBufferWindow, clock)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Could be val.

build.gradle.kts Outdated
Comment on lines 86 to 96
// generated sts/sso credential providers have quite a few warnings
val optOutWErrorProjects = setOf("aws-config")

subprojects {
tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile> {
kotlinOptions.allWarningsAsErrors = true
if (name !in optOutWErrorProjects) {
// FIXME - this is NOT lazily evaluated
tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile> {
kotlinOptions.allWarningsAsErrors = true
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than maintaining a denylist, wouldn't it be simpler to turn off allWarningsAsErrors directly in aws-config's build file similar to what we do for dokka-aws?

Comment on lines 16 to 19
"aws.sdk.kotlin.runtime.InternalSdkApi",
"aws.sdk.kotlin.runtime.InternalSdkApi"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Removed trailing comma.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 20 Code Smells

No Coverage information No Coverage information
1.7% 1.7% Duplication

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-feat-cred-providers

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

Labels

None yet

Projects

None yet

2 participants