databricks / click

The "Command Line Interactive Controller for Kubernetes"
Apache License 2.0
1.49k stars 84 forks source link

Add support for multiple files in KUBECONFIG #63

Closed lofim closed 5 years ago

lofim commented 6 years ago

Hi there,

let me start by saying that you have a very nice project here. I've been enjoying using it for couple of the last days, however the lack of support for multiple files in KUBECONFIG makes it pretty usable for me.

I've been playing around with it a bit and decided to hack together a simple support for multiple files (addressing #49).

What I'm doing here is that I'm processing all paths in KUBECONFIG into separate config::Config structs and then merging them into a single one that is then fed into the environment. The source_file field is merged using OS specific path separators. While merging of the other fields is done by simple map extending.

I've tested it with my workflow and it worked flawlessly, but I can image the change might break something I've not tested.

Let me know what you think, I'm open to any suggestions.

sibrahim001982 commented 6 years ago

Are you using it on GKE? Been trying to run it against a kubeadm-based cluster and to no avail.

lofim commented 6 years ago

Interesting, I've tested it with minikube, several Kops provisioned clusters and EKS on AWS.

How far in the loading process did you get? Did it failed during path splitting or config parsing? Can you provide any error messages perhaps?

My KUBECONFIG setup looks like this:

export KUBECONFIG=~/.kube/config                       # Minikube config
export KUBECONFIG=$KUBECONFIG:~/.kube/config-kops-dev  # Kops cluster
export KUBECONFIG=$KUBECONFIG:~/.kube/config-kops-prod # Kops cluster
export KUBECONFIG=$KUBECONFIG:~/.kube/config-eks       # EKS cluster

The config contents are pretty standard as far as I can tell. Minikube config:

apiVersion: v1
clusters:
- cluster:
    certificate-authority: /Users/my.user/.minikube/ca.crt
    server: https://192.168.99.100:8443
  name: minikube
contexts:
- context:
    cluster: minikube
    user: minikube
  name: minikube
current-context: minikube
kind: Config
preferences: {}
users:
- name: minikube
  user:
    as-user-extra: {}
    client-certificate: /Users/my.user/.minikube/client.crt
    client-key: /Users/my.user/.minikube/client.key

Kops cluster config (replaced sensitive data with <>):

apiVersion: v1
clusters:
- cluster:
    certificate-authority-data: <certificate>
    server: <server-url>
  name: <cluster-name>.local
contexts:
- context:
    cluster: <cluster-name>.local
    user: <cluster-name>.local
  name: <cluster-name>.local
current-context: <cluster-name>.local
kind: Config
preferences: {}
users:
- name: <cluster-name>.local
  user:
    client-certificate-data: <certificate>
    client-key-data: <certificate>
    password: <password>
    username: <username>
- name: <cluster-name>.local-basic-auth
  user:
    password: <password>
    username: <username>

EKS one is pretty similar except it's missing the auth stuff.

I've no idea what the difference with kubeadm could be. Maybe it's the token stuff click supports for GKE.

nicklan commented 6 years ago

Thanks for both of your PRs! I'll be testing/reviewing them shortly (and sorry for the delay on initial response)

lofim commented 6 years ago

I've managed to remove the commit for job support. Sorry for the confusion. I need to get better with my branching.

nicklan commented 6 years ago

Cool! This is something we should add for sure. Looking over this I like that you're using split_paths to make a vec, but I think we should just get rid of the from_file method and have only a from_files (which will take a one element vector if there's only one config file). Then from_files can look similar to the current from_file, except it'll create an IConfig for each file, and then do the same logic to parse and copy over the data from each IConfig into the components for the final Config.

It's conceptually similar but I don't think we need the complexity of building lots of separate Configs and then merging them.

Let me know what you think (maybe there's a great reason to be doing it this way I'm missing). I can probably take a stab at doing this based off what you have here too, if you don't have time to modify this.

Thanks!

lofim commented 6 years ago

It definitely makes sense to do it on one step. I've hacked something together although I'm not sure I like duplicated for loops.