feat(kubernetes): Support multiple kubectl versions

[jervi]

Currently it is hard to support clusters that runs different Kubernetes versions. Because kubectl executables can be defined per cluster, we can overcome this limitation by installing multiple kubectl versions, and then define which binary to use in the cluster config. The kubectl versions are stored without patch version, so that we can upgrade to newer versions without having to change the configuration. We also symlink kubectl to the current default version to not break existing configuration.

To opt in to a newer version, define it like this in the cluster config: kubectlExecutable: /usr/local/bin/kubectl-1.28 E.g.:

kubernetes:
  enabled: true
  primaryAccount: my-cluster
  accounts:
    - name: my-cluster
      kubeconfigFile: configserver:k8s/my-cluster.yml
      cacheThreads: 1
      cachingPolicies: []
      checkPermissionsOnStartup: true
      customResources: []
      dockerRegistries: []
      kinds: []
      liveManifestCalls: false
      namespaces: []
      oAuthScopes: []
      omitKinds: []
      omitNamespaces: []
      onlySpinnakerManaged: false
      providerVersion: V2
      kubectlExecutable: /usr/local/bin/kubectl-1.28

[dbyron-sf]

This looks great, thank you! Can I convince you to take a look at adding some mechanism for testing these additional kubectl versions, perhaps by teaching the integrations tests to have yet another dimension for kubectl version. KubernetesCluster is also likely relevant.

[jervi]

Thanks @dbyron-sf! I've taken a stab at it now. I opted to not add another dimension to the matrix, because it would cause a really huge number of builds to test every combination. Also it would go against the version skew policy of Kubernetes, which is what I really wanted to fix here in the first place. So instead I added a lookup table of hard coded kubectl versions for each Kubernetes version to be used in the tests. This means the integration tests will be running with the same kubectl version and Kubernetes version. WDYT?

[jervi]

Seems like the branch protections needs to be updated as well, once this is ready to be merged.

[jervi]

@dbyron-sf Any chance you can have another look at this? 😄

[dbyron-sf]

Sorry, I've been slammed with other things. Definitely have this on my radar though.

[dbyron-sf]

The mapping is in KubernetesCluster.java, right? And doesn't this comment more belong in the Dockerfiles?

[jervi]

You are correct, I'll update the comment to reference KubernetesCluster.java instead. But no, the mapping is only used for integration tests (and lives within the integration test sources). So the mapping is independent of what is defined in the Dockerfile (unless we implement the changes you mention here, of course).

[dbyron-sf]

Aaah right, these tests download kubectl, they don't depend on what's in the docker image.

[dbyron-sf]

I'm torn about this. Part of me really likes it. Part of me thinks we end up testing something farther away from what we ship, because the default version of kubectl is the only one that's used by default.

Does it make sense to change the logic e.g. here to dynamically choose the kubectl binary that matches the target cluster version? Which I suppose would then mean on startup / periodically we'd have to query target clusters for their version?

[jervi]

I'd love that feature! It would mean that the scope of this PR grows substantially, however... Do you want me to give it a go? What k8s versions would we support then? Also I haven't found a good way to automatically download the latest patch version of a given kubectl release. It would be awesome to not have to manually maintain that kubectl list in all of the dockerfiles and the deb packaging code.

[dbyron-sf]

I wasn't thinking about dynamically downloading kubectl binaries based on clusters we discover. I suppose that's a possibility, but could be tough to keep compliance folks happy.

I was more thinking about choosing from among the ones we've got. It probably does belong in another PR.

For this PR, what do you think of continuing to test the default kubectl version against all the k8s cluster versions, and then one more round of testing using the mapping here, and if we're very lucky, removing dups? That's way less than every client x every server, but also doesn't reduce the coverage we're getting today.

Then if we ever do dynamically choose the k8s version, we could drop the default round of testing.

One other thought that's been rolling around. I'd love to have another step in the PR build github action that invokes a new gradle Test task to exercise the just-built docker image with testcontainers. Not sure what the magic is to learn the name and tag, or perhaps we get to specify the tag and then know what to tell gradle.

With this, we could potentially adjust these tests to use the kubectl binaries in the docker image and not have to download them.

[jervi]

For this PR, what do you think of continuing to test the default kubectl version against all the k8s cluster versions, and then one more round of testing using the mapping here, and if we're very lucky, removing dups? That's way less than every client x every server, but also doesn't reduce the coverage we're getting today.

I've removed the in-code kubectl selection stuff and reverted to just using GitHub Actions. It's a bit cleaner (albeit with a bit of duplication in the action matrix), and it achieves what you describe above: one round of testing with the default kubectl version and another round using the version that corresponds to the k8s version.

[jervi]

A bit ugly to repeat all the kindest images here, but that was the best way I could find to conditionally add to an existing matrix. If anyone has a better solution, I'm listening. Relevant documentation.

[dbyron-sf]

Yeah, it's clunky, but if it works, it's ok with me. And from the looks of the PR checks that ran, it does:

[dbyron-sf]

LGTM, and thanks for your persistence here. I'll update the PR check requirements and get this in.