[envoy-15622] add generic XDS protobuf definitions #15760

su225 · 2021-03-30T08:19:54Z

TODO

Refine protobuf messages and add it to different versions of the API
Add and refine inline documentation

Commit Message: Enhanced CSDS API to support reporting status for generic xDS resource types
Additional Description:
Risk Level: Low
Docs Changes: Inline protobuf docs (not done yet)
Release Notes: Enhanced CSDS API to support reporting status for generic xDS resource types
[Optional Fixes #15622]

repokitteh-read-only · 2021-03-30T08:19:56Z

Hi @su225, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #15760 was opened by su225.

see: more, trace.

repokitteh-read-only · 2021-03-30T08:20:01Z

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

🐱

Caused by: #15760 was opened by su225.

see: more, trace.

mattklein123 · 2021-03-30T19:02:47Z

Can you provide some background context on this PR? Thank you.

/wait

api/envoy/admin/v3/config_dump.proto

api/envoy/service/status/v3/csds.proto

markdroth · 2021-03-30T19:13:33Z

api/envoy/admin/v3/config_dump.proto

+    UpdateFailureState error_state = 6;
+  }
+
+  string type_url = 1;


Matt and others can also comment on this, but I'd ideally like to see us move to a place where we use this simple structure for all resources, regardless of type. If we do that, then there isn't necessarily any reason to have a separate GenericXdsConfigDump message for each resource type; we could instead just send all resources in a single GenericXdsConfigDump message.

Note that once we move to new-style resource names, there's no wire-size savings to be had here by putting each resource type into its own message, because the resource type will be part of the individual resource names anyway.

api/envoy/admin/v3/config_dump.proto

api/envoy/service/status/v3/csds.proto

api/envoy/admin/v3/config_dump.proto

api/envoy/service/status/v3/csds.proto

su225 · 2021-04-08T18:13:48Z

Can you provide some background context on this PR? Thank you.

Described in #15622 . Some use cases

As Envoy adds more xDS config types, the API would get more complex to support new types. Some like ECDS don't have support at the moment.
There are some projects like Istio which also have additional xDS types like NDS (Name Discovery Service), PCDS (Proxy Config Discovery Service). Technically "istio-agent+envoy" forms the dataplane in Istio. From the point of view of other UDPA implementations, it makes sense to define their own xDS types for their specific usecase without having to upstream to Envoy, but still use the same kind of API. This is helpful to use it with generic UDPA related tooling.

lizan

@markdroth any further comment or does this look good to you? I need a non-tetrands review.

api/envoy/service/status/v3/csds.proto

…t docs yet) Signed-off-by: su225 <[email protected]>

Signed-off-by: su225 <[email protected]>

markdroth · 2021-05-19T18:21:03Z

I like this direction, but I'd like to get input from folks like @htuch, @mattklein123, @fuqianggao, and @lidizheng to make sure that we're all bought-in on the idea of using a single representation for all resource types.

lidizheng

I like this change. This makes both assembling and parsing the CSDS response much simpler.

One question I have is for static resources. Admin config dump differentiate static resources given by yaml file and dynamic resources updated by control plane. Should we send static resources as GenericXdsConfig as well?

Also, for ECDS, are we going to expose them via the normal HTTP config dump page?

api/envoy/service/status/v3/csds.proto

fuqianggao

High level idea LGTM

fuqianggao · 2021-05-20T05:59:26Z

api/envoy/service/status/v3/csds.proto

+    string type_url = 1;
+
+    // name of the xDS resource
+    string name = 2;


does this mean that this represent a single resource? I am a bit concerned that the resources are not grouped by resource type, might have worse UX.

Yes, it represents a single resource.

In practice, I suspect that most implementations will send all resources of the same type next to each other anyway, simply because that's the obvious way to implement CSDS. But you're right that there is no requirement to do that here.

Keep in mind, though, that even if the server doesn't group those resources together, the client can always group them that way for display purposes. I think the API and the UX can be handled independently.

mattklein123 · 2021-05-21T03:28:48Z

