Skip to content

Conversation

kyessenov
Copy link
Contributor

@kyessenov kyessenov commented Aug 8, 2019

Signed-off-by: Kuat Yessenov kuat@google.com

Omit the node identifier from subsequent discovery requests on the same stream.
Restricted to non-incremental xDS for tractability.

Risk Level: low, affects xDS protocol but guarded by an option
Testing: Unit/integration tests are updated
Docs Changes: xDS spec clarification
Release Notes: omit the node identifier from subsequent discovery requests
Fixes: #7860

Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov
Copy link
Contributor Author

kyessenov commented Aug 8, 2019

/assign @htuch

@repokitteh-read-only
Copy link

neither of and, API, shepherds can be assigned to this issue.

🐱

Caused by: a #7876 (comment) was created by @kyessenov.

see: more, trace.

@mandarjog
Copy link
Contributor

Should this be an opt-in option to protect management servers that have incorrectly relied on node data being present in every request?
For pilot we can ensure that it does not assume this before flipping the switch.

@kyessenov
Copy link
Contributor Author

Yes, of course. My current plan is to add a bool to config source in xDS.

Signed-off-by: Kuat Yessenov <kuat@google.com>
@repokitteh-read-only
Copy link

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

🐱

Caused by: #7876 was synchronize by kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <kuat@google.com>
@repokitteh-read-only
Copy link

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

🐱

Caused by: #7876 was synchronize by kyessenov.

see: more, trace.

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.

Mostly LGTM, thanks.
/wait

Signed-off-by: Kuat Yessenov <kuat@google.com>
@repokitteh-read-only
Copy link

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

🐱

Caused by: #7876 was synchronize by kyessenov.

see: more, trace.

@kyessenov
Copy link
Contributor Author

Any chance you can take a look at it @htuch?

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.

LGTM modulo a few nits, thanks.

@htuch htuch added the waiting label Aug 13, 2019
Signed-off-by: Kuat Yessenov <kuat@google.com>
@repokitteh-read-only
Copy link

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

🐱

Caused by: #7876 was synchronize by kyessenov.

see: more, trace.

@repokitteh-read-only
Copy link

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

🐱

Caused by: #7876 was synchronize by kyessenov.

see: more, trace.

@repokitteh-read-only
Copy link

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

🐱

Caused by: #7876 was synchronize by kyessenov.

see: more, trace.

@kyessenov
Copy link
Contributor Author

This should be ready for review again.

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!

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.

xds: omit node in subsequent discovery requests in the gRPC call
3 participants