-
Notifications
You must be signed in to change notification settings - Fork 35
feat: Automatic clock skew adjustment #968
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
import software.amazon.smithy.swift.codegen.SwiftWriter | ||
import software.amazon.smithy.swift.codegen.integration.ProtocolGenerator | ||
import software.amazon.smithy.swift.codegen.middleware.MiddlewareRenderable | ||
|
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 type renders the clock skew provider into the orchestrator.
) | ||
operationMiddleware.appendMiddleware(operation, DeserializeMiddleware(ctx.model, ctx.symbolProvider)) | ||
operationMiddleware.appendMiddleware(operation, LoggingMiddleware(ctx.model, ctx.symbolProvider)) | ||
operationMiddleware.appendMiddleware(operation, ClockSkewMiddleware(ctx.model, ctx.symbolProvider, clockSkewProviderSymbol)) |
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.
Creates the type directly above to render the proper middleware.
|
||
val clockSkewProviderSymbol: Symbol | ||
get() = DefaultClockSkewProviderSymbol | ||
|
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.
Here & above is the symbol for the clock skew provider.
In aws-sdk-swift, this property is overridden to provide an AWS-specific implementation.
import struct Foundation.Date | ||
import struct Foundation.TimeInterval | ||
|
||
public typealias ClockSkewProvider<Request, Response> = @Sendable (Request, Response, Error, Date) -> TimeInterval? |
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.
Just a typealias for the closure type, since it's pretty long.
// SPDX-License-Identifier: Apache-2.0 | ||
// | ||
|
||
import struct Foundation.TimeInterval |
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 type is just a concurrency-safe store for clock skew values, keyed by the hostname of the server where that clock skew was measured.
signedBodyValue: signedBodyValue, | ||
flags: flags, | ||
date: Date(), | ||
date: Date().addingTimeInterval(clockSkew), |
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.
Clock skew is applied to current date in the signing config to adjust signature expiration for the difference in client vs. server clock time.
// | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// | ||
import AwsCommonRuntimeKit |
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.
The extension below is unused so it & the CRT dependency are removed.
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.
RetryErrorType+CRT below is entirely unneeded and is removed.
} else { | ||
throw TestHTTPError(statusCode: response.statusCode) | ||
} | ||
}) |
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.
The deserializer above and the switch statement below were corrected to ensure that a HTTP response is available during retries.
case .httpError(let statusCode): | ||
throw TestHTTPError(statusCode: statusCode) | ||
let httpStatusCode = HTTPStatusCode(rawValue: statusCode)! | ||
return HTTPResponse(statusCode: httpStatusCode) |
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.
Rather than throw here, successfully return an HTTP response that indicates a non-success error code.
Instead, the service error will get thrown in the deserializer above (this is how "real" clients work.)
"SmithyRetriesAPI", | ||
.product(name: "AwsCommonRuntimeKit", package: "aws-crt-swift"), | ||
] | ||
dependencies: ["SmithyRetriesAPI"] |
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.
Removed the CRT dependency from SmithyRetries since the only CRT references in that target were unused and are now removed.
internal var deserialize: ((ResponseType, Context) async throws -> OutputType)? | ||
internal var retryStrategy: (any RetryStrategy)? | ||
internal var retryErrorInfoProvider: ((Error) -> RetryErrorInfo?)? | ||
internal var clockSkewProvider: (ClockSkewProvider<RequestType, ResponseType>)? |
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.
The OrchestratorBuilder
is modified to accept a clock skew provider. This is set in codegen when the orchestrator is constructed for an operation.
|
||
/// Message that is sent from client to service. | ||
public protocol RequestMessage { | ||
public protocol RequestMessage: Sendable { |
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.
Sendable
is added to request & response types so they can be safely passed through async contexts.
(HTTPRequest
and HTTPResponse
are both already Sendable
so no implementation change needed for this.)
return Endpoint(uri: self.destination, headers: self.headers) | ||
} | ||
public var endpoint: Endpoint { .init(uri: destination, headers: headers) } | ||
public internal(set) var signedAt: Date? |
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.
signedAt
property is added to HTTPRequest
. It is nil
until the request is signed, then it is set with the Date
that was used as "now" for signing.
public private(set) var port: UInt16? | ||
public private(set) var protocolType: URIScheme = .https | ||
public private(set) var trailingHeaders: Headers = Headers() | ||
public private(set) var signedAt: Date? |
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.
Here, signedAt
is added to the builder for HTTPRequest
.
} | ||
|
||
let signingConfig = try constructSigningConfig(identity: identity, signingProperties: signingProperties) | ||
let signedAt = Date() |
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.
The signer gets current Date
and keeps it in a var, so the same date can be used for signing and for updating the HTTPRequest.signedAt
property.
signedBodyValue: signedBodyValue, | ||
flags: flags, | ||
date: Date(), | ||
date: signedAt.addingTimeInterval(clockSkew), |
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.
Correct the signedAt
value with clock skew, then pass it into the signer.
from crtRequest: HTTPRequestBase, | ||
originalRequest: HTTPRequest, | ||
signedAt: Date | ||
) -> HTTPRequestBuilder { |
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.
Here, the update()
function on HTTPRequestBuilder
, which is used to apply a signature to an unsigned HTTPRequest
, is modified to also set the updated request's signedAt
property.
crtRequest.path = pathToMatch | ||
|
||
let updatedRequest = HTTPRequestBuilder().update(from: crtRequest, originalRequest: originalRequest).build() | ||
let updatedRequest = HTTPRequestBuilder().update(from: crtRequest, originalRequest: originalRequest, signedAt: Date()).build() |
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.
A signedAt
value had to be passed to allow compile. It is not used in this test.
func computeNextBackoffDelay(attempt: Int) -> TimeInterval { 0.0 } | ||
} | ||
|
||
func test_clockSkew_retriesWithClockSkewApplied() async throws { |
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 an orchestrator test that verifies:
- clock skew is obtained from the provider & stored
- retry is performed when clock skew changes
- retry is not performed when the error isn't retriable & clock skew didn't change
- retry eventually succeeds if max attempts wasn't exceeded
XCTAssertNil(endpoint.url, "An invalid endpoint should result in a nil URL.") | ||
} | ||
|
||
func test_requestBuilder_SignedAt() { |
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.
Simple test to verify builder sets signedAt
field in built request
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.
Nit on a comment, everything lgtm
/// - response: The response that was returned from the server. (Typically this is a `HTTPResponse`) | ||
/// - error: The error that was returned by the server; typically this is a `ServiceError` with an error code that | ||
/// indicates clock skew is or might be the cause of the failed request. | ||
/// - previous: The previously clock skew value, or `nil` if non was recorded. |
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.
nit: The previously measured ...
Description of changes
Automatically adjusts for differences between client and server wall clock time, to prevent signed requests from being rejected due to expired signature.
The basic smithy-swift implementation has a customization point for clock skew determination, but never detects a clock skew by default. The actual clock-skew determination to be used on AWS services is implemented in the companion aws-sdk-swift PR.
SignerMiddleware
andSigV4Signer
are modified to use clock skew when one is set.Also:
SmithyRetries
dependency on CRTScope
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.