+1 I like this direction. Can we just deprecate the old way and use the new way as the only supported way?

mattklein123 · 2021-05-21T03:31:07Z

Oops nevermind I see you did deprecate it. +1

markdroth · 2021-05-21T15:49:21Z

One question I have is for static resources. Admin config dump differentiate static resources given by yaml file and dynamic resources updated by control plane. Should we send static resources as GenericXdsConfig as well?

Good question. Do we care about the distinction between static and dynamic resources from the perspective of CSDS?

The Envoy folks will need to weigh in on this. gRPC doesn't support static resources, so we don't need this.

If this distinction is important, we could add a bool to the message indicating that the resource is static. Would that be sufficient?

htuch · 2021-05-23T12:58:22Z

I think it makes sense to include static, since this is the experience that folks working with the admin interface are used to. I agree with adding a bool (or enum?) to distinguish.

Signed-off-by: su225 <[email protected]>

markdroth · 2021-05-24T14:54:17Z

/lgtm api

Signed-off-by: su225 <[email protected]>

su225 · 2021-05-26T18:07:46Z

@htuch - Added the flag.

api/envoy/service/status/v3/csds.proto

htuch · 2021-05-28T03:30:33Z

Thanks. @fuqianggao can you take another pass?

fuqianggao

/lgtm

api/envoy/service/status/v3/csds.proto

repokitteh-read-only · 2021-05-28T21:33:06Z

please specify a single label can be specified

🐱

Caused by: a #15760 (review) was submitted by @fuqianggao.

see: more, trace.

Signed-off-by: su225 <[email protected]>

repokitteh-read-only bot added the api label Mar 30, 2021

repokitteh-read-only bot assigned mattklein123 Mar 30, 2021

su225 force-pushed the 15622-generic-csds-api branch from 907fa8c to 45183a7 Compare March 30, 2021 08:21

This was referenced Mar 30, 2021

Support generic CSDS types #15622

Closed

[WIP] enhance statusgen and proxy-status with last sync time and additional XDS statuses istio/istio#31595

Closed

repokitteh-read-only bot added the waiting label Mar 30, 2021

markdroth reviewed Mar 30, 2021

View reviewed changes

lizan suggested changes Mar 30, 2021

View reviewed changes

api/envoy/admin/v3/config_dump.proto Outdated Show resolved Hide resolved

api/envoy/admin/v3/config_dump.proto Outdated Show resolved Hide resolved

api/envoy/service/status/v3/csds.proto Outdated Show resolved Hide resolved

repokitteh-read-only bot removed the waiting label Apr 8, 2021

su225 force-pushed the 15622-generic-csds-api branch from aee3fb1 to 46c2b3d Compare April 8, 2021 17:45

su225 requested a review from lizan April 8, 2021 18:14

mattklein123 added the waiting label Apr 8, 2021

repokitteh-read-only bot removed the waiting label Apr 15, 2021

mattklein123 added the waiting label Apr 16, 2021

su225 force-pushed the 15622-generic-csds-api branch from f13559d to 8f4eeab Compare May 2, 2021 12:28

repokitteh-read-only bot removed the waiting label May 2, 2021

su225 changed the title ~~[envoy-15622][WIP] add generic XDS protobuf definitions~~ [envoy-15622] add generic XDS protobuf definitions May 2, 2021

su225 requested a review from markdroth May 13, 2021 13:59

lizan reviewed May 19, 2021

View reviewed changes

api/envoy/service/status/v3/csds.proto Show resolved Hide resolved

su225 requested a review from lizan May 19, 2021 08:40

su225 added 2 commits May 19, 2021 22:49

[envoy-15622] add generic XDS protobuf definitions for v3 API (withou…

484fcd7

…t docs yet) Signed-off-by: su225 <[email protected]>

[envoy-15622] add description for generic_xds_configs field

7097451

Signed-off-by: su225 <[email protected]>

su225 force-pushed the 15622-generic-csds-api branch from 453e846 to 7097451 Compare May 19, 2021 18:14

