api: move generic matcher proto to its own package by snowp · Pull Request #17096 · envoyproxy/envoy

snowp · 2021-06-23T02:40:11Z

In order to unblock a dependency between route_components.proto and the matcher tree, move the alpha Matcher to
its own package. To provide an upgrade path for users using this, we'll keep the old copy around for a single deprecation
cycle.

Signed-off-by: Snow Pettersen snowp@lyft.com

Risk Level: Low
Testing: n/a
Docs Changes: n/a
Release Notes: Deprecation note

Signed-off-by: Snow Pettersen <snowp@lyft.com>

repokitteh-read-only · 2021-06-23T02:40:17Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17096 was opened by snowp.

see: more, trace.

snowp · 2021-06-23T02:41:44Z

@htuch This moves the proto as per #17037. I figured we'd provide some kind of upgrade path via supporting both the new and old proto, though let me know if you think we should just do an aggressive move given the alpha status.

htuch

@snowp very exciting to see routing and sub-linear matchers coming together.

One question I have on this is whether we are breaking the circular dependency at the ideal place. The fact that generic matchers depend on HeaderMatcher in router seems wrong, ideally the header matcher would be somewhere generic. OTOH to fix this would require wasting time deprecating the existing location and moving the type for route table, which is very disruptive to existing users (almost everyone using Envoy and HTTP).

Is what you ahve here the least bad you can think of?

markdroth · 2021-06-24T16:16:59Z

While you're at it, can you also update the following doc to indicate an exception for [#alpha:]?

https://github.com/envoyproxy/envoy/blob/main/api/API_VERSIONING.md#backwards-compatibility

Thanks!

Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp · 2021-07-07T14:00:48Z

Is what you ahve here the least bad you can think of?

I think this is the least bad in terms how many people this would be affected by it (only users of the new matching API). I'm also not sure if moving the header matching protos would help without a breaking API change: the protos currently referencing them are not in alpha, so even if we copied them we wouldn't be able to remove the build time dependency on the existing route matching protos since we need to keep the deprecated field around. With this change we can remove the old matching tree proto (immediately? or maybe following a deprecation cycle?) and continue to evolve the new proto without being held back by the API stability policy.

Signed-off-by: Snow Pettersen <snowp@lyft.com>

markdroth · 2021-07-07T18:21:49Z

Another thought: Since we're moving this any way, would it make sense to move it to the cncf/xds repo at this point?

snowp · 2021-07-08T14:37:57Z

@markdroth I'm not opposed to that since I don't think it would be too much work, so happy to do that if we feel like it's the right call. @envoyproxy/api-shepherds thoughts?

snowp · 2021-07-08T14:49:11Z

Also: given the exception we're now giving breaking changes to alpha protos, does it still make sense to provide the migration path that this PR offers at the moment? After looking at it some more, none of the affected protos are referenced by type url (only ExtensionWithMatch is, which isnt changed), so if we were to swap the type of the matcher field to something that looks the same that should not break anything on the wire

Thoughts?

htuch · 2021-07-09T00:19:38Z

I think moving to cncf/xds would make sense as this is a fairly fundamental generic construct. But, don't feel so strongly I'd bike shed a ton.

Migration path makes sense if it helps out in your situation @snowp, otherwise I think it's fine to just make the breaking change in alpha.

snowp · 2021-07-09T01:24:51Z

