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

Stop using insecure serving. #1694

Merged
merged 1 commit into from
Jul 26, 2017
Merged

Stop using insecure serving. #1694

merged 1 commit into from
Jul 26, 2017

Conversation

dlorenc
Copy link
Contributor

@dlorenc dlorenc commented Jul 13, 2017

This is mostly done. I want to add an integration test to verify nothing is serving on 8080.

I had to modify addon-manager.yaml to pass in the kubeconfig, I'm not sure how other environments do this. I wasn't able to find any examples.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 13, 2017
// hostport := net.JoinHostPort(lk.APIServerInsecureAddress.String(), strconv.Itoa(lk.APIServerInsecurePort))
// addr := "http://" + path.Join(hostport, "healthz")
return noop
// return healthCheck(addr)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to get turned back on over the new address.

@codecov-io
Copy link

codecov-io commented Jul 13, 2017

Codecov Report

Merging #1694 into master will increase coverage by 0.52%.
The diff coverage is 57.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1694      +/-   ##
==========================================
+ Coverage   36.76%   37.29%   +0.52%     
==========================================
  Files          51       51              
  Lines        3321     3365      +44     
==========================================
+ Hits         1221     1255      +34     
- Misses       1920     1928       +8     
- Partials      180      182       +2
Impacted Files Coverage Δ
pkg/localkube/kubelet.go 0% <0%> (ø) ⬆️
pkg/util/constants.go 0% <0%> (ø) ⬆️
pkg/localkube/storage_provisioner.go 0% <0%> (ø) ⬆️
pkg/localkube/controller-manager.go 0% <0%> (ø) ⬆️
pkg/localkube/proxy.go 0% <0%> (ø) ⬆️
pkg/localkube/scheduler.go 0% <0%> (ø) ⬆️
pkg/localkube/apiserver.go 0% <0%> (ø) ⬆️
pkg/minikube/cluster/cluster.go 43.37% <100%> (+2.24%) ⬆️
pkg/localkube/ready.go 53.84% <65.21%> (+9.4%) ⬆️
pkg/util/kubeconfig/config.go 48.29% <73.33%> (+1.44%) ⬆️

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 45eabf6...3336c24. Read the comment docs.

@@ -29,7 +30,8 @@ func StartKubeletServer(lk LocalkubeServer) func() error {
config := options.NewKubeletServer()

// Master details
config.APIServerList = []string{lk.GetAPIServerInsecureURL()}
config.KubeConfig = flag.NewStringFlag("/var/lib/localkube/kubeconfig")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be pointing at util.DefaultKubeConfigPath?

@r2d4
Copy link
Contributor

r2d4 commented Jul 13, 2017

Fixes #1628

@dlorenc
Copy link
Contributor Author

dlorenc commented Jul 26, 2017

This is ready for a final look.

@dlorenc dlorenc merged commit 8837045 into kubernetes:master Jul 26, 2017
@dlorenc dlorenc deleted the noinsecure branch July 26, 2017 21:39
config.InsecureServing.BindAddress = lk.APIServerInsecureAddress
config.InsecureServing.BindPort = lk.APIServerInsecurePort
// 0 turns off insecure serving.
config.InsecureServing.BindPort = 0
Copy link
Contributor

@aaron-prindle aaron-prindle Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also change the default to be 0 as this option might want to be configured by users. I am not sure if this is something users might rely on, the insecure api-server but I am seeing issues with the --none driver currently

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants