-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Changes to enable Smithy S3 CRT #3352
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
bb3526d to
2ff6095
Compare
| } | ||
|
|
||
|
|
||
| AwsSmithyClientBase::ResolveEndpointOutcome AwsSmithyClientBase::ResolveIdentityAuth( |
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 refactor of existing code into a helper API for reuse
| auto span = tracer->CreateSpan(Aws::String(this->GetServiceClientName()) + "." + request.GetServiceRequestName(), | ||
| {{ TracingUtils::SMITHY_METHOD_DIMENSION, request.GetServiceRequestName() }, { TracingUtils::SMITHY_SERVICE_DIMENSION, this->GetServiceClientName() }, { TracingUtils::SMITHY_SYSTEM_DIMENSION, TracingUtils::SMITHY_METHOD_AWS_VALUE }}, | ||
| smithy::components::tracing::SpanKind::CLIENT); | ||
| #if(($serviceModel.metadata.namespace == "S3" || ($serviceModel.metadata.namespace == "S3Crt" && !$operation.s3CrtEnabled)) && $operation.shouldUsePropertyBag) |
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.
Move this block so that service params set before account id based identity resolution happens. Without this, s3 express wont work
|
|
||
| AwsSmithyClientAsyncRequestContext() = default; | ||
|
|
||
| AwsSmithyClientAsyncRequestContext( |
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 defaults initialization of few members which was previously being done in request processing
ac3bbb4 to
afa1937
Compare
| private: | ||
| void UpdateAuthSchemeFromEndpoint(const Aws::Endpoint::AWSEndpoint& endpoint, AuthSchemeOption& authscheme) const; | ||
|
|
||
| bool ResolveIdentityAuth( |
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.
- pExecutor and responseHandler assumes that the function is async, why does it also return a simple boolean? How does it going to fit into the existing async design?
- Why do we need 2 method of
ResolveIdentityAuth?
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 a Private helper method , the reason it returns bool is simply for the caller to know if any error occurred. The actual type specific outcome is handled in responseHandler. The return type is bool because this function throws different types of outcome errors which will need a variant and complicate the code much more in the calling function. The equivalent is achieved by choosing a simple return type but still processing and returning correct outcome type through the responseHandler.
- ResolveIdentityAuth is a private method that just only houses the code for identity and auth resolution. It is simply reused in preexisting 'AwsSmithyClientBase::MakeRequestAsync'. The reason this part of code is moved to a helper function is because in S3-CRT operations like put/getObjectAsync, we only resolve the endpoint in sdk and for rest of the request handling happens in crt.
| } | ||
|
|
||
| Aws::Endpoint::EndpointParameters epParams = request ? request->GetEndpointContextParams() : Aws::Endpoint::EndpointParameters(); | ||
| Aws::Endpoint::EndpointParameters epParams = pRequestCtx->m_pRequest ? pRequestCtx->m_pRequest->GetEndpointContextParams() : Aws::Endpoint::EndpointParameters(); |
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.
Why ResolveIdentity also resolves an endpoint?
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 helper function just houses the code to select auth scheme, resolve identity and resolve endpoint.
In pass case, the crt methods need the endpointOutcome.
For example, from s3-crtclient.cpp, for one of the async apis, the crt initialization takes endpoint, which is simply obtained in the smithy auth call flow aforementioned.
auto endpointResolutionOutcome = TracingUtils::MakeCallWithTiming<ResolveEndpointOutcome>(
[&]() -> ResolveEndpointOutcome {
return AwsSmithyClientT::ResolveIdentityAuth(
&request,
request.GetServiceRequestName(),
std::move(endpointCallback));
},
TracingUtils::SMITHY_CLIENT_ENDPOINT_RESOLUTION_METRIC,
*meter,
{{TracingUtils::SMITHY_METHOD_DIMENSION, request.GetServiceRequestName()}, {TracingUtils::SMITHY_SERVICE_DIMENSION, this->GetServiceClientName()}});
// make aws_s3_meta_request with callbacks
CrtRequestCallbackUserData *userData = Aws::New<CrtRequestCallbackUserData>(ALLOCATION_TAG);
aws_s3_meta_request_options options;
AWS_ZERO_STRUCT(options);
aws_uri endpoint;
InitCrtEndpointFromUri(endpoint, endpointResolutionOutcome.GetResult().GetURI());
| { | ||
| responseHandler(std::move(outcome)); | ||
| } ); | ||
| return 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.
Why ResolveIdentity also validates DNS host name?
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.
Since the helper function just houses the code to select auth, resolve identity and then resolve endpoint, this validation code is the existing check we have to ensure endpoint resolved is valid or not. In case it is not valid, it reflects that in the outcome.
| std::move(responseHandler), | ||
| std::move(endpointCallback), | ||
| pExecutor)) | ||
| { |
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.
Async in MakeRequestAsync stands for async execution of this method, the handler is not being called, the caller will wait forever for a 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.
I am not sure which handler is being referred to, but 'responseHandler' is being called. This is tested with S3-crt and S3-crt express integration and unit tests
| request.SetServiceSpecificParameters( | ||
| [&]() -> std::shared_ptr<Http::ServiceSpecificParameters> { | ||
| Aws::Map<Aws::String, Aws::String> params; | ||
| params.emplace("bucketName", request.GetBucket()); | ||
| ServiceSpecificParameters serviceSpecificParameters{params}; | ||
| return Aws::MakeShared<ServiceSpecificParameters>(ALLOCATION_TAG, serviceSpecificParameters); | ||
| }()); | ||
| EndpointUpdateCallback endpointCallback = [&](Aws::Endpoint::AWSEndpoint& resolvedEndpoint){ |
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.
Please provide a design doc / explanation why this change is needed.
The function below ResolveIdentityAuth has a very questionable design (creates a new executor, then executes a function in a sync blocking mode and then returns).
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 callback is used in this smithy client the same way for other clients. It is simply used in existing resolveEndpoint function.
EndpointUpdateCallback endpointCallback = [&](Aws::Endpoint::AWSEndpoint& resolvedEndpoint){
resolvedEndpoint.AddPathSegments(request.GetKey());
};
auto meter = m_clientConfiguration.telemetryProvider->getMeter(this->GetServiceClientName(), {});
auto endpointResolutionOutcome = TracingUtils::MakeCallWithTiming<ResolveEndpointOutcome>(
[&]() -> ResolveEndpointOutcome {
return AwsSmithyClientT::ResolveIdentityAuth(
&request,
request.GetServiceRequestName(),
std::move(endpointCallback));
},
It is used in client base as:
auto epResolutionOutcome = this->ResolveEndpoint(std::move(epParams), std::move(endpointCallback));
0165e2e to
ef76dd0
Compare
Issue #, if available:
Description of changes:
The changes are pre-requisites to enable smithy s3 crt client. This covers:
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.