Implement Target cache hook (#190)

* Implement Target cache hook

Targets sometimes need to maintain state about the system. For example,
the Gatekeeper target needs to track the set of current Namespaces on
the cluster in order to properly match objects to Constraints when Audit
is called.

This commit adds a Cache interface which Targets may choose to
implement. If they implement this interface, Client attempts to add and
remove objects from the Target cache just as it does for Driver caches.

These operations are not atomic, so it is possible for systems to get
into an inconsistent state. There isn't a good solution to this now -
I've opened #189 to solve this in the future. The implications are quite
complex and there's a lot of edge cases.

This commit also modifies the test target handler matchers - they now
access the test target's cache in order to function. These matchers
aren't called yet - we don't want to break Gatekeeper since Gatekeeper
Golang matchers are not yet implemented.

Signed-off-by: Will Beason <[email protected]>

* Fix merge conflicts

Signed-off-by: Will Beason <[email protected]>

* Remove ability for caches to fail deleting

Otherwise it is easy to get into inconsistent cache states. There's lots
of edge cases that can cause unpredictable behaviors that we don't want
to allow.

Signed-off-by: Will Beason <[email protected]>

* Make addition atomic

Since adding data can fail in the target cache, remove data from the
driver cache.

Note that addition/deletion occur in opposite orders for AddData and
RemoveData - this is because we want to prioritize reversible over
potentially-irreversible operations. Removing data from the handler
cache can't fail, so it is safe to add it first.

Signed-off-by: Will Beason <[email protected]>

* Make it impossible for handler caches to fail deletion

Otherwise we can easily end up in very annoying inconsistent states. If
deleteion really, really needs to fail then the application should panic
rather than allow things to get in an inconsistent state.

Per discussion

Signed-off-by: Will Beason <[email protected]>

constraint/pkg/client/client.go

@@ -58,6 +58,8 @@ func createDataPath(target, subpath string) string {
 // On error, the responses return value will still be populated so that
 // partial results can be analyzed.
 func (c *Client) AddData(ctx context.Context, data interface{}) (*types.Responses, error) {
+	// TODO(#189): Make AddData atomic across all Drivers/Targets.
+
 	resp := types.NewResponses()
 	errMap := make(clienterrors.ErrorMap)
 	for target, h := range c.targets {
@@ -69,12 +71,40 @@ func (c *Client) AddData(ctx context.Context, data interface{}) (*types.Response
 		if !handled {
 			continue
 		}
-		if err := c.backend.driver.PutData(ctx, createDataPath(target, relPath), processedData); err != nil {
+
+		var cache handler.Cache
+		if cacher, ok := h.(handler.Cacher); ok {
+			cache = cacher.GetCache()
+		}
+
+		// Add to the target cache first because cache.Remove cannot fail. Thus, we
+		// can prevent the system from getting into an inconsistent state.
+		if cache != nil {
+			err = cache.Add(relPath, processedData)
+			if err != nil {
+				// Use a different key than the driver to avoid clobbering errors.
+				errMap[target] = err
+
+				continue
+			}
+		}
+
+		// paths passed to driver must be specific to the target to prevent key
+		// collisions.
+		driverPath := createDataPath(target, relPath)
+		err = c.backend.driver.PutData(ctx, driverPath, processedData)
+		if err != nil {
 			errMap[target] = err
+
+			if cache != nil {
+				cache.Remove(relPath)
+			}
 			continue
 		}
+
 		resp.Handled[target] = true
 	}
+
 	if len(errMap) == 0 {
 		return resp, nil
 	}
@@ -96,15 +126,24 @@ func (c *Client) RemoveData(ctx context.Context, data interface{}) (*types.Respo
 		if !handled {
 			continue
 		}
+
 		if _, err := c.backend.driver.DeleteData(ctx, createDataPath(target, relPath)); err != nil {
 			errMap[target] = err
 			continue
 		}
 		resp.Handled[target] = true
+
+		if cacher, ok := h.(handler.Cacher); ok {
+			cache := cacher.GetCache()
+
+			cache.Remove(relPath)
+		}
 	}
+
 	if len(errMap) == 0 {
 		return resp, nil
 	}
+
 	return resp, &errMap
 }
 

constraint/pkg/client/client_test.go

@@ -1620,3 +1620,178 @@ func TestClient_AddTemplate_Duplicate(t *testing.T) {
 		t.Fatal(diff)
 	}
 }
+
+func TestClient_AddData_Cache(t *testing.T) {
+	tests := []struct {
+		name    string
+		before  map[string]*handlertest.Object
+		add     interface{}
+		want    map[interface{}]interface{}
+		wantErr error
+	}{
+		{
+			name:   "add invalid type",
+			before: nil,
+			add:    "foo",
+			want:   nil,
+			wantErr: &clienterrors.ErrorMap{
+				handlertest.HandlerName: handlertest.ErrInvalidType,
+			},
+		},
+		{
+			name:   "add invalid Object",
+			before: nil,
+			add: &handlertest.Object{
+				Namespace: "",
+				Name:      "",
+			},
+			want: nil,
+			wantErr: &clienterrors.ErrorMap{
+				handlertest.HandlerName: handlertest.ErrInvalidObject,
+			},
+		},
+		{
+			name:   "add Object",
+			before: nil,
+			add: &handlertest.Object{
+				Namespace: "foo",
+				Name:      "bar",
+			},
+			want:    nil,
+			wantErr: nil,
+		},
+		{
+			name:   "add Namespace",
+			before: nil,
+			add: &handlertest.Object{
+				Namespace: "foo",
+			},
+			want: map[interface{}]interface{}{
+				"namespace/foo/": &handlertest.Object{
+					Namespace: "foo",
+				},
+			},
+			wantErr: nil,
+		},
+		{
+			name: "replace Namespace",
+			before: map[string]*handlertest.Object{
+				"namespace/foo/": {
+					Namespace: "foo",
+					Data:      "qux",
+				},
+			},
+			add: &handlertest.Object{
+				Namespace: "foo",
+				Data:      "bar",
+			},
+			want: map[interface{}]interface{}{
+				"namespace/foo/": &handlertest.Object{
+					Namespace: "foo",
+					Data:      "bar",
+				},
+			},
+			wantErr: nil,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cache := &handlertest.Cache{}
+			h := &handlertest.Handler{Cache: cache}
+
+			c := clienttest.New(t, client.Targets(h))
+
+			ctx := context.Background()
+			for _, v := range tt.before {
+				_, err := c.AddData(ctx, v)
+				if err != nil {
+					t.Fatal(err)
+				}
+			}
+
+			_, err := c.AddData(ctx, tt.add)
+			if !errors.Is(err, tt.wantErr) {
+				t.Fatalf("got error: %#v,\nwant %#v", err, tt.wantErr)
+			}
+
+			got := make(map[interface{}]interface{})
+			cache.Namespaces.Range(func(key, value interface{}) bool {
+				got[key] = value
+				return true
+			})
+
+			if diff := cmp.Diff(tt.want, got, cmpopts.EquateEmpty()); diff != "" {
+				t.Error(diff)
+			}
+		})
+	}
+}
+
+func TestClient_RemoveData_Cache(t *testing.T) {
+	tests := []struct {
+		name    string
+		before  map[string]*handlertest.Object
+		remove  interface{}
+		want    map[interface{}]interface{}
+		wantErr error
+	}{
+		{
+			name:   "remove invalid",
+			before: nil,
+			remove: "foo",
+			want:   nil,
+			wantErr: &clienterrors.ErrorMap{
+				handlertest.HandlerName: handlertest.ErrInvalidType,
+			},
+		},
+		{
+			name:    "remove nonexistent",
+			before:  nil,
+			remove:  &handlertest.Object{Namespace: "foo"},
+			want:    nil,
+			wantErr: nil,
+		},
+		{
+			name: "remove Namespace",
+			before: map[string]*handlertest.Object{
+				"/namespace/foo": {Namespace: "foo"},
+			},
+			remove:  &handlertest.Object{Namespace: "foo"},
+			want:    nil,
+			wantErr: nil,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cache := &handlertest.Cache{}
+			h := &handlertest.Handler{Cache: cache}
+
+			c := clienttest.New(t, client.Targets(h))
+
+			ctx := context.Background()
+			for _, v := range tt.before {
+				_, err := c.AddData(ctx, v)
+				if err != nil {
+					t.Fatal(err)
+				}
+			}
+
+			_, err := c.RemoveData(ctx, tt.remove)
+			if !errors.Is(err, tt.wantErr) {
+				t.Fatalf("got error: %v,\nwant %v", err, tt.wantErr)
+			}
+
+			got := make(map[interface{}]interface{})
+			cache.Namespaces.Range(func(key, value interface{}) bool {
+				got[key] = value
+				return true
+			})
+
+			if diff := cmp.Diff(tt.want, got, cmpopts.EquateEmpty()); diff != "" {
+				t.Error(diff)
+			}
+		})
+	}
+}

constraint/pkg/handler/cache.go

@@ -0,0 +1,44 @@
+package handler
+
+// Cacher is a type - usually a Handler - which needs to cache state.
+// Handlers only need implement this interface if they have need of a cache.
+// Handlers which do not implement Cacher are assumed to be stateless from
+// Client's perspective.
+type Cacher interface {
+	// GetCache returns the Cache. If nil, the Cacher is treated as having no
+	// cache.
+	GetCache() Cache
+}
+
+// Cache is an interface for Handlers to define which allows them to track
+// objects not currently under review. For example, this is required to make
+// referential constraints work, or to have Constraint match criteria which
+// relies on more than just the object under review.
+//
+// Implementations must satisfy the per-method requirements for Client to handle
+// the Cache properly.
+type Cache interface {
+	// Add inserts a new object into Cache with identifier key. If an object
+	// already exists, replaces the object at key.
+	Add(key string, object interface{}) error
+
+	// Remove deletes the object at key from Cache. Deletion succeeds if key
+	// does not exist.
+	// Remove always succeeds; if for some reason key cannot be deleted the application
+	// should panic.
+	Remove(key string)
+}
+
+type NoCache struct{}
+
+func (n NoCache) Add(key string, object interface{}) error {
+	return nil
+}
+
+func (n NoCache) Get(key string) (interface{}, error) {
+	return nil, nil
+}
+
+func (n NoCache) Remove(key string) {}
+
+var _ Cache = NoCache{}

constraint/pkg/handler/handlertest/cache.go

@@ -0,0 +1,43 @@
+package handlertest
+
+import (
+	"errors"
+	"fmt"
+	"sync"
+
+	"github.com/open-policy-agent/frameworks/constraint/pkg/handler"
+)
+
+var ErrInvalidObject = errors.New("invalid object")
+
+// Cache is a threadsafe Cache for the test Handler which keeps track of
+// Namespaces.
+type Cache struct {
+	Namespaces sync.Map
+}
+
+var _ handler.Cache = &Cache{}
+
+// Add inserts object into Cache if object is a Namespace.
+func (c *Cache) Add(key string, object interface{}) error {
+	obj, ok := object.(*Object)
+	if !ok {
+		return fmt.Errorf("%w: got object type %T, want %T", ErrInvalidType, object, &Object{})
+	}
+
+	if obj.Name != "" {
+		return nil
+	}
+
+	if obj.Namespace == "" {
+		return fmt.Errorf("%w: must specify one of Name or Namespace", ErrInvalidObject)
+	}
+
+	c.Namespaces.Store(key, object)
+
+	return nil
+}
+
+func (c *Cache) Remove(key string) {
+	c.Namespaces.Delete(key)
+}

constraint/pkg/handler/handlertest/handler.go

@@ -14,6 +14,8 @@ import (
 
 var _ handler.TargetHandler = &Handler{}
 
+var _ handler.Cacher = &Handler{}
+
 // HandlerName is the default handler name.
 const HandlerName = "test.target"
 
@@ -28,6 +30,8 @@ type Handler struct {
 	// ProcessDataError is the error to return when ProcessData is called.
 	// If nil returns no error.
 	ProcessDataError error
+
+	Cache *Cache
 }
 
 func (h *Handler) GetName() string {
@@ -112,13 +116,10 @@ func (h *Handler) ProcessData(obj interface{}) (bool, string, interface{}, error
 			return false, "", nil, nil
 		}
 
-		if o.Namespace == "" {
-			return true, fmt.Sprintf("cluster/%s", o.Name), obj, nil
-		}
-		return true, fmt.Sprintf("namespace/%s/%s", o.Namespace, o.Name), obj, nil
+		return true, o.Key(), obj, nil
 	default:
-		return false, "", nil, fmt.Errorf("unrecognized type %T, want %T",
-			obj, &Object{})
+		return false, "", nil, fmt.Errorf("%w: got object type %T, want %T",
+			ErrInvalidType, obj, &Object{})
 	}
 }
 
@@ -167,5 +168,13 @@ func (h *Handler) ToMatcher(constraint *unstructured.Unstructured) (constraints.
 		return nil, fmt.Errorf("unable to get spec.matchNamespace: %w", err)
 	}
 
-	return Matcher{namespace: ns}, nil
+	return Matcher{namespace: ns, cache: h.Cache}, nil
+}
+
+func (h *Handler) GetCache() handler.Cache {
+	if h.Cache == nil {
+		return handler.NoCache{}
+	}
+
+	return h.Cache
 }