-
Notifications
You must be signed in to change notification settings - Fork 517
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
delta: [#558] Support dynamic wildcard subscription in delta-xds #559
Conversation
Unittests are not provided yet, will add soon |
5f4d7df
to
498dede
Compare
Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
…ard mode Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
498dede
to
f3652b0
Compare
This build error seems to be related to an envoy upstream update in the repo 14 days ago
|
@valerian-roche any updates on upstream still failing around this? |
I think I need to rebase, will do |
@alecholmez Build is fixed after merging main |
A first draft for sotw is available at valerian-roche#4 but won't be raised here until this one has reached a conclusion as it's building on it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor cleanup. Overall it looks good though!
pkg/server/stream/v3/stream.go
Outdated
@@ -23,20 +23,44 @@ type DeltaStream interface { | |||
|
|||
// StreamState will keep track of resource state per type on a stream. | |||
type StreamState struct { // nolint:golint,revive | |||
// Indicates whether the original DeltaRequest was a wildcard LDS/RDS request. | |||
// Indicates whether the stream currently has a wildcard watch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this comment explicit to delta since SOTW also uses this StreamState internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I have a follow-up PR to add it for sotw, will re-update then
pkg/server/delta/v3/server.go
Outdated
continue | ||
} | ||
if _, ok := sv[resource]; ok && streamState.IsWildcard() { | ||
// xds protocol specifically states that if a resource if unsubscribed while a wildcard watch is present, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we clean this comment block up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to make it clearer, though it is still so counter-intuitive to me I have a hard time formalizing it
Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
f79d7cc
to
45c406b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome thanks for these changes @valerian-roche.
… top of #559 (#657) * Set safe directory in CI script to fix build issue related to VCS stamping in binaries Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com> * Revert "Fix for the SDS update failure (#615)" This reverts commit 0e0f25d. Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
Envoy 1.21.0 introduced a new definition for wildcard watches in the xds protocol
I provided more details in #558, but it impacts both the semantics and the behavior
This PR is implementing this flow for delta-xds (I will follow-up with a sotw later on)
It is for now ignoring a potential grey area of the protocol (which was somewhat applicable before): if a stream is opened in legacy wildcard mode (no resource provided) then is subscribing to a resource which is not wildcard, it will remain wildcard. I don't think this should be impactful as it would mean envoy using both legacy and new model in the same stream, but worth noting