feat: deprecate Helm 2 #1179

jdolce · 2021-03-15T18:17:49Z

Signed-off-by: Julian Dolce jdolce@qnx.com

What this PR does / why we need it:
Deprecates Helm 2 chart

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 #1076

Special notes for your reviewer:

sozercan · 2021-03-15T18:26:46Z

manifest_staging/charts/gatekeeper/values.yaml

+  labelNamespace:
+    enabled: false
+    image:
+      repository: bskim45/helm-kubectl-jq


is this only used for kubectl? can we re-use line/kubectl-kustomize image we use for kustomize?

yep for sure will look at using that.

codecov-io · 2021-03-19T13:07:39Z

Codecov Report

Merging #1179 (00ba3a4) into master (5b0472a) will decrease coverage by 0.24%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1179      +/-   ##
==========================================
- Coverage   50.14%   49.89%   -0.25%     
==========================================
  Files          64       64              
  Lines        4465     4465              
==========================================
- Hits         2239     2228      -11     
- Misses       1922     1928       +6     
- Partials      304      309       +5

Flag	Coverage Δ
unittests	`49.89% <ø> (-0.25%)`	⬇️

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

Impacted Files	Coverage Δ
...onstrainttemplate/constrainttemplate_controller.go	`53.07% <0.00%> (-3.56%)`	⬇️

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 5b0472a...00ba3a4. Read the comment docs.

sozercan · 2021-03-23T23:16:00Z

Makefile

+		./.staging/helm/linux-amd64/helm install gatekeeper gatekeeper/gatekeeper --version ${BASE_RELEASE} --debug --wait \
+		 --set emitAdmissionEvents=true \
+		 --set emitAuditEvents=true \
+		 --set customResourceDefinitions.create=false;\


customResourceDefinition.create should be removed now?

The reason why I left this was because this is the currently released chart being installed before we try and do an upgrade. So customResourceDefinition.create is still in that chart.

sozercan · 2021-03-23T23:20:21Z

cmd/build/helmify/static/README.md

+$ helm install -n gatekeeper-system [RELEASE_NAME] gatekeeper/gatekeeper
+
+# Helm install and create namespace
+$ helm install -n gatekeeper-system [RELEASE_NAME] gatekeeper/gatekeeper --create-namespace --set postInstall.labelNamespace.enabled=true


@ritazh @maxsmythe are we okay with this behavior change? this should be documented in release notes.

What is the behavior change? the --create-namespace flag?

TBH, I don't think of Helm as part of Gatekeeper's backwards compatibility contract b/c it's an installation method, not an interface, though reasonable people can disagree here. More importantly, it may not be possible to maintain backwards compatibility as it seems like some if these changes are forced by upstream changes to Helm itself?

With that in mind, I would do whatever the community standard is for Helm charts when they need to change.

This postInstall hook is definitely not obvious. +1 on adding it to release notes and all the docs.
Can postInstall.labelNamespace.enabled be default to true so users don't need to do this every time? And this would ensure existing namespace also gets the admission.gatekeeper.sh/ignore=no-self-managing label.

The behaviour change here is that the Helm chart no longer creates and manages the Namespace. Helm will create it if you ask it by setting the --create-namespace flag, but it's not managed by the chart itself. For example if you uninstall the chart the Namespace will remain.

The challenge with this chart is that we want the admission.gatekeeper.sh/ignore label on the Namespace Gatekeeper is installed in, which is what the postInstall hook stuff does.

As an alternative to this could we include a Config in the chart and set a reasonable default to the Namespace, and allow callers to override it? Based on the docs it seems like this is another alternative to the
--exempt-namespace flag. Also happy to set postInstall.labelNamespace.enabled to true by default, and remove the post-upgrade hook so that it only happens on first time installs. Because that labeling only has to happen one time I would think.

happy to set postInstall.labelNamespace.enabled to true by default, and remove the post-upgrade hook so that it only happens on first time installs. Because that labeling only has to happen one time I would think.

+1

sozercan · 2021-03-23T23:24:27Z

cmd/build/helmify/static/templates/namespace-post-install.yaml

+    name: gatekeeper-update-namespace
+    namespace: {{ .Release.Namespace | quote }}
+
+{{- end }}


