Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix scheme registration in pkg/apis/templates #142

Merged
merged 3 commits into from
Oct 12, 2021
Merged

Fix scheme registration in pkg/apis/templates #142

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c6f3e0f
Fix scheme registration in pkg/apis/templates
julianKatz Oct 4, 2021
654f030
Merge branch 'master' of github.com:open-policy-agent/frameworks into…
julianKatz Oct 11, 2021
65407b5
Rename a variable
julianKatz Oct 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 0 additions & 28 deletions constraint/pkg/apis/templates/v1/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,10 @@
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/apiextensions-apiserver/pkg/apiserver/schema/defaulting"
"k8s.io/apimachinery/pkg/runtime"
)

const version = "v1"

var (
structuralSchema *schema.Structural
Scheme *runtime.Scheme
)

func init() {
Scheme = runtime.NewScheme()
var err error
if err = apiextensionsv1.AddToScheme(Scheme); err != nil {
panic(err)
}
if err = apiextensions.AddToScheme(Scheme); err != nil {
panic(err)
}
if err = AddToScheme(Scheme); err != nil {
panic(err)
}
if structuralSchema, err = ctschema.CRDSchema(Scheme, version); err != nil {
panic(err)
}
}

func addDefaultingFuncs(scheme *runtime.Scheme) error {
return RegisterDefaults(scheme)
}
Expand Down
4 changes: 2 additions & 2 deletions constraint/pkg/apis/templates/v1/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import (
// versionless api representation.
func (versioned *ConstraintTemplate) ToVersionless() (*templates.ConstraintTemplate, error) {
versionedCopy := versioned.DeepCopy()
Scheme.Default(versionedCopy)
sch.Default(versionedCopy)

versionless := &templates.ConstraintTemplate{}
if err := Scheme.Convert(versionedCopy, versionless, nil); err != nil {
if err := sch.Convert(versionedCopy, versionless, nil); err != nil {
return nil, err
}

Expand Down
111 changes: 111 additions & 0 deletions constraint/pkg/apis/templates/v1/helpers_test.go
View file
Open in desktop
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)
}
})
}
}
42 changes: 42 additions & 0 deletions constraint/pkg/apis/templates/v1/scheme.go
View file
Open in desktop
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
julianKatz marked this conversation as resolved.
Show resolved Hide resolved
)

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)
Copy link
Contributor

@maxsmythe maxsmythe Oct 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should create a scheme builder that explicitly registers all necessary registrations. Otherwise this function will still be vulnerable to failures due to reordering that changes the contents of localSchemeBuilder

Also, we can move this to the helpers.go package, since this is the only thing that should depend on the sch scheme.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define the schemeBuilder outside of a function as a global var to avoid ordering concerns:

var schemeBuilder = runtime.NewSchemeBuilder(fn1, fn2, fn3, ...)

Copy link
Contributor Author

@julianKatz julianKatz Oct 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we can move this to the helpers.go package, since this is the only thing that should depend on the sch scheme.

The defaults.go file also depends on the Scheme, as the scheme is used when creating the structuralSchema. I thought the separate file would prevent the assumption that the scheme only belongs to one thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define the schemeBuilder outside of a function as a global var to avoid ordering concerns:

var schemeBuilder = runtime.NewSchemeBuilder(fn1, fn2, fn3, ...)

This actually re-introduces ordering problems. One of the funcs that's included in the schemeBuilder (SchemeBuilder.AddToScheme) is declared as a package level variable in register.go: https://github.com/open-policy-agent/frameworks/blob/master/constraint/pkg/apis/templates/v1/register.go#L39

As @maxsmythe suggests, not using localSchemeBuilder's contents directly has a possible benefit. The difference between what I have here and writing out SchemeBuilder.AddToScheme, addDefaultingFuncs is that if localSchemeBuilder were to change in the future, we would not see those changes in the separate schemebuilder I'm making. That could be good or bad, depending on the scenario.

The key idea here is to take advantage of this ordering:

  1. All package variables instantiated
  2. All package init() functions run

Basically I can reliably use any package variable, but I can't rely on other init() funcs. To truly not rely on other init() functions, I'll actually need to duplicate this line from constrainttemplate_types.go in my own schemebuilder.

I see two problems that we're choosing between:

  1. Relying on init() function ordering
  2. maintaining a separate list of items that should be registered to the scheme from the one that external packages will receive when using our package's external AddToScheme function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My vote is for option 2 since it is explicit and doesn't rely on magic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's okay to have our own package-internal scheme,

var toVersionlessSchemeBuilder = runtime.NewSchemeBuilder(...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The defaults.go file also depends on the Scheme, as the scheme is used when creating the structuralSchema. I thought the separate file would prevent the assumption that the scheme only belongs to one thing.

ack, SGTM

is that if localSchemeBuilder were to change in the future, we would not see those changes in the separate schemebuilder I'm making. That could be good or bad, depending on the scenario.

Fortunately we don't need all changes to the schema builder, just those features that we rely on (defaulting and conversion it looks like).

Also note that we are coupled to the init() functions if we are using anything whose contents may change because of the init functions (such as localSchemeBuilder).

+1 to option 2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's okay to have our own package-internal scheme,

var toVersionlessSchemeBuilder = runtime.NewSchemeBuilder(...)

I'd like to avoid doing this as a package-level variable as I step back towards ordering problems. Putting all of the logic inside an init() function that is decoupled from other init() functions allows me to take advantage of other package level variables while still isolating my schemeBuilder from side-effects.


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)
}
}
28 changes: 0 additions & 28 deletions constraint/pkg/apis/templates/v1alpha1/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,10 @@
package v1alpha1

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/apiextensions-apiserver/pkg/apiserver/schema/defaulting"
"k8s.io/apimachinery/pkg/runtime"
)

const version = "v1alpha1"

var (
structuralSchema *schema.Structural
Scheme *runtime.Scheme
)

func init() {
Scheme = runtime.NewScheme()
var err error
if err = apiextensionsv1.AddToScheme(Scheme); err != nil {
panic(err)
}
if err = apiextensions.AddToScheme(Scheme); err != nil {
panic(err)
}
if err = AddToScheme(Scheme); err != nil {
panic(err)
}
if structuralSchema, err = ctschema.CRDSchema(Scheme, version); err != nil {
panic(err)
}
}

func addDefaultingFuncs(scheme *runtime.Scheme) error {
return RegisterDefaults(scheme)
}
Expand Down
4 changes: 2 additions & 2 deletions constraint/pkg/apis/templates/v1alpha1/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import (
// versionless api representation.
func (versioned *ConstraintTemplate) ToVersionless() (*templates.ConstraintTemplate, error) {
versionedCopy := versioned.DeepCopy()
Scheme.Default(versionedCopy)
sch.Default(versionedCopy)

versionless := &templates.ConstraintTemplate{}
if err := Scheme.Convert(versionedCopy, versionless, nil); err != nil {
if err := sch.Convert(versionedCopy, versionless, nil); err != nil {
return nil, err
}

Expand Down
111 changes: 111 additions & 0 deletions constraint/pkg/apis/templates/v1alpha1/helpers_test.go
View file
Open in desktop
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)
}
})
}
}
Loading