helm: apply namespace default transformation

[geekflyer]

When I specify namespace on a chart via pulumi it seems this only populates the Release.Namespace template variable, whereas when running helm install --namespace <namespace> tiller seems to actively add an metadata.namespace entry before applying the templates to the cluster. Many charts unfortunately don’t have the Release.Namespace variable but instead rely on the aforementioned tiller transformation.

Pulumi should apply the same transformation as default, if the namespace attribute has been set on the component. This providers better UX, is closer to helm install and avoids a lot of surprises for new pulumi users.

The default may be as simple as

    manifest => {
      manifest.metadata.namespace = this.args.namespace;
    }

Corresponding slack conversation: https://pulumi-community.slack.com/archives/C84L4E3N1/p1537573095000100

[hausdorff]

To recap offline discussion, I am guessing that helm install --namespace does not work by rewriting the .metadata.namespace inside the API objects themselves, but instead by works by re-using client-go's implementation of the --namespace (which is what is used by kubectl). This approach is vastly simpler, because (1) there are a few gotchas in the rewriting approach, like the fact that many resources do not use namespaces, and (2) we already support this at a global level, so we'd just need to find a way to scope those operations to a local level.

This should be doable, let me see if we can fit it into M18.

[lblackstone]

As a workaround for now, you should be able to use a transformation to update the namespace.

function addNamespace(o: any) {
    if (o !== undefined) {
        if (o.metadata !== undefined) {
            o.metadata.namespace = "my-new-namespace";
        } else {
            o.metadata = {namespace: "my-new-namespace"}
        }
    }
}

const nginxController = new k8s.helm.v2.Chart(
  "nginx-ingress",
  {
      repo: "stable",
      version: "0.31.0",
      chart: "nginx-ingress",
      transformations: [addNamespace]
  });

[hausdorff]

Per discussion on #467, this is basically the same issue as #415, and we should therefore solve this at the same time.

[lblackstone]

I ran into further complications trying to implement default behavior for the namespace flag.

1. We're using helm template under the hood to provide Helm support. Helm doesn't change namespaces on the client side, only on the server side if Tiller is in use.
2. Upstream charts are not consistent in their templating behavior. Some charts explicitly set resources namespaces using the Release.Namespace variable, which does not require Tiller to change the namespace. These charts will already work properly when the Chart namespace flag is set in Pulumi.
3. CustomResources in k8s can be either namespace-scoped or cluster-scoped, and it's not possible to determine which it is at the language SDK layer where our Helm support is implemented. It would require major changes to support this feature as requested.

Considering these factors, I'm going to close out this issue as "won't fix".

There are several possible workarounds depending on the chart:

1. Use the transformations parameter to make the desired changes to a chart. This is a very flexible approach, but should be used with care since it can set state that doesn't make sense (e.g., assigning a namespace to a cluster-scoped resource).
2. Use the existing namespace flag on the Chart SDKs for charts that set resource namespaces with the Release.Namespace variable. Users can file an issue against the relevant chart repo where this is not the case.
3. Set the default namespace parameter on the provider for the chart. This will change the namespace for any namespace-scoped resource that doesn't have one set. For example:

const provider = new k8s.Provider("foo", {namespace: "bar"});
const chart = new k8s.helm.v2.Chart("example", {
    repo: "stable",
    chart: "not-a-real-chart",
}, {providers: {kubernetes: provider}});

4. Port the chart to Pulumi and create the resources with the native APIs. This approach requires the most work up front, but provides control over every aspect of the resource creation.

[nesl247]

This is really unfortunate to see.

I think a good middle ground would be to introduce an option useNamespaceImplicitly or something, that will automatically perform the transformation for any known resource definitions that are not cluster specific. It could even take a non-bool value to specify using it on all resources.

