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

xds server: Update reference/resource checks for DefaultFilterChain #949

Conversation

sfc-gh-randerson
Copy link
Contributor

This updates the consistency check to also consider the "DefaultFilterChain" in addition to the list of filter chains.

@sfc-gh-randerson sfc-gh-randerson force-pushed the also-check-default-filter-chain branch from dd23fdd to 99e14a0 Compare May 29, 2024 02:08
This updates the consistency check to also consider the
"DefaultFilterChain" in addition to the list of filter chains.

Signed-off-by: Ryan Anderson <ryan.anderson@snowflake.com>
@sfc-gh-randerson sfc-gh-randerson force-pushed the also-check-default-filter-chain branch from 99e14a0 to cb32075 Compare May 29, 2024 20:08
@sfc-gh-randerson sfc-gh-randerson changed the title Update reference/resource checks for DefaultFilterChain xds server: Update reference/resource checks for DefaultFilterChain May 30, 2024
Copy link
Contributor

@valerian-roche valerian-roche left a comment

Choose a reason for hiding this comment

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

Minor comment only
Tbh this code is not really maintained and very likely out of sync with envoy which has evolved on supporting partial snapshots.
Can you confirm if you rely on this functionality? My long term goal is to fully remove this logic as it is imo out of place and is also creating issues to support other data-planes (for instance grpc libraries)

pkg/cache/v3/resource.go Outdated Show resolved Hide resolved
@sfc-gh-randerson
Copy link
Contributor Author

Minor comment only Tbh this code is not really maintained and very likely out of sync with envoy which has evolved on supporting partial snapshots. Can you confirm if you rely on this functionality? My long term goal is to fully remove this logic as it is imo out of place and is also creating issues to support other data-planes (for instance grpc libraries)

By "this code", do you mean the resource/reference check part of cache.Snapshot? Or something more broad?

We are setting ads=true when we call NewSnapshotCache() which seems to imply that our snapshots have to be consistent, which is what prompted this change. We can probably change away from that, but there's not really an indication right now that it's discouraged.

Signed-off-by: Ryan Anderson <ryan.anderson@snowflake.com>
@sfc-gh-randerson sfc-gh-randerson force-pushed the also-check-default-filter-chain branch from b7ac629 to 03832c1 Compare June 4, 2024 23:05
@valerian-roche valerian-roche enabled auto-merge (squash) June 5, 2024 14:31
Copy link
Contributor

@valerian-roche valerian-roche left a comment

Choose a reason for hiding this comment

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

lgtm

@valerian-roche valerian-roche merged commit 77feb56 into envoyproxy:main Jun 5, 2024
4 checks passed
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