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

api: add node field to csds request #12522

Merged
merged 3 commits into from
Aug 12, 2020
Merged

Conversation

Rainton
Copy link
Contributor

@Rainton Rainton commented Aug 6, 2020

Commit Message: added an node field to csds request to identify the CSDS client to the CSDS server, and removed the [#not-implemented-hide:] for the endpoint_config since it has been implemented in #11577
Additional Description:
Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Yutong Li [email protected]
/cc @fuqianggao @alexburnos

@repokitteh-read-only
Copy link

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

🐱

Caused by: #12522 was opened by Rainton.

see: more, trace.

@markdroth
Copy link
Contributor

I don't understand why this is needed. There's already a way to match on the node ID field in the NodeMatcher message:

StringMatcher node_id = 1;

@markdroth
Copy link
Contributor

As per offline discussion, it seems like what you want here is not a way to select the data that you want CSDS to return but rather a way to identify the CSDS client to the CSDS server. In that case, I suspect the right way to do it is to add a Node message, not just a single string id field.

That having been said, I'm not actually all that familiar with how CSDS is used, so I'm going to defer to @htuch or one of the other API shephards to review this.

@htuch
Copy link
Member

htuch commented Aug 10, 2020

Yes, I think this probably belongs in the Node ID or metadata.

@Rainton
Copy link
Contributor Author

Rainton commented Aug 11, 2020

@markdroth @htuch Just want to make sure that what you suggested is replacing the string id field by a Node filed just like which in DiscoveryRequest, right?

@markdroth
Copy link
Contributor

Yes, exactly.

@htuch
Copy link
Member

htuch commented Aug 11, 2020

Yeah, it seems reasonable to re-use this structure if we consider nodes to be Envoy/gRPC clients, intermediaries such as proxies, CSDS, etc.

@Rainton Rainton changed the title api: add id to csds request api: add node field to csds request Aug 11, 2020
@Rainton
Copy link
Contributor Author

Rainton commented Aug 11, 2020

@markdroth @htuch Thanks for the suggestion! I've modified the csds request by adding a node field to it and also edited the commit message.

/cc @fuqianggao

@htuch
Copy link
Member

htuch commented Aug 12, 2020

LGTM

Copy link
Contributor

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

/lgtm api

@htuch htuch merged commit 90a97c7 into envoyproxy:master Aug 12, 2020
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.

3 participants