Skip to content

Remove the hidden_envoy_deprecated_UNSUPPORTED_REST_LEGACY #17804

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.

Sign up for GitHub

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

Closed
tyxia wants to merge 1 commit into from
Closed

Remove the hidden_envoy_deprecated_UNSUPPORTED_REST_LEGACY #17804

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions source/common/config/subscription_factory_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,6 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource(
api_config_source);
const auto transport_api_version = Utility::getAndCheckTransportVersion(api_config_source);
switch (api_config_source.api_type()) {
case envoy::config::core::v3::ApiConfigSource::hidden_envoy_deprecated_UNSUPPORTED_REST_LEGACY:

@htuch htuch Aug 25, 2021

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.

Since the PGV annotations for the v3 version of this field DEPRECATED_AND_UNAVAILABLE_DO_NOT_USE don't force rejection in some situations, should we keep this? When we get rid of generated_api_version we can then either use a PGV annotation or the DEPRECATED_AND_UNAVAILABLE_DO_NOT_USE name here instead.

@tyxia tyxia Aug 25, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for the review!

I think it should be safe to remove this because default case (i.e. default: NOT_REACHED_GCOVR_EXCL_LINE;) of switch statement will catch the case where people use this field by mistake.

But i feel keeping this might provide better/richer Exception message.
I will revert this change then/

@htuch htuch Aug 26, 2021

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.

Yeah, we can't rely on NOT_REACHED_GCOVR_EXCL_LINE, since this is an assertion, rather than a graceful configuration rejection which we see when we throw EnvoyException.

throw EnvoyException(
"REST_LEGACY no longer a supported ApiConfigSource. "
"Please specify an explicit supported api_type in the following config:\n" +
config.DebugString());
case envoy::config::core::v3::ApiConfigSource::REST:
return std::make_unique<HttpSubscriptionImpl>(
local_info_, cm_, api_config_source.cluster_names()[0], dispatcher_,
Expand Down