Add crds.yaml-based defaulting functions to pkg/apis/templates/... #129

julianKatz · 2021-07-29T00:06:59Z

Each ConstrainTemplate version (v1alpha1, v1beta1, v1) has a default
value for the LegacySchema field. true for v1alpha1, v1beta1, and
false for v1. These defaults are defined in the CRD yaml.

For the ConstraintTemplate golang types to have the same functionality
to have the same defaulting functionality. We are required to write
custom defaulting functions for each defaulted field.

While we could do this, we could easily see how this practice could
become unsustainable. A forgotten defaulting function would yield
unexpected bugs.

To remedy this, this PR adds defaulting functions that default based on
the content of deploy/crds.yaml, i.e. the ConstraintTemplate CRD
generated by controller-gen.

This provides us with a single defaulting source of truth: the
constrainttemplate_types.go files, which contain annotations that
define a field's default values.

Fixes #128

Signed-off-by: juliankatz juliankatz@google.com

Signed-off-by: juliankatz <juliankatz@google.com>

functions Signed-off-by: juliankatz <juliankatz@google.com>

Signed-off-by: juliankatz <juliankatz@google.com>

Each ConstrainTemplate version (v1alpha1, v1beta1, v1) has a default value for the LegacySchema field. `true` for v1alpha1, v1beta1, and `false` for v1. These defaults are defined in the CRD yaml. For the ConstraintTemplate golang types to have the same functionality to have the same defaulting functionality. We are required to write custom defaulting functions for each defaulted field. While we could do this, we could easily see how this practice could become unsustainable. A forgotten defaulting function would yield unexpected bugs. To remedy this, this PR adds defaulting functions that default based on the content of deploy/crds.yaml, i.e. the ConstraintTemplate CRD generated by controller-gen. This provides us with a single defaulting source of truth: the constrainttemplate_types.go files, which contain annotations that define a field's default values. Fixes open-policy-agent#128 Signed-off-by: juliankatz <juliankatz@google.com>

Signed-off-by: juliankatz <juliankatz@google.com>

codecov-commenter · 2021-07-29T00:10:36Z

Codecov Report

Merging #129 (889ec45) into master (d4a673c) will decrease coverage by 0.20%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #129      +/-   ##
==========================================
- Coverage   42.59%   42.39%   -0.21%     
==========================================
  Files          35       49      +14     
  Lines        2775     2951     +176     
==========================================
+ Hits         1182     1251      +69     
- Misses       1228     1321      +93     
- Partials      365      379      +14

Flag	Coverage Δ
unittests	`42.39% <ø> (-0.21%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...eworks/constraint/pkg/apis/templates/v1/helpers.go	`0.00% <0.00%> (ø)`
.../constraint/pkg/apis/templates/v1alpha1/helpers.go	`0.00% <0.00%> (ø)`
...s/constraint/pkg/apis/templates/v1beta1/helpers.go	`0.00% <0.00%> (ø)`
...traint/pkg/apis/templates/zz_generated.defaults.go	`0.00% <0.00%> (ø)`
...kg/apis/templates/v1beta1/zz_generated.defaults.go	`50.00% <0.00%> (ø)`
...eworks/constraint/pkg/apis/templates/crd_schema.go	`50.00% <0.00%> (ø)`
...t/frameworks/constraint/pkg/apis/templates/init.go	`100.00% <0.00%> (ø)`
...works/constraint/pkg/apis/templates/v1/defaults.go	`55.55% <0.00%> (ø)`
...int/pkg/apis/templates/v1/zz_generated.defaults.go	`50.00% <0.00%> (ø)`
...constraint/pkg/apis/templates/v1alpha1/defaults.go	`55.55% <0.00%> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4a673c...889ec45. Read the comment docs.

maxsmythe

LGTM with a couple nits

maxsmythe · 2021-07-29T00:14:38Z

constraint/pkg/apis/templates/crd_schema.go

+	}
+
+	unCRD := unstructured.Unstructured{}
+	unCRD.UnmarshalJSON(crdJSON)


We can't unmarshal directly to constraintTemplateCRD and save the middle step of converting to unstructured?

It doesn't look like CustomResourceDefinition has an Unmarshal function.

