ASF: implement multi-protocol support #13151
Conversation
bentsku
added
area: asf
semver: minor
bentsku
force-pushed
the
rpcv2-cbor-parser-serializer
branch
from
aef8ffa to
646b04c
Compare
bentsku
force-pushed
the
asf-multi-protocol
branch
2 times, most recently
from
6db9a1d to
f803320
Compare
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 7m 56s ⏱️ Results for commit 61416ea. ♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 36m 52s ⏱️ Results for commit 61416ea. ♻️ This comment has been updated with latest results. |
bentsku
requested review from
alexrashed,
baermat,
gregfurman and
thrau
as code owners
| raise ProtocolError( | ||
| f"Could not determine the protocol for the request: " | ||
| f"{request.method} {request.path} for the service '{service_model.service_name}' " | ||
| f"(available protocols: {protocols}" |
There was a problem hiding this comment.
nit: missing closing parenthesis, will fix before merging
| f"(available protocols: {protocols}" | |
| f"(available protocols: {protocols})" |
alexrashed
left a comment
alexrashed
left a comment
There was a problem hiding this comment.
Awesome! Really nice to see this final step be actually pretty smooth and clean after the nice preparations with the previous PRs! 💯
This is a huge step towards native multi-protocol support for basically all services out there based on a single service spec file! Really looking forward to #13151! 🤩
| # TODO: even though our Service Name Parser will only use a protocol that is available for the service, we might | ||
| # want to verify if the given protocol here is available for that service, in case we are manually calling | ||
| # this factory. Revisit once we implement multi-protocol support | ||
| service_protocol = protocol or service.protocol |
There was a problem hiding this comment.
question: Coming back to this comment in a previous PR: Would it make sense here to fail hard here if the protocol is not in the list of protocols for the service? Should we be more permissive or more strict here? 🤔
There was a problem hiding this comment.
So I initially decided against that, because this check is done in determine_aws_protocol and the only place we really manually use the serializer and parser are in our unit tests.
If a new code path needs to use the serializer/parser manually, it will need to use the protocol from the RequestContext, or hardcode to a specific protocol (like sqs-query Query API routes)
So I'm not sure adding an additional check is worth it? As it will be executed in every request (when not cached)
But if you have other cases I haven't though about, happy to hear and update 👍
| # TODO: we should come up with a better way to create a protocol-aware client | ||
| protocol_serializer = create_serializer(protocol_name=protocol, include_validation=False) | ||
| monkeypatch.setattr(client, "_serializer", protocol_serializer) | ||
| return client |
There was a problem hiding this comment.
praise: While this is a bit hacky, this is really creating an incredibly thorough test! 🤩
| :return: the protocol matched, if any | ||
| """ | ||
| for protocol in _PROTOCOL_DETECTION_PRIORITY: | ||
| if protocol in available_protocols and _matches_protocol(request, protocol): |
There was a problem hiding this comment.
question: Is it necessary to check if the request matches the protocol if there is only a single protocol supported by a service (which is the absolute vast majority of services)?
There was a problem hiding this comment.
This logic is done in determine_aws_protocol:
if not (protocols := service_model.metadata.get("protocols")):
# if the service does not define multiple protocols, return the `protocol` defined for the service
return service_model.protocol
if len(protocols) == 1:
return protocols[0]
if protocol := get_protocol_from_request(request, available_protocols=protocols):
return protocolWe only call this get_protocol_from_request, but I think the naming is wrong which is why this question came up 😄
maybe match_protocol or something like that? What would be a good word for picking a protocol out of a list 🤔
bentsku
force-pushed
the
asf-multi-protocol
branch
from
51871cc to
61416ea
Compare
alexrashed
added
the
notes: skip
Motivation
This PR is part of a bigger initiative (see #13028) to implement support for the new Smithy Protocol smithy-rpc-v2-cbor, as well as the new Botocore multi-protocol support for single services (before, a service could only define one protocol).
This PR implements a new protocol detection for services who support multiple protocols. It sets the
protocolas a first class citizen in our handler chain, allowing us to use the detected protocol to create our parser and serializer.The protocol detection is done by following the different specs as defined at https://smithy.io/2.0/aws/protocols/ and https://smithy.io/2.0/additional-specs/protocols
We have extensive testing of the protocol determination by upgrading our current automated service detection tests, by running them for all protocols a service supports and verifying that the protocol is the one we selected when serializing the request.
This also implements the retrieval of Service Indicators when trying to match a service based on the request, to find indicator if the request is done via
smithy-rpc-v2-cbor, as it defines a special way to encode its service and operation in the path.We also implement a priority detection on the protocol, as some services are pretty greedy (
rest-based services).One important change is of the signature of
ServiceModelIdentifier, as we now need to know allprotocolsa service support for thequeryandec2service detection (we need it to match it withservices.by_operation(values["Action"]))You can see how it is used for CloudWatch with the follow-up PR: #13161
Changes
protocolpart of theRequestContextprotocolstoServiceModelIdentifierinaws/spec.pytest_service_router_works_for_every_serviceto also verify protocol