Looking into what it would take to migrate this to udpa there are two messages outside of the immediate field that would also need to be migrated: TypedExtensionConfig and matcher.v3.StringMatcher. We could add these to udpa as well, though that will involve duck typing more functions (like I've done in this PR via templating) to make them compatible with the identical protos in the Envoy repo vs the udpa repo.

Thoughts? Not sure if this makes things more complicated than they have to be

Signed-off-by: Snow Pettersen <snowp@lyft.com>

…matcher Signed-off-by: Snow Pettersen <snowp@lyft.com>

markdroth

I'd be fine with creating a copy of both TypedExtensionConfig and StringMatcher in the cncf/xds repo.

markdroth · 2021-07-09T14:32:14Z

api/API_VERSIONING.md

    may be granted for scenarios in which these stricter conditions model behavior already implied
    structurally or by documentation.

+* Messages marked as [#alpha:] are excluded from the backwards compatibility guarantees.


Please put this in the bulletted list below, which lists the other items that have an exception.

Signed-off-by: Snow Pettersen <snowp@lyft.com>

repokitteh-read-only · 2021-07-09T19:23:06Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #17096 was synchronize by snowp.

see: more, trace.

Signed-off-by: Snow Pettersen <snowp@lyft.com>

htuch · 2021-08-01T20:54:37Z

Looks like CI failures.
/wait

Signed-off-by: Snow Pettersen <snowp@lyft.com>

moderation · 2021-08-05T21:27:28Z

I think it's lack of any release, but this seems strange it should be the first time since there are other deps in this situation. @moderation @phlax any thoughts?

Seems like the deps issue is resolved and there shouldn't be problems with deps without releases or targeting specific commits.

/lgtm deps

Signed-off-by: Snow Pettersen <snowp@lyft.com>

htuch

LGTM, thanks!

mathetake

LGTM for changes to SPIFFE validator extension

Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp · 2021-08-06T22:39:46Z

Finally passed CI on this on, ptal @htuch

htuch

LGTM, thanks!

* main: (687 commits) ci: set build debug information from env (envoyproxy#17635) ext_authz: do the authentication even the direct response is set (envoyproxy#17546) upstream: various cleanups in connection pool code (envoyproxy#17644) owners: promote Dmitry to maintainer (envoyproxy#17642) quiche: client session supports creating bidi stream (envoyproxy#17543) Update HTTP/2 METADATA documentation. (envoyproxy#17637) ext_proc: Check validity of the :status header (envoyproxy#17596) test: add ASSERT indicating that gRPC stream has not been started yet (envoyproxy#17614) ensure that the inline cookie header will be folded correctly (envoyproxy#17560) cluster_manager: Make ClusterEntry a class instead of a struct (envoyproxy#17616) owners: make Raúl a Thrift senior extension maintainer (envoyproxy#17641) quiche: update QUICHE dependency (envoyproxy#17618) Delete mock for removed RouteEntry::perFilterConfig() method (envoyproxy#17623) REPO_LAYOUT.md: fix outdated link (envoyproxy#17626) hcm: forbid use of detection extensions with use_remote_addr/xff_num_trusted_hops (envoyproxy#17558) thrift proxy: add request shadowing support (envoyproxy#17544) ext_proc: Ensure that timer is always cancelled (envoyproxy#17569) Proposal: Add CachePolicy interface to allow for custom cache behavior (envoyproxy#17362) proto: fix verify to point at v3 only (envoyproxy#17622) api: move generic matcher proto to its own package (envoyproxy#17096) ...

* main: (687 commits) ci: set build debug information from env (envoyproxy#17635) ext_authz: do the authentication even the direct response is set (envoyproxy#17546) upstream: various cleanups in connection pool code (envoyproxy#17644) owners: promote Dmitry to maintainer (envoyproxy#17642) quiche: client session supports creating bidi stream (envoyproxy#17543) Update HTTP/2 METADATA documentation. (envoyproxy#17637) ext_proc: Check validity of the :status header (envoyproxy#17596) test: add ASSERT indicating that gRPC stream has not been started yet (envoyproxy#17614) ensure that the inline cookie header will be folded correctly (envoyproxy#17560) cluster_manager: Make ClusterEntry a class instead of a struct (envoyproxy#17616) owners: make Raúl a Thrift senior extension maintainer (envoyproxy#17641) quiche: update QUICHE dependency (envoyproxy#17618) Delete mock for removed RouteEntry::perFilterConfig() method (envoyproxy#17623) REPO_LAYOUT.md: fix outdated link (envoyproxy#17626) hcm: forbid use of detection extensions with use_remote_addr/xff_num_trusted_hops (envoyproxy#17558) thrift proxy: add request shadowing support (envoyproxy#17544) ext_proc: Ensure that timer is always cancelled (envoyproxy#17569) Proposal: Add CachePolicy interface to allow for custom cache behavior (envoyproxy#17362) proto: fix verify to point at v3 only (envoyproxy#17622) api: move generic matcher proto to its own package (envoyproxy#17096) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

In order to unblock a dependency between route_components.proto and the matcher tree, move the alpha Matcher to its own package. To provide an upgrade path for users using this, we'll keep the old copy around for a single deprecation cycle. Risk Level: Low Testing: n/a Docs Changes: n/a Release Notes: Deprecation note Signed-off-by: Snow Pettersen <snowp@lyft.com>

Snow Pettersen added 2 commits June 23, 2021 02:35

api: move generic matcher proto to its own package

50fe3e8

Signed-off-by: Snow Pettersen <snowp@lyft.com>

deprecation note

81aa2bc

Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp assigned htuch Jun 23, 2021

repokitteh-read-only bot added the api label Jun 23, 2021

htuch reviewed Jun 24, 2021

View reviewed changes

htuch added the waiting label Jun 29, 2021

Merge remote-tracking branch 'envoy/main' into move-generic-matcher

7ff4d44

Signed-off-by: Snow Pettersen <snowp@lyft.com>

repokitteh-read-only bot removed the waiting label Jul 7, 2021

add new file, make factory generic, add test

7acbde0

Signed-off-by: Snow Pettersen <snowp@lyft.com>

Snow Pettersen added 2 commits July 7, 2021 17:06

fix docs

98100c7

Signed-off-by: Snow Pettersen <snowp@lyft.com>

add alpha exception

fd00924

Signed-off-by: Snow Pettersen <snowp@lyft.com>

Snow Pettersen added 2 commits July 9, 2021 03:20

fix examples

28bf418

Signed-off-by: Snow Pettersen <snowp@lyft.com>

Merge branch 'main' of github.com:envoyproxy/envoy into move-generic-…

f4a6292

…matcher Signed-off-by: Snow Pettersen <snowp@lyft.com>

markdroth reviewed Jul 9, 2021

View reviewed changes

use udpa version

a4b752b

Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp requested review from asraa, ggreenway and lizan as code owners July 9, 2021 19:22

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Jul 9, 2021

fix examples

41f07e4

Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp requested a review from jmarantz as a code owner July 27, 2021 15:57

Snow Pettersen added 2 commits July 27, 2021 15:57

Merge remote-tracking branch 'envoy/main' into move-generic-matcher

09c782e

Signed-off-by: Snow Pettersen <snowp@lyft.com>

more fixes

199a8b1

Signed-off-by: Snow Pettersen <snowp@lyft.com>

repokitteh-read-only bot added the waiting label Aug 1, 2021

more build fixes

73e1848

Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp requested review from mathetake and mattklein123 as code owners August 5, 2021 13:31

repokitteh-read-only bot removed the waiting label Aug 5, 2021

Snow Pettersen added 2 commits August 5, 2021 15:21

cleanups, fix composite example

4c9f5e6

Signed-off-by: Snow Pettersen <snowp@lyft.com>

update integration tests

cb006d0

Signed-off-by: Snow Pettersen <snowp@lyft.com>

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Aug 5, 2021

hopefully last test updates

64b2b45

Signed-off-by: Snow Pettersen <snowp@lyft.com>

htuch previously approved these changes Aug 6, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Aug 6, 2021

mathetake previously approved these changes Aug 6, 2021

View reviewed changes

add coveragte

162c85f

Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp dismissed stale reviews from mathetake and htuch via 162c85f August 6, 2021 15:12

repokitteh-read-only bot added the api label Aug 6, 2021

htuch approved these changes Aug 8, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Aug 8, 2021

htuch merged commit 40ed333 into envoyproxy:main Aug 8, 2021

rojkov mentioned this pull request Oct 18, 2021

Envoy is still checking program size of regex #18633

Closed

Conversation

snowp commented Jun 23, 2021

Uh oh!

repokitteh-read-only bot commented Jun 23, 2021

Uh oh!

snowp commented Jun 23, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth commented Jun 24, 2021

Uh oh!

snowp commented Jul 7, 2021

Uh oh!

markdroth commented Jul 7, 2021

Uh oh!

snowp commented Jul 8, 2021

Uh oh!

snowp commented Jul 8, 2021

Uh oh!

htuch commented Jul 9, 2021

Uh oh!

snowp commented Jul 9, 2021

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Jul 9, 2021

Uh oh!

htuch commented Aug 1, 2021

Uh oh!

moderation commented Aug 5, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

mathetake left a comment

Choose a reason for hiding this comment

Uh oh!

snowp commented Aug 6, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants