Skip to content

config: warn on registry when deprecated config names were used#9048

Merged
htuch merged 9 commits intoenvoyproxy:masterfrom
Shikugawa:fix-issue-8540
Nov 26, 2019
Merged

config: warn on registry when deprecated config names were used#9048
htuch merged 9 commits intoenvoyproxy:masterfrom
Shikugawa:fix-issue-8540

Conversation

@Shikugawa
Copy link
Copy Markdown
Member

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

Description: to resolve #8540, warn to envoy users they used deprecated config names
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa Shikugawa marked this pull request as ready for review November 16, 2019 08:26
@Shikugawa Shikugawa requested a review from lizan as a code owner November 16, 2019 08:26
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks, this is a good idea. A few comments to kick this off, looking for @lizan 's thoughts as well.

One other thing we might want is a stat, so that monitoring systems can pick up on this.

/wait

Signed-off-by: Shikugawa <rei@tetrate.io>
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks, this is really nice. Can you add tests and make sure CI passes?
/wait

Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Copy link
Copy Markdown
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

LGTM. @htuch ?

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks!

@htuch htuch merged commit ddcfc67 into envoyproxy:master Nov 26, 2019
auto it = deprecatedFactoryNames().find(name);
const bool status = it != deprecatedFactoryNames().end();
if (status) {
ENVOY_LOG(warn, "{} is deprecated, use {} instead.", it->first, it->second);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Shikugawa @lizan @htuch shouldn't we also increment the deprecated stat somehow? I feel like we really have to do this as part of our policy and should. @alyssawilk WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reopened #8540, we should increment deprecated stat / disable that in ENVOY_DISABLE_DEPRECATED_FEATURES cases etc.

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.

config: allow double registration with different name and deprecation

4 participants