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.

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

use downstream protocol on inbound clusters #16452

Closed
wants to merge 1 commit into from

Conversation

rshriram
Copy link
Member

Use HTTP1.1 or HTTP2, depending on which one is used on the downstream connection.

Signed-off-by: Shriram Rajagopalan rshriram@tetrate.io

Signed-off-by: Shriram Rajagopalan <rshriram@tetrate.io>
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Aug 21, 2019
@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 21, 2019
@istio-testing
Copy link
Collaborator

@rshriram: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
istio-pilot-multicluster-e2e-master f44e6bd link /test istio-pilot-multicluster-e2e-master

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@rshriram
Copy link
Member Author

Might be of some use to tackle #16391

@@ -601,6 +601,9 @@ func (configgen *ConfigGeneratorImpl) buildInboundClusterForPortOrUDS(pluginPara
localCluster := buildDefaultCluster(pluginParams.Env, clusterName, apiv2.Cluster_STATIC, localityLbEndpoints,
model.TrafficDirectionInbound, pluginParams.Node, nil)
setUpstreamProtocol(localCluster, instance.Endpoint.ServicePort)
// Set the upstream protocol selection based on the downstream protocol. This is for inbound clusters only.
// Use HTTP1.1 or HTTP2, depending on which one is used on the downstream connection.
localCluster.ProtocolSelection = apiv2.Cluster_USE_DOWNSTREAM_PROTOCOL
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens in this scenario ?

clientProxy - ( h2 upgrade ) ---->  serverProxy --(inbound cluster) --->  (http 1.1) application```
It seems that serverProxy will forward the h2 connection to the application which cannot speak h2. we need to guard for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Envoy does not support h2 upgrades in inbound or in cluster. So we don't have to worry about that problem. Secondly, guarding for this specific condition means we break all the protocol sniffing work where the application does not declare the port type. I think the latter is a much bigger win compared to the former. The former will be solved when envoy gains proper h2 upgrade facility.

@stale
Copy link

stale bot commented Sep 5, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale label Sep 5, 2019
@rshriram rshriram closed this Sep 13, 2019
@rshriram rshriram deleted the inbound_h1h2 branch December 6, 2019 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants