-
Notifications
You must be signed in to change notification settings - Fork 49
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix scheme registration in pkg/apis/templates
PR #138 introduced a bug: the type conversion functions required by the Scheme for use in the ToVersionless functions of the v1, v1beta1, v1alpha1 packages were missing. This manifested itself as an error in Gatekeeper: "unable to convert template: converting (v1beta1.ConstraintTemplate) to (templates.ConstraintTemplate): unknown conversion" This problem was due to the ordering of init() functions. init functions are called according to the lexicographic order of their containing files. zz_generated.conversion.go registers the conversion functions with the scheme, but did so after the init() function previously held in defaults.go due to the aforementioned ordering. This left the Scheme used in ToVersionless without the conversion functions it was expected to have. Rather than hack this ordering to ensure a correctly populated `localSchemeBuilder`, this PR makes a schemeBuilder with the explicit purpose of using it in the Scheme that's used in ToVersionless. This decouples the scheme from the ordering of init() funcs, resolving the issue. This PR also adds unit tests for each of the ToVersionless funcs, ensuring such a problem won't happen again. Signed-off-by: juliankatz <juliankatz@google.com>
- Loading branch information
1 parent
7a19e4e
commit c6f3e0f
Showing
13 changed files
with
468 additions
and
93 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| package v1 | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/google/go-cmp/cmp" | ||
| "github.com/open-policy-agent/frameworks/constraint/pkg/core/templates" | ||
| "github.com/open-policy-agent/frameworks/constraint/pkg/schema" | ||
| apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/utils/pointer" | ||
| ) | ||
|
|
||
| func TestToVersionless(t *testing.T) { | ||
| tcs := []struct { | ||
| name string | ||
| versioned *ConstraintTemplate | ||
| want *templates.ConstraintTemplate | ||
| }{ | ||
| { | ||
| name: "basic conversion", | ||
| versioned: &ConstraintTemplate{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| Kind: "ConstraintTemplate", | ||
| APIVersion: "templates.gatekeeper.sh/v1", | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "MustHaveMoreCats", | ||
| }, | ||
| Spec: ConstraintTemplateSpec{ | ||
| CRD: CRD{ | ||
| Spec: CRDSpec{ | ||
| Names: Names{ | ||
| Kind: "MustHaveMoreCats", | ||
| ShortNames: []string{"mhmc"}, | ||
| }, | ||
| Validation: &Validation{ | ||
| OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ | ||
| Properties: map[string]apiextensionsv1.JSONSchemaProps{ | ||
| "message": { | ||
| Type: "string", | ||
| }, | ||
| "labels": { | ||
| Type: "array", | ||
| Items: &apiextensionsv1.JSONSchemaPropsOrArray{ | ||
| Schema: &apiextensionsv1.JSONSchemaProps{ | ||
| Type: "object", | ||
| Properties: map[string]apiextensionsv1.JSONSchemaProps{ | ||
| "key": {Type: "string"}, | ||
| "allowedRegex": {Type: "string"}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| Targets: []Target{ | ||
| { | ||
| Target: "sometarget", | ||
| Rego: `package hello ; violation[{"msg": "msg"}] { true }`, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| want: &templates.ConstraintTemplate{ | ||
| // TypeMeta isn't copied in conversion | ||
| TypeMeta: metav1.TypeMeta{}, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "MustHaveMoreCats", | ||
| }, | ||
| Spec: templates.ConstraintTemplateSpec{ | ||
| CRD: templates.CRD{ | ||
| Spec: templates.CRDSpec{ | ||
| Names: templates.Names{ | ||
| Kind: "MustHaveMoreCats", | ||
| ShortNames: []string{"mhmc"}, | ||
| }, | ||
| Validation: &templates.Validation{ | ||
| // A default was applied | ||
| LegacySchema: pointer.BoolPtr(false), | ||
| OpenAPIV3Schema: schema.VersionlessSchema(), | ||
| }, | ||
| }, | ||
| }, | ||
| Targets: []templates.Target{ | ||
| { | ||
| Target: "sometarget", | ||
| Rego: `package hello ; violation[{"msg": "msg"}] { true }`, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tcs { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| got, err := tc.versioned.ToVersionless() | ||
| if err != nil { | ||
| t.Fatalf("Failed to convert to versionless: %s", err) | ||
| } | ||
|
|
||
| if diff := cmp.Diff(tc.want, got); diff != "" { | ||
| t.Errorf("ToVersionless() mismatch (-want +got):\n%s", diff) | ||
| } | ||
| }) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| package v1 | ||
|
|
||
| import ( | ||
| ctschema "github.com/open-policy-agent/frameworks/constraint/pkg/schema" | ||
| "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" | ||
| apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" | ||
| "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| ) | ||
|
|
||
| const version = "v1" | ||
|
|
||
| var ( | ||
| structuralSchema *schema.Structural | ||
| sch *runtime.Scheme | ||
| ) | ||
|
|
||
| func init() { | ||
| // Prevent problems with ordering of init() function calls. These | ||
| // functions are called according to the lexicographic order of their | ||
| // containing files. As Register() is called on the localSchemeBuilder by | ||
| // zz_generated.conversion.go, the conversion functions haven't been | ||
| // registered with the localSchemeBuilder by the time this init() function | ||
| // runs. We sidestep this problem by adding RegisterConversions here. | ||
| schemeBuilder := runtime.NewSchemeBuilder(localSchemeBuilder...) | ||
| schemeBuilder.Register(RegisterConversions) | ||
|
|
||
| sch = runtime.NewScheme() | ||
| var err error | ||
| if err = apiextensionsv1.AddToScheme(sch); err != nil { | ||
| panic(err) | ||
| } | ||
| if err = apiextensions.AddToScheme(sch); err != nil { | ||
| panic(err) | ||
| } | ||
| if err = schemeBuilder.AddToScheme(sch); err != nil { | ||
| panic(err) | ||
| } | ||
| if structuralSchema, err = ctschema.CRDSchema(sch, version); err != nil { | ||
| panic(err) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| package v1alpha1 | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/google/go-cmp/cmp" | ||
| "github.com/open-policy-agent/frameworks/constraint/pkg/core/templates" | ||
| "github.com/open-policy-agent/frameworks/constraint/pkg/schema" | ||
| apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/utils/pointer" | ||
| ) | ||
|
|
||
| func TestToVersionless(t *testing.T) { | ||
| tcs := []struct { | ||
| name string | ||
| versioned *ConstraintTemplate | ||
| want *templates.ConstraintTemplate | ||
| }{ | ||
| { | ||
| name: "basic conversion", | ||
| versioned: &ConstraintTemplate{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| Kind: "ConstraintTemplate", | ||
| APIVersion: "templates.gatekeeper.sh/v1", | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "MustHaveMoreCats", | ||
| }, | ||
| Spec: ConstraintTemplateSpec{ | ||
| CRD: CRD{ | ||
| Spec: CRDSpec{ | ||
| Names: Names{ | ||
| Kind: "MustHaveMoreCats", | ||
| ShortNames: []string{"mhmc"}, | ||
| }, | ||
| Validation: &Validation{ | ||
| OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ | ||
| Properties: map[string]apiextensionsv1.JSONSchemaProps{ | ||
| "message": { | ||
| Type: "string", | ||
| }, | ||
| "labels": { | ||
| Type: "array", | ||
| Items: &apiextensionsv1.JSONSchemaPropsOrArray{ | ||
| Schema: &apiextensionsv1.JSONSchemaProps{ | ||
| Type: "object", | ||
| Properties: map[string]apiextensionsv1.JSONSchemaProps{ | ||
| "key": {Type: "string"}, | ||
| "allowedRegex": {Type: "string"}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| Targets: []Target{ | ||
| { | ||
| Target: "sometarget", | ||
| Rego: `package hello ; violation[{"msg": "msg"}] { true }`, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| want: &templates.ConstraintTemplate{ | ||
| // TypeMeta isn't copied in conversion | ||
| TypeMeta: metav1.TypeMeta{}, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "MustHaveMoreCats", | ||
| }, | ||
| Spec: templates.ConstraintTemplateSpec{ | ||
| CRD: templates.CRD{ | ||
| Spec: templates.CRDSpec{ | ||
| Names: templates.Names{ | ||
| Kind: "MustHaveMoreCats", | ||
| ShortNames: []string{"mhmc"}, | ||
| }, | ||
| Validation: &templates.Validation{ | ||
| // A default was applied | ||
| LegacySchema: pointer.BoolPtr(true), | ||
| OpenAPIV3Schema: schema.VersionlessSchemaWithXPreserve(), | ||
| }, | ||
| }, | ||
| }, | ||
| Targets: []templates.Target{ | ||
| { | ||
| Target: "sometarget", | ||
| Rego: `package hello ; violation[{"msg": "msg"}] { true }`, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tcs { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| got, err := tc.versioned.ToVersionless() | ||
| if err != nil { | ||
| t.Fatalf("Failed to convert to versionless: %s", err) | ||
| } | ||
|
|
||
| if diff := cmp.Diff(tc.want, got); diff != "" { | ||
| t.Errorf("ToVersionless() mismatch (-want +got):\n%s", diff) | ||
| } | ||
| }) | ||
| } | ||
| } |
Oops, something went wrong.