-
Notifications
You must be signed in to change notification settings - Fork 85
routing/cluster changes: move cluster selection to Envoy's routing table #2087
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
Changes from 6 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -414,7 +414,6 @@ void Client::sendHeaders(envoy_stream_t stream, envoy_headers headers, bool end_ | |
| if (direct_stream) { | ||
| ScopeTrackerScopeState scope(direct_stream.get(), scopeTracker()); | ||
| RequestHeaderMapPtr internal_headers = Utility::toRequestHeaders(headers); | ||
| setDestinationCluster(*internal_headers); | ||
| // Set the x-forwarded-proto header to https because Envoy Mobile only has clusters with TLS | ||
| // enabled. This is done here because the ApiListener's synthetic connection would make the | ||
| // Http::ConnectionManager set the scheme to http otherwise. In the future we might want to | ||
|
|
@@ -585,53 +584,8 @@ void Client::removeStream(envoy_stream_t stream_handle) { | |
| } | ||
|
|
||
| namespace { | ||
|
|
||
| const LowerCaseString ClusterHeader{"x-envoy-mobile-cluster"}; | ||
| const LowerCaseString H2UpstreamHeader{"x-envoy-mobile-upstream-protocol"}; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @goaway on Friday we discussed having:
Looking at the code that uses
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
|
||
| // Alternate clusters included here are a stopgap to make it less likely for a given connection | ||
| // class to suffer "catastrophic" failure of all outbound requests due to a network blip, by | ||
| // distributing requests across a minimum of two potential connections per connection class. | ||
| // Long-term we will be working to generally provide more responsive connection handling within | ||
| // Envoy itself. | ||
|
|
||
| const char* BaseCluster = "base"; | ||
| const char* H2Cluster = "base_h2"; | ||
| const char* ClearTextCluster = "base_clear"; | ||
| const char* AlpnCluster = "base_alpn"; | ||
|
|
||
| } // namespace | ||
|
|
||
| void Client::setDestinationCluster(Http::RequestHeaderMap& headers) { | ||
| // Determine upstream cluster: | ||
| // - Use TLS by default. | ||
| // - Use http/2 or ALPN if requested explicitly via x-envoy-mobile-upstream-protocol. | ||
| // - Force http/1.1 if request scheme is http (cleartext). | ||
| const char* cluster{}; | ||
| auto h2_header = headers.get(H2UpstreamHeader); | ||
| if (headers.getSchemeValue() == Headers::get().SchemeValues.Http) { | ||
| cluster = ClearTextCluster; | ||
| } else if (!h2_header.empty()) { | ||
| ASSERT(h2_header.size() == 1); | ||
| const auto value = h2_header[0]->value().getStringView(); | ||
| if (value == "http2") { | ||
| cluster = H2Cluster; | ||
| } else if (value == "alpn") { | ||
| cluster = AlpnCluster; | ||
| } else { | ||
| RELEASE_ASSERT(value == "http1", fmt::format("using unsupported protocol version {}", value)); | ||
| cluster = BaseCluster; | ||
| } | ||
| } else { | ||
| cluster = BaseCluster; | ||
| } | ||
|
|
||
| if (!h2_header.empty()) { | ||
| headers.remove(H2UpstreamHeader); | ||
| } | ||
|
|
||
| headers.addCopy(ClusterHeader, std::string{cluster}); | ||
| } | ||
|
|
||
| } // namespace Http | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ - (instancetype)initWithAdminInterfaceEnabled:(BOOL)adminInterfaceEnabled | |
| h2ConnectionKeepaliveIdleIntervalMilliseconds: | ||
| (UInt32)h2ConnectionKeepaliveIdleIntervalMilliseconds | ||
| h2ConnectionKeepaliveTimeoutSeconds:(UInt32)h2ConnectionKeepaliveTimeoutSeconds | ||
| h2Hostnames:(NSArray<NSString *> *)h2Hostnames | ||
| statsFlushSeconds:(UInt32)statsFlushSeconds | ||
| streamIdleTimeoutSeconds:(UInt32)streamIdleTimeoutSeconds | ||
| perTryIdleTimeoutSeconds:(UInt32)perTryIdleTimeoutSeconds | ||
|
|
@@ -50,6 +51,7 @@ - (instancetype)initWithAdminInterfaceEnabled:(BOOL)adminInterfaceEnabled | |
| self.h2ConnectionKeepaliveIdleIntervalMilliseconds = | ||
| h2ConnectionKeepaliveIdleIntervalMilliseconds; | ||
| self.h2ConnectionKeepaliveTimeoutSeconds = h2ConnectionKeepaliveTimeoutSeconds; | ||
| self.h2Hostnames = h2Hostnames; | ||
| self.statsFlushSeconds = statsFlushSeconds; | ||
| self.streamIdleTimeoutSeconds = streamIdleTimeoutSeconds; | ||
| self.perTryIdleTimeoutSeconds = perTryIdleTimeoutSeconds; | ||
|
|
@@ -100,6 +102,18 @@ - (nullable NSString *)resolveTemplate:(NSString *)templateYAML { | |
| appendString:[[NSString alloc] initWithUTF8String:route_cache_reset_filter_insert]]; | ||
| } | ||
|
|
||
| BOOL hasH2Hostnames = self.h2Hostnames.count > 0; | ||
| if (hasH2Hostnames) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: actually build this |
||
| templateYAML = [templateYAML stringByReplacingOccurrencesOfString:@"#{fake_remote_responses}" | ||
| withString:self.directResponses]; | ||
| [customClusters appendString:[[NSString alloc] initWithUTF8String:fake_remote_cluster_insert]]; | ||
| [customListeners | ||
| appendString:[[NSString alloc] initWithUTF8String:fake_remote_listener_insert]]; | ||
| [customRoutes appendString:self.directResponseMatchers]; | ||
| [customFilters | ||
| appendString:[[NSString alloc] initWithUTF8String:route_cache_reset_filter_insert]]; | ||
| } | ||
|
|
||
| templateYAML = [templateYAML stringByReplacingOccurrencesOfString:@"#{custom_clusters}" | ||
| withString:customClusters]; | ||
| templateYAML = [templateYAML stringByReplacingOccurrencesOfString:@"#{custom_listeners}" | ||
|
|
@@ -138,6 +152,7 @@ - (nullable NSString *)resolveTemplate:(NSString *)templateYAML { | |
| (double)self.h2ConnectionKeepaliveIdleIntervalMilliseconds / 1000.0]; | ||
| [definitions appendFormat:@"- &h2_connection_keepalive_timeout %lus\n", | ||
| (unsigned long)self.h2ConnectionKeepaliveTimeoutSeconds]; | ||
| [definitions appendFormat:@"- &h2_hostnames %@\n", hasH2Hostnames ? @"[\"host.name\"]" : @"[\"host.name\"]"]; | ||
| [definitions | ||
| appendFormat:@"- &stream_idle_timeout %lus\n", (unsigned long)self.streamIdleTimeoutSeconds]; | ||
| [definitions | ||
|
|
||
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.
@goaway how come this is in the header but
base_protocol_options_defsis in the config_template?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.
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.