config: extended warnings on v2 API usage.#13238
Conversation
Inspired by a log statement needed for envoyproxy#10815, this patch: * Adds _MISC variants of sparse macros. * Sends _MISC log messages to fancy logger (these were previously ignored). This is a bit weird, as "misc" is also a logger and we use file-specific loggers in fancy logger, but in reality, most folks treat this macro as the generic Envoy log statement. * Avoid evaluating expressions in ENVOY_LOG* with fancy logger when beneath the configured log level. Risk level: Low Testing: Mostly covered by log_macros_test already, added some _MISC coverage in SparseLogMacrosTest. Signed-off-by: Harvey Tuch <htuch@google.com>
The earlier PR envoyproxy#10964 added in validation that we're not using v2 xDS transport, but we didn't have any warning on v2 bootstraps, v2 extension configs inside a v3 config and also for v2 resource types. This PR adds support for these validations by warning and incrementing the deprecated feature use stat. The following notifications are added: * Rate limited warning log * Trace-level log * The runtime.deprecated_features is bumped, but only after server initialization, so this doesn't apply to bootstrap. This is consistent with how other deprecated features are treated today. Risk level: Medium (it's possible that this will cause monitoring to alert, the boosting logic is also quite complex). Testing: Unit tests and server bootstrap tests added. Fixes envoyproxy#10815 Signed-off-by: Harvey Tuch <htuch@google.com>
|
Note that this depends on #13223 merging first. |
Signed-off-by: Harvey Tuch <htuch@google.com>
|
LMK when this is ready for review. /wait |
|
@mattklein123 ready for review. |
mattklein123
left a comment
There was a problem hiding this comment.
LGTM with small comments/questions. Thank you!
/wait
source/common/protobuf/utility.cc
Outdated
| "deprecated and will be removed from Envoy at the start of Q1 2021: {}", | ||
| desc); | ||
| ENVOY_LOG_MISC(trace, warning_str); | ||
| ENVOY_LOG_PERIODIC_MISC(warn, 1s, warning_str); |
There was a problem hiding this comment.
Is this going to be useful for the user since they may not see all the things that are warning? It's kind of awful but should this be a static map of warnings and we log each unique one occasionally? Thoughts?
There was a problem hiding this comment.
I added a static map, LMK what you think.
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks this LGTM. Can you check CI? Merging main should fix ASAN. Also, see the various GH/slack messages about strangeness in the tests around deprecating the watchdog field in bootstrap. If you have time to look at that it would be appreciated as it's related to this stuff (fine in a follow up).
/wait
|
Sorry needs one more main merge. /wait |
Signed-off-by: Harvey Tuch <htuch@google.com>
|
Needs main marge and looks like tidy is still failing. /wait |
Signed-off-by: Harvey Tuch <htuch@google.com>
|
Still needs main merge. /wait |
config: extended warnings on v2 API usage.
The earlier PR #10964 added in validation that we're not using v2 xDS
transport, but we didn't have any warning on v2 bootstraps, v2
extension configs inside a v3 config and also for v2 resource types.
This PR adds support for these validations by warning and incrementing
the deprecated feature use stat. The following notifications are added:
initialization, so this doesn't apply to bootstrap. This is consistent
with how other deprecated features are treated today.
Risk level: Medium (it's possible that this will cause monitoring to
alert, the boosting logic is also quite complex).
Testing: Unit tests and server bootstrap tests added.
Fixes #10815
Signed-off-by: Harvey Tuch htuch@google.com