Update to admissionregistration.k8s.io/v1 #1250

sozercan · 2021-04-15T02:50:18Z

Signed-off-by: Sertac Ozercan sozercan@gmail.com

What this PR does / why we need it:

Updates admissionregistration.k8s.io/v1beta1 to v1. This will change minimum required Kubernetes version to v1.16.
Updates controller-gen to v0.5.0.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #818

Special notes for your reviewer:

Upgrade test validates that a cluster with admissionregistration.k8s.io/v1beta1 can be upgraded to v1.

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

maxsmythe · 2021-04-16T22:42:47Z

config/webhook/manifests.yaml

- clientConfig:
-    caBundle: Cg==
+- admissionReviewVersions:
+  - v1


Are there any differences between the two admission review versions?

it's detailed here: https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.16.md#api-changes
doesn't sounds like any major differences, just required fields and api server support for receiving corresponding versions

maxsmythe

LGTM

ritazh · 2021-04-17T01:19:15Z

config/webhook/manifests.yaml

@@ -24,8 +26,11 @@ webhooks:
    - UPDATE
    resources:
    - namespaces
- clientConfig:
-    caBundle: Cg==


Looks like caBundle is removed here but they are persisted in the MutatingWebhookConfiguration in config/overlays/mutation_webhook/webhook.yaml

I think config/overlays/mutation_webhook/webhook.yaml is temporary location, and it's not generated. Once it's generated, it'll go to config/webhook/manifests.yaml (and caBundle doesn't get generated)

https://github.com/open-policy-agent/gatekeeper/pull/1250/files#diff-b51dd886772df2afc559857517988e0b2c597cc944583aa47891f6de293b4f21R50-R51

ah ok. should we manually update config/overlays/mutation_webhook/webhook.yaml for now?

I don't think it matters but, I updated to be same as it would have been generated.

Related bug: #817

Thanks for linking the issue @maxsmythe so this PR would fix that issue as a bonus.

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

codecov-commenter · 2021-04-19T19:17:49Z

Codecov Report

Merging #1250 (da9039d) into master (20a0e1a) will decrease coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1250      +/-   ##
==========================================
- Coverage   48.55%   48.49%   -0.07%     
==========================================
  Files          68       68              
  Lines        4879     4879              
==========================================
- Hits         2369     2366       -3     
- Misses       2159     2161       +2     
- Partials      351      352       +1

