Upgrade: controller-gen v0.5.0, conversion-gen v0.20.2 #111

julianKatz · 2021-04-23T00:23:56Z

frameworks/constraint had previously been reliant on an outdated version
of the controller-gen library for generating CRDs with embedded
JSONSchemas. The older version of the library wasn't built to detect
a nested JSONSchema, and thus would not refuse it.

With the requirement of v1 CRDs around the corner in k8s 1.22,
framework/constraint requires the newer controller-gen v0.5.0. This
library version can output v1 CRD.

The conversion logic was also in need of a library upgrade, with
conversion-gen v0.21.0 now generating the necessary files.

This PR does not output a v1 CRD for Constraint Template. That will
be left for a future PR. This PR brings the two binaries up-to-date.

Note: I attempted to dockerize these two binaries. Controller-gen
worked fine, but for some reason conversion-gen couldn't write to the
filesystem. The output looked fine, but no dice. Rather than figure
out why, I decided to punt on the dockeriziation effort.

julianKatz · 2021-04-23T00:29:17Z

constraint/config/crd_patch.yaml

-      served: true
-      storage: false
-
+  version: v1beta1


Would love to get rid of this completely, but I'm not sure how.

@maxsmythe you know the kubebuilder annotations pretty well... do you know how to change it?

That said, all the v1beta1 CRD examples in this kubernetes documentation don't even include the version: field, so maybe we can just remove it.

Yeah, the version field just needs to equal the first item in the versions list, if present.

From the looks of it, this can be removed.

It looks like leaving it in is actually breaking the unit tests

Removed it using kustomize

maxsmythe

Basically LGTM with two nits.

@sozercan Can you see if this alone is enough to unblock your upgrading controller-runtime in GK? I'm skeptical because I'm not seeing new methods in the autogenned code.

If not, maybe controller-gen takes its cues from the version of controller-runtime used. And we'll need to upgrade that dependency and regen.

constraint/config/crds/templates.gatekeeper.sh_constrainttemplates.yaml

maxsmythe · 2021-04-23T00:43:26Z

constraint/config/crd_patch.yaml

-      served: true
-      storage: false
-
+  version: v1beta1


Yeah, the version field just needs to equal the first item in the versions list, if present.

From the looks of it, this can be removed.

sozercan · 2021-04-23T22:12:46Z

@maxsmythe I think we will need to regenerate the conversions for it to work with new apimachinery. Sounds like when we regenerate with new version, it should remove s.Convert() or have it accept 2 args.

https://github.com/julianKatz/frameworks/blob/upgrade-controller-gen/constraint/pkg/apis/templates/v1beta1/zz_generated.conversion.go#L438

#17 59.04 # github.com/open-policy-agent/frameworks/constraint/pkg/apis/templates/v1alpha1
#17 59.04 vendor/github.com/open-policy-agent/frameworks/constraint/pkg/apis/templates/v1alpha1/zz_generated.conversion.go:438:22: too many arguments in call to s.Convert
#17 59.04 	have (*v1beta1.JSONSchemaProps, *apiextensions.JSONSchemaProps, number)
#17 59.04 	want (interface {}, interface {})
#17 59.04 vendor/github.com/open-policy-agent/frameworks/constraint/pkg/apis/templates/v1alpha1/zz_generated.conversion.go:457:22: too many arguments in call to s.Convert
#17 59.04 	have (*apiextensions.JSONSchemaProps, *v1beta1.JSONSchemaProps, number)
#17 59.04 	want (interface {}, interface {})
#17 59.07 # github.com/open-policy-agent/frameworks/constraint/pkg/apis/templates/v1beta1
#17 59.07 vendor/github.com/open-policy-agent/frameworks/constraint/pkg/apis/templates/v1beta1/zz_generated.conversion.go:438:22: too many arguments in call to s.Convert
#17 59.07 	have (*v1beta1.JSONSchemaProps, *apiextensions.JSONSchemaProps, number)
#17 59.07 	want (interface {}, interface {})
#17 59.07 vendor/github.com/open-policy-agent/frameworks/constraint/pkg/apis/templates/v1beta1/zz_generated.conversion.go:457:22: too many arguments in call to s.Convert
#17 59.07 	have (*apiextensions.JSONSchemaProps, *v1beta1.JSONSchemaProps, number)
#17 59.07 	want (interface {}, interface {})

maxsmythe · 2021-04-23T22:22:25Z

@sozercan WDYT of committing this as-is (as this change at least gets us to a workable version of controller-gen), then we can play around with figuring out whatever kind of poking we need to do to update the conversion function signatures?

