-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
http_11_proxy: Make inner transport_socket config optional #36414
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Tony Allen <[email protected]>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
@@ -35,5 +35,5 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; | |||
// | |||
message Http11ProxyUpstreamTransport { | |||
// The underlying transport socket being wrapped. | |||
config.core.v3.TransportSocket transport_socket = 1 [(validate.rules).message = {required: true}]; | |||
config.core.v3.TransportSocket transport_socket = 1; |
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.
can you say more about why the underlying socket is no longer required?
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 thought the PR description covered the reasoning. If it's unclear, can you give me a pointer on what details you're looking for?
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.
So sorry, I missed the PR description. That explains it, thanks!
Also, tests seem to be failing /wait |
The failure isn't related to this PR:
|
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.
Thanks for doing this!
Does the Envoy implementation need to be changed to default to plaintext if this field is unset?
api/envoy/extensions/transport_sockets/http_11_proxy/v3/upstream_http_11_connect.proto
Outdated
Show resolved
Hide resolved
@markdroth no change was required to the implementation. |
Signed-off-by: Tony Allen <[email protected]>
Signed-off-by: Tony Allen <[email protected]>
/lgtm api Thanks! |
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.
/lgtm api
@@ -35,5 +35,5 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; | |||
// | |||
message Http11ProxyUpstreamTransport { | |||
// The underlying transport socket being wrapped. | |||
config.core.v3.TransportSocket transport_socket = 1 [(validate.rules).message = {required: true}]; | |||
config.core.v3.TransportSocket transport_socket = 1; |
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.
So sorry, I missed the PR description. That explains it, thanks!
Please fix the build:
|
Signed-off-by: Tony Allen <[email protected]>
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.
/lgtm api
/retest |
http_11_proxy: Make inner transport_socket config optional
Given that the top-level Cluster.transport_socket field is optional and defaults to plaintext, this should also be optional. gRPC is adding support for this transport socket, but they do not have a
raw_buffer
to explicitly configure. See grpc/proposal#455 (comment) for additional context.Risk Level: Low.
Testing: Existing tests.
Docs Changes: n/a
Release Notes: Done.