apiextensions-apiserver/pkg/apis/apiextensions/v1/marshal.go does have unmarshalling logic, but it only supports doing so into a JSONSchemaPropsOrArray.

One idea:

I did the unmarshalling + FromUnstructured() because it made it easier to dive into the versions: and populate my ConstraintTemplateSchemas map. I could probably simplify this logic by diving straight to versions in the unstructured and unmarshalling those JSON blobs straight into apiextensionsv1.JSONSchemaProps. I'd then convert to apiextensions.JSONSchemaProps (apiextensions has no unmarshalling methods, only the versioned sub-packages) and create the Structural type for the map.

I tried this^ and it's more trouble than it's worth. B/c I use multiple properties of a version in my loop, it pays to convert to a CustomResourceDefinition{}.

constraint/pkg/apis/templates/crd_schema.go

constraint/pkg/apis/templates/v1/defaults.go

maxsmythe

Actually, digging a bit deeper, it looks like we have these defaulting functions (which is good), but they're not being applied anywhere.

This means issues like open-policy-agent/gatekeeper#1458 will still occur.

Signed-off-by: juliankatz <juliankatz@google.com>

maxsmythe

Looking good!

I think the last piece would be a "conversion" helper function that knows to apply defaults before converting. This could be leveraged by gator test

constraint/pkg/apis/templates/v1/constrainttemplate_types.go

julianKatz · 2021-07-30T18:00:43Z

Looking good!

I think the last piece would be a "conversion" helper function that knows to apply defaults before converting. This could be leveraged by gator test

Is there an existing place where we'll use this function? That would help me know I'm making the right thing.

Signed-off-by: juliankatz <juliankatz@google.com>

… crd-yaml-based-defaulting Signed-off-by: juliankatz <juliankatz@google.com>

Signed-off-by: juliankatz <juliankatz@google.com>

maxsmythe

LGTM, thanks for figuring this out!

maxsmythe · 2021-08-03T22:50:28Z

Though it looks like there are linting errors, and we need to re-bake the CRD yaml into the golang constant

maxsmythe

Sorry, missed some things

constraint/pkg/apis/templates/v1/helpers.go

Signed-off-by: juliankatz <juliankatz@google.com>

constraint/pkg/apis/templates/v1/helpers.go

constraint/pkg/apis/templates/v1beta1/helpers.go

constraint/pkg/apis/templates/v1alpha1/helpers.go

Signed-off-by: juliankatz <juliankatz@google.com>

maxsmythe

LGTM

Signed-off-by: juliankatz <juliankatz@google.com>

julianKatz · 2021-08-04T23:34:51Z

ping :) @shomron @ritazh @sozercan

constraint/pkg/apis/templates/scheme.go

constraint/pkg/apis/templates/crd_schema.go

willbeason · 2021-08-09T15:40:51Z

constraint/pkg/apis/templates/init.go

