Environment name set in spec.json is ignored

[Mikulas]

Currently tk.env.metadata.name is always set to the default generated value, regardless of whether metadata.name is set in spec.json or not.

This patch

• adds a test case for the expected behaviour and
• does not override the value loaded from spec.json.

I found a somewhat relevant mention in #163 and a link to #131, but that may be slightly different issue. As an alternative solution tk could instead throw an error when the name is set in spec.json if you believe the generated value is best. This would also be better for backwards compatibility if there are already users unknowingly using the default value while having a different name specified in spec.json.

/kind bug

[sh0rez]

Hey, good spot!

This is actually by design. The environment name is exclusively generated from the directories name. It is defined as the relative path from the projectRoot to the directory that contains main.jsonnet, which is described in https://tanka.dev/config.

The rationale behind this is to avoid confusion and drift between those, but also iirc this value is used in filepath.Join calls several times.

Nevertheless, I agree this is totally unclear from spec.json. I cannot offer a good solution here right now tbh. I prefer to keep it visible in the config, to stay true to the "k8s style" object. Raising an error sounds overkill to me, moving an environment should be a silent operation, given this value is implict after all

[Mikulas]

Gotcha. Tanka has a philosophy I can very much relate to so I totally respect you standing the ground on this.

The issue I originally run into is twofold:

• I was attempting a migration of our apps which already use a different naming scheme and wanted to keep the names.
• Kubernetes requires RFC 1123 names in many places https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names (though to my surprise metadata.name by itself does not actually have this limitation https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L111-L118). This makes the Tanka generated name hard to use and requires additional transformations.

I'm not sure I explained the error/warning appropriately. I've updated the patch to better illustrate my point. If this is still unnecessary complexity, at least https://github.com/grafana/tanka/blob/master/docs/docs/config.md certainly requires an update

[sh0rez]

I was attempting a migration of our apps which already use a different naming scheme and wanted to keep the names.

Could you explain how exactly this relates to the environment config? After all, spec.json is really runtime configuration for Tanka, that happens to look similar to a k8s object. It is not applied to any cluster.

This makes the Tanka generated name hard to use and requires additional transformations

When does this happen? afaict the name is something internal and never leaves Tankas area of responsibility

[Mikulas]

tldr tk.env.metadata.name ?= helm release name

Oh I see! The context I'm coming from is this:

• At first we deployed our apps through Helm, which has a concept of a release name which is generally used to generate object names as well as certain labels.
• We then migrated from helm to custom templating tool with kubectl apply prune with a metadata file that looked similar to this:

apiVersion: my.org/v1
kind: GenericRelease
metadata:
  name: name-for-my-service
  labels:
    service: foo
spec:
  target:
    cluster: target-cluster
    namespace: target-ns
  # … and basically helm values here

So very similar to how Tanka works, with the main difference being the addition of the templating values in the file.

From what I understood so far Tankas spec.json was basically this file holding metadata for the environment (release). So from my point of view not an internal name at all but rather one of the most exposed fields! That being said this is totally just a point of view. I can easily get custom names in through a different field.

[Mikulas]

To elaborate with a concrete example:
A helm chart would traditionally have object names generated from $.Release.Name (https://github.com/helm/charts/blob/master/stable/nginx-ingress/templates/controller-deployment.yaml#L15, https://github.com/helm/charts/blob/master/stable/nginx-ingress/templates/_helpers.tpl#L26).
Tanka users may be tempted to do the same, assuming tk.env.metadata.name in Tanka is an equivalent to Helm's $.Release.Name.

[sh0rez]

Ahh, now I see.

We use environments a bit differently I guess: environments/<app>/<cluster>.<namespace>
For example, we have loki and cortex apps, and several cluster these are deployed to.

The crucial part here is that environments don't actually include any resource definitions. There is no Deployment defined in an environment. Instead, we wrote libraries (jsonnet that is not an environment) to define them (see https://github.com/grafana/jsonnet-libs).

In environments, we only import and configure (ports, secrets, etc) those libraries. So your "Helm Chart" equivalent is a library, and a library usually knows what it creates (ie loki knows it creates Loki).

Would that be an option for you?

[Mikulas]

I think we are on the same page with libraries. In fact that is why I got stuck for the most part. In the end I replaced metadata.name with metadata.labels[release] and successfully migrated our apps. We have over 300 apps with multiple envs each and most of those are rendered from a generic template (hence GenericRelease). Things got to a point where the helm charts are no longer maintainable, so I utilized the recent Helm support in Tanka to load our helm charts to extend them with jsonnet / rewrite them to jsonnet.

That being said the original issue I believe is still valid. It is very confusing for users to have their metadata.name in spec.json silently ignored.

I prefer to keep it visible in the config, to stay true to the "k8s style" object. Raising an error sounds overkill to me, moving an environment should be a silent operation, given this value is implict after all

Not sure I got you there, but the latest change I propose allows users to explicitly set the value as long as it is identical to the generated value (that's what I understood from the “visible in the config” part).

[Duologic]

To my understanding, the current naming scheme at Grafana environments/<app>/<cluster>.<namespace> is a convention that ensures the name label is unique across the broader tanka/jsonnet code base. This has trickled into Tanka itself by setting Metadata.Name, ensuring its uniqueness.

I agree here with @Mikulas that this issue is valid and the behavior is confusing. I like the check implemented here but maybe we can drop a warning instead of returning an error? I also think it would be interesting to have a tk env repair command or something along those lines to help beginners.

Example warning:

Warning: The name (different.environment) in spec.json doesn't match the environment path (environments/actual/environment). Resolve this by running 'tk env repair environments/actual/environment'.

[sh0rez]

@Duologic I also think it would be interesting to have a tk env repair

Needing software to repair itself sounds like poor design to me :) I guess we can do better!

I agree with @Mikulas and @Duologic that we have confusing behavior here and erroring out would solve that at the cost of UX when moving environments around.

What might solve that though would be a tk env mv command, which moves the environment and directly updates spec.json. Wdyt?

[Duologic]

Needing software to repair itself sounds like poor design to me :) I guess we can do better!

It doesn't repair itself, it just a shortcut for typing sed -i '/name: /name: "correct.path.to.env"/' spec.json

Also intended for the use case where the move has already happened.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.