tcp_proxy: remove support for v1 routes #4446
Conversation
Signed-off-by: Venil Noronha <[email protected]>
| // <https://github.com/envoyproxy/envoy/issues/2441>`_. If you want to configure the filter | ||
| // using v1 config structure, please make this field a boolean with value ``true`` and configure | ||
| // via the opaque ``value`` field like is suggested in :api:`envoy/config/filter/README.md`. | ||
| DeprecatedV1 deprecated_v1 = 6 [deprecated = true]; |
There was a problem hiding this comment.
Please reserve this: https://developers.google.com/protocol-buffers/docs/proto3#reserved
@htuch Any other gotchas around removing fields from proto?
source/common/config/filter_json.cc
Outdated
| @@ -378,21 +378,9 @@ void FilterJson::translateTcpProxy( | |||
| json_config.validateSchema(Json::Schema::TCP_PROXY_NETWORK_FILTER_SCHEMA); | |||
|
|
|||
| JSON_UTIL_SET_STRING(json_config, proto_config, stat_prefix); | |||
| JSON_UTIL_SET_STRING(json_config, proto_config, cluster); | |||
There was a problem hiding this comment.
I don't think we need to add this. v1 config is deprecated and we're not adding new fields to it.
| // required to use more complex routing in the interim. | ||
| // | ||
| // The upstream cluster to connect to. Cluster matching is done via :ref:`filter | ||
| // chain matching rules <envoy_api_msg_listener.FilterChainMatch>`. | ||
| string cluster = 2; |
There was a problem hiding this comment.
Add [(validate.rules).string.min_bytes = 1]
| if (proto_config.has_deprecated_v1()) { | ||
| ASSERT(proto_config.deprecated_v1().routes_size() > 0); | ||
| } | ||
| ASSERT(!proto_config.cluster().empty()); |
There was a problem hiding this comment.
Since there has been no constraint on this field in the proto, I don't think it's acceptable to ASSERT this. Throw instead.
There was a problem hiding this comment.
Addressed all comments in 21ed079.
* Mark field 6 as reserved in proto * Add field validation to proto * Revert json conversion for cluster field * Add config validation Signed-off-by: Venil Noronha <[email protected]>
venilnoronha
force-pushed
the
remove-v1-tcp-routes
branch
from
7d8887d
to
21ed079
Compare
Signed-off-by: Venil Noronha <[email protected]>
Signed-off-by: Venil Noronha <[email protected]>
Signed-off-by: Venil Noronha <[email protected]>
|
We had a discussion on slack, and before we can delete this, we need to list it in DEPRECATED.md, then wait a release cycle. So unfortunately, we can't merge this for quite awhile. |
|
I think it's ambiguous whether "use of the v1 api" includes fields in the v2 api that happen to be named "deprecated_v1". For total clarity, let's add a specific mention of "tcp_proxy.deprecated_v1" being deprecated. If you don't want to, I'm happy to do it. |
|
@ggreenway I'd make that |
|
I'm on it now. |
|
Thanks @venilnoronha! |
This is a followup change as requested in this comment in #4446. Signed-off-by: Venil Noronha <[email protected]>
|
I'm going to close this for now, because we need to wait until the deprecation cycle has finished to do this. |
Description: This PR serves as a prereq for #4430 by removing the support for v1 routes in TCP Proxy.
Signed-off-by: Venil Noronha [email protected]
Risk Level: Low
Testing: Existing tests suffice
Docs Changes: Added feature removal notice to
version_history.rstRelease Notes: NA
/cc @ggreenway @rshriram