Skip to content

routing/cluster changes: move cluster selection to Envoy's routing table#2087

Closed
junr03 wants to merge 7 commits intomainfrom
routing-changes
Closed

routing/cluster changes: move cluster selection to Envoy's routing table#2087
junr03 wants to merge 7 commits intomainfrom
routing-changes

Conversation

@junr03
Copy link
Copy Markdown
Member

@junr03 junr03 commented Mar 7, 2022

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

junr03 and others added 4 commits March 6, 2022 18:40
Signed-off-by: José Ulises Niño Rivera <junr03@users.noreply.github.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
socket_address: { address: *statsd_host, port_value: *statsd_port }

!ignore protocol_defs: &http1_protocol_options_defs
!ignore http1_protocol_options_defs: &http1_protocol_options
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@goaway how come this is in the header but base_protocol_options_defs is in the config_template?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The header defines defaults. Overrides are injected between the header and the template, so any objects that contain internal values that you want to be overridable must be defined in the template.

Signed-off-by: Jose Nino <jnino@lyft.com>
namespace {

const LowerCaseString ClusterHeader{"x-envoy-mobile-cluster"};
const LowerCaseString H2UpstreamHeader{"x-envoy-mobile-upstream-protocol"};
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@goaway on Friday we discussed having:

  1. :scheme dictate if the request is routed to non-tls http1.1
  2. route specifically to h2 if the hostname is included.
  3. route everything else to alpn.

Looking at the code that uses x-envoy-mobile-upstream-protocol it seems that we have the gRPC platform bindings force routing to h2. I can preserve that functionality. But do we want to allow for any request to force routing via x-envoy-mobile-upstream-protocol?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think I might remove the protocol-based mapping (since it's incomplete anyways) and just leave in the cluster override header and use that for the gRPC case.

Signed-off-by: Jose Nino <jnino@lyft.com>
}

BOOL hasH2Hostnames = self.h2Hostnames.count > 0;
if (hasH2Hostnames) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TODO: actually build this

Signed-off-by: Jose Nino <jnino@lyft.com>
@junr03
Copy link
Copy Markdown
Member Author

junr03 commented Mar 7, 2022

@goaway Alternatively, we could start with a PR that leaves all the x-envoy-mobile-cluster and x-envoy-mobile-upstream-protocol logic alone, and only adds the forced h2 cluster routing.

@junr03
Copy link
Copy Markdown
Member Author

junr03 commented Mar 14, 2022

Closed in favor of #2088

@junr03 junr03 closed this Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants