Add ConstraintTemplate v1 #121

julianKatz · 2021-06-01T23:40:17Z

We recently upgraded Constraint Framework to produce v1 CRDs when
creating Constraint kind CRDs. This was in preparation for the release
of k8s 1.22, which removes the v1beta1 CRD version. See
open-policy-agent/gatekeeper#550 for more info.

As v1beta1 ConstraintTemplate did not required any user-entered schema
information to be structural, transformation logic was implemented to
"structuralize" the user-inputted schema information as needed.

The new v1 ConstraintTemplate version purposefully does no
transformation, as it is meant to put the ConstraintTemplate creation
experience in line with that of a v1 CRDs. Any schema information added
by the user is expected to be structural. If non-structural schema info
is added, an error should be returned.

Contributes to open-policy-agent/gatekeeper#550

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

codecov-commenter · 2021-06-01T23:42:58Z

Codecov Report

Merging #121 (0e3672b) into master (1a90d7c) will decrease coverage by 2.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   44.31%   42.19%   -2.13%     
==========================================
  Files          30       35       +5     
  Lines        2419     2766     +347     
==========================================
+ Hits         1072     1167      +95     
- Misses       1022     1234     +212     
- Partials      325      365      +40

Flag	Coverage Δ
unittests	`42.19% <ø> (-2.13%)`	⬇️

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

Impacted Files	Coverage Δ
...apis/templates/v1beta1/constrainttemplate_types.go	`100.00% <0.00%> (ø)`
...pis/templates/v1alpha1/constrainttemplate_types.go	`100.00% <0.00%> (ø)`
.../pkg/apis/templates/v1/constrainttemplate_types.go	`100.00% <0.00%> (ø)`
...rks/constraint/pkg/apis/templates/v1/conversion.go	`50.00% <0.00%> (ø)`
...int/pkg/apis/templates/v1/zz_generated.deepcopy.go	`29.46% <0.00%> (ø)`
...t/pkg/apis/templates/v1/zz_generated.conversion.go	`24.20% <0.00%> (ø)`
...works/constraint/pkg/apis/templates/v1/register.go	`0.00% <0.00%> (ø)`
...onstraint/pkg/apis/templates/v1beta1/conversion.go	`50.00% <0.00%> (+4.54%)`	⬆️
...nstraint/pkg/apis/templates/v1alpha1/conversion.go	`50.00% <0.00%> (+4.54%)`	⬆️

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 1a90d7c...0e3672b. Read the comment docs.

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

julianKatz · 2021-06-03T17:03:56Z

@maxsmythe and I have been trying to decide if Gatekeeper needs a Version Conversion Webhook to handle the conversion between v1alpha1, v1beta1, and v1 ConstraintTemplates.

To deal with reverse-compatibility, we've added the legacySchema: bool field. This handles the case of requesting a CT that was applied as v1beta1 as a v1, where the schema is expected to be structural. Rather than transform the schema information at request time (which would also require that we un-transform it when going back the other way), we can slap onlegacyScheam: true and leave it as is. The difference between the versions will be that legacySchema defaults to true in v1alpha1 and v1beta, but to false in v1.

To validate that this acted as expected, I did the following manual test:

build and deploy Gatekeeper at HEAD, with a version of the CT CRD that contains only versions v1alpha1 and v1beta1
Apply a v1beta1 CT, wait for it to persist on the API server
Apply the updated CT CRD, which contains version v1. v1 becomes the storage version.
Use kubectl to retrieve the CT we applied earlier

This test showed that the CT (originally applied as a v1beta1) was retrieved by kubectl as v1, and that legacySchema was set to true. This bodes well. After an upgrade, it looks like Kubernetes will correctly assign our defaults to the object in a way that constrainttemplate_controller.go can depend on.

Now, I'm going to dig into the details of how this process happens in the API server. We want to insure that this is the supported path, and not just a happy accident.

julianKatz · 2021-06-04T21:34:40Z

Further investigation in the docs has confirmed this behavior:

"Defaulting happens on the object... when reading from etcd using the storage version defaults".

As the previous comment showed, the defaults from the version that a given CR was stored as are applied when retrieving that CR. This appears to be the case even when that CR is retrieved another version. In my test case, that meant that a v1beta1 ConstraintTemplate saved in etcd was given (at retrieval time) the default value assigned to v1beta1 CTs in the CT CRD. This happened even though the CT was requested at the newer v1 version, which has a different default value.

maxsmythe · 2021-06-08T20:00:49Z

What's here LGTM, but should we also add the honoring of legacySchema to this PR so that we don't recognize v1 without also recognizing its behavioral changes?

maxsmythe

Forgot to mark my comment as a change request on the PR

