release: flipping fatal-by-defaults by alyssawilk · Pull Request #8847 · envoyproxy/envoy

alyssawilk · 2019-11-01T00:48:04Z

Making the following configs fatal by default:
"envoy.deprecated_features.route.proto:regex_match",
"envoy.deprecated_features.route.proto:regex",
"envoy.deprecated_features.cds.proto:tls_context",
"envoy.deprecated_features.route.proto:allow_origin",
"envoy.deprecated_features.string.proto:regex",
"envoy.deprecated_features.route.proto:value",
"envoy.deprecated_features.http_connection_manager.proto:idle_timeout",
"envoy.deprecated_features.lds.proto:use_original_dst",
"envoy.deprecated_features.config_source.proto:DEPRECATED_AND_UNAVAILABLE_DO_NOT_USE",
"envoy.deprecated_features.route.proto:pattern",
"envoy.deprecated_features.listener.proto:tls_context",
"envoy.deprecated_features.http_connection_manager.proto:operation_name",
"envoy.deprecated_features.health_check.proto:use_http2",
"envoy.deprecated_features.trace.proto:DEPRECATED_AND_UNAVAILABLE_DO_NOT_USE",
"envoy.deprecated_features.trace.proto:HTTP_JSON_V1",
"envoy.deprecated_features.cds.proto:ORIGINAL_DST_LB",
"envoy.deprecated_features.route.proto:regex",
"envoy.deprecated_features.cds.proto:extension_protocol_options",
"envoy.deprecated_features.route.proto:method",

Risk Level: High

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

alyssawilk · 2019-11-01T00:49:42Z

I'm going to put this out here before the tests pass so @envoyproxy/maintainers can bike shed about what things we don't want to have fatal.

Also meta, while I love landing code and then turning features on, I think quarterly cadence is too slow, because it makes for one massive high risk PR. Should we true-by-default and just leave things in the code for 6 months? true by default unless it's super high risk (like the buffer change)? Thoughts?

alyssawilk · 2019-11-01T00:59:02Z

Also as long as I'm inviting bike shedding, I'm inclined to split out fatal-by-defaults from feature flips, in case one causes more problems than the other. Thoughts?

May check in tomorrow but I don't think it's urgent we land this all before monday so we'll see :-P

alyssawilk · 2019-11-01T01:03:44Z

Yeah, tests are unhappy locally. I'll look into it Monday unless someone else wants to take a look?
I still encourage discussion in the meantime.

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

mattklein123

Yeah definitely some stuff to discuss in here. Let's all chat as a group on Monday.

/wait

mattklein123 · 2019-11-01T03:02:53Z

source/common/runtime/runtime_features.cc

+    // 1.12.0
+    "envoy.deprecated_features.route.proto:regex_match",
+    "envoy.deprecated_features.route.proto:regex",
+    "envoy.deprecated_features.accesslog.proto:config",


We just did this 1-2 weeks ago so this seems pretty mean for those running on master. Defer to next release? Although aren't we moving to a 6 month cycle for this anyway? I can't remember. (Applies to all config in this list). Also I think this deprecation isn't even fully completed yet as @lizan identified some stragglers. cc @yanavlasov

Stragglers should be addressed PR #8861

Do we want to leave everything with config until the next release then? I don't think we can do the stragglers in this release

+1 let's just leave until next release.

mattklein123 · 2019-11-01T03:03:08Z

source/common/runtime/runtime_features.cc

+    "envoy.deprecated_features.route.proto:regex",
+    "envoy.deprecated_features.accesslog.proto:config",
+    "envoy.deprecated_features.thrift_proxy.proto:config",
+    "envoy.deprecated_features.cds.proto:tls_context",


Same, just done, seems not good.

Given this has been stale for a month, WDYT? :-P

My concern is still that this is a config option that almost everyone sets. With that said, after a month, it's probably fine. @lizan WDYT?

mattklein123 · 2019-11-01T03:04:39Z

source/common/runtime/runtime_features.cc

+    "envoy.deprecated_features.route.proto:allow_origin",
+    "envoy.deprecated_features.string.proto:regex",
+    "envoy.deprecated_features.grpc_service.proto:config",
+    "envoy.deprecated_features.tcp_proxy.proto:deprecated",


Is this right? I think this should be "deprecated_v1"

Yeah, the script doesn't deal well with

