-
Notifications
You must be signed in to change notification settings - Fork 88
chore: Refactor internal clients #1995
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
Changes from all commits
16c3304
6e3b7ad
cccac6c
8c55a43
ea31cca
0d6a278
b4a2860
af96db1
f3a7201
3d2780c
f02902d
e173cce
be5269d
f66280b
7d5882a
99fd8b1
c62007a
85189f3
39b780f
fe88170
bcfc791
ae91bf1
b281baa
fe81170
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 |
---|---|---|
|
@@ -9,6 +9,7 @@ extension Target.Dependency { | |
static var awsSDKEventStreamsAuth: Self { "AWSSDKEventStreamsAuth" } | ||
static var awsSDKHTTPAuth: Self { "AWSSDKHTTPAuth" } | ||
static var awsSDKIdentity: Self { "AWSSDKIdentity" } | ||
static var awsSDKIdentityAPI: Self { "AWSSDKIdentityAPI" } | ||
static var awsSDKChecksums: Self { "AWSSDKChecksums" } | ||
static var awsSDKPartitions: Self { "AWSSDKPartitions" } | ||
|
||
|
@@ -59,7 +60,7 @@ let package = Package( | |
// MARK: Products | ||
|
||
private var runtimeProducts: [Product] { | ||
["AWSClientRuntime", "AWSSDKCommon", "AWSSDKEventStreamsAuth", "AWSSDKHTTPAuth", "AWSSDKIdentity", "AWSSDKChecksums"] | ||
["AWSClientRuntime", "AWSSDKCommon", "AWSSDKEventStreamsAuth", "AWSSDKHTTPAuth", "AWSSDKIdentityAPI", "AWSSDKIdentity", "AWSSDKChecksums"] | ||
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. AWSSDKIdentityAPI is published for use by customers (if they customize S3 Express) and in tests. |
||
.map { .library(name: $0, targets: [$0]) } | ||
} | ||
|
||
|
@@ -105,6 +106,7 @@ private var runtimeTargets: [Target] { | |
dependencies: [ | ||
.crt, | ||
.clientRuntime, | ||
.smithyIdentity, | ||
.smithyRetriesAPI, | ||
.smithyRetries, | ||
.awsSDKCommon, | ||
|
@@ -120,21 +122,26 @@ private var runtimeTargets: [Target] { | |
.target( | ||
name: "AWSSDKCommon", | ||
dependencies: [.crt], | ||
path: "Sources/Core/AWSSDKCommon/Sources" | ||
path: "Sources/Core/AWSSDKCommon/Sources/AWSSDKCommon" | ||
), | ||
.target( | ||
name: "AWSSDKEventStreamsAuth", | ||
dependencies: [.smithyEventStreamsAPI, .smithyEventStreamsAuthAPI, .smithyEventStreams, .crt, .clientRuntime, "AWSSDKHTTPAuth"], | ||
path: "Sources/Core/AWSSDKEventStreamsAuth/Sources" | ||
path: "Sources/Core/AWSSDKEventStreamsAuth/Sources/AWSSDKEventStreamsAuth" | ||
), | ||
.target( | ||
name: "AWSSDKHTTPAuth", | ||
dependencies: [.crt, .smithy, .clientRuntime, .smithyHTTPAuth, "AWSSDKChecksums", "AWSSDKIdentity"], | ||
path: "Sources/Core/AWSSDKHTTPAuth/Sources" | ||
dependencies: [.crt, .smithy, .clientRuntime, .smithyHTTPAuth, .awsSDKIdentityAPI, "AWSSDKChecksums"], | ||
path: "Sources/Core/AWSSDKHTTPAuth/Sources/AWSSDKHTTPAuth" | ||
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. Switched the AWSSDKHTTPAuth dependency from AWSSDKIdentity to the new AWSSDKIdentityAPI. This breaks a AWSSDKIdentity -> InternalAWSClient -> AWSSDKHTTPAuth -> AWSSDKIdentity circular dependency. 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.
Oh, I see that 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. Exactly 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. To be clear, the "circular dependencies" I refer to are those created by adding the internal AWS clients as dependencies of AWSSDKIdentity, before making the changes in this PR to break them. |
||
), | ||
.target( | ||
name: "AWSSDKIdentityAPI", | ||
dependencies: [.smithy, .smithyIdentityAPI], | ||
path: "Sources/Core/AWSSDKIdentityAPI/Sources/AWSSDKIdentityAPI" | ||
), | ||
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. The new AWSSDKIdentityAPI target above depends only on Smithy API modules. |
||
.target( | ||
name: "AWSSDKIdentity", | ||
dependencies: [.crt, .smithy, .clientRuntime, .smithyIdentity, .smithyIdentityAPI, .smithyHTTPAPI, .awsSDKCommon], | ||
dependencies: [.awsSDKIdentityAPI, .crt, .smithy, .clientRuntime, .smithyIdentity, .smithyIdentityAPI, .smithyHTTPAPI, .awsSDKCommon, "InternalAWSSTS", "InternalAWSSSO", "InternalAWSSSOOIDC", ], | ||
path: "Sources/Core/AWSSDKIdentity/Sources/AWSSDKIdentity" | ||
), | ||
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. AWSSDKIdentity now depends on AWSSDKIdentityAPI, and also depends directly on the 3 internal clients. This is no longer a circular dependency because the internal clients don't depend on AWSSDKIdentity. This also allows us to get rid of almost all of the boiler plate and code generation that we used to use to allow identity resolvers to access these clients, since they're now direct dependencies. |
||
.target( | ||
|
@@ -154,7 +161,6 @@ private var runtimeTargets: [Target] { | |
.smithyChecksums, | ||
.smithyWaitersAPI, | ||
.awsSDKCommon, | ||
.awsSDKIdentity, | ||
.awsSDKHTTPAuth, | ||
.awsSDKEventStreamsAuth, | ||
.awsSDKChecksums, | ||
|
@@ -178,7 +184,6 @@ private var runtimeTargets: [Target] { | |
.smithyChecksums, | ||
.smithyWaitersAPI, | ||
.awsSDKCommon, | ||
.awsSDKIdentity, | ||
.awsSDKHTTPAuth, | ||
.awsSDKEventStreamsAuth, | ||
.awsSDKChecksums, | ||
|
@@ -202,7 +207,6 @@ private var runtimeTargets: [Target] { | |
.smithyChecksums, | ||
.smithyWaitersAPI, | ||
.awsSDKCommon, | ||
.awsSDKIdentity, | ||
.awsSDKHTTPAuth, | ||
.awsSDKEventStreamsAuth, | ||
.awsSDKChecksums, | ||
|
@@ -226,7 +230,7 @@ private var runtimeTestTargets: [Target] { | |
return [ | ||
.testTarget( | ||
name: "AWSClientRuntimeTests", | ||
dependencies: [.awsClientRuntime, .clientRuntime, .smithyTestUtils, .awsSDKCommon], | ||
dependencies: [.awsClientRuntime, .clientRuntime, .smithyTestUtils, .awsSDKCommon, .awsSDKIdentity], | ||
path: "Sources/Core/AWSClientRuntime/Tests/AWSClientRuntimeTests", | ||
resources: [.process("Resources")] | ||
), | ||
|
@@ -242,7 +246,7 @@ private var runtimeTestTargets: [Target] { | |
), | ||
.testTarget( | ||
name: "AWSSDKIdentityTests", | ||
dependencies: [.smithy, .smithyIdentity, "AWSSDKIdentity", .awsClientRuntime], | ||
dependencies: ["AWSSDKIdentity", .smithy, .smithyIdentity, .awsClientRuntime], | ||
path: "Sources/Core/AWSSDKIdentity/Tests/AWSSDKIdentityTests", | ||
resources: [.process("Resources")] | ||
), | ||
|
@@ -267,13 +271,11 @@ private func target(_ service: String) -> Target { | |
.smithyChecksums, | ||
.smithyWaitersAPI, | ||
.awsSDKCommon, | ||
.awsSDKIdentityAPI, | ||
.awsSDKIdentity, | ||
.awsSDKHTTPAuth, | ||
.awsSDKEventStreamsAuth, | ||
.awsSDKChecksums, | ||
"InternalAWSSTS", | ||
"InternalAWSSSO", | ||
"InternalAWSSSOOIDC", | ||
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. The 3 internal clients are removed since they are not direct dependencies of the client (they are still linked though, as dependencies of AWSSDKIdentity.) |
||
], | ||
path: "Sources/Services/\(service)/Sources/\(service)" | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ extension Target.Dependency { | |
// AWS modules | ||
static var awsClientRuntime: Self { .product(name: "AWSClientRuntime", package: "aws-sdk-swift") } | ||
static var awsSDKCommon: Self { .product(name: "AWSSDKCommon", package: "aws-sdk-swift") } | ||
static var awsSDKIdentityAPI: Self { .product(name: "AWSSDKIdentityAPI", package: "aws-sdk-swift") } | ||
static var awsSDKIdentity: Self { .product(name: "AWSSDKIdentity", package: "aws-sdk-swift") } | ||
|
||
// Smithy modules | ||
|
@@ -104,6 +105,7 @@ private func integrationTestTarget(_ name: String) -> Target { | |
.awsClientRuntime, | ||
.smithyTestUtil, | ||
.awsSDKIdentity, | ||
.awsSDKIdentityAPI, | ||
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. AWSSDKIdentityAPI is added as a dependency of the integration test targets, so its types may be accessed in tests. |
||
.smithyIdentity, | ||
.awsSDKCommon, | ||
.awsIntegrationTestUtils, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ final class BedrockAPIKeyIntegrationTests: XCTestCase { | |
let envVarName = "AWS_BEARER_TOKEN_BEDROCK" | ||
let apiKeyDuration: TimeInterval = 600.0 | ||
|
||
func xtest_apiKey_createsAPIKeyAndCallsWithIt() async throws { | ||
func test_apiKey_createsAPIKeyAndCallsWithIt() async throws { | ||
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. Re-enabling this integration test; it's the one which was causing problems on systems that assume a role before testing. 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. BTW I re-ran this test on the internal build system, it now passes after the refactor. |
||
// Set a Bedrock API token into the environment. | ||
let generator = BedrockAPIKeyGenerator(region: region, duration: apiKeyDuration) | ||
let token = try await generator.generate() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -449,6 +449,7 @@ extension Target.Dependency { | |
static var awsSDKEventStreamsAuth: Self { "AWSSDKEventStreamsAuth" } | ||
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. For this file, see comments on Package.base.txt above |
||
static var awsSDKHTTPAuth: Self { "AWSSDKHTTPAuth" } | ||
static var awsSDKIdentity: Self { "AWSSDKIdentity" } | ||
static var awsSDKIdentityAPI: Self { "AWSSDKIdentityAPI" } | ||
static var awsSDKChecksums: Self { "AWSSDKChecksums" } | ||
static var awsSDKPartitions: Self { "AWSSDKPartitions" } | ||
|
||
|
@@ -499,7 +500,7 @@ let package = Package( | |
// MARK: Products | ||
|
||
private var runtimeProducts: [Product] { | ||
["AWSClientRuntime", "AWSSDKCommon", "AWSSDKEventStreamsAuth", "AWSSDKHTTPAuth", "AWSSDKIdentity", "AWSSDKChecksums"] | ||
["AWSClientRuntime", "AWSSDKCommon", "AWSSDKEventStreamsAuth", "AWSSDKHTTPAuth", "AWSSDKIdentityAPI", "AWSSDKIdentity", "AWSSDKChecksums"] | ||
.map { .library(name: $0, targets: [$0]) } | ||
} | ||
|
||
|
@@ -545,6 +546,7 @@ private var runtimeTargets: [Target] { | |
dependencies: [ | ||
.crt, | ||
.clientRuntime, | ||
.smithyIdentity, | ||
.smithyRetriesAPI, | ||
.smithyRetries, | ||
.awsSDKCommon, | ||
|
@@ -560,21 +562,26 @@ private var runtimeTargets: [Target] { | |
.target( | ||
name: "AWSSDKCommon", | ||
dependencies: [.crt], | ||
path: "Sources/Core/AWSSDKCommon/Sources" | ||
path: "Sources/Core/AWSSDKCommon/Sources/AWSSDKCommon" | ||
), | ||
.target( | ||
name: "AWSSDKEventStreamsAuth", | ||
dependencies: [.smithyEventStreamsAPI, .smithyEventStreamsAuthAPI, .smithyEventStreams, .crt, .clientRuntime, "AWSSDKHTTPAuth"], | ||
path: "Sources/Core/AWSSDKEventStreamsAuth/Sources" | ||
path: "Sources/Core/AWSSDKEventStreamsAuth/Sources/AWSSDKEventStreamsAuth" | ||
), | ||
.target( | ||
name: "AWSSDKHTTPAuth", | ||
dependencies: [.crt, .smithy, .clientRuntime, .smithyHTTPAuth, "AWSSDKChecksums", "AWSSDKIdentity"], | ||
path: "Sources/Core/AWSSDKHTTPAuth/Sources" | ||
dependencies: [.crt, .smithy, .clientRuntime, .smithyHTTPAuth, .awsSDKIdentityAPI, "AWSSDKChecksums"], | ||
path: "Sources/Core/AWSSDKHTTPAuth/Sources/AWSSDKHTTPAuth" | ||
), | ||
.target( | ||
name: "AWSSDKIdentityAPI", | ||
dependencies: [.smithy, .smithyIdentityAPI], | ||
path: "Sources/Core/AWSSDKIdentityAPI/Sources/AWSSDKIdentityAPI" | ||
), | ||
.target( | ||
name: "AWSSDKIdentity", | ||
dependencies: [.crt, .smithy, .clientRuntime, .smithyIdentity, .smithyIdentityAPI, .smithyHTTPAPI, .awsSDKCommon], | ||
dependencies: [.awsSDKIdentityAPI, .crt, .smithy, .clientRuntime, .smithyIdentity, .smithyIdentityAPI, .smithyHTTPAPI, .awsSDKCommon, "InternalAWSSTS", "InternalAWSSSO", "InternalAWSSSOOIDC", ], | ||
path: "Sources/Core/AWSSDKIdentity/Sources/AWSSDKIdentity" | ||
), | ||
.target( | ||
|
@@ -594,7 +601,6 @@ private var runtimeTargets: [Target] { | |
.smithyChecksums, | ||
.smithyWaitersAPI, | ||
.awsSDKCommon, | ||
.awsSDKIdentity, | ||
.awsSDKHTTPAuth, | ||
.awsSDKEventStreamsAuth, | ||
.awsSDKChecksums, | ||
|
@@ -618,7 +624,6 @@ private var runtimeTargets: [Target] { | |
.smithyChecksums, | ||
.smithyWaitersAPI, | ||
.awsSDKCommon, | ||
.awsSDKIdentity, | ||
.awsSDKHTTPAuth, | ||
.awsSDKEventStreamsAuth, | ||
.awsSDKChecksums, | ||
|
@@ -642,7 +647,6 @@ private var runtimeTargets: [Target] { | |
.smithyChecksums, | ||
.smithyWaitersAPI, | ||
.awsSDKCommon, | ||
.awsSDKIdentity, | ||
.awsSDKHTTPAuth, | ||
.awsSDKEventStreamsAuth, | ||
.awsSDKChecksums, | ||
|
@@ -666,7 +670,7 @@ private var runtimeTestTargets: [Target] { | |
return [ | ||
.testTarget( | ||
name: "AWSClientRuntimeTests", | ||
dependencies: [.awsClientRuntime, .clientRuntime, .smithyTestUtils, .awsSDKCommon], | ||
dependencies: [.awsClientRuntime, .clientRuntime, .smithyTestUtils, .awsSDKCommon, .awsSDKIdentity], | ||
path: "Sources/Core/AWSClientRuntime/Tests/AWSClientRuntimeTests", | ||
resources: [.process("Resources")] | ||
), | ||
|
@@ -682,7 +686,7 @@ private var runtimeTestTargets: [Target] { | |
), | ||
.testTarget( | ||
name: "AWSSDKIdentityTests", | ||
dependencies: [.smithy, .smithyIdentity, "AWSSDKIdentity", .awsClientRuntime], | ||
dependencies: ["AWSSDKIdentity", .smithy, .smithyIdentity, .awsClientRuntime], | ||
path: "Sources/Core/AWSSDKIdentity/Tests/AWSSDKIdentityTests", | ||
resources: [.process("Resources")] | ||
), | ||
|
@@ -707,13 +711,11 @@ private func target(_ service: String) -> Target { | |
.smithyChecksums, | ||
.smithyWaitersAPI, | ||
.awsSDKCommon, | ||
.awsSDKIdentityAPI, | ||
.awsSDKIdentity, | ||
.awsSDKHTTPAuth, | ||
.awsSDKEventStreamsAuth, | ||
.awsSDKChecksums, | ||
"InternalAWSSTS", | ||
"InternalAWSSSO", | ||
"InternalAWSSSOOIDC", | ||
], | ||
path: "Sources/Services/\(service)/Sources/\(service)" | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,13 +6,13 @@ | |
// | ||
|
||
import class Foundation.ProcessInfo | ||
import struct AWSSDKIdentity.DefaultBearerTokenIdentityResolverChain | ||
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 interceptor now looks for ClientConfigDefaultBearerTokenIdentityResolver (located in SmithyIdentity) instead of DefaultBearerTokenIdentityResolverChain (located in AWSSDKIdentity). As a result, AWSClientRuntime no longer depends on AWSSDKIdentity, breaking this dependency cycle: |
||
import protocol ClientRuntime.Interceptor | ||
import protocol ClientRuntime.AfterSerialization | ||
import struct Smithy.Attributes | ||
import struct Smithy.AttributeKey | ||
import class SmithyHTTPAPI.HTTPRequest | ||
import class SmithyHTTPAPI.HTTPResponse | ||
@_spi(ClientConfigDefaultIdentityResolver) import protocol SmithyIdentityAPI.ClientConfigDefaultIdentityResolver | ||
import protocol SmithyIdentity.BearerTokenIdentityResolver | ||
import struct SmithyIdentity.BearerTokenIdentity | ||
import struct SmithyIdentity.StaticBearerTokenIdentityResolver | ||
|
@@ -37,10 +37,10 @@ public struct BedrockAPIKeyInterceptor<InputType, OutputType>: Interceptor { | |
// If so, return immediately & use that instead of the Bedrock API token. | ||
let identityResolvers = attributes.getIdentityResolvers() ?? Attributes() | ||
let key = AttributeKey<any BearerTokenIdentityResolver>(name: "smithy.api#httpBearerAuth") | ||
guard !identityResolvers.contains(key: key) || identityResolvers.get(key: key) is | ||
DefaultBearerTokenIdentityResolverChain else { | ||
return | ||
} | ||
let configuredResolver = identityResolvers.get(key: key) | ||
let clientConfigDefaultIdentityResolver = configuredResolver as? any ClientConfigDefaultIdentityResolver | ||
let configuredResolverIsDefault = clientConfigDefaultIdentityResolver?.isClientConfigDefault ?? false | ||
guard configuredResolver == nil || configuredResolverIsDefault else { return } | ||
|
||
// Create a bearer token identity resolver with the resolved token, then | ||
// store it in the context. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -97,7 +97,8 @@ final class BedrockAPIKeyInterceptorTests: XCTestCase { | |
defer { unsetenv(envVarName) } | ||
let subject = BedrockAPIKeyInterceptor<String, String>() | ||
let context = Context(attributes: Attributes()) | ||
context.addIdentityResolver(value: try DefaultBearerTokenIdentityResolverChain(), schemeID: "smithy.api#httpBearerAuth") | ||
let resolver = DefaultBearerTokenIdentityResolverChain() | ||
context.addIdentityResolver(value: resolver, schemeID: "smithy.api#httpBearerAuth") | ||
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. The bearer token identity resolver is wrapped in a ClientConfigDefaultBearerTokenIdentityResolver to signify that it was not customer-supplied. |
||
let interceptorContext = DefaultInterceptorContext<String, String, HTTPRequest, HTTPResponse>(input: "", attributes: context) | ||
|
||
try await subject.readBeforeAttempt(context: interceptorContext) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,15 +58,6 @@ public struct SSOAWSCredentialIdentityResolver: AWSCredentialIdentityResolver { | |
} | ||
|
||
public func getIdentity(identityProperties: Attributes?) async throws -> AWSCredentialIdentity { | ||
guard let identityProperties, let internalSSOClient = identityProperties.get( | ||
key: InternalClientKeys.internalSSOClientKey | ||
) else { | ||
throw AWSCredentialIdentityResolverError.failedToResolveAWSCredentials( | ||
"SSOAWSCredentialIdentityResolver: " | ||
+ "Missing IdentityProvidingSSOClient in identity properties." | ||
) | ||
} | ||
|
||
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. No need to get the internal SSO client from identity properties anymore, since it can now be directly created by this type (just below). 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. Same change made in the following identity resolvers. |
||
let fileBasedConfig = try CRTFileBasedConfiguration( | ||
configFilePath: configFilePath, | ||
credentialsFilePath: credentialsFilePath | ||
|
@@ -97,7 +88,7 @@ public struct SSOAWSCredentialIdentityResolver: AWSCredentialIdentityResolver { | |
) | ||
} | ||
|
||
return try await internalSSOClient.getCredentialsWithSSOToken( | ||
return try await IdentityProvidingSSOClient().getCredentialsWithSSOToken( | ||
region: region, | ||
accessToken: ssoToken.token, | ||
accountID: accountID, | ||
|
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.
Added this package for identity-related things that aren't credential resolvers.
Right now this is only the S3 Express identity & identity resolver, but this broke a dependency between AWSSDKHTTPAuth & AWSSDKIdentity that was needed to eliminate circular dependencies on the internal clients.