-
Notifications
You must be signed in to change notification settings - Fork 5.4k
route match: Add runtime_fraction field for more granular routing #4217
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 7 commits
9490c37
9f7fc09
dad5be2
7ee808d
7e92dca
e40132c
fca465f
6d5c6bc
1ff4323
4df9106
336122e
4121f9d
aee0a34
87bc01a
cb67993
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 |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| syntax = "proto3"; | ||
|
|
||
| package envoy.type; | ||
|
|
||
| import "validate/validate.proto"; | ||
| import "gogoproto/gogo.proto"; | ||
|
|
||
| import "envoy/type/percent.proto"; | ||
|
|
||
| option (gogoproto.equal_all) = true; | ||
|
|
||
| // [#protodoc-title: RuntimePercent] | ||
|
|
||
| // Runtime derived FractionalPercent with defaults for when the numerator or denominator is not | ||
| // specified via a runtime key. | ||
| message RuntimeFractionalPercent { | ||
|
Member
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. Should this be in
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. I didn't do that because I didn't want to introduce a dependency on percent.proto. It was originally in percent.proto, but an earlier comment from @danielhochman suggested I move it out into its own file.
Member
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. It's fine for base to depend on percent.proto (just not the other way around). |
||
| // Default value if the runtime value's for the numerator/denominator keys are not available. | ||
| envoy.type.FractionalPercent default_value = 1 [(validate.rules).message.required = true]; | ||
|
|
||
| // Runtime key for a YAML representation of a FractionalPercent. | ||
| string runtime_key = 2; | ||
|
htuch marked this conversation as resolved.
Outdated
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,13 +11,15 @@ | |
|
|
||
| #include "envoy/http/header_map.h" | ||
| #include "envoy/runtime/runtime.h" | ||
| #include "envoy/type/percent.pb.validate.h" | ||
| #include "envoy/upstream/cluster_manager.h" | ||
| #include "envoy/upstream/upstream.h" | ||
|
|
||
| #include "common/common/assert.h" | ||
| #include "common/common/empty_string.h" | ||
| #include "common/common/fmt.h" | ||
| #include "common/common/hash.h" | ||
| #include "common/common/logger.h" | ||
| #include "common/common/utility.h" | ||
| #include "common/config/metadata.h" | ||
| #include "common/config/rds_json.h" | ||
|
|
@@ -26,6 +28,7 @@ | |
| #include "common/http/headers.h" | ||
| #include "common/http/utility.h" | ||
| #include "common/http/websocket/ws_handler_impl.h" | ||
| #include "common/protobuf/protobuf.h" | ||
| #include "common/protobuf/utility.h" | ||
| #include "common/router/retry_state_impl.h" | ||
|
|
||
|
|
@@ -260,7 +263,7 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost, | |
| timeout_(PROTOBUF_GET_MS_OR_DEFAULT(route.route(), timeout, DEFAULT_ROUTE_TIMEOUT_MS)), | ||
| idle_timeout_(PROTOBUF_GET_OPTIONAL_MS(route.route(), idle_timeout)), | ||
| max_grpc_timeout_(PROTOBUF_GET_OPTIONAL_MS(route.route(), max_grpc_timeout)), | ||
| runtime_(loadRuntimeData(route.match())), loader_(factory_context.runtime()), | ||
| loader_(factory_context.runtime()), runtime_(loadRuntimeData(route.match())), | ||
| host_redirect_(route.redirect().host_redirect()), | ||
| path_redirect_(route.redirect().path_redirect()), | ||
| https_redirect_(route.redirect().https_redirect()), | ||
|
|
@@ -345,8 +348,11 @@ bool RouteEntryImplBase::matchRoute(const Http::HeaderMap& headers, uint64_t ran | |
| bool matches = true; | ||
|
|
||
| if (runtime_) { | ||
| matches &= loader_.snapshot().featureEnabled(runtime_.value().key_, runtime_.value().default_, | ||
| random_value); | ||
| matches &= random_value % runtime_->denominator_val_ < runtime_->numerator_val_; | ||
| if (!matches) { | ||
| // No need to waste further cycles calculating a route match. | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| matches &= Http::HeaderUtility::matchHeaders(headers, config_headers_); | ||
|
|
@@ -404,14 +410,36 @@ void RouteEntryImplBase::finalizeResponseHeaders( | |
| absl::optional<RouteEntryImplBase::RuntimeData> | ||
| RouteEntryImplBase::loadRuntimeData(const envoy::api::v2::route::RouteMatch& route_match) { | ||
| absl::optional<RuntimeData> runtime; | ||
| if (route_match.has_runtime()) { | ||
| RuntimeData data; | ||
| data.key_ = route_match.runtime().runtime_key(); | ||
| data.default_ = route_match.runtime().default_value(); | ||
| runtime = data; | ||
| RuntimeData runtime_data; | ||
|
|
||
| if (route_match.runtime_specifier_case() == envoy::api::v2::route::RouteMatch::kRuntimeFraction) { | ||
| envoy::type::FractionalPercent fractional_percent; | ||
| const std::string& fraction_yaml = | ||
| loader_.snapshot().get(route_match.runtime_fraction().runtime_key()); | ||
|
|
||
| try { | ||
| MessageUtil::loadFromYamlAndValidate(fraction_yaml, fractional_percent); | ||
| } catch (const EnvoyException& ex) { | ||
| ENVOY_LOG(error, "failed to parse string value for runtime key {}: {}", | ||
| route_match.runtime_fraction().runtime_key(), ex.what()); | ||
| fractional_percent = route_match.runtime_fraction().default_value(); | ||
| } | ||
|
Member
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. If I read this right, failure to convert the denominator key into a valid denominator value causes us to use the numerator key's value with the default value's denominator. That seems like it might cause some pretty unexpected behavior. Is it reasonable to just switch using the default value altogether in this case?
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. I think what you propose is the sane thing to do. I'll change the logic.
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. Actually- scratch that. A user would have unexpected behavior either way if they misspell a denominator type and it switches to a default value. I'm not sure one way is better than the other. @htuch what do you think?
Member
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. Not sure what the best practice to alert when runtime is wrong; how do folks operationally determine when a runtime change has taken effect? Via One thought that occurred to me just now is that instead of having separate values for numerator/denominator, we have a single values, which is a textual representation of a |
||
|
|
||
| runtime_data.numerator_val_ = fractional_percent.numerator(); | ||
| runtime_data.denominator_val_ = | ||
| ProtobufPercentHelper::fractionalPercentDenominatorToInt(fractional_percent.denominator()); | ||
| } else if (route_match.runtime_specifier_case() == envoy::api::v2::route::RouteMatch::kRuntime) { | ||
| // For backwards compatibility, the deprecated 'runtime' field must be converted to a | ||
| // RuntimeData format with a variable denominator type. The 'runtime' field assumes a percentage | ||
| // (0-100), so the hard-coded denominator value reflects this. | ||
| runtime_data.denominator_val_ = 100; | ||
| runtime_data.numerator_val_ = loader_.snapshot().getInteger( | ||
| route_match.runtime().runtime_key(), route_match.runtime().default_value()); | ||
| } else { | ||
| return runtime; | ||
| } | ||
|
|
||
| return runtime; | ||
| return runtime_data; | ||
| } | ||
|
|
||
| void RouteEntryImplBase::finalizePathHeader(Http::HeaderMap& headers, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.