[Snapshot Cache] Provide new snapshot interface by valerian-roche · Pull Request #1405 · envoyproxy/go-control-plane

valerian-roche · 2026-02-16T22:49:58Z

Description

This PR adds a new Snapshot type to provide to the Snapshot Cache. This new snapshot types.Snapshot has the following properties:

a clean public/private interface split, to allow simpler further evolution
a much smaller public interface
uses CachedResource instances instead of types.Resource internally, allowing mutualization of serialization and hashing when computing and serializing responses
allow to build it from a new public SnapshotResource type, allowing extension (e.g. providing the resource version and TTL) in a uniform entry (e.g. avoiding the TTL split). This allows providing fully opaque resources without the need for a GetName method
allows reusing sub-snapshots per type. This provides the capability for instance to reuse the RDS snapshot when only changing the CDS one. As the version will not have changed resources will not be sent to envoy for the unchanged types. It is also possible to share the same sub-snapshot across snapshots.

The legacy snapshot remains present and functional for now. Users are encouraged to use the new type, but the snapshot cache will keep using the same. The cache.Snapshot interface no longer includes the methods GetResources(typeURL string) map[string]types.Resource and GetResourcesAndTTL(typeURL string) map[string]types.ResourceWithTTL but the legacy snapshot still implements them, and interface casting can still be used to access them. The cache.Snapshot interface now includes the additional GetTypeSnapshot(typeURL string) types.TypeSnapshot method. Users providing their own snapshot type will have to implement it. It is expected to be an easy addition as the types.NewTypeSnapshot should be a simple adaptation layer. If this creates a problem please create an issue on the repository.

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this been tested?

On top of additional tests in the PR itself, this has been in production through the Datadog fork for multiple months.

mmorel-35 · 2026-02-17T19:12:59Z

pkg/cache/v3/simple.go

 	if len(resourcesToReturn) == 0 && len(deletedResources) == 0 && !replyIfEmpty {
 		// Nothing's changed
-		return nil
+		return nil, nil


That's quite strange to have a nil, nil return

I notice that it is an already used pattern, ninil advise against it

Hmm I'm used to use it in a lot of places. Otherwise we end up returning errors that are not errors, which are, imo, a worst pattern. Will take a look at how to update this

I could add a boolean but I don't think it'd make the interface clearer

alecholmez · 2026-02-18T13:48:49Z

pkg/cache/internal/cached_resource.go

+}
+
+// WithMarshaledResource enables the user to provide the already marshaled bytes if they have them.
+// Those bytes should strive at being consistent if the object has not changed (beware protobuf non-deterministic marshaling)


Suggested change

// Those bytes should strive at being consistent if the object has not changed (beware protobuf non-deterministic marshaling)

// Those bytes should strive for consistency if the object has not changed (beware of protobuf non-deterministic marshaling)

alecholmez · 2026-02-18T13:49:50Z

pkg/cache/internal/cached_resource.go

+}
+
+// WithResourceTTL sets a TTL on the resource, that will be sent to the client with the payload.
+func WithResourceTTL(ttl *time.Duration) CachedResourceOption {


Maybe WithTTL? It sounds like you should be able to set the full Resource along with the TTL given the current name

with the new name definitely makes sense, updated (along with WithResourceVersion)

alecholmez · 2026-02-18T13:52:02Z

pkg/cache/internal/cached_resource.go

+	}
+}
+
+var deltaResourceTypeURL = "type.googleapis.com/" + string(proto.MessageName(&discovery.Resource{}))


Can we rename this to something non-delta specific?

alecholmez · 2026-02-18T13:56:34Z

pkg/cache/internal/cached_resource.go

