release: flipping deprecated features to be fatal-by-default #7549

alyssawilk · 2019-07-11T20:26:50Z

Flipping the following deprecated fields to be fatal-by-default

runtime from bootstrap.proto
catch_all_cluster from redis_proxy.proto
use_original_dst from lds.proto
max_stats from server_info.proto
cluster from redis_proxy.proto
max_obj_name_len from server_info.proto

Fixing a typo in release instructions
Undeprecating hosts as we're not going to remove it until v3.
Fixing canonical configs to use legal fields (see #7548 for forward-fixing this)

Risk Level: High (for folks ignoring their deprecation warnings)
Testing: //test/... (fixed canonical configs)
Docs Changes: no (should there be?)
Release Notes: no (should there be?)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2019-07-11T20:28:21Z

@htuch for API stuff and layered runtime
@envoyproxy/senior-maintainers for any objection to fatal-by-defaults

This PR should land with an envoy-announce email so please don't merge it unless you want to send that email :-)
Given it's pretty close to EoD Thursday I'll likely merge it Monday.

mattklein123

Thanks for staying on top of this. Few small comments.

/wait

mattklein123 · 2019-07-11T20:51:28Z

source/common/runtime/runtime_features.cc

+    "envoy.deprecated_features.bootstrap.proto:runtime",
+    "envoy.deprecated_features.redis_proxy.proto:catch_all_cluster",
+    "envoy.deprecated_features.lds.proto:use_original_dst",
+    "envoy.deprecated_features.server_info.proto:max_stats",


I think this and below are on output protos so they won't have any effect. Not sure how we could know this. Thoughts?

Good point. No ideas here - I'd be inclined to leave them in to avoid script churn.
Maybe have file or proto naming in v3 to make things more clear going forward?
@htuch

mattklein123 · 2019-07-11T20:52:34Z

configs/envoy_double_proxy_v2.template.yaml

-  symlink_root: "/srv/runtime_data/current"
-  subdirectory: envoy
-  override_subdirectory: envoy_override
+layered_runtime:


The equivalent of what we had before would be:

layered_runtime: layers: - name: root disk_layer: symlink_root: /srv/configset/envoydata/current subdirectory: envoy - name: override disk_layer: symlink_root: /srv/configset/envoydata/current subdirectory: envoy_override append_service_cluster: true - name: admin admin_layer: {}

Should we keep it equivalent?

mattklein123 · 2019-07-11T20:52:49Z

configs/envoy_front_proxy_v2.template.yaml

-  subdirectory: envoy
-  override_subdirectory: envoy_override
+layered_runtime:
+    layers:


see above comment, also indent?

mattklein123 · 2019-07-11T20:52:58Z

configs/envoy_service_to_service_v2.template.yaml

-  subdirectory: envoy
-  override_subdirectory: envoy_override
+layered_runtime:
+    layers:


mattklein123 · 2019-07-15T16:25:51Z

source/common/runtime/runtime_features.cc

+    "envoy.deprecated_features.lds.proto:use_original_dst",
+    "envoy.deprecated_features.server_info.proto:max_stats",
+    "envoy.deprecated_features.redis_proxy.proto:cluster",
+    "envoy.deprecated_features.server_info.proto:max_obj_name_len",


I think we need

envoy/source/common/config/utility.cc

Line 302 in b3f848d

"envoy.deprecated_features.v1_filter_json_config")) {

also?

Oh man, I totally missed this in review but this should have been envoy.reloadable_features.v1_filter_json_config which is why it wasn't caught.

I tried flipping it, and it broke the bootstrap test pretty hard. Beyond needing the same runtime layers update, there's some issue with proto constraint which I've tracked down to a really confusing red herring where when we to do proto to json conversion we end up failing and so having a completely empty proto, so failing the first validator which checks for non-empty bytes

Given this didn't land as a config change what do we think of renaming, and flipping in its own PR, ideally after some digging into the hot restart test?

Sure whatever you think is best. I mainly want to get this deprecation moving since this will allow us to delete a ton of code (all of v1).

Would you have cycles to look into what's going on with the filter config? I was going to ask we assign whoever introduced the variable to sorting out the test and looks like it was you :-P

Yes, sorry, what do you want me to do exactly? If you give me a diff that is broken I can debug it. :)

I think we land this as-is, and you flip the v1 json separately, since it currently doesn't pass tests?

OK got it, but you want me to change the name also?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

LGTM!

mattklein123 · 2019-07-15T21:27:45Z

source/common/runtime/runtime_features.cc

@@ -47,6 +47,12 @@ constexpr const char* disallowed_features[] = {
    // Acts as both a test entry for deprecated.proto and a marker for the Envoy
    // deprecation scripts.
    "envoy.deprecated_features.deprecated.proto:is_deprecated_fatal",
+    "envoy.deprecated_features.bootstrap.proto:runtime",


nit: I would probably alpha sort all of these if possible.

I will alpha order when I do my follow up PR.

"use_original_dst" and "bind_to_port" are two complementary parts of the "virtual listeners" feature, and they should be deprecated together. However, they are both currently exempted (see: envoyproxy#5355), so revert the change from envoyproxy#7549. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

"use_original_dst" and "bind_to_port" are two complementary parts of the "virtual listeners" feature, and they should be deprecated together. However, they are both currently exempted (see: #5355), so revert the change from #7549. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

"use_original_dst" and "bind_to_port" are two complementary parts of the "virtual listeners" feature, and they should be deprecated together. However, they are both currently exempted (see: envoyproxy#5355), so revert the change from envoyproxy#7549. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

release: flipping deprecated features to be fatal-by-default

e988d1d

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk assigned htuch Jul 11, 2019

mattklein123 requested changes Jul 11, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 11, 2019

mattklein123 reviewed Jul 15, 2019

View reviewed changes

canonical config

8d942c9

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Jul 15, 2019

mattklein123 approved these changes Jul 15, 2019

View reviewed changes

mattklein123 merged commit 7a55417 into envoyproxy:master Jul 16, 2019

PiotrSikora mentioned this pull request Jul 17, 2019

release: un-deprecate "use_original_dst". #7623

Merged

alyssawilk deleted the release3 branch July 31, 2019 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

release: flipping deprecated features to be fatal-by-default #7549

release: flipping deprecated features to be fatal-by-default #7549

alyssawilk commented Jul 11, 2019

alyssawilk commented Jul 11, 2019

mattklein123 left a comment

mattklein123 Jul 11, 2019

alyssawilk Jul 15, 2019

mattklein123 Jul 11, 2019

mattklein123 Jul 11, 2019

mattklein123 Jul 11, 2019

mattklein123 Jul 15, 2019

alyssawilk Jul 15, 2019

mattklein123 Jul 15, 2019

alyssawilk Jul 15, 2019

mattklein123 Jul 15, 2019

alyssawilk Jul 15, 2019

mattklein123 Jul 15, 2019

mattklein123 left a comment

mattklein123 Jul 15, 2019

mattklein123 Jul 16, 2019

release: flipping deprecated features to be fatal-by-default #7549

release: flipping deprecated features to be fatal-by-default #7549

Conversation

alyssawilk commented Jul 11, 2019

alyssawilk commented Jul 11, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment