From c051daa8693088b1bd8916df61e8969bd0402ea1 Mon Sep 17 00:00:00 2001 From: Valerian Roche Date: Wed, 24 Aug 2022 22:06:29 -0400 Subject: [PATCH 1/5] Rework Cache interface to isolate it from streamState and make it more uniform between sotw and delta Signed-off-by: Valerian Roche --- pkg/cache/v3/cache.go | 26 ++++++++++++-- pkg/cache/v3/delta.go | 15 ++++---- pkg/cache/v3/delta_test.go | 40 ++++++++++------------ pkg/cache/v3/linear.go | 21 ++++++------ pkg/cache/v3/linear_test.go | 58 +++++++++++++++---------------- pkg/cache/v3/mux.go | 6 ++-- pkg/cache/v3/simple.go | 27 +++++++-------- pkg/cache/v3/simple_test.go | 26 +++++++++----- pkg/cache/v3/status.go | 19 +++++++++-- pkg/server/delta/v3/server.go | 8 ++--- pkg/server/sotw/v3/ads.go | 10 ++++-- pkg/server/sotw/v3/server.go | 13 ++++--- pkg/server/sotw/v3/xds.go | 12 ++++--- pkg/server/stream/v3/stream.go | 62 ++++------------------------------ pkg/server/v3/delta_test.go | 15 ++++---- pkg/server/v3/server_test.go | 3 +- 16 files changed, 178 insertions(+), 183 deletions(-) diff --git a/pkg/cache/v3/cache.go b/pkg/cache/v3/cache.go index b92d50c91d..c3754b60d4 100644 --- a/pkg/cache/v3/cache.go +++ b/pkg/cache/v3/cache.go @@ -26,7 +26,6 @@ import ( "google.golang.org/protobuf/types/known/durationpb" "github.com/envoyproxy/go-control-plane/pkg/cache/types" - "github.com/envoyproxy/go-control-plane/pkg/server/stream/v3" discovery "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3" ) @@ -37,6 +36,27 @@ type Request = discovery.DiscoveryRequest // DeltaRequest is an alias for the delta discovery request type. type DeltaRequest = discovery.DeltaDiscoveryRequest +// ClientState provides additional data on the client knowledge for the type matching the request +// This allows proper implementation of stateful aspects of the protocol (e.g. returning only some updated resources) +// Though the methods may return mutable parts of the state for performance reasons, +// the cache is expected to consider this state as immutable and thread safe between a watch creation and its cancellation +type ClientState interface { + // GetKnownResources returns the list of resources the clients has ACKed and their associated version. + // The versions are: + // - delta protocol: version of the specific resource set in the response + // - sotw protocol: version of the global response when the resource was last ACKed + GetKnownResources() map[string]string + + // GetSubscribedResources returns the list of resources currently subscribed to by the client for the type. + // For delta it keeps track across requests + // For sotw it is a normalized view of the request resources + GetSubscribedResources() map[string]struct{} + + // IsWildcard returns whether the client has a wildcard watch. + // This considers subtilities related to the current migration of wildcard definition within the protocol. + IsWildcard() bool +} + // ConfigWatcher requests watches for configuration resources by a node, last // applied version identifier, and resource names hint. The watch should send // the responses when they are ready. The watch can be canceled by the @@ -54,7 +74,7 @@ type ConfigWatcher interface { // // Cancel is an optional function to release resources in the producer. If // provided, the consumer may call this function multiple times. - CreateWatch(*Request, stream.StreamState, chan Response) (cancel func()) + CreateWatch(*Request, ClientState, chan Response) (cancel func()) // CreateDeltaWatch returns a new open incremental xDS watch. // This is the entrypoint to propagate configuration changes the @@ -66,7 +86,7 @@ type ConfigWatcher interface { // // Cancel is an optional function to release resources in the producer. If // provided, the consumer may call this function multiple times. - CreateDeltaWatch(*DeltaRequest, stream.StreamState, chan DeltaResponse) (cancel func()) + CreateDeltaWatch(*DeltaRequest, ClientState, chan DeltaResponse) (cancel func()) } // ConfigFetcher fetches configuration resources from cache diff --git a/pkg/cache/v3/delta.go b/pkg/cache/v3/delta.go index deaeeb7ed1..c5adf9fb8b 100644 --- a/pkg/cache/v3/delta.go +++ b/pkg/cache/v3/delta.go @@ -18,7 +18,6 @@ import ( "context" "github.com/envoyproxy/go-control-plane/pkg/cache/types" - "github.com/envoyproxy/go-control-plane/pkg/server/stream/v3" ) // groups together resource-related arguments for the createDeltaResponse function @@ -28,7 +27,7 @@ type resourceContainer struct { systemVersion string } -func createDeltaResponse(ctx context.Context, req *DeltaRequest, state stream.StreamState, resources resourceContainer) *RawDeltaResponse { +func createDeltaResponse(ctx context.Context, req *DeltaRequest, state ClientState, resources resourceContainer) *RawDeltaResponse { // variables to build our response with var nextVersionMap map[string]string var filtered []types.Resource @@ -37,7 +36,7 @@ func createDeltaResponse(ctx context.Context, req *DeltaRequest, state stream.St // If we are handling a wildcard request, we want to respond with all resources switch { case state.IsWildcard(): - if len(state.GetResourceVersions()) == 0 { + if len(state.GetKnownResources()) == 0 { filtered = make([]types.Resource, 0, len(resources.resourceMap)) } nextVersionMap = make(map[string]string, len(resources.resourceMap)) @@ -46,7 +45,7 @@ func createDeltaResponse(ctx context.Context, req *DeltaRequest, state stream.St // we can just set it here to be used for comparison later version := resources.versionMap[name] nextVersionMap[name] = version - prevVersion, found := state.GetResourceVersions()[name] + prevVersion, found := state.GetKnownResources()[name] if !found || (prevVersion != version) { filtered = append(filtered, r) } @@ -54,17 +53,17 @@ func createDeltaResponse(ctx context.Context, req *DeltaRequest, state stream.St // Compute resources for removal // The resource version can be set to "" here to trigger a removal even if never returned before - for name := range state.GetResourceVersions() { + for name := range state.GetKnownResources() { if _, ok := resources.resourceMap[name]; !ok { toRemove = append(toRemove, name) } } default: - nextVersionMap = make(map[string]string, len(state.GetSubscribedResourceNames())) + nextVersionMap = make(map[string]string, len(state.GetSubscribedResources())) // state.GetResourceVersions() may include resources no longer subscribed // In the current code this gets silently cleaned when updating the version map - for name := range state.GetSubscribedResourceNames() { - prevVersion, found := state.GetResourceVersions()[name] + for name := range state.GetSubscribedResources() { + prevVersion, found := state.GetKnownResources()[name] if r, ok := resources.resourceMap[name]; ok { nextVersion := resources.versionMap[name] if prevVersion != nextVersion { diff --git a/pkg/cache/v3/delta_test.go b/pkg/cache/v3/delta_test.go index 4999cad603..c4e30dd04a 100644 --- a/pkg/cache/v3/delta_test.go +++ b/pkg/cache/v3/delta_test.go @@ -3,12 +3,12 @@ package cache_test import ( "context" "fmt" - "reflect" "testing" "time" "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "google.golang.org/protobuf/testing/protocmp" core "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" @@ -35,13 +35,14 @@ func TestSnapshotCacheDeltaWatch(t *testing.T) { // Make our initial request as a wildcard to get all resources and make sure the wildcard requesting works as intended for _, typ := range testTypes { watches[typ] = make(chan cache.DeltaResponse, 1) + state := stream.NewStreamState(true, nil) c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ Node: &core.Node{ Id: "node", }, TypeUrl: typ, ResourceNamesSubscribe: names[typ], - }, stream.NewStreamState(true, nil), watches[typ]) + }, state, watches[typ]) } if err := c.SetSnapshot(context.Background(), key, fixture.snapshot()); err != nil { @@ -69,7 +70,7 @@ func TestSnapshotCacheDeltaWatch(t *testing.T) { watches[typ] = make(chan cache.DeltaResponse, 1) state := stream.NewStreamState(false, versionMap[typ]) for resource := range versionMap[typ] { - state.GetSubscribedResourceNames()[resource] = struct{}{} + state.GetSubscribedResources()[resource] = struct{}{} } c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ Node: &core.Node{ @@ -126,20 +127,18 @@ func TestDeltaRemoveResources(t *testing.T) { }, *streams[typ], watches[typ]) } - if err := c.SetSnapshot(context.Background(), key, fixture.snapshot()); err != nil { - t.Fatal(err) - } + require.NoError(t, c.SetSnapshot(context.Background(), key, fixture.snapshot())) + snapshot := fixture.snapshot() for _, typ := range testTypes { t.Run(typ, func(t *testing.T) { select { case out := <-watches[typ]: - snapshot := fixture.snapshot() assertResourceMapEqual(t, cache.IndexRawResourcesByName(out.(*cache.RawDeltaResponse).Resources), snapshot.GetResources(typ)) nextVersionMap := out.GetNextVersionMap() streams[typ].SetResourceVersions(nextVersionMap) case <-time.After(time.Second): - t.Fatal("failed to receive a snapshot response") + require.Fail(t, "failed to receive a snapshot response") } }) } @@ -152,20 +151,17 @@ func TestDeltaRemoveResources(t *testing.T) { Node: &core.Node{ Id: "node", }, - TypeUrl: typ, + TypeUrl: typ, + ResponseNonce: "nonce", }, *streams[typ], watches[typ]) } - if count := c.GetStatusInfo(key).GetNumDeltaWatches(); count != len(testTypes) { - t.Errorf("watches should be created for the latest version, saw %d watches expected %d", count, len(testTypes)) - } + assert.Equal(t, len(testTypes), c.GetStatusInfo(key).GetNumDeltaWatches(), "watches should be created for the latest version") // set a partially versioned snapshot with no endpoints snapshot2 := fixture.snapshot() snapshot2.Resources[types.Endpoint] = cache.NewResources(fixture.version2, []types.Resource{}) - if err := c.SetSnapshot(context.Background(), key, snapshot2); err != nil { - t.Fatal(err) - } + require.NoError(t, c.SetSnapshot(context.Background(), key, snapshot2)) // validate response for endpoints select { @@ -176,11 +172,9 @@ func TestDeltaRemoveResources(t *testing.T) { nextVersionMap := out.GetNextVersionMap() // make sure the version maps are different since we no longer are tracking any endpoint resources - if reflect.DeepEqual(streams[testTypes[0]].GetResourceVersions(), nextVersionMap) { - t.Fatalf("versionMap for the endpoint resource type did not change, received: %v, instead of an empty map", nextVersionMap) - } + assert.NotEqual(t, nextVersionMap, streams[testTypes[0]].GetKnownResources(), "versionMap for the endpoint resource type did not change") case <-time.After(time.Second): - t.Fatal("failed to receive snapshot response") + assert.Fail(t, "failed to receive snapshot response") } } @@ -203,13 +197,14 @@ func TestConcurrentSetDeltaWatch(t *testing.T) { t.Fatalf("snapshot failed: %s", err) } } else { + state := stream.NewStreamState(false, make(map[string]string)) cancel := c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ Node: &core.Node{ Id: id, }, TypeUrl: rsrc.EndpointType, ResourceNamesSubscribe: []string{clusterName}, - }, stream.NewStreamState(false, make(map[string]string)), responses) + }, state, responses) defer cancel() } @@ -226,7 +221,7 @@ func TestSnapshotDeltaCacheWatchTimeout(t *testing.T) { // Create a non-buffered channel that will block sends. watchCh := make(chan cache.DeltaResponse) state := stream.NewStreamState(false, nil) - state.SetSubscribedResourceNames(map[string]struct{}{names[rsrc.EndpointType][0]: {}}) + state.SetSubscribedResources(map[string]struct{}{names[rsrc.EndpointType][0]: {}}) c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ Node: &core.Node{ Id: key, @@ -269,13 +264,14 @@ func TestSnapshotCacheDeltaWatchCancel(t *testing.T) { c := cache.NewSnapshotCache(true, group{}, logger{t: t}) for _, typ := range testTypes { responses := make(chan cache.DeltaResponse, 1) + state := stream.NewStreamState(false, make(map[string]string)) cancel := c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ Node: &core.Node{ Id: key, }, TypeUrl: typ, ResourceNamesSubscribe: names[typ], - }, stream.NewStreamState(false, make(map[string]string)), responses) + }, state, responses) // Cancel the watch cancel() diff --git a/pkg/cache/v3/linear.go b/pkg/cache/v3/linear.go index cf5ab7e268..33385b2959 100644 --- a/pkg/cache/v3/linear.go +++ b/pkg/cache/v3/linear.go @@ -24,7 +24,6 @@ import ( "github.com/envoyproxy/go-control-plane/pkg/cache/types" "github.com/envoyproxy/go-control-plane/pkg/log" - "github.com/envoyproxy/go-control-plane/pkg/server/stream/v3" ) type watches = map[chan Response]struct{} @@ -164,11 +163,11 @@ func (cache *LinearCache) notifyAll(modified map[string]struct{}) { } for id, watch := range cache.deltaWatches { - if !watch.StreamState.WatchesResources(modified) { + if !watch.WatchesResources(modified) { continue } - res := cache.respondDelta(watch.Request, watch.Response, watch.StreamState) + res := cache.respondDelta(watch.Request, watch.Response, watch.clientState) if res != nil { delete(cache.deltaWatches, id) } @@ -176,8 +175,8 @@ func (cache *LinearCache) notifyAll(modified map[string]struct{}) { } } -func (cache *LinearCache) respondDelta(request *DeltaRequest, value chan DeltaResponse, state stream.StreamState) *RawDeltaResponse { - resp := createDeltaResponse(context.Background(), request, state, resourceContainer{ +func (cache *LinearCache) respondDelta(request *DeltaRequest, value chan DeltaResponse, clientState ClientState) *RawDeltaResponse { + resp := createDeltaResponse(context.Background(), request, clientState, resourceContainer{ resourceMap: cache.resources, versionMap: cache.versionMap, systemVersion: cache.getVersion(), @@ -187,7 +186,7 @@ func (cache *LinearCache) respondDelta(request *DeltaRequest, value chan DeltaRe if len(resp.Resources) > 0 || len(resp.RemovedResources) > 0 { if cache.log != nil { cache.log.Debugf("[linear cache] node: %s, sending delta response for typeURL %s with resources: %v removed resources: %v with wildcard: %t", - request.GetNode().GetId(), request.TypeUrl, GetResourceNames(resp.Resources), resp.RemovedResources, state.IsWildcard()) + request.GetNode().GetId(), request.TypeUrl, GetResourceNames(resp.Resources), resp.RemovedResources, clientState.IsWildcard()) } value <- resp return resp @@ -298,7 +297,7 @@ func (cache *LinearCache) GetResources() map[string]types.Resource { return resources } -func (cache *LinearCache) CreateWatch(request *Request, _ stream.StreamState, value chan Response) func() { +func (cache *LinearCache) CreateWatch(request *Request, clientState ClientState, value chan Response) func() { if request.TypeUrl != cache.typeURL { value <- nil return nil @@ -371,7 +370,7 @@ func (cache *LinearCache) CreateWatch(request *Request, _ stream.StreamState, va } } -func (cache *LinearCache) CreateDeltaWatch(request *DeltaRequest, state stream.StreamState, value chan DeltaResponse) func() { +func (cache *LinearCache) CreateDeltaWatch(request *DeltaRequest, clientState ClientState, value chan DeltaResponse) func() { cache.mu.Lock() defer cache.mu.Unlock() @@ -388,7 +387,7 @@ func (cache *LinearCache) CreateDeltaWatch(request *DeltaRequest, state stream.S cache.log.Errorf("failed to update version map: %v", err) } } - response := cache.respondDelta(request, value, state) + response := cache.respondDelta(request, value, clientState) // if respondDelta returns nil this means that there is no change in any resource version // create a new watch accordingly @@ -396,10 +395,10 @@ func (cache *LinearCache) CreateDeltaWatch(request *DeltaRequest, state stream.S watchID := cache.nextDeltaWatchID() if cache.log != nil { cache.log.Infof("[linear cache] open delta watch ID:%d for %s Resources:%v, system version %q", watchID, - cache.typeURL, state.GetSubscribedResourceNames(), cache.getVersion()) + cache.typeURL, clientState.GetSubscribedResources(), cache.getVersion()) } - cache.deltaWatches[watchID] = DeltaResponseWatch{Request: request, Response: value, StreamState: state} + cache.deltaWatches[watchID] = DeltaResponseWatch{Request: request, Response: value, clientState: clientState} return cache.cancelDeltaWatch(watchID) } diff --git a/pkg/cache/v3/linear_test.go b/pkg/cache/v3/linear_test.go index 617d90366e..b5f9a0183a 100644 --- a/pkg/cache/v3/linear_test.go +++ b/pkg/cache/v3/linear_test.go @@ -191,10 +191,10 @@ func hashResource(t *testing.T, resource types.Resource) string { func createWildcardDeltaWatch(c *LinearCache, w chan DeltaResponse) { state := stream.NewStreamState(true, nil) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state, w) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) resp := <-w state.SetResourceVersions(resp.GetNextVersionMap()) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state, w) // Ensure the watch is set properly with cache values + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) // Ensure the watch is set properly with cache values } func TestLinearInitialResources(t *testing.T) { @@ -462,11 +462,11 @@ func TestLinearDeltaWildcard(t *testing.T) { c := NewLinearCache(testType) state1 := stream.NewStreamState(true, map[string]string{}) w1 := make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state1, w1) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state1, w1) mustBlockDelta(t, w1) state2 := stream.NewStreamState(true, map[string]string{}) w2 := make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state2, w2) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state2, w2) mustBlockDelta(t, w1) checkDeltaWatchCount(t, c, 2) @@ -491,16 +491,16 @@ func TestLinearDeltaExistingResources(t *testing.T) { assert.NoError(t, err) state := stream.NewStreamState(false, nil) - state.SetSubscribedResourceNames(map[string]struct{}{"b": {}, "c": {}}) // watching b and c - not interested in a + state.SetSubscribedResources(map[string]struct{}{"b": {}, "c": {}}) // watching b and c - not interested in a w := make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state, w) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) checkDeltaWatchCount(t, c, 0) verifyDeltaResponse(t, w, []resourceInfo{{"b", hashB}}, []string{}) state = stream.NewStreamState(false, nil) - state.SetSubscribedResourceNames(map[string]struct{}{"a": {}, "b": {}}) + state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w = make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state, w) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) checkDeltaWatchCount(t, c, 0) verifyDeltaResponse(t, w, []resourceInfo{{"b", hashB}, {"a", hashA}}, nil) } @@ -517,16 +517,16 @@ func TestLinearDeltaInitialResourcesVersionSet(t *testing.T) { assert.NoError(t, err) state := stream.NewStreamState(false, map[string]string{"b": hashB}) - state.SetSubscribedResourceNames(map[string]struct{}{"a": {}, "b": {}}) + state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w := make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state, w) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) checkDeltaWatchCount(t, c, 0) verifyDeltaResponse(t, w, []resourceInfo{{"a", hashA}}, nil) // b is up to date and shouldn't be returned state = stream.NewStreamState(false, map[string]string{"a": hashA, "b": hashB}) - state.SetSubscribedResourceNames(map[string]struct{}{"a": {}, "b": {}}) + state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w = make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state, w) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) b = &endpoint.ClusterLoadAssignment{ClusterName: "b", Endpoints: []*endpoint.LocalityLbEndpoints{{Priority: 10}}} // new version of b @@ -551,17 +551,17 @@ func TestLinearDeltaResourceUpdate(t *testing.T) { checkVersionMapNotSet(t, c) state := stream.NewStreamState(false, nil) - state.SetSubscribedResourceNames(map[string]struct{}{"a": {}, "b": {}}) + state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w := make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state, w) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) checkDeltaWatchCount(t, c, 0) verifyDeltaResponse(t, w, []resourceInfo{{"b", hashB}, {"a", hashA}}, nil) checkVersionMapSet(t, c) state = stream.NewStreamState(false, map[string]string{"a": hashA, "b": hashB}) - state.SetSubscribedResourceNames(map[string]struct{}{"a": {}, "b": {}}) + state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w = make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state, w) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) @@ -587,16 +587,16 @@ func TestLinearDeltaResourceDelete(t *testing.T) { assert.NoError(t, err) state := stream.NewStreamState(false, nil) - state.SetSubscribedResourceNames(map[string]struct{}{"a": {}, "b": {}}) + state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w := make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state, w) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) checkDeltaWatchCount(t, c, 0) verifyDeltaResponse(t, w, []resourceInfo{{"b", hashB}, {"a", hashA}}, nil) state = stream.NewStreamState(false, map[string]string{"a": hashA, "b": hashB}) - state.SetSubscribedResourceNames(map[string]struct{}{"a": {}, "b": {}}) + state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w = make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state, w) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) @@ -612,13 +612,13 @@ func TestLinearDeltaMultiResourceUpdates(t *testing.T) { c := NewLinearCache(testType) state := stream.NewStreamState(false, nil) - state.SetSubscribedResourceNames(map[string]struct{}{"a": {}, "b": {}}) + state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w := make(chan DeltaResponse, 1) checkVersionMapNotSet(t, c) assert.Equal(t, 0, c.NumResources()) // Initial update - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state, w) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) // The version map should now be created, even if empty @@ -636,7 +636,7 @@ func TestLinearDeltaMultiResourceUpdates(t *testing.T) { state.SetResourceVersions(resp.GetNextVersionMap()) // Multiple updates - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state, w) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) a = &endpoint.ClusterLoadAssignment{ClusterName: "a", Endpoints: []*endpoint.LocalityLbEndpoints{ //resource update @@ -656,7 +656,7 @@ func TestLinearDeltaMultiResourceUpdates(t *testing.T) { state.SetResourceVersions(resp.GetNextVersionMap()) // Update/add/delete - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state, w) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) a = &endpoint.ClusterLoadAssignment{ClusterName: "a", Endpoints: []*endpoint.LocalityLbEndpoints{ //resource update @@ -675,7 +675,7 @@ func TestLinearDeltaMultiResourceUpdates(t *testing.T) { state.SetResourceVersions(resp.GetNextVersionMap()) // Re-add previously deleted watched resource - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, state, w) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) b = &endpoint.ClusterLoadAssignment{ClusterName: "b", Endpoints: []*endpoint.LocalityLbEndpoints{}} // recreate watched resource @@ -738,7 +738,7 @@ func TestLinearMixedWatches(t *testing.T) { sotwState := stream.NewStreamState(false, nil) w := make(chan Response, 1) - c.CreateWatch(&Request{ResourceNames: []string{"a", "b"}, TypeUrl: testType, VersionInfo: c.getVersion()}, sotwState, w) + c.CreateWatch(&Request{ResourceNames: []string{"a", "b"}, TypeUrl: testType, VersionInfo: c.getVersion()}, &sotwState, w) mustBlock(t, w) checkVersionMapNotSet(t, c) @@ -752,16 +752,16 @@ func TestLinearMixedWatches(t *testing.T) { verifyResponse(t, w, c.getVersion(), 1) checkVersionMapNotSet(t, c) - c.CreateWatch(&Request{ResourceNames: []string{"a", "b"}, TypeUrl: testType, VersionInfo: c.getVersion()}, sotwState, w) + c.CreateWatch(&Request{ResourceNames: []string{"a", "b"}, TypeUrl: testType, VersionInfo: c.getVersion()}, &sotwState, w) mustBlock(t, w) checkVersionMapNotSet(t, c) deltaState := stream.NewStreamState(false, map[string]string{"a": hashA, "b": hashB}) - deltaState.SetSubscribedResourceNames(map[string]struct{}{"a": {}, "b": {}}) + deltaState.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) wd := make(chan DeltaResponse, 1) // Initial update - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, deltaState, wd) + c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &deltaState, wd) mustBlockDelta(t, wd) checkDeltaWatchCount(t, c, 1) checkVersionMapSet(t, c) diff --git a/pkg/cache/v3/mux.go b/pkg/cache/v3/mux.go index 9fdfb090d6..dd4c13a3e5 100644 --- a/pkg/cache/v3/mux.go +++ b/pkg/cache/v3/mux.go @@ -17,8 +17,6 @@ package cache import ( "context" "errors" - - "github.com/envoyproxy/go-control-plane/pkg/server/stream/v3" ) // MuxCache multiplexes across several caches using a classification function. @@ -37,7 +35,7 @@ type MuxCache struct { var _ Cache = &MuxCache{} -func (mux *MuxCache) CreateWatch(request *Request, state stream.StreamState, value chan Response) func() { +func (mux *MuxCache) CreateWatch(request *Request, state ClientState, value chan Response) func() { key := mux.Classify(request) cache, exists := mux.Caches[key] if !exists { @@ -47,7 +45,7 @@ func (mux *MuxCache) CreateWatch(request *Request, state stream.StreamState, val return cache.CreateWatch(request, state, value) } -func (mux *MuxCache) CreateDeltaWatch(request *DeltaRequest, state stream.StreamState, value chan DeltaResponse) func() { +func (mux *MuxCache) CreateDeltaWatch(request *DeltaRequest, state ClientState, value chan DeltaResponse) func() { key := mux.ClassifyDelta(request) cache, exists := mux.Caches[key] if !exists { diff --git a/pkg/cache/v3/simple.go b/pkg/cache/v3/simple.go index d3875ed575..216b2e41a1 100644 --- a/pkg/cache/v3/simple.go +++ b/pkg/cache/v3/simple.go @@ -23,7 +23,6 @@ import ( "github.com/envoyproxy/go-control-plane/pkg/cache/types" "github.com/envoyproxy/go-control-plane/pkg/log" - "github.com/envoyproxy/go-control-plane/pkg/server/stream/v3" ) // ResourceSnapshot is an abstract snapshot of a collection of resources that @@ -287,7 +286,7 @@ func (cache *snapshotCache) SetSnapshot(ctx context.Context, node string, snapsh snapshot, watch.Request, watch.Response, - watch.StreamState, + watch.clientState, ) if err != nil { return err @@ -345,7 +344,7 @@ func superset(names map[string]bool, resources map[string]types.ResourceWithTTL) // CreateWatch returns a watch for an xDS request. A nil function may be // returned if an error occurs. -func (cache *snapshotCache) CreateWatch(request *Request, streamState stream.StreamState, value chan Response) func() { +func (cache *snapshotCache) CreateWatch(request *Request, clientState ClientState, value chan Response) func() { nodeID := cache.hash.ID(request.Node) cache.mu.Lock() @@ -369,7 +368,7 @@ func (cache *snapshotCache) CreateWatch(request *Request, streamState stream.Str } if exists { - knownResourceNames := streamState.GetKnownResourceNames(request.TypeUrl) + knownResourceNames := clientState.GetKnownResources() diff := []string{} for _, r := range request.ResourceNames { if _, ok := knownResourceNames[r]; !ok { @@ -485,7 +484,7 @@ func createResponse(ctx context.Context, request *Request, resources map[string] } // CreateDeltaWatch returns a watch for a delta xDS request which implements the Simple SnapshotCache. -func (cache *snapshotCache) CreateDeltaWatch(request *DeltaRequest, state stream.StreamState, value chan DeltaResponse) func() { +func (cache *snapshotCache) CreateDeltaWatch(request *DeltaRequest, clientState ClientState, value chan DeltaResponse) func() { nodeID := cache.hash.ID(request.Node) t := request.GetTypeUrl() @@ -514,7 +513,7 @@ func (cache *snapshotCache) CreateDeltaWatch(request *DeltaRequest, state stream if err != nil { cache.log.Errorf("failed to compute version for snapshot resources inline: %s", err) } - response, err := cache.respondDelta(context.Background(), snapshot, request, value, state) + response, err := cache.respondDelta(context.Background(), snapshot, request, value, clientState) if err != nil { cache.log.Errorf("failed to respond with delta response: %s", err) } @@ -526,12 +525,12 @@ func (cache *snapshotCache) CreateDeltaWatch(request *DeltaRequest, state stream watchID := cache.nextDeltaWatchID() if exists { - cache.log.Infof("open delta watch ID:%d for %s Resources:%v from nodeID: %q, version %q", watchID, t, state.GetSubscribedResourceNames(), nodeID, snapshot.GetVersion(t)) + cache.log.Infof("open delta watch ID:%d for %s Resources:%v from nodeID: %q, version %q", watchID, t, clientState.GetSubscribedResources(), nodeID, snapshot.GetVersion(t)) } else { - cache.log.Infof("open delta watch ID:%d for %s Resources:%v from nodeID: %q", watchID, t, state.GetSubscribedResourceNames(), nodeID) + cache.log.Infof("open delta watch ID:%d for %s Resources:%v from nodeID: %q", watchID, t, clientState.GetSubscribedResources(), nodeID) } - info.setDeltaResponseWatch(watchID, DeltaResponseWatch{Request: request, Response: value, StreamState: state}) + info.setDeltaResponseWatch(watchID, DeltaResponseWatch{Request: request, Response: value, clientState: clientState}) return cache.cancelDeltaWatch(nodeID, watchID) } @@ -539,20 +538,20 @@ func (cache *snapshotCache) CreateDeltaWatch(request *DeltaRequest, state stream } // Respond to a delta watch with the provided snapshot value. If the response is nil, there has been no state change. -func (cache *snapshotCache) respondDelta(ctx context.Context, snapshot ResourceSnapshot, request *DeltaRequest, value chan DeltaResponse, state stream.StreamState) (*RawDeltaResponse, error) { - resp := createDeltaResponse(ctx, request, state, resourceContainer{ +func (cache *snapshotCache) respondDelta(ctx context.Context, snapshot ResourceSnapshot, request *DeltaRequest, value chan DeltaResponse, clientState ClientState) (*RawDeltaResponse, error) { + resp := createDeltaResponse(ctx, request, clientState, resourceContainer{ resourceMap: snapshot.GetResources(request.TypeUrl), versionMap: snapshot.GetVersionMap(request.TypeUrl), systemVersion: snapshot.GetVersion(request.TypeUrl), }) // Only send a response if there were changes - // We want to respond immediately for the first wildcard request in a stream, even if the response is empty + // We want to respond immediately for the first request in a stream if it is wildcard, even if the response is empty // otherwise, envoy won't complete initialization - if len(resp.Resources) > 0 || len(resp.RemovedResources) > 0 || (state.IsWildcard() && state.IsFirst()) { + if len(resp.Resources) > 0 || len(resp.RemovedResources) > 0 || (clientState.IsWildcard() && request.ResponseNonce == "") { if cache.log != nil { cache.log.Debugf("node: %s, sending delta response for typeURL %s with resources: %v removed resources: %v with wildcard: %t", - request.GetNode().GetId(), request.TypeUrl, GetResourceNames(resp.Resources), resp.RemovedResources, state.IsWildcard()) + request.GetNode().GetId(), request.TypeUrl, GetResourceNames(resp.Resources), resp.RemovedResources, clientState.IsWildcard()) } select { case value <- resp: diff --git a/pkg/cache/v3/simple_test.go b/pkg/cache/v3/simple_test.go index 5fb9bbc62d..e0784cc9a4 100644 --- a/pkg/cache/v3/simple_test.go +++ b/pkg/cache/v3/simple_test.go @@ -135,7 +135,9 @@ func TestSnapshotCacheWithTTL(t *testing.T) { t.Errorf("get resources %v, want %v", out.(*cache.RawResponse).Resources, snapshotWithTTL.GetResourcesAndTTL(typ)) } // Update streamState - streamState.SetKnownResourceNamesAsList(typ, out.GetRequest().GetResourceNames()) + for _, resource := range out.GetRequest().GetResourceNames() { + streamState.GetKnownResources()[resource] = fixture.version + } case <-time.After(2 * time.Second): t.Errorf("failed to receive snapshot response") } @@ -172,7 +174,9 @@ func TestSnapshotCacheWithTTL(t *testing.T) { updatesByType[typ]++ - streamState.SetKnownResourceNamesAsList(typ, out.GetRequest().ResourceNames) + for _, resource := range out.GetRequest().GetResourceNames() { + streamState.GetKnownResources()[resource] = fixture.version + } case <-end: cancel() return @@ -298,7 +302,9 @@ func TestSnapshotCacheWatch(t *testing.T) { if !reflect.DeepEqual(cache.IndexResourcesByName(out.(*cache.RawResponse).Resources), snapshot.GetResourcesAndTTL(typ)) { t.Errorf("get resources %v, want %v", out.(*cache.RawResponse).Resources, snapshot.GetResourcesAndTTL(typ)) } - streamState.SetKnownResourceNamesAsList(typ, out.GetRequest().GetResourceNames()) + for _, resource := range out.GetRequest().GetResourceNames() { + streamState.GetKnownResources()[resource] = fixture.version + } case <-time.After(time.Second): t.Fatal("failed to receive snapshot response") } @@ -359,6 +365,7 @@ func TestConcurrentSetWatch(t *testing.T) { Node: &core.Node{Id: id}, TypeUrl: rsrc.EndpointType, }, streamState, value) + defer cancel() } }) @@ -450,8 +457,9 @@ func TestSnapshotCreateWatchWithResourcePreviouslyNotRequested(t *testing.T) { // Request resource with name=ClusterName go func() { + state := stream.NewStreamState(false, map[string]string{}) c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType, ResourceNames: []string{clusterName}}, - stream.NewStreamState(false, map[string]string{}), watch) + &state, watch) }() select { @@ -470,9 +478,9 @@ func TestSnapshotCreateWatchWithResourcePreviouslyNotRequested(t *testing.T) { // Request additional resource with name=clusterName2 for same version go func() { state := stream.NewStreamState(false, map[string]string{}) - state.SetKnownResourceNames(rsrc.EndpointType, map[string]struct{}{clusterName: {}}) + state.SetResourceVersions(map[string]string{clusterName: fixture.version}) c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType, VersionInfo: fixture.version, - ResourceNames: []string{clusterName, clusterName2}}, state, watch) + ResourceNames: []string{clusterName, clusterName2}}, &state, watch) }() select { @@ -489,9 +497,9 @@ func TestSnapshotCreateWatchWithResourcePreviouslyNotRequested(t *testing.T) { // Repeat request for with same version and make sure a watch is created state := stream.NewStreamState(false, map[string]string{}) - state.SetKnownResourceNames(rsrc.EndpointType, map[string]struct{}{clusterName: {}, clusterName2: {}}) + state.SetResourceVersions(map[string]string{clusterName: fixture.version, clusterName2: fixture.version}) if cancel := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType, VersionInfo: fixture.version, - ResourceNames: []string{clusterName, clusterName2}}, state, watch); cancel == nil { + ResourceNames: []string{clusterName, clusterName2}}, &state, watch); cancel == nil { t.Fatal("Should create a watch") } else { cancel() @@ -622,7 +630,7 @@ func TestAvertPanicForWatchOnNonExistentSnapshot(t *testing.T) { } ss := stream.NewStreamState(false, map[string]string{"cluster": "abcdef"}) responder := make(chan cache.Response) - c.CreateWatch(req, ss, responder) + c.CreateWatch(req, &ss, responder) go func() { // Wait for at least one heartbeat to occur, then set snapshot. diff --git a/pkg/cache/v3/status.go b/pkg/cache/v3/status.go index 1b3e8f490b..605169d1b3 100644 --- a/pkg/cache/v3/status.go +++ b/pkg/cache/v3/status.go @@ -20,7 +20,6 @@ import ( "time" core "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" - "github.com/envoyproxy/go-control-plane/pkg/server/stream/v3" ) // NodeHash computes string identifiers for Envoy nodes. @@ -101,7 +100,23 @@ type DeltaResponseWatch struct { Response chan DeltaResponse // VersionMap for the stream - StreamState stream.StreamState + clientState ClientState +} + +// WatchesResources returns whether at least one of the resource provided is currently watch by the stream +// It is currently only applicable to delta-xds +// If the request is wildcard, it will always return true +// Otherwise it will compare the provided resources to the list of resources currently subscribed +func (w *DeltaResponseWatch) WatchesResources(resourceNames map[string]struct{}) bool { + if w.clientState.IsWildcard() { + return true + } + for resourceName := range resourceNames { + if _, ok := w.clientState.GetSubscribedResources()[resourceName]; ok { + return true + } + } + return false } // newStatusInfo initializes a status info data structure. diff --git a/pkg/server/delta/v3/server.go b/pkg/server/delta/v3/server.go index 9152ab2d96..ebadd74849 100644 --- a/pkg/server/delta/v3/server.go +++ b/pkg/server/delta/v3/server.go @@ -185,7 +185,7 @@ func (s *server) processDelta(str stream.DeltaStream, reqCh <-chan *discovery.De s.unsubscribe(req.GetResourceNamesUnsubscribe(), &watch.state) watch.responses = make(chan cache.DeltaResponse, 1) - watch.cancel = s.cache.CreateDeltaWatch(req, watch.state, watch.responses) + watch.cancel = s.cache.CreateDeltaWatch(req, &watch.state, watch.responses) watches.deltaWatches[typeURL] = watch go func() { @@ -227,7 +227,7 @@ func (s *server) DeltaStreamHandler(str stream.DeltaStream, typeURL string) erro // When we subscribe, we just want to make the cache know we are subscribing to a resource. // Even if the stream is wildcard, we keep the list of explicitly subscribed resources as the wildcard subscription can be discarded later on. func (s *server) subscribe(resources []string, streamState *stream.StreamState) { - sv := streamState.GetSubscribedResourceNames() + sv := streamState.GetSubscribedResources() for _, resource := range resources { if resource == "*" { streamState.SetWildcard(true) @@ -240,7 +240,7 @@ func (s *server) subscribe(resources []string, streamState *stream.StreamState) // Unsubscriptions remove resources from the stream's subscribed resource list. // If a client explicitly unsubscribes from a wildcard request, the stream is updated and now watches only subscribed resources. func (s *server) unsubscribe(resources []string, streamState *stream.StreamState) { - sv := streamState.GetSubscribedResourceNames() + sv := streamState.GetSubscribedResources() for _, resource := range resources { if resource == "*" { streamState.SetWildcard(false) @@ -255,7 +255,7 @@ func (s *server) unsubscribe(resources []string, streamState *stream.StreamState // To achieve that, we mark the resource as having been returned with an empty version. While creating the response, the cache will either: // * detect the version change, and return the resource (as an update) // * detect the resource deletion, and set it as removed in the response - streamState.GetResourceVersions()[resource] = "" + streamState.GetKnownResources()[resource] = "" } delete(sv, resource) } diff --git a/pkg/server/sotw/v3/ads.go b/pkg/server/sotw/v3/ads.go index 1efb97ba4f..417d9c3689 100644 --- a/pkg/server/sotw/v3/ads.go +++ b/pkg/server/sotw/v3/ads.go @@ -101,10 +101,12 @@ func (s *server) processADS(sw *streamWrapper, reqCh chan *discovery.DiscoveryRe } } + streamState := sw.streamStates[req.TypeUrl] + if lastResponse, ok := sw.lastDiscoveryResponses[req.TypeUrl]; ok { if lastResponse.nonce == "" || lastResponse.nonce == nonce { // Let's record Resource names that a client has received. - sw.streamState.SetKnownResourceNames(req.TypeUrl, lastResponse.resources) + streamState.SetResourceVersions(lastResponse.resources) } } @@ -123,7 +125,7 @@ func (s *server) processADS(sw *streamWrapper, reqCh chan *discovery.DiscoveryRe } sw.watches.addWatch(typeURL, &watch{ - cancel: s.cache.CreateWatch(req, sw.streamState, responder), + cancel: s.cache.CreateWatch(req, streamState, responder), response: responder, }) } @@ -131,10 +133,12 @@ func (s *server) processADS(sw *streamWrapper, reqCh chan *discovery.DiscoveryRe // No pre-existing watch exists, let's create one. // We need to precompute the watches first then open a watch in the cache. sw.watches.addWatch(typeURL, &watch{ - cancel: s.cache.CreateWatch(req, sw.streamState, responder), + cancel: s.cache.CreateWatch(req, streamState, responder), response: responder, }) } + + sw.streamStates[req.TypeUrl] = streamState } } } diff --git a/pkg/server/sotw/v3/server.go b/pkg/server/sotw/v3/server.go index 0813a655c7..a5d2a257f9 100644 --- a/pkg/server/sotw/v3/server.go +++ b/pkg/server/sotw/v3/server.go @@ -91,7 +91,7 @@ type streamWrapper struct { // The below fields are used for tracking resource // cache state and should be maintained per stream. - streamState stream.StreamState + streamStates map[string]stream.StreamState lastDiscoveryResponses map[string]lastDiscoveryResponse } @@ -110,12 +110,17 @@ func (s *streamWrapper) send(resp cache.Response) (string, error) { // increment nonce and convert it to base10 out.Nonce = strconv.FormatInt(atomic.AddInt64(&s.nonce, 1), 10) + version, err := resp.GetVersion() + if err != nil { + return "", err + } + lastResponse := lastDiscoveryResponse{ nonce: out.Nonce, - resources: make(map[string]struct{}), + resources: make(map[string]string), } for _, r := range resp.GetRequest().ResourceNames { - lastResponse.resources[r] = struct{}{} + lastResponse.resources[r] = version } s.lastDiscoveryResponses[resp.GetRequest().TypeUrl] = lastResponse @@ -141,7 +146,7 @@ func (s *streamWrapper) shutdown() { // regardless current snapshot version (even if it is not changed yet) type lastDiscoveryResponse struct { nonce string - resources map[string]struct{} + resources map[string]string } // StreamHandler converts a blocking read call to channels and initiates stream processing diff --git a/pkg/server/sotw/v3/xds.go b/pkg/server/sotw/v3/xds.go index 145fba9c0f..61cc07905b 100644 --- a/pkg/server/sotw/v3/xds.go +++ b/pkg/server/sotw/v3/xds.go @@ -27,7 +27,7 @@ func (s *server) process(str stream.Stream, reqCh chan *discovery.DiscoveryReque // a collection of stack allocated watches per request type. watches: newWatches(), - streamState: stream.NewStreamState(false, map[string]string{}), + streamStates: make(map[string]stream.StreamState), lastDiscoveryResponses: make(map[string]lastDiscoveryResponse), } @@ -110,6 +110,8 @@ func (s *server) process(str stream.Stream, reqCh chan *discovery.DiscoveryReque req.TypeUrl = defaultTypeURL } + streamState := sw.streamStates[req.TypeUrl] + if s.callbacks != nil { if err := s.callbacks.OnStreamRequest(sw.ID, req); err != nil { return err @@ -119,7 +121,7 @@ func (s *server) process(str stream.Stream, reqCh chan *discovery.DiscoveryReque if lastResponse, ok := sw.lastDiscoveryResponses[req.TypeUrl]; ok { if lastResponse.nonce == "" || lastResponse.nonce == nonce { // Let's record Resource names that a client has received. - sw.streamState.SetKnownResourceNames(req.TypeUrl, lastResponse.resources) + streamState.SetResourceVersions(lastResponse.resources) } } @@ -132,7 +134,7 @@ func (s *server) process(str stream.Stream, reqCh chan *discovery.DiscoveryReque w.close() sw.watches.addWatch(typeURL, &watch{ - cancel: s.cache.CreateWatch(req, sw.streamState, responder), + cancel: s.cache.CreateWatch(req, streamState, responder), response: responder, }) } @@ -140,11 +142,13 @@ func (s *server) process(str stream.Stream, reqCh chan *discovery.DiscoveryReque // No pre-existing watch exists, let's create one. // We need to precompute the watches first then open a watch in the cache. sw.watches.addWatch(typeURL, &watch{ - cancel: s.cache.CreateWatch(req, sw.streamState, responder), + cancel: s.cache.CreateWatch(req, streamState, responder), response: responder, }) } + sw.streamStates[req.TypeUrl] = streamState + // Recompute the dynamic select cases for this stream. sw.watches.recompute(s.ctx, reqCh) default: diff --git a/pkg/server/stream/v3/stream.go b/pkg/server/stream/v3/stream.go index 1664a941e0..28dea295b5 100644 --- a/pkg/server/stream/v3/stream.go +++ b/pkg/server/stream/v3/stream.go @@ -35,12 +35,6 @@ type StreamState struct { // nolint:golint,revive // This field stores the last state sent to the client. resourceVersions map[string]string - // knownResourceNames contains resource names that a client has received previously (SOTW). - knownResourceNames map[string]map[string]struct{} - - // First indicates whether the StreamState has been modified since its creation - first bool - // Ordered indicates whether we want an ordered ADS stream or not ordered bool } @@ -51,8 +45,6 @@ func NewStreamState(wildcard bool, initialResourceVersions map[string]string) St wildcard: wildcard, subscribedResourceNames: map[string]struct{}{}, resourceVersions: initialResourceVersions, - first: true, - knownResourceNames: map[string]map[string]struct{}{}, ordered: false, // Ordered comes from the first request since that's when we discover if they want ADS } @@ -66,74 +58,32 @@ func NewStreamState(wildcard bool, initialResourceVersions map[string]string) St // GetSubscribedResourceNames returns the list of resources currently explicitly subscribed to // If the request is set to wildcard it may be empty // Currently populated only when using delta-xds -func (s *StreamState) GetSubscribedResourceNames() map[string]struct{} { +func (s StreamState) GetSubscribedResources() map[string]struct{} { return s.subscribedResourceNames } // SetSubscribedResourceNames is setting the list of resources currently explicitly subscribed to // It is decorrelated from the wildcard state of the stream // Currently used only when using delta-xds -func (s *StreamState) SetSubscribedResourceNames(subscribedResourceNames map[string]struct{}) { +func (s *StreamState) SetSubscribedResources(subscribedResourceNames map[string]struct{}) { s.subscribedResourceNames = subscribedResourceNames } -// WatchesResources returns whether at least one of the resource provided is currently watch by the stream -// It is currently only applicable to delta-xds -// If the request is wildcard, it will always return true -// Otherwise it will compare the provided resources to the list of resources currently subscribed -func (s *StreamState) WatchesResources(resourceNames map[string]struct{}) bool { - if s.IsWildcard() { - return true - } - for resourceName := range resourceNames { - if _, ok := s.subscribedResourceNames[resourceName]; ok { - return true - } - } - return false -} - -func (s *StreamState) SetWildcard(wildcard bool) { - s.wildcard = wildcard -} - -// GetResourceVersions returns a map of current resources grouped by type URL. -func (s *StreamState) GetResourceVersions() map[string]string { +func (s StreamState) GetKnownResources() map[string]string { return s.resourceVersions } // SetResourceVersions sets a list of resource versions by type URL and removes the flag // of "first" since we can safely assume another request has come through the stream. func (s *StreamState) SetResourceVersions(resourceVersions map[string]string) { - s.first = false s.resourceVersions = resourceVersions } -// IsFirst returns whether or not the state of the stream is based upon the initial request. -func (s *StreamState) IsFirst() bool { - return s.first +func (s *StreamState) SetWildcard(wildcard bool) { + s.wildcard = wildcard } // IsWildcard returns whether or not an xDS client requested in wildcard mode on the initial request. -func (s *StreamState) IsWildcard() bool { +func (s StreamState) IsWildcard() bool { return s.wildcard } - -// GetKnownResourceNames returns the current known list of resources on a SOTW stream. -func (s *StreamState) GetKnownResourceNames(url string) map[string]struct{} { - return s.knownResourceNames[url] -} - -// SetKnownResourceNames sets a list of resource names in a stream utilizing the SOTW protocol. -func (s *StreamState) SetKnownResourceNames(url string, names map[string]struct{}) { - s.knownResourceNames[url] = names -} - -// SetKnownResourceNamesAsList is a helper function to set resource names as a slice input. -func (s *StreamState) SetKnownResourceNamesAsList(url string, names []string) { - m := map[string]struct{}{} - for _, name := range names { - m[name] = struct{}{} - } - s.knownResourceNames[url] = m -} diff --git a/pkg/server/v3/delta_test.go b/pkg/server/v3/delta_test.go index f8429f2997..99a8039f28 100644 --- a/pkg/server/v3/delta_test.go +++ b/pkg/server/v3/delta_test.go @@ -15,12 +15,11 @@ import ( "github.com/envoyproxy/go-control-plane/pkg/cache/types" "github.com/envoyproxy/go-control-plane/pkg/cache/v3" rsrc "github.com/envoyproxy/go-control-plane/pkg/resource/v3" - "github.com/envoyproxy/go-control-plane/pkg/server/stream/v3" "github.com/envoyproxy/go-control-plane/pkg/server/v3" "github.com/envoyproxy/go-control-plane/pkg/test/resource/v3" ) -func (config *mockConfigWatcher) CreateDeltaWatch(req *discovery.DeltaDiscoveryRequest, state stream.StreamState, out chan cache.DeltaResponse) func() { +func (config *mockConfigWatcher) CreateDeltaWatch(req *discovery.DeltaDiscoveryRequest, state cache.ClientState, out chan cache.DeltaResponse) func() { config.deltaCounts[req.TypeUrl] = config.deltaCounts[req.TypeUrl] + 1 // This is duplicated from pkg/cache/v3/delta.go as private there @@ -37,7 +36,7 @@ func (config *mockConfigWatcher) CreateDeltaWatch(req *discovery.DeltaDiscoveryR // If we are handling a wildcard request, we want to respond with all resources switch { case state.IsWildcard(): - if len(state.GetResourceVersions()) == 0 { + if len(state.GetKnownResources()) == 0 { filtered = make([]types.Resource, 0, len(resourceMap)) } nextVersionMap = make(map[string]string, len(resourceMap)) @@ -46,24 +45,24 @@ func (config *mockConfigWatcher) CreateDeltaWatch(req *discovery.DeltaDiscoveryR // we can just set it here to be used for comparison later version := versionMap[name] nextVersionMap[name] = version - prevVersion, found := state.GetResourceVersions()[name] + prevVersion, found := state.GetKnownResources()[name] if !found || (prevVersion != version) { filtered = append(filtered, r) } } // Compute resources for removal - for name := range state.GetResourceVersions() { + for name := range state.GetKnownResources() { if _, ok := resourceMap[name]; !ok { toRemove = append(toRemove, name) } } default: - nextVersionMap = make(map[string]string, len(state.GetSubscribedResourceNames())) + nextVersionMap = make(map[string]string, len(state.GetSubscribedResources())) // state.GetResourceVersions() may include resources no longer subscribed // In the current code this gets silently cleaned when updating the version map - for name := range state.GetSubscribedResourceNames() { - prevVersion, found := state.GetResourceVersions()[name] + for name := range state.GetSubscribedResources() { + prevVersion, found := state.GetKnownResources()[name] if r, ok := resourceMap[name]; ok { nextVersion := versionMap[name] if prevVersion != nextVersion { diff --git a/pkg/server/v3/server_test.go b/pkg/server/v3/server_test.go index 4f281cf15c..b8cac11cfd 100644 --- a/pkg/server/v3/server_test.go +++ b/pkg/server/v3/server_test.go @@ -33,7 +33,6 @@ import ( "github.com/envoyproxy/go-control-plane/pkg/cache/v3" rsrc "github.com/envoyproxy/go-control-plane/pkg/resource/v3" "github.com/envoyproxy/go-control-plane/pkg/server/sotw/v3" - "github.com/envoyproxy/go-control-plane/pkg/server/stream/v3" "github.com/envoyproxy/go-control-plane/pkg/server/v3" "github.com/envoyproxy/go-control-plane/pkg/test/resource/v3" ) @@ -49,7 +48,7 @@ type mockConfigWatcher struct { mu *sync.RWMutex } -func (config *mockConfigWatcher) CreateWatch(req *discovery.DiscoveryRequest, _ stream.StreamState, out chan cache.Response) func() { +func (config *mockConfigWatcher) CreateWatch(req *discovery.DiscoveryRequest, state cache.ClientState, out chan cache.Response) func() { config.counts[req.TypeUrl] = config.counts[req.TypeUrl] + 1 if len(config.responses[req.TypeUrl]) > 0 { out <- config.responses[req.TypeUrl][0] From 06b24bf202d54df1b793f9d232df1bb3dbfab8ca Mon Sep 17 00:00:00 2001 From: Valerian Roche Date: Wed, 24 Aug 2022 23:09:50 -0400 Subject: [PATCH 2/5] Fix unittest update wrongly inverted an assert Signed-off-by: Valerian Roche --- pkg/cache/v3/delta_test.go | 20 +++++++++++++------- pkg/cache/v3/simple_test.go | 20 ++++++++++++++++---- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/pkg/cache/v3/delta_test.go b/pkg/cache/v3/delta_test.go index c4e30dd04a..52bccda4b5 100644 --- a/pkg/cache/v3/delta_test.go +++ b/pkg/cache/v3/delta_test.go @@ -113,6 +113,7 @@ func TestDeltaRemoveResources(t *testing.T) { watches := make(map[string]chan cache.DeltaResponse) streams := make(map[string]*stream.StreamState) + // At this stage the cache is empty, so a watch is opened for _, typ := range testTypes { watches[typ] = make(chan cache.DeltaResponse, 1) state := stream.NewStreamState(true, make(map[string]string)) @@ -127,7 +128,12 @@ func TestDeltaRemoveResources(t *testing.T) { }, *streams[typ], watches[typ]) } - require.NoError(t, c.SetSnapshot(context.Background(), key, fixture.snapshot())) + snapshot := fixture.snapshot() + snapshot.Resources[types.Endpoint] = cache.NewResources(fixture.version, []types.Resource{ + testEndpoint, + resource.MakeEndpoint("otherCluster", 8080), + }) + require.NoError(t, c.SetSnapshot(context.Background(), key, snapshot)) snapshot := fixture.snapshot() for _, typ := range testTypes { @@ -158,19 +164,19 @@ func TestDeltaRemoveResources(t *testing.T) { assert.Equal(t, len(testTypes), c.GetStatusInfo(key).GetNumDeltaWatches(), "watches should be created for the latest version") - // set a partially versioned snapshot with no endpoints + // set a partially versioned snapshot with only one endpoint snapshot2 := fixture.snapshot() - snapshot2.Resources[types.Endpoint] = cache.NewResources(fixture.version2, []types.Resource{}) + snapshot2.Resources[types.Endpoint] = cache.NewResources(fixture.version2, []types.Resource{ + testEndpoint, // this cluster is not changed, we do not expect it back in "resources" + }) require.NoError(t, c.SetSnapshot(context.Background(), key, snapshot2)) // validate response for endpoints select { case out := <-watches[testTypes[0]]: - snapshot2 := fixture.snapshot() - snapshot2.Resources[types.Endpoint] = cache.NewResources(fixture.version2, []types.Resource{}) - assertResourceMapEqual(t, cache.IndexRawResourcesByName(out.(*cache.RawDeltaResponse).Resources), snapshot2.GetResources(rsrc.EndpointType)) + assert.Empty(t, out.(*cache.RawDeltaResponse).Resources) + assert.Equal(t, []string{"otherCluster"}, out.(*cache.RawDeltaResponse).RemovedResources) nextVersionMap := out.GetNextVersionMap() - // make sure the version maps are different since we no longer are tracking any endpoint resources assert.NotEqual(t, nextVersionMap, streams[testTypes[0]].GetKnownResources(), "versionMap for the endpoint resource type did not change") case <-time.After(time.Second): diff --git a/pkg/cache/v3/simple_test.go b/pkg/cache/v3/simple_test.go index e0784cc9a4..0e11e4f093 100644 --- a/pkg/cache/v3/simple_test.go +++ b/pkg/cache/v3/simple_test.go @@ -91,10 +91,22 @@ type logger struct { t *testing.T } -func (log logger) Debugf(format string, args ...interface{}) { log.t.Logf(format, args...) } -func (log logger) Infof(format string, args ...interface{}) { log.t.Logf(format, args...) } -func (log logger) Warnf(format string, args ...interface{}) { log.t.Logf(format, args...) } -func (log logger) Errorf(format string, args ...interface{}) { log.t.Logf(format, args...) } +func (log logger) Debugf(format string, args ...interface{}) { + log.t.Helper() + log.t.Logf(format, args...) +} +func (log logger) Infof(format string, args ...interface{}) { + log.t.Helper() + log.t.Logf(format, args...) +} +func (log logger) Warnf(format string, args ...interface{}) { + log.t.Helper() + log.t.Logf(format, args...) +} +func (log logger) Errorf(format string, args ...interface{}) { + log.t.Helper() + log.t.Logf(format, args...) +} func TestSnapshotCacheWithTTL(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) From 4fe6c1ab86acf3a22c5dc1f1b2d4531be74f0435 Mon Sep 17 00:00:00 2001 From: Valerian Roche Date: Tue, 1 Nov 2022 17:44:11 -0400 Subject: [PATCH 3/5] Reword comments on new ClientState and watch methods Signed-off-by: Valerian Roche --- pkg/cache/v3/cache.go | 4 ++-- pkg/cache/v3/delta_test.go | 1 - pkg/cache/v3/status.go | 8 ++++---- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/pkg/cache/v3/cache.go b/pkg/cache/v3/cache.go index c3754b60d4..6174073802 100644 --- a/pkg/cache/v3/cache.go +++ b/pkg/cache/v3/cache.go @@ -48,8 +48,8 @@ type ClientState interface { GetKnownResources() map[string]string // GetSubscribedResources returns the list of resources currently subscribed to by the client for the type. - // For delta it keeps track across requests - // For sotw it is a normalized view of the request resources + // For delta it keeps track of subscription updates across requests + // For sotw it is a normalized view of the last request resources GetSubscribedResources() map[string]struct{} // IsWildcard returns whether the client has a wildcard watch. diff --git a/pkg/cache/v3/delta_test.go b/pkg/cache/v3/delta_test.go index 52bccda4b5..d423ce0465 100644 --- a/pkg/cache/v3/delta_test.go +++ b/pkg/cache/v3/delta_test.go @@ -135,7 +135,6 @@ func TestDeltaRemoveResources(t *testing.T) { }) require.NoError(t, c.SetSnapshot(context.Background(), key, snapshot)) - snapshot := fixture.snapshot() for _, typ := range testTypes { t.Run(typ, func(t *testing.T) { select { diff --git a/pkg/cache/v3/status.go b/pkg/cache/v3/status.go index 605169d1b3..dd867f88c7 100644 --- a/pkg/cache/v3/status.go +++ b/pkg/cache/v3/status.go @@ -103,10 +103,10 @@ type DeltaResponseWatch struct { clientState ClientState } -// WatchesResources returns whether at least one of the resource provided is currently watch by the stream -// It is currently only applicable to delta-xds -// If the request is wildcard, it will always return true -// Otherwise it will compare the provided resources to the list of resources currently subscribed +// WatchesResources returns whether at least one of the resources provided is currently being watched by the stream. +// It is currently only applicable to delta-xds. +// If the request is wildcard, it will always return true, +// otherwise it will compare the provided resources to the list of resources currently subscribed func (w *DeltaResponseWatch) WatchesResources(resourceNames map[string]struct{}) bool { if w.clientState.IsWildcard() { return true From db72c1810d4b9f16310f4d88366fb69049ff6381 Mon Sep 17 00:00:00 2001 From: Valerian Roche Date: Tue, 6 Jun 2023 23:25:29 -0400 Subject: [PATCH 4/5] Rename ClientState to SubscriptionState. Ensure WatchesResources is also compatible with sotw Signed-off-by: Valerian Roche --- pkg/cache/v3/cache.go | 19 ++- pkg/cache/v3/delta.go | 2 +- pkg/cache/v3/delta_test.go | 39 +++-- pkg/cache/v3/linear.go | 25 +-- pkg/cache/v3/linear_test.go | 225 ++++++++++++++++----------- pkg/cache/v3/mux.go | 9 +- pkg/cache/v3/simple.go | 24 +-- pkg/cache/v3/simple_test.go | 68 ++++---- pkg/cache/v3/status.go | 18 +-- pkg/server/delta/v3/server.go | 14 +- pkg/server/delta/v3/watches.go | 2 +- pkg/server/sotw/v3/ads.go | 27 +++- pkg/server/sotw/v3/server.go | 2 +- pkg/server/sotw/v3/xds.go | 51 +++++- pkg/server/stream/v3/stream.go | 66 -------- pkg/server/stream/v3/subscription.go | 81 ++++++++++ pkg/server/v3/delta_test.go | 6 +- pkg/server/v3/server_test.go | 6 +- 18 files changed, 413 insertions(+), 271 deletions(-) create mode 100644 pkg/server/stream/v3/subscription.go diff --git a/pkg/cache/v3/cache.go b/pkg/cache/v3/cache.go index 6174073802..b26e0f5bc8 100644 --- a/pkg/cache/v3/cache.go +++ b/pkg/cache/v3/cache.go @@ -36,12 +36,12 @@ type Request = discovery.DiscoveryRequest // DeltaRequest is an alias for the delta discovery request type. type DeltaRequest = discovery.DeltaDiscoveryRequest -// ClientState provides additional data on the client knowledge for the type matching the request +// SubscriptionState provides additional data on the client knowledge for the type matching the request // This allows proper implementation of stateful aspects of the protocol (e.g. returning only some updated resources) // Though the methods may return mutable parts of the state for performance reasons, // the cache is expected to consider this state as immutable and thread safe between a watch creation and its cancellation -type ClientState interface { - // GetKnownResources returns the list of resources the clients has ACKed and their associated version. +type SubscriptionState interface { + // GetKnownResources returns a list of resources that the client has ACK'd and their associated version. // The versions are: // - delta protocol: version of the specific resource set in the response // - sotw protocol: version of the global response when the resource was last ACKed @@ -53,8 +53,15 @@ type ClientState interface { GetSubscribedResources() map[string]struct{} // IsWildcard returns whether the client has a wildcard watch. - // This considers subtilities related to the current migration of wildcard definition within the protocol. + // This considers subtleties related to the current migration of wildcard definitions within the protocol. + // More details on the behavior of wildcard are present at https://www.envoyproxy.io/docs/envoy/latest/api-docs/xds_protocol#how-the-client-specifies-what-resources-to-return IsWildcard() bool + + // WatchesResources returns whether at least one of the resources provided is currently being watched by the subscription. + // It is currently only applicable to delta-xds. + // If the request is wildcard, it will always return true, + // otherwise it will compare the provided resources to the list of resources currently subscribed + WatchesResources(resourceNames map[string]struct{}) bool } // ConfigWatcher requests watches for configuration resources by a node, last @@ -74,7 +81,7 @@ type ConfigWatcher interface { // // Cancel is an optional function to release resources in the producer. If // provided, the consumer may call this function multiple times. - CreateWatch(*Request, ClientState, chan Response) (cancel func()) + CreateWatch(*Request, SubscriptionState, chan Response) (cancel func(), err error) // CreateDeltaWatch returns a new open incremental xDS watch. // This is the entrypoint to propagate configuration changes the @@ -86,7 +93,7 @@ type ConfigWatcher interface { // // Cancel is an optional function to release resources in the producer. If // provided, the consumer may call this function multiple times. - CreateDeltaWatch(*DeltaRequest, ClientState, chan DeltaResponse) (cancel func()) + CreateDeltaWatch(*DeltaRequest, SubscriptionState, chan DeltaResponse) (cancel func(), err error) } // ConfigFetcher fetches configuration resources from cache diff --git a/pkg/cache/v3/delta.go b/pkg/cache/v3/delta.go index c5adf9fb8b..9dc3f87127 100644 --- a/pkg/cache/v3/delta.go +++ b/pkg/cache/v3/delta.go @@ -27,7 +27,7 @@ type resourceContainer struct { systemVersion string } -func createDeltaResponse(ctx context.Context, req *DeltaRequest, state ClientState, resources resourceContainer) *RawDeltaResponse { +func createDeltaResponse(ctx context.Context, req *DeltaRequest, state SubscriptionState, resources resourceContainer) *RawDeltaResponse { // variables to build our response with var nextVersionMap map[string]string var filtered []types.Resource diff --git a/pkg/cache/v3/delta_test.go b/pkg/cache/v3/delta_test.go index d423ce0465..25dddd6b29 100644 --- a/pkg/cache/v3/delta_test.go +++ b/pkg/cache/v3/delta_test.go @@ -35,14 +35,15 @@ func TestSnapshotCacheDeltaWatch(t *testing.T) { // Make our initial request as a wildcard to get all resources and make sure the wildcard requesting works as intended for _, typ := range testTypes { watches[typ] = make(chan cache.DeltaResponse, 1) - state := stream.NewStreamState(true, nil) - c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ + state := stream.NewSubscriptionState(true, nil) + _, err := c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ Node: &core.Node{ Id: "node", }, TypeUrl: typ, ResourceNamesSubscribe: names[typ], }, state, watches[typ]) + require.NoError(t, err) } if err := c.SetSnapshot(context.Background(), key, fixture.snapshot()); err != nil { @@ -68,17 +69,18 @@ func TestSnapshotCacheDeltaWatch(t *testing.T) { // all resources as well as individual resource removals for _, typ := range testTypes { watches[typ] = make(chan cache.DeltaResponse, 1) - state := stream.NewStreamState(false, versionMap[typ]) + state := stream.NewSubscriptionState(false, versionMap[typ]) for resource := range versionMap[typ] { state.GetSubscribedResources()[resource] = struct{}{} } - c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ + _, err := c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ Node: &core.Node{ Id: "node", }, TypeUrl: typ, ResourceNamesSubscribe: names[typ], }, state, watches[typ]) + require.NoError(t, err) } if count := c.GetStatusInfo(key).GetNumDeltaWatches(); count != len(testTypes) { @@ -111,21 +113,22 @@ func TestSnapshotCacheDeltaWatch(t *testing.T) { func TestDeltaRemoveResources(t *testing.T) { c := cache.NewSnapshotCache(false, group{}, logger{t: t}) watches := make(map[string]chan cache.DeltaResponse) - streams := make(map[string]*stream.StreamState) + streams := make(map[string]*stream.SubscriptionState) // At this stage the cache is empty, so a watch is opened for _, typ := range testTypes { watches[typ] = make(chan cache.DeltaResponse, 1) - state := stream.NewStreamState(true, make(map[string]string)) + state := stream.NewSubscriptionState(true, make(map[string]string)) streams[typ] = &state // We don't specify any resource name subscriptions here because we want to make sure we test wildcard // functionality. This means we should receive all resources back without requesting a subscription by name. - c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ + _, err := c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ Node: &core.Node{ Id: "node", }, TypeUrl: typ, }, *streams[typ], watches[typ]) + require.NoError(t, err) } snapshot := fixture.snapshot() @@ -141,7 +144,7 @@ func TestDeltaRemoveResources(t *testing.T) { case out := <-watches[typ]: assertResourceMapEqual(t, cache.IndexRawResourcesByName(out.(*cache.RawDeltaResponse).Resources), snapshot.GetResources(typ)) nextVersionMap := out.GetNextVersionMap() - streams[typ].SetResourceVersions(nextVersionMap) + streams[typ].SetKnownResources(nextVersionMap) case <-time.After(time.Second): require.Fail(t, "failed to receive a snapshot response") } @@ -152,13 +155,14 @@ func TestDeltaRemoveResources(t *testing.T) { // test the removal of certain resources from a partial snapshot for _, typ := range testTypes { watches[typ] = make(chan cache.DeltaResponse, 1) - c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ + _, err := c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ Node: &core.Node{ Id: "node", }, TypeUrl: typ, ResponseNonce: "nonce", }, *streams[typ], watches[typ]) + require.NoError(t, err) } assert.Equal(t, len(testTypes), c.GetStatusInfo(key).GetNumDeltaWatches(), "watches should be created for the latest version") @@ -202,14 +206,15 @@ func TestConcurrentSetDeltaWatch(t *testing.T) { t.Fatalf("snapshot failed: %s", err) } } else { - state := stream.NewStreamState(false, make(map[string]string)) - cancel := c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ + state := stream.NewSubscriptionState(false, make(map[string]string)) + cancel, err := c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ Node: &core.Node{ Id: id, }, TypeUrl: rsrc.EndpointType, ResourceNamesSubscribe: []string{clusterName}, }, state, responses) + require.NoError(t, err) defer cancel() } @@ -225,21 +230,22 @@ func TestSnapshotDeltaCacheWatchTimeout(t *testing.T) { // Create a non-buffered channel that will block sends. watchCh := make(chan cache.DeltaResponse) - state := stream.NewStreamState(false, nil) + state := stream.NewSubscriptionState(false, nil) state.SetSubscribedResources(map[string]struct{}{names[rsrc.EndpointType][0]: {}}) - c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ + _, err := c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ Node: &core.Node{ Id: key, }, TypeUrl: rsrc.EndpointType, ResourceNamesSubscribe: names[rsrc.EndpointType], }, state, watchCh) + require.NoError(t, err) // The first time we set the snapshot without consuming from the blocking channel, so this should time out. ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond) defer cancel() - err := c.SetSnapshot(ctx, key, fixture.snapshot()) + err = c.SetSnapshot(ctx, key, fixture.snapshot()) assert.EqualError(t, err, context.Canceled.Error()) // Now reset the snapshot with a consuming channel. This verifies that if setting the snapshot fails, @@ -269,14 +275,15 @@ func TestSnapshotCacheDeltaWatchCancel(t *testing.T) { c := cache.NewSnapshotCache(true, group{}, logger{t: t}) for _, typ := range testTypes { responses := make(chan cache.DeltaResponse, 1) - state := stream.NewStreamState(false, make(map[string]string)) - cancel := c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ + state := stream.NewSubscriptionState(false, make(map[string]string)) + cancel, err := c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{ Node: &core.Node{ Id: key, }, TypeUrl: typ, ResourceNamesSubscribe: names[typ], }, state, responses) + require.NoError(t, err) // Cancel the watch cancel() diff --git a/pkg/cache/v3/linear.go b/pkg/cache/v3/linear.go index 33385b2959..f1c8fcac28 100644 --- a/pkg/cache/v3/linear.go +++ b/pkg/cache/v3/linear.go @@ -17,6 +17,7 @@ package cache import ( "context" "errors" + "fmt" "strconv" "strings" "sync" @@ -163,11 +164,11 @@ func (cache *LinearCache) notifyAll(modified map[string]struct{}) { } for id, watch := range cache.deltaWatches { - if !watch.WatchesResources(modified) { + if !watch.subscriptionState.WatchesResources(modified) { continue } - res := cache.respondDelta(watch.Request, watch.Response, watch.clientState) + res := cache.respondDelta(watch.Request, watch.Response, watch.subscriptionState) if res != nil { delete(cache.deltaWatches, id) } @@ -175,7 +176,7 @@ func (cache *LinearCache) notifyAll(modified map[string]struct{}) { } } -func (cache *LinearCache) respondDelta(request *DeltaRequest, value chan DeltaResponse, clientState ClientState) *RawDeltaResponse { +func (cache *LinearCache) respondDelta(request *DeltaRequest, value chan DeltaResponse, clientState SubscriptionState) *RawDeltaResponse { resp := createDeltaResponse(context.Background(), request, clientState, resourceContainer{ resourceMap: cache.resources, versionMap: cache.versionMap, @@ -297,10 +298,10 @@ func (cache *LinearCache) GetResources() map[string]types.Resource { return resources } -func (cache *LinearCache) CreateWatch(request *Request, clientState ClientState, value chan Response) func() { +func (cache *LinearCache) CreateWatch(request *Request, _ SubscriptionState, value chan Response) (func(), error) { if request.TypeUrl != cache.typeURL { value <- nil - return nil + return nil, fmt.Errorf("request type %s does not match cache type %s", request.TypeUrl, cache.typeURL) } // If the version is not up to date, check whether any requested resource has // been updated between the last version and the current version. This avoids the problem @@ -336,7 +337,7 @@ func (cache *LinearCache) CreateWatch(request *Request, clientState ClientState, } if stale { cache.respond(value, staleResources) - return nil + return nil, nil } // Create open watches since versions are up to date. if len(request.ResourceNames) == 0 { @@ -345,7 +346,7 @@ func (cache *LinearCache) CreateWatch(request *Request, clientState ClientState, cache.mu.Lock() defer cache.mu.Unlock() delete(cache.watchAll, value) - } + }, nil } for _, name := range request.ResourceNames { set, exists := cache.watches[name] @@ -367,10 +368,10 @@ func (cache *LinearCache) CreateWatch(request *Request, clientState ClientState, delete(cache.watches, name) } } - } + }, nil } -func (cache *LinearCache) CreateDeltaWatch(request *DeltaRequest, clientState ClientState, value chan DeltaResponse) func() { +func (cache *LinearCache) CreateDeltaWatch(request *DeltaRequest, clientState SubscriptionState, value chan DeltaResponse) (func(), error) { cache.mu.Lock() defer cache.mu.Unlock() @@ -398,12 +399,12 @@ func (cache *LinearCache) CreateDeltaWatch(request *DeltaRequest, clientState Cl cache.typeURL, clientState.GetSubscribedResources(), cache.getVersion()) } - cache.deltaWatches[watchID] = DeltaResponseWatch{Request: request, Response: value, clientState: clientState} + cache.deltaWatches[watchID] = DeltaResponseWatch{Request: request, Response: value, subscriptionState: clientState} - return cache.cancelDeltaWatch(watchID) + return cache.cancelDeltaWatch(watchID), nil } - return nil + return nil, nil } func (cache *LinearCache) updateVersionMap(modified map[string]struct{}) error { diff --git a/pkg/cache/v3/linear_test.go b/pkg/cache/v3/linear_test.go index b5f9a0183a..51a67882fb 100644 --- a/pkg/cache/v3/linear_test.go +++ b/pkg/cache/v3/linear_test.go @@ -189,57 +189,62 @@ func hashResource(t *testing.T, resource types.Resource) string { return v } -func createWildcardDeltaWatch(c *LinearCache, w chan DeltaResponse) { - state := stream.NewStreamState(true, nil) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) +func createWildcardDeltaWatch(c *LinearCache, w chan DeltaResponse) error { + state := stream.NewSubscriptionState(true, nil) + if _, err := c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w); err != nil { + return err + } resp := <-w - state.SetResourceVersions(resp.GetNextVersionMap()) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) // Ensure the watch is set properly with cache values + state.SetKnownResources(resp.GetNextVersionMap()) + _, err := c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) // Ensure the watch is set properly with cache values + return err } func TestLinearInitialResources(t *testing.T) { - streamState := stream.NewStreamState(false, map[string]string{}) + streamState := stream.NewSubscriptionState(false, map[string]string{}) c := NewLinearCache(testType, WithInitialResources(map[string]types.Resource{"a": testResource("a"), "b": testResource("b")})) w := make(chan Response, 1) - c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType}, streamState, w) + _, err := c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType}, streamState, w) + require.NoError(t, err) verifyResponse(t, w, "0", 1) - c.CreateWatch(&Request{TypeUrl: testType}, streamState, w) + _, err = c.CreateWatch(&Request{TypeUrl: testType}, streamState, w) + require.NoError(t, err) verifyResponse(t, w, "0", 2) checkVersionMapNotSet(t, c) } func TestLinearCornerCases(t *testing.T) { - streamState := stream.NewStreamState(false, map[string]string{}) + streamState := stream.NewSubscriptionState(false, map[string]string{}) c := NewLinearCache(testType) err := c.UpdateResource("a", nil) - if err == nil { - t.Error("expected error on nil resource") - } + assert.Error(t, err, "expected error on nil resource") + // create an incorrect type URL request w := make(chan Response, 1) - c.CreateWatch(&Request{TypeUrl: "test"}, streamState, w) + _, err = c.CreateWatch(&Request{TypeUrl: "test"}, streamState, w) + assert.Error(t, err, "watch should fail to be created") select { case r := <-w: - if r != nil { - t.Error("response should be nil") - } + assert.Nil(t, r, "response should be nil") default: - t.Error("should receive nil response") + assert.Fail(t, "should receive nil response") } } func TestLinearBasic(t *testing.T) { - streamState := stream.NewStreamState(false, map[string]string{}) + streamState := stream.NewSubscriptionState(false, map[string]string{}) c := NewLinearCache(testType) // Create watches before a resource is ready w1 := make(chan Response, 1) - c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w1) + _, err := c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w1) + require.NoError(t, err) mustBlock(t, w1) checkVersionMapNotSet(t, c) w := make(chan Response, 1) - c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "0"}, streamState, w) + _, err = c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "0"}, streamState, w) + require.NoError(t, err) mustBlock(t, w) checkWatchCount(t, c, "a", 2) checkWatchCount(t, c, "b", 1) @@ -250,34 +255,40 @@ func TestLinearBasic(t *testing.T) { verifyResponse(t, w, "1", 1) // Request again, should get same response - c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w) + _, err = c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w) + require.NoError(t, err) checkWatchCount(t, c, "a", 0) verifyResponse(t, w, "1", 1) - c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "0"}, streamState, w) + _, err = c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "0"}, streamState, w) + require.NoError(t, err) checkWatchCount(t, c, "a", 0) verifyResponse(t, w, "1", 1) // Add another element and update the first, response should be different require.NoError(t, c.UpdateResource("b", testResource("b"))) require.NoError(t, c.UpdateResource("a", testResource("aa"))) - c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w) + _, err = c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w) + require.NoError(t, err) verifyResponse(t, w, "3", 1) - c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "0"}, streamState, w) + _, err = c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "0"}, streamState, w) + require.NoError(t, err) verifyResponse(t, w, "3", 2) // Ensure the version map was not created as we only ever used stow watches checkVersionMapNotSet(t, c) } func TestLinearSetResources(t *testing.T) { - streamState := stream.NewStreamState(false, map[string]string{}) + streamState := stream.NewSubscriptionState(false, map[string]string{}) c := NewLinearCache(testType) // Create new resources w1 := make(chan Response, 1) - c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w1) + _, err := c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w1) + require.NoError(t, err) mustBlock(t, w1) w2 := make(chan Response, 1) - c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "0"}, streamState, w2) + _, err = c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "0"}, streamState, w2) + require.NoError(t, err) mustBlock(t, w2) c.SetResources(map[string]types.Resource{ "a": testResource("a"), @@ -287,9 +298,11 @@ func TestLinearSetResources(t *testing.T) { verifyResponse(t, w2, "1", 2) // the version was only incremented once for all resources // Add another element and update the first, response should be different - c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "1"}, streamState, w1) + _, err = c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "1"}, streamState, w1) + require.NoError(t, err) mustBlock(t, w1) - c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "1"}, streamState, w2) + _, err = c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "1"}, streamState, w2) + require.NoError(t, err) mustBlock(t, w2) c.SetResources(map[string]types.Resource{ "a": testResource("aa"), @@ -300,9 +313,11 @@ func TestLinearSetResources(t *testing.T) { verifyResponse(t, w2, "2", 3) // Delete resource - c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "2"}, streamState, w1) + _, err = c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "2"}, streamState, w1) + require.NoError(t, err) mustBlock(t, w1) - c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "2"}, streamState, w2) + _, err = c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "2"}, streamState, w2) + require.NoError(t, err) mustBlock(t, w2) c.SetResources(map[string]types.Resource{ "b": testResource("b"), @@ -330,49 +345,57 @@ func TestLinearGetResources(t *testing.T) { } func TestLinearVersionPrefix(t *testing.T) { - streamState := stream.NewStreamState(false, map[string]string{}) + streamState := stream.NewSubscriptionState(false, map[string]string{}) c := NewLinearCache(testType, WithVersionPrefix("instance1-")) w := make(chan Response, 1) - c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w) + _, err := c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w) + require.NoError(t, err) verifyResponse(t, w, "instance1-0", 0) require.NoError(t, c.UpdateResource("a", testResource("a"))) - c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w) + _, err = c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w) + require.NoError(t, err) verifyResponse(t, w, "instance1-1", 1) - c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "instance1-1"}, streamState, w) + _, err = c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "instance1-1"}, streamState, w) + require.NoError(t, err) mustBlock(t, w) checkWatchCount(t, c, "a", 1) } func TestLinearDeletion(t *testing.T) { - streamState := stream.NewStreamState(false, map[string]string{}) + streamState := stream.NewSubscriptionState(false, map[string]string{}) c := NewLinearCache(testType, WithInitialResources(map[string]types.Resource{"a": testResource("a"), "b": testResource("b")})) w := make(chan Response, 1) - c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w) + _, err := c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w) + require.NoError(t, err) mustBlock(t, w) checkWatchCount(t, c, "a", 1) require.NoError(t, c.DeleteResource("a")) verifyResponse(t, w, "1", 0) checkWatchCount(t, c, "a", 0) - c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "0"}, streamState, w) + _, err = c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "0"}, streamState, w) + require.NoError(t, err) verifyResponse(t, w, "1", 1) checkWatchCount(t, c, "b", 0) require.NoError(t, c.DeleteResource("b")) - c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "1"}, streamState, w) + _, err = c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "1"}, streamState, w) + require.NoError(t, err) verifyResponse(t, w, "2", 0) checkWatchCount(t, c, "b", 0) } func TestLinearWatchTwo(t *testing.T) { - streamState := stream.NewStreamState(false, map[string]string{}) + streamState := stream.NewSubscriptionState(false, map[string]string{}) c := NewLinearCache(testType, WithInitialResources(map[string]types.Resource{"a": testResource("a"), "b": testResource("b")})) w := make(chan Response, 1) - c.CreateWatch(&Request{ResourceNames: []string{"a", "b"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w) + _, err := c.CreateWatch(&Request{ResourceNames: []string{"a", "b"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w) + require.NoError(t, err) mustBlock(t, w) w1 := make(chan Response, 1) - c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "0"}, streamState, w1) + _, err = c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "0"}, streamState, w1) + require.NoError(t, err) mustBlock(t, w1) require.NoError(t, c.UpdateResource("a", testResource("aa"))) // should only get the modified resource @@ -381,20 +404,22 @@ func TestLinearWatchTwo(t *testing.T) { } func TestLinearCancel(t *testing.T) { - streamState := stream.NewStreamState(false, map[string]string{}) + streamState := stream.NewSubscriptionState(false, map[string]string{}) c := NewLinearCache(testType) require.NoError(t, c.UpdateResource("a", testResource("a"))) // cancel watch-all w := make(chan Response, 1) - cancel := c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "1"}, streamState, w) + cancel, err := c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "1"}, streamState, w) + require.NoError(t, err) mustBlock(t, w) checkWatchCount(t, c, "a", 1) cancel() checkWatchCount(t, c, "a", 0) // cancel watch for "a" - cancel = c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "1"}, streamState, w) + cancel, err = c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "1"}, streamState, w) + require.NoError(t, err) mustBlock(t, w) checkWatchCount(t, c, "a", 1) cancel() @@ -404,10 +429,14 @@ func TestLinearCancel(t *testing.T) { w2 := make(chan Response, 1) w3 := make(chan Response, 1) w4 := make(chan Response, 1) - cancel = c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "1"}, streamState, w) - cancel2 := c.CreateWatch(&Request{ResourceNames: []string{"b"}, TypeUrl: testType, VersionInfo: "1"}, streamState, w2) - cancel3 := c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "1"}, streamState, w3) - cancel4 := c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "1"}, streamState, w4) + cancel, err = c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "1"}, streamState, w) + require.NoError(t, err) + cancel2, err := c.CreateWatch(&Request{ResourceNames: []string{"b"}, TypeUrl: testType, VersionInfo: "1"}, streamState, w2) + require.NoError(t, err) + cancel3, err := c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "1"}, streamState, w3) + require.NoError(t, err) + cancel4, err := c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "1"}, streamState, w4) + require.NoError(t, err) mustBlock(t, w) mustBlock(t, w2) mustBlock(t, w3) @@ -429,7 +458,7 @@ func TestLinearCancel(t *testing.T) { // TODO(mattklein123): This test requires GOMAXPROCS or -parallel >= 100. This should be // rewritten to not require that. This is not the case in the GH actions environment. func TestLinearConcurrentSetWatch(t *testing.T) { - streamState := stream.NewStreamState(false, map[string]string{}) + streamState := stream.NewSubscriptionState(false, map[string]string{}) c := NewLinearCache(testType) n := 50 for i := 0; i < 2*n; i++ { @@ -444,12 +473,13 @@ func TestLinearConcurrentSetWatch(t *testing.T) { id2 := fmt.Sprintf("%d", i-1) t.Logf("request resources %q and %q", id, id2) value := make(chan Response, 1) - c.CreateWatch(&Request{ + _, err := c.CreateWatch(&Request{ // Only expect one to become stale ResourceNames: []string{id, id2}, VersionInfo: "0", TypeUrl: testType, }, streamState, value) + require.NoError(t, err) // wait until all updates apply verifyResponse(t, value, "", 1) } @@ -460,19 +490,21 @@ func TestLinearConcurrentSetWatch(t *testing.T) { func TestLinearDeltaWildcard(t *testing.T) { c := NewLinearCache(testType) - state1 := stream.NewStreamState(true, map[string]string{}) + state1 := stream.NewSubscriptionState(true, map[string]string{}) w1 := make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state1, w1) + _, err := c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state1, w1) + require.NoError(t, err) mustBlockDelta(t, w1) - state2 := stream.NewStreamState(true, map[string]string{}) + state2 := stream.NewSubscriptionState(true, map[string]string{}) w2 := make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state2, w2) + _, err = c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state2, w2) + require.NoError(t, err) mustBlockDelta(t, w1) checkDeltaWatchCount(t, c, 2) a := &endpoint.ClusterLoadAssignment{ClusterName: "a"} hash := hashResource(t, a) - err := c.UpdateResource("a", a) + err = c.UpdateResource("a", a) assert.NoError(t, err) checkDeltaWatchCount(t, c, 0) verifyDeltaResponse(t, w1, []resourceInfo{{"a", hash}}, nil) @@ -490,17 +522,19 @@ func TestLinearDeltaExistingResources(t *testing.T) { err = c.UpdateResource("b", b) assert.NoError(t, err) - state := stream.NewStreamState(false, nil) + state := stream.NewSubscriptionState(false, nil) state.SetSubscribedResources(map[string]struct{}{"b": {}, "c": {}}) // watching b and c - not interested in a w := make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + _, err = c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + require.NoError(t, err) checkDeltaWatchCount(t, c, 0) verifyDeltaResponse(t, w, []resourceInfo{{"b", hashB}}, []string{}) - state = stream.NewStreamState(false, nil) + state = stream.NewSubscriptionState(false, nil) state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w = make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + _, err = c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + require.NoError(t, err) checkDeltaWatchCount(t, c, 0) verifyDeltaResponse(t, w, []resourceInfo{{"b", hashB}, {"a", hashA}}, nil) } @@ -516,17 +550,19 @@ func TestLinearDeltaInitialResourcesVersionSet(t *testing.T) { err = c.UpdateResource("b", b) assert.NoError(t, err) - state := stream.NewStreamState(false, map[string]string{"b": hashB}) + state := stream.NewSubscriptionState(false, map[string]string{"b": hashB}) state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w := make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + _, err = c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + require.NoError(t, err) checkDeltaWatchCount(t, c, 0) verifyDeltaResponse(t, w, []resourceInfo{{"a", hashA}}, nil) // b is up to date and shouldn't be returned - state = stream.NewStreamState(false, map[string]string{"a": hashA, "b": hashB}) + state = stream.NewSubscriptionState(false, map[string]string{"a": hashA, "b": hashB}) state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w = make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + _, err = c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + require.NoError(t, err) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) b = &endpoint.ClusterLoadAssignment{ClusterName: "b", Endpoints: []*endpoint.LocalityLbEndpoints{{Priority: 10}}} // new version of b @@ -550,18 +586,20 @@ func TestLinearDeltaResourceUpdate(t *testing.T) { // There is currently no delta watch checkVersionMapNotSet(t, c) - state := stream.NewStreamState(false, nil) + state := stream.NewSubscriptionState(false, nil) state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w := make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + _, err = c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + require.NoError(t, err) checkDeltaWatchCount(t, c, 0) verifyDeltaResponse(t, w, []resourceInfo{{"b", hashB}, {"a", hashA}}, nil) checkVersionMapSet(t, c) - state = stream.NewStreamState(false, map[string]string{"a": hashA, "b": hashB}) + state = stream.NewSubscriptionState(false, map[string]string{"a": hashA, "b": hashB}) state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w = make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + _, err = c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + require.NoError(t, err) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) @@ -586,17 +624,19 @@ func TestLinearDeltaResourceDelete(t *testing.T) { err = c.UpdateResource("b", b) assert.NoError(t, err) - state := stream.NewStreamState(false, nil) + state := stream.NewSubscriptionState(false, nil) state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w := make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + _, err = c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + require.NoError(t, err) checkDeltaWatchCount(t, c, 0) verifyDeltaResponse(t, w, []resourceInfo{{"b", hashB}, {"a", hashA}}, nil) - state = stream.NewStreamState(false, map[string]string{"a": hashA, "b": hashB}) + state = stream.NewSubscriptionState(false, map[string]string{"a": hashA, "b": hashB}) state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w = make(chan DeltaResponse, 1) - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + _, err = c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + require.NoError(t, err) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) @@ -611,14 +651,15 @@ func TestLinearDeltaResourceDelete(t *testing.T) { func TestLinearDeltaMultiResourceUpdates(t *testing.T) { c := NewLinearCache(testType) - state := stream.NewStreamState(false, nil) + state := stream.NewSubscriptionState(false, nil) state.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) w := make(chan DeltaResponse, 1) checkVersionMapNotSet(t, c) assert.Equal(t, 0, c.NumResources()) // Initial update - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + _, err := c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + require.NoError(t, err) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) // The version map should now be created, even if empty @@ -627,16 +668,17 @@ func TestLinearDeltaMultiResourceUpdates(t *testing.T) { hashA := hashResource(t, a) b := &endpoint.ClusterLoadAssignment{ClusterName: "b"} hashB := hashResource(t, b) - err := c.UpdateResources(map[string]types.Resource{"a": a, "b": b}, nil) + err = c.UpdateResources(map[string]types.Resource{"a": a, "b": b}, nil) assert.NoError(t, err) resp := <-w validateDeltaResponse(t, resp, []resourceInfo{{"a", hashA}, {"b", hashB}}, nil) checkVersionMapSet(t, c) assert.Equal(t, 2, c.NumResources()) - state.SetResourceVersions(resp.GetNextVersionMap()) + state.SetKnownResources(resp.GetNextVersionMap()) // Multiple updates - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + _, err = c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + require.NoError(t, err) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) a = &endpoint.ClusterLoadAssignment{ClusterName: "a", Endpoints: []*endpoint.LocalityLbEndpoints{ //resource update @@ -653,10 +695,11 @@ func TestLinearDeltaMultiResourceUpdates(t *testing.T) { validateDeltaResponse(t, resp, []resourceInfo{{"a", hashA}, {"b", hashB}}, nil) checkVersionMapSet(t, c) assert.Equal(t, 2, c.NumResources()) - state.SetResourceVersions(resp.GetNextVersionMap()) + state.SetKnownResources(resp.GetNextVersionMap()) // Update/add/delete - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + _, err = c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + require.NoError(t, err) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) a = &endpoint.ClusterLoadAssignment{ClusterName: "a", Endpoints: []*endpoint.LocalityLbEndpoints{ //resource update @@ -672,10 +715,11 @@ func TestLinearDeltaMultiResourceUpdates(t *testing.T) { validateDeltaResponse(t, resp, []resourceInfo{{"a", hashA}}, []string{"b"}) checkVersionMapSet(t, c) assert.Equal(t, 2, c.NumResources()) - state.SetResourceVersions(resp.GetNextVersionMap()) + state.SetKnownResources(resp.GetNextVersionMap()) // Re-add previously deleted watched resource - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + _, err = c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &state, w) + require.NoError(t, err) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) b = &endpoint.ClusterLoadAssignment{ClusterName: "b", Endpoints: []*endpoint.LocalityLbEndpoints{}} // recreate watched resource @@ -688,10 +732,10 @@ func TestLinearDeltaMultiResourceUpdates(t *testing.T) { validateDeltaResponse(t, resp, []resourceInfo{{"b", hashB}}, nil) // d is not watched and should not be returned checkVersionMapSet(t, c) assert.Equal(t, 2, c.NumResources()) - state.SetResourceVersions(resp.GetNextVersionMap()) + state.SetKnownResources(resp.GetNextVersionMap()) // Wildcard create/update - createWildcardDeltaWatch(c, w) + require.NoError(t, createWildcardDeltaWatch(c, w)) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) b = &endpoint.ClusterLoadAssignment{ClusterName: "b", Endpoints: []*endpoint.LocalityLbEndpoints{ //resource update @@ -707,7 +751,7 @@ func TestLinearDeltaMultiResourceUpdates(t *testing.T) { assert.Equal(t, 3, c.NumResources()) // Wildcard update/delete - createWildcardDeltaWatch(c, w) + require.NoError(t, createWildcardDeltaWatch(c, w)) mustBlockDelta(t, w) checkDeltaWatchCount(t, c, 1) a = &endpoint.ClusterLoadAssignment{ClusterName: "a", Endpoints: []*endpoint.LocalityLbEndpoints{ //resource update @@ -736,9 +780,10 @@ func TestLinearMixedWatches(t *testing.T) { assert.NoError(t, err) assert.Equal(t, 2, c.NumResources()) - sotwState := stream.NewStreamState(false, nil) + sotwState := stream.NewSubscriptionState(false, nil) w := make(chan Response, 1) - c.CreateWatch(&Request{ResourceNames: []string{"a", "b"}, TypeUrl: testType, VersionInfo: c.getVersion()}, &sotwState, w) + _, err = c.CreateWatch(&Request{ResourceNames: []string{"a", "b"}, TypeUrl: testType, VersionInfo: c.getVersion()}, &sotwState, w) + require.NoError(t, err) mustBlock(t, w) checkVersionMapNotSet(t, c) @@ -752,16 +797,18 @@ func TestLinearMixedWatches(t *testing.T) { verifyResponse(t, w, c.getVersion(), 1) checkVersionMapNotSet(t, c) - c.CreateWatch(&Request{ResourceNames: []string{"a", "b"}, TypeUrl: testType, VersionInfo: c.getVersion()}, &sotwState, w) + _, err = c.CreateWatch(&Request{ResourceNames: []string{"a", "b"}, TypeUrl: testType, VersionInfo: c.getVersion()}, &sotwState, w) + require.NoError(t, err) mustBlock(t, w) checkVersionMapNotSet(t, c) - deltaState := stream.NewStreamState(false, map[string]string{"a": hashA, "b": hashB}) + deltaState := stream.NewSubscriptionState(false, map[string]string{"a": hashA, "b": hashB}) deltaState.SetSubscribedResources(map[string]struct{}{"a": {}, "b": {}}) wd := make(chan DeltaResponse, 1) // Initial update - c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &deltaState, wd) + _, err = c.CreateDeltaWatch(&DeltaRequest{TypeUrl: testType}, &deltaState, wd) + require.NoError(t, err) mustBlockDelta(t, wd) checkDeltaWatchCount(t, c, 1) checkVersionMapSet(t, c) diff --git a/pkg/cache/v3/mux.go b/pkg/cache/v3/mux.go index dd4c13a3e5..d4bb5791a9 100644 --- a/pkg/cache/v3/mux.go +++ b/pkg/cache/v3/mux.go @@ -17,6 +17,7 @@ package cache import ( "context" "errors" + "fmt" ) // MuxCache multiplexes across several caches using a classification function. @@ -35,22 +36,22 @@ type MuxCache struct { var _ Cache = &MuxCache{} -func (mux *MuxCache) CreateWatch(request *Request, state ClientState, value chan Response) func() { +func (mux *MuxCache) CreateWatch(request *Request, state SubscriptionState, value chan Response) (func(), error) { key := mux.Classify(request) cache, exists := mux.Caches[key] if !exists { value <- nil - return nil + return nil, fmt.Errorf("no cache defined for key %s", key) } return cache.CreateWatch(request, state, value) } -func (mux *MuxCache) CreateDeltaWatch(request *DeltaRequest, state ClientState, value chan DeltaResponse) func() { +func (mux *MuxCache) CreateDeltaWatch(request *DeltaRequest, state SubscriptionState, value chan DeltaResponse) (func(), error) { key := mux.ClassifyDelta(request) cache, exists := mux.Caches[key] if !exists { value <- nil - return nil + return nil, fmt.Errorf("no cache defined for key %s", key) } return cache.CreateDeltaWatch(request, state, value) } diff --git a/pkg/cache/v3/simple.go b/pkg/cache/v3/simple.go index 216b2e41a1..36a344ff28 100644 --- a/pkg/cache/v3/simple.go +++ b/pkg/cache/v3/simple.go @@ -286,7 +286,7 @@ func (cache *snapshotCache) SetSnapshot(ctx context.Context, node string, snapsh snapshot, watch.Request, watch.Response, - watch.clientState, + watch.subscriptionState, ) if err != nil { return err @@ -344,7 +344,7 @@ func superset(names map[string]bool, resources map[string]types.ResourceWithTTL) // CreateWatch returns a watch for an xDS request. A nil function may be // returned if an error occurs. -func (cache *snapshotCache) CreateWatch(request *Request, clientState ClientState, value chan Response) func() { +func (cache *snapshotCache) CreateWatch(request *Request, clientState SubscriptionState, value chan Response) (func(), error) { nodeID := cache.hash.ID(request.Node) cache.mu.Lock() @@ -386,9 +386,9 @@ func (cache *snapshotCache) CreateWatch(request *Request, clientState ClientStat if err := cache.respond(context.Background(), request, value, resources, version, false); err != nil { cache.log.Errorf("failed to send a response for %s%v to nodeID %q: %s", request.TypeUrl, request.ResourceNames, nodeID, err) - return nil + return nil, fmt.Errorf("failed to send the response: %w", err) } - return func() {} + return func() {}, nil } } } @@ -401,7 +401,7 @@ func (cache *snapshotCache) CreateWatch(request *Request, clientState ClientStat info.mu.Lock() info.watches[watchID] = ResponseWatch{Request: request, Response: value} info.mu.Unlock() - return cache.cancelWatch(nodeID, watchID) + return cache.cancelWatch(nodeID, watchID), nil } // otherwise, the watch may be responded immediately @@ -409,10 +409,10 @@ func (cache *snapshotCache) CreateWatch(request *Request, clientState ClientStat if err := cache.respond(context.Background(), request, value, resources, version, false); err != nil { cache.log.Errorf("failed to send a response for %s%v to nodeID %q: %s", request.TypeUrl, request.ResourceNames, nodeID, err) - return nil + return nil, fmt.Errorf("failed to send the response: %w", err) } - return func() {} + return func() {}, nil } func (cache *snapshotCache) nextWatchID() int64 { @@ -484,7 +484,7 @@ func createResponse(ctx context.Context, request *Request, resources map[string] } // CreateDeltaWatch returns a watch for a delta xDS request which implements the Simple SnapshotCache. -func (cache *snapshotCache) CreateDeltaWatch(request *DeltaRequest, clientState ClientState, value chan DeltaResponse) func() { +func (cache *snapshotCache) CreateDeltaWatch(request *DeltaRequest, clientState SubscriptionState, value chan DeltaResponse) (func(), error) { nodeID := cache.hash.ID(request.Node) t := request.GetTypeUrl() @@ -530,15 +530,15 @@ func (cache *snapshotCache) CreateDeltaWatch(request *DeltaRequest, clientState cache.log.Infof("open delta watch ID:%d for %s Resources:%v from nodeID: %q", watchID, t, clientState.GetSubscribedResources(), nodeID) } - info.setDeltaResponseWatch(watchID, DeltaResponseWatch{Request: request, Response: value, clientState: clientState}) - return cache.cancelDeltaWatch(nodeID, watchID) + info.setDeltaResponseWatch(watchID, DeltaResponseWatch{Request: request, Response: value, subscriptionState: clientState}) + return cache.cancelDeltaWatch(nodeID, watchID), nil } - return nil + return nil, nil } // Respond to a delta watch with the provided snapshot value. If the response is nil, there has been no state change. -func (cache *snapshotCache) respondDelta(ctx context.Context, snapshot ResourceSnapshot, request *DeltaRequest, value chan DeltaResponse, clientState ClientState) (*RawDeltaResponse, error) { +func (cache *snapshotCache) respondDelta(ctx context.Context, snapshot ResourceSnapshot, request *DeltaRequest, value chan DeltaResponse, clientState SubscriptionState) (*RawDeltaResponse, error) { resp := createDeltaResponse(ctx, request, clientState, resourceContainer{ resourceMap: snapshot.GetResources(request.TypeUrl), versionMap: snapshot.GetVersionMap(request.TypeUrl), diff --git a/pkg/cache/v3/simple_test.go b/pkg/cache/v3/simple_test.go index 0e11e4f093..3179df45d3 100644 --- a/pkg/cache/v3/simple_test.go +++ b/pkg/cache/v3/simple_test.go @@ -131,13 +131,14 @@ func TestSnapshotCacheWithTTL(t *testing.T) { wg := sync.WaitGroup{} // All the resources should respond immediately when version is not up to date. - streamState := stream.NewStreamState(false, map[string]string{}) + streamState := stream.NewSubscriptionState(false, map[string]string{}) for _, typ := range testTypes { wg.Add(1) t.Run(typ, func(t *testing.T) { defer wg.Done() value := make(chan cache.Response, 1) - c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ]}, streamState, value) + _, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ]}, streamState, value) + require.NoError(t, err) select { case out := <-value: if gotVersion, _ := out.GetVersion(); gotVersion != fixture.version { @@ -168,8 +169,9 @@ func TestSnapshotCacheWithTTL(t *testing.T) { end := time.After(5 * time.Second) for { value := make(chan cache.Response, 1) - cancel := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ], VersionInfo: fixture.version}, + cancel, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ], VersionInfo: fixture.version}, streamState, value) + require.NoError(t, err) select { case out := <-value: @@ -230,9 +232,11 @@ func TestSnapshotCache(t *testing.T) { // try to get endpoints with incorrect list of names // should not receive response value := make(chan cache.Response, 1) - streamState := stream.NewStreamState(false, map[string]string{}) - c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType, ResourceNames: []string{"none"}}, + streamState := stream.NewSubscriptionState(false, map[string]string{}) + _, err = c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType, ResourceNames: []string{"none"}}, streamState, value) + require.NoError(t, err) + select { case out := <-value: t.Errorf("watch for endpoints and mismatched names => got %v, want none", out) @@ -242,9 +246,10 @@ func TestSnapshotCache(t *testing.T) { for _, typ := range testTypes { t.Run(typ, func(t *testing.T) { value := make(chan cache.Response, 1) - streamState := stream.NewStreamState(false, map[string]string{}) - c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ]}, + streamState := stream.NewSubscriptionState(false, map[string]string{}) + _, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ]}, streamState, value) + require.NoError(t, err) select { case out := <-value: snapshot := fixture.snapshot() @@ -295,10 +300,11 @@ func TestSnapshotCacheFetch(t *testing.T) { func TestSnapshotCacheWatch(t *testing.T) { c := cache.NewSnapshotCache(true, group{}, logger{t: t}) watches := make(map[string]chan cache.Response) - streamState := stream.NewStreamState(false, map[string]string{}) + streamState := stream.NewSubscriptionState(false, map[string]string{}) for _, typ := range testTypes { watches[typ] = make(chan cache.Response, 1) - c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ]}, streamState, watches[typ]) + _, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ]}, streamState, watches[typ]) + require.NoError(t, err) } if err := c.SetSnapshot(context.Background(), key, fixture.snapshot()); err != nil { t.Fatal(err) @@ -326,8 +332,9 @@ func TestSnapshotCacheWatch(t *testing.T) { // open new watches with the latest version for _, typ := range testTypes { watches[typ] = make(chan cache.Response, 1) - c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ], VersionInfo: fixture.version}, + _, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ], VersionInfo: fixture.version}, streamState, watches[typ]) + require.NoError(t, err) } if count := c.GetStatusInfo(key).GetNumWatches(); count != len(testTypes) { t.Errorf("watches should be created for the latest version: %d", count) @@ -372,11 +379,12 @@ func TestConcurrentSetWatch(t *testing.T) { t.Fatalf("failed to set snapshot %q: %s", id, err) } } else { - streamState := stream.NewStreamState(false, map[string]string{}) - cancel := c.CreateWatch(&discovery.DiscoveryRequest{ + streamState := stream.NewSubscriptionState(false, map[string]string{}) + cancel, err := c.CreateWatch(&discovery.DiscoveryRequest{ Node: &core.Node{Id: id}, TypeUrl: rsrc.EndpointType, }, streamState, value) + require.NoError(t, err) defer cancel() } @@ -386,10 +394,11 @@ func TestConcurrentSetWatch(t *testing.T) { func TestSnapshotCacheWatchCancel(t *testing.T) { c := cache.NewSnapshotCache(true, group{}, logger{t: t}) - streamState := stream.NewStreamState(false, map[string]string{}) + streamState := stream.NewSubscriptionState(false, map[string]string{}) for _, typ := range testTypes { value := make(chan cache.Response, 1) - cancel := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ]}, streamState, value) + cancel, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ]}, streamState, value) + require.NoError(t, err) cancel() } // should be status info for the node @@ -413,15 +422,16 @@ func TestSnapshotCacheWatchTimeout(t *testing.T) { // Create a non-buffered channel that will block sends. watchCh := make(chan cache.Response) - streamState := stream.NewStreamState(false, map[string]string{}) - c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType, ResourceNames: names[rsrc.EndpointType]}, + streamState := stream.NewSubscriptionState(false, map[string]string{}) + _, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType, ResourceNames: names[rsrc.EndpointType]}, streamState, watchCh) + require.NoError(t, err) // The first time we set the snapshot without consuming from the blocking channel, so this should time out. ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond) defer cancel() - err := c.SetSnapshot(ctx, key, fixture.snapshot()) + err = c.SetSnapshot(ctx, key, fixture.snapshot()) assert.EqualError(t, err, context.Canceled.Error()) // Now reset the snapshot with a consuming channel. This verifies that if setting the snapshot fails, @@ -469,9 +479,10 @@ func TestSnapshotCreateWatchWithResourcePreviouslyNotRequested(t *testing.T) { // Request resource with name=ClusterName go func() { - state := stream.NewStreamState(false, map[string]string{}) - c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType, ResourceNames: []string{clusterName}}, + state := stream.NewSubscriptionState(false, map[string]string{}) + _, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType, ResourceNames: []string{clusterName}}, &state, watch) + require.NoError(t, err) }() select { @@ -489,10 +500,11 @@ func TestSnapshotCreateWatchWithResourcePreviouslyNotRequested(t *testing.T) { // Request additional resource with name=clusterName2 for same version go func() { - state := stream.NewStreamState(false, map[string]string{}) - state.SetResourceVersions(map[string]string{clusterName: fixture.version}) - c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType, VersionInfo: fixture.version, + state := stream.NewSubscriptionState(false, map[string]string{}) + state.SetKnownResources(map[string]string{clusterName: fixture.version}) + _, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType, VersionInfo: fixture.version, ResourceNames: []string{clusterName, clusterName2}}, &state, watch) + require.NoError(t, err) }() select { @@ -508,12 +520,13 @@ func TestSnapshotCreateWatchWithResourcePreviouslyNotRequested(t *testing.T) { } // Repeat request for with same version and make sure a watch is created - state := stream.NewStreamState(false, map[string]string{}) - state.SetResourceVersions(map[string]string{clusterName: fixture.version, clusterName2: fixture.version}) - if cancel := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType, VersionInfo: fixture.version, + state := stream.NewSubscriptionState(false, map[string]string{}) + state.SetKnownResources(map[string]string{clusterName: fixture.version, clusterName2: fixture.version}) + if cancel, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType, VersionInfo: fixture.version, ResourceNames: []string{clusterName, clusterName2}}, &state, watch); cancel == nil { t.Fatal("Should create a watch") } else { + require.NoError(t, err) cancel() } } @@ -640,9 +653,10 @@ func TestAvertPanicForWatchOnNonExistentSnapshot(t *testing.T) { ResourceNames: []string{"rtds"}, TypeUrl: rsrc.RuntimeType, } - ss := stream.NewStreamState(false, map[string]string{"cluster": "abcdef"}) + ss := stream.NewSubscriptionState(false, map[string]string{"cluster": "abcdef"}) responder := make(chan cache.Response) - c.CreateWatch(req, &ss, responder) + _, err := c.CreateWatch(req, &ss, responder) + require.NoError(t, err) go func() { // Wait for at least one heartbeat to occur, then set snapshot. diff --git a/pkg/cache/v3/status.go b/pkg/cache/v3/status.go index dd867f88c7..0495261e52 100644 --- a/pkg/cache/v3/status.go +++ b/pkg/cache/v3/status.go @@ -100,23 +100,7 @@ type DeltaResponseWatch struct { Response chan DeltaResponse // VersionMap for the stream - clientState ClientState -} - -// WatchesResources returns whether at least one of the resources provided is currently being watched by the stream. -// It is currently only applicable to delta-xds. -// If the request is wildcard, it will always return true, -// otherwise it will compare the provided resources to the list of resources currently subscribed -func (w *DeltaResponseWatch) WatchesResources(resourceNames map[string]struct{}) bool { - if w.clientState.IsWildcard() { - return true - } - for resourceName := range resourceNames { - if _, ok := w.clientState.GetSubscribedResources()[resourceName]; ok { - return true - } - } - return false + subscriptionState SubscriptionState } // newStatusInfo initializes a status info data structure. diff --git a/pkg/server/delta/v3/server.go b/pkg/server/delta/v3/server.go index ebadd74849..af14293f51 100644 --- a/pkg/server/delta/v3/server.go +++ b/pkg/server/delta/v3/server.go @@ -131,7 +131,7 @@ func (s *server) processDelta(str stream.DeltaStream, reqCh <-chan *discovery.De watch := watches.deltaWatches[typ] watch.nonce = nonce - watch.state.SetResourceVersions(resp.GetNextVersionMap()) + watch.state.SetKnownResources(resp.GetNextVersionMap()) watches.deltaWatches[typ] = watch case req, more := <-reqCh: // input stream ended or errored out @@ -176,7 +176,7 @@ func (s *server) processDelta(str stream.DeltaStream, reqCh <-chan *discovery.De // We also set the stream as wildcard based on its legacy meaning (no resource name sent in resource_names_subscribe). // If the state starts with this legacy mode, adding new resources will not unsubscribe from wildcard. // It can still be done by explicitly unsubscribing from "*" - watch.state = stream.NewStreamState(len(req.GetResourceNamesSubscribe()) == 0, req.GetInitialResourceVersions()) + watch.state = stream.NewSubscriptionState(len(req.GetResourceNamesSubscribe()) == 0, req.GetInitialResourceVersions()) } else { watch.Cancel() } @@ -185,7 +185,11 @@ func (s *server) processDelta(str stream.DeltaStream, reqCh <-chan *discovery.De s.unsubscribe(req.GetResourceNamesUnsubscribe(), &watch.state) watch.responses = make(chan cache.DeltaResponse, 1) - watch.cancel = s.cache.CreateDeltaWatch(req, &watch.state, watch.responses) + var err error + watch.cancel, err = s.cache.CreateDeltaWatch(req, &watch.state, watch.responses) + if err != nil { + return err + } watches.deltaWatches[typeURL] = watch go func() { @@ -226,7 +230,7 @@ func (s *server) DeltaStreamHandler(str stream.DeltaStream, typeURL string) erro // When we subscribe, we just want to make the cache know we are subscribing to a resource. // Even if the stream is wildcard, we keep the list of explicitly subscribed resources as the wildcard subscription can be discarded later on. -func (s *server) subscribe(resources []string, streamState *stream.StreamState) { +func (s *server) subscribe(resources []string, streamState *stream.SubscriptionState) { sv := streamState.GetSubscribedResources() for _, resource := range resources { if resource == "*" { @@ -239,7 +243,7 @@ func (s *server) subscribe(resources []string, streamState *stream.StreamState) // Unsubscriptions remove resources from the stream's subscribed resource list. // If a client explicitly unsubscribes from a wildcard request, the stream is updated and now watches only subscribed resources. -func (s *server) unsubscribe(resources []string, streamState *stream.StreamState) { +func (s *server) unsubscribe(resources []string, streamState *stream.SubscriptionState) { sv := streamState.GetSubscribedResources() for _, resource := range resources { if resource == "*" { diff --git a/pkg/server/delta/v3/watches.go b/pkg/server/delta/v3/watches.go index c88548388a..1712ed37dd 100644 --- a/pkg/server/delta/v3/watches.go +++ b/pkg/server/delta/v3/watches.go @@ -36,7 +36,7 @@ type watch struct { cancel func() nonce string - state stream.StreamState + state stream.SubscriptionState } // Cancel calls terminate and cancel diff --git a/pkg/server/sotw/v3/ads.go b/pkg/server/sotw/v3/ads.go index 417d9c3689..ae2ac7f40e 100644 --- a/pkg/server/sotw/v3/ads.go +++ b/pkg/server/sotw/v3/ads.go @@ -8,6 +8,7 @@ import ( "github.com/envoyproxy/go-control-plane/pkg/cache/types" "github.com/envoyproxy/go-control-plane/pkg/cache/v3" "github.com/envoyproxy/go-control-plane/pkg/resource/v3" + "github.com/envoyproxy/go-control-plane/pkg/server/stream/v3" ) // process handles a bi-di stream request @@ -101,15 +102,23 @@ func (s *server) processADS(sw *streamWrapper, reqCh chan *discovery.DiscoveryRe } } - streamState := sw.streamStates[req.TypeUrl] + streamState, ok := sw.streamStates[req.TypeUrl] + if !ok { + // Supports legacy wildcard mode + // Wildcard will be set to true if no resource is set + streamState = stream.NewSubscriptionState(len(req.ResourceNames) == 0, nil) + } + // ToDo: track ACK through subscription state if lastResponse, ok := sw.lastDiscoveryResponses[req.TypeUrl]; ok { if lastResponse.nonce == "" || lastResponse.nonce == nonce { // Let's record Resource names that a client has received. - streamState.SetResourceVersions(lastResponse.resources) + streamState.SetKnownResources(lastResponse.resources) } } + updateSubscriptionResources(req, &streamState) + typeURL := req.GetTypeUrl() // Use the multiplexed channel for new watches. responder := sw.watches.responders[resource.AnyType].response @@ -124,16 +133,26 @@ func (s *server) processADS(sw *streamWrapper, reqCh chan *discovery.DiscoveryRe return err } + cancel, err := s.cache.CreateWatch(req, streamState, responder) + if err != nil { + return err + } + sw.watches.addWatch(typeURL, &watch{ - cancel: s.cache.CreateWatch(req, streamState, responder), + cancel: cancel, response: responder, }) } } else { // No pre-existing watch exists, let's create one. // We need to precompute the watches first then open a watch in the cache. + cancel, err := s.cache.CreateWatch(req, streamState, responder) + if err != nil { + return err + } + sw.watches.addWatch(typeURL, &watch{ - cancel: s.cache.CreateWatch(req, streamState, responder), + cancel: cancel, response: responder, }) } diff --git a/pkg/server/sotw/v3/server.go b/pkg/server/sotw/v3/server.go index a5d2a257f9..7fcb64b013 100644 --- a/pkg/server/sotw/v3/server.go +++ b/pkg/server/sotw/v3/server.go @@ -91,7 +91,7 @@ type streamWrapper struct { // The below fields are used for tracking resource // cache state and should be maintained per stream. - streamStates map[string]stream.StreamState + streamStates map[string]stream.SubscriptionState lastDiscoveryResponses map[string]lastDiscoveryResponse } diff --git a/pkg/server/sotw/v3/xds.go b/pkg/server/sotw/v3/xds.go index 61cc07905b..59d90b6257 100644 --- a/pkg/server/sotw/v3/xds.go +++ b/pkg/server/sotw/v3/xds.go @@ -27,7 +27,7 @@ func (s *server) process(str stream.Stream, reqCh chan *discovery.DiscoveryReque // a collection of stack allocated watches per request type. watches: newWatches(), - streamStates: make(map[string]stream.StreamState), + streamStates: make(map[string]stream.SubscriptionState), lastDiscoveryResponses: make(map[string]lastDiscoveryResponse), } @@ -121,10 +121,12 @@ func (s *server) process(str stream.Stream, reqCh chan *discovery.DiscoveryReque if lastResponse, ok := sw.lastDiscoveryResponses[req.TypeUrl]; ok { if lastResponse.nonce == "" || lastResponse.nonce == nonce { // Let's record Resource names that a client has received. - streamState.SetResourceVersions(lastResponse.resources) + streamState.SetKnownResources(lastResponse.resources) } } + updateSubscriptionResources(req, &streamState) + typeURL := req.GetTypeUrl() responder := make(chan cache.Response, 1) if w, ok := sw.watches.responders[typeURL]; ok { @@ -133,16 +135,24 @@ func (s *server) process(str stream.Stream, reqCh chan *discovery.DiscoveryReque if w.nonce == "" || w.nonce == nonce { w.close() + cancel, err := s.cache.CreateWatch(req, streamState, responder) + if err != nil { + return err + } sw.watches.addWatch(typeURL, &watch{ - cancel: s.cache.CreateWatch(req, streamState, responder), + cancel: cancel, response: responder, }) } } else { // No pre-existing watch exists, let's create one. // We need to precompute the watches first then open a watch in the cache. + cancel, err := s.cache.CreateWatch(req, streamState, responder) + if err != nil { + return err + } sw.watches.addWatch(typeURL, &watch{ - cancel: s.cache.CreateWatch(req, streamState, responder), + cancel: cancel, response: responder, }) } @@ -168,3 +178,36 @@ func (s *server) process(str stream.Stream, reqCh chan *discovery.DiscoveryReque } } } + +// updateSubscriptionResources provides a normalized view of resources to be used in Cache +// It is also implementing the new behavior of wildcard as described in +// https://www.envoyproxy.io/docs/envoy/latest/api-docs/xds_protocol#how-the-client-specifies-what-resources-to-return +func updateSubscriptionResources(req *discovery.DiscoveryRequest, subscriptionState *stream.SubscriptionState) { + subscribedResources := make(map[string]struct{}, len(req.ResourceNames)) + explicitWildcard := false + for _, resource := range req.ResourceNames { + if resource == "*" { + explicitWildcard = true + } else { + subscribedResources[resource] = struct{}{} + } + } + + if subscriptionState.IsWildcard() && len(req.ResourceNames) == 0 && len(subscriptionState.GetSubscribedResources()) == 0 { + // We were wildcard and no resource has been subscribed + // Legacy wildcard mode states that we remain in wildcard mode + subscriptionState.SetWildcard(true) + } else if explicitWildcard { + // Explicit subscription to wildcard + // Documentation states that we should no longer allow to fallback to the previous case + // and no longer setting wildcard would no longer subscribe to anything + // For now we ignore this case and will not support unsubscribing in this case + subscriptionState.SetWildcard(true) + } else { + // The subscription is currently not wildcard, or there are resources or have been resources subscribed to + // This is no longer the legacy wildcard case as described by the specification + subscriptionState.SetWildcard(false) + } + subscriptionState.SetSubscribedResources(subscribedResources) + +} diff --git a/pkg/server/stream/v3/stream.go b/pkg/server/stream/v3/stream.go index 28dea295b5..b999d40c78 100644 --- a/pkg/server/stream/v3/stream.go +++ b/pkg/server/stream/v3/stream.go @@ -21,69 +21,3 @@ type DeltaStream interface { Send(*discovery.DeltaDiscoveryResponse) error Recv() (*discovery.DeltaDiscoveryRequest, error) } - -// StreamState will keep track of resource cache state per type on a stream. -type StreamState struct { // nolint:golint,revive - // Indicates whether the delta stream currently has a wildcard watch - wildcard bool - - // Provides the list of resources explicitly requested by the client - // This list might be non-empty even when set as wildcard - subscribedResourceNames map[string]struct{} - - // ResourceVersions contains a hash of the resource as the value and the resource name as the key. - // This field stores the last state sent to the client. - resourceVersions map[string]string - - // Ordered indicates whether we want an ordered ADS stream or not - ordered bool -} - -// NewStreamState initializes a stream state. -func NewStreamState(wildcard bool, initialResourceVersions map[string]string) StreamState { - state := StreamState{ - wildcard: wildcard, - subscribedResourceNames: map[string]struct{}{}, - resourceVersions: initialResourceVersions, - ordered: false, // Ordered comes from the first request since that's when we discover if they want ADS - } - - if initialResourceVersions == nil { - state.resourceVersions = make(map[string]string) - } - - return state -} - -// GetSubscribedResourceNames returns the list of resources currently explicitly subscribed to -// If the request is set to wildcard it may be empty -// Currently populated only when using delta-xds -func (s StreamState) GetSubscribedResources() map[string]struct{} { - return s.subscribedResourceNames -} - -// SetSubscribedResourceNames is setting the list of resources currently explicitly subscribed to -// It is decorrelated from the wildcard state of the stream -// Currently used only when using delta-xds -func (s *StreamState) SetSubscribedResources(subscribedResourceNames map[string]struct{}) { - s.subscribedResourceNames = subscribedResourceNames -} - -func (s StreamState) GetKnownResources() map[string]string { - return s.resourceVersions -} - -// SetResourceVersions sets a list of resource versions by type URL and removes the flag -// of "first" since we can safely assume another request has come through the stream. -func (s *StreamState) SetResourceVersions(resourceVersions map[string]string) { - s.resourceVersions = resourceVersions -} - -func (s *StreamState) SetWildcard(wildcard bool) { - s.wildcard = wildcard -} - -// IsWildcard returns whether or not an xDS client requested in wildcard mode on the initial request. -func (s StreamState) IsWildcard() bool { - return s.wildcard -} diff --git a/pkg/server/stream/v3/subscription.go b/pkg/server/stream/v3/subscription.go new file mode 100644 index 0000000000..c5a17b46b8 --- /dev/null +++ b/pkg/server/stream/v3/subscription.go @@ -0,0 +1,81 @@ +package stream + +// SubscriptionState will keep track of a resource subscription on a stream. +type SubscriptionState struct { + // wildcard is set if the subscription currently has a wildcard watch + wildcard bool + + // subscribedResourceNames provides the resources explicitly requested by the client + // This list might be non-empty even when set as wildcard + subscribedResourceNames map[string]struct{} + + // resourceVersions contains the resources acknowledged by the client and the versions + // associated to them + resourceVersions map[string]string +} + +// NewSubscriptionState initializes a stream state. +func NewSubscriptionState(wildcard bool, initialResourceVersions map[string]string) SubscriptionState { + state := SubscriptionState{ + wildcard: wildcard, + subscribedResourceNames: map[string]struct{}{}, + resourceVersions: initialResourceVersions, + } + + if initialResourceVersions == nil { + state.resourceVersions = make(map[string]string) + } + + return state +} + +// GetSubscribedResources returns the list of resources currently explicitly subscribed to +// If the request is set to wildcard it may be empty +// Currently populated only when using delta-xds +func (s SubscriptionState) GetSubscribedResources() map[string]struct{} { + return s.subscribedResourceNames +} + +// SetSubscribedResources is setting the list of resources currently explicitly subscribed to +// It is decorrelated from the wildcard state of the stream +// Currently used only when using delta-xds +func (s *SubscriptionState) SetSubscribedResources(subscribedResourceNames map[string]struct{}) { + s.subscribedResourceNames = subscribedResourceNames +} + +// GetKnownResources returns the list of resources acknowledged by the client +// and their acknowledged version +func (s SubscriptionState) GetKnownResources() map[string]string { + return s.resourceVersions +} + +// SetKnownResources sets a list of resource versions currently known by the client +// The cache can use this state to compute resources added/updated/deleted +func (s *SubscriptionState) SetKnownResources(resourceVersions map[string]string) { + s.resourceVersions = resourceVersions +} + +// SetWildcard will set the subscription to return all known resources +func (s *SubscriptionState) SetWildcard(wildcard bool) { + s.wildcard = wildcard +} + +// IsWildcard returns whether or not the subscription currently has a wildcard watch +func (s SubscriptionState) IsWildcard() bool { + return s.wildcard +} + +// WatchesResources returns whether at least one of the resources provided is currently being watched by the subscription. +// If the request is wildcard, it will always return true, +// otherwise it will compare the provided resources to the list of resources currently subscribed +func (s SubscriptionState) WatchesResources(resourceNames map[string]struct{}) bool { + if s.wildcard { + return true + } + for resourceName := range resourceNames { + if _, ok := s.subscribedResourceNames[resourceName]; ok { + return true + } + } + return false +} diff --git a/pkg/server/v3/delta_test.go b/pkg/server/v3/delta_test.go index 99a8039f28..476a1fe378 100644 --- a/pkg/server/v3/delta_test.go +++ b/pkg/server/v3/delta_test.go @@ -19,7 +19,7 @@ import ( "github.com/envoyproxy/go-control-plane/pkg/test/resource/v3" ) -func (config *mockConfigWatcher) CreateDeltaWatch(req *discovery.DeltaDiscoveryRequest, state cache.ClientState, out chan cache.DeltaResponse) func() { +func (config *mockConfigWatcher) CreateDeltaWatch(req *discovery.DeltaDiscoveryRequest, state cache.SubscriptionState, out chan cache.DeltaResponse) (func(), error) { config.deltaCounts[req.TypeUrl] = config.deltaCounts[req.TypeUrl] + 1 // This is duplicated from pkg/cache/v3/delta.go as private there @@ -87,10 +87,10 @@ func (config *mockConfigWatcher) CreateDeltaWatch(req *discovery.DeltaDiscoveryR config.deltaWatches++ return func() { config.deltaWatches-- - } + }, nil } - return nil + return nil, nil } type mockDeltaStream struct { diff --git a/pkg/server/v3/server_test.go b/pkg/server/v3/server_test.go index b8cac11cfd..905a0b5351 100644 --- a/pkg/server/v3/server_test.go +++ b/pkg/server/v3/server_test.go @@ -48,7 +48,7 @@ type mockConfigWatcher struct { mu *sync.RWMutex } -func (config *mockConfigWatcher) CreateWatch(req *discovery.DiscoveryRequest, state cache.ClientState, out chan cache.Response) func() { +func (config *mockConfigWatcher) CreateWatch(req *discovery.DiscoveryRequest, _ cache.SubscriptionState, out chan cache.Response) (func(), error) { config.counts[req.TypeUrl] = config.counts[req.TypeUrl] + 1 if len(config.responses[req.TypeUrl]) > 0 { out <- config.responses[req.TypeUrl][0] @@ -57,9 +57,9 @@ func (config *mockConfigWatcher) CreateWatch(req *discovery.DiscoveryRequest, st config.watches++ return func() { config.watches-- - } + }, nil } - return nil + return nil, nil } func (config *mockConfigWatcher) Fetch(_ context.Context, req *discovery.DiscoveryRequest) (cache.Response, error) { From f62d46a467c98136890a07e48212d7618b79ca63 Mon Sep 17 00:00:00 2001 From: Valerian Roche Date: Fri, 9 Jun 2023 20:43:23 -0400 Subject: [PATCH 5/5] Propose new cache interface Signed-off-by: Valerian Roche --- go.mod | 2 +- pkg/cache/v3/cache.go | 335 ++++----------------------- pkg/cache/v3/cache_test.go | 37 ++- pkg/cache/v3/delta.go | 260 +++++++++++++++++++-- pkg/cache/v3/delta_test.go | 7 +- pkg/cache/v3/linear.go | 110 +++++---- pkg/cache/v3/linear_test.go | 8 + pkg/cache/v3/mux.go | 10 +- pkg/cache/v3/resources_test.go | 3 + pkg/cache/v3/simple.go | 150 ++++++------ pkg/cache/v3/simple_test.go | 143 ++++++------ pkg/cache/v3/sotw.go | 241 +++++++++++++++++++ pkg/cache/v3/status.go | 11 +- pkg/server/delta/v3/server.go | 28 ++- pkg/server/sotw/v3/ads.go | 97 +++----- pkg/server/sotw/v3/server.go | 49 ++-- pkg/server/sotw/v3/watches.go | 53 +++-- pkg/server/sotw/v3/xds.go | 73 +++--- pkg/server/stream/v3/subscription.go | 12 + pkg/server/v3/delta_test.go | 50 ++-- pkg/server/v3/gateway_test.go | 30 +-- pkg/server/v3/server_test.go | 118 +++++----- pkg/test/main/main.go | 4 +- 23 files changed, 1054 insertions(+), 777 deletions(-) create mode 100644 pkg/cache/v3/sotw.go diff --git a/go.mod b/go.mod index dd0f66e037..e1f7eab40a 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/envoyproxy/go-control-plane -go 1.17 +go 1.18 require ( github.com/census-instrumentation/opencensus-proto v0.4.1 diff --git a/pkg/cache/v3/cache.go b/pkg/cache/v3/cache.go index b26e0f5bc8..2222a516c9 100644 --- a/pkg/cache/v3/cache.go +++ b/pkg/cache/v3/cache.go @@ -17,24 +17,21 @@ package cache import ( "context" - "errors" - "fmt" - "sync/atomic" - - "google.golang.org/protobuf/proto" - "google.golang.org/protobuf/types/known/anypb" - "google.golang.org/protobuf/types/known/durationpb" - - "github.com/envoyproxy/go-control-plane/pkg/cache/types" + core "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" discovery "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3" ) -// Request is an alias for the discovery request type. -type Request = discovery.DiscoveryRequest +type Request interface { + GetNode() *core.Node + GetTypeUrl() string + GetVersionInfo() string +} -// DeltaRequest is an alias for the delta discovery request type. -type DeltaRequest = discovery.DeltaDiscoveryRequest +type DeltaRequest interface { + GetNode() *core.Node + GetTypeUrl() string +} // SubscriptionState provides additional data on the client knowledge for the type matching the request // This allows proper implementation of stateful aspects of the protocol (e.g. returning only some updated resources) @@ -57,6 +54,10 @@ type SubscriptionState interface { // More details on the behavior of wildcard are present at https://www.envoyproxy.io/docs/envoy/latest/api-docs/xds_protocol#how-the-client-specifies-what-resources-to-return IsWildcard() bool + // IsFirstResponse returns whether the subscription has already been replied to at least once + // It is needed for some edge cases when a response must be returned (even if empty) at first + IsFirstResponse() bool + // WatchesResources returns whether at least one of the resources provided is currently being watched by the subscription. // It is currently only applicable to delta-xds. // If the request is wildcard, it will always return true, @@ -81,7 +82,7 @@ type ConfigWatcher interface { // // Cancel is an optional function to release resources in the producer. If // provided, the consumer may call this function multiple times. - CreateWatch(*Request, SubscriptionState, chan Response) (cancel func(), err error) + CreateWatch(Request, SubscriptionState, chan Response) (cancel func(), err error) // CreateDeltaWatch returns a new open incremental xDS watch. // This is the entrypoint to propagate configuration changes the @@ -93,13 +94,18 @@ type ConfigWatcher interface { // // Cancel is an optional function to release resources in the producer. If // provided, the consumer may call this function multiple times. - CreateDeltaWatch(*DeltaRequest, SubscriptionState, chan DeltaResponse) (cancel func(), err error) + CreateDeltaWatch(DeltaRequest, SubscriptionState, chan DeltaResponse) (cancel func(), err error) +} + +type FetchRequest interface { + Request + GetResourceNames() []string } // ConfigFetcher fetches configuration resources from cache type ConfigFetcher interface { // Fetch implements the polling method of the config cache using a non-empty request. - Fetch(context.Context, *Request) (Response, error) + Fetch(context.Context, FetchRequest) (Response, error) } // Cache is a generic config cache with a watcher. @@ -110,299 +116,44 @@ type Cache interface { // Response is a wrapper around Envoy's DiscoveryResponse. type Response interface { - // Get the Constructed DiscoveryResponse - GetDiscoveryResponse() (*discovery.DiscoveryResponse, error) - - // Get the original Request for the Response. - GetRequest() *discovery.DiscoveryRequest + GetRequest() Request // Get the version in the Response. GetVersion() (string, error) + // Get the list of resources included in the response. + GetReturnedResources() []string + // Get the context provided during response creation. GetContext() context.Context + + // Get the Constructed DiscoveryResponse + GetDiscoveryResponse() (*discovery.DiscoveryResponse, error) + + // Get the request that created the watch that we're now responding to. This is provided to allow the caller to correlate the + // response with a request. + GetDiscoveryRequest() (*discovery.DiscoveryRequest, error) } // DeltaResponse is a wrapper around Envoy's DeltaDiscoveryResponse type DeltaResponse interface { - // Get the constructed DeltaDiscoveryResponse - GetDeltaDiscoveryResponse() (*discovery.DeltaDiscoveryResponse, error) - - // Get the request that created the watch that we're now responding to. This is provided to allow the caller to correlate the - // response with a request. Generally this will be the latest request seen on the stream for the specific type. - GetDeltaRequest() *discovery.DeltaDiscoveryRequest + GetDeltaRequest() DeltaRequest // Get the version in the DeltaResponse. This field is generally used for debugging purposes as noted by the Envoy documentation. GetSystemVersion() (string, error) - // Get the version map of the internal cache. - // The version map consists of updated version mappings after this response is applied - GetNextVersionMap() map[string]string + // Get the list of resources included in the response. + // If the resource was set in Resources, its version is provided + // If the version was set in RemovedResources, its version is "" + GetReturnedResources() map[string]string // Get the context provided during response creation GetContext() context.Context -} - -// RawResponse is a pre-serialized xDS response containing the raw resources to -// be included in the final Discovery Response. -type RawResponse struct { - // Request is the original request. - Request *discovery.DiscoveryRequest - - // Version of the resources as tracked by the cache for the given type. - // Proxy responds with this version as an acknowledgement. - Version string - - // Resources to be included in the response. - Resources []types.ResourceWithTTL - - // Whether this is a heartbeat response. For xDS versions that support TTL, this - // will be converted into a response that doesn't contain the actual resource protobuf. - // This allows for more lightweight updates that server only to update the TTL timer. - Heartbeat bool - - // Context provided at the time of response creation. This allows associating additional - // information with a generated response. - Ctx context.Context - - // marshaledResponse holds an atomic reference to the serialized discovery response. - marshaledResponse atomic.Value -} - -// RawDeltaResponse is a pre-serialized xDS response that utilizes the delta discovery request/response objects. -type RawDeltaResponse struct { - // Request is the latest delta request on the stream. - DeltaRequest *discovery.DeltaDiscoveryRequest - - // SystemVersionInfo holds the currently applied response system version and should be used for debugging purposes only. - SystemVersionInfo string - - // Resources to be included in the response. - Resources []types.Resource - - // RemovedResources is a list of resource aliases which should be dropped by the consuming client. - RemovedResources []string - - // NextVersionMap consists of updated version mappings after this response is applied - NextVersionMap map[string]string - - // Context provided at the time of response creation. This allows associating additional - // information with a generated response. - Ctx context.Context - - // Marshaled Resources to be included in the response. - marshaledResponse atomic.Value -} - -var _ Response = &RawResponse{} -var _ DeltaResponse = &RawDeltaResponse{} - -// PassthroughResponse is a pre constructed xDS response that need not go through marshaling transformations. -type PassthroughResponse struct { - // Request is the original request. - Request *discovery.DiscoveryRequest - - // The discovery response that needs to be sent as is, without any marshaling transformations. - DiscoveryResponse *discovery.DiscoveryResponse - - ctx context.Context -} - -// DeltaPassthroughResponse is a pre constructed xDS response that need not go through marshaling transformations. -type DeltaPassthroughResponse struct { - // Request is the latest delta request on the stream - DeltaRequest *discovery.DeltaDiscoveryRequest - - // NextVersionMap consists of updated version mappings after this response is applied - NextVersionMap map[string]string - - // This discovery response that needs to be sent as is, without any marshaling transformations - DeltaDiscoveryResponse *discovery.DeltaDiscoveryResponse - - ctx context.Context -} - -var _ Response = &PassthroughResponse{} -var _ DeltaResponse = &DeltaPassthroughResponse{} - -// GetDiscoveryResponse performs the marshaling the first time its called and uses the cached response subsequently. -// This is necessary because the marshaled response does not change across the calls. -// This caching behavior is important in high throughput scenarios because grpc marshaling has a cost and it drives the cpu utilization under load. -func (r *RawResponse) GetDiscoveryResponse() (*discovery.DiscoveryResponse, error) { - marshaledResponse := r.marshaledResponse.Load() - - if marshaledResponse == nil { - marshaledResources := make([]*anypb.Any, len(r.Resources)) - for i, resource := range r.Resources { - maybeTtldResource, resourceType, err := r.maybeCreateTTLResource(resource) - if err != nil { - return nil, err - } - marshaledResource, err := MarshalResource(maybeTtldResource) - if err != nil { - return nil, err - } - marshaledResources[i] = &anypb.Any{ - TypeUrl: resourceType, - Value: marshaledResource, - } - } - - marshaledResponse = &discovery.DiscoveryResponse{ - VersionInfo: r.Version, - Resources: marshaledResources, - TypeUrl: r.Request.TypeUrl, - } - - r.marshaledResponse.Store(marshaledResponse) - } - - return marshaledResponse.(*discovery.DiscoveryResponse), nil -} - -// GetDeltaDiscoveryResponse performs the marshaling the first time its called and uses the cached response subsequently. -// We can do this because the marshaled response does not change across the calls. -// This caching behavior is important in high throughput scenarios because grpc marshaling has a cost and it drives the cpu utilization under load. -func (r *RawDeltaResponse) GetDeltaDiscoveryResponse() (*discovery.DeltaDiscoveryResponse, error) { - marshaledResponse := r.marshaledResponse.Load() - - if marshaledResponse == nil { - marshaledResources := make([]*discovery.Resource, len(r.Resources)) - - for i, resource := range r.Resources { - name := GetResourceName(resource) - marshaledResource, err := MarshalResource(resource) - if err != nil { - return nil, err - } - version := HashResource(marshaledResource) - if version == "" { - return nil, errors.New("failed to create a resource hash") - } - marshaledResources[i] = &discovery.Resource{ - Name: name, - Resource: &anypb.Any{ - TypeUrl: r.DeltaRequest.TypeUrl, - Value: marshaledResource, - }, - Version: version, - } - } - - marshaledResponse = &discovery.DeltaDiscoveryResponse{ - Resources: marshaledResources, - RemovedResources: r.RemovedResources, - TypeUrl: r.DeltaRequest.TypeUrl, - SystemVersionInfo: r.SystemVersionInfo, - } - r.marshaledResponse.Store(marshaledResponse) - } - - return marshaledResponse.(*discovery.DeltaDiscoveryResponse), nil -} - -// GetRequest returns the original Discovery Request. -func (r *RawResponse) GetRequest() *discovery.DiscoveryRequest { - return r.Request -} - -func (r *RawResponse) GetContext() context.Context { - return r.Ctx -} - -// GetDeltaRequest returns the original DeltaRequest -func (r *RawDeltaResponse) GetDeltaRequest() *discovery.DeltaDiscoveryRequest { - return r.DeltaRequest -} - -// GetVersion returns the response version. -func (r *RawResponse) GetVersion() (string, error) { - return r.Version, nil -} - -// GetSystemVersion returns the raw SystemVersion -func (r *RawDeltaResponse) GetSystemVersion() (string, error) { - return r.SystemVersionInfo, nil -} - -// NextVersionMap returns the version map which consists of updated version mappings after this response is applied -func (r *RawDeltaResponse) GetNextVersionMap() map[string]string { - return r.NextVersionMap -} - -func (r *RawDeltaResponse) GetContext() context.Context { - return r.Ctx -} - -var deltaResourceTypeURL = "type.googleapis.com/" + string(proto.MessageName(&discovery.Resource{})) - -func (r *RawResponse) maybeCreateTTLResource(resource types.ResourceWithTTL) (types.Resource, string, error) { - if resource.TTL != nil { - wrappedResource := &discovery.Resource{ - Name: GetResourceName(resource.Resource), - Ttl: durationpb.New(*resource.TTL), - } - - if !r.Heartbeat { - rsrc, err := anypb.New(resource.Resource) - if err != nil { - return nil, "", err - } - rsrc.TypeUrl = r.Request.TypeUrl - wrappedResource.Resource = rsrc - } - - return wrappedResource, deltaResourceTypeURL, nil - } - - return resource.Resource, r.Request.TypeUrl, nil -} - -// GetDiscoveryResponse returns the final passthrough Discovery Response. -func (r *PassthroughResponse) GetDiscoveryResponse() (*discovery.DiscoveryResponse, error) { - return r.DiscoveryResponse, nil -} - -// GetDeltaDiscoveryResponse returns the final passthrough Delta Discovery Response. -func (r *DeltaPassthroughResponse) GetDeltaDiscoveryResponse() (*discovery.DeltaDiscoveryResponse, error) { - return r.DeltaDiscoveryResponse, nil -} - -// GetRequest returns the original Discovery Request -func (r *PassthroughResponse) GetRequest() *discovery.DiscoveryRequest { - return r.Request -} - -// GetDeltaRequest returns the original Delta Discovery Request -func (r *DeltaPassthroughResponse) GetDeltaRequest() *discovery.DeltaDiscoveryRequest { - return r.DeltaRequest -} - -// GetVersion returns the response version. -func (r *PassthroughResponse) GetVersion() (string, error) { - if r.DiscoveryResponse != nil { - return r.DiscoveryResponse.VersionInfo, nil - } - return "", fmt.Errorf("DiscoveryResponse is nil") -} -func (r *PassthroughResponse) GetContext() context.Context { - return r.ctx -} - -// GetSystemVersion returns the response version. -func (r *DeltaPassthroughResponse) GetSystemVersion() (string, error) { - if r.DeltaDiscoveryResponse != nil { - return r.DeltaDiscoveryResponse.SystemVersionInfo, nil - } - return "", fmt.Errorf("DeltaDiscoveryResponse is nil") -} - -// NextVersionMap returns the version map from a DeltaPassthroughResponse -func (r *DeltaPassthroughResponse) GetNextVersionMap() map[string]string { - return r.NextVersionMap -} + // Get the constructed DeltaDiscoveryResponse + GetDeltaDiscoveryResponse() (*discovery.DeltaDiscoveryResponse, error) -func (r *DeltaPassthroughResponse) GetContext() context.Context { - return r.ctx + // Get the request that created the watch that we're now responding to. This is provided to allow the caller to correlate the + // response with a request. + GetDeltaDiscoveryRequest() (*discovery.DeltaDiscoveryRequest, error) } diff --git a/pkg/cache/v3/cache_test.go b/pkg/cache/v3/cache_test.go index 2b76723312..65fad2fa2b 100644 --- a/pkg/cache/v3/cache_test.go +++ b/pkg/cache/v3/cache_test.go @@ -1,4 +1,4 @@ -package cache_test +package cache import ( "testing" @@ -11,7 +11,6 @@ import ( route "github.com/envoyproxy/go-control-plane/envoy/config/route/v3" discovery "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3" "github.com/envoyproxy/go-control-plane/pkg/cache/types" - "github.com/envoyproxy/go-control-plane/pkg/cache/v3" "github.com/envoyproxy/go-control-plane/pkg/resource/v3" ) @@ -20,16 +19,16 @@ const ( ) func TestResponseGetDiscoveryResponse(t *testing.T) { - routes := []types.ResourceWithTTL{{Resource: &route.RouteConfiguration{Name: resourceName}}} - resp := cache.RawResponse{ - Request: &discovery.DiscoveryRequest{TypeUrl: resource.RouteType}, - Version: "v", - Resources: routes, + routes := []resourceWithTTLAndName{ttlResource{resource: &route.RouteConfiguration{Name: resourceName}, name: resourceName}} + resp := rawResponse{ + request: &discovery.DiscoveryRequest{TypeUrl: resource.RouteType}, + version: "v", + resources: routes, } discoveryResponse, err := resp.GetDiscoveryResponse() assert.Nil(t, err) - assert.Equal(t, discoveryResponse.VersionInfo, resp.Version) + assert.Equal(t, discoveryResponse.VersionInfo, resp.version) assert.Equal(t, len(discoveryResponse.Resources), 1) cachedResponse, err := resp.GetDiscoveryResponse() @@ -51,14 +50,14 @@ func TestPassthroughResponseGetDiscoveryResponse(t *testing.T) { Resources: []*anypb.Any{rsrc}, VersionInfo: "v", } - resp := cache.PassthroughResponse{ - Request: &discovery.DiscoveryRequest{TypeUrl: resource.RouteType}, - DiscoveryResponse: dr, + resp := passthroughResponse{ + discoveryRequest: &discovery.DiscoveryRequest{TypeUrl: resource.RouteType}, + discoveryResponse: dr, } discoveryResponse, err := resp.GetDiscoveryResponse() assert.Nil(t, err) - assert.Equal(t, discoveryResponse.VersionInfo, resp.DiscoveryResponse.VersionInfo) + assert.Equal(t, discoveryResponse.VersionInfo, resp.discoveryResponse.VersionInfo) assert.Equal(t, len(discoveryResponse.Resources), 1) r := &route.RouteConfiguration{} @@ -69,17 +68,17 @@ func TestPassthroughResponseGetDiscoveryResponse(t *testing.T) { } func TestHeartbeatResponseGetDiscoveryResponse(t *testing.T) { - routes := []types.ResourceWithTTL{{Resource: &route.RouteConfiguration{Name: resourceName}}} - resp := cache.RawResponse{ - Request: &discovery.DiscoveryRequest{TypeUrl: resource.RouteType}, - Version: "v", - Resources: routes, - Heartbeat: true, + routes := []resourceWithTTLAndName{ttlResource{resource: &route.RouteConfiguration{Name: resourceName}, name: resourceName}} + resp := rawResponse{ + request: &discovery.DiscoveryRequest{TypeUrl: resource.RouteType}, + version: "v", + resources: routes, + heartbeat: true, } discoveryResponse, err := resp.GetDiscoveryResponse() assert.Nil(t, err) - assert.Equal(t, discoveryResponse.VersionInfo, resp.Version) + assert.Equal(t, discoveryResponse.VersionInfo, resp.version) assert.Equal(t, len(discoveryResponse.Resources), 1) assert.False(t, isTTLResource(discoveryResponse.Resources[0])) diff --git a/pkg/cache/v3/delta.go b/pkg/cache/v3/delta.go index 9dc3f87127..1ead80dbc4 100644 --- a/pkg/cache/v3/delta.go +++ b/pkg/cache/v3/delta.go @@ -16,10 +16,207 @@ package cache import ( "context" + "errors" + "fmt" + "sync/atomic" + discovery "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3" "github.com/envoyproxy/go-control-plane/pkg/cache/types" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/anypb" ) +type resourceWithNameAndVersion interface { + getResource() types.Resource + getName() string + getVersion() string +} + +func getResourceNames(resources []resourceWithNameAndVersion) []string { + names := make([]string, 0, len(resources)) + for _, res := range resources { + names = append(names, res.getName()) + } + return names +} + +var _ DeltaResponse = &rawDeltaResponse{} + +// RawDeltaResponse is a pre-serialized xDS response that utilizes the delta discovery request/response objects. +type rawDeltaResponse struct { + // Request is the original request. + deltaRequest DeltaRequest + + // SystemVersionInfo holds the currently applied response system version and should be used for debugging purposes only. + systemVersionInfo string + + // Resources to be included in the response. + resources []resourceWithNameAndVersion + + // RemovedResources is a list of resource aliases which should be dropped by the consuming client. + removedResources []string + + // Context provided at the time of response creation. This allows associating additional + // information with a generated response. + ctx context.Context + + // Marshaled Resources to be included in the response. + marshaledResponse atomic.Value +} + +// ToDo: merge with versionedResource +type TestNamedResource struct { + Name string + Version string + Resource types.Resource +} + +func (r TestNamedResource) getResource() types.Resource { return r.Resource } +func (r TestNamedResource) getName() string { return r.Name } +func (r TestNamedResource) getVersion() string { return r.Version } + +func NewTestRawDeltaResponse(request DeltaRequest, version string, resources []TestNamedResource, removedResources []string) *rawDeltaResponse { + resourceInterfaces := make([]resourceWithNameAndVersion, 0, len(resources)) + for _, resource := range resources { + resourceInterfaces = append(resourceInterfaces, resource) + } + return &rawDeltaResponse{ + deltaRequest: request, + systemVersionInfo: version, + resources: resourceInterfaces, + removedResources: removedResources, + } +} + +// GetDeltaDiscoveryResponse performs the marshaling the first time its called and uses the cached response subsequently. +// We can do this because the marshaled response does not change across the calls. +// This caching behavior is important in high throughput scenarios because grpc marshaling has a cost and it drives the cpu utilization under load. +func (r *rawDeltaResponse) GetDeltaDiscoveryResponse() (*discovery.DeltaDiscoveryResponse, error) { + marshaledResponse := r.marshaledResponse.Load() + + if marshaledResponse == nil { + marshaledResources := make([]*discovery.Resource, len(r.resources)) + + for i, resource := range r.resources { + marshaledResource, err := MarshalResource(resource.getResource()) + if err != nil { + return nil, err + } + marshaledResources[i] = &discovery.Resource{ + Name: resource.getName(), + Resource: &anypb.Any{ + TypeUrl: r.deltaRequest.GetTypeUrl(), + Value: marshaledResource, + }, + Version: resource.getVersion(), + } + } + + marshaledResponse = &discovery.DeltaDiscoveryResponse{ + Resources: marshaledResources, + RemovedResources: r.removedResources, + TypeUrl: r.deltaRequest.GetTypeUrl(), + SystemVersionInfo: r.systemVersionInfo, + } + r.marshaledResponse.Store(marshaledResponse) + } + + return marshaledResponse.(*discovery.DeltaDiscoveryResponse), nil +} + +// Get the list of resources included in the response. +// If the resource was set in Resources, its version is provided +// If the version was set in RemovedResources, its version is "" +func (r *rawDeltaResponse) GetReturnedResources() map[string]string { + resources := make(map[string]string, len(r.resources)+len(r.removedResources)) + for _, resource := range r.resources { + resources[resource.getName()] = resource.getVersion() + } + for _, resourceName := range r.removedResources { + resources[resourceName] = "" + } + return resources +} + +func (r *rawDeltaResponse) GetDeltaRequest() DeltaRequest { + return r.deltaRequest +} + +// GetDeltaDiscoveryRequest returns the original Discovery DeltaRequest if available +func (r *rawDeltaResponse) GetDeltaDiscoveryRequest() (*discovery.DeltaDiscoveryRequest, error) { + if r.deltaRequest == nil { + return nil, errors.New("no request was provided") + } else if req, ok := r.deltaRequest.(*discovery.DeltaDiscoveryRequest); ok { + return req, nil + } else { + return nil, errors.New("provided request is not a DeltaDiscoveryRequest") + } +} + +// GetSystemVersion returns the raw SystemVersion +func (r *rawDeltaResponse) GetSystemVersion() (string, error) { + return r.systemVersionInfo, nil +} + +func (r *rawDeltaResponse) GetContext() context.Context { + return r.ctx +} + +var _ DeltaResponse = &deltaPassthroughResponse{} + +// DeltaPassthroughResponse is a pre constructed xDS response that need not go through marshaling transformations. +type deltaPassthroughResponse struct { + // DeltaDiscoveryRequest is the original request. + deltaDiscoveryRequest *discovery.DeltaDiscoveryRequest + + // This discovery response that needs to be sent as is, without any marshaling transformations + deltaDiscoveryResponse *discovery.DeltaDiscoveryResponse + + ctx context.Context +} + +var deltaResourceTypeURL = "type.googleapis.com/" + string(proto.MessageName(&discovery.Resource{})) + +func (r *deltaPassthroughResponse) GetDeltaRequest() DeltaRequest { + return r.deltaDiscoveryRequest +} + +// GetDeltaDiscoveryResponse returns the final passthrough Delta Discovery Response. +func (r *deltaPassthroughResponse) GetDeltaDiscoveryResponse() (*discovery.DeltaDiscoveryResponse, error) { + return r.deltaDiscoveryResponse, nil +} + +// GetDeltaRequest returns the original Request +func (r *deltaPassthroughResponse) GetDeltaDiscoveryRequest() (*discovery.DeltaDiscoveryRequest, error) { + return r.deltaDiscoveryRequest, nil +} + +// GetSystemVersion returns the response version. +func (r *deltaPassthroughResponse) GetSystemVersion() (string, error) { + if r.deltaDiscoveryResponse != nil { + return r.deltaDiscoveryResponse.SystemVersionInfo, nil + } + return "", fmt.Errorf("DeltaDiscoveryResponse is nil") +} + +func (r *deltaPassthroughResponse) GetContext() context.Context { + return r.ctx +} + +// Get the list of resources included in the response. +// If the resource was set in Resources, its version is provided +// If the version was set in RemovedResources, its version is "" +func (r *deltaPassthroughResponse) GetReturnedResources() map[string]string { + resources := make(map[string]string, len(r.deltaDiscoveryResponse.Resources)+len(r.deltaDiscoveryResponse.RemovedResources)) + for _, resource := range r.deltaDiscoveryResponse.Resources { + resources[resource.Name] = resource.Version + } + for _, resourceName := range r.deltaDiscoveryResponse.RemovedResources { + resources[resourceName] = "" + } + return resources +} + // groups together resource-related arguments for the createDeltaResponse function type resourceContainer struct { resourceMap map[string]types.Resource @@ -27,39 +224,60 @@ type resourceContainer struct { systemVersion string } -func createDeltaResponse(ctx context.Context, req *DeltaRequest, state SubscriptionState, resources resourceContainer) *RawDeltaResponse { +type versionedResource struct { + resource types.Resource + name string + version string +} + +func (r versionedResource) getResource() types.Resource { + return r.resource +} + +func (r versionedResource) getName() string { + return r.name +} + +func (r versionedResource) getVersion() string { + return r.version +} + +func createDeltaResponse(ctx context.Context, req DeltaRequest, state SubscriptionState, resources resourceContainer) *rawDeltaResponse { // variables to build our response with - var nextVersionMap map[string]string - var filtered []types.Resource + var filtered []resourceWithNameAndVersion var toRemove []string // If we are handling a wildcard request, we want to respond with all resources switch { case state.IsWildcard(): if len(state.GetKnownResources()) == 0 { - filtered = make([]types.Resource, 0, len(resources.resourceMap)) + filtered = make([]resourceWithNameAndVersion, 0, len(resources.resourceMap)) } - nextVersionMap = make(map[string]string, len(resources.resourceMap)) for name, r := range resources.resourceMap { // Since we've already precomputed the version hashes of the new snapshot, // we can just set it here to be used for comparison later version := resources.versionMap[name] - nextVersionMap[name] = version prevVersion, found := state.GetKnownResources()[name] if !found || (prevVersion != version) { - filtered = append(filtered, r) + filtered = append(filtered, versionedResource{ + name: name, + version: version, + resource: r, + }) } } // Compute resources for removal - // The resource version can be set to "" here to trigger a removal even if never returned before - for name := range state.GetKnownResources() { + for name, version := range state.GetKnownResources() { + if version == "" { + // The resource version can be set to "" to indicate that we already notified the deletion + continue + } if _, ok := resources.resourceMap[name]; !ok { toRemove = append(toRemove, name) } } default: - nextVersionMap = make(map[string]string, len(state.GetSubscribedResources())) // state.GetResourceVersions() may include resources no longer subscribed // In the current code this gets silently cleaned when updating the version map for name := range state.GetSubscribedResources() { @@ -67,21 +285,23 @@ func createDeltaResponse(ctx context.Context, req *DeltaRequest, state Subscript if r, ok := resources.resourceMap[name]; ok { nextVersion := resources.versionMap[name] if prevVersion != nextVersion { - filtered = append(filtered, r) + filtered = append(filtered, versionedResource{ + name: name, + version: nextVersion, + resource: r, + }) } - nextVersionMap[name] = nextVersion - } else if found { + } else if found && prevVersion != "" { toRemove = append(toRemove, name) } } } - return &RawDeltaResponse{ - DeltaRequest: req, - Resources: filtered, - RemovedResources: toRemove, - NextVersionMap: nextVersionMap, - SystemVersionInfo: resources.systemVersion, - Ctx: ctx, + return &rawDeltaResponse{ + deltaRequest: req, + resources: filtered, + removedResources: toRemove, + systemVersionInfo: resources.systemVersion, + ctx: ctx, } } diff --git a/pkg/cache/v3/delta_test.go b/pkg/cache/v3/delta_test.go index 25dddd6b29..be10f8ff74 100644 --- a/pkg/cache/v3/delta_test.go +++ b/pkg/cache/v3/delta_test.go @@ -1,4 +1,4 @@ -package cache_test +package cache import ( "context" @@ -14,7 +14,6 @@ import ( core "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" discovery "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3" "github.com/envoyproxy/go-control-plane/pkg/cache/types" - "github.com/envoyproxy/go-control-plane/pkg/cache/v3" rsrc "github.com/envoyproxy/go-control-plane/pkg/resource/v3" "github.com/envoyproxy/go-control-plane/pkg/server/stream/v3" "github.com/envoyproxy/go-control-plane/pkg/test/resource/v3" @@ -29,8 +28,8 @@ func assertResourceMapEqual(t *testing.T, want map[string]types.Resource, got ma } func TestSnapshotCacheDeltaWatch(t *testing.T) { - c := cache.NewSnapshotCache(false, group{}, logger{t: t}) - watches := make(map[string]chan cache.DeltaResponse) + c := NewSnapshotCache(false, group{}, logger{t: t}) + watches := make(map[string]chan DeltaResponse) // Make our initial request as a wildcard to get all resources and make sure the wildcard requesting works as intended for _, typ := range testTypes { diff --git a/pkg/cache/v3/linear.go b/pkg/cache/v3/linear.go index f1c8fcac28..45df39162f 100644 --- a/pkg/cache/v3/linear.go +++ b/pkg/cache/v3/linear.go @@ -27,7 +27,7 @@ import ( "github.com/envoyproxy/go-control-plane/pkg/log" ) -type watches = map[chan Response]struct{} +type watches = map[chan Response]Request // LinearCache supports collections of opaque resources. This cache has a // single collection indexed by resource names and manages resource versions @@ -113,45 +113,58 @@ func NewLinearCache(typeURL string, opts ...LinearCacheOption) *LinearCache { return out } -func (cache *LinearCache) respond(value chan Response, staleResources []string) { - var resources []types.ResourceWithTTL +func (cache *LinearCache) respond(req Request, value chan Response, staleResources map[string]struct{}) { + var resources []resourceWithTTLAndName // TODO: optimize the resources slice creations across different clients if len(staleResources) == 0 { - resources = make([]types.ResourceWithTTL, 0, len(cache.resources)) - for _, resource := range cache.resources { - resources = append(resources, types.ResourceWithTTL{Resource: resource}) + resources = make([]resourceWithTTLAndName, 0, len(cache.resources)) + for name, resource := range cache.resources { + resources = append(resources, ttlResource{name: name, resource: resource}) } } else { - resources = make([]types.ResourceWithTTL, 0, len(staleResources)) - for _, name := range staleResources { + resources = make([]resourceWithTTLAndName, 0, len(staleResources)) + for name := range staleResources { resource := cache.resources[name] if resource != nil { - resources = append(resources, types.ResourceWithTTL{Resource: resource}) + resources = append(resources, ttlResource{name: name, resource: resource}) } } } - value <- &RawResponse{ - Request: &Request{TypeUrl: cache.typeURL}, - Resources: resources, - Version: cache.getVersion(), - Ctx: context.Background(), + value <- &rawResponse{ + request: req, + resources: resources, + version: cache.getVersion(), + ctx: context.Background(), } } func (cache *LinearCache) notifyAll(modified map[string]struct{}) { + type staleWatch struct { + staleResources map[string]struct{} + req Request + } + // de-duplicate watches that need to be responded - notifyList := make(map[chan Response][]string) + notifyList := make(map[chan Response]staleWatch) + for name := range modified { - for watch := range cache.watches[name] { - notifyList[watch] = append(notifyList[watch], name) + for w, req := range cache.watches[name] { + if watch, ok := notifyList[w]; ok { + watch.staleResources[name] = struct{}{} + } else { + notifyList[w] = staleWatch{ + staleResources: map[string]struct{}{name: {}}, + req: req, + } + } } delete(cache.watches, name) } - for value, stale := range notifyList { - cache.respond(value, stale) + for value, staleWatch := range notifyList { + cache.respond(staleWatch.req, value, staleWatch.staleResources) } - for value := range cache.watchAll { - cache.respond(value, nil) + for value, req := range cache.watchAll { + cache.respond(req, value, nil) } cache.watchAll = make(watches) @@ -176,7 +189,7 @@ func (cache *LinearCache) notifyAll(modified map[string]struct{}) { } } -func (cache *LinearCache) respondDelta(request *DeltaRequest, value chan DeltaResponse, clientState SubscriptionState) *RawDeltaResponse { +func (cache *LinearCache) respondDelta(request DeltaRequest, value chan DeltaResponse, clientState SubscriptionState) *rawDeltaResponse { resp := createDeltaResponse(context.Background(), request, clientState, resourceContainer{ resourceMap: cache.resources, versionMap: cache.versionMap, @@ -184,10 +197,10 @@ func (cache *LinearCache) respondDelta(request *DeltaRequest, value chan DeltaRe }) // Only send a response if there were changes - if len(resp.Resources) > 0 || len(resp.RemovedResources) > 0 { + if len(resp.resources) > 0 || len(resp.removedResources) > 0 { if cache.log != nil { cache.log.Debugf("[linear cache] node: %s, sending delta response for typeURL %s with resources: %v removed resources: %v with wildcard: %t", - request.GetNode().GetId(), request.TypeUrl, GetResourceNames(resp.Resources), resp.RemovedResources, clientState.IsWildcard()) + request.GetNode().GetId(), request.GetTypeUrl(), getResourceNames(resp.resources), resp.removedResources, clientState.IsWildcard()) } value <- resp return resp @@ -298,22 +311,22 @@ func (cache *LinearCache) GetResources() map[string]types.Resource { return resources } -func (cache *LinearCache) CreateWatch(request *Request, _ SubscriptionState, value chan Response) (func(), error) { - if request.TypeUrl != cache.typeURL { +func (cache *LinearCache) CreateWatch(request Request, clientState SubscriptionState, value chan Response) (func(), error) { + if request.GetTypeUrl() != cache.typeURL { value <- nil - return nil, fmt.Errorf("request type %s does not match cache type %s", request.TypeUrl, cache.typeURL) + return nil, fmt.Errorf("request type %s does not match cache type %s", request.GetTypeUrl(), cache.typeURL) } // If the version is not up to date, check whether any requested resource has // been updated between the last version and the current version. This avoids the problem // of sending empty updates whenever an irrelevant resource changes. stale := false - staleResources := []string{} // empty means all + var staleResources map[string]struct{} // empty means all // strip version prefix if it is present var lastVersion uint64 var err error - if strings.HasPrefix(request.VersionInfo, cache.versionPrefix) { - lastVersion, err = strconv.ParseUint(request.VersionInfo[len(cache.versionPrefix):], 0, 64) + if strings.HasPrefix(request.GetVersionInfo(), cache.versionPrefix) { + lastVersion, err = strconv.ParseUint(request.GetVersionInfo()[len(cache.versionPrefix):], 0, 64) } else { err = errors.New("mis-matched version prefix") } @@ -322,44 +335,57 @@ func (cache *LinearCache) CreateWatch(request *Request, _ SubscriptionState, val defer cache.mu.Unlock() if err != nil { + // Version was not set or cannot be parsed + // It is ignored and all resources are returned stale = true - staleResources = request.ResourceNames - } else if len(request.ResourceNames) == 0 { + staleResources = clientState.GetSubscribedResources() + } else if clientState.IsWildcard() { + // For wildcard watches, all resources are sent if the version no longer matches + // As we expect at least one resource to have changed stale = lastVersion != cache.version } else { - for _, name := range request.ResourceNames { + // For non-wildcard watches, compare the provided version to the version of subscribed objects + // If at least one version is changed, we send back a response + staleResources = make(map[string]struct{}) + for name := range clientState.GetSubscribedResources() { + if _, ok := clientState.GetKnownResources()[name]; !ok { + // A resource is newly requested. Even if the version of the request matches or is higher than the object + // we must return it + staleResources[name] = struct{}{} + continue + } // When a resource is removed, its version defaults 0 and it is not considered stale. if lastVersion < cache.versionVector[name] { stale = true - staleResources = append(staleResources, name) + staleResources[name] = struct{}{} } } } if stale { - cache.respond(value, staleResources) + cache.respond(request, value, staleResources) return nil, nil } // Create open watches since versions are up to date. - if len(request.ResourceNames) == 0 { - cache.watchAll[value] = struct{}{} + if clientState.IsWildcard() { + cache.watchAll[value] = request return func() { cache.mu.Lock() defer cache.mu.Unlock() delete(cache.watchAll, value) }, nil } - for _, name := range request.ResourceNames { + for name := range clientState.GetSubscribedResources() { set, exists := cache.watches[name] if !exists { set = make(watches) cache.watches[name] = set } - set[value] = struct{}{} + set[value] = request } return func() { cache.mu.Lock() defer cache.mu.Unlock() - for _, name := range request.ResourceNames { + for name := range clientState.GetSubscribedResources() { set, exists := cache.watches[name] if exists { delete(set, value) @@ -371,7 +397,7 @@ func (cache *LinearCache) CreateWatch(request *Request, _ SubscriptionState, val }, nil } -func (cache *LinearCache) CreateDeltaWatch(request *DeltaRequest, clientState SubscriptionState, value chan DeltaResponse) (func(), error) { +func (cache *LinearCache) CreateDeltaWatch(request DeltaRequest, clientState SubscriptionState, value chan DeltaResponse) (func(), error) { cache.mu.Lock() defer cache.mu.Unlock() @@ -450,7 +476,7 @@ func (cache *LinearCache) nextDeltaWatchID() int64 { return atomic.AddInt64(&cache.deltaWatchCount, 1) } -func (cache *LinearCache) Fetch(context.Context, *Request) (Response, error) { +func (cache *LinearCache) Fetch(context.Context, FetchRequest) (Response, error) { return nil, errors.New("not implemented") } diff --git a/pkg/cache/v3/linear_test.go b/pkg/cache/v3/linear_test.go index 51a67882fb..f611e0730b 100644 --- a/pkg/cache/v3/linear_test.go +++ b/pkg/cache/v3/linear_test.go @@ -204,9 +204,11 @@ func TestLinearInitialResources(t *testing.T) { streamState := stream.NewSubscriptionState(false, map[string]string{}) c := NewLinearCache(testType, WithInitialResources(map[string]types.Resource{"a": testResource("a"), "b": testResource("b")})) w := make(chan Response, 1) + streamState.SetSubscribedResources(map[string]struct{}{"a": {}}) _, err := c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType}, streamState, w) require.NoError(t, err) verifyResponse(t, w, "0", 1) + streamState.SetSubscribedResources(map[string]struct{}{}) _, err = c.CreateWatch(&Request{TypeUrl: testType}, streamState, w) require.NoError(t, err) verifyResponse(t, w, "0", 2) @@ -237,12 +239,14 @@ func TestLinearBasic(t *testing.T) { // Create watches before a resource is ready w1 := make(chan Response, 1) + streamState.SetSubscribedResources(map[string]struct{}{"a": {}}) _, err := c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w1) require.NoError(t, err) mustBlock(t, w1) checkVersionMapNotSet(t, c) w := make(chan Response, 1) + streamState.SetSubscribedResources(map[string]struct{}{}) _, err = c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "0"}, streamState, w) require.NoError(t, err) mustBlock(t, w) @@ -255,10 +259,12 @@ func TestLinearBasic(t *testing.T) { verifyResponse(t, w, "1", 1) // Request again, should get same response + streamState.SetSubscribedResources(map[string]struct{}{"a": {}}) _, err = c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w) require.NoError(t, err) checkWatchCount(t, c, "a", 0) verifyResponse(t, w, "1", 1) + streamState.SetSubscribedResources(map[string]struct{}{}) _, err = c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "0"}, streamState, w) require.NoError(t, err) checkWatchCount(t, c, "a", 0) @@ -267,9 +273,11 @@ func TestLinearBasic(t *testing.T) { // Add another element and update the first, response should be different require.NoError(t, c.UpdateResource("b", testResource("b"))) require.NoError(t, c.UpdateResource("a", testResource("aa"))) + streamState.SetSubscribedResources(map[string]struct{}{"a": {}}) _, err = c.CreateWatch(&Request{ResourceNames: []string{"a"}, TypeUrl: testType, VersionInfo: "0"}, streamState, w) require.NoError(t, err) verifyResponse(t, w, "3", 1) + streamState.SetSubscribedResources(map[string]struct{}{}) _, err = c.CreateWatch(&Request{TypeUrl: testType, VersionInfo: "0"}, streamState, w) require.NoError(t, err) verifyResponse(t, w, "3", 2) diff --git a/pkg/cache/v3/mux.go b/pkg/cache/v3/mux.go index d4bb5791a9..a4a05076be 100644 --- a/pkg/cache/v3/mux.go +++ b/pkg/cache/v3/mux.go @@ -28,15 +28,15 @@ import ( // making sure there is always a matching cache. type MuxCache struct { // Classification functions. - Classify func(*Request) string - ClassifyDelta func(*DeltaRequest) string + Classify func(Request) string + ClassifyDelta func(DeltaRequest) string // Muxed caches. Caches map[string]Cache } var _ Cache = &MuxCache{} -func (mux *MuxCache) CreateWatch(request *Request, state SubscriptionState, value chan Response) (func(), error) { +func (mux *MuxCache) CreateWatch(request Request, state SubscriptionState, value chan Response) (func(), error) { key := mux.Classify(request) cache, exists := mux.Caches[key] if !exists { @@ -46,7 +46,7 @@ func (mux *MuxCache) CreateWatch(request *Request, state SubscriptionState, valu return cache.CreateWatch(request, state, value) } -func (mux *MuxCache) CreateDeltaWatch(request *DeltaRequest, state SubscriptionState, value chan DeltaResponse) (func(), error) { +func (mux *MuxCache) CreateDeltaWatch(request DeltaRequest, state SubscriptionState, value chan DeltaResponse) (func(), error) { key := mux.ClassifyDelta(request) cache, exists := mux.Caches[key] if !exists { @@ -56,6 +56,6 @@ func (mux *MuxCache) CreateDeltaWatch(request *DeltaRequest, state SubscriptionS return cache.CreateDeltaWatch(request, state, value) } -func (mux *MuxCache) Fetch(context.Context, *Request) (Response, error) { +func (mux *MuxCache) Fetch(context.Context, FetchRequest) (Response, error) { return nil, errors.New("not implemented") } diff --git a/pkg/cache/v3/resources_test.go b/pkg/cache/v3/resources_test.go index 5751f3a518..9f2254dd1d 100644 --- a/pkg/cache/v3/resources_test.go +++ b/pkg/cache/v3/resources_test.go @@ -2,6 +2,7 @@ package cache_test import ( "testing" + "time" "github.com/stretchr/testify/assert" @@ -9,6 +10,8 @@ import ( "github.com/envoyproxy/go-control-plane/pkg/cache/v3" ) +var ttl = 2 * time.Second + func TestIndexResourcesByName(t *testing.T) { tests := []struct { name string diff --git a/pkg/cache/v3/simple.go b/pkg/cache/v3/simple.go index 36a344ff28..fecb7cf4dd 100644 --- a/pkg/cache/v3/simple.go +++ b/pkg/cache/v3/simple.go @@ -23,6 +23,7 @@ import ( "github.com/envoyproxy/go-control-plane/pkg/cache/types" "github.com/envoyproxy/go-control-plane/pkg/log" + "github.com/envoyproxy/go-control-plane/pkg/server/stream/v3" ) // ResourceSnapshot is an abstract snapshot of a collection of resources that @@ -192,8 +193,8 @@ func (cache *snapshotCache) sendHeartbeats(ctx context.Context, node string) { info.mu.Lock() for id, watch := range info.watches { // Respond with the current version regardless of whether the version has changed. - version := snapshot.GetVersion(watch.Request.TypeUrl) - resources := snapshot.GetResourcesAndTTL(watch.Request.TypeUrl) + version := snapshot.GetVersion(watch.Request.GetTypeUrl()) + resources := snapshot.GetResourcesAndTTL(watch.Request.GetTypeUrl()) // TODO(snowp): Construct this once per type instead of once per watch. resourcesWithTTL := map[string]types.ResourceWithTTL{} @@ -206,8 +207,8 @@ func (cache *snapshotCache) sendHeartbeats(ctx context.Context, node string) { if len(resourcesWithTTL) == 0 { continue } - cache.log.Debugf("respond open watch %d%v with heartbeat for version %q", id, watch.Request.ResourceNames, version) - err := cache.respond(ctx, watch.Request, watch.Response, resourcesWithTTL, version, true) + cache.log.Debugf("respond open watch %d%v with heartbeat for version %q", id, watch.subscriptionState.GetSubscribedResources(), version) + err := cache.respond(ctx, watch, resourcesWithTTL, version, true) if err != nil { cache.log.Errorf("received error when attempting to respond to watches: %v", err) } @@ -234,11 +235,11 @@ func (cache *snapshotCache) SetSnapshot(ctx context.Context, node string, snapsh // responder callback for SOTW watches respond := func(watch ResponseWatch, id int64) error { - version := snapshot.GetVersion(watch.Request.TypeUrl) - if version != watch.Request.VersionInfo { - cache.log.Debugf("respond open watch %d %s%v with new version %q", id, watch.Request.TypeUrl, watch.Request.ResourceNames, version) - resources := snapshot.GetResourcesAndTTL(watch.Request.TypeUrl) - err := cache.respond(ctx, watch.Request, watch.Response, resources, version, false) + version := snapshot.GetVersion(watch.Request.GetTypeUrl()) + if version != watch.Request.GetVersionInfo() { + cache.log.Debugf("respond open watch %d %s%v with new version %q", id, watch.Request.GetTypeUrl(), watch.subscriptionState.GetSubscribedResources(), version) + resources := snapshot.GetResourcesAndTTL(watch.Request.GetTypeUrl()) + err := cache.respond(ctx, watch, resources, version, false) if err != nil { return err } @@ -323,17 +324,8 @@ func (cache *snapshotCache) ClearSnapshot(node string) { delete(cache.status, node) } -// nameSet creates a map from a string slice to value true. -func nameSet(names []string) map[string]bool { - set := make(map[string]bool, len(names)) - for _, name := range names { - set[name] = true - } - return set -} - // superset checks that all resources are listed in the names set. -func superset(names map[string]bool, resources map[string]types.ResourceWithTTL) error { +func superset[T any](names map[string]T, resources map[string]types.ResourceWithTTL) error { for resourceName := range resources { if _, exists := names[resourceName]; !exists { return fmt.Errorf("%q not listed", resourceName) @@ -344,18 +336,20 @@ func superset(names map[string]bool, resources map[string]types.ResourceWithTTL) // CreateWatch returns a watch for an xDS request. A nil function may be // returned if an error occurs. -func (cache *snapshotCache) CreateWatch(request *Request, clientState SubscriptionState, value chan Response) (func(), error) { - nodeID := cache.hash.ID(request.Node) +func (cache *snapshotCache) CreateWatch(request Request, clientState SubscriptionState, value chan Response) (func(), error) { + nodeID := cache.hash.ID(request.GetNode()) cache.mu.Lock() defer cache.mu.Unlock() info, ok := cache.status[nodeID] if !ok { - info = newStatusInfo(request.Node) + info = newStatusInfo(request.GetNode()) cache.status[nodeID] = info } + watch := ResponseWatch{Request: request, Response: value, subscriptionState: clientState} + // update last watch request time info.mu.Lock() info.lastWatchRequestTime = time.Now() @@ -364,28 +358,28 @@ func (cache *snapshotCache) CreateWatch(request *Request, clientState Subscripti var version string snapshot, exists := cache.snapshots[nodeID] if exists { - version = snapshot.GetVersion(request.TypeUrl) + version = snapshot.GetVersion(request.GetTypeUrl()) } if exists { knownResourceNames := clientState.GetKnownResources() diff := []string{} - for _, r := range request.ResourceNames { + for r := range clientState.GetSubscribedResources() { if _, ok := knownResourceNames[r]; !ok { diff = append(diff, r) } } cache.log.Debugf("nodeID %q requested %s%v and known %v. Diff %v", nodeID, - request.TypeUrl, request.ResourceNames, knownResourceNames, diff) + request.GetTypeUrl(), clientState.GetSubscribedResources(), knownResourceNames, diff) if len(diff) > 0 { - resources := snapshot.GetResourcesAndTTL(request.TypeUrl) + resources := snapshot.GetResourcesAndTTL(request.GetTypeUrl()) for _, name := range diff { if _, exists := resources[name]; exists { - if err := cache.respond(context.Background(), request, value, resources, version, false); err != nil { - cache.log.Errorf("failed to send a response for %s%v to nodeID %q: %s", request.TypeUrl, - request.ResourceNames, nodeID, err) + if err := cache.respond(context.Background(), watch, resources, version, false); err != nil { + cache.log.Errorf("failed to send a response for %s%v to nodeID %q: %s", request.GetTypeUrl(), + clientState.GetSubscribedResources(), nodeID, err) return nil, fmt.Errorf("failed to send the response: %w", err) } return func() {}, nil @@ -395,20 +389,20 @@ func (cache *snapshotCache) CreateWatch(request *Request, clientState Subscripti } // if the requested version is up-to-date or missing a response, leave an open watch - if !exists || request.VersionInfo == version { + if !exists || request.GetVersionInfo() == version { watchID := cache.nextWatchID() - cache.log.Debugf("open watch %d for %s%v from nodeID %q, version %q", watchID, request.TypeUrl, request.ResourceNames, nodeID, request.VersionInfo) + cache.log.Debugf("open watch %d for %s%v from nodeID %q, version %q", watchID, request.GetTypeUrl(), clientState.GetSubscribedResources(), nodeID, request.GetVersionInfo()) info.mu.Lock() - info.watches[watchID] = ResponseWatch{Request: request, Response: value} + info.watches[watchID] = watch info.mu.Unlock() return cache.cancelWatch(nodeID, watchID), nil } // otherwise, the watch may be responded immediately - resources := snapshot.GetResourcesAndTTL(request.TypeUrl) - if err := cache.respond(context.Background(), request, value, resources, version, false); err != nil { - cache.log.Errorf("failed to send a response for %s%v to nodeID %q: %s", request.TypeUrl, - request.ResourceNames, nodeID, err) + resources := snapshot.GetResourcesAndTTL(request.GetTypeUrl()) + if err := cache.respond(context.Background(), watch, resources, version, false); err != nil { + cache.log.Errorf("failed to send a response for %s%v to nodeID %q: %s", request.GetTypeUrl(), + clientState.GetSubscribedResources(), nodeID, err) return nil, fmt.Errorf("failed to send the response: %w", err) } @@ -435,57 +429,65 @@ func (cache *snapshotCache) cancelWatch(nodeID string, watchID int64) func() { // Respond to a watch with the snapshot value. The value channel should have capacity not to block. // TODO(kuat) do not respond always, see issue https://github.com/envoyproxy/go-control-plane/issues/46 -func (cache *snapshotCache) respond(ctx context.Context, request *Request, value chan Response, resources map[string]types.ResourceWithTTL, version string, heartbeat bool) error { +func (cache *snapshotCache) respond(ctx context.Context, watch ResponseWatch, resources map[string]types.ResourceWithTTL, version string, heartbeat bool) error { // for ADS, the request names must match the snapshot names // if they do not, then the watch is never responded, and it is expected that envoy makes another request - if len(request.ResourceNames) != 0 && cache.ads { - if err := superset(nameSet(request.ResourceNames), resources); err != nil { + if !watch.subscriptionState.IsWildcard() && cache.ads { + if err := superset(watch.subscriptionState.GetSubscribedResources(), resources); err != nil { cache.log.Warnf("ADS mode: not responding to request: %v", err) return nil } } - cache.log.Debugf("respond %s%v version %q with version %q", request.TypeUrl, request.ResourceNames, request.VersionInfo, version) + cache.log.Debugf("respond %s%v version %q with version %q", watch.Request.GetTypeUrl(), watch.subscriptionState.GetSubscribedResources(), watch.Request.GetVersionInfo(), version) select { - case value <- createResponse(ctx, request, resources, version, heartbeat): + case watch.Response <- createResponse(ctx, watch.Request, watch.subscriptionState, resources, version, heartbeat): return nil case <-ctx.Done(): return context.Canceled } } -func createResponse(ctx context.Context, request *Request, resources map[string]types.ResourceWithTTL, version string, heartbeat bool) Response { - filtered := make([]types.ResourceWithTTL, 0, len(resources)) +func createResponse(ctx context.Context, request Request, clientState SubscriptionState, resources map[string]types.ResourceWithTTL, version string, heartbeat bool) Response { + filtered := make([]resourceWithTTLAndName, 0, len(resources)) // Reply only with the requested resources. Envoy may ask each resource // individually in a separate stream. It is ok to reply with the same version // on separate streams since requests do not share their response versions. - if len(request.ResourceNames) != 0 { - set := nameSet(request.ResourceNames) + if !clientState.IsWildcard() { + subscribed := clientState.GetSubscribedResources() for name, resource := range resources { - if set[name] { - filtered = append(filtered, resource) + if _, ok := subscribed[name]; ok { + filtered = append(filtered, ttlResource{ + name: name, + resource: resource.Resource, + ttl: resource.TTL, + }) } } } else { - for _, resource := range resources { - filtered = append(filtered, resource) + for name, resource := range resources { + filtered = append(filtered, ttlResource{ + name: name, + resource: resource.Resource, + ttl: resource.TTL, + }) } } - return &RawResponse{ - Request: request, - Version: version, - Resources: filtered, - Heartbeat: heartbeat, - Ctx: ctx, + return &rawResponse{ + request: request, + version: version, + resources: filtered, + heartbeat: heartbeat, + ctx: ctx, } } // CreateDeltaWatch returns a watch for a delta xDS request which implements the Simple SnapshotCache. -func (cache *snapshotCache) CreateDeltaWatch(request *DeltaRequest, clientState SubscriptionState, value chan DeltaResponse) (func(), error) { - nodeID := cache.hash.ID(request.Node) +func (cache *snapshotCache) CreateDeltaWatch(request DeltaRequest, clientState SubscriptionState, value chan DeltaResponse) (func(), error) { + nodeID := cache.hash.ID(request.GetNode()) t := request.GetTypeUrl() cache.mu.Lock() @@ -493,7 +495,7 @@ func (cache *snapshotCache) CreateDeltaWatch(request *DeltaRequest, clientState info, ok := cache.status[nodeID] if !ok { - info = newStatusInfo(request.Node) + info = newStatusInfo(request.GetNode()) cache.status[nodeID] = info } @@ -538,20 +540,20 @@ func (cache *snapshotCache) CreateDeltaWatch(request *DeltaRequest, clientState } // Respond to a delta watch with the provided snapshot value. If the response is nil, there has been no state change. -func (cache *snapshotCache) respondDelta(ctx context.Context, snapshot ResourceSnapshot, request *DeltaRequest, value chan DeltaResponse, clientState SubscriptionState) (*RawDeltaResponse, error) { +func (cache *snapshotCache) respondDelta(ctx context.Context, snapshot ResourceSnapshot, request DeltaRequest, value chan DeltaResponse, clientState SubscriptionState) (*rawDeltaResponse, error) { resp := createDeltaResponse(ctx, request, clientState, resourceContainer{ - resourceMap: snapshot.GetResources(request.TypeUrl), - versionMap: snapshot.GetVersionMap(request.TypeUrl), - systemVersion: snapshot.GetVersion(request.TypeUrl), + resourceMap: snapshot.GetResources(request.GetTypeUrl()), + versionMap: snapshot.GetVersionMap(request.GetTypeUrl()), + systemVersion: snapshot.GetVersion(request.GetTypeUrl()), }) // Only send a response if there were changes // We want to respond immediately for the first request in a stream if it is wildcard, even if the response is empty // otherwise, envoy won't complete initialization - if len(resp.Resources) > 0 || len(resp.RemovedResources) > 0 || (clientState.IsWildcard() && request.ResponseNonce == "") { + if len(resp.resources) > 0 || len(resp.removedResources) > 0 || (clientState.IsWildcard() && clientState.IsFirstResponse()) { if cache.log != nil { cache.log.Debugf("node: %s, sending delta response for typeURL %s with resources: %v removed resources: %v with wildcard: %t", - request.GetNode().GetId(), request.TypeUrl, GetResourceNames(resp.Resources), resp.RemovedResources, clientState.IsWildcard()) + request.GetNode().GetId(), request.GetTypeUrl(), getResourceNames(resp.resources), resp.removedResources, clientState.IsWildcard()) } select { case value <- resp: @@ -582,8 +584,8 @@ func (cache *snapshotCache) cancelDeltaWatch(nodeID string, watchID int64) func( // Fetch implements the cache fetch function. // Fetch is called on multiple streams, so responding to individual names with the same version works. -func (cache *snapshotCache) Fetch(ctx context.Context, request *Request) (Response, error) { - nodeID := cache.hash.ID(request.Node) +func (cache *snapshotCache) Fetch(ctx context.Context, request FetchRequest) (Response, error) { + nodeID := cache.hash.ID(request.GetNode()) cache.mu.RLock() defer cache.mu.RUnlock() @@ -591,14 +593,22 @@ func (cache *snapshotCache) Fetch(ctx context.Context, request *Request) (Respon if snapshot, exists := cache.snapshots[nodeID]; exists { // Respond only if the request version is distinct from the current snapshot state. // It might be beneficial to hold the request since Envoy will re-attempt the refresh. - version := snapshot.GetVersion(request.TypeUrl) - if request.VersionInfo == version { + version := snapshot.GetVersion(request.GetTypeUrl()) + if request.GetVersionInfo() == version { cache.log.Warnf("skip fetch: version up to date") return nil, &types.SkipFetchError{} } - resources := snapshot.GetResourcesAndTTL(request.TypeUrl) - out := createResponse(ctx, request, resources, version, false) + resources := snapshot.GetResourcesAndTTL(request.GetTypeUrl()) + + clientState := stream.NewSubscriptionState(len(request.GetResourceNames()) == 0, nil) + requestedResources := map[string]struct{}{} + for _, name := range request.GetResourceNames() { + requestedResources[name] = struct{}{} + } + clientState.SetSubscribedResources(requestedResources) + + out := createResponse(ctx, request, clientState, resources, version, false) return out, nil } diff --git a/pkg/cache/v3/simple_test.go b/pkg/cache/v3/simple_test.go index 3179df45d3..01fd61570e 100644 --- a/pkg/cache/v3/simple_test.go +++ b/pkg/cache/v3/simple_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package cache_test +package cache import ( "context" @@ -33,7 +33,6 @@ import ( core "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" discovery "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3" "github.com/envoyproxy/go-control-plane/pkg/cache/types" - "github.com/envoyproxy/go-control-plane/pkg/cache/v3" rsrc "github.com/envoyproxy/go-control-plane/pkg/resource/v3" "github.com/envoyproxy/go-control-plane/pkg/server/stream/v3" "github.com/envoyproxy/go-control-plane/pkg/test/resource/v3" @@ -42,19 +41,19 @@ import ( type group struct{} const ( - key = "node" + nodeKey = "node" ) func (group) ID(node *core.Node) string { if node != nil { return node.Id } - return key + return nodeKey } var ( ttl = 2 * time.Second - snapshotWithTTL, _ = cache.NewSnapshotWithTTLs(fixture.version, map[rsrc.Type][]types.ResourceWithTTL{ + snapshotWithTTL, _ = NewSnapshotWithTTLs(fixture.version, map[rsrc.Type][]types.ResourceWithTTL{ rsrc.EndpointType: {{Resource: testEndpoint, TTL: &ttl}}, rsrc.ClusterType: {{Resource: testCluster}}, rsrc.RouteType: {{Resource: testRoute}, {Resource: testEmbeddedRoute}}, @@ -111,17 +110,17 @@ func (log logger) Errorf(format string, args ...interface{}) { func TestSnapshotCacheWithTTL(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - c := cache.NewSnapshotCacheWithHeartbeating(ctx, true, group{}, logger{t: t}, time.Second) + c := NewSnapshotCacheWithHeartbeating(ctx, true, group{}, logger{t: t}, time.Second) - if _, err := c.GetSnapshot(key); err == nil { - t.Errorf("unexpected snapshot found for key %q", key) + if _, err := c.GetSnapshot(nodeKey); err == nil { + t.Errorf("unexpected snapshot found for key %q", nodeKey) } - if err := c.SetSnapshot(context.Background(), key, snapshotWithTTL); err != nil { + if err := c.SetSnapshot(context.Background(), nodeKey, snapshotWithTTL); err != nil { t.Fatal(err) } - snap, err := c.GetSnapshot(key) + snap, err := c.GetSnapshot(nodeKey) if err != nil { t.Fatal(err) } @@ -136,7 +135,7 @@ func TestSnapshotCacheWithTTL(t *testing.T) { wg.Add(1) t.Run(typ, func(t *testing.T) { defer wg.Done() - value := make(chan cache.Response, 1) + value := make(chan Response, 1) _, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ]}, streamState, value) require.NoError(t, err) select { @@ -144,8 +143,8 @@ func TestSnapshotCacheWithTTL(t *testing.T) { if gotVersion, _ := out.GetVersion(); gotVersion != fixture.version { t.Errorf("got version %q, want %q", gotVersion, fixture.version) } - if !reflect.DeepEqual(cache.IndexResourcesByName(out.(*cache.RawResponse).Resources), snapshotWithTTL.GetResourcesAndTTL(typ)) { - t.Errorf("get resources %v, want %v", out.(*cache.RawResponse).Resources, snapshotWithTTL.GetResourcesAndTTL(typ)) + if !reflect.DeepEqual(IndexResourcesByName(out.(*rawResponse).resources), snapshotWithTTL.GetResourcesAndTTL(typ)) { + t.Errorf("get resources %v, want %v", out.(*rawResponse).resources, snapshotWithTTL.GetResourcesAndTTL(typ)) } // Update streamState for _, resource := range out.GetRequest().GetResourceNames() { @@ -168,7 +167,7 @@ func TestSnapshotCacheWithTTL(t *testing.T) { end := time.After(5 * time.Second) for { - value := make(chan cache.Response, 1) + value := make(chan Response, 1) cancel, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ], VersionInfo: fixture.version}, streamState, value) require.NoError(t, err) @@ -178,12 +177,12 @@ func TestSnapshotCacheWithTTL(t *testing.T) { if gotVersion, _ := out.GetVersion(); gotVersion != fixture.version { t.Errorf("got version %q, want %q", gotVersion, fixture.version) } - if !reflect.DeepEqual(cache.IndexResourcesByName(out.(*cache.RawResponse).Resources), snapshotWithTTL.GetResourcesAndTTL(typ)) { - t.Errorf("get resources %v, want %v", out.(*cache.RawResponse).Resources, snapshotWithTTL.GetResources(typ)) + if !reflect.DeepEqual(IndexResourcesByName(out.(*rawResponse).resources), snapshotWithTTL.GetResourcesAndTTL(typ)) { + t.Errorf("get resources %v, want %v", out.(*rawResponse).resources, snapshotWithTTL.GetResources(typ)) } - if !reflect.DeepEqual(cache.IndexResourcesByName(out.(*cache.RawResponse).Resources), snapshotWithTTL.GetResourcesAndTTL(typ)) { - t.Errorf("get resources %v, want %v", out.(*cache.RawResponse).Resources, snapshotWithTTL.GetResources(typ)) + if !reflect.DeepEqual(IndexResourcesByName(out.(*rawResponse).resources), snapshotWithTTL.GetResourcesAndTTL(typ)) { + t.Errorf("get resources %v, want %v", out.(*rawResponse).resources, snapshotWithTTL.GetResources(typ)) } updatesByType[typ]++ @@ -211,17 +210,17 @@ func TestSnapshotCacheWithTTL(t *testing.T) { } func TestSnapshotCache(t *testing.T) { - c := cache.NewSnapshotCache(true, group{}, logger{t: t}) + c := NewSnapshotCache(true, group{}, logger{t: t}) - if _, err := c.GetSnapshot(key); err == nil { - t.Errorf("unexpected snapshot found for key %q", key) + if _, err := c.GetSnapshot(nodeKey); err == nil { + t.Errorf("unexpected snapshot found for key %q", nodeKey) } - if err := c.SetSnapshot(context.Background(), key, fixture.snapshot()); err != nil { + if err := c.SetSnapshot(context.Background(), nodeKey, fixture.snapshot()); err != nil { t.Fatal(err) } - snap, err := c.GetSnapshot(key) + snap, err := c.GetSnapshot(nodeKey) if err != nil { t.Fatal(err) } @@ -231,7 +230,7 @@ func TestSnapshotCache(t *testing.T) { // try to get endpoints with incorrect list of names // should not receive response - value := make(chan cache.Response, 1) + value := make(chan Response, 1) streamState := stream.NewSubscriptionState(false, map[string]string{}) _, err = c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType, ResourceNames: []string{"none"}}, streamState, value) @@ -245,7 +244,7 @@ func TestSnapshotCache(t *testing.T) { for _, typ := range testTypes { t.Run(typ, func(t *testing.T) { - value := make(chan cache.Response, 1) + value := make(chan Response, 1) streamState := stream.NewSubscriptionState(false, map[string]string{}) _, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ]}, streamState, value) @@ -256,8 +255,8 @@ func TestSnapshotCache(t *testing.T) { if gotVersion, _ := out.GetVersion(); gotVersion != fixture.version { t.Errorf("got version %q, want %q", gotVersion, fixture.version) } - if !reflect.DeepEqual(cache.IndexResourcesByName(out.(*cache.RawResponse).Resources), snapshot.GetResourcesAndTTL(typ)) { - t.Errorf("get resources %v, want %v", out.(*cache.RawResponse).Resources, snapshot.GetResourcesAndTTL(typ)) + if !reflect.DeepEqual(IndexResourcesByName(out.(*rawResponse).resources), snapshot.GetResourcesAndTTL(typ)) { + t.Errorf("get resources %v, want %v", out.(*rawResponse).resources, snapshot.GetResourcesAndTTL(typ)) } case <-time.After(time.Second): t.Fatal("failed to receive snapshot response") @@ -267,8 +266,8 @@ func TestSnapshotCache(t *testing.T) { } func TestSnapshotCacheFetch(t *testing.T) { - c := cache.NewSnapshotCache(true, group{}, logger{t: t}) - if err := c.SetSnapshot(context.Background(), key, fixture.snapshot()); err != nil { + c := NewSnapshotCache(true, group{}, logger{t: t}) + if err := c.SetSnapshot(context.Background(), nodeKey, fixture.snapshot()); err != nil { t.Fatal(err) } @@ -298,15 +297,15 @@ func TestSnapshotCacheFetch(t *testing.T) { } func TestSnapshotCacheWatch(t *testing.T) { - c := cache.NewSnapshotCache(true, group{}, logger{t: t}) - watches := make(map[string]chan cache.Response) + c := NewSnapshotCache(true, group{}, logger{t: t}) + watches := make(map[string]chan Response) streamState := stream.NewSubscriptionState(false, map[string]string{}) for _, typ := range testTypes { - watches[typ] = make(chan cache.Response, 1) + watches[typ] = make(chan Response, 1) _, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ]}, streamState, watches[typ]) require.NoError(t, err) } - if err := c.SetSnapshot(context.Background(), key, fixture.snapshot()); err != nil { + if err := c.SetSnapshot(context.Background(), nodeKey, fixture.snapshot()); err != nil { t.Fatal(err) } for _, typ := range testTypes { @@ -317,8 +316,8 @@ func TestSnapshotCacheWatch(t *testing.T) { t.Errorf("got version %q, want %q", gotVersion, fixture.version) } snapshot := fixture.snapshot() - if !reflect.DeepEqual(cache.IndexResourcesByName(out.(*cache.RawResponse).Resources), snapshot.GetResourcesAndTTL(typ)) { - t.Errorf("get resources %v, want %v", out.(*cache.RawResponse).Resources, snapshot.GetResourcesAndTTL(typ)) + if !reflect.DeepEqual(IndexResourcesByName(out.(*rawResponse).resources), snapshot.GetResourcesAndTTL(typ)) { + t.Errorf("get resources %v, want %v", out.(*rawResponse).resources, snapshot.GetResourcesAndTTL(typ)) } for _, resource := range out.GetRequest().GetResourceNames() { streamState.GetKnownResources()[resource] = fixture.version @@ -331,22 +330,22 @@ func TestSnapshotCacheWatch(t *testing.T) { // open new watches with the latest version for _, typ := range testTypes { - watches[typ] = make(chan cache.Response, 1) + watches[typ] = make(chan Response, 1) _, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ], VersionInfo: fixture.version}, streamState, watches[typ]) require.NoError(t, err) } - if count := c.GetStatusInfo(key).GetNumWatches(); count != len(testTypes) { + if count := c.GetStatusInfo(nodeKey).GetNumWatches(); count != len(testTypes) { t.Errorf("watches should be created for the latest version: %d", count) } // set partially-versioned snapshot snapshot2 := fixture.snapshot() - snapshot2.Resources[types.Endpoint] = cache.NewResources(fixture.version2, []types.Resource{resource.MakeEndpoint(clusterName, 9090)}) - if err := c.SetSnapshot(context.Background(), key, snapshot2); err != nil { + snapshot2.Resources[types.Endpoint] = NewResources(fixture.version2, []types.Resource{resource.MakeEndpoint(clusterName, 9090)}) + if err := c.SetSnapshot(context.Background(), nodeKey, snapshot2); err != nil { t.Fatal(err) } - if count := c.GetStatusInfo(key).GetNumWatches(); count != len(testTypes)-1 { + if count := c.GetStatusInfo(nodeKey).GetNumWatches(); count != len(testTypes)-1 { t.Errorf("watches should be preserved for all but one: %d", count) } @@ -356,8 +355,8 @@ func TestSnapshotCacheWatch(t *testing.T) { if gotVersion, _ := out.GetVersion(); gotVersion != fixture.version2 { t.Errorf("got version %q, want %q", gotVersion, fixture.version2) } - if !reflect.DeepEqual(cache.IndexResourcesByName(out.(*cache.RawResponse).Resources), snapshot2.Resources[types.Endpoint].Items) { - t.Errorf("got resources %v, want %v", out.(*cache.RawResponse).Resources, snapshot2.Resources[types.Endpoint].Items) + if !reflect.DeepEqual(IndexResourcesByName(out.(*rawResponse).resources), snapshot2.Resources[types.Endpoint].Items) { + t.Errorf("got resources %v, want %v", out.(*rawResponse).resources, snapshot2.Resources[types.Endpoint].Items) } case <-time.After(time.Second): t.Fatal("failed to receive snapshot response") @@ -365,16 +364,16 @@ func TestSnapshotCacheWatch(t *testing.T) { } func TestConcurrentSetWatch(t *testing.T) { - c := cache.NewSnapshotCache(false, group{}, logger{t: t}) + c := NewSnapshotCache(false, group{}, logger{t: t}) for i := 0; i < 50; i++ { i := i t.Run(fmt.Sprintf("worker%d", i), func(t *testing.T) { t.Parallel() id := t.Name() - value := make(chan cache.Response, 1) + value := make(chan Response, 1) if i < 25 { - snap := cache.Snapshot{} - snap.Resources[types.Endpoint] = cache.NewResources(fmt.Sprintf("v%d", i), []types.Resource{resource.MakeEndpoint(clusterName, uint32(i))}) + snap := Snapshot{} + snap.Resources[types.Endpoint] = NewResources(fmt.Sprintf("v%d", i), []types.Resource{resource.MakeEndpoint(clusterName, uint32(i))}) if err := c.SetSnapshot(context.Background(), id, &snap); err != nil { t.Fatalf("failed to set snapshot %q: %s", id, err) } @@ -393,10 +392,10 @@ func TestConcurrentSetWatch(t *testing.T) { } func TestSnapshotCacheWatchCancel(t *testing.T) { - c := cache.NewSnapshotCache(true, group{}, logger{t: t}) + c := NewSnapshotCache(true, group{}, logger{t: t}) streamState := stream.NewSubscriptionState(false, map[string]string{}) for _, typ := range testTypes { - value := make(chan cache.Response, 1) + value := make(chan Response, 1) cancel, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: typ, ResourceNames: names[typ]}, streamState, value) require.NoError(t, err) cancel() @@ -407,7 +406,7 @@ func TestSnapshotCacheWatchCancel(t *testing.T) { } for _, typ := range testTypes { - if count := c.GetStatusInfo(key).GetNumWatches(); count > 0 { + if count := c.GetStatusInfo(nodeKey).GetNumWatches(); count > 0 { t.Errorf("watches should be released for %s", typ) } } @@ -418,10 +417,10 @@ func TestSnapshotCacheWatchCancel(t *testing.T) { } func TestSnapshotCacheWatchTimeout(t *testing.T) { - c := cache.NewSnapshotCache(true, group{}, logger{t: t}) + c := NewSnapshotCache(true, group{}, logger{t: t}) // Create a non-buffered channel that will block sends. - watchCh := make(chan cache.Response) + watchCh := make(chan Response) streamState := stream.NewSubscriptionState(false, map[string]string{}) _, err := c.CreateWatch(&discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType, ResourceNames: names[rsrc.EndpointType]}, streamState, watchCh) @@ -431,20 +430,20 @@ func TestSnapshotCacheWatchTimeout(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond) defer cancel() - err = c.SetSnapshot(ctx, key, fixture.snapshot()) + err = c.SetSnapshot(ctx, nodeKey, fixture.snapshot()) assert.EqualError(t, err, context.Canceled.Error()) // Now reset the snapshot with a consuming channel. This verifies that if setting the snapshot fails, // we can retry by setting the same snapshot. In other words, we keep the watch open even if we failed // to respond to it within the deadline. - watchTriggeredCh := make(chan cache.Response) + watchTriggeredCh := make(chan Response) go func() { response := <-watchCh watchTriggeredCh <- response close(watchTriggeredCh) }() - err = c.SetSnapshot(context.WithValue(context.Background(), testKey{}, "bar"), key, fixture.snapshot()) + err = c.SetSnapshot(context.WithValue(context.Background(), testKey{}, "bar"), nodeKey, fixture.snapshot()) assert.NoError(t, err) // The channel should get closed due to the watch trigger. @@ -461,9 +460,9 @@ func TestSnapshotCreateWatchWithResourcePreviouslyNotRequested(t *testing.T) { clusterName2 := "clusterName2" routeName2 := "routeName2" listenerName2 := "listenerName2" - c := cache.NewSnapshotCache(false, group{}, logger{t: t}) + c := NewSnapshotCache(false, group{}, logger{t: t}) - snapshot2, _ := cache.NewSnapshot(fixture.version, map[rsrc.Type][]types.Resource{ + snapshot2, _ := NewSnapshot(fixture.version, map[rsrc.Type][]types.Resource{ rsrc.EndpointType: {testEndpoint, resource.MakeEndpoint(clusterName2, 8080)}, rsrc.ClusterType: {testCluster, resource.MakeCluster(resource.Ads, clusterName2)}, rsrc.RouteType: {testRoute, resource.MakeRouteConfig(routeName2, clusterName2)}, @@ -472,10 +471,10 @@ func TestSnapshotCreateWatchWithResourcePreviouslyNotRequested(t *testing.T) { rsrc.SecretType: {}, rsrc.ExtensionConfigType: {}, }) - if err := c.SetSnapshot(context.Background(), key, snapshot2); err != nil { + if err := c.SetSnapshot(context.Background(), nodeKey, snapshot2); err != nil { t.Fatal(err) } - watch := make(chan cache.Response) + watch := make(chan Response) // Request resource with name=ClusterName go func() { @@ -491,8 +490,8 @@ func TestSnapshotCreateWatchWithResourcePreviouslyNotRequested(t *testing.T) { t.Errorf("got version %q, want %q", gotVersion, fixture.version) } want := map[string]types.ResourceWithTTL{clusterName: snapshot2.Resources[types.Endpoint].Items[clusterName]} - if !reflect.DeepEqual(cache.IndexResourcesByName(out.(*cache.RawResponse).Resources), want) { - t.Errorf("got resources %v, want %v", out.(*cache.RawResponse).Resources, want) + if !reflect.DeepEqual(IndexResourcesByName(out.(*rawResponse).resources), want) { + t.Errorf("got resources %v, want %v", out.(*rawResponse).resources, want) } case <-time.After(time.Second): t.Fatal("failed to receive snapshot response") @@ -512,8 +511,8 @@ func TestSnapshotCreateWatchWithResourcePreviouslyNotRequested(t *testing.T) { if gotVersion, _ := out.GetVersion(); gotVersion != fixture.version { t.Errorf("got version %q, want %q", gotVersion, fixture.version) } - if !reflect.DeepEqual(cache.IndexResourcesByName(out.(*cache.RawResponse).Resources), snapshot2.Resources[types.Endpoint].Items) { - t.Errorf("got resources %v, want %v", out.(*cache.RawResponse).Resources, snapshot2.Resources[types.Endpoint].Items) + if !reflect.DeepEqual(IndexResourcesByName(out.(*rawResponse).resources), snapshot2.Resources[types.Endpoint].Items) { + t.Errorf("got resources %v, want %v", out.(*rawResponse).resources, snapshot2.Resources[types.Endpoint].Items) } case <-time.After(time.Second): t.Fatal("failed to receive snapshot response") @@ -532,12 +531,12 @@ func TestSnapshotCreateWatchWithResourcePreviouslyNotRequested(t *testing.T) { } func TestSnapshotClear(t *testing.T) { - c := cache.NewSnapshotCache(true, group{}, logger{t: t}) - if err := c.SetSnapshot(context.Background(), key, fixture.snapshot()); err != nil { + c := NewSnapshotCache(true, group{}, logger{t: t}) + if err := c.SetSnapshot(context.Background(), nodeKey, fixture.snapshot()); err != nil { t.Fatal(err) } - c.ClearSnapshot(key) - if empty := c.GetStatusInfo(key); empty != nil { + c.ClearSnapshot(nodeKey) + if empty := c.GetStatusInfo(nodeKey); empty != nil { t.Errorf("cache should be cleared") } if keys := c.GetStatusKeys(); len(keys) != 0 { @@ -600,7 +599,7 @@ func TestSnapshotSingleResourceFetch(t *testing.T) { durationTypeURL := "type.googleapis.com/" + string(proto.MessageName(&durationpb.Duration{})) anyDuration := func(d time.Duration) *anypb.Any { - bytes, err := cache.MarshalResource(durationpb.New(d)) + bytes, err := MarshalResource(durationpb.New(d)) require.NoError(t, err) return &anypb.Any{ TypeUrl: durationTypeURL, @@ -614,8 +613,8 @@ func TestSnapshotSingleResourceFetch(t *testing.T) { return dst } - c := cache.NewSnapshotCache(true, group{}, logger{t: t}) - require.NoError(t, c.SetSnapshot(context.Background(), key, &singleResourceSnapshot{ + c := NewSnapshotCache(true, group{}, logger{t: t}) + require.NoError(t, c.SetSnapshot(context.Background(), nodeKey, &singleResourceSnapshot{ version: "version-one", typeurl: durationTypeURL, name: "one-second", @@ -645,16 +644,16 @@ func TestSnapshotSingleResourceFetch(t *testing.T) { func TestAvertPanicForWatchOnNonExistentSnapshot(t *testing.T) { ctx := context.Background() - c := cache.NewSnapshotCacheWithHeartbeating(ctx, false, cache.IDHash{}, nil, time.Millisecond) + c := NewSnapshotCacheWithHeartbeating(ctx, false, IDHash{}, nil, time.Millisecond) // Create watch. - req := &cache.Request{ + req := &discovery.DiscoveryRequest{ Node: &core.Node{Id: "test"}, ResourceNames: []string{"rtds"}, TypeUrl: rsrc.RuntimeType, } ss := stream.NewSubscriptionState(false, map[string]string{"cluster": "abcdef"}) - responder := make(chan cache.Response) + responder := make(chan Response) _, err := c.CreateWatch(req, &ss, responder) require.NoError(t, err) diff --git a/pkg/cache/v3/sotw.go b/pkg/cache/v3/sotw.go new file mode 100644 index 0000000000..b07b9d127c --- /dev/null +++ b/pkg/cache/v3/sotw.go @@ -0,0 +1,241 @@ +package cache + +import ( + "context" + "errors" + "fmt" + "sync/atomic" + "time" + + discovery "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3" + "github.com/envoyproxy/go-control-plane/pkg/cache/types" + "google.golang.org/protobuf/types/known/anypb" + "google.golang.org/protobuf/types/known/durationpb" +) + +type resourceWithName interface { + // getResource is returning the resource which can be directly serialized for the response + getResource() types.Resource + getName() string +} + +type resourceWithTTLAndName interface { + resourceWithName + getTTL() *time.Duration +} + +type ttlResource struct { + resource types.Resource + name string + ttl *time.Duration +} + +func (r ttlResource) getResource() types.Resource { + return r.resource +} + +func (r ttlResource) getName() string { + return r.name +} + +func (r ttlResource) getTTL() *time.Duration { + return r.ttl +} + +var _ Response = &rawResponse{} + +// RawResponse is a pre-serialized xDS response containing the raw resources to +// be included in the final Discovery Response. +type rawResponse struct { + // Request is the original request. + request Request + + // Version of the resources as tracked by the cache for the given type. + // Proxy responds with this version as an acknowledgement. + version string + + // Resources to be included in the response. + resources []resourceWithTTLAndName + + // Whether this is a heartbeat response. For xDS versions that support TTL, this + // will be converted into a response that doesn't contain the actual resource protobuf. + // This allows for more lightweight updates that server only to update the TTL timer. + heartbeat bool + + // Context provided at the time of response creation. This allows associating additional + // information with a generated response. + ctx context.Context + + // marshaledResponse holds an atomic reference to the serialized discovery response. + marshaledResponse atomic.Value +} + +func NewTestRawResponse(request Request, version string, resources []types.ResourceWithTTL) *rawResponse { + var namedResources []resourceWithTTLAndName + for _, res := range resources { + namedResources = append(namedResources, ttlResource{ + name: GetResourceName(res.Resource), + resource: res.Resource, + ttl: res.TTL, + }) + } + return &rawResponse{ + request: request, + version: version, + resources: namedResources, + } +} + +// GetDiscoveryResponse performs the marshaling the first time its called and uses the cached response subsequently. +// This is necessary because the marshaled response does not change across the calls. +// This caching behavior is important in high throughput scenarios because grpc marshaling has a cost and it drives the cpu utilization under load. +func (r *rawResponse) GetDiscoveryResponse() (*discovery.DiscoveryResponse, error) { + marshaledResponse := r.marshaledResponse.Load() + + if marshaledResponse == nil { + marshaledResources := make([]*anypb.Any, len(r.resources)) + + for i, resource := range r.resources { + maybeTtldResource, resourceType, err := r.maybeCreateTTLResource(resource) + if err != nil { + return nil, err + } + marshaledResource, err := MarshalResource(maybeTtldResource.getResource()) + if err != nil { + return nil, err + } + marshaledResources[i] = &anypb.Any{ + TypeUrl: resourceType, + Value: marshaledResource, + } + } + + marshaledResponse = &discovery.DiscoveryResponse{ + VersionInfo: r.version, + Resources: marshaledResources, + TypeUrl: r.request.GetTypeUrl(), + } + + r.marshaledResponse.Store(marshaledResponse) + } + + return marshaledResponse.(*discovery.DiscoveryResponse), nil +} + +func (r *rawResponse) GetReturnedResources() []string { + names := make([]string, 0, len(r.resources)) + for _, res := range r.resources { + names = append(names, res.getName()) + } + return names +} + +func (r *rawResponse) GetRequest() Request { + return r.request +} + +// GetDiscoveryRequest returns the original Discovery Request if available +func (r *rawResponse) GetDiscoveryRequest() (*discovery.DiscoveryRequest, error) { + if r.request == nil { + return nil, errors.New("no request was provided") + } else if req, ok := r.request.(*discovery.DiscoveryRequest); ok { + return req, nil + } else { + return nil, errors.New("provided request is not a DiscoveryRequest") + } +} + +func (r *rawResponse) GetContext() context.Context { + return r.ctx +} + +// GetVersion returns the response version. +func (r *rawResponse) GetVersion() (string, error) { + return r.version, nil +} + +type namedResource struct { + types.Resource + name string +} + +func (r namedResource) getResource() types.Resource { + return r.Resource +} + +func (r namedResource) getName() string { + return r.name +} + +func (r *rawResponse) maybeCreateTTLResource(resource resourceWithTTLAndName) (resourceWithName, string, error) { + if resource.getTTL() != nil { + wrappedResource := &discovery.Resource{ + Name: resource.getName(), + Ttl: durationpb.New(*resource.getTTL()), + } + + if !r.heartbeat { + rsrc, err := anypb.New(resource.getResource()) + if err != nil { + return namedResource{}, "", err + } + rsrc.TypeUrl = r.request.GetTypeUrl() + wrappedResource.Resource = rsrc + } + + return namedResource{ + Resource: wrappedResource, + name: resource.getName(), + }, deltaResourceTypeURL, nil + } + + return resource, r.request.GetTypeUrl(), nil +} + +var _ Response = &passthroughResponse{} + +// passthroughResponse is a pre constructed xDS response that need not go through marshaling transformations. +type passthroughResponse struct { + // discoveryRequest is the original request. + discoveryRequest *discovery.DiscoveryRequest + + // The discovery response that needs to be sent as is, without any marshaling transformations. + discoveryResponse *discovery.DiscoveryResponse + + ctx context.Context +} + +func (r *passthroughResponse) GetDiscoveryResponse() (*discovery.DiscoveryResponse, error) { + return r.discoveryResponse, nil +} + +func (r *passthroughResponse) GetReturnedResources() []string { + if r.discoveryResponse == nil { + return nil + } + names := make([]string, 0, len(r.discoveryResponse.Resources)) + for _, res := range r.discoveryResponse.Resources { + names = append(names, GetResourceName(res)) + } + return names +} + +func (r *passthroughResponse) GetRequest() Request { + return r.discoveryRequest +} + +func (r *passthroughResponse) GetDiscoveryRequest() (*discovery.DiscoveryRequest, error) { + return r.discoveryRequest, nil +} + +func (r *passthroughResponse) GetContext() context.Context { + return r.ctx +} + +// GetVersion returns the response version. +func (r *passthroughResponse) GetVersion() (string, error) { + if r.discoveryResponse != nil { + return r.discoveryResponse.VersionInfo, nil + } + return "", fmt.Errorf("DiscoveryResponse is nil") +} diff --git a/pkg/cache/v3/status.go b/pkg/cache/v3/status.go index 0495261e52..c86463393c 100644 --- a/pkg/cache/v3/status.go +++ b/pkg/cache/v3/status.go @@ -85,21 +85,24 @@ type statusInfo struct { // ResponseWatch is a watch record keeping both the request and an open channel for the response. type ResponseWatch struct { // Request is the original request for the watch. - Request *Request + Request Request // Response is the channel to push responses to. Response chan Response + + // subscriptionState is the current state of the given type subscription on the stream + subscriptionState SubscriptionState } // DeltaResponseWatch is a watch record keeping both the delta request and an open channel for the delta response. type DeltaResponseWatch struct { // Request is the most recent delta request for the watch - Request *DeltaRequest + Request DeltaRequest // Response is the channel to push the delta responses to Response chan DeltaResponse - // VersionMap for the stream + // subscriptionState is the current state of the given type subscription on the stream subscriptionState SubscriptionState } @@ -167,7 +170,7 @@ func (info *statusInfo) orderResponseWatches() { for id, watch := range info.watches { info.orderedWatches[index] = key{ ID: id, - TypeURL: watch.Request.TypeUrl, + TypeURL: watch.Request.GetTypeUrl(), } index++ } diff --git a/pkg/server/delta/v3/server.go b/pkg/server/delta/v3/server.go index af14293f51..cba07114a5 100644 --- a/pkg/server/delta/v3/server.go +++ b/pkg/server/delta/v3/server.go @@ -35,8 +35,6 @@ type Callbacks interface { OnStreamDeltaResponse(int64, *discovery.DeltaDiscoveryRequest, *discovery.DeltaDiscoveryResponse) } -var deltaErrorResponse = &cache.RawDeltaResponse{} - type server struct { cache cache.ConfigWatcher callbacks Callbacks @@ -94,10 +92,15 @@ func (s *server) processDelta(str stream.DeltaStream, reqCh <-chan *discovery.De return "", err } + req, err := resp.GetDeltaDiscoveryRequest() + if err != nil { + return "", err + } + streamNonce = streamNonce + 1 response.Nonce = strconv.FormatInt(streamNonce, 10) if s.callbacks != nil { - s.callbacks.OnStreamDeltaResponse(streamID, resp.GetDeltaRequest(), response) + s.callbacks.OnStreamDeltaResponse(streamID, req, response) } return response.Nonce, str.Send(response) @@ -119,7 +122,7 @@ func (s *server) processDelta(str stream.DeltaStream, reqCh <-chan *discovery.De } typ := resp.GetDeltaRequest().GetTypeUrl() - if resp == deltaErrorResponse { + if _, err := resp.GetDeltaDiscoveryResponse(); err != nil { return status.Errorf(codes.Unavailable, typ+" watch failed") } @@ -131,7 +134,15 @@ func (s *server) processDelta(str stream.DeltaStream, reqCh <-chan *discovery.De watch := watches.deltaWatches[typ] watch.nonce = nonce - watch.state.SetKnownResources(resp.GetNextVersionMap()) + for resource, version := range resp.GetReturnedResources() { + if version != "" { + watch.state.GetKnownResources()[resource] = version + } else { + // The response indicates that the resource is deleted + // We keep the knowledge that the resource does not exist + watch.state.GetKnownResources()[resource] = "" + } + } watches.deltaWatches[typ] = watch case req, more := <-reqCh: // input stream ended or errored out @@ -256,10 +267,13 @@ func (s *server) unsubscribe(resources []string, streamState *stream.Subscriptio // * a resource is explicitly unsubscribed by name // Then the control-plane must return in the response whether the resource is removed (if no longer present for this node) // or still existing. In the latter case the entire resource must be returned, same as if it had been created or updated - // To achieve that, we mark the resource as having been returned with an empty version. While creating the response, the cache will either: + // To achieve that, we mark the resource as having been returned with a fake version. While creating the response, the cache will either: // * detect the version change, and return the resource (as an update) // * detect the resource deletion, and set it as removed in the response - streamState.GetKnownResources()[resource] = "" + streamState.GetKnownResources()[resource] = "unsubscribed" + } else { + // If a resource is unsubscribed, we must assume that envoy no longer tracks it + delete(streamState.GetKnownResources(), resource) } delete(sv, resource) } diff --git a/pkg/server/sotw/v3/ads.go b/pkg/server/sotw/v3/ads.go index ae2ac7f40e..3326029b82 100644 --- a/pkg/server/sotw/v3/ads.go +++ b/pkg/server/sotw/v3/ads.go @@ -8,19 +8,12 @@ import ( "github.com/envoyproxy/go-control-plane/pkg/cache/types" "github.com/envoyproxy/go-control-plane/pkg/cache/v3" "github.com/envoyproxy/go-control-plane/pkg/resource/v3" - "github.com/envoyproxy/go-control-plane/pkg/server/stream/v3" ) // process handles a bi-di stream request func (s *server) processADS(sw *streamWrapper, reqCh chan *discovery.DiscoveryRequest, defaultTypeURL string) error { // We make a responder channel here so we can multiplex responses from the dynamic channels. - sw.watches.addWatch(resource.AnyType, &watch{ - // Create a buffered channel the size of the known resource types. - response: make(chan cache.Response, types.UnknownType), - cancel: func() { - close(sw.watches.responders[resource.AnyType].response) - }, - }) + muxChannel := make(chan cache.Response, types.UnknownType) process := func(resp cache.Response) error { nonce, err := sw.send(resp) @@ -28,7 +21,7 @@ func (s *server) processADS(sw *streamWrapper, reqCh chan *discovery.DiscoveryRe return err } - sw.watches.responders[resp.GetRequest().TypeUrl].nonce = nonce + sw.subscriptions.responders[resp.GetRequest().GetTypeUrl()].nonce = nonce return nil } @@ -43,8 +36,8 @@ func (s *server) processADS(sw *streamWrapper, reqCh chan *discovery.DiscoveryRe for { select { // We watch the multiplexed ADS channel for incoming responses. - case res := <-sw.watches.responders[resource.AnyType].response: - if res.GetRequest().TypeUrl != typeURL { + case res := <-muxChannel: + if res.GetRequest().GetTypeUrl() != typeURL { if err := process(res); err != nil { return err } @@ -64,7 +57,7 @@ func (s *server) processADS(sw *streamWrapper, reqCh chan *discovery.DiscoveryRe case <-s.ctx.Done(): return nil // We only watch the multiplexed channel since all values will come through from process. - case res := <-sw.watches.responders[resource.AnyType].response: + case res := <-muxChannel: if err := process(res); err != nil { return status.Errorf(codes.Unavailable, err.Error()) } @@ -102,62 +95,46 @@ func (s *server) processADS(sw *streamWrapper, reqCh chan *discovery.DiscoveryRe } } - streamState, ok := sw.streamStates[req.TypeUrl] - if !ok { - // Supports legacy wildcard mode - // Wildcard will be set to true if no resource is set - streamState = stream.NewSubscriptionState(len(req.ResourceNames) == 0, nil) - } - - // ToDo: track ACK through subscription state - if lastResponse, ok := sw.lastDiscoveryResponses[req.TypeUrl]; ok { - if lastResponse.nonce == "" || lastResponse.nonce == nonce { - // Let's record Resource names that a client has received. - streamState.SetKnownResources(lastResponse.resources) - } - } - - updateSubscriptionResources(req, &streamState) - typeURL := req.GetTypeUrl() - // Use the multiplexed channel for new watches. - responder := sw.watches.responders[resource.AnyType].response - if w, ok := sw.watches.responders[typeURL]; ok { - // We've found a pre-existing watch, lets check and update if needed. - // If these requirements aren't satisfied, leave an open watch. - if w.nonce == "" || w.nonce == nonce { - w.close() - - // Only process if we have an existing watch otherwise go ahead and create. - if err := processAllExcept(typeURL); err != nil { - return err - } - cancel, err := s.cache.CreateWatch(req, streamState, responder) - if err != nil { - return err - } - - sw.watches.addWatch(typeURL, &watch{ - cancel: cancel, - response: responder, - }) + // sub must not be modified until any potential watch is closed + // as we commit in the Cache interface that it is immutable for the lifetime of the watch + sub, ok := sw.subscriptions.responders[typeURL] + if ok { + // Existing subscription, lets check and update if needed. + // If these requirements aren't satisfied, leave an open watch. + if sub.nonce == "" || sub.nonce == nonce { + sub.watch.close() + } else { + // The request does not match the previous nonce. + // Currently we drop the new request in this context + break } - } else { - // No pre-existing watch exists, let's create one. - // We need to precompute the watches first then open a watch in the cache. - cancel, err := s.cache.CreateWatch(req, streamState, responder) - if err != nil { + + if err := processAllExcept(typeURL); err != nil { return err } - sw.watches.addWatch(typeURL, &watch{ - cancel: cancel, - response: responder, - }) + // Record Resource names that a client has received. + sub.state.SetKnownResources(sub.lastResponseResources) + } else { + // Supports legacy wildcard mode + // Wildcard will be set to true if no resource is set + sub = newSubscription(len(req.ResourceNames) == 0) + } + + updateSubscriptionResources(req, &sub.state) + + cancel, err := s.cache.CreateWatch(req, sub.state, muxChannel) + if err != nil { + return err + } + sub.watch = watch{ + cancel: cancel, + response: muxChannel, } - sw.streamStates[req.TypeUrl] = streamState + sw.subscriptions.addSubscription(req.TypeUrl, sub) } } } diff --git a/pkg/server/sotw/v3/server.go b/pkg/server/sotw/v3/server.go index 7fcb64b013..c8bfd86f16 100644 --- a/pkg/server/sotw/v3/server.go +++ b/pkg/server/sotw/v3/server.go @@ -83,16 +83,14 @@ type server struct { type streamWrapper struct { stream stream.Stream // parent stream object ID int64 // stream ID in relation to total stream count - nonce int64 // nonce per stream - watches watches // collection of stack allocated watchers per request type + nonce int64 // global nonce per stream, used to seed new messages callbacks Callbacks // callbacks for performing actions through stream lifecycle node *core.Node // registered xDS client - // The below fields are used for tracking resource - // cache state and should be maintained per stream. - streamStates map[string]stream.SubscriptionState - lastDiscoveryResponses map[string]lastDiscoveryResponse + // subscriptions track the current resource types requested, including the current watches + // and subscription states + subscriptions subscriptions } // Send packages the necessary resources before sending on the gRPC stream, @@ -115,18 +113,32 @@ func (s *streamWrapper) send(resp cache.Response) (string, error) { return "", err } - lastResponse := lastDiscoveryResponse{ - nonce: out.Nonce, - resources: make(map[string]string), + req, err := resp.GetDiscoveryRequest() + if err != nil { + return "", err + } + + subscription := s.subscriptions.responders[resp.GetRequest().GetTypeUrl()] + + resources := make(map[string]string, len(resp.GetReturnedResources())) + + // Add all resources explicitly requested and not returned + // The absence of the resource in the response is knowledge from the client side + // This is also used to track resources addition from one request to the next one + for r := range subscription.state.GetSubscribedResources() { + resources[r] = "" } - for _, r := range resp.GetRequest().ResourceNames { - lastResponse.resources[r] = version + + // Fill in the resources actually returned + for _, r := range resp.GetReturnedResources() { + resources[r] = version } - s.lastDiscoveryResponses[resp.GetRequest().TypeUrl] = lastResponse + + subscription.lastResponseResources = resources // Register with the callbacks provided that we are sending the response. if s.callbacks != nil { - s.callbacks.OnStreamResponse(resp.GetContext(), s.ID, resp.GetRequest(), out) + s.callbacks.OnStreamResponse(resp.GetContext(), s.ID, req, out) } return out.Nonce, s.stream.Send(out) @@ -134,21 +146,12 @@ func (s *streamWrapper) send(resp cache.Response) (string, error) { // Shutdown closes all open watches, and notifies API consumers the stream has closed. func (s *streamWrapper) shutdown() { - s.watches.close() + s.subscriptions.close() if s.callbacks != nil { s.callbacks.OnStreamClosed(s.ID, s.node) } } -// Discovery response that is sent over GRPC stream. -// We need to record what resource names are already sent to a client -// So if the client requests a new name we can respond back -// regardless current snapshot version (even if it is not changed yet) -type lastDiscoveryResponse struct { - nonce string - resources map[string]string -} - // StreamHandler converts a blocking read call to channels and initiates stream processing func (s *server) StreamHandler(stream stream.Stream, typeURL string) error { // a channel for receiving incoming requests diff --git a/pkg/server/sotw/v3/watches.go b/pkg/server/sotw/v3/watches.go index d781f663e6..35a0f7e577 100644 --- a/pkg/server/sotw/v3/watches.go +++ b/pkg/server/sotw/v3/watches.go @@ -7,39 +7,55 @@ import ( discovery "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3" "github.com/envoyproxy/go-control-plane/pkg/cache/types" "github.com/envoyproxy/go-control-plane/pkg/cache/v3" + "github.com/envoyproxy/go-control-plane/pkg/server/stream/v3" ) -// watches for all xDS resource types -type watches struct { - responders map[string]*watch +// subscription is the state of the stream for a given resource type (cannot be AnyType) +type subscription struct { + nonce string + watch watch + state stream.SubscriptionState + // ToDo: move tracking of previous responses in state + lastResponseResources map[string]string +} + +func newSubscription(wildcard bool) *subscription { + return &subscription{ + state: stream.NewSubscriptionState(wildcard, nil), + } +} + +// subscriptions for all xDS resource types +type subscriptions struct { + responders map[string]*subscription // cases is a dynamic select case for the watched channels. cases []reflect.SelectCase } -// newWatches creates and initializes watches. -func newWatches() watches { - return watches{ - responders: make(map[string]*watch, int(types.UnknownType)), +// newSubscriptions creates and initializes subscriptions. +func newSubscriptions() subscriptions { + return subscriptions{ + responders: make(map[string]*subscription, int(types.UnknownType)), cases: make([]reflect.SelectCase, 0), } } -// addWatch creates a new watch entry in the watches map. -// Watches are sorted by typeURL. -func (w *watches) addWatch(typeURL string, watch *watch) { - w.responders[typeURL] = watch +// addSubscription creates a new subscription entry in the subscriptions map. +// subscriptions are sorted by typeURL. +func (w *subscriptions) addSubscription(typeURL string, subscription *subscription) { + w.responders[typeURL] = subscription } // close all open watches -func (w *watches) close() { - for _, watch := range w.responders { - watch.close() +func (w *subscriptions) close() { + for _, subscription := range w.responders { + subscription.watch.close() } } // recomputeWatches rebuilds the known list of dynamic channels if needed -func (w *watches) recompute(ctx context.Context, req <-chan *discovery.DiscoveryRequest) { +func (w *subscriptions) recomputeWatches(ctx context.Context, req <-chan *discovery.DiscoveryRequest) { w.cases = w.cases[:0] // Clear the existing cases while retaining capacity. w.cases = append(w.cases, @@ -52,10 +68,10 @@ func (w *watches) recompute(ctx context.Context, req <-chan *discovery.Discovery }, ) - for _, watch := range w.responders { + for _, subscription := range w.responders { w.cases = append(w.cases, reflect.SelectCase{ Dir: reflect.SelectRecv, - Chan: reflect.ValueOf(watch.response), + Chan: reflect.ValueOf(subscription.watch.response), }) } } @@ -63,12 +79,11 @@ func (w *watches) recompute(ctx context.Context, req <-chan *discovery.Discovery // watch contains the necessary modifiable data for receiving resource responses type watch struct { cancel func() - nonce string response chan cache.Response } // close cancels an open watch -func (w *watch) close() { +func (w watch) close() { if w.cancel != nil { w.cancel() } diff --git a/pkg/server/sotw/v3/xds.go b/pkg/server/sotw/v3/xds.go index 59d90b6257..dc8ffd9e29 100644 --- a/pkg/server/sotw/v3/xds.go +++ b/pkg/server/sotw/v3/xds.go @@ -26,9 +26,7 @@ func (s *server) process(str stream.Stream, reqCh chan *discovery.DiscoveryReque node: &core.Node{}, // node may only be set on the first discovery request // a collection of stack allocated watches per request type. - watches: newWatches(), - streamStates: make(map[string]stream.SubscriptionState), - lastDiscoveryResponses: make(map[string]lastDiscoveryResponse), + subscriptions: newSubscriptions(), } // cleanup once our stream has ended. @@ -43,14 +41,14 @@ func (s *server) process(str stream.Stream, reqCh chan *discovery.DiscoveryReque // do an initial recompute so we can load the first 2 channels: // <-reqCh // s.ctx.Done() - sw.watches.recompute(s.ctx, reqCh) + sw.subscriptions.recomputeWatches(s.ctx, reqCh) for { // The list of select cases looks like this: // 0: <- ctx.Done // 1: <- reqCh // 2...: per type watches - index, value, ok := reflect.Select(sw.watches.cases) + index, value, ok := reflect.Select(sw.subscriptions.cases) switch index { // ctx.Done() -> if we receive a value here we return // as no further computation is needed @@ -110,57 +108,48 @@ func (s *server) process(str stream.Stream, reqCh chan *discovery.DiscoveryReque req.TypeUrl = defaultTypeURL } - streamState := sw.streamStates[req.TypeUrl] - if s.callbacks != nil { if err := s.callbacks.OnStreamRequest(sw.ID, req); err != nil { return err } } - if lastResponse, ok := sw.lastDiscoveryResponses[req.TypeUrl]; ok { - if lastResponse.nonce == "" || lastResponse.nonce == nonce { - // Let's record Resource names that a client has received. - streamState.SetKnownResources(lastResponse.resources) + // sub must not be modified until any potential watch is closed + // as we commit in the Cache interface that it is immutable for the lifetime of the watch + sub, ok := sw.subscriptions.responders[req.GetTypeUrl()] + if ok { + // Existing subscription, lets check and update if needed. + // If these requirements aren't satisfied, leave an open watch. + if sub.nonce == "" || sub.nonce == nonce { + sub.watch.close() + } else { + // The request does not match the previous nonce. + // Currently we drop the new request in this context + break } + + // Record Resource names that a client has received. + sub.state.SetKnownResources(sub.lastResponseResources) + } else { + sub = newSubscription(len(req.ResourceNames) == 0) } - updateSubscriptionResources(req, &streamState) + updateSubscriptionResources(req, &sub.state) - typeURL := req.GetTypeUrl() responder := make(chan cache.Response, 1) - if w, ok := sw.watches.responders[typeURL]; ok { - // We've found a pre-existing watch, lets check and update if needed. - // If these requirements aren't satisfied, leave an open watch. - if w.nonce == "" || w.nonce == nonce { - w.close() - - cancel, err := s.cache.CreateWatch(req, streamState, responder) - if err != nil { - return err - } - sw.watches.addWatch(typeURL, &watch{ - cancel: cancel, - response: responder, - }) - } - } else { - // No pre-existing watch exists, let's create one. - // We need to precompute the watches first then open a watch in the cache. - cancel, err := s.cache.CreateWatch(req, streamState, responder) - if err != nil { - return err - } - sw.watches.addWatch(typeURL, &watch{ - cancel: cancel, - response: responder, - }) + cancel, err := s.cache.CreateWatch(req, sub.state, responder) + if err != nil { + return err + } + sub.watch = watch{ + cancel: cancel, + response: responder, } - sw.streamStates[req.TypeUrl] = streamState + sw.subscriptions.addSubscription(req.TypeUrl, sub) // Recompute the dynamic select cases for this stream. - sw.watches.recompute(s.ctx, reqCh) + sw.subscriptions.recomputeWatches(s.ctx, reqCh) default: // Channel n -> these are the dynamic list of responders that correspond to the stream request typeURL if !ok { @@ -174,7 +163,7 @@ func (s *server) process(str stream.Stream, reqCh chan *discovery.DiscoveryReque return err } - sw.watches.responders[res.GetRequest().TypeUrl].nonce = nonce + sw.subscriptions.responders[res.GetRequest().GetTypeUrl()].nonce = nonce } } } diff --git a/pkg/server/stream/v3/subscription.go b/pkg/server/stream/v3/subscription.go index c5a17b46b8..90d10503d6 100644 --- a/pkg/server/stream/v3/subscription.go +++ b/pkg/server/stream/v3/subscription.go @@ -11,7 +11,12 @@ type SubscriptionState struct { // resourceVersions contains the resources acknowledged by the client and the versions // associated to them + // a resource with a version set to "" is a resource not returned when requested + // which has a functional meaning of "the resource does not exist" resourceVersions map[string]string + + // + isFirstResponse bool } // NewSubscriptionState initializes a stream state. @@ -20,6 +25,7 @@ func NewSubscriptionState(wildcard bool, initialResourceVersions map[string]stri wildcard: wildcard, subscribedResourceNames: map[string]struct{}{}, resourceVersions: initialResourceVersions, + isFirstResponse: true, } if initialResourceVersions == nil { @@ -53,6 +59,7 @@ func (s SubscriptionState) GetKnownResources() map[string]string { // The cache can use this state to compute resources added/updated/deleted func (s *SubscriptionState) SetKnownResources(resourceVersions map[string]string) { s.resourceVersions = resourceVersions + s.isFirstResponse = false } // SetWildcard will set the subscription to return all known resources @@ -65,6 +72,11 @@ func (s SubscriptionState) IsWildcard() bool { return s.wildcard } +// IsWildcard returns whether or not the subscription currently has a wildcard watch +func (s SubscriptionState) IsFirstResponse() bool { + return s.isFirstResponse +} + // WatchesResources returns whether at least one of the resources provided is currently being watched by the subscription. // If the request is wildcard, it will always return true, // otherwise it will compare the provided resources to the list of resources currently subscribed diff --git a/pkg/server/v3/delta_test.go b/pkg/server/v3/delta_test.go index 476a1fe378..ffb7576131 100644 --- a/pkg/server/v3/delta_test.go +++ b/pkg/server/v3/delta_test.go @@ -19,46 +19,52 @@ import ( "github.com/envoyproxy/go-control-plane/pkg/test/resource/v3" ) -func (config *mockConfigWatcher) CreateDeltaWatch(req *discovery.DeltaDiscoveryRequest, state cache.SubscriptionState, out chan cache.DeltaResponse) (func(), error) { - config.deltaCounts[req.TypeUrl] = config.deltaCounts[req.TypeUrl] + 1 +func (config *mockConfigWatcher) CreateDeltaWatch(req cache.DeltaRequest, state cache.SubscriptionState, out chan cache.DeltaResponse) (func(), error) { + config.deltaCounts[req.GetTypeUrl()] = config.deltaCounts[req.GetTypeUrl()] + 1 // This is duplicated from pkg/cache/v3/delta.go as private there - resourceMap := config.deltaResources[req.TypeUrl] + resourceMap := config.deltaResources[req.GetTypeUrl()] versionMap := map[string]string{} for name, resource := range resourceMap { marshaledResource, _ := cache.MarshalResource(resource) versionMap[name] = cache.HashResource(marshaledResource) } - var nextVersionMap map[string]string - var filtered []types.Resource + + // variables to build our response with + var filtered []cache.TestNamedResource var toRemove []string // If we are handling a wildcard request, we want to respond with all resources switch { case state.IsWildcard(): if len(state.GetKnownResources()) == 0 { - filtered = make([]types.Resource, 0, len(resourceMap)) + filtered = make([]cache.TestNamedResource, 0, len(resourceMap)) } - nextVersionMap = make(map[string]string, len(resourceMap)) for name, r := range resourceMap { // Since we've already precomputed the version hashes of the new snapshot, // we can just set it here to be used for comparison later version := versionMap[name] - nextVersionMap[name] = version prevVersion, found := state.GetKnownResources()[name] if !found || (prevVersion != version) { - filtered = append(filtered, r) + filtered = append(filtered, cache.TestNamedResource{ + Name: name, + Version: version, + Resource: r, + }) } } // Compute resources for removal - for name := range state.GetKnownResources() { + for name, version := range state.GetKnownResources() { + if version == "" { + // The resource version can be set to "" to indicate that we already notified the deletion + continue + } if _, ok := resourceMap[name]; !ok { toRemove = append(toRemove, name) } } default: - nextVersionMap = make(map[string]string, len(state.GetSubscribedResources())) // state.GetResourceVersions() may include resources no longer subscribed // In the current code this gets silently cleaned when updating the version map for name := range state.GetSubscribedResources() { @@ -66,23 +72,25 @@ func (config *mockConfigWatcher) CreateDeltaWatch(req *discovery.DeltaDiscoveryR if r, ok := resourceMap[name]; ok { nextVersion := versionMap[name] if prevVersion != nextVersion { - filtered = append(filtered, r) + filtered = append(filtered, cache.TestNamedResource{ + Name: name, + Version: nextVersion, + Resource: r, + }) } - nextVersionMap[name] = nextVersion - } else if found { + } else if found && prevVersion != "" { toRemove = append(toRemove, name) } } } if len(filtered)+len(toRemove) > 0 { - out <- &cache.RawDeltaResponse{ - DeltaRequest: req, - Resources: filtered, - RemovedResources: toRemove, - SystemVersionInfo: "", - NextVersionMap: nextVersionMap, - } + out <- cache.NewTestRawDeltaResponse( + req, + "", + filtered, + toRemove, + ) } else { config.deltaWatches++ return func() { diff --git a/pkg/server/v3/gateway_test.go b/pkg/server/v3/gateway_test.go index 26dba5be8a..176034e433 100644 --- a/pkg/server/v3/gateway_test.go +++ b/pkg/server/v3/gateway_test.go @@ -33,25 +33,25 @@ func TestGateway(t *testing.T) { config := makeMockConfigWatcher() config.responses = map[string][]cache.Response{ resource.ClusterType: { - &cache.RawResponse{ - Version: "2", - Resources: []types.ResourceWithTTL{{Resource: cluster}}, - Request: &discovery.DiscoveryRequest{TypeUrl: resource.ClusterType}, - }, + cache.NewTestRawResponse( + &discovery.DiscoveryRequest{TypeUrl: resource.ClusterType}, + "2", + []types.ResourceWithTTL{{Resource: cluster}}, + ), }, resource.RouteType: { - &cache.RawResponse{ - Version: "3", - Resources: []types.ResourceWithTTL{{Resource: route}}, - Request: &discovery.DiscoveryRequest{TypeUrl: resource.RouteType}, - }, + cache.NewTestRawResponse( + &discovery.DiscoveryRequest{TypeUrl: resource.RouteType}, + "3", + []types.ResourceWithTTL{{Resource: route}}, + ), }, resource.ListenerType: { - &cache.RawResponse{ - Version: "4", - Resources: []types.ResourceWithTTL{{Resource: httpListener}, {Resource: httpScopedListener}}, - Request: &discovery.DiscoveryRequest{TypeUrl: resource.ListenerType}, - }, + cache.NewTestRawResponse( + &discovery.DiscoveryRequest{TypeUrl: resource.ListenerType}, + "4", + []types.ResourceWithTTL{{Resource: httpListener}, {Resource: httpScopedListener}}, + ), }, } gtw := server.HTTPGateway{Server: server.NewServer(context.Background(), config, nil)} diff --git a/pkg/server/v3/server_test.go b/pkg/server/v3/server_test.go index 905a0b5351..a20f1c1792 100644 --- a/pkg/server/v3/server_test.go +++ b/pkg/server/v3/server_test.go @@ -48,11 +48,11 @@ type mockConfigWatcher struct { mu *sync.RWMutex } -func (config *mockConfigWatcher) CreateWatch(req *discovery.DiscoveryRequest, _ cache.SubscriptionState, out chan cache.Response) (func(), error) { - config.counts[req.TypeUrl] = config.counts[req.TypeUrl] + 1 - if len(config.responses[req.TypeUrl]) > 0 { - out <- config.responses[req.TypeUrl][0] - config.responses[req.TypeUrl] = config.responses[req.TypeUrl][1:] +func (config *mockConfigWatcher) CreateWatch(req cache.Request, _ cache.SubscriptionState, out chan cache.Response) (func(), error) { + config.counts[req.GetTypeUrl()] = config.counts[req.GetTypeUrl()] + 1 + if len(config.responses[req.GetTypeUrl()]) > 0 { + out <- config.responses[req.GetTypeUrl()][0] + config.responses[req.GetTypeUrl()] = config.responses[req.GetTypeUrl()][1:] } else { config.watches++ return func() { @@ -62,10 +62,10 @@ func (config *mockConfigWatcher) CreateWatch(req *discovery.DiscoveryRequest, _ return nil, nil } -func (config *mockConfigWatcher) Fetch(_ context.Context, req *discovery.DiscoveryRequest) (cache.Response, error) { - if len(config.responses[req.TypeUrl]) > 0 { - out := config.responses[req.TypeUrl][0] - config.responses[req.TypeUrl] = config.responses[req.TypeUrl][1:] +func (config *mockConfigWatcher) Fetch(_ context.Context, req cache.FetchRequest) (cache.Response, error) { + if len(config.responses[req.GetTypeUrl()]) > 0 { + out := config.responses[req.GetTypeUrl()][0] + config.responses[req.GetTypeUrl()] = config.responses[req.GetTypeUrl()][1:] return out, nil } return nil, errors.New("missing") @@ -177,75 +177,75 @@ var ( func makeResponses() map[string][]cache.Response { return map[string][]cache.Response{ rsrc.EndpointType: { - &cache.RawResponse{ - Version: "1", - Resources: []types.ResourceWithTTL{{Resource: endpoint}}, - Request: &discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType}, - }, + cache.NewTestRawResponse( + &discovery.DiscoveryRequest{TypeUrl: rsrc.EndpointType}, + "1", + []types.ResourceWithTTL{{Resource: endpoint}}, + ), }, rsrc.ClusterType: { - &cache.RawResponse{ - Version: "2", - Resources: []types.ResourceWithTTL{{Resource: cluster}}, - Request: &discovery.DiscoveryRequest{TypeUrl: rsrc.ClusterType}, - }, + cache.NewTestRawResponse( + &discovery.DiscoveryRequest{TypeUrl: rsrc.ClusterType}, + "2", + []types.ResourceWithTTL{{Resource: cluster}}, + ), }, rsrc.RouteType: { - &cache.RawResponse{ - Version: "3", - Resources: []types.ResourceWithTTL{{Resource: route}}, - Request: &discovery.DiscoveryRequest{TypeUrl: rsrc.RouteType}, - }, + cache.NewTestRawResponse( + &discovery.DiscoveryRequest{TypeUrl: rsrc.RouteType}, + "3", + []types.ResourceWithTTL{{Resource: route}}, + ), }, rsrc.ScopedRouteType: { - &cache.RawResponse{ - Version: "4", - Resources: []types.ResourceWithTTL{{Resource: scopedRoute}}, - Request: &discovery.DiscoveryRequest{TypeUrl: rsrc.ScopedRouteType}, - }, + cache.NewTestRawResponse( + &discovery.DiscoveryRequest{TypeUrl: rsrc.ScopedRouteType}, + "4", + []types.ResourceWithTTL{{Resource: scopedRoute}}, + ), }, rsrc.VirtualHostType: { - &cache.RawResponse{ - Version: "5", - Resources: []types.ResourceWithTTL{{Resource: virtualHost}}, - Request: &discovery.DiscoveryRequest{TypeUrl: rsrc.VirtualHostType}, - }, + cache.NewTestRawResponse( + &discovery.DiscoveryRequest{TypeUrl: rsrc.VirtualHostType}, + "5", + []types.ResourceWithTTL{{Resource: virtualHost}}, + ), }, rsrc.ListenerType: { - &cache.RawResponse{ - Version: "6", - Resources: []types.ResourceWithTTL{{Resource: httpListener}, {Resource: httpScopedListener}}, - Request: &discovery.DiscoveryRequest{TypeUrl: rsrc.ListenerType}, - }, + cache.NewTestRawResponse( + &discovery.DiscoveryRequest{TypeUrl: rsrc.ListenerType}, + "6", + []types.ResourceWithTTL{{Resource: httpListener}, {Resource: httpScopedListener}}, + ), }, rsrc.SecretType: { - &cache.RawResponse{ - Version: "7", - Resources: []types.ResourceWithTTL{{Resource: secret}}, - Request: &discovery.DiscoveryRequest{TypeUrl: rsrc.SecretType}, - }, + cache.NewTestRawResponse( + &discovery.DiscoveryRequest{TypeUrl: rsrc.SecretType}, + "7", + []types.ResourceWithTTL{{Resource: secret}}, + ), }, rsrc.RuntimeType: { - &cache.RawResponse{ - Version: "8", - Resources: []types.ResourceWithTTL{{Resource: runtime}}, - Request: &discovery.DiscoveryRequest{TypeUrl: rsrc.RuntimeType}, - }, + cache.NewTestRawResponse( + &discovery.DiscoveryRequest{TypeUrl: rsrc.RuntimeType}, + "8", + []types.ResourceWithTTL{{Resource: runtime}}, + ), }, rsrc.ExtensionConfigType: { - &cache.RawResponse{ - Version: "9", - Resources: []types.ResourceWithTTL{{Resource: extensionConfig}}, - Request: &discovery.DiscoveryRequest{TypeUrl: rsrc.ExtensionConfigType}, - }, + cache.NewTestRawResponse( + &discovery.DiscoveryRequest{TypeUrl: rsrc.ExtensionConfigType}, + "9", + []types.ResourceWithTTL{{Resource: extensionConfig}}, + ), }, // Pass-through type (xDS does not exist for this type) opaqueType: { - &cache.RawResponse{ - Version: "10", - Resources: []types.ResourceWithTTL{{Resource: opaque}}, - Request: &discovery.DiscoveryRequest{TypeUrl: opaqueType}, - }, + cache.NewTestRawResponse( + &discovery.DiscoveryRequest{TypeUrl: opaqueType}, + "10", + []types.ResourceWithTTL{{Resource: opaque}}, + ), }, } } diff --git a/pkg/test/main/main.go b/pkg/test/main/main.go index 3072e16578..ae9d3258a4 100644 --- a/pkg/test/main/main.go +++ b/pkg/test/main/main.go @@ -185,8 +185,8 @@ func main() { eds := cache.NewLinearCache(typeURL) if mux { configCache = &cache.MuxCache{ - Classify: func(req *cache.Request) string { - if req.TypeUrl == typeURL { + Classify: func(req cache.Request) string { + if req.GetTypeUrl() == typeURL { return "eds" } return "default"