Added sshd, kubeconfig and insecure-serving flags to localkube for di… #1975

aaron-prindle · 2017-09-18T17:50:08Z

…nd image

dlorenc · 2017-09-18T18:44:08Z

cmd/localkube/cmd/options.go

 		APIServerName:            constants.APIServerName,
+		ShouldGenerateKubeconfig: true,


Should this default to false?

r2d4 · 2017-09-28T21:27:35Z

deploy/docker/localkube-dind/Dockerfile

+RUN sed -i 's|authorized_keys|authorized_keys /etc/localkube-ssh/localkube_rsa.pub|g' /etc/ssh/sshd_config
+
+ENV NOTVISIBLE "in users profile"
+RUN echo "export VISIBLE=now" >> /etc/profile


What do these two lines do?

r2d4 · 2017-09-28T21:27:50Z

deploy/docker/localkube-dind/start.sh

--extra-config=kubelet.AllowPrivileged="true"
+--extra-config=kubelet.AllowPrivileged="true" \
+--v=100 \
+--logtostderr &> /var/log/localkube/logs.txt


done. Also added auto-newline setting to editor, hopefully this will be the last newline comment

theres a vs-code addon if you haven't gotten it 😂

r2d4 · 2017-09-28T21:29:07Z

pkg/localkube/proxy.go

@@ -62,7 +69,11 @@ func StartProxyServer(lk LocalkubeServer) func() error {

 	return func() error {
 		// Creating this config requires the API Server to be up, so do it in the start function itself.
-		server, err := kubeproxy.NewProxyServer(config, false, runtime.NewScheme(), "")
+		url := ""


use var url string instead

r2d4 · 2017-09-28T21:30:12Z

pkg/localkube/proxy.go

-		server, err := kubeproxy.NewProxyServer(config, false, runtime.NewScheme(), "")
+		url := ""
+		if lk.APIServerInsecurePort != 0 {
+			url = lk.GetAPIServerInsecureURL()


Can this function just return "" if lk.APIServerInsecurePort == 0?

Not sure what you mean. If insecureport is enabled, use that url as the parameter as was done previously. If not, do what is done currently and use "".
https://github.com/kubernetes/minikube/blob/master/pkg/localkube/proxy.go#L65

Are you suggesting to flip the case so that the url defaults to the insecureURL and then reverse the if statement so that it would set it oppositely? I don't understand what is gained.

I meant

func (lk LocalkubeServer) GetAPIServerInsecureURL() { if lk.APIServerInsecurePort != 0 { return fmt.Sprintf("http://%s:%d", lk.APIServerInsecureAddress.String(), lk.APIServerInsecurePort) } return "" }

Otherwise, you might end up calling GetAPIServerInsecureURL() with lk.APIServerInsecurePort == 0 and get

http://:0

It might as well give the default value of the return type instead of garbage, that way you can use it how you want to here.

r2d4 · 2017-09-28T21:35:09Z

pkg/localkube/ready.go

@@ -27,7 +27,26 @@ import (

 type HealthCheck func() bool

-func healthCheck(addr string, lk LocalkubeServer) HealthCheck {
+func healthCheckInsecure(addr string, lk LocalkubeServer) HealthCheck {


Can you refactor this to be more like

func healthCheck(addr, lk LocalkubeServer) { transport := lk.GetTransport() client := http.Client{Transport: transport} ... }

lk.GetTransport() will return a new http.Transport with the TLSconfig set if it is secure serving, otherwise just a http.Transport{} if not.

r2d4 · 2017-09-28T21:35:17Z

pkg/localkube/proxy.go

@@ -38,6 +40,11 @@ func (lk LocalkubeServer) NewProxyServer() Server {
 }

 func StartProxyServer(lk LocalkubeServer) func() error {
+	fmt.Println(lk.APIServerAddress)


r2d4 · 2017-09-28T21:36:24Z

pkg/localkube/apiserver.go

-	hostport := net.JoinHostPort("localhost", strconv.Itoa(lk.APIServerPort))
-	addr := "https://" + path.Join(hostport, "healthz")
-	return healthCheck(addr, lk)
+	if lk.APIServerInsecurePort != 0 {


see comments below

changed to only have one invocation of healtCheck

r2d4 · 2017-09-28T21:44:04Z

pkg/localkube/proxy.go

+	fmt.Println(lk.APIServerAddress)
+	bindaddress := lk.APIServerAddress.String()
+	if lk.APIServerInsecurePort != 0 {
+		bindaddress = lk.APIServerInsecureAddress.String()


Might be worth it to split this out into another helper function where the logic is clearer:

If insecure serving and and secure serving are both enabled, the insecure serving address will be used as the bind address.

like

func (LocalkubeServer) GetAPIServerAddress() net.IP { ... }

which you could use here, anywhere else that needs to know how to talk to the API server, and for the healthchecks

r2d4 · 2017-09-28T22:24:41Z

pkg/localkube/apiserver.go

-	return healthCheck(addr, lk)
+	protocol := "https://"
+	if lk.APIServerInsecurePort != 0 {
+		protocol = "http://"


I think you generally want to stay away from constructions like

default value if condition { overwrite default value }

Here is another place where you're implicitly hiding the behavior of
"if both secure and insecure serving are enabled, use the insecure serving port"
which is a bit arbitrary and I'd like to minimize the amount of places that logic has to live, so its easy to change/maintain/notice

This could live in the GetTransport function - since if you're adding a TLSConfig, you know that you'll be using https

r2d4 · 2017-09-28T22:25:13Z

pkg/localkube/ready.go

@@ -50,8 +50,8 @@ func healthCheck(addr string, lk LocalkubeServer) HealthCheck {
 			RootCAs:      caCertPool,
 		}
 		tlsConfig.BuildNameToCertificate()
-		transport := &http.Transport{TLSClientConfig: tlsConfig}
-		client := &http.Client{Transport: transport}
+		transport := lk.GetTransport()


This function doesn't exist yet - you'll have to rip out the relevant parts from the existing healthCheck function

…nd image

codecov-io · 2017-09-29T00:29:17Z

Codecov Report

Merging #1975 into master will decrease coverage by 0.21%.
The diff coverage is 20%.

@@            Coverage Diff            @@
##           master   #1975      +/-   ##
=========================================
- Coverage   29.81%   29.6%   -0.22%     
=========================================
  Files          77      77              
  Lines        4769    4810      +41     
=========================================
+ Hits         1422    1424       +2     
- Misses       3167    3204      +37     
- Partials      180     182       +2

Impacted Files	Coverage Δ
pkg/localkube/apiserver.go	`0% <0%> (ø)`	⬆️
pkg/localkube/proxy.go	`0% <0%> (ø)`	⬆️
pkg/localkube/ready.go	`48% <100%> (-5.85%)`	⬇️
pkg/localkube/localkube.go	`44.23% <20.75%> (-7.7%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d0c80d...ec17df3. Read the comment docs.

r2d4

once you remove the panic, LGTM

r2d4 · 2017-09-29T01:31:48Z

cmd/localkube/cmd/start.go

+	if s.ShouldGenerateKubeconfig {
+		if err := s.GenerateKubeconfig(); err != nil {
+			fmt.Println("Failed to create kubeconfig!")
+			panic(err)


handle this error by logging it to glog and exiting

sorry I didn't see this, merged the PR. ShouldGenerateCerts panics also. We should add a PR to change both of them.

aaron-prindle · 2017-09-29T01:39:21Z

PTAL

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 18, 2017

aaron-prindle force-pushed the sshd-dind branch 2 times, most recently from 21eefe8 to 2dbf0a0 Compare September 18, 2017 17:52

dlorenc reviewed Sep 18, 2017

View reviewed changes

aaron-prindle force-pushed the sshd-dind branch 3 times, most recently from 8abbf57 to 75ddc1e Compare September 28, 2017 21:29

r2d4 reviewed Sep 28, 2017

View reviewed changes

aaron-prindle force-pushed the sshd-dind branch 4 times, most recently from b12827d to 8043dbf Compare September 28, 2017 22:16

r2d4 reviewed Sep 28, 2017

View reviewed changes

aaron-prindle force-pushed the sshd-dind branch 5 times, most recently from 58e367d to 085edef Compare September 28, 2017 22:56

Added sshd, kubeconfig and insecure-serving flags to localkube for di…

ec17df3

…nd image

aaron-prindle force-pushed the sshd-dind branch from 085edef to ec17df3 Compare September 28, 2017 23:05

r2d4 approved these changes Sep 29, 2017

View reviewed changes

aaron-prindle merged commit 7abee78 into kubernetes:master Sep 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added sshd, kubeconfig and insecure-serving flags to localkube for di… #1975

Added sshd, kubeconfig and insecure-serving flags to localkube for di… #1975

aaron-prindle commented Sep 18, 2017

dlorenc Sep 18, 2017

aaron-prindle Sep 21, 2017

r2d4 Sep 28, 2017

aaron-prindle Sep 28, 2017

r2d4 Sep 28, 2017

aaron-prindle Sep 28, 2017

r2d4 Sep 28, 2017

r2d4 Sep 28, 2017

r2d4 Sep 28, 2017

aaron-prindle Sep 28, 2017

r2d4 Sep 28, 2017

r2d4 Sep 28, 2017

aaron-prindle Sep 28, 2017

r2d4 Sep 28, 2017

aaron-prindle Sep 28, 2017

r2d4 Sep 28, 2017

r2d4 Sep 28, 2017

aaron-prindle Sep 28, 2017

r2d4 Sep 28, 2017

r2d4 Sep 28, 2017

r2d4 Sep 28, 2017

aaron-prindle Sep 28, 2017

r2d4 Sep 28, 2017

codecov-io commented Sep 29, 2017

r2d4 left a comment

r2d4 Sep 29, 2017

aaron-prindle Sep 29, 2017 •

edited

Loading

aaron-prindle commented Sep 29, 2017

		APIServerName: constants.APIServerName,
		ShouldGenerateKubeconfig: true,

Added sshd, kubeconfig and insecure-serving flags to localkube for di… #1975

Added sshd, kubeconfig and insecure-serving flags to localkube for di… #1975

Conversation

aaron-prindle commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 29, 2017

Codecov Report

r2d4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-prindle Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

aaron-prindle commented Sep 29, 2017

aaron-prindle Sep 29, 2017 •

edited

Loading