copy matcher proto and dependencies from Envoy #8

snowp · 2021-07-09T15:08:53Z

Moves the Matcher proto into the udpa repo along with a few dependencies: TypedExtensionConfig, StringMatcher and RegexMatcher.

Signed-off-by: Snow Pettersen <[email protected]>

snowp · 2021-07-09T15:09:34Z

@htuch @markdroth

Signed-off-by: Snow Pettersen <[email protected]>

markdroth

Thanks for doing this!

These files should all be in the xds tree, not the udpa tree. We're moving away from udpa.

markdroth · 2021-07-09T21:39:27Z

udpa/extension/v1/extension.proto

@@ -0,0 +1,24 @@
+syntax = "proto3";
+
+package udpa.extension.v1;


Should TypedExtensionConfig be in the xds.extension tree? Or should it be in xds.core? This isn't itself an extension; it's really a type used to point at an extension, which is a capability that is basically built into core.

htuch · 2021-07-12T00:38:43Z

Nice, thanks! Yep, agree that this should be in the xds/ tree and TypedExtensionConfig should be core.

Signed-off-by: Snow Pettersen <[email protected]>

snowp · 2021-07-14T12:36:36Z

@htuch @markdroth ptal, i made the moves

Also FYI I dropped reserved fields, deprecated fields, udpa annotations, etc. when copying these over

htuch

LGTM, will defer to Mark before merge. I think we need to port protoformat over to this repo at some point @phlax.

markdroth

Just one potential issue about the java package names.

markdroth · 2021-07-16T15:32:16Z

xds/core/v3/extension.proto

+  string name = 1 [(validate.rules).string = {min_len: 1}];
+
+  // The typed config for the extension. The type URL will be used to identify
+  // the extension. In the case that the type URL is *udpa.type.v1.TypedStruct*,


@htuch We'll need to remember to update this comment to use xds.type.v1.TypedStruct when we copy files as part of addressing #2.

markdroth · 2021-07-16T15:48:53Z

xds/core/v3/extension.proto

+
+option java_outer_classname = "ExtensionProto";
+option java_multiple_files = true;
+option java_package = "com.github.udpa.xds.core.v1";


Hmm, why is "udpa" part of this string? Looks like that's the case for the existing protos too. @htuch, is this something we can fix without breaking the API? We should probably try to take care of this as part of #2.

Also, this string should say "v3" instead of "v1".

It would break the generated Java code, so we're extending API compatibility to generated code I don't think we can change it?

Fixed the version string

But this is lift-and-shift across repos, so we don't have strict compatibility requirements, for API or code?

I think for existing protos we can take this on a case-by-case basis. gRPC folks are probably best positioned to know the Java implications for the various protos (ORCA, TypedStruct); the validation proto cases are not an issue IMHO.

Sure we could change the package of this new file to be com.github.xds.core.v1, but won't that make this inconsistent with the other packages in here? I was thinking about the idea of changing the java_package of all the protos in xds/ to not include udpa, which affects more than just this file

I think we should do the following:

Decide what we want the java package path to be for all of the xds protos.

Use the new path for these new files.

Decide on a plan to fix the existing files. If we can just change them without breaking anything, we should do that; otherwise, we need to come up with an alternative deprecate-and-migrate plan.

@ejona86 may have some thoughts on how this affects things for Java.

What is the migration process expected for this proto? It has a different package name and file path, so it seems like a brand new proto from protobuf's perspective. In that case, it is a new proto with no prior usages and the Java path can be anything but io.envoyproxy.envoy.config.core.v3 (to avoid colliding with the other copy).

Mark spoke to me and made clear the issue is the existing files.

The existing files are already included in grpc-java, however, there aren't any references to the messages outside of generated code. That means that as long as the consuming protos are generated in the same build process as these protos we'd be fine. That is true for Google-internal and, since grpc-java is not using packages from maven-central but instead building and shading the generated code in grpc-xds. it would be true for the github builds of grpc-java as well.

So it seems fine to change these package names from grpc-java's perspective.

Okay, super, I'll send a separate PR to update the existing files.

Should we use com.github.cncf.xds instead of just com.github.xds, to avoid potential conflicts with some other github <something>/xds repo?

Actually, nevermind that last question -- we're already using xds as the top-level proto package, so com.github.xds is probably fine.

Sent #9 with the changes to the existing files.

Signed-off-by: Snow Pettersen <[email protected]>

