nextflow icon indicating copy to clipboard operation
nextflow copied to clipboard

Change k8s client to use namespace and service account from nextflow config

Open bentsherman opened this issue 1 year ago • 8 comments

Closes #2266

bentsherman avatar Sep 20 '22 14:09 bentsherman

I used <class>.metaClass.static.<method> to mock static methods. Looks like it works for Java classes as well, so it could eliminate the need to create little helper methods like ConfigDiscovery.path() just for testing purposes.

bentsherman avatar Sep 20 '22 14:09 bentsherman

I thought it was cleaner to separate the config discovery from the auth token discovery, rather than pass the namespace and service account down the call stack to discoverAuthToken(). That required making ConfigDiscovery a static helper class, which makes sense to me because ConfigDiscovery doesn't really have any state like an object. However this testing framework really doesn't play well with static methods. I guess I could make ConfigDiscovery a singleton instead, like OperatorImpl.

bentsherman avatar Sep 20 '22 17:09 bentsherman

it's already, what's done for the context, that than it's used in the fromConfig that's the same scope invoking also discoverAuthToken.

Looks straightforward extending it to support also namespace and serviceAccount

pditommaso avatar Sep 20 '22 18:09 pditommaso

ConfigDiscovery.fromConfig loads the namespace and user from the current context, but K8sConfig.getClient applies the namespace and serviceAccount from the nextflow config if they are provided. Basically, the nextflow k8s config settings should be used first, and the current context used as a backup.

I think there is still a logic error here -- if the current context contains a user with a token, it will be used even if a service account is provided in the nextflow config. But I think the service account from the nextflow config should be used first.

bentsherman avatar Sep 20 '22 18:09 bentsherman

ConfigDiscovery.fromConfig loads the namespace and user from the current context, but K8sConfig.getClient applies the namespace and serviceAccount from the nextflow config if they are provided. Basically, the nextflow k8s config settings should be used first, and the current context used as a backup.

Indeed, namespace and serviceAccount in the k8s config should take precedence over the one in context.

Let's focus on this.

pditommaso avatar Sep 20 '22 18:09 pditommaso

Here is my understanding of things:

  1. load user, cluster, and namespace from current context
  2. load certs and auth token from current user (if present)
  3. use namespace from nextflow config (if present)
  4. use service account from nextflow config (if present)

And the priority for credentials is as follows:

  1. service account from nextflow config -- discover auth token via kubectl
  2. current context user -- use certs from kube config
  3. current context user -- use auth token from kube config

If these priorities are correct then I need to adjust the logic slightly. Also need to make sure that default namespace and default service account are used by default.

bentsherman avatar Sep 20 '22 18:09 bentsherman

It sounds reasonable

pditommaso avatar Sep 20 '22 19:09 pditommaso

I think the logic is correct, however the use of discoverAuthToken can fail when running it within a pod.

Is there any Kube API that can be used to find it?

pditommaso avatar Sep 22 '22 12:09 pditommaso

Okay it's ready. The only caveat now is that if a user+token is defined in the KUBECONFIG, it will override k8s.serviceAccount for the purposes of API requests. But I think most users don't do that anyway. I will make another PR later to deal with this minor issue.

So the overall logic is mostly unchanged, except that the namespace and serviceAccount are now used to discover the auth token.

bentsherman avatar Sep 23 '22 15:09 bentsherman

Merge manually trying to not rely on any intermetied state for the namespace and serviceAccount attributes. See c3364d0f

pditommaso avatar Oct 02 '22 10:10 pditommaso