Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
Signed-off-by: Max Smythe <smythe@google.com>
  • Loading branch information
maxsmythe committed Feb 25, 2023
1 parent 8a87bbc commit 8d38e31
Show file tree
Hide file tree
Showing 6 changed files with 47 additions and 32 deletions.
50 changes: 25 additions & 25 deletions constraint/pkg/client/client_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/open-policy-agent/frameworks/constraint/pkg/client/clienttest/cts"
"github.com/open-policy-agent/frameworks/constraint/pkg/client/drivers/dummy"
"github.com/open-policy-agent/frameworks/constraint/pkg/client/drivers/dummy/schema"
"github.com/open-policy-agent/frameworks/constraint/pkg/client/drivers/fake"
"github.com/open-policy-agent/frameworks/constraint/pkg/client/drivers/fake/schema"
"github.com/open-policy-agent/frameworks/constraint/pkg/core/templates"
"github.com/open-policy-agent/frameworks/constraint/pkg/handler/handlertest"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
Expand Down Expand Up @@ -52,10 +52,10 @@ func TestMultiDriverAddTemplate(t *testing.T) {
"constraint3": constraint3.DeepCopy(),
}

cleanSlate := func() (*dummy.Driver, *dummy.Driver, *dummy.Driver, *Client) {
driverA := dummy.New("driverA")
driverB := dummy.New("driverB")
driverC := dummy.New("driverC")
cleanSlate := func() (*fake.Driver, *fake.Driver, *fake.Driver, *Client) {
driverA := fake.New("driverA")
driverB := fake.New("driverB")
driverC := fake.New("driverC")

client, err := NewClient(
Targets(&handlertest.Handler{Name: pointer.String("h1")}),
Expand Down Expand Up @@ -734,9 +734,9 @@ func TestMultiDriverAddTemplate(t *testing.T) {
})

t.Run("Multi-Driver Template, Reverse Order", func(t *testing.T) {
driverA := dummy.New("driverA")
driverB := dummy.New("driverB")
driverC := dummy.New("driverC")
driverA := fake.New("driverA")
driverB := fake.New("driverB")
driverC := fake.New("driverC")

client, err := NewClient(
Targets(&handlertest.Handler{Name: pointer.String("h1")}),
Expand Down Expand Up @@ -820,10 +820,10 @@ func TestMultiDriverRemoveTemplate(t *testing.T) {
"constraint2": constraint2.DeepCopy(),
}

cleanSlate := func() (*dummy.Driver, *dummy.Driver, *dummy.Driver, *Client) {
driverA := dummy.New("driverA")
driverB := dummy.New("driverB")
driverC := dummy.New("driverC")
cleanSlate := func() (*fake.Driver, *fake.Driver, *fake.Driver, *Client) {
driverA := fake.New("driverA")
driverB := fake.New("driverB")
driverC := fake.New("driverC")

client, err := NewClient(
Targets(&handlertest.Handler{Name: pointer.String("h1")}),
Expand Down Expand Up @@ -908,7 +908,7 @@ func TestDriverForTemplate(t *testing.T) {
name: "One Driver",
options: []Opt{
Targets(&handlertest.Handler{Name: pointer.String("h1")}),
Driver(dummy.New("driverA")),
Driver(fake.New("driverA")),
},
template: cts.New(cts.OptTargets(
cts.TargetCustomEngines(
Expand All @@ -922,7 +922,7 @@ func TestDriverForTemplate(t *testing.T) {
name: "One Driver, Mismatch",
options: []Opt{
Targets(&handlertest.Handler{Name: pointer.String("h1")}),
Driver(dummy.New("driverA")),
Driver(fake.New("driverA")),
},
template: cts.New(cts.OptTargets(
cts.TargetCustomEngines(
Expand All @@ -936,8 +936,8 @@ func TestDriverForTemplate(t *testing.T) {
name: "Multi Driver",
options: []Opt{
Targets(&handlertest.Handler{Name: pointer.String("h1")}),
Driver(dummy.New("driverA")),
Driver(dummy.New("driverB")),
Driver(fake.New("driverA")),
Driver(fake.New("driverB")),
},
template: cts.New(cts.OptTargets(
cts.TargetCustomEngines(
Expand All @@ -951,8 +951,8 @@ func TestDriverForTemplate(t *testing.T) {
name: "Multi Driver, Second",
options: []Opt{
Targets(&handlertest.Handler{Name: pointer.String("h1")}),
Driver(dummy.New("driverB")),
Driver(dummy.New("driverA")),
Driver(fake.New("driverB")),
Driver(fake.New("driverA")),
},
template: cts.New(cts.OptTargets(
cts.TargetCustomEngines(
Expand All @@ -966,7 +966,7 @@ func TestDriverForTemplate(t *testing.T) {
name: "One Driver, Multi-Template",
options: []Opt{
Targets(&handlertest.Handler{Name: pointer.String("h1")}),
Driver(dummy.New("driverA")),
Driver(fake.New("driverA")),
},
template: cts.New(cts.OptTargets(
cts.TargetCustomEngines(
Expand All @@ -981,7 +981,7 @@ func TestDriverForTemplate(t *testing.T) {
name: "One Driver, Multi-Template Second",
options: []Opt{
Targets(&handlertest.Handler{Name: pointer.String("h1")}),
Driver(dummy.New("driverB")),
Driver(fake.New("driverB")),
},
template: cts.New(cts.OptTargets(
cts.TargetCustomEngines(
Expand All @@ -996,8 +996,8 @@ func TestDriverForTemplate(t *testing.T) {
name: "Two Driver, Multi-Template",
options: []Opt{
Targets(&handlertest.Handler{Name: pointer.String("h1")}),
Driver(dummy.New("driverA")),
Driver(dummy.New("driverB")),
Driver(fake.New("driverA")),
Driver(fake.New("driverB")),
},
template: cts.New(cts.OptTargets(
cts.TargetCustomEngines(
Expand All @@ -1012,8 +1012,8 @@ func TestDriverForTemplate(t *testing.T) {
name: "Two Driver, Multi-Template, Second",
options: []Opt{
Targets(&handlertest.Handler{Name: pointer.String("h1")}),
Driver(dummy.New("driverB")),
Driver(dummy.New("driverA")),
Driver(fake.New("driverB")),
Driver(fake.New("driverA")),
},
template: cts.New(cts.OptTargets(
cts.TargetCustomEngines(
Expand Down
3 changes: 3 additions & 0 deletions constraint/pkg/client/client_opts.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ func Driver(d drivers.Driver) Opt {
if d.Name() == "" {
return ErrNoDriverName
}
if _, ok := client.drivers[d.Name()]; ok {
return fmt.Errorf("%w: %s", ErrDuplicateDriver, d.Name())
}
client.drivers[d.Name()] = d
client.driverPriority[d.Name()] = len(client.drivers)
return nil
Expand Down
15 changes: 13 additions & 2 deletions constraint/pkg/client/client_opts_test.go
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
package client

import (
"errors"
"reflect"
"testing"

"github.com/open-policy-agent/frameworks/constraint/pkg/client/drivers/dummy"
"github.com/open-policy-agent/frameworks/constraint/pkg/client/drivers/fake"
"github.com/open-policy-agent/frameworks/constraint/pkg/handler/handlertest"
"k8s.io/utils/pointer"
)

func TestAddingDrivers(t *testing.T) {
c, err := NewClient(Targets(&handlertest.Handler{Name: pointer.String("foo")}), Driver(dummy.New("driver1")), Driver(dummy.New("driver2")))
c, err := NewClient(Targets(&handlertest.Handler{Name: pointer.String("foo")}), Driver(fake.New("driver1")), Driver(fake.New("driver2")))
if err != nil {
t.Fatal(err)
}
Expand All @@ -24,3 +25,13 @@ func TestAddingDrivers(t *testing.T) {
t.Errorf("driver2 missing from driverset")
}
}

func TestNoDuplicates(t *testing.T) {
_, err := NewClient(Targets(&handlertest.Handler{Name: pointer.String("foo")}), Driver(fake.New("driver1")), Driver(fake.New("driver1")))
if err == nil {
t.Fatal("expected error, got none")
}
if !errors.Is(err, ErrDuplicateDriver) {
t.Errorf("wanted duplicate driver error, got %v", err)
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package dummy
package fake

import (
"context"
Expand All @@ -9,7 +9,7 @@ import (

apiconstraints "github.com/open-policy-agent/frameworks/constraint/pkg/apis/constraints"
"github.com/open-policy-agent/frameworks/constraint/pkg/client/drivers"
"github.com/open-policy-agent/frameworks/constraint/pkg/client/drivers/dummy/schema"
"github.com/open-policy-agent/frameworks/constraint/pkg/client/drivers/fake/schema"
"github.com/open-policy-agent/frameworks/constraint/pkg/core/templates"
"github.com/open-policy-agent/frameworks/constraint/pkg/types"
"github.com/open-policy-agent/opa/storage"
Expand Down Expand Up @@ -110,21 +110,21 @@ func (d *Driver) AddTemplate(ctx context.Context, ct *templates.ConstraintTempla
if len(ct.Spec.Targets) != 1 {
return errors.New("wrong number of targets defined, only 1 target allowed")
}
var dummyCode templates.Code
var fakeCode templates.Code
found := false
for _, code := range ct.Spec.Targets[0].Code {
if code.Engine != d.name {
continue
}
dummyCode = code
fakeCode = code
found = true
break
}
if !found {
return errors.New("SimplePolicy code not defined")
}

source, err := schema.GetSource(dummyCode)
source, err := schema.GetSource(fakeCode)
if err != nil {
return err
}
Expand Down
1 change: 1 addition & 0 deletions constraint/pkg/client/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
var (
ErrCreatingBackend = errors.New("unable to create backend")
ErrNoDriverName = errors.New("driver has no name")
ErrDuplicateDriver = errors.New("duplicate drivers of the same name")
ErrCreatingClient = errors.New("unable to create client")
ErrMissingConstraint = errors.New("missing Constraint")
ErrMissingConstraintTemplate = errors.New("missing ConstraintTemplate")
Expand Down

0 comments on commit 8d38e31

Please sign in to comment.