@@ -0,0 +1,6 @@
+package templates
+
+func init() {


It is valid to have multiple init() functions in the same Go package. You can rename both initializeScheme and initializeCTSchemaMap to init and it will work.

The problem wasn't the inability to have multiple init() calls, but rather that initializeCTSchemaMap() uses the Scheme that's created in initializeScheme(). As init functions are run according to the order of their containing file's name (crd_schema.go before scheme.go), this caused a null-pointer exception.

Rather than fix this through naming, I figured the best way to was to call them in an intentional order like I did in this init.go file. I'll add a comment to init.go explaining that choice.

Or, perhaps there's a pattern that would better suit this use-case? @willbeason

Is there a reason the contents of those two methods can't be in the same function? initializeScheme is rather short.

constraint/pkg/apis/templates/test_fakes.go

constraint/pkg/apis/templates/v1/conversion.go

constraint/pkg/apis/templates/v1/defaults_test.go

constraint/pkg/apis/templates/v1beta1/defaults.go

constraint/pkg/apis/templates/crd_schema.go

Signed-off-by: juliankatz <juliankatz@google.com>

… crd-yaml-based-defaulting Signed-off-by: juliankatz <juliankatz@google.com>

julianKatz added 9 commits July 27, 2021 16:34

Create a make target that saves the crd yaml as a string constant

e78be93

Signed-off-by: juliankatz <juliankatz@google.com>

Add CRD unmarshalling and a basic test

e09cef3

Signed-off-by: juliankatz <juliankatz@google.com>

Add a function for retrieving a JSONSchemaProps

444b5a1

Signed-off-by: juliankatz <juliankatz@google.com>

ingest the CT yaml into a map of Structural types and build conversion

25f28eb

functions Signed-off-by: juliankatz <juliankatz@google.com>

Generate default files for v1, v1beta1, v1alpha1

2141461

Signed-off-by: juliankatz <juliankatz@google.com>

Add LegacySchema to templates.ConstraintTemplate. Update conversion

3824d78

Signed-off-by: juliankatz <juliankatz@google.com>

Upgrade kustomize in Dockerfile

1829b90

Signed-off-by: juliankatz <juliankatz@google.com>

Improved panic messaging

de1a658

Signed-off-by: juliankatz <juliankatz@google.com>

julianKatz requested review from shomron, sozercan, willbeason and maxsmythe July 29, 2021 00:06

maxsmythe approved these changes Jul 29, 2021

View reviewed changes

maxsmythe requested changes Jul 29, 2021

View reviewed changes

julianKatz added 4 commits July 29, 2021 16:17

Removed an unncessary deepCopy step

71a8d01

Signed-off-by: juliankatz <juliankatz@google.com>

Fix trailing spaces for lint

a63dc1b

Signed-off-by: juliankatz <juliankatz@google.com>

Fixed some linter errors

395e49f

Signed-off-by: juliankatz <juliankatz@google.com>

Removed JULIAN comments

ba962d9

Signed-off-by: juliankatz <juliankatz@google.com>

julianKatz requested a review from maxsmythe July 29, 2021 23:40

julianKatz added 6 commits July 29, 2021 17:06

Make legacySchema type *bool

845a55b

Signed-off-by: juliankatz <juliankatz@google.com>

DRY up constrainttemplate_types_test fixtures for v1, v1alpha1, v1beta1

1b3c5e0

Signed-off-by: juliankatz <juliankatz@google.com>

Add conversion test cases for LegacySchema: nil

f3649b4

Signed-off-by: juliankatz <juliankatz@google.com>

Simplify crd_schema with json.Unmarshal()

afd0219

Signed-off-by: juliankatz <juliankatz@google.com>

Use scheme.Convert in tests instead of direct conversion funcs

69e494e

Signed-off-by: juliankatz <juliankatz@google.com>

Update generated code with legacySchema as *bool

249b98d

Signed-off-by: juliankatz <juliankatz@google.com>

maxsmythe requested changes Jul 30, 2021

View reviewed changes

constraint/pkg/apis/templates/v1/constrainttemplate_types.go Outdated Show resolved Hide resolved

Improve *bool comment

e0fe0e6

Signed-off-by: juliankatz <juliankatz@google.com>

julianKatz added 2 commits August 3, 2021 14:46

Merge branch 'master' of github.com:open-policy-agent/frameworks into…

47eaa2d

… crd-yaml-based-defaulting Signed-off-by: juliankatz <juliankatz@google.com>

Add the ToVersionless helper function

54c6097

Signed-off-by: juliankatz <juliankatz@google.com>

maxsmythe approved these changes Aug 3, 2021

View reviewed changes

maxsmythe requested changes Aug 3, 2021

View reviewed changes

constraint/pkg/apis/templates/v1/helpers.go Outdated Show resolved Hide resolved

julianKatz added 3 commits August 3, 2021 15:52

re-gen

17e4042

Signed-off-by: juliankatz <juliankatz@google.com>

Fixed *bool errors

b5181cd

Signed-off-by: juliankatz <juliankatz@google.com>

Generate schemes for ToVersionless functions

8b29d78

Signed-off-by: juliankatz <juliankatz@google.com>

maxsmythe requested changes Aug 3, 2021

View reviewed changes

constraint/pkg/apis/templates/v1/helpers.go Outdated Show resolved Hide resolved

constraint/pkg/apis/templates/v1beta1/helpers.go Outdated Show resolved Hide resolved

constraint/pkg/apis/templates/v1alpha1/helpers.go Outdated Show resolved Hide resolved

Use a shared scheme for pkg/apis/templates/...

73dcbcb

Signed-off-by: juliankatz <juliankatz@google.com>

julianKatz force-pushed the crd-yaml-based-defaulting branch from 11b763b to 73dcbcb Compare August 4, 2021 20:00

maxsmythe approved these changes Aug 4, 2021

View reviewed changes

Reorganize init actions

7e84176

Signed-off-by: juliankatz <juliankatz@google.com>

julianKatz force-pushed the crd-yaml-based-defaulting branch from 239a1e9 to 7e84176 Compare August 4, 2021 23:32

julianKatz requested a review from ritazh August 4, 2021 23:34

maxsmythe reviewed Aug 7, 2021

View reviewed changes

constraint/pkg/apis/templates/scheme.go Outdated Show resolved Hide resolved

willbeason reviewed Aug 9, 2021

View reviewed changes

ritazh reviewed Aug 10, 2021

View reviewed changes

constraint/pkg/apis/templates/crd_schema.go Outdated Show resolved Hide resolved

Fix review comments

7068064

Signed-off-by: juliankatz <juliankatz@google.com>

willbeason approved these changes Aug 10, 2021

View reviewed changes

julianKatz requested a review from ritazh August 10, 2021 21:29

julianKatz added 2 commits August 10, 2021 14:36

Fix null pointer exception by reverting conversion.go files

1cf5832

Signed-off-by: juliankatz <juliankatz@google.com>

Fix pointer copying problem in copying LegacySchema

65f059b

Signed-off-by: juliankatz <juliankatz@google.com>

julianKatz force-pushed the crd-yaml-based-defaulting branch from c2c4af0 to 65f059b Compare August 11, 2021 00:03

ritazh approved these changes Aug 11, 2021

View reviewed changes

Merge branch 'master' of github.com:open-policy-agent/frameworks into…

889ec45

… crd-yaml-based-defaulting Signed-off-by: juliankatz <juliankatz@google.com>

julianKatz merged commit 2924b2c into open-policy-agent:master Aug 16, 2021

julianKatz deleted the crd-yaml-based-defaulting branch August 16, 2021 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add crds.yaml-based defaulting functions to pkg/apis/templates/... #129

Add crds.yaml-based defaulting functions to pkg/apis/templates/... #129

julianKatz commented Jul 29, 2021

codecov-commenter commented Jul 29, 2021 •

edited

maxsmythe left a comment

maxsmythe Jul 29, 2021

julianKatz Jul 29, 2021

julianKatz Jul 29, 2021

maxsmythe left a comment •

edited

maxsmythe left a comment

julianKatz commented Jul 30, 2021

maxsmythe left a comment

maxsmythe commented Aug 3, 2021

maxsmythe left a comment

maxsmythe left a comment

julianKatz commented Aug 4, 2021

willbeason Aug 9, 2021

julianKatz Aug 10, 2021 •

edited

willbeason Aug 10, 2021

Add crds.yaml-based defaulting functions to pkg/apis/templates/... #129

Add crds.yaml-based defaulting functions to pkg/apis/templates/... #129

Conversation

julianKatz commented Jul 29, 2021

codecov-commenter commented Jul 29, 2021 • edited

Codecov Report

maxsmythe left a comment

Choose a reason for hiding this comment

maxsmythe Jul 29, 2021

Choose a reason for hiding this comment

julianKatz Jul 29, 2021

Choose a reason for hiding this comment

julianKatz Jul 29, 2021

Choose a reason for hiding this comment

maxsmythe left a comment • edited

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

julianKatz commented Jul 30, 2021

maxsmythe left a comment

Choose a reason for hiding this comment

maxsmythe commented Aug 3, 2021

maxsmythe left a comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

julianKatz commented Aug 4, 2021

willbeason Aug 9, 2021

Choose a reason for hiding this comment

julianKatz Aug 10, 2021 • edited

Choose a reason for hiding this comment

willbeason Aug 10, 2021

Choose a reason for hiding this comment

codecov-commenter commented Jul 29, 2021 •

edited

maxsmythe left a comment •

edited

julianKatz Aug 10, 2021 •

edited