kubeflow / notebooks

Kubeflow Notebooks lets you run web-based development environments on your Kubernetes cluster by running them inside Pods.
Apache License 2.0
18 stars 22 forks source link

How to init the Kubernetes client for the workspace backend #12

Closed ederign closed 5 months ago

ederign commented 6 months ago

Checks

Motivation

The goal of this issue is to develop:

Implementation

func NewKubernetesClient() (*KubernetesClient, error) {

    clientSet, err := newClientSet()
    if err != nil {
        return nil, fmt.Errorf("failed to create Kubernetes clientset: %w", err)
    }

    return &KubernetesClient{ClientSet: clientSet}, nil
}

func getRestConfig() (*restclient.Config, error) {
    loadingRules := clientcmd.NewDefaultClientConfigLoadingRules()
    configOverrides := &clientcmd.ConfigOverrides{}
    kubeConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, configOverrides)
    return kubeConfig.ClientConfig()
}

func newClientSet() (*kubernetes.Clientset, error) {
    restConfig, err := getRestConfig()
    if err != nil {
        return nil, err
    }
    return kubernetes.NewForConfig(restConfig)
}

Are you willing & able to help?

ederign commented 6 months ago

FYI: I'm currently working on this. (I don't have 'assign' privileges yet).

ederign commented 6 months ago

Inside cluster:

> kubectl port-forward service/my-service-nb 8080:80
Forwarding from 127.0.0.1:8080 -> 4001
Forwarding from [::1]:8080 -> 4001
Handling connection for 8080
Handling connection for 8080
image

Outside cluster, you can either use .kube config OR export KUBECONFIG=pathtokubeconfig.yaml

ederign commented 6 months ago

Also, I've moved the Kubernetes client to app startup. Seems like the Kubernetes client is designed to handle long-lived connections and can manage its own timeouts and retries.

However, on future, we can improve it with proper configuration of timeouts and other settings

And also see if we should deal with refresh the client due to changes in the cluster configuration or credentials. But I believe we can approach both things in future.

ederign commented 6 months ago

@thesuperzapper one thing that I would like to discuss today!

jiridanek commented 6 months ago

I'm guessing this is how it works here

see https://github.com/GoogleCloudPlatform/oss-test-infra/blob/fbd8c2ba69587b403bcd4dd23362eb3e019e7ea5/prow/oss/plugins.yaml#L319-L335

The sources at https://github.com/kubernetes-sigs/prow/tree/main/pkg/plugins seem to be the only docs for the supported comands?

/priority p1 /assign @ederign

google-oss-prow[bot] commented 6 months ago

@jiridanek: The label(s) priority/p1 cannot be applied, because the repository doesn't have them.

In response to [this](https://github.com/kubeflow/notebooks/issues/12#issuecomment-2140800281): >I'm guessing this is how it works here > >/priority p1 >/assign @ederign Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
jiridanek commented 6 months ago

/help

jiridanek commented 6 months ago

uh, sorry, I thought that /help will maybe display some help text for these commands ;P

/remove-help

thesuperzapper commented 6 months ago

I think we might be able to just use the BuildConfigFromFlags constructor directly with the KUBECONFIG env-var, like this: (WRONG: see https://github.com/kubeflow/notebooks/issues/12#issuecomment-2148569483)

kubeconfig := os.Getenv("KUBECONFIG")
config, err = clientcmd.BuildConfigFromFlags("", kubeconfig)
if err != nil {
    return nil, fmt.Errorf("failed to load kubeconfig: %w", err)
}

Because the upstream docs say that BuildConfigFromFlags() will fallback to the default home location:

BuildConfigFromFlags is a helper function that builds configs from a master url or a kubeconfig filepath. These are passed in as command line flags for cluster components. Warnings should reflect this usage. If neither masterUrl or kubeconfigPath are passed in we fallback to inClusterConfig. If inClusterConfig fails, we fallback to the default config.

So the flow would be:

  1. Try reading KUBECONFIG, if that fails
  2. Try in cluster, if that fails
  3. Fall back to home directory
thesuperzapper commented 5 months ago

@ederign sorry, I was mistaken in https://github.com/kubeflow/notebooks/issues/12#issuecomment-2141051590.

The correct usage of the clientcmd package (see here) seems to be something like this:

func getRestConfig() (*restclient.Config, error) {
    loadingRules := clientcmd.NewDefaultClientConfigLoadingRules()
    configOverrides := &clientcmd.ConfigOverrides{}
    kubeConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, configOverrides)
    return kubeConfig.ClientConfig()
}

func newClientSet() (*kubernetes.Clientset, error) {
    restConfig, err := getRestConfig()
    if err != nil {
        return nil, err
    }
    return kubernetes.NewForConfig(restConfig)
}

This combination of NewDefaultClientConfigLoadingRules and NewNonInteractiveDeferredLoadingClientConfig cause the following order of precedence:

  1. Try reading KUBECONFIG (splitting by : if there are multiple files specified in it).
  2. Next, try using the default $HOME/.kube/config for the system.
  3. Next, try using "in cluster config"
  4. Fail otherwise

I have tested this locally with and without KUBECONFIG being set, and on a Pod (in a Notebook) to test the "in cluster" configs.

ederign commented 5 months ago

@thesuperzapper Yeah, thank you for the guidance; in the previous suggestion, I got an exception when the KUBECONFIG was not set. I tested it in all scenarios and can confirm that it works as expected.

ederign commented 5 months ago

PR https://github.com/kubeflow/notebooks/pull/15/files