Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use InClusterConfig but allow master IP to be overwritten? #281

Closed
MrHohn opened this issue Aug 28, 2017 · 9 comments
Closed

Use InClusterConfig but allow master IP to be overwritten? #281

MrHohn opened this issue Aug 28, 2017 · 9 comments

Comments

@MrHohn
Copy link
Member

MrHohn commented Aug 28, 2017

Is this a BUG REPORT or FEATURE REQUEST?:
/kind feature

What happened:
When trying to deploy kube-proxy DaemonSet with service account, I found InClusterConfig() not usable for kube-proxy, as this function sets master's IP to kubernetes service's VIP, which depends on kube-proxy itself to set up the proxy rules first...

At this point, in order to use service account on kube-proxy, we have to carry the responsibility of provisioning kube-proxy's kubeconfig at cluster deployment level, like what kubeadm does:

  kubeconfig.conf: |
    apiVersion: v1
    kind: Config
    clusters:
    - cluster:
        certificate-authority: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt
        server: {{ .MasterEndpoint }}
      name: default
    contexts:
    - context:
        cluster: default
        namespace: default
        user: default
      name: default
    current-context: default
    users:
    - name: default
      user:
        tokenFile: /var/run/secrets/kubernetes.io/serviceaccount/token

(Or alternatively we may be able to overwrite the Host field after calling InClusterConfig?)

What you expected to happen:
I would like to have a supported way to generate in-cluster config for components like kube-proxy. Probably something like InClusterProxyConfig(host string)?

How to reproduce it (as minimally and precisely as possible):
See kubernetes/kubernetes#51172 (comment).

cc @luxas @murali-reddy

@lavalamp
Copy link
Member

lavalamp commented Aug 28, 2017 via email

@murali-reddy
Copy link

murali-reddy commented Aug 28, 2017

@MrHohn thanks for looping me in. If its possible to achive this it would be helpful.

I implemented a service proxy in kube-router. I am sure there will be other implementations of service proxy for Kubernetes. This problem will affect any implemenation of service proxy.

@bjhaid
Copy link

bjhaid commented Aug 29, 2017

is explicitly setting the env var KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT not an adequate solve? We do this today in our deployment for all our incluster configurations as we don't use the in cluster VIP (for other reasons though)

@ericchiang
Copy link
Contributor

ericchiang commented Sep 6, 2017

You can do this today without modifications to client-go:

func InClusterProxyConfig(host string) (*rest.Config, error) {
	token, err := ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/token")
	if err != nil {
		return nil, fmt.Errorf("read service account token: %v", err)
	}
	return &rest.Config{
		Host:            host,
		BearerToken:     string(token),
		TLSClientConfig: rest.TLSClientConfig{
			CAFile: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt",
		},
	}, nil
}

Or even:

func InClusterProxyConfig(host string) (*rest.Config, error) {
	config, err := rest.InClusterConfig()
	if err != nil {
		return nil, err
	}
	// Overwrite discovered in-cluster host.
	config.Host = host
	return config, nil
}

The service account token and CA are documented at https://kubernetes.io/docs/tasks/access-application-cluster/access-cluster/#accessing-the-api-from-a-pod

Probably better to just improve the documentation for how to create your own in-cluster clients and use the rest package instead of adding more helpers.

@MrHohn
Copy link
Member Author

MrHohn commented Sep 6, 2017

Thanks all for the inputs. Overwriting the host field makes sense to me, I will go with that path. Closing this issue.

@MrHohn MrHohn closed this as completed Sep 6, 2017
@murali-reddy
Copy link

@MrHohn sorry, i am still not clear. while we can set hostfield in InClusterConfig, from service proxy perspective, how do we know host details to set?

@MrHohn
Copy link
Member Author

MrHohn commented Sep 7, 2017

@MrHohn sorry, i am still not clear. while we can set hostfield in InClusterConfig, from service proxy perspective, how do we know host details to set?

@murali-reddy IMHO the host details (server ip:port in this case) should still be given by cluster deployment layer instead of deriving from client library. The original purpose for this issue was to avoid generating kubeconfig files in cluster deployment layer, and setting hostfield in InClusterConfig seems to be a viable solution to me.

@murali-reddy
Copy link

Agree this is not the scope of the bug @MrHohn i guess the problem of how to build a service proxy that can run as daemon set with service accont is still a chicken-and-egg problem.

@MrHohn
Copy link
Member Author

MrHohn commented Sep 7, 2017

i guess the problem of how to build a service proxy that can run as daemon set with service accont is still a chicken-and-egg problem.

Agree with that :)

k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this issue Oct 14, 2017
Automatic merge from submit-queue (batch tested with PRs 52883, 52183, 53915, 53848). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

[GCE kube-up] Don't provision kubeconfig file for kube-proxy service account

**What this PR does / why we need it**:

Offloading the burden of provisioning kubeconfig file for kube-proxy service account from GCE startup scripts. This also helps us decoupling kube-proxy daemonset upgrade from node upgrade.

Previous attempt on #51172, using InClusterConfig for kube-proxy based on discussions on kubernetes/client-go#281.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #NONE 

**Special notes for your reviewer**:
/assign @bowei @thockin 
cc @luxas @murali-reddy

**Release note**:

```release-note
NONE
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants