Provide driver AddConstraint and RemoveConstraint (#170) (#182)

* provide driver AddConstraint and RemoveConstraint (#170) Signed-off-by: Becky Huang <beckyhd@google.com> * unit tests for Driver.AddConstraint (#170) Signed-off-by: Becky Huang <beckyhd@google.com> * No error returns for removing a non-matching constraint (#170) Signed-off-by: Becky Huang <beckyhd@google.com> * provide driver AddConstraint and RemoveConstraint (#170) Signed-off-by: Becky Huang <beckyhd@google.com> * provide driver AddConstraint and RemoveConstraint (#170) Signed-off-by: Becky Huang <beckyhd@google.com> * unit tests for Driver.AddConstraint (#170) Signed-off-by: Becky Huang <beckyhd@google.com> * No error returns for removing a non-matching constraint (#170) Signed-off-by: Becky Huang <beckyhd@google.com> * Protect d.templateHandler read/write/delete with mutex, fix test(#170) Signed-off-by: Becky Huang <beckyhd@google.com> * gvk group check for constraint Signed-off-by: Becky Huang <beckyhd@google.com> * provide driver AddConstraint and RemoveConstraint (#170) Signed-off-by: Becky Huang <beckyhd@google.com> * provide driver AddConstraint and RemoveConstraint (#170) Signed-off-by: Becky Huang <beckyhd@google.com> * unit tests for Driver.AddConstraint (#170) Signed-off-by: Becky Huang <beckyhd@google.com> * No error returns for removing a non-matching constraint (#170) Signed-off-by: Becky Huang <beckyhd@google.com> * Protect d.templateHandler read/write/delete with mutex, fix test(#170) Signed-off-by: Becky Huang <beckyhd@google.com> * gvk group check for constraint Signed-off-by: Becky Huang <beckyhd@google.com> Co-authored-by: Will Beason <willbeason@google.com>
open-policy-agent · Feb 4, 2022 · 83f5c4f · 83f5c4f
1 parent 0f61a46
commit 83f5c4f
Show file tree

Hide file tree

Showing 14 changed files with 422 additions and 176 deletions.
diff --git a/constraint/pkg/client/client.go b/constraint/pkg/client/client.go
@@ -37,8 +37,9 @@ type Client struct {
 	targets map[string]handler.TargetHandler
 
 	// mtx guards access to both templates and constraints.
-	mtx         sync.RWMutex
-	templates   map[templateKey]*templateEntry
+	mtx       sync.RWMutex
+	templates map[templateKey]*templateEntry
+	// TODO: https://github.com/open-policy-agent/frameworks/issues/187
 	constraints map[schema.GroupKind]map[string]*unstructured.Unstructured
 
 	AllowedDataFields []string
@@ -414,15 +415,6 @@ func createConstraintGKPath(target string, gk schema.GroupKind) string {
 	return constraintPathMerge(target, createConstraintGKSubPath(gk))
 }
 
-// createConstraintPath returns the storage path for a given constraint: constraints.<target>.cluster.<group>.<kind>.<name>.
-func createConstraintPath(target string, constraint *unstructured.Unstructured) (string, error) {
-	p, err := createConstraintSubPath(constraint)
-	if err != nil {
-		return "", err
-	}
-	return constraintPathMerge(target, p), nil
-}
-
 // constraintPathMerge is a shared function for creating constraint paths to
 // ensure uniformity, it is not meant to be called directly.
 func constraintPathMerge(target, subpath string) string {
@@ -470,7 +462,6 @@ func (c *Client) AddConstraint(ctx context.Context, constraint *unstructured.Uns
 	defer c.mtx.Unlock()
 
 	resp := types.NewResponses()
-	errMap := make(clienterrors.ErrorMap)
 	entry, err := c.getTemplateEntry(constraint, false)
 	if err != nil {
 		return resp, err
@@ -493,27 +484,14 @@ func (c *Client) AddConstraint(ctx context.Context, constraint *unstructured.Uns
 	if err := c.validateConstraint(constraint, false); err != nil {
 		return resp, err
 	}
+	if err := c.backend.driver.AddConstraint(ctx, constraint); err != nil {
+		return resp, err
+	}
 	for _, target := range entry.Targets {
-		relPath, err := createConstraintPath(target, constraint)
-		// If we ever create multi-target constraints we will need to handle this more cleverly.
-		// the short-circuiting question, cleanup, etc.
-		if err != nil {
-			errMap[target] = err
-			continue
-		}
-		if err := c.backend.driver.PutData(ctx, relPath, constraint.Object); err != nil {
-			errMap[target] = err
-			continue
-		}
 		resp.Handled[target] = true
 	}
-
-	if len(errMap) == 0 {
-		c.constraints[constraint.GroupVersionKind().GroupKind()][subPath] = constraint.DeepCopy()
-		return resp, nil
-	}
-
-	return resp, &errMap
+	c.constraints[constraint.GroupVersionKind().GroupKind()][subPath] = constraint.DeepCopy()
+	return resp, nil
 }
 
 // RemoveConstraint removes a constraint from OPA. On error, the responses
@@ -527,7 +505,6 @@ func (c *Client) RemoveConstraint(ctx context.Context, constraint *unstructured.
 
 func (c *Client) removeConstraintNoLock(ctx context.Context, constraint *unstructured.Unstructured) (*types.Responses, error) {
 	resp := types.NewResponses()
-	errMap := make(clienterrors.ErrorMap)
 	entry, err := c.getTemplateEntry(constraint, false)
 	if err != nil {
 		return resp, err
@@ -536,26 +513,14 @@ func (c *Client) removeConstraintNoLock(ctx context.Context, constraint *unstruc
 	if err != nil {
 		return resp, err
 	}
+	if err := c.backend.driver.RemoveConstraint(ctx, constraint); err != nil {
+		return resp, err
+	}
 	for _, target := range entry.Targets {
-		relPath, err := createConstraintPath(target, constraint)
-		// If we ever create multi-target constraints we will need to handle this more cleverly.
-		// the short-circuiting question, cleanup, etc.
-		if err != nil {
-			errMap[target] = err
-			continue
-		}
-		if _, err := c.backend.driver.DeleteData(ctx, relPath); err != nil {
-			errMap[target] = err
-		}
 		resp.Handled[target] = true
 	}
-	if len(errMap) == 0 {
-		// If we ever create multi-target constraints we will need to handle this more cleverly.
-		// the short-circuiting question, cleanup, etc.
-		delete(c.constraints[constraint.GroupVersionKind().GroupKind()], subPath)
-		return resp, nil
-	}
-	return resp, &errMap
+	delete(c.constraints[constraint.GroupVersionKind().GroupKind()], subPath)
+	return resp, nil
 }
 
 // getConstraintNoLock gets the currently recognized constraint without the lock.

diff --git a/constraint/pkg/client/client_addconstraint_bench_test.go b/constraint/pkg/client/client_addconstraint_bench_test.go
@@ -5,6 +5,8 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/open-policy-agent/frameworks/constraint/pkg/client/clienttest/cts"
+
 	"github.com/open-policy-agent/frameworks/constraint/pkg/client/clienttest"
 )
 
@@ -22,7 +24,7 @@ func BenchmarkClient_AddConstraint(b *testing.B) {
 
 	for i := 0; i < b.N; i++ {
 		name := fmt.Sprintf("foo-%d", i)
-		constraint := clienttest.MakeConstraint(b, clienttest.KindCheckData, name, clienttest.WantData("bar"))
+		constraint := cts.MakeConstraint(b, clienttest.KindCheckData, name, cts.WantData("bar"))
 
 		_, err = c.AddConstraint(ctx, constraint)
 		if err != nil {

diff --git a/constraint/pkg/client/client_audit_benchmark_test.go b/constraint/pkg/client/client_audit_benchmark_test.go
@@ -6,6 +6,8 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/open-policy-agent/frameworks/constraint/pkg/client/clienttest/cts"
+
 	"github.com/open-policy-agent/frameworks/constraint/pkg/client/clienttest"
 	"github.com/open-policy-agent/frameworks/constraint/pkg/handler/handlertest"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -19,7 +21,7 @@ func BenchmarkClient_Audit(b *testing.B) {
 	}{{
 		name: "success",
 		makeConstraint: func(tid int, name string) *unstructured.Unstructured {
-			return clienttest.MakeConstraint(b, makeKind(tid), name, clienttest.WantData("bar"))
+			return cts.MakeConstraint(b, makeKind(tid), name, cts.WantData("bar"))
 		},
 		makeObject: func(oid int) *handlertest.Object {
 			name := fmt.Sprintf("has-foo-%d", oid)
@@ -31,7 +33,7 @@ func BenchmarkClient_Audit(b *testing.B) {
 	}, {
 		name: "fail",
 		makeConstraint: func(tid int, name string) *unstructured.Unstructured {
-			return clienttest.MakeConstraint(b, makeKind(tid), name, clienttest.WantData("bar"))
+			return cts.MakeConstraint(b, makeKind(tid), name, cts.WantData("bar"))
 		},
 		makeObject: func(oid int) *handlertest.Object {
 			name := fmt.Sprintf("has-foo-%d", oid)
@@ -43,9 +45,9 @@ func BenchmarkClient_Audit(b *testing.B) {
 	}, {
 		name: "filtered out",
 		makeConstraint: func(tid int, name string) *unstructured.Unstructured {
-			return clienttest.MakeConstraint(b, makeKind(tid), name,
-				clienttest.WantData("bar"),
-				clienttest.MatchNamespace("zab"))
+			return cts.MakeConstraint(b, makeKind(tid), name,
+				cts.WantData("bar"),
+				cts.MatchNamespace("zab"))
 		},
 		makeObject: func(oid int) *handlertest.Object {
 			name := fmt.Sprintf("has-foo-%d", oid)

diff --git a/constraint/pkg/client/client_review_benchmark_test.go b/constraint/pkg/client/client_review_benchmark_test.go
@@ -6,6 +6,8 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/open-policy-agent/frameworks/constraint/pkg/client/clienttest/cts"
+
 	"github.com/open-policy-agent/frameworks/constraint/pkg/client/clienttest"
 	"github.com/open-policy-agent/frameworks/constraint/pkg/handler/handlertest"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -25,7 +27,7 @@ func BenchmarkClient_Review(b *testing.B) {
 			},
 		},
 		makeConstraint: func(tid int, name string) *unstructured.Unstructured {
-			return clienttest.MakeConstraint(b, makeKind(tid), name, clienttest.WantData("bar"))
+			return cts.MakeConstraint(b, makeKind(tid), name, cts.WantData("bar"))
 		},
 	}, {
 		name: "fail",
@@ -36,7 +38,7 @@ func BenchmarkClient_Review(b *testing.B) {
 			},
 		},
 		makeConstraint: func(tid int, name string) *unstructured.Unstructured {
-			return clienttest.MakeConstraint(b, makeKind(tid), name, clienttest.WantData("bar"))
+			return cts.MakeConstraint(b, makeKind(tid), name, cts.WantData("bar"))
 		},
 	}, {
 		name: "filtered out",
@@ -48,9 +50,9 @@ func BenchmarkClient_Review(b *testing.B) {
 			},
 		},
 		makeConstraint: func(tid int, name string) *unstructured.Unstructured {
-			return clienttest.MakeConstraint(b, makeKind(tid), name,
-				clienttest.WantData("bar"),
-				clienttest.MatchNamespace("zab"))
+			return cts.MakeConstraint(b, makeKind(tid), name,
+				cts.WantData("bar"),
+				cts.MatchNamespace("zab"))
 		},
 	}, {
 		name: "autoreject",
@@ -62,9 +64,9 @@ func BenchmarkClient_Review(b *testing.B) {
 			Autoreject: true,
 		},
 		makeConstraint: func(tid int, name string) *unstructured.Unstructured {
-			return clienttest.MakeConstraint(b, makeKind(tid), name,
-				clienttest.WantData("bar"),
-				clienttest.EnableAutoreject)
+			return cts.MakeConstraint(b, makeKind(tid), name,
+				cts.WantData("bar"),
+				cts.EnableAutoreject)
 		},
 	}}
 

diff --git a/constraint/pkg/client/client_test.go b/constraint/pkg/client/client_test.go
@@ -695,12 +695,12 @@ func TestClient_RemoveTemplate_CascadingDelete(t *testing.T) {
 		t.Errorf("err = %v; want nil", err)
 	}
 
-	cst1 := clienttest.MakeConstraint(t, "CascadingDelete", "cascadingdelete")
+	cst1 := cts.MakeConstraint(t, "CascadingDelete", "cascadingdelete")
 	if _, err = c.AddConstraint(context.Background(), cst1); err != nil {
 		t.Fatalf("could not add first constraint: %v", err)
 	}
 
-	cst2 := clienttest.MakeConstraint(t, "CascadingDelete", "cascadingdelete2")
+	cst2 := cts.MakeConstraint(t, "CascadingDelete", "cascadingdelete2")
 	if _, err = c.AddConstraint(context.Background(), cst2); err != nil {
 		t.Fatalf("could not add second constraint: %v", err)
 	}
@@ -710,12 +710,12 @@ func TestClient_RemoveTemplate_CascadingDelete(t *testing.T) {
 		t.Errorf("err = %v; want nil", err)
 	}
 
-	cst3 := clienttest.MakeConstraint(t, "StillPersists", "stillpersists")
+	cst3 := cts.MakeConstraint(t, "StillPersists", "stillpersists")
 	if _, err = c.AddConstraint(context.Background(), cst3); err != nil {
 		t.Fatalf("could not add third constraint: %v", err)
 	}
 
-	cst4 := clienttest.MakeConstraint(t, "StillPersists", "stillpersists2")
+	cst4 := cts.MakeConstraint(t, "StillPersists", "stillpersists2")
 	if _, err = c.AddConstraint(context.Background(), cst4); err != nil {
 		t.Fatalf("could not add fourth constraint: %v", err)
 	}
@@ -796,31 +796,31 @@ func TestClient_AddConstraint(t *testing.T) {
 		{
 			name:                   "Good Constraint",
 			template:               cts.New(cts.OptName("foos"), cts.OptCRDNames("Foos")),
-			constraint:             clienttest.MakeConstraint(t, "Foos", "foo"),
+			constraint:             cts.MakeConstraint(t, "Foos", "foo"),
 			wantHandled:            map[string]bool{handlertest.HandlerName: true},
 			wantAddConstraintError: nil,
 			wantGetConstraintError: nil,
 		},
 		{
 			name:                   "No Name",
 			template:               cts.New(cts.OptName("foos"), cts.OptCRDNames("Foos")),
-			constraint:             clienttest.MakeConstraint(t, "Foos", ""),
+			constraint:             cts.MakeConstraint(t, "Foos", ""),
 			wantHandled:            nil,
 			wantAddConstraintError: crds.ErrInvalidConstraint,
 			wantGetConstraintError: crds.ErrInvalidConstraint,
 		},
 		{
 			name:                   "No Kind",
 			template:               cts.New(cts.OptName("foos"), cts.OptCRDNames("Foos")),
-			constraint:             clienttest.MakeConstraint(t, "", "foo"),
+			constraint:             cts.MakeConstraint(t, "", "foo"),
 			wantHandled:            nil,
 			wantAddConstraintError: crds.ErrInvalidConstraint,
 			wantGetConstraintError: crds.ErrInvalidConstraint,
 		},
 		{
 			name:                   "No Template",
 			template:               nil,
-			constraint:             clienttest.MakeConstraint(t, "Foo", "foo"),
+			constraint:             cts.MakeConstraint(t, "Foo", "foo"),
 			wantHandled:            nil,
 			wantAddConstraintError: client.ErrMissingConstraintTemplate,
 			wantGetConstraintError: client.ErrMissingConstraint,
@@ -920,37 +920,37 @@ func TestClient_RemoveConstraint(t *testing.T) {
 		{
 			name:        "Good Constraint",
 			template:    cts.New(cts.OptName("foos"), cts.OptCRDNames("Foos")),
-			constraint:  clienttest.MakeConstraint(t, "Foos", "foo"),
-			toRemove:    clienttest.MakeConstraint(t, "Foos", "foo"),
+			constraint:  cts.MakeConstraint(t, "Foos", "foo"),
+			toRemove:    cts.MakeConstraint(t, "Foos", "foo"),
 			wantHandled: map[string]bool{handlertest.HandlerName: true},
 			wantError:   nil,
 		},
 		{
 			name:        "No name",
 			template:    cts.New(cts.OptName("foos"), cts.OptCRDNames("Foos")),
-			constraint:  clienttest.MakeConstraint(t, "Foos", "foo"),
-			toRemove:    clienttest.MakeConstraint(t, "Foos", ""),
+			constraint:  cts.MakeConstraint(t, "Foos", "foo"),
+			toRemove:    cts.MakeConstraint(t, "Foos", ""),
 			wantHandled: nil,
 			wantError:   crds.ErrInvalidConstraint,
 		},
 		{
 			name:        "No Kind",
 			template:    cts.New(cts.OptName("foos"), cts.OptCRDNames("Foos")),
-			constraint:  clienttest.MakeConstraint(t, "Foos", "foo"),
-			toRemove:    clienttest.MakeConstraint(t, "", "foo"),
+			constraint:  cts.MakeConstraint(t, "Foos", "foo"),
+			toRemove:    cts.MakeConstraint(t, "", "foo"),
 			wantHandled: nil,
 			wantError:   crds.ErrInvalidConstraint,
 		},
 		{
 			name:        "No Template",
-			toRemove:    clienttest.MakeConstraint(t, "Foos", "foo"),
+			toRemove:    cts.MakeConstraint(t, "Foos", "foo"),
 			wantHandled: nil,
 			wantError:   client.ErrMissingConstraintTemplate,
 		},
 		{
 			name:        "No Constraint",
 			template:    cts.New(cts.OptName("foos"), cts.OptCRDNames("Foos")),
-			toRemove:    clienttest.MakeConstraint(t, "Foos", "bar"),
+			toRemove:    cts.MakeConstraint(t, "Foos", "bar"),
 			wantHandled: map[string]bool{handlertest.HandlerName: true},
 			wantError:   nil,
 		},

diff --git a/...aint/pkg/client/clienttest/constraints.go → .../pkg/client/clienttest/cts/constraints.go b/...aint/pkg/client/clienttest/constraints.go → .../pkg/client/clienttest/cts/constraints.go
@@ -1,4 +1,4 @@
-package clienttest
+package cts
 
 import (
 	"testing"

diff --git a/constraint/pkg/client/clienttest/cts/opts.go b/constraint/pkg/client/clienttest/cts/opts.go
@@ -5,18 +5,23 @@ import (
 	"github.com/open-policy-agent/frameworks/constraint/pkg/handler/handlertest"
 )
 
-const ModuleDeny = `
+const (
+	ModuleDeny = `
 package foo
 
 violation[{"msg": msg}] {
   true
   msg := "denied"
 }
 `
+	MockTemplateName  string = "fakes"
+	MockTemplate      string = "Fakes"
+	MockTargetHandler string = "foo"
+)
 
 var defaults = []Opt{
-	OptName("fakes"),
-	OptCRDNames("Fakes"),
+	OptName(MockTemplateName),
+	OptCRDNames(MockTemplate),
 	OptTargets(Target(handlertest.HandlerName, ModuleDeny)),
 }
 

diff --git a/constraint/pkg/client/drivers/interface.go b/constraint/pkg/client/drivers/interface.go
@@ -5,6 +5,7 @@ import (
 
 	"github.com/open-policy-agent/frameworks/constraint/pkg/core/templates"
 	"github.com/open-policy-agent/frameworks/constraint/pkg/types"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
 type QueryCfg struct {
@@ -27,6 +28,10 @@ type Driver interface {
 	AddTemplate(ct *templates.ConstraintTemplate) error
 	// RemoveTemplate removes the template source code from OPA
 	RemoveTemplate(ctx context.Context, ct *templates.ConstraintTemplate) error
+	// AddConstraint inserts validated constraint into OPA
+	AddConstraint(ctx context.Context, constraint *unstructured.Unstructured) error
+	// RemoveConstraint removes a constraint from OPA
+	RemoveConstraint(ctx context.Context, constraint *unstructured.Unstructured) error
 	PutData(ctx context.Context, path string, data interface{}) error
 	DeleteData(ctx context.Context, path string) (bool, error)
 	Query(ctx context.Context, path string, input interface{}, opts ...QueryOpt) (*types.Response, error)

diff --git a/constraint/pkg/client/drivers/local/args.go b/constraint/pkg/client/drivers/local/args.go
@@ -21,7 +21,9 @@ func Defaults() Arg {
 		if d.modules == nil {
 			d.modules = make(map[string]*ast.Module)
 		}
-
+		if d.templateHandler == nil {
+			d.templateHandler = make(map[string][]string)
+		}
 		if d.storage == nil {
 			d.storage = inmem.New()
 		}