// [#next-free-field: 11]
message TcpProxy {
// [#not-implemented-hide:] Deprecated.
// TCP Proxy filter configuration using V1 format.
message DeprecatedV1 {
option deprecated = true;

I think this one is beyond python so should manually tweak.

mattklein123 · 2019-11-01T03:05:31Z

source/common/runtime/runtime_features.cc

+    "envoy.deprecated_features.grpc_service.proto:config",
+    "envoy.deprecated_features.tcp_proxy.proto:deprecated",
+    "envoy.deprecated_features.overload.proto:config",
+    "envoy.deprecated_features.route.proto:value",


I think this is in a nested message. I'm a little worried this might accidentally conflict? Should the name actually include the full ptah?

mmm, I agree, but I think changing naming at this point is going to be ugly.
I'll see what I can do - I think we'll have to transition over to the new naming pretty carefully.

Oh yeah, I remember why I didn't do this - it's really non-trivial to do given the python script is fairly stateless and doesn't know the message it's in. I think we'd have to fundamentally rewrite from scratch into something that did proto introspection.

Another alternate is we could have a not-yet-fatal section in runtime_features, and ASSERT in test that everything tagged as deprecated is either in disallowed_features or not_yet_disallowed_features. This would CI-regression-test that folks to add their own fully qualified fields as they added code, and then we could just cut from not_yet_disallowed to disallowed as part of the release.

WDYT?

@alyssawilk do you mind briefly syncing up with @htuch about this? Some of the proto parsing stuff he is doing now is pretty incredibly and I'm guessing it wouldn't be that difficult to change your script to actually run against the real proto tree.

Yeah, I think the approach you could take here is to leverage the API type database to pull out this information. There may be other approaches we could take, e.g. we could include deprecation date annotations which could allow us to infer the disallowed fields via reflection.

Sure. Do you have someone who could take a look some time this release (at which point I can revert this) and file a tracking bug at someone else? Alternately I could snag some time on your calendar for code pointers, given my python fu is pretty weak.

I think anyone who can do this is probably going to be busy landing v3 for this release, so I think we can TODO/file issue here.

OK, so I'll just land manual cleanups, and TODO script cleanups or script rewrites.

mattklein123 · 2019-11-01T03:06:02Z

source/common/runtime/runtime_features.cc

+    "envoy.deprecated_features.listener.proto:tls_context",
+    "envoy.deprecated_features.listener.proto:config",
+    "envoy.deprecated_features.health_check.proto:config",
+    "envoy.deprecated_features.http_connection_manager.proto:[(validate.rules).enum",


mmm, that's
OperationName operation_name = 1
[(validate.rules).enum = {defined_only: true}, deprecated = true];

Again I think there's some things that are beyond simple checking - we'd have to either do this programatically which would be awesome but I'm not sure how to do, or accept some clean up

See above comment, let's sync up with @htuch

mattklein123 · 2019-11-01T03:07:10Z

Should we true-by-default and just leave things in the code for 6 months? true by default unless it's super high risk (like the buffer change)? Thoughts?

+1

Also as long as I'm inviting bike shedding, I'm inclined to split out fatal-by-defaults from feature flips, in case one causes more problems than the other. Thoughts?

+1

stale · 2019-11-12T16:29:04Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

alyssawilk

Woo, I have time to work on Envoy this month!
Still not ready for review - I'm going to do some script fixes, but sending out comments for conversation

alyssawilk · 2019-12-02T17:09:18Z

source/common/runtime/runtime_features.cc

+    // 1.12.0
+    "envoy.deprecated_features.route.proto:regex_match",
+    "envoy.deprecated_features.route.proto:regex",
+    "envoy.deprecated_features.accesslog.proto:config",


Do we want to leave everything with config until the next release then? I don't think we can do the stragglers in this release

alyssawilk · 2019-12-02T17:09:30Z

source/common/runtime/runtime_features.cc

+    "envoy.deprecated_features.route.proto:regex",
+    "envoy.deprecated_features.accesslog.proto:config",
+    "envoy.deprecated_features.thrift_proxy.proto:config",
+    "envoy.deprecated_features.cds.proto:tls_context",


Given this has been stale for a month, WDYT? :-P

alyssawilk · 2019-12-02T17:32:13Z

source/common/runtime/runtime_features.cc

+    "envoy.deprecated_features.grpc_service.proto:config",
+    "envoy.deprecated_features.tcp_proxy.proto:deprecated",
+    "envoy.deprecated_features.overload.proto:config",
+    "envoy.deprecated_features.route.proto:value",


mmm, I agree, but I think changing naming at this point is going to be ugly.
I'll see what I can do - I think we'll have to transition over to the new naming pretty carefully.

alyssawilk · 2019-12-02T17:33:16Z

source/common/runtime/runtime_features.cc

+    "envoy.deprecated_features.route.proto:allow_origin",
+    "envoy.deprecated_features.string.proto:regex",
+    "envoy.deprecated_features.grpc_service.proto:config",
+    "envoy.deprecated_features.tcp_proxy.proto:deprecated",


Yeah, the script doesn't deal well with

// [#next-free-field: 11]
message TcpProxy {
// [#not-implemented-hide:] Deprecated.
// TCP Proxy filter configuration using V1 format.
message DeprecatedV1 {
option deprecated = true;

I think this one is beyond python so should manually tweak.

alyssawilk · 2019-12-02T17:34:28Z

source/common/runtime/runtime_features.cc

+    "envoy.deprecated_features.listener.proto:tls_context",
+    "envoy.deprecated_features.listener.proto:config",
+    "envoy.deprecated_features.health_check.proto:config",
+    "envoy.deprecated_features.http_connection_manager.proto:[(validate.rules).enum",


mmm, that's
OperationName operation_name = 1
[(validate.rules).enum = {defined_only: true}, deprecated = true];

Again I think there's some things that are beyond simple checking - we'd have to either do this programatically which would be awesome but I'm not sure how to do, or accept some clean up

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

alyssawilk · 2019-12-12T17:00:24Z

Ok, got rid of stragglers. Running tests locally but figured I'd ping about protocol buffer names - do we want to do name.field for 1.12 or do the changeover consistently for all fields when we cut to the next API version? I can go either way

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

mattklein123

Thanks for coming back to this. A few comments.

/wait

mattklein123 · 2019-12-12T20:33:50Z

configs/envoy_double_proxy_v2.template.yaml

        tracing:
          operation_name: INGRESS
-        idle_timeout: 840s
+        http2_protocol_options: {}


oops, fixed

Did you mean to delete this?

mattklein123 · 2019-12-12T20:34:12Z

configs/envoy_double_proxy_v2.template.yaml

+        common_http_protocol_options:
+          idle_timeout: 840s


How did this pass tests in the first place? Shouldn't this have failed somehow on the compile time options build?

We had the example configs test tagged as excluded. I'll go and fix that as I think it's more trouble than it's worth.

mattklein123 · 2019-12-12T20:35:23Z

examples/cors/backend/front-envoy.yaml

+                - safe_regex:
+                    google_re2: {}
+                    regex: .*


nit: do an exact match on "*" here. Same question though on how this passed tests before?

mattklein123 · 2019-12-12T20:37:18Z

Running tests locally but figured I'd ping about protocol buffer names - do we want to do name.field for 1.12 or do the changeover consistently for all fields when we cut to the next API version? I can go either way

Sorry can you clarify? I'm not sure what we are choosing between?

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

alyssawilk · 2019-12-12T21:55:45Z

At a high level we were discussing if we wanted to have
proto.file:field_name
or
proto.file:MessageName:field_name
for the new ones only.
While I'm not positive I can update the scripts to do the latter I can do a one off manually and Harvey and team can fix it for the next release.
or we can just switch naming when we cut v3.

mattklein123 · 2019-12-13T00:03:21Z

While I'm not positive I can update the scripts to do the latter I can do a one off manually and Harvey and team can fix it for the next release.

IMO I would just manually fix it now? The downside would be potentially breaking people who are setting these flags but IMO an envoy-announce@ email would be good enough for that. WDYT?

/wait-any

alyssawilk · 2019-12-16T21:11:34Z

Oh sorry, did you want to fix including all the ones that are overridedden today, or just the new ones? I thought we'd have to support both for a bit to not be crummy.

Also two questions. If we're going to change this
Do we want to just go from filename:fieldname to FieldDescriptor fullname, i.e. from
envoy.deprecated_features.deprecated.proto:is_deprecated to
envoy.test.deprecation_test.Base.is_deprecated
Second and @htuch is this all getting replaced by something fancier like auto-conversion in v2 and I should just revert this PR and let you sort things out going forward?

alyssawilk · 2019-12-16T21:13:35Z

Fullname has the disadvantage f being a bit longer, but AFIK can't have conflicts

mattklein123 · 2019-12-17T01:11:09Z

Oh sorry, did you want to fix including all the ones that are overridedden today, or just the new ones? I thought we'd have to support both for a bit to not be crummy.

IMO it's probably OK to just change it without supporting both as long as we email envoy-announce, since users should be able to set both variables in runtime if needed before the deploy. I think this will effect almost no one so it would be better to reduce our pain if we think that is ok.

Fullname has the disadvantage f being a bit longer, but AFIK can't have conflicts

Big +1 on full name. @htuch any thoughts here? Also see the comment above about auto conversion.

/wait

htuch · 2019-12-17T03:54:38Z

Yeah, full name is the way to go for sure, way less danger of conflict. Regarding whether this gets replaced by something fancier; I think in terms of the technology we use, we have options after v3 lands, we now have some pretty deep proto and C++ analysis/rewrite capabilities (Envoy begins learning at a geometric rate and becomes self aware at 2:14am Eastern time, January 1 2020).

OTOH, I do like the idea of fatal-by-default flips each quarter, so orthogonal to the technology choice, I think we want to maintain this capability. This encourages earlier migration to new API use best practices and provides an audit trail of where folks are lagging, within a single major API version.

stale · 2019-12-24T04:22:33Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-12-31T04:36:59Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

alyssawilk · 2020-01-06T21:14:20Z

So I had this theory wherein I'd get this out today, and unexpectedly spent [longer than I will admit] why I couldn't get things to fail

Turns out with the v3 migration, we had depcated and made fatal by default
envoy.deprecated_features.http_connection_manager.proto:idle_timeout
but now there's only the field
envoy.deprecated_features.http_connection_manager.proto:hidden_envoy_deprecated_idle_timeout
so my checks for "do I have parity between old deprected fields and my new fully qualified ones" are kinda moot because we just renamed all the things which were deprecated.

v2 is still supported AFIK via proto conversion so I think for this release we mark the "fatal" ones illegal conversions, and then deal with new deprecations after the release?
I'm going to block off some time tomorrow morning to sort out a plan with @htuch

htuch · 2020-01-07T04:17:01Z

Yeah, let's chat tomorrow. The deprecated field renaming is very mechanical, so I feel there must be a way to recover the information you are after here.

This PR makes the following fatal by default: from cluster.proto: ORIGINAL_DST_LB, tls_context, extension_protocol_options from health_check.proto: use_http2 from route_components.proto: allow_origin regex, pattern, method, regex_match, value from http_connection_manager.proto: operation_name from trace.proto: HTTP_JSON_V1 from string.proto: regex Risk Level: Medium (who knows who is using them) Testing: test framework updates Docs Changes: n/a Release Notes: n/a Originally #8847 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

release: flipping flags and fatal-by-defaults

78aff42

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

only doing one thing at a time

a9ac01e

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

alyssawilk changed the title ~~release: flipping flags and fatal-by-defaults~~ release: flipping fatal-by-defaults Nov 1, 2019

mattklein123 requested changes Nov 1, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 1, 2019

mattklein123 self-assigned this Nov 1, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 12, 2019

alyssawilk added the no stalebot Disables stalebot from closing an issue label Nov 13, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Nov 13, 2019

alyssawilk commented Dec 2, 2019

View reviewed changes

alyssawilk mentioned this pull request Dec 2, 2019

tools: removing flag flip tooling #9184

Merged

mattklein123 added waiting and removed no stalebot Disables stalebot from closing an issue waiting labels Dec 3, 2019

cleanup, finally

37c6335

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

repokitteh-read-only bot removed the waiting label Dec 12, 2019

alyssawilk added 2 commits December 12, 2019 12:00

Merge branch 'master' into flips

c67a8f6

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

config fixups

34ddfde

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

mattklein123 requested changes Dec 12, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 12, 2019

reviewer comments

1dc8649

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

repokitteh-read-only bot removed the waiting label Dec 12, 2019

repokitteh-read-only bot added the waiting:any label Dec 13, 2019

repokitteh-read-only bot removed the waiting:any label Dec 16, 2019

repokitteh-read-only bot added the waiting label Dec 17, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 24, 2019

stale bot closed this Dec 31, 2019

alyssawilk mentioned this pull request Jan 6, 2020

runtime: splitting classes into two files #9576

Merged

This was referenced Jan 7, 2020

tooling: restoring runtime fatal-by-defaults and changing to fully qualified #9591

Merged

config: flipping deprecated configs fatal by default #9636

Merged

alyssawilk deleted the flips branch May 18, 2020 14:22

Conversation

alyssawilk commented Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk commented Nov 1, 2019

Uh oh!

alyssawilk commented Nov 1, 2019

Uh oh!

alyssawilk commented Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Nov 1, 2019

Uh oh!

stale bot commented Nov 12, 2019

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Dec 12, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Nov 1, 2019 •

edited

Loading

alyssawilk commented Nov 1, 2019 •

edited

Loading