terraform-provider-helm Do not update helm_release state if chart download error

Description

Do not update helm_release state if chart fails to download (server down, bad chart path, bad chart version).

After feedback on #512 this is attempt 2 at fixing this. The feedback there suggested that this was supposed to be caught during Terraform's plan phase instead. Based on that, I narrowed the error down to the "return nil" line in resource_release.go, which is swallowing an error when the chart fails to download. This PR makes it return the error. Note that this error occurs before any of the code of #464, so that PR does NOT fix this bug.

Unfortunately, I'm still not quite sure if this is the right fix. This "return nil" was intentionally introduced in https://github.com/terraform-providers/terraform-provider-helm/pull/161/files#r255350818 to handle the edge case of the chart not being available at plan time. The comment describing the reason for this odd choice of return value refers to a case where helm_repository data source/resource as not being setup.

However, #466 officially deprecates helm_repository, and it can be argued that this edge case no longer applies. If that's the case, then this probably is the correct fix. If we must assume that possibility of getChart() failing but not being allowed to accept the failure, then #512 is the right fix. You can still argue #512 is the right fix because in theory getChart() can work properly during plan, but fail during apply (e.g. someone deletes the repo?), in which case this fix won't detect the issue.

Note that the comments in #161 also indicates that "downloading the chart on the diff evaluation stage is not a good idea", but that behavior seems built into the assumption of this update working.

While here, I found that the resourceGetter argument to getChart is unused as of Helm 3, so I removed it to simplify the method call.

How to demonstrate the bug this is fixing

(From #472, tested to occur in Terraform Helm provider 1.2.1 and Terraform 0.12.25)

Create a resource normally.

resource "helm_release" "metrics-server" {
  name       = "metrics-server"
  repository = "https://kubernetes-charts.storage.googleapis.com"
  chart      = "metrics-server"
  version    = "2.11.1"
}

Change the chart to something non-existent. In my case, change the chart to "foo/metrics-server". This will cause Terraform to fail.

resource "helm_release" "metrics-server" {
  name       = "metrics-server"
  repository = "https://kubernetes-charts.storage.googleapis.com"
  chart      = "foo/metrics-server"
  version    = "2.11.1"
}

Error: chart "foo/metrics-server" version "2.11.1" not found in https://kubernetes-charts.storage.googleapis.com repository

  on metrics_server.tf line 22, in resource "helm_release" "metrics-server":
  22: resource "helm_release" "metrics-server" {

terraform apply again. It will show no diff; the Terraform state was updated with the invalid chartname.

Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

Revert to the first one with the correct name. Terraform apply will show a diff, showing that the incorrect chart name actually got written into the Terraform state. Note that the UPDATED timestamp column in helm list shows the helm release was never actually touched by the above operations:

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # helm_release.metrics-server will be updated in-place
  ~ resource "helm_release" "metrics-server" {
        atomic                     = false
      ~ chart                      = "foo/metrics-server" -> "metrics-server"
...
Plan: 0 to add, 1 to change, 0 to destroy.

Acceptance tests

[ ] Have you added an acceptance test for the functionality being added?
[ ] Have you run the acceptance tests on this branch? (If so, please include the test log in a gist)

I am not set up to run acceptance tests on my own infrastructure. On top of that, we have no easy way to mock out calls to getChart. Other calls to getChart do not have mocks either.

References

Fixes #472 (or at least part of it).

Jun 02 '20 02:06 mbarrien

Can either this PR or the https://github.com/hashicorp/terraform-provider-helm/pull/512 be prioritized? we are hitting the bug in https://github.com/hashicorp/terraform-provider-helm/issues/472 and apparently there is no suitable workaround.