Support --dry-run flag when installing crd

[jungopro]

Output of helm version:

version.BuildInfo{Version:"v3.0.2", GitCommit:"19e47ee3283ae98139d98460de796c1be1e3975f", GitTreeState:"clean", GoVersion:"go1.13.5"}

Output of kubectl version:

Client Version: version.Info{Major:"1", Minor:"17", GitVersion:"v1.17.0", GitCommit:"70132b0f130acc0bed193d9ba59dd186f0e634cf", GitTreeState:"clean", BuildDate:"2019-12-13T11:52:32Z", GoVersion:"go1.13.4", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"15", GitVersion:"v1.15.7", GitCommit:"6c143d35bb11d74970e7bc0b6c45b6bfdffc0bd4", GitTreeState:"clean", BuildDate:"2019-12-11T12:34:17Z", GoVersion:"go1.12.12", Compiler:"gc", Platform:"linux/amd64"}

Cloud Provider/Platform (AKS, GKE, Minikube etc.):
AKS v1.15.7

Hello

What happened?

Trying to run a dry-run fails but actual install succeeds. I'm unsure if it's a problem with helm as a tool or the specific chart

$ helm upgrade -i prometheus-operator stable/prometheus-operator --dry-run
Release "prometheus-operator" does not exist. Installing it now.
manifest_sorter.go:175: info: skipping unknown hook: "crd-install"
manifest_sorter.go:175: info: skipping unknown hook: "crd-install"
manifest_sorter.go:175: info: skipping unknown hook: "crd-install"
manifest_sorter.go:175: info: skipping unknown hook: "crd-install"
manifest_sorter.go:175: info: skipping unknown hook: "crd-install"
Error: unable to build kubernetes objects from release manifest: [unable to recognize "": no matches for kind "Alertmanager" in version "monitoring.coreos.com/v1", unable to recognize "": no matches for kind "Prometheus" in version "monitoring.coreos.com/v1", unable to recognize "": no matches for kind "PrometheusRule" in version "monitoring.coreos.com/v1", unable to recognize "": no matches for kind "ServiceMonitor" in version "monitoring.coreos.com/v1"]

$ helm upgrade -i prometheus-operator stable/prometheus-operator          
Release "prometheus-operator" does not exist. Installing it now.
manifest_sorter.go:175: info: skipping unknown hook: "crd-install"
manifest_sorter.go:175: info: skipping unknown hook: "crd-install"
manifest_sorter.go:175: info: skipping unknown hook: "crd-install"
manifest_sorter.go:175: info: skipping unknown hook: "crd-install"
manifest_sorter.go:175: info: skipping unknown hook: "crd-install"
NAME: prometheus-operator
LAST DEPLOYED: Wed Jan 22 07:59:14 2020
NAMESPACE: default
STATUS: deployed
REVISION: 1
NOTES:
The Prometheus Operator has been installed. Check its status by running:
  kubectl --namespace default get pods -l "release=prometheus-operator"

Visit https://github.com/coreos/prometheus-operator for instructions on how
to create & configure Alertmanager and Prometheus instances using the Operator.

Suggested proposal (from @technosophos in #7449 (comment)):
The most promising proposal (which nobody has yet implemented) was to write a client-side validation for CRs, which would load any CRDs in the crds directory into the local validator and then specifically validate those. While it sounds easy, that's actually a fairly major work-around, since the existing Kubernetes-provided libraries don't really provide us with a way of doing this. PRs are definitely welcome if anyone can think of a clean way of doing this.

[hickeyma]

@jungopro This looks like a chart that is using crd-install hooks which are no longer supported in Helm 3. Helm 3 will ignore the hooks and log the output. The chart would need to be updated to use the new CRD support. Refer to docs on CRDs for more details.

[Xartos]

At the time of writing prometheus-operator chart includes the new format (the crds/ folder) and it still doesn't work, even when removing the templates using the crd-install hooks.
For what I've found, it seems like all charts that installs CRDs and uses them in the same chart can only install and can't do install --dry-run.

$ helm3 version
version.BuildInfo{Version:"v3.0.3", GitCommit:"ac925eb7279f4a6955df663a0128044a8a6b7593", GitTreeState:"clean", GoVersion:"go1.13.6"}

[hickeyma]

@Xartos Thanks for raising this issue again.

I have confirmed that it fails on --dry-run from the master branch. I am going to set as a bug but would like to get feedback from people who worked on the CRD capability.

@technosophos Is this a bug or expected behaviour?

[technosophos]

This was a documented behavior.

At issue is the fact that during a dry run, we do not install the CRDs, but we do use the Kubernetes validation against the output of the chart. So any CR that uses a CRD that is installed by the chart will fail validation during dry run.

This is one of those edge cases that surfaced during our protracted discussions of CRDs, and their strange place in the Kubernetes world of objects. The purpose of Dry Run is (at least in my mind) to validate that the output of the chart will actually work if sent to the server. But CRDs are actually a modification of the server's behavior. We can't install the CRD on a dry run, so the discovery client will not know about that CR, and validation will fail. But the workaround seems to be ignoring the CR errors, which means one could get spurious validation where the actual server would not have validated it. Effectively, that would invalidate what dry run is supposed to be doing.

The most promising proposal (which nobody has yet implemented) was to write a client-side validation for CRs, which would load any CRDs in the crds directory into the local validator and then specifically validate those. While it sounds easy, that's actually a fairly major work-around, since the existing Kubernetes-provided libraries don't really provide us with a way of doing this. PRs are definitely welcome if anyone can think of a clean way of doing this.

Current work-arounds:

• Use helm template instead of dry run
• Don't reference CRs in the same chart that has the CRDs
• Install the CRD separately before running the dry run

[hickeyma]

@technosophos Thanks for the feedback and the detailed response.

Just wondering:

• Where is this documented? I don't see any mention of it in the CRD doc
• Is there a proposal/issue already open that means this issue can be duplicated?

[hickeyma]

The CRD implementation is described in detail in PR #6243, which implements CRDs in Helm 3. Maybe it needs to be added to the doc and use leave this issue open?

[technosophos]

I guess it could make sense to put that whole thing in documentation.

[hickeyma]

@jungopro @technosophos Opened a doc PR to describe that --dry-run flag does not work when CRDs in a chart.

Going to set this to a feature (as this is expected behaviour) and change the title. I will also add @technosophos proposal in #7449 (comment) to the description.

Thanks @technosophos for the informed feedback and @jungopro for raising this.

[bacongobbler]

looks like this can be closed now that helm/helm-www#545 has been merged for some time.