[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.

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 <[email protected]>

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

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

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

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.

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.

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

Fix lint errors

9a0e64f

alecholmez requested changes Feb 18, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Snapshot Cache] Provide new snapshot interface#1405

[Snapshot Cache] Provide new snapshot interface#1405
valerian-roche wants to merge 2 commits intomainfrom
vr/snapshot-v2

valerian-roche commented Feb 16, 2026

Uh oh!

mmorel-35 Feb 17, 2026

Uh oh!

mmorel-35 Feb 17, 2026 •

edited

Loading

Uh oh!

alecholmez Feb 18, 2026

Uh oh!

alecholmez Feb 18, 2026

Uh oh!

alecholmez Feb 18, 2026

Uh oh!

alecholmez Feb 18, 2026

Uh oh!

alecholmez Feb 18, 2026

Uh oh!

alecholmez Feb 18, 2026

Uh oh!

alecholmez Feb 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mmorel-35 Feb 17, 2026 •

edited

Loading