Flag	Coverage Δ
unittests	`48.49% <ø> (-0.07%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/webhook/mutation.go	`17.64% <ø> (ø)`
pkg/webhook/namespacelabel.go	`72.00% <ø> (ø)`
pkg/webhook/policy.go	`28.62% <ø> (ø)`
...onstrainttemplate/constrainttemplate_controller.go	`56.63% <0.00%> (-0.98%)`	⬇️

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 20a0e1a...da9039d. Read the comment docs.

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan · 2021-04-19T20:47:09Z

cert-controller PR: open-policy-agent/cert-controller#34

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

maxsmythe · 2021-04-20T21:26:28Z

still LGTM, if needed

maxsmythe · 2021-04-20T21:26:40Z

Though the tests look unhappy

sozercan · 2021-04-20T21:28:15Z

@maxsmythe failures are due to open-policy-agent/cert-controller#31 that includes controller-runtime v0.8 which includes breaking changes

maxsmythe · 2021-04-20T21:29:55Z

Major breaking changes? Or do we just need to tweak some things?

sozercan · 2021-04-20T23:12:51Z

Sounds like conversion function changes in apimachinery. I think this will have to wait until frameworks repo updates unless we want to revert controller-runtime changes in cert-controller

#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 {})

willbeason · 2021-04-27T15:18:20Z

We should explicitly set matchPolicy and sideEffects in our Validating/MutatingWebhookConfigurations to preserve behavior and be valid. In several files we aren't declaring these fields, which is problematic as the defaults have changed.

matchPolicy now defaults to "Equivalent" instead of "Exact". We aren't being explicit in our Validating/MutatingWebhookConfiguration, so this will cause a change in behavior. I don't know whether we intend to be the current default of "Exact". Note that if a user applies a different API Version of an object than the mutator is written for, "Exact" will ignore it but "Equivalent" will match it. However, the webhook would then needs to make sure it properly converts the object to the expected API Version.

sideEffects has changed and now needs to be explicitly set. "In admissionregistration.k8s.io/v1, sideEffects must be set to None or NoneOnDryRun."

I would have linked to a specific file as an example inline but I don't seem to have permission to do that?

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan · 2021-05-03T23:15:46Z

manifest_staging/deploy/gatekeeper.yaml

@@ -357,14 +368,15 @@ spec:
            created:
              type: boolean
          type: object
-  version: v1beta1
+      type: object
+  version: v1alpha1


@maxsmythe @julianKatz do we want version to be v1alpha1 or v1beta1?

I think it should be v1beta1 here https://github.com/open-policy-agent/frameworks/blob/3f51770b2910689616b1c78b7b5e75010cd899a0/constraint/config/crds/templates.gatekeeper.sh_constrainttemplates.yaml#L109

version needs to be equal to the first item in the versions list. It looks like it doesn't actually do anything except break if it doesn't match:

https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#customresourcedefinitionspec-v1beta1-apiextensions-k8s-io

Note: In apiextensions.k8s.io/v1beta1, there was a version field instead of versions. The version field is deprecated and optional, but if it is not empty, it must match the first item in the versions field.

Looks like version is deprecated. Do we want to keep it?

It's okay to remove but @julianKatz wasn't able to find a way for controller-gen to generate without version I believe

Yeah, it should go away when we switch to generating v1 CRDs (which don't have this field)

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan · 2021-05-03T23:29:20Z

@willbeason thanks! explicitly defined matchPolicy to be Exact to match previous behavior. I think we are already defining sideEffects explicitly.

@ritazh @maxsmythe thoughts on whether matchPolicy should be Equivalent or Exact?
https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#matching-requests-matchpolicy

ritazh · 2021-05-04T00:14:29Z

+1 Setting matchPolicy to Exact to match previous behavior. If we have enough people asking for Equivalent, then we could expose a helm parameter for users to set it.

maxsmythe · 2021-05-04T04:48:24Z

Exact match works. Because we match against / by default, either would be fine. Users who customize the config may be surprised.

sozercan · 2021-05-05T16:16:20Z

rebased with @shomron's changes to controller-runtime. @ritazh @maxsmythe still lgty?

maxsmythe

still LGTM!

ritazh

lgtm

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

update to admissionregistration.k8s.io/v1

6acc6e2

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan requested a review from a team April 15, 2021 02:50

update

f7aefb3

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

maxsmythe reviewed Apr 16, 2021

View reviewed changes

maxsmythe approved these changes Apr 16, 2021

View reviewed changes

ritazh reviewed Apr 17, 2021

View reviewed changes

sozercan requested a review from ritazh April 19, 2021 18:59

update mutation webhook yaml

7fc8a08

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan force-pushed the update-vwh-v1 branch from 996afca to 7fc8a08 Compare April 19, 2021 19:15

Merge branch 'master' into update-vwh-v1

371f81e

fix manifest

cc7d835

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

ritazh approved these changes Apr 19, 2021

View reviewed changes

update to cert-controller v0.2.0

7e8d73f

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan added 3 commits May 3, 2021 13:50

Merge branch 'master' into update-vwh-v1

1c711fe

frameworks 3f51770b2910

fd953c1

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

make manifests

2c1cf9a

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan commented May 3, 2021

View reviewed changes

explicitly define matchPolicy

3a14381

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

Merge branch 'master' into update-vwh-v1

b3a09b9

maxsmythe approved these changes May 6, 2021

View reviewed changes

ritazh approved these changes May 6, 2021

View reviewed changes

sozercan added 2 commits May 5, 2021 19:18

Merge branch 'master' into update-vwh-v1

9e08c07

make manifests

da9039d

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan merged commit 0fccf8b into open-policy-agent:master May 6, 2021

sozercan deleted the update-vwh-v1 branch May 6, 2021 03:19

sozercan mentioned this pull request May 6, 2021

Upgrade controller-gen to 0.5.0 #1285

Closed

sozercan added a commit to sozercan/gatekeeper that referenced this pull request May 7, 2021

revert open-policy-agent#1250 on charts

3c63aeb

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan mentioned this pull request May 7, 2021

revert #1250 on charts #1290

Merged

sozercan added a commit that referenced this pull request May 10, 2021

revert #1250 on charts (#1290)

dc33b0a

julianKatz pushed a commit to julianKatz/gatekeeper that referenced this pull request May 11, 2021

Update to admissionregistration.k8s.io/v1 (open-policy-agent#1250)

d00266e

julianKatz pushed a commit to julianKatz/gatekeeper that referenced this pull request May 11, 2021

revert open-policy-agent#1250 on charts (open-policy-agent#1290)

5e88f56

This was referenced May 26, 2021

Every deploy overwrites ValidatingWebhookConfiguration: webhooks[*].clientConfig.caBundle #817

Closed

deprek8ion warning #1332

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to admissionregistration.k8s.io/v1 #1250

Update to admissionregistration.k8s.io/v1 #1250

sozercan commented Apr 15, 2021 •

edited

Loading

maxsmythe Apr 16, 2021

sozercan Apr 16, 2021

maxsmythe left a comment

ritazh Apr 17, 2021 •

edited

Loading

sozercan Apr 19, 2021 •

edited

Loading

ritazh Apr 19, 2021 •

edited

Loading

sozercan Apr 19, 2021

maxsmythe Apr 19, 2021

ritazh Apr 19, 2021

codecov-commenter commented Apr 19, 2021 •

edited

Loading

sozercan commented Apr 19, 2021

maxsmythe commented Apr 20, 2021

maxsmythe commented Apr 20, 2021

sozercan commented Apr 20, 2021

maxsmythe commented Apr 20, 2021

sozercan commented Apr 20, 2021 •

edited

Loading

willbeason commented Apr 27, 2021 •

edited

Loading

sozercan May 3, 2021

ritazh May 4, 2021

maxsmythe May 4, 2021 •

edited

Loading

ritazh May 4, 2021

sozercan May 4, 2021 •

edited

Loading

maxsmythe May 4, 2021

sozercan commented May 3, 2021 •

edited

Loading

ritazh commented May 4, 2021

maxsmythe commented May 4, 2021

sozercan commented May 5, 2021

maxsmythe left a comment

ritazh left a comment

Update to admissionregistration.k8s.io/v1 #1250

Update to admissionregistration.k8s.io/v1 #1250

Conversation

sozercan commented Apr 15, 2021 • edited Loading

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

ritazh Apr 17, 2021 • edited Loading

Choose a reason for hiding this comment

sozercan Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

ritazh Apr 19, 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

codecov-commenter commented Apr 19, 2021 • edited Loading

Codecov Report

sozercan commented Apr 19, 2021

maxsmythe commented Apr 20, 2021

maxsmythe commented Apr 20, 2021

sozercan commented Apr 20, 2021

maxsmythe commented Apr 20, 2021

sozercan commented Apr 20, 2021 • edited Loading

willbeason commented Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxsmythe May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan commented May 3, 2021 • edited Loading

ritazh commented May 4, 2021

maxsmythe commented May 4, 2021

sozercan commented May 5, 2021

maxsmythe left a comment

Choose a reason for hiding this comment

ritazh left a comment

Choose a reason for hiding this comment

sozercan commented Apr 15, 2021 •

edited

Loading

ritazh Apr 17, 2021 •

edited

Loading

sozercan Apr 19, 2021 •

edited

Loading

ritazh Apr 19, 2021 •

edited

Loading

codecov-commenter commented Apr 19, 2021 •

edited

Loading

sozercan commented Apr 20, 2021 •

edited

Loading

willbeason commented Apr 27, 2021 •

edited

Loading

maxsmythe May 4, 2021 •

edited

Loading

sozercan May 4, 2021 •

edited

Loading

sozercan commented May 3, 2021 •

edited

Loading