maxsmythe · 2021-04-24T04:57:25Z

@sozercan @julianKatz It looks like controller-gen doesn't integrate conversion-gen, which is what is used to generate the conversion functions. We'll need to keep building/applying that one manually (if possible):

https://pkg.go.dev/k8s.io/code-generator/cmd/conversion-gen

Luckily conversion-gen appears to be versioned in lockstep with K8s, so we should be able to upgrade it along with controller-runtime.

maxsmythe · 2021-04-24T05:24:54Z

Also, because we can just move dependencies in lockstep now, we wont need to run conversion-gen as it's own, self-contained gomodule/docker container.

sozercan · 2021-04-26T21:42:11Z

can we re-add the conversion-gen parts back to makefile (and whatever else is missing)? So we can only focus on upgrading controller-gen in this PR and follow up on conversion-gen later.

codecov-commenter · 2021-04-27T20:10:40Z

Codecov Report

Merging #111 (4c29f6c) into master (804ff2e) will decrease coverage by 0.26%.
The diff coverage is n/a.

❗ Current head 4c29f6c differs from pull request most recent head 56ce6ad. Consider uploading reports for the commit 56ce6ad to get more accurate results

@@            Coverage Diff             @@
##           master     #111      +/-   ##
==========================================
- Coverage   44.15%   43.89%   -0.27%     
==========================================
  Files          27       27              
  Lines        2405     2383      -22     
==========================================
- Hits         1062     1046      -16     
+ Misses       1023     1017       -6     
  Partials      320      320

Flag	Coverage Δ
unittests	`43.89% <ø> (-0.27%)`	⬇️

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

Impacted Files	Coverage Δ
...kg/apis/templates/v1beta1/zz_generated.deepcopy.go	`29.46% <0.00%> (-3.87%)`	⬇️
...g/apis/templates/v1alpha1/zz_generated.deepcopy.go	`29.46% <0.00%> (-3.87%)`	⬇️
...apis/templates/v1beta1/constrainttemplate_types.go	`100.00% <0.00%> (ø)`
...pis/templates/v1alpha1/constrainttemplate_types.go	`100.00% <0.00%> (ø)`

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 804ff2e...56ce6ad. Read the comment docs.

sozercan · 2021-04-28T16:51:56Z

constraint/pkg/apis/templates/v1beta1/zz_generated.conversion.go

-			return err
-		}
+		// FIXME: Provide conversion function to convert apiextensionsv1beta1.JSONSchemaProps to apiextensions.JSONSchemaProps
+		compileErrorOnMissingConversion()


this is an issue generating these conversions (and why lint is failing). not sure why this happens since these conversions exists.

example:

func autoConvert_v1beta1_Validation_To_templates_Validation(in *Validation, out *templates.Validation, s conversion.Scope) error { if in.OpenAPIV3Schema != nil { in, out := &in.OpenAPIV3Schema, &out.OpenAPIV3Schema *out = new(apiextensions.JSONSchemaProps) if err := v1.Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps(*in, *out, s); err != nil { return err } } else { out.OpenAPIV3Schema = nil } return nil } func autoConvert_templates_Validation_To_v1beta1_Validation(in *templates.Validation, out *Validation, s conversion.Scope) error { if in.OpenAPIV3Schema != nil { in, out := &in.OpenAPIV3Schema, &out.OpenAPIV3Schema *out = new(v1.JSONSchemaProps) if err := v1.Convert_apiextensions_JSONSchemaProps_To_v1_JSONSchemaProps(*in, *out, s); err != nil { return err } } else { out.OpenAPIV3Schema = nil } return nil }