nit: newline

ritazh · 2021-03-24T01:45:04Z

cmd/build/helmify/static/README.md

+## Exempting Namespace
+
+The Helm chart automatically set the Gatekeeper flag `--exempt-namespace={{ .Release.Namespace }}` in order to exempt the namespace where the chart is installed. In order for this to work correctly the namespace must have the `admission.gatekeeper.sh/ignore` label.
+This can be done by setting up the namespace with the label before installing the chart, or setting `postInstall.labelNamespace.enabled: true`, which will add the label to the release namespace during the post install hook.


why does it need the label before installing the chart?

ritazh · 2021-03-24T01:47:08Z

cmd/build/helmify/static/templates/namespace-post-install.yaml

+apiVersion: batch/v1
+kind: Job
+metadata:
+  name: gatekeeper-update-namespace


Suggested change

name: gatekeeper-update-namespace

name: gatekeeper-update-namespace-label

ritazh · 2021-03-24T02:14:37Z

I kicked off upgrade / [Helm] Upgrade test (3.4.2) job again. let's make sure that e2e passes without any issues.

jdolce · 2021-03-24T18:02:54Z

I kicked off upgrade / [Helm] Upgrade test (3.4.2) job again. let's make sure that e2e passes without any issues.

I think there is an issue with the tests where the tests set enforcementAction: warn but that is not a valid value in gatekeeper 3.1.1 which is the base version of the upgrade path.

Error from server (Could not find the provided enforcementAction value within the supported list [deny dryrun]): error when applying patch:
...

jdolce · 2021-03-29T09:56:45Z

There is an issue with upgrading that makes it difficult if not impossible to upgrade from older versions.

helm install gatekeeper gatekeeper/gatekeeper --version 3.23 ....

This command will install and manage the Helm chart in the default namespace but all of the resources are installed to the gatekeeper-system namespace.

When you upgrade with the changes here we use the following command to replace the resources in gatekeeper-system namespace.

helm upgrade -n gatekeeper-system gatekeeper manifest_staging/charts/gatekeeper

The problem is that since Helm has installed the chart into the default namespace, it can't find the chart in the gatekeeper-namespace to upgrade.

The only way to make this work would be to initially install the chart with createNamespace=false, but that is not in a released version of the chart yet. This does also mean that there is a real potential for current users to not have a valid upgrade path with this chart.

ritazh · 2021-03-30T06:01:06Z

@jdolce Thanks for raising the issue! Sounds like we need a way to derive gatekeeper.namespace to support cases where the helm release namespace and the namespace that contains all the resources are different.

Maybe we need a helper function in cmd/build/helmify/static/templates/_helpers.tpl that defaults gatekeeper.namespace to gatekeeper-system when Release.Namespace is default, otherwise, use Release.Namespace :

{{- define "gatekeeper.namespace" -}}
{{- if .Release.Namespace default }}
{{- printf "gatekeeper-system" }}
{{- else }}
{{- .Release.Namespace }}
{{- end }}
{{- end -}}

This would ensure helm releases will continue to be in the same namespace users specify with -namespace and the resource's namespace will be gatekeeper.namespace. WDYT?

jdolce · 2021-03-30T11:21:19Z

@ritazh that might work, but I am concerned that it might be confusing as it is not expected Helm behaviour. Someone could have installed the chart into another namespace that isn't default or gatekeeper-system and they would run into the same problem.

I feel like this is kind of stuck in a bad spot where the current behaviour doesn't allow us to follow Helm best practices without some level of breakage. The changes in 3.4 help with this as it allows you to install it correctly, but that won't help folks that have installed on an older version.

My vote would be to break things with this change as we are deprecating something, and it will be easier to call out this behaviour. This will get us on a good track going forward. Doing so will require users to delete the old chart first to re-install it in the gatekeeper-system namespace. Or you could install it in a different namespace, like gatekeeper and run both for a brief moment, and delete the old one.

If we don't want to break anything, I think we can potentially come up with a solution like what Rita mentions above.

ritazh · 2021-03-31T06:06:47Z

@jdolce I have added this to this week's community call agenda.

ritazh · 2021-03-31T21:21:48Z

