tooling: restoring runtime fatal-by-defaults and changing to fully qualified by alyssawilk · Pull Request #9591 · envoyproxy/envoy

alyssawilk · 2020-01-07T21:29:46Z

Changing from relative name to absolute name, and fixing the fatal-by-defaults that were broken by the v3 switch.

The old way to allow fatal-by-defaults was
envoy.deprecated_features:proto_file.proto:field_name
the new way is
envoy.deprecated_features:full.namespace.field_name

When we switched to v3, all the hard-coded v2 names stopped working. This reinstates them via hopefully more permanent proto annotation.

The only remaining ugly bit is that unfortunately the full namespace and field name are the v3 versions even if the original config was v2. Between @htuch and I we should fix that before merging.

Risk Level: Medium
Testing: added new unit tests
Docs Changes: updated
Release Notes: n/a

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

repokitteh-read-only · 2020-01-07T21:29:52Z

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

🐱

Caused by: #9591 was opened by alyssawilk.

see: more, trace.

mattklein123 · 2020-01-07T22:01:32Z

api/envoy/annotations/deprecation.proto

+import "google/protobuf/descriptor.proto";
+
+// Magic number in this file derived from top 28bit of SHA256 digest of "envoy.annotation.do_not_use"
+extend google.protobuf.FieldOptions {


Do you mind adding some comments about what these annotations do? I'm a little confused what the proposed flow is?

htuch · 2020-01-08T04:37:02Z

api/envoy/annotations/deprecation.proto

+
+// Magic number in this file derived from top 28bit of SHA256 digest of "envoy.annotation.do_not_use_enum"
+extend google.protobuf.EnumValueOptions {
+  bool do_not_use_enum = 162744140;


I think you can also call this do_not_use

I don't think I can.
Things defined in the extend field don't get namespace prefixed. If you notice in the utility file, they're
envoy::annotations::disallowed_by_default_enum
and envoy::annotations::disallowed_by_default
and they conflict if they are both disallowed_by_default

htuch · 2020-01-08T04:39:01Z

api/envoy/annotations/deprecation.proto

+import "google/protobuf/descriptor.proto";
+
+// Magic number in this file derived from top 28bit of SHA256 digest of "envoy.annotation.do_not_use"
+extend google.protobuf.FieldOptions {


One other interesting thing here is we're getting awfully close to being able to either encode the runtime key here as an annotation, or via reflection to being able to derive it. Not sure where the status quo there is today. Something worth thinking about, even if it's beyond the scope of this PR.

htuch · 2020-01-08T04:40:12Z

source/common/protobuf/utility.cc

    bool warn_only = false;
 #else
-    bool warn_only = true;
+    bool warn_only = field->options().GetExtension(envoy::annotations::do_not_use) == false;


Nit: ! rather than ==false

htuch

No strong opinion on 1, 2 or 3, other than to not violate our existing guidance (so maybe 3 is out).

htuch · 2020-01-08T04:44:34Z

api/envoy/annotations/deprecation.proto

+
+// Magic number in this file derived from top 28bit of SHA256 digest of "envoy.annotation.do_not_use"
+extend google.protobuf.FieldOptions {
+  bool do_not_use = 169129351;


Bike shedding: prefer to call this disallowed_by_default.

htuch · 2020-01-08T04:46:16Z

test/proto/deprecated.proto

  string not_deprecated = 1;
  string is_deprecated = 2 [deprecated = true];
-  string is_deprecated_fatal = 3 [deprecated = true];
+  string is_deprecated_fatal = 3 [deprecated = true, (envoy.annotations.do_not_use) = true];


So, IIUC, your tooling or manual deprecations will set this on v2. As long as protoxform doesn't delete them (which it might today, ping me tomorrow on how to avoid this), we're good for v3, since the field doesn't exist there.

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

alyssawilk · 2020-01-08T18:20:58Z

I think I addressed reviewer comments.
Still trying to figure out how to propagate the new tags with the proto tools so I can actually annotate things. Prior attempts were wiped out by fix_format :-(

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

mattklein123 · 2020-01-08T20:25:56Z

Thanks this makes sense to me at a high level. Agree either (1) or (2) SGTM.

alyssawilk · 2020-01-08T20:38:53Z

One more meta comment, I think if we're fully qualifying I'm inclined to drop the filename from the name.
When converting things over it turns out at least one thing has changed file locations. I doubt it happens often but if we do that in future we'd break folks' guards.

mattklein123 · 2020-01-08T20:41:38Z

I think removing filename is reasonable.

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

alyssawilk · 2020-01-08T21:14:19Z

UGH, we have yet another problem.
Because of the way we convert protos, our warning and disabling is now based on whatever Envoy head thinks it's using

so now that we're fully qualifying the v2 test for the deprecated field "runtime key" complains that you're using the deprecated
envoy.deprecated_features:envoy.config.route.v3alpha.RouteAction.RequestMirrorPolicy.hidden_envoy_deprecated_runtime_key

which is pretty terrible, because every time we api upgrade we'll break folks runtime guards

paging in @htuch - even if we dropped the bit where we fully qualify, the field rename from foo to hidden_envoy_foo is pretty crummy.

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

alyssawilk · 2020-01-08T22:05:37Z

Ok, a bit more context for @htuch

This PR is almost good to go.
The problem I'm encountering is visible in test/common/router/config_impl_test.cc
the test sends a v2 API proto, it hits source/common/protobuf/utility.cc
That determines that the v2 field "runtime_key" is no longer allowed and unhelpfully informs the user that they're using envoy.deprecated_features:envoy.config.route.v3alpha.RouteAction.RequestMirrorPolicy.hidden_envoy_deprecated_runtime_key
What would be good is if in checkForUnexpectedFields rather than use
absl::StrCat"envoy.deprecated_features:", field->full_name()),
I could swap full_name for full_original_name per the oracle we discussed.

htuch · 2020-01-08T22:13:03Z

@alyssawilk tracking the API oracle work at #9612

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

htuch

LGTM once the stars align. Thanks.

htuch · 2020-01-09T18:59:59Z

docs/root/configuration/operations/runtime.rst

-and use of that configuration field will cause the config to be rejected by default. 
-This fail-by-default mode can be overridden in runtime configuration by setting
-envoy.deprecated_features.filename.proto:fieldname or envoy.deprecated_features.filename.proto:enum_value
+In the second phase the field will be tagged as disallowed_by_default


Nit: *disallowed_by_default*.

source/common/runtime/runtime_features.cc

alyssawilk · 2020-01-14T18:19:23Z

Hm, so with @htuch's latest merged, I'm now getting runtime failures unless we flag both the v2 paht and v3 alpha path as OK.
Looking into it....

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

alyssawilk · 2020-01-14T19:16:53Z

test/extensions/filters/network/redis_proxy/config_test.cc

+  Runtime::LoaderSingleton::getExisting()->mergeValues(
+      {{"envoy.deprecated_features:envoy.extensions.filters.network.redis_proxy.v3alpha.RedisProxy."
+        "PrefixRoutes.hidden_envoy_deprecated_cluster",
+        "true"}});


This weirdness is because we're loading deprecated v2 config with a v3 proto. Arguably we should have left the deprecated feature test using a v2 proto rather than envoy::extensions::filters::network::redis_proxy::v3alpha::RedisProxy
Can fix if we care but I'm pretty convinced we don't require both for v3 by the intergration tests not neeting it.

Probably fine for this unit test, we tend to make v3alpha everywhere for unit tests.

alyssawilk · 2020-01-14T19:18:21Z

test/integration/server.cc

-  Runtime::RuntimeFeaturesPeer::setAllFeaturesAllowed();
+  // TODO(alyssawilk) improve this.
+  Runtime::LoaderSingleton::getExisting()->mergeValues(
+      {{"envoy.deprecated_features:envoy.config.route.v3alpha.CorsPolicy."


I had wanted the PoC PR to show that an integration test using v2 config would succeed with a v2 override.
Unfortunately we switched the integration test framework to v3alpha so we don't have that.

Do we think it's worth doing? My concern is otherwise there might be somewhere in the upgrade protos + downgrade protos pipeline that loses the v2ness, and we'll end up needing double validation as we do with the redis test

@alyssawilk you could try and force the integration test config down to v2 with API_DOWNGRADE macro before passing to Envoy. That would give you a v2 path without changing any code (well, one line :)

FWIW, we have tended to try and force v2 for the xDS aspects of the integration tests with a similar trick, but didn't do this for boostrap. I think it would be a good thing TBH to have the integration tests be focused on v2 external artifacts today.

Yeah, as discussed a forced downgrade before we write files to disk fails because the HCM is typed config and we don't have utils for that.

Manual testing looks good. Diff patch for trying bad cors config below, and it gets me whining about v2 CorsPolicy rather than v3. Forcing runtime to accept "envoy.deprecated_features:envoy.api.v2.route.CorsPolicy.enabled" results in the failure turning back to a warning

[2020-01-14 17:12:55.235][90650][warning][misc] [source/common/protobuf/utility.cc:423] Using deprecated option 'envoy.api.v2.route.CorsPolicy.allow_origin' from file route_components.proto. This configuration will be removed from Envoy soon. Please see https://www.envoyproxy.io/docs/envoy/latest/intro/deprecated for details.
[2020-01-14 17:12:55.235][90650][critical][main] [source/server/server.cc:94] error initializing configuration 'configs/google_com_proxy.v2.yaml': Proto constraint validation failed (Using deprecated option 'envoy.api.v2.route.CorsPolicy.enabled' from file route_components.proto. This configuration will be removed from Envoy soon. Please see https://www.envoyproxy.io/docs/envoy/latest/intro/deprecated for details. If continued use of this field is absolutely necessary, see https://www.envoyproxy.io/docs/envoy/latest/configuration/operations/runtime#using-runtime-overrides-for-deprecated-features for how to apply a temporary and highly discouraged override.):

--- a/configs/google_com_proxy.v2.yaml
+++ b/configs/google_com_proxy.v2.yaml
@@ -30,6 +30,13 @@ static_resources:
route:
host_rewrite: www.google.com
cluster: service_google

cors:

enabled: {}

allow_origin: "test-origin-1"

allow_origin: "test-host-2"

allow_methods: "POST"

allow_headers: "content-type"

max_age: "100" http_filters: - name: envoy.router
clusters:
diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc
index f10d053b89..24818e60a4 100644
--- a/source/common/runtime/runtime_impl.cc

Ack, thanks for the verification. We have a reasonable test in protobuf/utility.cc that validates we are doing cross-version deprecation checks (added in my last PR), so I'm reasonably confident based on that this PR is solid.

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

alyssawilk · 2020-01-14T23:03:20Z

/azp run

azure-pipelines · 2020-01-14T23:03:38Z

Azure Pipelines successfully started running 3 pipeline(s).

htuch

LGTM, thanks!

htuch · 2020-01-14T23:29:06Z

api/envoy/api/v2/core/config_source.proto

    // Ideally this would be 'reserved 0' but one can't reserve the default
    // value. Instead we throw an exception if this is ever used.
-    UNSUPPORTED_REST_LEGACY = 0 [deprecated = true];
+    UNSUPPORTED_REST_LEGACY = 0


I'm kind of surprised this wasn't disabled earlier, we have zero support for this AFAIK.

It was disabled, just the old way via the runtime strings :-)

tooling: restoring runtime fatal-by-defaults and changing to fully qu…

d5a261f

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

alyssawilk assigned htuch Jan 7, 2020

repokitteh-read-only bot added the api label Jan 7, 2020

mattklein123 reviewed Jan 7, 2020

View reviewed changes

htuch reviewed Jan 8, 2020

View reviewed changes

alyssawilk added 2 commits January 8, 2020 12:23

reviewer comments

be47f9f

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

fixing fix format, probably breaking docs build.

6d35c10

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

alyssawilk added the waiting label Jan 8, 2020

while(1){python--;}

aea3988

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

alyssawilk added 2 commits January 8, 2020 15:45

redeprecating all the things

257e930

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

removing filenames and cleanup

3b28ac1

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

naming is hard

97c7304

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

repokitteh-read-only bot removed the waiting label Jan 8, 2020

htuch mentioned this pull request Jan 8, 2020

Track original type URL in upgraded protos #9612

Closed

alyssawilk added 2 commits January 8, 2020 17:24

finally, tests passing

23b37e5

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

more cleanup. still wondering about the docs build

ed0dd5e

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

alyssawilk marked this pull request as ready for review January 8, 2020 22:31

format

0875430

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

alyssawilk mentioned this pull request Jan 9, 2020

config: flipping deprecated configs fatal by default #9636

Merged

htuch suggested changes Jan 9, 2020

View reviewed changes

alyssawilk added the waiting label Jan 14, 2020

Merge branch 'refs/heads/master' into flips_new

b748791

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

repokitteh-read-only bot removed the waiting label Jan 14, 2020

alyssawilk added the waiting label Jan 14, 2020

alyssawilk commented Jan 14, 2020

View reviewed changes

alyssawilk removed the waiting label Jan 14, 2020

test fix

d52433d

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

alyssawilk force-pushed the flips_new branch from 9657d80 to d52433d Compare January 14, 2020 20:21

htuch approved these changes Jan 14, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jan 14, 2020

alyssawilk merged commit 156d7c9 into envoyproxy:master Jan 15, 2020

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

Conversation

alyssawilk commented Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jan 7, 2020

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!

htuch 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 Jan 8, 2020

Uh oh!

mattklein123 commented Jan 8, 2020

Uh oh!

alyssawilk commented Jan 8, 2020

Uh oh!

mattklein123 commented Jan 8, 2020

Uh oh!

alyssawilk commented Jan 8, 2020

Uh oh!

alyssawilk commented Jan 8, 2020

Uh oh!

htuch commented Jan 8, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alyssawilk commented Jan 14, 2020

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!

alyssawilk commented Jan 14, 2020

Uh oh!

azure-pipelines bot commented Jan 14, 2020

Uh oh!

htuch 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alyssawilk commented Jan 7, 2020 •

edited

Loading