Dynamically change the API version of the PDB in Helm Chart #1502

tenzen-y · 2021-08-17T06:59:50Z

Signed-off-by: tenzen-y yuki.iwai.tz@gmail.com

What this PR does / why we need it:
PodDisruptionBudget is deprecated in v1.21+, unavailable in v1.25+.
So, I would like to upgrade policy/v1beta1 PDB to policy/v1.

The PodDisruptionBudget API has been promoted to policy/v1 with no schema changes. The only functional change is that an empty selector ({}) written to a policy/v1 PodDisruptionBudget now selects all pods in the namespace. The behavior of the policy/v1beta1 API remains unchanged. The policy/v1beta1 PodDisruptionBudget API is deprecated and will no longer be served in 1.25+. (#99290, @mortent) [SIG API Machinery, Apps, Auth, Autoscaling, CLI, Cloud Provider, Cluster Lifecycle, Instrumentation, Scheduling and Testing]

https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.21.md#api-change-1

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 #

Special notes for your reviewer:

Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com>

tenzen-y · 2021-08-17T07:41:17Z

@thelinuxfoundation I consent.

ritazh · 2021-08-17T15:42:08Z

@tenzen-y Thank you for the PR! Since PodDisruptionBudget policy/v1 was introduced in kubernetes v1.21, we cannot move to it until v1.20 reaches end of life in kubernetes and gatekeeper. Since policy/v1beta1 PodDisruptionBudget will continue to work until v1.25 we can revisit this when v1.20 release reaches End of Life: 2022-02-28: https://kubernetes.io/releases/ Until then, policy/v1beta1 PodDisruptionBudget should still work. For Helm, the change could be conditioned based on the kubenetes version.

tenzen-y · 2021-08-17T17:35:51Z

@ritazh It would make sense to reconsider the upgrade to policy/v1 PodDisruptionBudge on 2022-02-28.

For Helm, the change could be conditioned based on the kubenetes version.

I would like to work on dynamically changing the API version of the PDB in Helm Chart.

sozercan · 2021-08-17T18:45:20Z

Today, Gatekeeper's minimum supported k8s version is v1.16 (due to v1 CRDs), and this will substantially increase the minimum requirement.

Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com>

ritazh · 2021-08-17T20:16:47Z

Today, Gatekeeper's minimum supported k8s version is v1.16 (due to v1 CRDs), and this will substantially increase the minimum requirement.

Should we change the minimum k8s version for gatekeeper to at least v1.18 since k8s stopped supporting 1.18 now.

maxsmythe · 2021-08-18T00:36:08Z

cmd/build/helmify/kustomize-for-helm.yaml

@@ -164,6 +164,7 @@ webhooks:
  timeoutSeconds: HELMSUBST_VALIDATING_WEBHOOK_TIMEOUT
  failurePolicy: HELMSUBST_VALIDATING_WEBHOOK_CHECK_IGNORE_FAILURE_POLICY
 ---
+HELMSUBSET_PDB_POLICY_GROUP_VERSION: ""


nit: HELMSUBST

@maxsmythe thanks!

maxsmythe · 2021-08-18T00:38:20Z

cmd/build/helmify/replacements.go

@@ -66,4 +66,10 @@ var replacements = map[string]string{
        {{- range .Values.controllerManager.exemptNamespaces}}
        - --exempt-namespace={{ . }}
        {{- end }}`,
+	`HELMSUBSET_PDB_POLICY_GROUP_VERSION: ""


This could break if the ordering of fields in the Kustomize output is not maintained.

It might be better to modify https://github.com/open-policy-agent/gatekeeper/blob/master/cmd/build/helmify/main.go to directly detect these and make the replacement?

@maxsmythe I followed the documentation below and made some changes outside of main.go, but there is certainly a danger that this implementation will not work.

please edit kustomization.yaml, kustomize-for-helm.yaml and replacements.go under that directory and then run make manifests.

https://github.com/open-policy-agent/gatekeeper/blob/e00262b76acc82f5dc2a7e3a371d57894e7776b3/charts/gatekeeper/README.md#contributing-changes

I will modify main.go to directly detect these. Thanks.

Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com>

codecov-commenter · 2021-08-18T03:38:46Z

Codecov Report

Merging #1502 (ae8cc57) into master (a1b50a0) will increase coverage by 0.10%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1502      +/-   ##
==========================================
+ Coverage   52.25%   52.36%   +0.10%     
==========================================
  Files          82       82              
  Lines        7377     7377              
==========================================
+ Hits         3855     3863       +8     
+ Misses       3163     3158       -5     
+ Partials      359      356       -3

Flag	Coverage Δ
unittests	`52.36% <ø> (+0.10%)`	⬆️

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

Impacted Files	Coverage Δ
...onstrainttemplate/constrainttemplate_controller.go	`54.54% <0.00%> (+1.91%)`	⬆️

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 a1b50a0...ae8cc57. Read the comment docs.

sozercan

LGTM

maxsmythe

LGTM, thanks for making the changes!

sathieu · 2021-09-07T08:32:10Z

@tenzen-y @sozercan Unfortunately, this broke gatekeeper installation with ArgoCD.

The reason is ArgoCD is using helm template command, passing all supported APIVersions, but Helm pre-fill this with the supported versions of the Kubernetes used during helm build (i.e. k8s 1.21).

There are a few related bug reports like in argoproj/argo-cd#6351, argoproj/argo-cd#3594.

I can submit a PR. One of the following:

Revert this MR completely, and wait for k8s v1.25 release to re-introduce it
Invert the logic: {{- if .Capabilities.APIVersions.Has "policy/v1beta1" }} (will still raise deprecation warnings)
Match on Kubernetes version >= 1.21 (no deprecation warnings)
Match on exact resource type (that is not prefilled by Helm): {{- if .Capabilities.APIVersions.Has "policy/v1/PodDisruptionBudget" }})

WDYT?

The behavior of helm template should also be fixed when --api-version is passed.

sathieu · 2021-09-07T11:42:48Z

I have proposed workaround number 4 in #1533

tenzen-y · 2021-09-07T12:27:55Z

@sabre1041
Thank you for reporting about this.

I have proposed workaround number 4 in #1533

It makes sense. I think that number 4 is a good workaround.

change pdb api version

84bea4a

Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com>

tenzen-y changed the title ~~Upgrade policy/v1beta1 PodDisruptionBudget to policy/v1~~ Dynamically change the API version of the PDB in Helm Chart Aug 17, 2021

dynamic change api version of PDB in Helm

65ecd98

Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com>

ritazh approved these changes Aug 17, 2021

View reviewed changes

maxsmythe reviewed Aug 18, 2021

View reviewed changes

tenzen-y added 2 commits August 18, 2021 10:33

fix typo

b7164ae

Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com>

directly detect PDB by helmify

4d46ffa

Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com>

tenzen-y force-pushed the change-pdb-api-version branch from b026e73 to 4d46ffa Compare August 18, 2021 02:19

tenzen-y requested a review from maxsmythe August 18, 2021 02:20

sozercan approved these changes Aug 18, 2021

View reviewed changes

maxsmythe approved these changes Aug 19, 2021

View reviewed changes

sozercan and others added 2 commits August 18, 2021 18:01

Merge branch 'master' into change-pdb-api-version

7dbacee

Merge branch 'master' into change-pdb-api-version

ae8cc57

sozercan merged commit 0238780 into open-policy-agent:master Aug 19, 2021

tenzen-y deleted the change-pdb-api-version branch August 19, 2021 06:40

sathieu mentioned this pull request Sep 7, 2021

Workaround helm template bug when testing for APIVersions #1533

Merged

Ragatzino mentioned this pull request Sep 7, 2021

Gatekeeper 3.6.0 installation failed on Kubernetes v1.20 due to PodDisruptionBudget API version. #1534

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamically change the API version of the PDB in Helm Chart #1502

Dynamically change the API version of the PDB in Helm Chart #1502

tenzen-y commented Aug 17, 2021

tenzen-y commented Aug 17, 2021

ritazh commented Aug 17, 2021 •

edited

Loading

tenzen-y commented Aug 17, 2021

sozercan commented Aug 17, 2021 •

edited

Loading

ritazh commented Aug 17, 2021

maxsmythe Aug 18, 2021

tenzen-y Aug 18, 2021

maxsmythe Aug 18, 2021

tenzen-y Aug 18, 2021

codecov-commenter commented Aug 18, 2021 •

edited

Loading

sozercan left a comment

maxsmythe left a comment

sathieu commented Sep 7, 2021

sathieu commented Sep 7, 2021

tenzen-y commented Sep 7, 2021

Dynamically change the API version of the PDB in Helm Chart #1502

Dynamically change the API version of the PDB in Helm Chart #1502

Conversation

tenzen-y commented Aug 17, 2021

tenzen-y commented Aug 17, 2021

ritazh commented Aug 17, 2021 • edited Loading

tenzen-y commented Aug 17, 2021

sozercan commented Aug 17, 2021 • edited Loading

ritazh commented Aug 17, 2021

maxsmythe Aug 18, 2021

Choose a reason for hiding this comment

tenzen-y Aug 18, 2021

Choose a reason for hiding this comment

maxsmythe Aug 18, 2021

Choose a reason for hiding this comment

tenzen-y Aug 18, 2021

Choose a reason for hiding this comment

codecov-commenter commented Aug 18, 2021 • edited Loading

Codecov Report

sozercan left a comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

sathieu commented Sep 7, 2021

sathieu commented Sep 7, 2021

tenzen-y commented Sep 7, 2021

ritazh commented Aug 17, 2021 •

edited

Loading

sozercan commented Aug 17, 2021 •

edited

Loading

codecov-commenter commented Aug 18, 2021 •

edited

Loading