Looks like this is a bug (kubernetes/kubernetes#98380)

False alarm! I just needed to make that package available to conversion-gen. Thanks for posting that example @sozercan, that was the clue I needed.

~~Issue says it's for types containing an array of pointers to custom objects. Does this apply to our types?~~

~~Is this a blocker? What are our options here?~~

glad it's resolved!

Sorry, it isn't completely resolved. I've included notes in Makefile.

But, I've got us frankensteined together enough to move forward.

sozercan · 2021-04-28T16:54:50Z

constraint/Makefile

+# Install the generation dependencies on the location machine
+gen-dependencies:
+	GO111MODULE=on go get sigs.k8s.io/controller-tools/cmd/controller-gen@v0.5.0
+	GO111MODULE=on go get k8s.io/code-generator/cmd/conversion-gen@v0.21.0


should we align this with controller-gen dependency version (v0.20.2)?

We probably want to align this with the cert-controller dependency version (but that is also v0.20.2):

https://github.com/open-policy-agent/cert-controller/blob/c6f841d912f7d057a4e9fc3fe2c49951b68ba931/go.mod#L9-L11

Will update

maxsmythe · 2021-04-28T21:51:50Z

Checks are failing.

Can we exclude zz_generated.* files from the linter? Because we don't control their contents we're mostly concerned with whether or not they build.

For the unit tests, it looks like we need to change the controller-gen invocation to go run ...

sozercan · 2021-04-29T03:09:32Z

@maxsmythe linter is failing because of undefined function getting generated (which is meant to cause build failure) #111 (review)

julianKatz · 2021-04-29T19:39:35Z

With the default call to conversion-gen, there are four missing conversions, divided into two packages (constraint/pkg/apis/templates/v1alpha1, constraint/pkg/apis/templates/v1beta). The missing conversions are the same two:

apiextensions.JSONSchemaProps to apiextensionsv1beta1.JSONSchemaProps

and

 apiextensions.JSONSchemaProps to v1beta1.JSONSchemaProps

The first is fixed by including the apiextensionsv1beta1 package (containing the auto-genned conversion function) as a flag on converison-gen:

--extra-peer-dirs=k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1

But, the second remains unfixed. Why?

It turns out that the conversion function we'd expect is actually a human written one, found at vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/conversion.go (as opposed to the generated zz_generated.conversion.go within the same package). For some reason, conversion-gen isn't able to handle that when including apiextensions/v1beta.

AFAICT, this is a bug (albeit a different one than the one I mentioned earlier).

In the meantime, I can change the line of code manually and we can move on. I'll file a bug in conversion-gen as well.

EDIT: This isn't quite right actually. It turns out that the necessary function is already generated. See it defined in the generated file.

I'm lost as to why this is happening.

sozercan · 2021-04-29T20:14:40Z

@julianKatz there are more investigations at kubernetes/kubernetes#101567

julianKatz · 2021-04-30T22:13:14Z

Update:

I've gotten around the conversion-gen bug with some manual code editing and created an issue (#112) to track the fix in this repository.

In the meantime, I believe this PR can move forward.

julianKatz · 2021-04-30T22:16:48Z

Unit tests are passing now. PTAL: @maxsmythe @sozercan @shomron

maxsmythe

Still LGTM with 1 nit, @sozercan LGTY?

maxsmythe · 2021-04-30T22:24:25Z

constraint/Makefile

 	docker build . -t constraint-test && docker run -t constraint-test

 # Install CRDs into a cluster
 install: manifests
 	kubectl apply -f config/crds

+# Install the generation dependencies on the location machine


nit: location => local?

oops, will change

sozercan · 2021-05-03T19:01:20Z

Not a huge fan of the manual code update, but I don't see any other way forward :( Can we document what to manually update in conversion? (perhaps add in #112)

julianKatz · 2021-05-03T20:05:19Z

@sozercan Good call.

I'll add a sample diff in #112 and link to it in both conversion files

frameworks/constraint had previously been reliant on an outdated version of the controller-gen library for generating CRDs with embedded JSONSchemas. The older version of the library wasn't built to detect a nested JSONSchema, and thus would not refuse it. With the requirement of v1 CRDs around the corner in k8s 1.22, framework/constraint requires the newer controller-gen v0.5.0. This library version can output v1 CRD. The conversion logic was also in need of a library upgrade, with conversion-gen v0.21.0 now generating the necessary files. This PR _does not_ output a v1 CRD for Constraint Template. That will be left for a future PR. This PR brings the two binaries up-to-date. Note: I attempted to dockerize these two binaries. Controller-gen worked fine, but for some reason conversion-gen couldn't write to the filesystem. The output looked fine, but no dice. Rather than figure out why, I decided to punt on the dockeriziation effort. Signed-off-by: juliankatz <juliankatz@google.com>

sozercan · 2021-05-03T20:13:14Z

@julianKatz ready to merge?

julianKatz force-pushed the upgrade-controller-gen branch from 40c7936 to 97d09fe Compare April 23, 2021 00:25

julianKatz requested a review from maxsmythe April 23, 2021 00:25

julianKatz commented Apr 23, 2021

View reviewed changes

maxsmythe approved these changes Apr 23, 2021

View reviewed changes

julianKatz force-pushed the upgrade-controller-gen branch 2 times, most recently from 4c29f6c to 56ce6ad Compare April 27, 2021 20:07

julianKatz force-pushed the upgrade-controller-gen branch 2 times, most recently from 6f0e4bb to b7f544a Compare April 28, 2021 00:07

julianKatz changed the title ~~Dockerize and upgrade controller-gen to v0.5.0~~ Upgrade: controller-gen v0.5.0, conversion-gen v0.21.0 Apr 28, 2021

sozercan reviewed Apr 28, 2021

View reviewed changes

julianKatz force-pushed the upgrade-controller-gen branch from b7f544a to eedf931 Compare April 28, 2021 20:51

julianKatz force-pushed the upgrade-controller-gen branch from eedf931 to 0c5e494 Compare April 29, 2021 18:17

julianKatz force-pushed the upgrade-controller-gen branch from 0c5e494 to f37e4f8 Compare April 29, 2021 20:06

julianKatz force-pushed the upgrade-controller-gen branch 4 times, most recently from a4911cf to 197a4b3 Compare April 30, 2021 22:08

julianKatz changed the title ~~Upgrade: controller-gen v0.5.0, conversion-gen v0.21.0~~ Upgrade: controller-gen v0.5.0, conversion-gen v0.20.2 Apr 30, 2021

julianKatz mentioned this pull request Apr 30, 2021

Re-introduce make generate into CI #112

Open

maxsmythe approved these changes Apr 30, 2021

View reviewed changes

maxsmythe requested a review from sozercan April 30, 2021 22:41

sozercan approved these changes May 3, 2021

View reviewed changes

julianKatz force-pushed the upgrade-controller-gen branch from 197a4b3 to 0e7217e Compare May 3, 2021 20:09

julianKatz merged commit 3f51770 into open-policy-agent:master May 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade: controller-gen v0.5.0, conversion-gen v0.20.2 #111

Upgrade: controller-gen v0.5.0, conversion-gen v0.20.2 #111

julianKatz commented Apr 23, 2021 •

edited

Loading

julianKatz Apr 23, 2021

maxsmythe Apr 23, 2021

maxsmythe Apr 23, 2021

julianKatz Apr 27, 2021

maxsmythe left a comment

maxsmythe Apr 23, 2021

sozercan commented Apr 23, 2021 •

edited

Loading

maxsmythe commented Apr 23, 2021

maxsmythe commented Apr 24, 2021

maxsmythe commented Apr 24, 2021

sozercan commented Apr 26, 2021

codecov-commenter commented Apr 27, 2021 •

edited

Loading

sozercan Apr 28, 2021 •

edited

Loading

julianKatz Apr 28, 2021 •

edited

Loading

julianKatz Apr 29, 2021

sozercan Apr 29, 2021 •

edited

Loading

julianKatz Apr 30, 2021

sozercan Apr 28, 2021

maxsmythe Apr 28, 2021

julianKatz Apr 28, 2021

maxsmythe commented Apr 28, 2021

sozercan commented Apr 29, 2021

julianKatz commented Apr 29, 2021 •

edited

Loading

sozercan commented Apr 29, 2021

julianKatz commented Apr 30, 2021

julianKatz commented Apr 30, 2021

maxsmythe left a comment

maxsmythe Apr 30, 2021

julianKatz May 3, 2021

sozercan commented May 3, 2021

julianKatz commented May 3, 2021 •

edited

Loading

sozercan commented May 3, 2021

Upgrade: controller-gen v0.5.0, conversion-gen v0.20.2 #111

Upgrade: controller-gen v0.5.0, conversion-gen v0.20.2 #111

Conversation

julianKatz commented Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan commented Apr 23, 2021 • edited Loading

maxsmythe commented Apr 23, 2021

maxsmythe commented Apr 24, 2021

maxsmythe commented Apr 24, 2021

sozercan commented Apr 26, 2021

codecov-commenter commented Apr 27, 2021 • edited Loading

Codecov Report

sozercan Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

julianKatz Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxsmythe commented Apr 28, 2021

sozercan commented Apr 29, 2021

julianKatz commented Apr 29, 2021 • edited Loading

sozercan commented Apr 29, 2021

julianKatz commented Apr 30, 2021

julianKatz commented Apr 30, 2021

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan commented May 3, 2021

julianKatz commented May 3, 2021 • edited Loading

sozercan commented May 3, 2021

julianKatz commented Apr 23, 2021 •

edited

Loading

sozercan commented Apr 23, 2021 •

edited

Loading

codecov-commenter commented Apr 27, 2021 •

edited

Loading

sozercan Apr 28, 2021 •

edited

Loading

julianKatz Apr 28, 2021 •

edited

Loading

sozercan Apr 29, 2021 •

edited

Loading

julianKatz commented Apr 29, 2021 •

edited

Loading

julianKatz commented May 3, 2021 •

edited

Loading