Previously, I left ConstraintTemplate (CT) versions v1alpha1 and v1beta1 using apiextensionsv1beta1.JSONSchemaProps. I thought this appropriate, given that was what they were previously implemented as. v1 CT, on the other hand, would use v1beta1 JSONSchemaProps. This turns out to be a bad idea. Any logic we write that interacts with OpenAPIV3Schema in a CT's Validation section is forced to interact with OpenAPIV3Schema's type. Before this PR, that type is apiextensionsv1beta1.JSONSchemaProps. This makes our life difficult, as v1 CT's version of this variable will have a different type. This would then require us to re-implement any logic for this section to implement a second type, or to do some gross conversions. Further, apiextensionsv1beta1 will be removed from k8s.io/apiextensions-apiserver, preventing us from upgrading that library without doing this work. Basically, this change is unavoidable and appropriate. This blocks open-policy-agent#121 Contributes to open-policy-agent/gatekeeper#550 Signed-off-by: juliankatz <juliankatz@google.com>

julianKatz · 2021-06-09T23:29:36Z

What's here LGTM, but should we also add the honoring of legacySchema to this PR so that we don't recognize v1 without also recognizing its behavioral changes?

Does the newer version of the code do what you had in mind?

Basically, for all three CT versions, the transformation is only done if legacySchema: true is specified.

maxsmythe

LGTM with one nit.

@ritazh @shomron @sozercan LGTY?

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

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

We recently upgraded Constraint Framework to produce v1 CRDs when creating Constraint kind CRDs. This was in preparation for the release of k8s 1.22, which removes the `v1beta1` CRD version. See open-policy-agent/gatekeeper#550 for more info. As v1beta1 ConstraintTemplate did _not_ required any user-entered schema information to be structural, transformation logic was implemented to "structuralize" the user-inputted schema information as needed. The new v1 ConstraintTemplate version purposefully does _no_ transformation, as it is meant to put the ConstraintTemplate creation experience in line with that of a v1 CRDs. Any schema information added by the user is expected to be structural. If non-structural schema info is added, an error should be returned. Signed-off-by: juliankatz <juliankatz@google.com>

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

julianKatz · 2021-07-01T18:43:18Z

ping :) @shomron @sozercan

shomron

LGTM

This PR incorporates frameworks commit 1dbe261 Add ConstraintTemplate v1 (open-policy-agent#121) An imperative install CRD install step was added to the Helm Upgrade GH Workflow, as Helm does not update CRDs. Contributes to #550 Signed-off-by: juliankatz juliankatz@google.com

julianKatz force-pushed the add-constraint-template-v1 branch from 8597947 to 0b05084 Compare June 2, 2021 00:06

julianKatz requested review from maxsmythe, ritazh, shomron and sozercan June 2, 2021 00:21

maxsmythe reviewed Jun 2, 2021

View reviewed changes

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

julianKatz requested a review from maxsmythe June 8, 2021 19:30

maxsmythe requested changes Jun 8, 2021

View reviewed changes

julianKatz mentioned this pull request Jun 9, 2021

ConstraintTemplate versions use apiextensionsv1 #123

Merged

julianKatz force-pushed the add-constraint-template-v1 branch from e63d671 to 2d7d8e6 Compare June 9, 2021 23:25

julianKatz requested a review from maxsmythe June 10, 2021 00:49

maxsmythe approved these changes Jun 10, 2021

View reviewed changes

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

julianKatz added 8 commits June 22, 2021 13:25

Setup basic scaffolding for v1 CT

43a229c

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

Update CRD. v1 package tests now running

86ef21f

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

Add legacySchema field with defaults

8ec1d94

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

Add custom conversion file for templates/v1

4202aca

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

Added legacySchema checks in conversion and fixed tests

1561b2c

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

Added extra test cases for when LegacySchema=false

ae8a149

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

Removed unused error field from test structs

0e3672b

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

julianKatz force-pushed the add-constraint-template-v1 branch from 6fc5688 to 0e3672b Compare June 22, 2021 20:25

shomron approved these changes Jul 1, 2021

View reviewed changes

julianKatz merged commit 1dbe261 into open-policy-agent:master Jul 1, 2021

julianKatz deleted the add-constraint-template-v1 branch July 1, 2021 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ConstraintTemplate v1 #121

Add ConstraintTemplate v1 #121

julianKatz commented Jun 1, 2021 •

edited

Loading

codecov-commenter commented Jun 1, 2021 •

edited

Loading

julianKatz commented Jun 3, 2021

julianKatz commented Jun 4, 2021

maxsmythe commented Jun 8, 2021

maxsmythe left a comment

julianKatz commented Jun 9, 2021

maxsmythe left a comment

julianKatz commented Jul 1, 2021

shomron left a comment

Add ConstraintTemplate v1 #121

Add ConstraintTemplate v1 #121

Conversation

julianKatz commented Jun 1, 2021 • edited Loading

codecov-commenter commented Jun 1, 2021 • edited Loading

Codecov Report

julianKatz commented Jun 3, 2021

julianKatz commented Jun 4, 2021

maxsmythe commented Jun 8, 2021

maxsmythe left a comment

Choose a reason for hiding this comment

julianKatz commented Jun 9, 2021

maxsmythe left a comment

Choose a reason for hiding this comment

julianKatz commented Jul 1, 2021

shomron left a comment

Choose a reason for hiding this comment

julianKatz commented Jun 1, 2021 •

edited

Loading

codecov-commenter commented Jun 1, 2021 •

edited

Loading