+
+// GetSotwResource returns the serialized resource to return within a sotw response, including TTL handling if applicable.
+// It uses lazily computed and cached fields to ensure a resource is serialized at most once per update.
+func (c *CachedResource) GetSotwResource(isHeartbeat bool) (*anypb.Any, error) {


I'm wondering if we should call this method and the "delta" version something like

GetAnyResource()

vs

GetDiscoveryResource()

or something along those lines. My thought here is that this CachedResource doesn't necessarily care about Sotw or Delta, it just cares about different ways to serialize a discovery response given a provided marshaled resource. IMHO the method names should be description as to how the data is returned rather than the protocol that it will be used with

Good call, especially as Sotw does use Discovery also when TTL is used

alecholmez · 2026-02-18T14:02:38Z

pkg/cache/internal/cached_resource.go

+}
+
+// getMarshaledResource lazily marshals the resource and returns the bytes.
+func (c *CachedResource) getMarshaledResource() ([]byte, error) {


Can we delete this method? It seems to only be called in the below methods and it just returns c.marshalfunc() anyways?

alecholmez · 2026-02-18T14:15:38Z

pkg/cache/types/snapshot.go

@@ -0,0 +1,116 @@
+package types


I think this is a good step in the right direction. That being said, I think we may want to think about the organization, type names, and package structure.

Right now I think the public API isn't as declarative as it could be which is yielding a bit of a clunky user experience. I'm wondering if we should play with the idea of "Snapshots" being per type, and then we introduce something else for nodes "Snapshot" consisting of many type snapshots. As an example we could introduce another package for the higher level node grouping of a per type snapshot list.

I think exploring how the imports look would be a really good way to think about this. Something along the lines of:

routeSnapshot := type.Snapshot{}

proxySnapshot := node.Snapshot{ Resources: map[string]type.Snapshot{ routeTypeURL: routeSnapshot } }

I personally also like the ability to sometimes declare objects without constructors but that's just a preference thing.

Let me take a look at this. The idea of package to match node or type looks good, but I'm unsure how it will actually end up reflecting in user code. If they already have packages with those (common) names they will end up with renaming that might makes it more confusing than the type itself carrying the meaning. Maybe something along the lines of cache.TypeSnapshot and cache.NodeSnapshot would carry a clear state while avoiding relying on the package name

I updated to call them types.NodeSnapshot and types.TypeSnapshot. If you feel strongly about it I can update to type and node package names, though it'd require using public interface only, which may limit how much we can do to connect them

alecholmez · 2026-02-18T14:17:54Z

pkg/cache/internal/cached_resource.go

@@ -0,0 +1,221 @@
+package internal


I really think we should avoid the use of an "internal" package. See my comment below about potentially adding new packages and/or files.

I'm fine renaming this. I spent some time on this and could not figure something which ended up not requiring renaming in all imports. Imo it is not that important though as part of the private API

This commit adds a new Snapshot type to provide to the Snapshot Cache. This new snapshot `types.Snapshot` has the following properties: - a clean public/private interface split, to allow simpler further evolution - a much smaller public interface - uses `CachedResource` instances instead of `types.Resource` internally, allowing mutualization of serialization and hashing when computing and serializing responses - allow to build it from a new public `SnapshotResource` type, allowing extension (e.g. providing the resource version and TTL) in a uniform entry (e.g. avoiding the TTL split). This allows providing fully opaque resources without the need for a `GetName` method - allows reusing sub-snapshots per type. This provides the capability for instance to reuse the RDS snapshot when only changing the CDS one. As the version will not have changed resources will not be sent to envoy for the unchanged types. It is also possible to share the same sub-snapshot across snapshots. The legacy snapshot remains present and functional for now. Users are encouraged to use the new type, but the snapshot cache will keep using the same. The `cache.Snapshot` interface no longer includes the methods `GetResources(typeURL string) map[string]types.Resource` and `GetResourcesAndTTL(typeURL string) map[string]types.ResourceWithTTL` but the legacy snapshot still implements them, and interface casting can still be used to access them. The `cache.Snapshot` interface now includes the additional `GetTypeSnapshot(typeURL string) types.TypeSnapshot` method. Users providing their own snapshot type will have to implement it. It is expected to be an easy addition as the `types.NewTypeSnapshot` should be a simple adaptation layer. If this creates a problem please create an issue on the repository. Signed-off-by: Valerian Roche <valerian.roche@gmail.com>

valerian-roche

Pushed some renaming

valerian-roche · 2026-03-09T01:11:00Z

pkg/cache/v3/simple.go

 	if len(resourcesToReturn) == 0 && len(deletedResources) == 0 && !replyIfEmpty {
 		// Nothing's changed
-		return nil
+		return nil, nil


I could add a boolean but I don't think it'd make the interface clearer

valerian-roche · 2026-03-09T01:11:47Z

pkg/cache/internal/cached_resource.go

+}
+
+// WithMarshaledResource enables the user to provide the already marshaled bytes if they have them.
+// Those bytes should strive at being consistent if the object has not changed (beware protobuf non-deterministic marshaling)


valerian-roche · 2026-03-09T01:12:17Z

pkg/cache/internal/cached_resource.go

+}
+
+// WithResourceTTL sets a TTL on the resource, that will be sent to the client with the payload.
+func WithResourceTTL(ttl *time.Duration) CachedResourceOption {


with the new name definitely makes sense, updated (along with WithResourceVersion)

valerian-roche · 2026-03-09T01:14:05Z

pkg/cache/internal/cached_resource.go

+}
+
+// getMarshaledResource lazily marshals the resource and returns the bytes.
+func (c *CachedResource) getMarshaledResource() ([]byte, error) {


valerian-roche · 2026-03-09T01:14:10Z

pkg/cache/internal/cached_resource.go

+	}
+}
+
+var deltaResourceTypeURL = "type.googleapis.com/" + string(proto.MessageName(&discovery.Resource{}))


valerian-roche · 2026-03-09T01:14:54Z

pkg/cache/types/snapshot.go

@@ -0,0 +1,116 @@
+package types


I updated to call them types.NodeSnapshot and types.TypeSnapshot. If you feel strongly about it I can update to type and node package names, though it'd require using public interface only, which may limit how much we can do to connect them

github-actions · 2026-04-08T04:56:34Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

valerian-roche force-pushed the vr/snapshot-v2 branch 2 times, most recently from 7b2b311 to 9facddb Compare February 16, 2026 23:43

valerian-roche marked this pull request as ready for review February 17, 2026 19:04

valerian-roche requested a review from mmorel-35 February 17, 2026 19:04

valerian-roche force-pushed the vr/snapshot-v2 branch from 9facddb to 1164859 Compare February 17, 2026 19:09

mmorel-35 reviewed Feb 17, 2026

View reviewed changes

alecholmez requested changes Feb 18, 2026

View reviewed changes

valerian-roche and others added 3 commits March 8, 2026 20:50

Fix lint errors

743c5db

Rename internal package. Rename some functions and types

5294d36

valerian-roche force-pushed the vr/snapshot-v2 branch from 9a0e64f to 5294d36 Compare March 9, 2026 01:18

valerian-roche commented Mar 9, 2026

View reviewed changes

github-actions bot added the stale label Apr 8, 2026

	// Those bytes should strive at being consistent if the object has not changed (beware protobuf non-deterministic marshaling)
	// Those bytes should strive for consistency if the object has not changed (beware of protobuf non-deterministic marshaling)

Conversation

valerian-roche commented Feb 16, 2026

Description

Type of change

How has this been tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmorel-35 Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valerian-roche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mmorel-35 Feb 17, 2026 •

edited

Loading