grafana / tanka

Flexible, reusable and concise configuration for Kubernetes
https://tanka.dev
Apache License 2.0
2.39k stars 167 forks source link

Tanka incorrectly specifies a namespace when creating a Namespace #190

Closed rfratto closed 4 years ago

rfratto commented 4 years ago

Tanka applies the spec.json's namespace to a Namespace's metadata, which can be confusing (and is a no-op):

apiVersion: v1 
kind: Namespace 
metadata:
  name: dev
  namespace: testing 

To reproduce, create an environment using a namespace of testing and then use this for the main.jsonnet file:

(import 'github.com/grafana/jsonnet-libs/ksonnet-util/kausal.libsonnet') +
{
  ns: $.core.v1.namespace.new('dev'),
}
rfratto commented 4 years ago

It doesn't look like this is as straightforward as just not setting the namespace when reconciling a namespace object; Tanka still thinks it needs to create that object even when the namespace already exists.

sh0rez commented 4 years ago

We naively add the default namespace to everything that does not have one, regardless of whether it actually takes one, assuming this was silently discarded by kubectl when not applicable.

Apparently this becomes an issue with the List type (that one is causing real headaches, lol), which fails kubectl validation then.

The only proper fix I can think of is to use the types of k8s.io/apimachinery, to figure out if something is namespaced, or not. While this is cool, it will require some work to keep the code as clean as current pkg/manifest does .... the kubernetes code is far more verbose.

mplzik commented 4 years ago

I just noticed this as well; in a broader context, all non-namespaced objects will get the namespace data anyway during Reconcile.

W.r.t. using types in k8s.io/apimachinery, I'm afraid it might not be sufficient. The master could have k8s CRDs for non-namespaced objects (that is, e.g. cert-manager's ClusterIssuer). Off the top of my head, I can think of three approaches that can be taken here:

olegmayko commented 4 years ago

I'd add that having namespace definition in tanka looks a bit opinionated because you are forced to split your services, which may be defined in single main.jsonnet into having several "environments" for each service in one k8s cluster e.g. Fluentd, Kube-prometheus, backup and so on. I'd say that namespace definition should be part of jsonnet not tanka.

image

davidovich commented 4 years ago

I agree with @olegmayko, internally, I had to automate the copying of a tanka template environment just to change the namespace where the manifests would be applied.

sh0rez commented 4 years ago

I'd add that having namespace definition in tanka looks a bit opinionated ...

@olegmayko actually not! It is only injected, if the output from Jsonnet is missing one. So it is a default namespace, but you can still selectively override it from Jsonnet for the set of resources you need.

With #208 this will get even closer to the kubectl behaviour, as we delegate that feature to kubectl from then on.

olegmayko commented 4 years ago

@sh0rez thanks for clarification. It has a side-effect such as injecting it into "List" objects image and this leads to

error validating data: ValidationError(RoleBindingList.metadata): unknown field "namespace" in io.k8s.apimachinery.pkg.apis.meta.v1.ListMeta; I guess it be fixed in #208 as well.

olegmayko commented 4 years ago

@sh0rez thanks for fast fix.:tada: