rds: allow v1 rds config to use a v2 subscription#1583
Conversation
|
@htuch per your recommendation the change is not documented, as to not push folks to start using V2. Also in terms of testing this change, should I move the translateApiConfigSource function into the Utility namespace, make it static, and create a test for it? |
|
Please add this to v1 docs. It doesn't need to be verbose but you can link to proto in envoy-api repo. |
|
If we don't want to add docs, then please add a TODO for yourself to doc later. |
|
If adding to v1 docs, we need some kind of disclaimer/warning that this
isn't prod ready. I feel that once we have constraints, we can relax, but
right now, minor misconfigurations will cause Envoy to crash hard with v2.
…On Thu, Aug 31, 2017 at 7:50 PM Matt Klein ***@***.***> wrote:
Please add this to v1 docs. It doesn't need to be verbose but you can link
to proto in envoy-api repo.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1583 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AKaLv_AS1kYqO-dPn1JJiCG4IUW4gRyJks5sd0begaJpZM4PJiz->
.
|
|
Yup that's fine. Let's just add TODO for missing docs. |
source/common/config/utility.cc
Outdated
| envoy::api::v2::ConfigSource& eds_config) { | ||
| translateApiConfigSource(json_config.getObject("cluster")->getString("name"), | ||
| json_config.getInteger("refresh_delay_ms", 30000), | ||
| json_config.getString("api_type", "REST_LEGACY"), |
There was a problem hiding this comment.
Make "REST_LEGACY" a const somewhere. And also yes please make the above some static utility function w/ tests.
|
@mattklein123 @htuch updated. |
source/common/config/utility.h
Outdated
| return {ALL_SUBSCRIPTION_STATS(POOL_COUNTER(scope))}; | ||
| } | ||
|
|
||
| static const std::string REST_LEGACY; |
There was a problem hiding this comment.
nit: This should use the const singleton pattern. See Http::Headers
There was a problem hiding this comment.
Sorry, I am confused about this. If declare and initialize like in source/common/http/headers.h with curly brace initialization the compiler complains about in-class initialization of static data member of non-literal type, but of course I can't make them non-static because I would be using the variable in a static member function. What am I missing?
There was a problem hiding this comment.
You create a class with non-static members, and then access it via the static get() in https://github.com/lyft/envoy/blob/master/source/common/common/singleton.h.
There was a problem hiding this comment.
Nice this makes it better. I knew I had seen this as the Const counter part to the mutable singleton, should have scrolled to the end of the file. Another tool in the toolshed 👍
htuch
left a comment
There was a problem hiding this comment.
LGTM with the exception of outstanding comments.
source/common/config/utility.cc
Outdated
| api_config_source.set_api_type(envoy::api::v2::ApiConfigSource::REST_LEGACY); | ||
| } else if (api_type == REST) { | ||
| api_config_source.set_api_type(envoy::api::v2::ApiConfigSource::REST); | ||
| } else if (api_type == GRPC) { |
There was a problem hiding this comment.
This should probably be:
} else {
ASERT(api_type == GRPC);
...
}
since the JSON schema knows this is an enum and has validated.
|
@mattklein123 @htuch should be ready. |
cds: allow v1 config to define api_type in the json-schema Description: #1583 enabled v1 bootstrap configs to define the api type of the management server. This PR adds that constraint to the CDS definition schema. Risk Level: Low Testing: existing utility tests test the translation; existing tests test that a v2 grpc cds server can be used. Signed-off-by: Jose Nino <jnino@lyft.com>
Description: API for addDnsQueryTimeoutSeconds to configure behavior introduced in #1580 Risk Level: low - optional builder API Testing: updated suite. Docs Changes: added Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: API for addDnsQueryTimeoutSeconds to configure behavior introduced in #1580 Risk Level: low - optional builder API Testing: updated suite. Docs Changes: added Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
…tor (#1583) **Description** This moves translator.LLMTokenUsage struct into metrics package from translator package to make sure that all call site of metrics interface uses the same signature by simply passing the cost info from translator. This will help unify all the per-endpoint implementation detail. **Related Issues/PRs (if applicable)** Preparation for #90 --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Right now if envoy is configured with a v1 base configuration, the xDS APIs are only enabled to use a v1 REST subscription. This change allows a v1 rds configuration to get a v2 subscription (both REST and gRPC).