From 09c80a4ae27cb6f0dd2b01077df5a20155d00d43 Mon Sep 17 00:00:00 2001 From: Becky Huang Date: Tue, 11 Jan 2022 11:58:35 -0800 Subject: [PATCH] Add client.ValidateConstraintTemplate interface to replace validation logic from client.CeateCRD (#171) Signed-off-by: Becky Huang --- constraint/pkg/client/client.go | 68 ++++-- constraint/pkg/client/client_test.go | 244 +++++++++++++++++++ constraint/pkg/client/drivers/local/args.go | 4 +- constraint/pkg/client/drivers/local/local.go | 33 ++- 4 files changed, 310 insertions(+), 39 deletions(-) diff --git a/constraint/pkg/client/client.go b/constraint/pkg/client/client.go index 6931b0c67..4c7fb65f6 100644 --- a/constraint/pkg/client/client.go +++ b/constraint/pkg/client/client.go @@ -197,23 +197,10 @@ func (c *Client) createBasicTemplateArtifacts(templ *templates.ConstraintTemplat if err != nil { return nil, err } - - kind := templ.Spec.CRD.Spec.Names.Kind - if kind == "" { - return nil, fmt.Errorf("%w: ConstraintTemplate %q does not specify CRD Kind", - ErrInvalidConstraintTemplate, templ.GetName()) - } - - if !strings.EqualFold(templ.ObjectMeta.Name, kind) { - return nil, fmt.Errorf("%w: the ConstraintTemplate's name %q is not equal to the lowercase of CRD's Kind: %q", - ErrInvalidConstraintTemplate, templ.ObjectMeta.Name, strings.ToLower(kind)) - } - - targetSpec, targetHandler, err := c.validateTargets(templ) + targetSpec, targetHandler, err := c.ValidateConstraintTemplateBasic(templ) if err != nil { - return nil, fmt.Errorf("failed to validate targets for template %s: %w", templ.Name, err) + return nil, err } - sch := c.backend.crd.createSchema(templ, targetHandler) crd, err := c.backend.crd.createCRD(templ, sch) @@ -251,6 +238,40 @@ func (c *Client) CreateCRD(templ *templates.ConstraintTemplate) (*apiextensions. return artifacts.crd, nil } +func (c *Client) ValidateConstraintTemplateBasic(templ *templates.ConstraintTemplate) (*templates.Target, TargetHandler, error) { + kind := templ.Spec.CRD.Spec.Names.Kind + if kind == "" { + return nil, nil, fmt.Errorf("%w: ConstraintTemplate %q does not specify CRD Kind", + ErrInvalidConstraintTemplate, templ.GetName()) + } + + if !strings.EqualFold(templ.ObjectMeta.Name, kind) { + return nil, nil, fmt.Errorf("%w: the ConstraintTemplate's name %q is not equal to the lowercase of CRD's Kind: %q", + ErrInvalidConstraintTemplate, templ.ObjectMeta.Name, strings.ToLower(kind)) + } + + targetSpec, targetHandler, err := c.validateTargets(templ) + if err != nil { + return nil, nil, fmt.Errorf("failed to validate targets for template %s: %w", templ.Name, err) + } + return targetSpec, targetHandler, nil +} + +func (c *Client) ValidateConstraintTemplate(templ *templates.ConstraintTemplate) error { + if templ == nil { + return fmt.Errorf(`%w: ConstraintTemplate is nil`, + ErrInvalidConstraintTemplate) + } + if _, _, err := c.ValidateConstraintTemplateBasic(templ); err != nil { + return err + } + if dr, ok := c.backend.driver.(*local.Driver); ok { + _, _, err := dr.ValidateConstraintTemplate(templ) + return err + } + return fmt.Errorf("driver %T is not supported", c.backend.driver) +} + // AddTemplate adds the template source code to OPA and registers the CRD with the client for // schema validation on calls to AddConstraint. On error, the responses return value // will still be populated so that partial results can be analyzed. @@ -274,22 +295,17 @@ func (c *Client) AddTemplate(templ *templates.ConstraintTemplate) (*types.Respon if err = c.backend.driver.AddTemplate(templ); err != nil { return resp, err } - - artifacts, err := c.createBasicTemplateArtifacts(templ) - if err != nil { - return resp, err - } cpy := templ.DeepCopy() cpy.Status = templates.ConstraintTemplateStatus{} - c.templates[artifacts.Key()] = &templateEntry{ + c.templates[basicArtifacts.Key()] = &templateEntry{ template: cpy, - CRD: artifacts.crd, - Targets: []string{artifacts.targetHandler.GetName()}, + CRD: basicArtifacts.crd, + Targets: []string{basicArtifacts.targetHandler.GetName()}, } - if _, ok := c.constraints[artifacts.gk]; !ok { - c.constraints[artifacts.gk] = make(map[string]*unstructured.Unstructured) + if _, ok := c.constraints[basicArtifacts.gk]; !ok { + c.constraints[basicArtifacts.gk] = make(map[string]*unstructured.Unstructured) } - resp.Handled[artifacts.targetHandler.GetName()] = true + resp.Handled[basicArtifacts.targetHandler.GetName()] = true return resp, nil } diff --git a/constraint/pkg/client/client_test.go b/constraint/pkg/client/client_test.go index 4f0a3acff..a0b987ed2 100644 --- a/constraint/pkg/client/client_test.go +++ b/constraint/pkg/client/client_test.go @@ -1359,3 +1359,247 @@ violation[msg] {msg := "always"}`, }) } } + +func TestClient_ValidateConstraintTemplate(t *testing.T) { + testCases := []struct { + name string + targets []TargetHandler + template *templates.ConstraintTemplate + want *apiextensions.CustomResourceDefinition + wantErr error + }{ + { + name: "nil", + template: nil, + want: nil, + wantErr: ErrInvalidConstraintTemplate, + }, + { + name: "empty", + template: &templates.ConstraintTemplate{}, + want: nil, + wantErr: ErrInvalidConstraintTemplate, + }, + { + name: "no CRD kind", + template: &templates.ConstraintTemplate{ + ObjectMeta: v1.ObjectMeta{Name: "foo"}, + }, + want: nil, + wantErr: ErrInvalidConstraintTemplate, + }, + { + name: "name-kind mismatch", + template: &templates.ConstraintTemplate{ + ObjectMeta: v1.ObjectMeta{Name: "foo"}, + Spec: templates.ConstraintTemplateSpec{ + CRD: templates.CRD{ + Spec: templates.CRDSpec{ + Names: templates.Names{ + Kind: "Bar", + }, + }, + }, + Targets: []templates.Target{{ + Target: "handler", + Rego: `package foo + +violation[msg] {msg := "always"}`, + }}, + }, + }, + want: nil, + wantErr: ErrInvalidConstraintTemplate, + }, + { + name: "no targets", + template: &templates.ConstraintTemplate{ + ObjectMeta: v1.ObjectMeta{Name: "foo"}, + Spec: templates.ConstraintTemplateSpec{ + CRD: templates.CRD{ + Spec: templates.CRDSpec{ + Names: templates.Names{ + Kind: "Foo", + }, + }, + }, + }, + }, + want: nil, + wantErr: ErrInvalidConstraintTemplate, + }, + { + name: "wrong target", + template: &templates.ConstraintTemplate{ + ObjectMeta: v1.ObjectMeta{Name: "foo"}, + Spec: templates.ConstraintTemplateSpec{ + CRD: templates.CRD{ + Spec: templates.CRDSpec{ + Names: templates.Names{ + Kind: "Foo", + }, + }, + }, + Targets: []templates.Target{{ + Target: "handler.2", + }}, + }, + }, + want: nil, + wantErr: ErrInvalidConstraintTemplate, + }, + { + name: "multiple targets", + template: &templates.ConstraintTemplate{ + ObjectMeta: v1.ObjectMeta{Name: "foo"}, + Spec: templates.ConstraintTemplateSpec{ + CRD: templates.CRD{ + Spec: templates.CRDSpec{ + Names: templates.Names{ + Kind: "Foo", + }, + }, + }, + Targets: []templates.Target{{ + Target: "handler", + Rego: `package foo + +violation[msg] {msg := "always"}`, + }, { + Target: "handler.2", + Rego: `package foo + +violation[msg] {msg := "always"}`, + }}, + }, + }, + want: nil, + wantErr: ErrInvalidConstraintTemplate, + }, + { + name: "no rego", + targets: []TargetHandler{&badHandler{Name: "handler", HasLib: true}}, + template: &templates.ConstraintTemplate{ + ObjectMeta: v1.ObjectMeta{Name: "foo"}, + Spec: templates.ConstraintTemplateSpec{ + CRD: templates.CRD{ + Spec: templates.CRDSpec{ + Names: templates.Names{ + Kind: "Foo", + }, + }, + }, + Targets: []templates.Target{{ + Target: "handler", + }}, + }, + }, + want: nil, + wantErr: local.ErrInvalidConstraintTemplate, + }, + { + name: "empty rego package", + targets: []TargetHandler{&badHandler{Name: "handler", HasLib: true}}, + template: &templates.ConstraintTemplate{ + ObjectMeta: v1.ObjectMeta{Name: "foo"}, + Spec: templates.ConstraintTemplateSpec{ + CRD: templates.CRD{ + Spec: templates.CRDSpec{ + Names: templates.Names{ + Kind: "Foo", + }, + }, + }, + Targets: []templates.Target{{ + Target: "handler", + Rego: `package foo`, + }}, + }, + }, + want: nil, + wantErr: local.ErrInvalidConstraintTemplate, + }, + { + name: "minimal working", + targets: []TargetHandler{&badHandler{Name: "handler", HasLib: true}}, + template: &templates.ConstraintTemplate{ + ObjectMeta: v1.ObjectMeta{Name: "foo"}, + Spec: templates.ConstraintTemplateSpec{ + CRD: templates.CRD{ + Spec: templates.CRDSpec{ + Names: templates.Names{ + Kind: "Foo", + }, + }, + }, + Targets: []templates.Target{{ + Target: "handler", + Rego: `package foo + +violation[msg] {msg := "always"}`, + }}, + }, + }, + want: &apiextensions.CustomResourceDefinition{ + ObjectMeta: v1.ObjectMeta{ + Name: "foo.constraints.gatekeeper.sh", + Labels: map[string]string{"gatekeeper.sh/constraint": "yes"}, + }, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "constraints.gatekeeper.sh", + Version: "v1beta1", + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "foo", + Singular: "foo", + Kind: "Foo", + ListKind: "FooList", + Categories: []string{"constraint", "constraints"}, + }, + Scope: apiextensions.ClusterScoped, + Subresources: &apiextensions.CustomResourceSubresources{ + Status: &apiextensions.CustomResourceSubresourceStatus{}, + }, + Versions: []apiextensions.CustomResourceDefinitionVersion{{ + Name: "v1beta1", Served: true, Storage: true, + }, { + Name: "v1alpha1", Served: true, + }}, + Conversion: &apiextensions.CustomResourceConversion{ + Strategy: apiextensions.NoneConverter, + }, + PreserveUnknownFields: pointer.BoolPtr(false), + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"v1beta1"}, + }, + }, + wantErr: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + d := local.New() + + b, err := NewBackend(Driver(d)) + if err != nil { + t.Fatal(err) + } + targets := Targets(&handler{}) + if tc.targets != nil { + targets = Targets(tc.targets...) + } + c, err := b.NewClient(targets) + if err != nil { + t.Fatal(err) + } + + err = c.ValidateConstraintTemplate(tc.template) + + if !errors.Is(err, tc.wantErr) { + t.Fatalf("got CreateTemplate() error = %v, want %v", + err, tc.wantErr) + } + }) + } +} diff --git a/constraint/pkg/client/drivers/local/args.go b/constraint/pkg/client/drivers/local/args.go index eea197625..a21d6fe31 100644 --- a/constraint/pkg/client/drivers/local/args.go +++ b/constraint/pkg/client/drivers/local/args.go @@ -47,13 +47,13 @@ func Tracing(enabled bool) Arg { } func PrintEnabled(enabled bool) Arg { - return func(d *driver) { + return func(d *Driver) { d.printEnabled = enabled } } func PrintHook(hook print.Hook) Arg { - return func(d *driver) { + return func(d *Driver) { d.printHook = hook } } diff --git a/constraint/pkg/client/drivers/local/local.go b/constraint/pkg/client/drivers/local/local.go index 76e5e6b97..64266b666 100644 --- a/constraint/pkg/client/drivers/local/local.go +++ b/constraint/pkg/client/drivers/local/local.go @@ -508,10 +508,11 @@ func (d *Driver) Dump(ctx context.Context) (string, error) { return string(b), nil } -// AddTemplate implements drivers.Driver. -func (d *Driver) AddTemplate(templ *templates.ConstraintTemplate) error { +// ValidateConstraintTemplate validates the rego in template target by parsing +// rego modules. +func (d *Driver) ValidateConstraintTemplate(templ *templates.ConstraintTemplate) (string, []string, error) { if err := validateTargets(templ); err != nil { - return err + return "", nil, err } targetSpec := templ.Spec.Targets[0] targetHandler := targetSpec.Target @@ -523,28 +524,28 @@ func (d *Driver) AddTemplate(templ *templates.ConstraintTemplate) error { []string{libPrefix}, d.externs) if err != nil { - return fmt.Errorf("creating rego rewriter: %w", err) + return "", nil, fmt.Errorf("creating rego rewriter: %w", err) } namePrefix := createTemplatePath(targetHandler, kind) entryPoint, err := parseModule(namePrefix, templ.Spec.Targets[0].Rego) if err != nil { - return fmt.Errorf("%w: %v", ErrInvalidConstraintTemplate, err) + return "", nil, fmt.Errorf("%w: %v", ErrInvalidConstraintTemplate, err) } if entryPoint == nil { - return fmt.Errorf("%w: failed to parse module for unknown reason", + return "", nil, fmt.Errorf("%w: failed to parse module for unknown reason", ErrInvalidConstraintTemplate) } if err = rewriteModulePackage(namePrefix, entryPoint); err != nil { - return err + return "", nil, err } req := map[string]struct{}{violation: {}} if err = requireModuleRules(entryPoint, req); err != nil { - return fmt.Errorf("%w: invalid rego: %v", + return "", nil, fmt.Errorf("%w: invalid rego: %v", ErrInvalidConstraintTemplate, err) } @@ -552,14 +553,14 @@ func (d *Driver) AddTemplate(templ *templates.ConstraintTemplate) error { for idx, libSrc := range targetSpec.Libs { libPath := fmt.Sprintf(`%s["lib_%d"]`, pkgPrefix, idx) if err = rr.AddLib(libPath, libSrc); err != nil { - return fmt.Errorf("%w: %v", + return "", nil, fmt.Errorf("%w: %v", ErrInvalidConstraintTemplate, err) } } sources, err := rr.Rewrite() if err != nil { - return fmt.Errorf("%w: %v", + return "", nil, fmt.Errorf("%w: %v", ErrInvalidConstraintTemplate, err) } @@ -573,9 +574,15 @@ func (d *Driver) AddTemplate(templ *templates.ConstraintTemplate) error { return nil }) if err != nil { - return fmt.Errorf("%w: %v", + return "", nil, fmt.Errorf("%w: %v", ErrInvalidConstraintTemplate, err) } + return namePrefix, mods, nil +} + +// AddTemplate implements drivers.Driver. +func (d *Driver) AddTemplate(templ *templates.ConstraintTemplate) error { + namePrefix, mods, err := d.ValidateConstraintTemplate(templ) if err != nil { return err } @@ -662,6 +669,10 @@ func requireModuleRules(module *ast.Module, requiredRules map[string]struct{}) e // validateTargets ensures that the targets field has the appropriate values. func validateTargets(templ *templates.ConstraintTemplate) error { + if templ == nil { + return fmt.Errorf(`%w: ConstraintTemplate is nil`, + ErrInvalidConstraintTemplate) + } targets := templ.Spec.Targets if targets == nil { return fmt.Errorf(`%w: field "targets" not specified in ConstraintTemplate spec`,