Per community call:

We want to deprecate support for when the helm release namespace and GK namespace are different
Need to test how existing CT and constraints will be persisted during helm delete and helm install
Side by side upgrade is not supported

jdolce · 2021-04-01T00:45:35Z

Per community call:

We want to deprecate support for when the helm release namespace and GK namespace are different
Need to test how existing CT and constraints will be persisted during helm delete and helm install
Side by side upgrade is not supported

Just tested this and after uninstalling the helm chart the CRD's remained along with the constraints.

maxsmythe · 2021-04-02T02:01:17Z

Sounds like the uninstall/reinstall path is viable then?

jdolce · 2021-04-02T16:21:04Z

@ritazh @maxsmythe @sozercan I came up with a migration script that seems to work well, and is being used in the upgrade tests. It prevents one from uninstalling the current chart and deleting any resources. There wasn't a clear spot to put this so it's just at the root of the repo. Let me know if there is a better place and I can move it.

sozercan · 2021-04-02T20:20:00Z

manifest_staging/charts/gatekeeper/README.md

+```
+
+Option 2:  
+Run the `helm_migrate.sh` script before installing the 3.4.0 or greater chart. This will remove the Helm secret for the original release, while keeping all of the resources. It then udpates the annotations of the resources so that the new chart can import and manage them.


Suggested change

Run the `helm_migrate.sh` script before installing the 3.4.0 or greater chart. This will remove the Helm secret for the original release, while keeping all of the resources. It then udpates the annotations of the resources so that the new chart can import and manage them.

Run the `helm_migrate.sh` script before installing the 3.4.0 or greater chart. This will remove the Helm secret for the original release, while keeping all of the resources. It then updates the annotations of the resources so that the new chart can import and manage them.

sozercan · 2021-04-02T20:24:38Z

helm_migrate.sh

+
+kubectl annotate --overwrite -n gatekeeper-system deployment gatekeeper-controller-manager meta.helm.sh/release-name=${RELEASE_NAME}
+kubectl annotate --overwrite -n gatekeeper-system deployment gatekeeper-controller-manager meta.helm.sh/release-namespace=${NEW_NAMESPACE}
+


is this missing pdb?

Error: rendered manifests contain a resource that already exists. Unable to continue with install: PodDisruptionBudget "gatekeeper-controller-manager" in namespace "gatekeeper-system" exists and cannot be imported into the current release: invalid ownership metadata; annotation validation error: key "meta.helm.sh/release-namespace" must equal "gatekeeper-system": current value is "default"

I added it. Didn't catch it the first time because it's not in the 3.2.3 Helm chart, which is the base revision for the upgrade.

sozercan · 2021-04-02T21:58:31Z

cmd/build/helmify/static/README.md

+```console
+$ helm_migrate.sh
+$ helm install -n gatekeeper-system gatekeeper gatekeeper/gatekeeper
+```


I am not seeing the helm release after doing helm ls here.

You would need helm ls -n gatekeeper-system since the chart is installed in that namespace.
You can also see the secret that Helm creates to manage the release kubectl -n gatekeeper-system get secrets -l release=gatekeeper

Signed-off-by: Julian Dolce <jdolce@qnx.com>

sozercan

Thanks! LGTM

maxsmythe

LGTM

sozercan reviewed Mar 15, 2021

View reviewed changes

jdolce force-pushed the helm3 branch from ef612b6 to b1fe648 Compare March 15, 2021 18:26

jdolce marked this pull request as draft March 15, 2021 18:32

jdolce force-pushed the helm3 branch 5 times, most recently from e6f19d4 to 575da07 Compare March 19, 2021 13:04

jdolce force-pushed the helm3 branch 3 times, most recently from 00ba3a4 to eb23f69 Compare March 19, 2021 13:54

jdolce marked this pull request as ready for review March 19, 2021 14:30

ritazh added this to the v3.4.0 milestone Mar 23, 2021

sozercan reviewed Mar 23, 2021

View reviewed changes

ritazh reviewed Mar 24, 2021

View reviewed changes

jdolce force-pushed the helm3 branch 3 times, most recently from c7bd46f to bbc8fe6 Compare March 26, 2021 19:29

jdolce force-pushed the helm3 branch 5 times, most recently from f893048 to 9085602 Compare April 2, 2021 16:08

sozercan reviewed Apr 2, 2021

View reviewed changes

jdolce force-pushed the helm3 branch 3 times, most recently from 5bd3d52 to 6421834 Compare April 2, 2021 21:04

sozercan reviewed Apr 2, 2021

View reviewed changes

feat: deprecate Helm 2

1f2fe86

Signed-off-by: Julian Dolce <jdolce@qnx.com>

jdolce force-pushed the helm3 branch from 6421834 to 1f2fe86 Compare April 6, 2021 10:19

sozercan approved these changes Apr 6, 2021

View reviewed changes

sozercan requested review from maxsmythe and ritazh April 6, 2021 22:08

maxsmythe approved these changes Apr 6, 2021

View reviewed changes

Merge branch 'master' into helm3

a73fdd2

ritazh approved these changes Apr 7, 2021

View reviewed changes

ritazh merged commit a6b2d64 into open-policy-agent:master Apr 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: deprecate Helm 2 #1179

feat: deprecate Helm 2 #1179

jdolce commented Mar 15, 2021

sozercan Mar 15, 2021

jdolce Mar 15, 2021

codecov-io commented Mar 19, 2021 •

edited

Loading

sozercan Mar 23, 2021

jdolce Mar 24, 2021

sozercan Mar 23, 2021

maxsmythe Mar 24, 2021

ritazh Mar 24, 2021 •

edited

Loading

jdolce Mar 24, 2021

ritazh Mar 25, 2021

sozercan Mar 23, 2021

ritazh Mar 24, 2021

ritazh Mar 24, 2021

ritazh commented Mar 24, 2021 •

edited

Loading

jdolce commented Mar 24, 2021

jdolce commented Mar 29, 2021

ritazh commented Mar 30, 2021

jdolce commented Mar 30, 2021

ritazh commented Mar 31, 2021

ritazh commented Mar 31, 2021

jdolce commented Apr 1, 2021

maxsmythe commented Apr 2, 2021

jdolce commented Apr 2, 2021

sozercan Apr 2, 2021

sozercan Apr 2, 2021 •

edited

Loading

jdolce Apr 2, 2021

sozercan Apr 2, 2021

jdolce Apr 2, 2021

sozercan left a comment

maxsmythe left a comment

	name: gatekeeper-update-namespace
	name: gatekeeper-update-namespace-label

	Run the `helm_migrate.sh` script before installing the 3.4.0 or greater chart. This will remove the Helm secret for the original release, while keeping all of the resources. It then udpates the annotations of the resources so that the new chart can import and manage them.
	Run the `helm_migrate.sh` script before installing the 3.4.0 or greater chart. This will remove the Helm secret for the original release, while keeping all of the resources. It then updates the annotations of the resources so that the new chart can import and manage them.


		kubectl annotate --overwrite -n gatekeeper-system deployment gatekeeper-controller-manager meta.helm.sh/release-name=${RELEASE_NAME}
		kubectl annotate --overwrite -n gatekeeper-system deployment gatekeeper-controller-manager meta.helm.sh/release-namespace=${NEW_NAMESPACE}

feat: deprecate Helm 2 #1179

feat: deprecate Helm 2 #1179

Conversation

jdolce commented Mar 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 19, 2021 • edited Loading

Codecov Report

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

ritazh Mar 24, 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

Choose a reason for hiding this comment

ritazh commented Mar 24, 2021 • edited Loading

jdolce commented Mar 24, 2021

jdolce commented Mar 29, 2021

ritazh commented Mar 30, 2021

jdolce commented Mar 30, 2021

ritazh commented Mar 31, 2021

ritazh commented Mar 31, 2021

jdolce commented Apr 1, 2021

maxsmythe commented Apr 2, 2021

jdolce commented Apr 2, 2021

Choose a reason for hiding this comment

sozercan Apr 2, 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

sozercan left a comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 19, 2021 •

edited

Loading

ritazh Mar 24, 2021 •

edited

Loading

ritazh commented Mar 24, 2021 •

edited

Loading

sozercan Apr 2, 2021 •

edited

Loading