Skip to content
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

access_loggers: use new-style names #9921

Conversation

zuercher
Copy link
Member

@zuercher zuercher commented Feb 4, 2020

Modifies the well-known-names of the built-in access loggers to
use the same name as the extension build system.

(This is the first in a series, but I expect whatever changes may be
requested here will apply to all the rest, so I'm going to hold the others
back.)

Risk Level: low, previous name is still accepted
Testing: added test to prove factory accepts old names
Docs Changes: updated names
Release Notes: updated
Deprecated: old names are logged as deprecated

Signed-off-by: Stephan Zuercher [email protected]

Modifies the well-known-names of the built-in access loggers to
use the same name as the extension build system.

Risk Level: low, previous name is still accepted
Testing: existing tests
Docs Changes: updated names
Release Notes: updated
Deprecated: old names are logged as deprecated

Signed-off-by: Stephan Zuercher <[email protected]>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9921 was opened by zuercher.

see: more, trace.

@junr03
Copy link
Member

junr03 commented Feb 4, 2020

@kyessenov do you mind taking a look?

@kyessenov
Copy link
Contributor

@zuercher After #9618, getAndCheckFactory uses the type URL in the config as the first lookup to pick an extension. That means the name field in the config proto becomes optional. Can you check that is indeed the case by setting it to something random? I haven't tested the access log extensions, only the HTTP extensions. There might be a difference depending how the factory is selected.

@zuercher
Copy link
Member Author

zuercher commented Feb 4, 2020

It's correct that with typed_config the name is ignored. Mostly I'm undertaking this change to unify the build extension and config names for v2 in the short term. The top of each protobuf doc currently emits an extension name that isn't necessarily the correct one to use with config.

@kyessenov
Copy link
Contributor

LGTM. I agree we need this to support untyped configs in the short term, for consistency with the rest of the extensions. I can do a follow-up to assign random optional names to extensions later, once we feel that the world has moved on to v3 and 1.13.

@zuercher
Copy link
Member Author

zuercher commented Feb 5, 2020

So can we move forward with this?

@repokitteh-read-only repokitteh-read-only bot removed the api label Feb 5, 2020
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Hero!

@mattklein123 mattklein123 merged commit 9cc7a5c into envoyproxy:master Feb 6, 2020
@zuercher zuercher deleted the szuercher_update_accesslogger_ext_names branch September 22, 2021 22:10
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.

5 participants