Move host-network services off of port 8080 #9355
Conversation
k8s-ci-robot
added
cncf-cla: yes
| @@ -129,7 +130,7 @@ func (s *healthCheckServer) proxyRequest(w http.ResponseWriter, forwardRequest * | |||
| } | |||
|
|
|||
| func run() error { | |||
| listen := ":8080" | |||
| listen := fmt.Sprintf(":%d", wellknownports.KubeAPIServerHealthCheck) | |||
There was a problem hiding this comment.
Just to make sure, does the health check have to listen to all ips? I did a quick check to see where reqs were coming from and seemed to be all localhost. It was a very quick test though.
There was a problem hiding this comment.
This PR is not changing that aspect.
| @@ -129,7 +130,7 @@ func (s *healthCheckServer) proxyRequest(w http.ResponseWriter, forwardRequest * | |||
| } | |||
|
|
|||
| func run() error { | |||
| listen := ":8080" | |||
| listen := fmt.Sprintf(":%d", wellknownports.KubeAPIServerHealthCheck) | |||
| @@ -14233,7 +14233,7 @@ data: | |||
| force_tcp | |||
| } | |||
| prometheus :9253 | |||
| health {{ KubeDNS.NodeLocalDNS.LocalIP }}:8080 | |||
| health {{ KubeDNS.NodeLocalDNS.LocalIP }}:{{ NodeLocalDNSHealthCheck }} | |||
There was a problem hiding this comment.
I was wondering if this should be configurable in the cluster spec. Then you can use the well-known port as default in components/kubedns.go instead of adding the template function. Just a thought.
There was a problem hiding this comment.
What is the use case for making this port configurable?
Incidentally, I wonder if we could have kops automatically default the value of clusterDNS in kubelet and masterKubelet to 169.254.20.10 when needed rather than just mentioning it in documentation.
There was a problem hiding this comment.
Well. Both can conflict with other things, so it isn't unreasonable to have this configurable, I think.
There was a problem hiding this comment.
Our approach is to reserve ports in wellknownports. If anything else runs in host network and conflicts with those, the other things are what have to move.
There was a problem hiding this comment.
It is still a breaking change in kops. That said, it is something we need to do, so maybe we should just reserve a range of ports for kops usage.
I think it's a good idea also to make it as easy as possible to turn on NodeLocalDNS as possible. Can we change the defaulting logic when nodelocal dns is enabled, but continue to honor an explicit value if the user sets one?
|
Can confirm that this fixes #9245 |
| @@ -126,7 +127,7 @@ spec: | |||
| func (b *KubeApiserverBuilder) buildHealthcheckSidecar() (*corev1.Pod, error) { | |||
| // TODO: pull from bundle | |||
| bundle := "(embedded kube-apiserver-healthcheck manifest)" | |||
| manifest := []byte(defaultManifest) | |||
| manifest := []byte(fmt.Sprintf(defaultManifest, wellknownports.KubeAPIServerHealthCheck)) | |||
There was a problem hiding this comment.
Nit: I normally try to avoid using Sprintf with our templates, favoring {{ HealthCheckPort }} and strings.ReplaceAll, because the %d is harder to read, and has more gotchas (repeated placeholders don't work, it's vaguely possible that a % placeholder could appear 'accidentally')
| @@ -14233,7 +14233,7 @@ data: | |||
| force_tcp | |||
| } | |||
| prometheus :9253 | |||
| health {{ KubeDNS.NodeLocalDNS.LocalIP }}:8080 | |||
| health {{ KubeDNS.NodeLocalDNS.LocalIP }}:{{ NodeLocalDNSHealthCheck }} | |||
There was a problem hiding this comment.
It is still a breaking change in kops. That said, it is something we need to do, so maybe we should just reserve a range of ports for kops usage.
I think it's a good idea also to make it as easy as possible to turn on NodeLocalDNS as possible. Can we change the defaulting logic when nodelocal dns is enabled, but continue to honor an explicit value if the user sets one?
|
Looks good to me; one nit that /approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johngmyers, justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/retest |
|
/retest Review the full test history for this PR. Silence the bot with an |
1 similar comment
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest |
1 similar comment
|
/retest |
|
@justinsb I thought I do think we should enable NodeLocalDNS by default when using CoreDNS. |
…55-upstream-release-1.18 Automated cherry pick of #9355: Move kube-apiserver-healthcheck to port 3990
I am unable to test the node-local-dns change.