-
Notifications
You must be signed in to change notification settings - Fork 55
Bootstrap event streams #537
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
Conversation
| // FIXME - should we just move these into core and get rid of aws-types at this point? | ||
| api(project(":aws-runtime:aws-types")) |
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.
Comment: Yes.
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.
I'll do it either in one of the follow on PRs or separately after event streams is complete
| /** | ||
| * The [CredentialsProvider] to complete the signing process with. Defaults to the provider configured | ||
| * on the service client. | ||
| * NOTE: This is not a common option. | ||
| */ | ||
| public val CredentialsProvider: ClientOption<CredentialsProvider> = ClientOption("CredentialsProvider") |
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.
Question: These attributes are intended to be used by end-users? If so, can we offer better guidance than "This is not a common option"? Maybe something like "Changing this option is only required in advanced use cases".
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.
Sure we can change it. I'm not convinced all of these are intended for use by end-users but I also don't see a reason why they couldn't be. We need to spend some time looking at per/operation config and interceptors. Originally the execution context was envisioned to be something that end users may influence directly but currently they have no way of accessing it.
| } | ||
| } | ||
|
|
||
| val signedRequest = AwsSigner.signRequest(signableRequest, opSigningConfig.toCrt()) |
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.
Question: Does AwsSigner.signRequest still need to exist?
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.
Are you asking should it still exist in aws-crt-kotlin?
Probably not if the sign() function returning AwsSigningResult supersedes it.
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.
Yeah, asking about the implementation in aws-crt-kotlin specifically. I didn't see any remaining usage after this removal.
| public fun Flow<Message>.sign( | ||
| context: ExecutionContext, | ||
| config: AwsSigningConfig, | ||
| ): Flow<Message> = flow { | ||
| val messages = this@sign | ||
|
|
||
| // NOTE: We need the signature of the initial HTTP request to seed the event stream signatures | ||
| // This is a bit of a chicken and egg problem since the event stream is constructed before the request | ||
| // is signed. The body of the stream shouldn't start being consumed though until after the entire request | ||
| // is built. Thus, by the time we get here the signature will exist in the context. | ||
| var prevSignature = context.getOrNull(AuthAttributes.RequestSignature) ?: error("expected initial HTTP signature to be set before message signing commences") | ||
|
|
||
| // signature date is updated per event message | ||
| val configBuilder = config.toBuilder() | ||
|
|
||
| messages.collect { message -> | ||
| // FIXME - can we get an estimate here on size? | ||
| val buffer = SdkByteBuffer(0U) | ||
| message.encode(buffer) | ||
|
|
||
| // the entire message is wrapped as the payload of the signed message | ||
| val result = signPayload(configBuilder, prevSignature, buffer.bytes()) | ||
| prevSignature = result.signature | ||
| emit(result.output) | ||
| } | ||
| } |
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.
Question: Am I reading correctly that this will collect every message (i.e., the entire event stream) and then emit a new stream with signed messages? Isn't that wasteful and unnecessarily greedy? Could/should we map instead?
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.
This is inside of a flow collector so collection is expected and how many of the provided transforms are implemented internally.
Note the = flow { on line 31. We are creating a new flow that processes elements from the current one. Collection will not happen until the returned flow is collected
| useDoubleUriEncode = false | ||
| normalizeUriPath = true |
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.
Question: We purposely ignore these values in AwsSigningConfig? Do any services (e.g., S3) need special values here?
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.
We purposely ignore these values in AwsSigningConfig?
I don't follow...
Do any services (e.g., S3) need special values here?
Unknown. There is still some work left before I can send an input stream. I've validated most of the message serialization locally with some integration tests (that I will add to future PR after I clean them up). Signing is an area that may change in a future PR once I understand what we need to do better.
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.
We purposely ignore these values in AwsSigningConfig?
I don't follow...
Sorry, disregard. I misread this code.
| val job = scope.launch { | ||
| encodedMessages.collect { | ||
| ch.writeFully(it) | ||
| } | ||
| } | ||
|
|
||
| job.invokeOnCompletion { cause -> | ||
| ch.close(cause) | ||
| } | ||
|
|
||
| return ch.toHttpBody() |
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.
Question: Why is a Job necessary here? If invokeOnCompletion waits for the job to complete anyway then it seems like this could be replaced with simply invoking collect directly in this method.
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.
We don't want to collect directly in this method. We are processing a stream and it needs to be done asynchronously since it is of unknown size. It also needs to happen after we have sent the initial request (which now that I look at the generated code may need tweaked a bit, will find out when I get it actually working on a real service).
We launch a coroutine to do this (which returns Job). invokeOnCompletion does not wait for the Job to complete, it simply wires up a completion handler that will fire when the job transitions to the Complete state (which will happen for either success or failed/cancelled jobs).
codegen/sdk/build.gradle.kts
Outdated
| // transcribe streaming contains exclusively EventStream operations which are not supported | ||
| "transcribestreaming", | ||
| // "transcribestreaming", |
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.
Question: Can be removed?
| object AwsEventStream { | ||
| val decodeFrames = runtimeSymbol("decodeFrames", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| val Message = runtimeSymbol("Message", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| val HeaderValue = runtimeSymbol("HeaderValue", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| val buildMessage = runtimeSymbol("buildMessage", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| val MessageType = runtimeSymbol("MessageType", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| val MessageTypeExt = runtimeSymbol("type", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
|
|
||
| val sign = runtimeSymbol("sign", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| val newEventStreamSigningConfig = runtimeSymbol("newEventStreamSigningConfig", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| val encode = runtimeSymbol("encode", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| val asEventStreamHttpBody = runtimeSymbol("asEventStreamHttpBody", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
|
|
||
| val expectBool = runtimeSymbol("expectBool", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| val expectByte = runtimeSymbol("expectByte", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| val expectInt16 = runtimeSymbol("expectInt16", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| val expectInt32 = runtimeSymbol("expectInt32", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| val expectInt64 = runtimeSymbol("expectInt64", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| val expectByteArray = runtimeSymbol("expectByteArray", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| val expectString = runtimeSymbol("expectString", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| val expectTimestamp = runtimeSymbol("expectTimestamp", AwsKotlinDependency.AWS_EVENT_STREAM) | ||
| } |
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.
Style: I suggest we sort these lexicographically.
| val streamingMember = ioShape.findStreamingMember(model) | ||
| streamingMember?.isUnionShape ?: false | ||
| val target = streamingMember?.let { model.expectShape(it.target) } | ||
| target?.isUnionShape ?: false |
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.
Question: Why do we still need this integration?
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.
We may or may not depending on what we do with RPC based event streams. I need to look into how much work it will be to support them (IIRC Kinesis and QLDB are the only ones that use this whereas transcribe streaming and S3 use the HTTP request/response for the initial request/response of the event stream)
| writer.write("val messages = stream.#T {", RuntimeTypes.KotlinxCoroutines.Flow.map) | ||
| .indent() | ||
| .indent() | ||
| .write("#T(it)", encodeFn) | ||
| .dedent() | ||
| .write("}") | ||
| .write(".#T(context, signingConfig)", AwsRuntimeTypes.AwsEventStream.sign) | ||
| .write(".#T()", AwsRuntimeTypes.AwsEventStream.encode) | ||
| .dedent() |
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.
Style: This indentation is non-standard and the encoding function doesn't need a full lambda body. I suggest:
writer.withBlock("val messages = stream", "") {
write(".#T(::#T)", RuntimeTypes.KotlinxCoroutines.Flow.map, encodeFn)
write(".#T(context, signingConfig)", AwsRuntimeTypes.AwsEventStream.sign)
write(".#T()", AwsRuntimeTypes.AwsEventStream.encode)
}
Yielding:
val messages = stream
.map(::encodeFooEventMessage)
.sign(context, signingConfig)
.encode()|
Kudos, SonarCloud Quality Gate passed!
|
* fix event stream filtering * add parsing of common headers * refractor frame decoder into a Flow * remove event stream operation filter from customizations * refactore event stream parsing; implement rough deserialization codegen * fix warning * filter out event stream errors * render deserialization for exception event stream messages * inject http request signature into the execution context once known * add support for chunked signing * add encode transform for message stream * inline signing config builder * initial event stream serialize implementation * fix compile issues * disable wip integration tests * suppress test; cleanup codegen
* Bootstrap event streams (#537) * fix event stream filtering * add parsing of common headers * refractor frame decoder into a Flow * remove event stream operation filter from customizations * refactore event stream parsing; implement rough deserialization codegen * fix warning * filter out event stream errors * render deserialization for exception event stream messages * inject http request signature into the execution context once known * add support for chunked signing * add encode transform for message stream * inline signing config builder * initial event stream serialize implementation * fix compile issues * disable wip integration tests * suppress test; cleanup codegen * Event Stream Codegen Tests (#542) * Checkpoint Event Streams (#544) * fix tests * increase windows runner memory








Issue #
Description of changes
WIP branch for implementing event streams.
This PR mostly covers generating serialization and deserialization for the
vnd.amazon.event-streamcontent type.Flowdirectly to simplify the generated codeExample Codegen
Example for transcribe streaming
StartStreamTranscriptionoperation (which has both an input and output stream)Serialize:
Deserialize:
Current state
I've been able to test deserialization (output event streams) for S3 select object and it works fine. Serialization has been tested as part of some local integration testing but the event stream signing needs a bit more work before I can send a real request with an input event stream (e.g. transcribe streaming).
Left to do (follow on PRs)
I will probably leave RPC event streams as a follow up to this work. Basically an event stream can send/receive an initial request/response. That initial request/response can be tied to the HTTP request OR be sent as the first message on the event stream. The former is what we have now and how S3 and transcribe streaming work. The latter will require a bit more investigation and abstraction to figure out how to implement them. To my knowledge QLDB and kinesis leverage RPC event streams
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.