Use "app" label in selectors

kupson commented 5 years ago

Fixes issue #16 - Deployment's selector should be immutable

Scenario:

Deployment is created with kube.libsonnet, CI process adds additional labels e.g. "ci-job-id"
Deployment is updated with the same process but the CI process was updated to add new label e.g. "ci-cluster"
kubectl apply fails as the selector field is immutable

This adds an additional label "app" to Pod, Deployment, StatefulSet, DaemonSet, Job and CronJob objects and use it when those objects needs to be referred by selectors.

anguslees commented 5 years ago

Thanks for the suggestion! Using "app" is probably a common situation for people migrating an existing config to kube.libsonnet, so it's good to see what we can do to make this easier.

There are a couple of conceptually-separate changes here, which I'd like to discuss separately:

1. Rename `name=foo` label to `app=foo`

I don't think we can do (1) as it is in this PR :disappointed:. I know a lot of the rest of the community uses "app=name", but this convention is purely cosmetic, and would introduce a change of labels to all existing kube.libsonnet users (triggering exactly the issue this PR is attempting to avoid!).

If I had a time machine, I would probably choose to use app=foo instead, although even then I would question it tbh. In the low-level automated context we're talking about in kube.libsonnet we don't know what the "app" in question is - all we know is the name given to the k8s object (eg: we know "elasticsearch-master-node", not "elasticsearch"). The introduction of Application objects confuses this even further if the app labels don't match the same grouping hierarchy.

2. Introduce a new `_AppObject()` "constructor"

(2) is unnecessary, if we drop (1).

As a general pattern however, creating new constructors doesn't compose well (what if we had several special tweaks we wanted to apply to this Object?), and "mixins" tend to work better.

ie: Consider the following. You can chain multiple "mixins" to add additional modifiers:

{
  applabels:: {
    local name = self.metadata.name,
    metadata+: {labels: {app: name}},
  },

  Deployment(name):: $._Object(name) + $.applabels + {
    // as normal ...
  },
}

3. Change selector to a subset of the labels (just `{app: value}`)

Separating the top-level Deployment labels from the low-level Pod label/selector is probably a good idea, for the reasons you raise. I liked the way the selector was "exported" from the object in question in the previous version (by just assuming that it was all metadata.labels), rather than having everything else "know" that the appropriate choice is {app=target.labels.app}. It would be good to get some of that back - probably by adding a hidden selector field to all objects, and then using that everywhere we need a cross-object selector...

Let me think about that for a bit :stuck_out_tongue:

Note for (1)

If a local site was using an "app=foo" convention, and would like to continue using this convention with kube.libsonnet then they can easily do this with a local customisation of kube.libsonnet that has something like this PR in a separate "overlay" file:

// Call this "myorganisation.libsonnet" or similar
local kube = import "kube.libsonnet";
local applabels = {
  metadata+: {labels: {app: $.metadata.name}},
};

{
  Deployment(name):: kube.Deployment(name) + applabels {
    // .. and any other local policies
  },
  StatefulSet(name):: kube.Deployment(name) + applabels,
  // and so on, for other modified types.
}

.. and then of course use it as usual:


// Example usage
local myorg = import "myorganisation.libsonnet";
{
  example: myorg.Deployment(name) {
    spec+: {
      // ... as usual
    },
  },
}

kupson commented 5 years ago

Thank you for the review and ideas how to implement our fixes in a better way. I'll close the pull request as the issue needs to be fixed in a different way.

bitnami / kube-libsonnet