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

delta: [#558] Support dynamic wildcard subscription in delta-xds #559

Merged
merged 6 commits into from
Jun 28, 2022

Conversation

valerian-roche
Copy link
Contributor

@valerian-roche valerian-roche commented Apr 19, 2022

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

@valerian-roche
Copy link
Contributor Author

Unittests are not provided yet, will add soon

@valerian-roche valerian-roche force-pushed the vr/wildcard branch 6 times, most recently from 5f4d7df to 498dede Compare April 19, 2022 20:50
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>
@valerian-roche valerian-roche marked this pull request as ready for review April 22, 2022 15:44
@valerian-roche
Copy link
Contributor Author

This build error seems to be related to an envoy upstream update in the repo 14 days ago
Envoy is failing to add listeners with

listener-0: Didn't find a registered implementation for 'envoy.filters.http.router' with type URL: ''
listener-1: Didn't find a registered implementation for 'envoy.filters.http.router' with type URL: ''
listener-2: Didn't find a registered implementation for 'envoy.filters.http.router' with type URL: ''

@valerian-roche valerian-roche changed the title [#558] Support dynamic wildcard subscription in delta-xds delta: [#558] Support dynamic wildcard subscription in delta-xds Apr 22, 2022
@alecholmez
Copy link
Contributor

@valerian-roche any updates on upstream still failing around this?

@valerian-roche
Copy link
Contributor Author

I think I need to rebase, will do

@valerian-roche
Copy link
Contributor Author

@alecholmez Build is fixed after merging main

@valerian-roche
Copy link
Contributor Author

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

@alecholmez alecholmez self-requested a review May 27, 2022 19:24
Copy link
Contributor

@alecholmez alecholmez left a 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/cache/v3/delta.go Show resolved Hide resolved
pkg/cache/v3/delta.go Show resolved Hide resolved
@@ -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
Copy link
Contributor

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.

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. I have a follow-up PR to add it for sotw, will re-update then

pkg/server/stream/v3/stream.go Show resolved Hide resolved
pkg/server/delta/v3/server.go Outdated Show resolved Hide resolved
pkg/server/delta/v3/server.go Outdated Show resolved Hide resolved
continue
}
if _, ok := sv[resource]; ok && streamState.IsWildcard() {
// xds protocol specifically states that if a resource if unsubscribed while a wildcard watch is present,
Copy link
Contributor

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?

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 tried to make it clearer, though it is still so counter-intuitive to me I have a hard time formalizing it

pkg/server/delta/v3/server.go Outdated Show resolved Hide resolved
pkg/server/stream/v3/stream.go Show resolved Hide resolved
Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
Copy link
Contributor

@alecholmez alecholmez left a 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.

@alecholmez alecholmez self-requested a review June 28, 2022 14:27
alecholmez pushed a commit that referenced this pull request Mar 20, 2023
… 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>
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.

2 participants