snowp · 2021-07-19T17:13:53Z

Sorry about all the force pushes, I need to get my aliases right so I sign commits in repos without the pre-commit hooks :)

htuch

LGTM, thanks, will defer to Mark for final approval.

markdroth

Looks great! We can follow up on fixing the java package path on the existing files separately.

markdroth · 2021-07-21T16:31:10Z

Harvey, I don't have merge access on this repo, so you'll need to do that.

htuch · 2021-07-21T18:13:45Z

@markdroth done, I've fixed it so you should be able to merge in the future.

As discussed in #8. Signed-off-by: Mark D. Roth <[email protected]>

add matcher proto

b5c7961

Signed-off-by: Snow Pettersen <[email protected]>

snowp force-pushed the matcher branch from c63a3d0 to b5c7961 Compare July 9, 2021 15:09

fixes, remove unused messsage

2e5c007

Signed-off-by: Snow Pettersen <[email protected]>

markdroth requested changes Jul 9, 2021

View reviewed changes

markdroth mentioned this pull request Jul 9, 2021

http: add StringMatcher in HeaderMatcher and deprecate the old fields (exact, prefix, etc.) envoyproxy/envoy#17119

Merged

feedback

3e17b7a

Signed-off-by: Snow Pettersen <[email protected]>

snowp force-pushed the matcher branch from c82e0cd to 3e17b7a Compare July 14, 2021 12:11

snowp added 2 commits July 14, 2021 12:12

format

c7ec161

Signed-off-by: Snow Pettersen <[email protected]>

compile go protos

b4d4f75

Signed-off-by: Snow Pettersen <[email protected]>

htuch previously approved these changes Jul 15, 2021

View reviewed changes

markdroth reviewed Jul 16, 2021

View reviewed changes

snowp dismissed htuch’s stale review via bf6bd12 July 16, 2021 18:08

update java package name

216ef14

Signed-off-by: Snow Pettersen <[email protected]>

snowp force-pushed the matcher branch from bf6bd12 to 216ef14 Compare July 16, 2021 18:13

remove udpa from java namespace

14a8e63

Signed-off-by: Snow Pettersen <[email protected]>

snowp force-pushed the matcher branch from aa85d77 to 14a8e63 Compare July 19, 2021 17:07

htuch approved these changes Jul 21, 2021

View reviewed changes

markdroth approved these changes Jul 21, 2021

View reviewed changes

markdroth mentioned this pull request Jul 21, 2021

remove "udpa." from java package paths #9

Merged

htuch merged commit a8536b9 into cncf:main Jul 21, 2021

htuch pushed a commit that referenced this pull request Jul 22, 2021

remote "udpa." from java package paths (#9)

dd25fe8

As discussed in #8. Signed-off-by: Mark D. Roth <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

copy matcher proto and dependencies from Envoy #8

copy matcher proto and dependencies from Envoy #8

snowp commented Jul 9, 2021

snowp commented Jul 9, 2021

markdroth left a comment

markdroth Jul 9, 2021

htuch commented Jul 12, 2021

snowp commented Jul 14, 2021

htuch left a comment

markdroth left a comment

markdroth Jul 16, 2021

htuch Jul 19, 2021

markdroth Jul 16, 2021

snowp Jul 16, 2021

htuch Jul 19, 2021 •

edited

Loading

snowp Jul 19, 2021

markdroth Jul 19, 2021

ejona86 Jul 21, 2021

ejona86 Jul 21, 2021

markdroth Jul 21, 2021

markdroth Jul 21, 2021

markdroth Jul 21, 2021

snowp commented Jul 19, 2021

htuch left a comment

markdroth left a comment

markdroth commented Jul 21, 2021

htuch commented Jul 21, 2021

		@@ -0,0 +1,24 @@
		syntax = "proto3";

		package udpa.extension.v1;

copy matcher proto and dependencies from Envoy #8

copy matcher proto and dependencies from Envoy #8

Conversation

snowp commented Jul 9, 2021

snowp commented Jul 9, 2021

markdroth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Jul 12, 2021

snowp commented Jul 14, 2021

htuch left a comment

Choose a reason for hiding this comment

markdroth 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

htuch Jul 19, 2021 • edited Loading

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

snowp commented Jul 19, 2021

htuch left a comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

markdroth commented Jul 21, 2021

htuch commented Jul 21, 2021

htuch Jul 19, 2021 •

edited

Loading