Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate and remove Helm v2 support #1076

Closed
sozercan opened this issue Jan 20, 2021 · 6 comments · Fixed by #1179
Closed

Deprecate and remove Helm v2 support #1076

sozercan opened this issue Jan 20, 2021 · 6 comments · Fixed by #1179
Assignees
Labels
enhancement New feature or request

Comments

@sozercan
Copy link
Member

sozercan commented Jan 20, 2021

Describe the solution you'd like
Helm v2 is deprecated as of August 2020. We can deprecate and remove support for Helm v2 charts.

Helm v3 will still be supported.

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

Environment:

  • Gatekeeper version:
  • Kubernetes version: (use kubectl version):
@sozercan sozercan added the enhancement New feature or request label Jan 20, 2021
@sozercan sozercan changed the title Deprecate Helm v2 support Deprecate and remove Helm v2 support Jan 20, 2021
@sozercan
Copy link
Member Author

sozercan commented Feb 26, 2021

deprecate createNamespace since helm 3 includes --create-namespace flag: #1132 (comment)

@jdolce
Copy link
Contributor

jdolce commented Mar 3, 2021

Was going to start on this one but wanted some feedback on how to manage the following test.

@test "gatekeeper ns ignore label can be patched" {
  kubectl patch ns ${GATEKEEPER_NAMESPACE} --type=json -p='[{"op": "replace", "path": "/metadata/labels/admission.gatekeeper.sh~1ignore", "value": "ignore-label-test-passed"}]'
}

Ideally the --create-namespace flag would be used when installing the chart in the tests. However, the test above requires that the namespace has a admission.gatekeeper.sh/ignore annotation.

We could have the tests annotate the namespace on setup, we could change the test to use something other then the namespace, or we could remove it.

Thoughts?

@maxsmythe
Copy link
Contributor

That particular webhook is only called against namespaces, so the resource has to be a namespace.

If Helm isn't installing the admission.gatekeeper.sh/ignore label on a created namespace, that's an issue, as the gatekeeper webhook would be called against the gatekeeper pod, which is a circular dependency.

@sozercan
Copy link
Member Author

sozercan commented Mar 4, 2021

Maybe a post-install hook? It's not pretty but might work

@jdolce
Copy link
Contributor

jdolce commented Mar 4, 2021

Ok I understand this better now. I re-read the docs around --exempt-namespace and it's clear.

Unfortunately there is no way to manipulate a namespace when it is created with Helm with the --create-namespace flag. See this comment - helm/helm#3503 (comment)

I still think it is important to get ride of the Namespace yaml in the Helm chart as this is a Helm anti-pattern, but some guidance around installation will be needed.

Options:

  1. Explain that you need to have the namespace created with the following metadata before installing the chart. This is fine but has its limitations where people depend on Helm to create namespaces in their pipelines.
  2. Use a post-install helm hook to add the necessary metadata. Something like this https://gist.github.com/kvudata/12ba57ae1e7f01799aaa7f36350a9b2e. The biggest thing I don't like about this is that there isn't an official docker image with kubectl. So the chart would have a 3rd party dependency, which I am not a fan of. We could use this option as an example for people to add to their own chart if they wanted so it wasn't actually added to the chart itself.
  3. Is it safe to assume you always want to exempt the namespace gatekeeper is installed in? Could we make this more explicit by passing in the namespace with
        - name: MY_POD_NAMESPACE
          valueFrom:
            fieldRef:
              fieldPath: metadata.namespace

The idea here is that the gatekeeper namespace would be handled differently and the metadata on the Namespace wouldn't be required.

@jdolce
Copy link
Contributor

jdolce commented Mar 4, 2021

Just as another touch point, the Minio chart uses the kubectl post install hook approach for something similar - https://github.com/minio/charts/blob/master/minio/templates/post-install-prometheus-metrics-job.yaml#L101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants