Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Client Status Discovery Service (CSDS) API definition #9383

Merged
merged 51 commits into from
Jan 22, 2020

Conversation

fuqianggao
Copy link
Contributor

Add Client Status Discovery Service (CSDS) API definition. This can be used by debug tools to obtain config information for specific clients from control plane.

Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
@repokitteh-read-only
Copy link

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

🐱

Caused by: #9383 was opened by fuqianggao.

see: more, trace.

Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
@jmarantz
Copy link
Contributor

@fuqianggao can you look at CI?

Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
@fuqianggao
Copy link
Contributor Author

fuqianggao commented Dec 18, 2019

CI is passing. htuch@ PTAL.

@mattklein123 mattklein123 self-assigned this Dec 19, 2019
@mattklein123
Copy link
Member

@fuqianggao thanks for this. This is interesting. To help with review, can you provide some more high level context on rationale, etc.? Thank you.

/wait-any

@fuqianggao
Copy link
Contributor Author

@mattklein123 The purpose of this API is to provide a way for control plane to expose its view on the status of clients connected to it. For example, in a service mesh with many Envoys, a standalone debug tool can connect to control plane and stream CSDS, to know which Envoys are actually connected to the control plane and what configs they are getting. This will be very helpful for debugging purpose. Istio has a similar command line tool: https://istio.io/docs/ops/diagnostic-tools/proxy-cmd/, I imagine they can possibly use CSDS. Besides configs, it's also possible to include other stats in CSDS, but there are other means to expose those (e.g. stackdriver).

@mattklein123
Copy link
Member

@fuqianggao agreed this is a very useful thing to add, thanks for the context. I'm tagging a bunch of people at Lyft who will be interested in this as we are looking at building out some behavior along these lines in a management tool.

cc @danielhochman @ryancox @sindhura

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattklein123 FWIW, I've already done some internal design discussion with @fuqianggao on this, definitely valuable to see what other orgs and potential users think.

api/envoy/service/discovery/v2/csds.proto Outdated Show resolved Hide resolved
api/envoy/service/discovery/v2/csds.proto Outdated Show resolved Hide resolved
Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool stuff. Generally LGTM w/ one question.

api/envoy/service/discovery/v2/csds.proto Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Dec 30, 2019

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 stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 30, 2019
Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 2, 2020
@fuqianggao
Copy link
Contributor Author

fuqianggao commented Jan 18, 2020

@htuch @mattklein123 Made a minor change: make MetadataMatcher inside NodeMatcher repeated to enable match on multiple metadata fields. PTAL again. Thanks!

StringMatcher node_id = 1;

// Specifies match criteria on the node metadata.
repeated MetadataMatcher node_metadatas = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this AND semantics? Might it be the case that someone one day wants OR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is intended to be AND. If OR is needed, one can have repeated NodeMatcher, and only include either node_id or node_metadatas in them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify this in the docs per my other comment both here and there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thank you.

"envoy.service.status.v2.ClientStatusRequest";

// Management server can use these match criteria to identify clients.
repeated type.matcher.v3.NodeMatcher node_matchers = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need multiple of these if NodeMatcher is sufficiently expressive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how we could achieve requesting client status for multiple unique node ids with only a single NodeMatcher. For example, if I want to request client status for "client-abc", and "client-123", it's hard to come up with a match that will only select this two.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is an OR match, right? Can you clarify this in the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@fuqianggao fuqianggao requested a review from htuch January 21, 2020 01:26
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other than remaining doc comments. Please merge master as you will need to add another protodoc-title I think. Thank you.

/wait

"envoy.service.status.v2.ClientStatusRequest";

// Management server can use these match criteria to identify clients.
repeated type.matcher.v3.NodeMatcher node_matchers = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is an OR match, right? Can you clarify this in the docs?

StringMatcher node_id = 1;

// Specifies match criteria on the node metadata.
repeated MetadataMatcher node_metadatas = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify this in the docs per my other comment both here and there?

option java_outer_classname = "NodeProto";
option java_multiple_files = true;

// [#protodoc-title:NodeMatcher]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "// [#protodoc-title: Node matcher]"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that other files still have the original format, e.g.: https://github.com/envoyproxy/envoy/blob/master/api/envoy/type/matcher/v3/metadata.proto#L15

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind fixing the other one(s)? It's a doc nit but generally we try for human readable docs.

Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other than small doc nit, thanks.

/wait

option java_outer_classname = "NodeProto";
option java_multiple_files = true;

// [#protodoc-title:NodeMatcher]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind fixing the other one(s)? It's a doc nit but generally we try for human readable docs.

Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
Signed-off-by: Fuqiang Gao <fuqianggao@google.com>
@fuqianggao
Copy link
Contributor Author

LGTM other than small doc nit, thanks.

/wait

Done. Thanks for your review!

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@htuch htuch merged commit 10f756e into envoyproxy:master Jan 22, 2020
htuch added a commit to htuch/envoy that referenced this pull request Jan 24, 2020
Since envoyproxy#9383 merged, I've been unable to bazel build @envoy_api//...

Upsteam fix at protocolbuffers/protobuf#7135

Adding as a patch to avoid having to bump protobuf version when that
merges just for this.

Risk level: Low
Testing: bazel build @envoy_api//...

Signed-off-by: Harvey Tuch <htuch@google.com>
htuch added a commit that referenced this pull request Jan 24, 2020
Since #9383 merged, I've been unable to bazel build @envoy_api//...

Upstream fix at protocolbuffers/protobuf#7135

Adding as a patch to avoid having to bump protobuf version when that
merges just for this.

Risk level: Low
Testing: bazel build @envoy_api//...

Signed-off-by: Harvey Tuch <htuch@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants