Skip to content
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.

Sign up for GitHub

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

Jump to bottom

http_11_proxy: Make inner transport_socket config optional #36414

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

http_11_proxy: Make inner transport_socket config optional #36414

wants to merge 4 commits into from
+5 −3

Conversation

tonya11en
Copy link
Member

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.

@tonya11en
http_11_proxy: Make inner transport_socket config optional
6e98909 
Signed-off-by: Tony Allen <txallen@google.com>
@repokitteh-read-only RepoKitteh - Read Only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #36414 was opened by tonya11en.

see: more, trace.

abeyad
abeyad reviewed Oct 2, 2024
View reviewed changes
api/envoy/extensions/transport_sockets/http_11_proxy/v3/upstream_http_11_connect.proto
@@ -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;
Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Contributor

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!

@abeyad
Copy link
Contributor

abeyad commented Oct 2, 2024

Also, tests seem to be failing

/wait

@tonya11en
Copy link
Member Author

Also, tests seem to be failing

/wait

The failure isn't related to this PR:

================================================================================
INFO: Elapsed time: 2312.606s, Critical Path: 1217.31s
INFO: 26910 processes: 11952 remote cache hit, 11870 internal, 1 local, 2 processwrapper-sandbox, 3085 remote.
//test/integration:load_stats_integration_test                           FAILED in 377.3s
  /build/bazel_root/base/execroot/envoy/bazel-out/k8-dbg/testlogs/test/integration/load_stats_integration_test/test.log

markdroth
markdroth reviewed Oct 2, 2024
View reviewed changes
Copy link
Contributor

@markdroth markdroth left a 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
@tonya11en
Copy link
Member Author

Thanks for doing this!

Does the Envoy implementation need to be changed to default to plaintext if this field is unset?

@markdroth no change was required to the implementation.

@tonya11en
Merge remote-tracking branch 'upstream/main' into optional_ts_h11p
0971217 
Signed-off-by: Tony Allen <txallen@google.com>
@tonya11en
more
dc8d5f5 
Signed-off-by: Tony Allen <txallen@google.com>
@markdroth
Copy link
Contributor

/lgtm api

Thanks!

@repokitteh-read-only repokitteh-read-only bot removed the api label Oct 2, 2024
@markdroth markdroth mentioned this pull request Oct 2, 2024
Open
abeyad
abeyad previously approved these changes Oct 3, 2024
View reviewed changes
Copy link
Contributor

@abeyad abeyad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm api

api/envoy/extensions/transport_sockets/http_11_proxy/v3/upstream_http_11_connect.proto
@@ -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;
Copy link
Contributor

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!

@kyessenov
Copy link
Contributor

Please fix the build:

envoy/extensions/transport_sockets/http_11_proxy/v3/upstream_http_11_connect.proto:8:1:Import "validate/validate.proto" is unused.```

@tonya11en
woops
2d09673 
Signed-off-by: Tony Allen <txallen@google.com>
markdroth
markdroth reviewed Oct 3, 2024
View reviewed changes
Copy link
Contributor

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm api

@repokitteh-read-only repokitteh-read-only bot removed the api label Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markdroth markdroth markdroth left review comments

@abeyad abeyad abeyad left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@abeyad abeyad

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tonya11en @abeyad @markdroth @kyessenov