chore: Refactor internal clients #1995
Conversation
| @@ -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" } | |||
There was a problem hiding this comment.
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.
| @@ -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.
AWSSDKIdentityAPI is published for use by customers (if they customize S3 Express) and in tests.
| 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.
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.
Q: Where was AWSSDKIdentity dependent on internal clients?
Oh, I see that AWSSDKIdentity -> internal client dependency was introduced in this PR to allow credential resolvers to instantiate internal clients directly 👍
There was a problem hiding this comment.
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.
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.
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.
| name: "InternalAWSCommon", | ||
| dependencies: [.smithy, .smithyIdentity], | ||
| path: "Sources/Core/AWSSDKIdentity/Sources/InternalAWSCommon" | ||
| ), |
There was a problem hiding this comment.
This is a very simple target with a dummy AWS credential identity resolver, not dependent on AWSSDKIdentity, that is used as the default identity resolver in the internal clients.
This target is not published, and is only linked to the internal clients.
| .awsSDKHTTPAuth, | ||
| .awsSDKEventStreamsAuth, | ||
| .awsSDKChecksums, | ||
| "InternalAWSCommon", |
There was a problem hiding this comment.
In this & the other 2 internal clients, the AWSSDKIdentity dependency is removed since that module is no longer referenced in the client. The InternalAWSCommon module is added since that is where the internal clients get their default identity resolvers from.
| .awsSDKIdentity, | ||
| .awsSDKHTTPAuth, | ||
| .awsSDKEventStreamsAuth, | ||
| .awsSDKChecksums, | ||
| "InternalAWSSTS", | ||
| "InternalAWSSSO", | ||
| "InternalAWSSSOOIDC", |
There was a problem hiding this comment.
The 3 internal clients are removed since they are not direct dependencies of the client (they are still linked though, as dependencies of AWSSDKIdentity.)
| @@ -104,6 +105,7 @@ private func integrationTestTarget(_ name: String) -> Target { | |||
| .awsClientRuntime, | |||
| .smithyTestUtil, | |||
| .awsSDKIdentity, | |||
| .awsSDKIdentityAPI, | |||
There was a problem hiding this comment.
AWSSDKIdentityAPI is added as a dependency of the integration test targets, so its types may be accessed in tests.
| @@ -448,6 +448,7 @@ extension Target.Dependency { | |||
| static var awsSDKEventStreamsAuth: Self { "AWSSDKEventStreamsAuth" } | |||
There was a problem hiding this comment.
For this file, see comments on Package.base.txt above
| + "Missing IdentityProvidingSSOClient in identity properties." | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
Same change made in the following identity resolvers.
| // All Rights Reserved. | ||
| // | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // |
There was a problem hiding this comment.
This file and the next two are basically unchanged from the previous IdentityProviding...Client types, except that they are now permanent runtime files & not code-generated. (I only adjusted imports & formatting, and removed the protocol conformances.) Since AWSSDKIdentity can now depend on the internal clients, these wrappers are moved into AWSSDKIdentity and no longer need to be generated. The protocols for these wrappers' interfaces were removed because they are also not needed anymore.
I chose to keep these wrappers even though they're technically no longer needed because:
- they keep a lot of setup code out of the credential resolvers
- there's less change (and less chance of introducing bugs) from our current arrangement.
|
|
||
| import struct Smithy.Attributes | ||
| import protocol SmithyIdentity.AWSCredentialIdentityResolver | ||
| import struct SmithyIdentity.AWSCredentialIdentity |
There was a problem hiding this comment.
This AWS credential identity resolver is used as the default value for AWS credential identity resolver in the "internal" clients. Using this in place of the "default chain" resolvers eliminates the internal clients' dependence on AWSSDKIdentity.
Note that this client throws if you attempt to resolve credentials with it. That's okay because the internal clients never actually use it; they either access "noAuth" operations or the identity resolver statically provides it credentials.
| @@ -14,20 +14,20 @@ import protocol SmithyHTTPAuthAPI.AuthSchemeResolver | |||
| import protocol SmithyHTTPAuthAPI.AuthSchemeResolverParameters | |||
| import struct SmithyHTTPAuthAPI.AuthOption | |||
There was a problem hiding this comment.
The visibility of this and the other internal clients has been changed from internal to package so that AWSSDKIdentity can access these clients, but they are invisible to customers.
| // All Rights Reserved. | ||
| // | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // |
There was a problem hiding this comment.
This and the other internal client "wrappers" have been moved to AWSSDKIdentity (see above) and are no longer code-generated.
| @@ -7,18 +7,18 @@ | |||
|
|
|||
| // Code generated by smithy-swift-codegen. DO NOT EDIT! | |||
|
|
|||
| import class AWSSDKIdentity.DefaultAWSCredentialIdentityResolverChain | |||
| import protocol ClientRuntime.ClientConfiguration | |||
There was a problem hiding this comment.
Notice in all 3 internal clients' source files that the references to AWSSDKIdentity are all gone.
| } | ||
| }, | ||
| ).render(ctx) | ||
| AuthSchemeResolverGenerator().render(ctx) |
There was a problem hiding this comment.
Internal clients are no longer set in identity properties since the AWS credential resolvers can now create them directly.
| ) | ||
| } else { | ||
| writer.write("\$N(),", AWSSDKIdentityTypes.DefaultAWSCredentialIdentityResolverChain) | ||
| } |
There was a problem hiding this comment.
Here & below, default value for awsCredentialIdentityResolver is set differently for internal or normal clients.
| @@ -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.
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.
BTW I re-ran this test on the internal build system, it now passes after the refactor.
| @@ -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 = ClientConfigDefaultBearerTokenIdentityResolver(try DefaultBearerTokenIdentityResolverChain()) | |||
| context.addIdentityResolver(value: resolver, schemeID: "smithy.api#httpBearerAuth") | |||
There was a problem hiding this comment.
The bearer token identity resolver is wrapped in a ClientConfigDefaultBearerTokenIdentityResolver to signify that it was not customer-supplied.
| @@ -6,14 +6,14 @@ | |||
| // | |||
|
|
|||
| import class Foundation.ProcessInfo | |||
| import struct AWSSDKIdentity.DefaultBearerTokenIdentityResolverChain | |||
There was a problem hiding this comment.
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:
AWSSDKIdentity -> InternalClient -> AWSClientRuntime -> AWSSDKIdentity
| "config.awsCredentialIdentityResolver = \$N()", | ||
| AWSSDKIdentityTypes.DefaultAWSCredentialIdentityResolverChain, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Above, internal clients get a private type of AWSCredentialIdentityResolver that does not depend on AWSSDKIdentity. This, combined with breaking a couple other dependency cycles in runtime modules, means that the internal clients have no reference back to AWSSDKIdentity, and the credential resolvers in those packages can create and use those internal clients directly.
External clients continue to get the same "default chain" identity resolver from AWSSDKIdentity.
| if (AuthUtils(ctx).isSupportedAuthScheme(HttpBearerAuthTrait.ID)) { | ||
| writer.write( | ||
| "config.bearerTokenIdentityResolver = try \$N()", | ||
| "config.bearerTokenIdentityResolver = try \$N(\$N())", | ||
| SmithyIdentityTypes.ClientConfigDefaultBearerTokenIdentityResolver, | ||
| AWSSDKIdentityTypes.DefaultBearerTokenIdentityResolverChain, |
There was a problem hiding this comment.
Above, the DefaultBearerTokenIdentityResolverChain resolver is wrapped in a ClientConfigDefaultBearerTokenIdentityResolver to signify that it was set as a default value.
| @@ -41,51 +41,27 @@ private struct InternalModeledS3AuthSchemeResolver: S3AuthSchemeResolver { | |||
| switch serviceParams.operation { | |||
There was a problem hiding this comment.
Codegen test changes are below. Note that auth scheme resolvers don't have to set clients in identity properties anymore; this results in about 13,000 lines less code across the SDK.
There was a problem hiding this comment.
Really nice improvements to internal client consumption for credential resolution process! Everything looks good, but just got one comment on companion PR: smithy-lang/smithy-swift#952 (comment). After that comment is resolved will approve both PRs.
| @@ -7,6 +7,7 @@ | |||
|
|
|||
| import protocol SmithyIdentity.BearerTokenIdentityResolver | |||
| import struct SmithyIdentity.BearerTokenIdentity | |||
| @_spi(ClientConfigDefaultIdentityResolver) import protocol SmithyIdentityAPI.ClientConfigDefaultIdentityResolver | |||
There was a problem hiding this comment.
Below, added ClientConfigDefaultIdentityResolver conformance to this type.
When it's created by passing in a chain, it's not a client config default.
When it's created with no params, it's a client config default. (This initializer is package-scoped so customers can't use it.)
| @@ -73,6 +73,7 @@ data class AwsService( | |||
| val gitRepo: String, | |||
| val sdkId: String, | |||
| val visibility: String, | |||
| val internalClient: Boolean, | |||
There was a problem hiding this comment.
This file adds support for the new internalClient field on SwiftSettings and uses that field for generating internal clients for use in identity resolvers.
Description of changes
Breaks all dependencies between
AWSSDKIdentityand internal clients, so that internal clients can be created and used directly by credential resolvers without being injected after creation.Advantages:
To accomplish this, all dependencies from the internal clients to
AWSSDKIdentityhad to be eliminated. This involves eliminating these three dependency cycles:AWSSDKHTTPAuthtoAWSSDKIdentity. This existed because the AWSSigV4Signer takes a S3Express identity for signing when used in the S3Express signing mode. This was broken by moving the S3Express identity type & identity resolver protocol into a new moduleAWSSDKIdentityAPIthat contains only data types & interfaces. Typealiases make the S3Express-related types available at their original locations.AWSClientRuntimetoAWSSDKIdentitythat resulted from the Bedrock API Key customization referencing a credential resolver type directly. The customization was changed so that it no longer needs to reference concrete types inAWSSDKIdentityto do its job.AWSSDKIdentityidentity resolvers as their default values. A new default resolver was created for internal clients which is not located inAWSSDKIdentity. This is okay because internal clients never use the default resolver anyway (they either use noAuth operations or are given static credentials.)New/existing dependencies impact assessment, if applicable
No new dependencies were added to this change.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.