HelmRelease targetNamespace not honored by helm release secret about helm-controller HOT 8 CLOSED

The feature was initially aimed at multi-tenancy where "something" would be injected into the tenants namespace, and hiding the storage from the tenant made sense for this. Since the introduction of the KubeConfig feature, and because people at times seem to have other use-cases and/or structural setups, this seems to be a limitation.

To offer full flexibility, I think

Let's expose a second property on the HelmRelease called storageNamespace and let users pick their preferred storageNamespace

is a great idea, but in a way that is backwards compatible.

from helm-controller.

Put up a draft PR to add the property. It is optional so I think it should maintain backwards compatibility while extending the functionality.

Would love some input and if everyone is happy with the approach I can add some tests

#198

from helm-controller.

@gtracer I think this would be a perfect solution, solving all issues reported up to now here and maintaining backwards compatibility for current users.
I would suggest also changing the docs around helm-operator to helm-controller migration to point out that if users are using helm-operator with helmreleases.helm.fluxcd.io/v1 exploiting the targetNamespace feature, they should convert those to helmreleases.helm.toolkit.fluxcd.io/v2beta1 setting both targetNamespace and storageNamespace to the same namespace in order to maintain the same behavior.

from helm-controller.

It is intentional, there is some discussion in #96. I also recall some discussion about wanting to implement a flag for this upstream in the Helm CLI, but can't find that.

from helm-controller.

@seaneagan Thank you for the info!
The issue I see with this approach is that, besides not being able to manually upgrade the helm release as mentioned in the discussion you linked, the targetNamespace used to work differently with helm-operator. Therefore, if users are migrating from helm-operator using targetNamespace to helm-controller, the migration guide is no longer valid.
Is there any plan to possibly make the storage namespace for targetNamespace configurable?
And is there any official guide/advice for this specific migration?

from helm-controller.

I just stumbled upon that in combination with kubeConfig by trying to deploy a HelmRelease on a remote cluster. It fails because the namespace where the HelmRelease is applied does not exist on the remote cluster. This breaks the remote cluster ability.

apiVersion: helm.toolkit.fluxcd.io/v2beta1
kind: HelmRelease
metadata:
  name: my-chart
  namespace: local-namespace
spec:
  interval: 5m
  releaseName: my-chart
  targetNamespace: default
  kubeConfig:
    secretRef:
      name: kubeconfig
  chart:
    spec:
      chart: my-chart
      version: 1.0.0
      sourceRef:
        kind: HelmRepository
        name: my-repo
        namespace: flux-system

{"level":"error","ts":"2021-01-07T22:49:05.248Z","logger":"controller","msg":"Reconciler error","reconcilerGroup":"helm.toolkit.fluxcd.io","reconcilerKind":"HelmRelease","controller":"helmrelease","name":"my-chart","namespace":"local-namespace","error":"Helm install failed: create: failed to create: namespaces \"local-namespace\" not found"}

from helm-controller.

@cubic3d I am running into the exact same issue with remote clusters.

@tommasopozzetti similar to you I found that it is the helm client config code that is problematic.

For remote users it is a pretty limiting factor because that code is essentially stripping any ability to set the storage namespace. In comparison, with raw helm it defaults to the namespace used in my config so I can at least implicitly set it.

I see a few possible solutions:

1. Let's treat targetNamespace as the namespace for both storage and manifests (the same way helm treats --namespace)
2. Don't override the namespace at all and instead use the namespace on the remote kubeconfig (the same way the helm docs recommend today)
3. Let's expose a second property on the HelmRelease called storageNamespace and let users pick their preferred storageNamespace

As I read through the code, I don't see any glaring reasons why we couldn't configure the storageNamespace since:

1. The runner is instantiated for every reconcile (allowing HelmReleases to have different storageNamespaces)
2. The runner seems to be nicely designed as the single layer for working with Helm therefore making the code change pretty clean

Tagging those who seem like they have interest / knowledge based on #96

@seaneagan @hiddeco

Also, happy to contribute code to any solutions

from helm-controller.

Available in helm-controller v0.6.1 and flux v0.7.0.

from helm-controller.