feat: endpoints2: aws middleware and integration changes #751

lucix-aws · 2022-11-12T03:21:25Z

Description of changes

Implement aws extensions for smithy-kotlin generated endpoint provider middleware
Add new client configs to S3 and S3Control for endpoint builtins
Remove old partitions.json-based resolver generator

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

aajtodd · 2022-11-14T15:07:26Z

aws-runtime/aws-endpoint/common/src/aws/sdk/kotlin/runtime/endpoint/AwsEndpoint.kt

+ */
+@InternalSdkApi
+public fun Endpoint.authScheme(): AuthScheme.SigV4? =
+    attributes.getOrNull(AuthSchemesAttributeKey)?.find { it is AuthScheme.SigV4 } as AuthScheme.SigV4?


correctness: as? AuthScheme.SigV4

aajtodd · 2022-11-14T15:09:53Z

aws-runtime/aws-endpoint/common/src/aws/sdk/kotlin/runtime/endpoint/AwsEndpoint.kt

+/**
+ * Sugar extension to pull an auth scheme out of the attribute set.
+ *
+ * FUTURE: Right now we only support sigv4. The auth scheme property can include multiple schemes, for now we only pull


Should this return a list of auth schemes then?

Endpoint.authSchemes(): List<AuthScheme> = attributes.getOrNull(AuthSchemesAttributeKey) ?: emptyList()

Then have a separate extensions like Endpoint.hasSigV4 or Endpoint.supportsSigv4?

e.g.

@InternalSdkApi public fun Endpoint.supportsSigv4(): Boolean = authSchemes().find { it is AuthScheme.SigV4} != null

So, I don't think it's necessary in the current context of us only supporting sigv4. When we have sigv4a as a first-class citizen the logic to retrieve this (it's basically used for presigning and for setting request context values for request signing) would have to evolve and at that point it would need extensions like this.

aajtodd · 2022-11-14T15:36:44Z

...thy-aws-kotlin-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/AwsServiceConfigIntegration.kt

+            """.trimIndent(),
+        )
+
+        val EndpointUrlProp = ClientConfigProperty {


question/concern: Wouldn't this just be done by overriding the endpoint provider? This seems like two different ways to do the same thing to me but maybe I'm missing something

This is part of the endpoints SEP, it basically allows you to sideload an endpoint that you want to use in the ruleset and have it validated / transformed if necessary.

That said there's nothing preventing someone from writing their own endpoint provider that just blindly resolves to a static endpoint.

aajtodd · 2022-11-14T15:37:53Z

...thy-aws-kotlin-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/AwsServiceConfigIntegration.kt

@@ -51,6 +48,31 @@ class AwsServiceConfigIntegration : KotlinIntegration {
                AwsRuntimeTypes.Config.Credentials.DefaultChainCredentialsProvider,
            )
        }
+
+        val UseFipsProp: ClientConfigProperty = ClientConfigProperty.Boolean(


question: It is correct to add fips/dual-stack to every generated AWS client? Or should this be predicated on their use as builtins in rules?

It is part of every aws client, yes.

aajtodd · 2022-11-14T15:40:07Z

codegen/smithy-aws-kotlin-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/PresignerGenerator.kt

@@ -382,12 +403,9 @@ class PresignerGenerator : KotlinIntegration {
                symbol = RuntimeTypes.Auth.Signing.AwsSigningCommon.SigningEndpointProvider
                name = "endpointProvider"
                documentation =
-                    "Provides the endpoint (hostname) and signing context to make requests to. When not provided a default resolver is configured automatically. This is an advanced client option."
+                    "Provides the endpoint (hostname) and signing context to make requests to. You MUST pass a provider will resolve to an endpoint for the desired request."


fix: doc comment You MUST pass a provider that will resolve an endpoint ...

aajtodd · 2022-11-14T15:53:22Z

...otlin-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/protocols/endpoints/EndpointBuiltins.kt

+    ) {
+        builtinParams.forEach {
+            when (it.builtIn.get()) {
+                "AWS::Region" -> renderBasicConfigBinding(writer, it, "region")


nit: Can we use the config property names here rather than hard coding the strings? Or use shared constants?

aajtodd · 2022-11-14T15:54:23Z

...otlin-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/protocols/endpoints/EndpointBuiltins.kt

+                    writer.write("#L = config.endpointUrl?.toString()", EndpointParametersGenerator.getParamKotlinName(it))
+
+                // as a newer SDK we do NOT support these values, they are always false
+                "AWS::S3::UseGlobalEndpoint", "AWS::STS::UseGlobalEndpoint" ->


Question/Correctness: Should we even render this for the Kotlin SDK? This seems like a legacy concern that we could filter or make internal?

We have to force them false here if we definitely don't want to support this route in the SDK, since the ruleset could default them to true.

What I meant was should we even be surfacing this at the config level? I guess it's fine, it's just legacy cruft that doesn't really apply to us though.

aajtodd · 2022-11-14T15:57:01Z

...lin/aws/sdk/kotlin/codegen/protocols/protocoltest/AwsHttpProtocolUnitTestRequestGenerator.kt

@@ -59,4 +57,10 @@ internal fun <T : HttpMessageTestCase> HttpProtocolUnitTestGenerator<T>.renderCo

    // specify a default region
    writer.write("region = \"us-east-1\"")
+    writer.write(
+        "endpointResolver = #T { #T(#S) }",


fix: Isn't this the wrong config property name? (FWIW I still sort of think we should name the generated type/field EndpointResolver but I believe it's endpointProvider in generated config right now)

It was incorrectly different based on whether or not the service had a ruleset, that's getting fixed in this next iteration (it'll all be the modern endpointProvider).

aajtodd · 2022-11-14T16:00:14Z

...otlin-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/protocols/endpoints/EndpointBuiltins.kt

+ */
+fun renderBindAwsBuiltins(ctx: ProtocolGenerator.GenerationContext, writer: KotlinWriter, builtinParams: List<Parameter>) {
+    writer.withBlock(
+        "public fun #T.Builder.bindAwsBuiltins(config: #T.Config) {",


fix: internal

aajtodd · 2022-11-14T16:04:24Z

.../src/main/kotlin/aws/sdk/kotlin/codegen/protocols/middleware/ResolveAwsEndpointMiddleware.kt

 import software.amazon.smithy.model.shapes.OperationShape
+import software.amazon.smithy.rulesengine.language.EndpointRuleSet
+import software.amazon.smithy.rulesengine.traits.EndpointRuleSetTrait

 /**
 * HTTP client interceptor that resolves service endpoints for a single service
 */
 class ResolveAwsEndpointMiddleware(private val ctx: ProtocolGenerator.GenerationContext) : ProtocolMiddleware {


comment: It would be really nice if we didn't even need this custom middleware anymore and could just use the one from smithy-kotlin.

In fact I don't think we do need this.

We seem to have punted on any kind of SPI or hook to customize bindings (which is fine) but we never really needed one anyway.

We have extremely fine grained ways to customize how parts of the code are generated via SectionWriter which integrations can register.

An example of this in current use is the service client declares a section (in this case with default code being generated in others it's empty and serves as a hook to append, example). Example of that section being overridden in AWS SDK.

We don't have to go this route but looking at EndpointParameterBindingGenerator it actually seems like a reasonable use case to declare sections for rendering the various binding types (which will guarantee the order is correct). Integrations can wholesale override or append. Food for thought.

ianbotsf · 2022-11-14T16:34:26Z

codegen/sdk/aws-models/cloudwatch-logs.json

-                        "smithy.api#documentation": "<p>The end time of the range for the request, expressed as the number of milliseconds\n      after Jan 1, 1970 00:00:00 UTC. Events with a timestamp later than this time are not\n      exported.</p>",
+                        "smithy.api#documentation": "<p>The end time of the range for the request, expressed as the number of milliseconds\n      after Jan 1, 1970 00:00:00 UTC. Events with a timestamp later than this time are not\n      exported.</p>\n         <p>You must specify a time that is not earlier than when this log group was created.</p>",


Question: Why are there service model changes in this PR?

This was just an incidental model update to make sure I wasn't ever breaking anything provider-wise with the newest version of the rules I could get. We can respect the in-main versions when we merge, or take the update, I don't have a real preference.

ianbotsf · 2022-11-14T16:37:22Z

codegen/smithy-aws-kotlin-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/PresignerGenerator.kt

@@ -382,12 +403,9 @@ class PresignerGenerator : KotlinIntegration {
                symbol = RuntimeTypes.Auth.Signing.AwsSigningCommon.SigningEndpointProvider
                name = "endpointProvider"
                documentation =
-                    "Provides the endpoint (hostname) and signing context to make requests to. When not provided a default resolver is configured automatically. This is an advanced client option."
+                    "Provides the endpoint (hostname) and signing context to make requests to. You MUST pass a provider will resolve to an endpoint for the desired request."


Nit: "You MUST pass a provider that will resolve..."?

Also, we don't usually scream "MUST" in our KDocs. That's appropriate for specifications but documentation would generally say something like "this is a required parameter".

ianbotsf · 2022-11-14T16:47:29Z

.../src/main/kotlin/aws/sdk/kotlin/codegen/protocols/middleware/ResolveAwsEndpointMiddleware.kt

    override fun render(ctx: ProtocolGenerator.GenerationContext, op: OperationShape, writer: KotlinWriter) {
-        val resolverFeatureSymbol = buildSymbol {
-            name = "ResolveAwsEndpoint"
-            namespace(AwsKotlinDependency.AWS_HTTP, subpackage = "middleware")
+        if (!ctx.service.hasTrait<EndpointRuleSetTrait>()) {
+            writeDefault(writer)
+            return
+        }
+
+        val rules = EndpointRuleSet.fromNode(ctx.service.expectTrait<EndpointRuleSetTrait>().ruleSet)
+        val middlewareSymbol = ResolveEndpointMiddlewareGenerator.getSymbol(ctx.settings)
+        val (inputSymbol, outputSymbol) = OperationIndex.of(ctx.model).getOperationInputOutputSymbols(op, ctx.symbolProvider)
+
+        writer.withBlock(
+            "op.install(#T<#T, #T>(config.endpointProvider) { input ->",
+            "})",
+            middlewareSymbol,
+            inputSymbol,
+            outputSymbol,
+        ) {
+            write("#T(config)", bindAwsBuiltinsSymbol(ctx.settings))
+            EndpointParameterBindingGenerator(ctx.model, ctx.service, writer, op, rules, "input.").render()
        }
-        writer.addImport(resolverFeatureSymbol)
-        writer.write("op.install(#T(ServiceId, config.endpointResolver))", resolverFeatureSymbol)
+    }


Question: Why should this be AWS-specific going forward? The Smithy spec now includes endpoint rules and in theory any custom service could use them, right?

It isn't, there's some minor code duplication to generate the default provider / tests in both this and the base ProtocolGenerator - that's a duplication we chose to take on in the provider PR based on how we expose the hook to generate endpoints code on ProtocolGenerator.

I think I misread your question. This isn't AWS-specific in the sense that there IS a base smithy version (in the other PR), this is just the AWS version that adds the builtin bindings.

insert, fix protocol tests

aajtodd

Couple questions (and a few unanswered from previous review). Overall looks really good, think we're close

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/ImdsEndpointProvider.kt

aws-runtime/aws-endpoint/common/src/aws/sdk/kotlin/runtime/endpoint/AwsEndpoint.kt

...n/src/main/kotlin/aws/sdk/kotlin/codegen/protocols/endpoints/EndpointProviderGeneratorExt.kt

...gen/src/main/kotlin/aws/sdk/kotlin/codegen/protocols/core/AwsHttpBindingProtocolGenerator.kt

.../src/main/kotlin/aws/sdk/kotlin/codegen/protocols/middleware/ResolveAwsEndpointMiddleware.kt

ianbotsf · 2022-11-17T23:54:10Z

aws-runtime/aws-endpoint/common/src/aws/sdk/kotlin/runtime/endpoint/AuthScheme.kt

+public val Endpoint.authScheme: AuthScheme.SigV4? get() =
+    attributes.getOrNull(AuthSchemesAttributeKey)?.find { it is AuthScheme.SigV4 } as? AuthScheme.SigV4


Style: I suggest moving the get() = to the next line with the getter body (ref: Kotlin coding conventions).

Surprised that ktlintFormat didn't catch this one tbh

aajtodd · 2022-11-18T14:17:04Z

.../src/main/kotlin/aws/sdk/kotlin/codegen/protocols/middleware/ResolveAwsEndpointMiddleware.kt

 import software.amazon.smithy.model.shapes.OperationShape
+import software.amazon.smithy.rulesengine.language.EndpointRuleSet
+import software.amazon.smithy.rulesengine.traits.EndpointRuleSetTrait

 /**
 * HTTP client interceptor that resolves service endpoints for a single service
 */
 class ResolveAwsEndpointMiddleware(private val ctx: ProtocolGenerator.GenerationContext) : ProtocolMiddleware {


In fact I don't think we do need this.

We seem to have punted on any kind of SPI or hook to customize bindings (which is fine) but we never really needed one anyway.

We have extremely fine grained ways to customize how parts of the code are generated via SectionWriter which integrations can register.

An example of this in current use is the service client declares a section (in this case with default code being generated in others it's empty and serves as a hook to append, example). Example of that section being overridden in AWS SDK.

We don't have to go this route but looking at EndpointParameterBindingGenerator it actually seems like a reasonable use case to declare sections for rendering the various binding types (which will guarantee the order is correct). Integrations can wholesale override or append. Food for thought.

sonarqubecloud · 2022-11-18T19:31:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

feat: endpoints2: implement aws endpoint middleware

c14fa15

lucix-aws requested a review from a team as a code owner November 12, 2022 03:21

aajtodd reviewed Nov 14, 2022

View reviewed changes

ianbotsf reviewed Nov 14, 2022

View reviewed changes

refactor for always-generated provider, add missing authschemes context

e8243a9

insert, fix protocol tests

aajtodd reviewed Nov 17, 2022

View reviewed changes

address some nits

4f34a1f

ianbotsf approved these changes Nov 17, 2022

View reviewed changes

aajtodd approved these changes Nov 18, 2022

View reviewed changes

final nits and make sure future auth schemes dont break us

9ab7df8

lucix-aws merged commit 96a835d into feat-endpoints2 Nov 18, 2022

aajtodd deleted the feat-endpoints2-middleware branch November 29, 2022 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: endpoints2: aws middleware and integration changes #751

feat: endpoints2: aws middleware and integration changes #751

lucix-aws commented Nov 12, 2022

aajtodd Nov 14, 2022

aajtodd Nov 14, 2022

lucix-aws Nov 17, 2022

aajtodd Nov 14, 2022

lucix-aws Nov 17, 2022

aajtodd Nov 14, 2022

lucix-aws Nov 17, 2022

aajtodd Nov 14, 2022

aajtodd Nov 14, 2022

aajtodd Nov 14, 2022

lucix-aws Nov 17, 2022

aajtodd Nov 17, 2022

aajtodd Nov 14, 2022

lucix-aws Nov 15, 2022

aajtodd Nov 14, 2022

aajtodd Nov 14, 2022

aajtodd Nov 18, 2022

ianbotsf Nov 14, 2022

lucix-aws Nov 15, 2022

ianbotsf Nov 14, 2022

ianbotsf Nov 14, 2022

lucix-aws Nov 15, 2022 •

edited

Loading

aajtodd left a comment

ianbotsf Nov 17, 2022

lucix-aws Nov 18, 2022

aajtodd Nov 18, 2022

sonarqubecloud bot commented Nov 18, 2022

		"smithy.api#documentation": "<p>The end time of the range for the request, expressed as the number of milliseconds\n after Jan 1, 1970 00:00:00 UTC. Events with a timestamp later than this time are not\n exported.</p>",
		"smithy.api#documentation": "<p>The end time of the range for the request, expressed as the number of milliseconds\n after Jan 1, 1970 00:00:00 UTC. Events with a timestamp later than this time are not\n exported.</p>\n <p>You must specify a time that is not earlier than when this log group was created.</p>",

		public val Endpoint.authScheme: AuthScheme.SigV4? get() =
		attributes.getOrNull(AuthSchemesAttributeKey)?.find { it is AuthScheme.SigV4 } as? AuthScheme.SigV4

feat: endpoints2: aws middleware and integration changes #751

feat: endpoints2: aws middleware and integration changes #751

Conversation

lucix-aws commented Nov 12, 2022

Description of changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucix-aws Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

aajtodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 18, 2022

lucix-aws Nov 15, 2022 •

edited

Loading