At least this way working with helm will be a lot easier (everyone won't have to implement the transformation themselves).

[lukehoban]

@lblackstone If helm template is sufficiently "incorrect" relative to user expectations for Helm charts, I expect we will have to do #511 to support a mode where Hem charts are deployed using Tiller directly.

It doesn't seem okay to just not have charts behave the same when deployed through Pulumi as when deployed "normally" through Helm.

Other thoughts on how to address the user concerns here if fixing this namespacing issue directly really isn't an issue?

[nesl247]

In our case, and I think in most people’s case these days, we don’t want to use tiller. This was actually a huge point as to why we just adopted helm through Pulumi.

Helm 3 actually does away with tiller. I’m not sure how they are solving the same issue, but I would look into replicating their logic.

[eh-am]

I had the same issue and ended up adding the .Release.Namespace to the charts I needed. It works better in the long run, since other communities can also benefit from it (I know Spinnaker also suffers from the same problem).

[nesl247]

Do you know if helm is really looking to do that? I found this issue via helm/charts#14773 when I reported that Jenkins wasn't working as intended.

[lblackstone]

There are quite a few instances where charts have done that already: https://github.com/helm/charts/search?l=YAML&q=Release.namespace

I suspect they'd accept a PR, considering that Tiller is going away and anybody using helm template directly is going to run into the same problem.

More relevant links:
helm/charts#13924
helm/helm#3553

[pjoe]

FWIW: here is my workaround transformer:

export const addNamespace = (namespace: string) => (obj: any) => {
  if (!obj) return;
  if (
    [
      "ServiceAccount",
      "Role",
      "RoleBinding",
      "ConfigMap",
      "Service",
      "DaemonSet",
      "Deployment"
    ].includes(obj.kind)
  ) {
    if (!obj.metadata) {
      obj.metadata = { namespace };
    } else {
      if (!obj.metadata.namespace) obj.metadata.namespace = namespace;
    }
  }
};

To be used like:

...
transformations: [addNamespace("some-namespace")]
...

It only defaults namespace for those specific resource kinds (so no CRD handling).

[brandonkal]

To expand on @pjoe's answer. This is what I needed to get things working:

import { ResourceTransformation } from '@pulumi/pulumi'

/** addNamespace transformation sets namespace to desired string.
 * Useful for helm charts `transformations: [addNamespace('ns')]` */
export const addNamespace = (namespace: string): ResourceTransformation => (
  args
) => {
  if (!args) return args
  if (
    [
      'Binding',
      'ConfigMap',
      'Endpoints',
      'Event',
      'LimitRange',
      'PersistentVolumeClaim',
      'Pod',
      'PodTemplate',
      'ReplicationController',
      'ResourceQuota',
      'Secret',
      'ServiceAccount',
      'Service',
      'Challenge',
      'Order',
      'ControllerRevision',
      'DaemonSet',
      'Deployment',
      'ReplicaSet',
      'StatefulSet',
      'LocalSubjectAccessReview',
      'HorizontalPodAutoscaler',
      'CronJob',
      'Job',
      'SealedSecret',
      'CertificateRequest',
      'Certificate',
      'Issuer',
      'Order',
      'Lease',
      'AuthCode',
      'AuthRequest',
      'Connector',
      'OAuth2Client',
      'OfflineSessions',
      'Password',
      'RefreshToken',
      'SigningKey',
      'Event',
      'Ingress',
      'PodMetrics',
      'Ingress',
      'NetworkPolicy',
      'PodDisruptionBudget',
      'RoleBinding',
      'Role',
    ].includes(args.props.kind)
  ) {
    if (!args.props.metadata) {
      args.props.metadata = { namespace }
    } else {
      if (!args.props.metadata.namespace)
        args.props.metadata.namespace = namespace
    }
  }
  return args
}

You can find all namespaced resources with: kubectl api-resources --namespaced=true -o name

[lukehoban]

Reactivating this - as it continues to be a pain point. It’s unclear how Helm 3 (without Tiller) handles this - and why Pulumi can’t do something compatible with Helm 3 to have better compatibility with charts without the need for manual transformations. If we do believe there is nothing we can do in Pulumi - we should audit top 20 charts and contribute fixes to them to improve our compatibility.

[pjoe]

I think what helm 3 is basically doing when installing a chart is something like:

Render template > kubectl --namespace $namespace apply -f -

So the namespace is not applied during templating (except for .Release.Namespace), but rather when applying the rendered result.

[lblackstone]

I took another look at this, and have a WIP PR up based on the input from @pjoe and @brandonkal. It needs some more testing, but I think it should handle the majority of cases.

The main problem will be CustomResources, which may require additional transformations if they also need to be namespaced, and the chart doesn't include that in the template.

[lblackstone]

This should be fixed now, including for CustomResource kinds. If you encounter cases where this isn't working properly, please open a new issue with details.

[lblackstone]

Had to temporarily revert this PR, so reopening the issue until the next release, which will include this fix again.

[almson]

It would be great if ConfigFile and ConfigGroup had a namespace parameter that used the same mechanism.