lidizheng reviewed May 19, 2021

View reviewed changes

api/envoy/service/status/v3/csds.proto Outdated Show resolved Hide resolved

fuqianggao reviewed May 20, 2021

View reviewed changes

add is_static_resource flag from code review comment

972f19c

Signed-off-by: su225 <[email protected]>

repokitteh-read-only bot removed the api label May 24, 2021

fix documentation comment

c7cd32d

Signed-off-by: su225 <[email protected]>

repokitteh-read-only bot added the api label May 24, 2021

[envoy-15622] fix formatting

a245ec4

Signed-off-by: su225 <[email protected]>

mattklein123 assigned markdroth May 26, 2021

mattklein123 added the waiting label May 26, 2021

fix formatting and docs

96244e4

Signed-off-by: su225 <[email protected]>

repokitteh-read-only bot removed the waiting label May 26, 2021

htuch reviewed May 28, 2021

View reviewed changes

api/envoy/service/status/v3/csds.proto Outdated Show resolved Hide resolved

fuqianggao reviewed May 28, 2021

View reviewed changes

api/envoy/service/status/v3/csds.proto Outdated Show resolved Hide resolved

su225 added 2 commits May 30, 2021 12:06

address code review feedback - fix documentation

361fbb5

Signed-off-by: su225 <[email protected]>

fix documentation

cf7aa96

Signed-off-by: su225 <[email protected]>

su225 requested a review from htuch May 30, 2021 14:48

mattklein123 approved these changes Jun 2, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jun 2, 2021

mattklein123 merged commit 53d8c9d into envoyproxy:main Jun 2, 2021

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

[envoy-15622] add generic XDS protobuf definitions (envoyproxy#15760)

1739086

Signed-off-by: su225 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[envoy-15622] add generic XDS protobuf definitions #15760

[envoy-15622] add generic XDS protobuf definitions #15760

su225 commented Mar 30, 2021 •

edited

Loading

repokitteh-read-only bot commented Mar 30, 2021

repokitteh-read-only bot commented Mar 30, 2021

mattklein123 commented Mar 30, 2021

markdroth Mar 30, 2021

su225 commented Apr 8, 2021

lizan left a comment

markdroth commented May 19, 2021

lidizheng left a comment

fuqianggao left a comment

fuqianggao May 20, 2021 •

edited

Loading

markdroth May 21, 2021

fuqianggao May 25, 2021

mattklein123 commented May 21, 2021

mattklein123 commented May 21, 2021

markdroth commented May 21, 2021

htuch commented May 23, 2021

markdroth commented May 24, 2021

su225 commented May 26, 2021

htuch commented May 28, 2021

fuqianggao left a comment

repokitteh-read-only bot commented May 28, 2021

[envoy-15622] add generic XDS protobuf definitions #15760

[envoy-15622] add generic XDS protobuf definitions #15760

Conversation

su225 commented Mar 30, 2021 • edited Loading

repokitteh-read-only bot commented Mar 30, 2021

repokitteh-read-only bot commented Mar 30, 2021

mattklein123 commented Mar 30, 2021

markdroth Mar 30, 2021

Choose a reason for hiding this comment

su225 commented Apr 8, 2021

lizan left a comment

Choose a reason for hiding this comment

markdroth commented May 19, 2021

lidizheng left a comment

Choose a reason for hiding this comment

fuqianggao left a comment

Choose a reason for hiding this comment

fuqianggao May 20, 2021 • edited Loading

Choose a reason for hiding this comment

markdroth May 21, 2021

Choose a reason for hiding this comment

fuqianggao May 25, 2021

Choose a reason for hiding this comment

mattklein123 commented May 21, 2021

mattklein123 commented May 21, 2021

markdroth commented May 21, 2021

htuch commented May 23, 2021

markdroth commented May 24, 2021

su225 commented May 26, 2021

htuch commented May 28, 2021

fuqianggao left a comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented May 28, 2021

su225 commented Mar 30, 2021 •

edited

Loading

fuqianggao May 20, 2021 •

edited

Loading