change health-check port to listen to node port addresses #104742

khenidak · 2021-09-02T23:30:00Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

kube-proxy health check ports used to listen to :<port> for each of the services. This is not needed and opens ports in addresses the cluster user may not have intended. The PR limits listening to all node address which are controlled by --nodeport-addresses flag.

Which issue(s) this PR fixes:

Fixes #103184

Special notes for your reviewer:

WIP don't review yet.

Does this PR introduce a user-facing change?

kube-proxy health check ports used to listen to :<port> for each of the services. This is not needed and opens ports in addresses the cluster user may not have intended. The PR limits listening to all node address which are controlled by `--nodeport-addresses` flag. if no addresses are provided then we default to existing behavior by listening to :<port> for each service

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/sig network
/hold

k8s-ci-robot · 2021-09-02T23:30:07Z

@khenidak: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aojea · 2021-09-05T20:07:24Z

the unit test failing seems related

    healthcheck_test.go:154: expected 1 open port, got 0
        (sets.String) {
        }
    healthcheck_test.go:157: expected port :9376 to be open
        (sets.String) {
        }
panic: interface conversion: healthcheck.httpServer is nil, not *healthcheck.fakeHTTPServer [recovered]
	panic: interface conversion: healthcheck.httpServer is nil, not *healthcheck.fakeHTTPServer

aojea · 2021-09-05T20:22:59Z

I think this should be aligned to current logic

kubernetes/pkg/proxy/iptables/proxier.go

Lines 996 to 1000 in eb72962

    
           localAddrSet := utilproxy.GetLocalAddrSet() 
        
           nodeAddresses, err := utilproxy.GetNodeAddresses(proxier.nodePortAddresses, proxier.networkInterfacer) 
        
           if err != nil { 
        
           	klog.ErrorS(err, "Failed to get node ip address matching nodeport cidrs, services with nodeport may not work as intended", "CIDRs", proxier.nodePortAddresses) 
        
           }

khenidak · 2021-09-07T22:22:31Z

Yes. I kept that one failing until i clean up the old code. The current commits are the change i want to make. But i am not happy with unit testing coverage so i am keeping the wip tag on until i figure out a way to cover selective addresses

khenidak · 2021-09-09T17:29:57Z

@danwinship @aojea -- Can you help reviewing this? (it is ready now)

khenidak · 2021-09-09T19:07:36Z

/test pull-kubernetes-unit

khenidak · 2021-09-09T20:44:07Z

/test pull-kubernetes-unit

danwinship · 2021-09-10T18:00:40Z

This is not needed and opens ports in addresses the cluster user may not have indented

"intended" 🙂
(in the release note text)

danwinship

mostly looks good

pkg/proxy/healthcheck/healthcheck_test.go

pkg/proxy/healthcheck/service_health.go

khenidak · 2021-09-11T14:24:56Z

/test pull-kubernetes-e2e-gci-gce-ipvs

danwinship · 2021-09-13T14:11:30Z

/lgtm
but not un-holding it since I'm not sure if there was some other reason for that

khenidak · 2021-09-13T15:12:01Z

The hold is to get as many eyes as possible on it. One can argue it is a behavior change so i am not sure how to deal with this one. There is no api change, just behavior change. Do we need anything more than release notes?

@thockin ?

pkg/proxy/healthcheck/service_health.go

pkg/proxy/healthcheck/healthcheck_test.go

pkg/proxy/healthcheck/service_health.go

thockin

This seems OK to me, but I wonder if there are use-cases we're missing. I guess we'll find out. I expected to see map[types.NamespacedName][]*hcInstance but this works, too :)

/lgtm
/approve

k8s-ci-robot · 2021-09-13T19:27:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: khenidak, thockin

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:

~~pkg/proxy/OWNERS~~ [khenidak,thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

khenidak · 2021-09-13T20:43:04Z

/unhold

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 2, 2021

k8s-ci-robot requested review from aojea and m1093782566 September 2, 2021 23:33

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2021

khenidak force-pushed the health-check-port branch from 1cabef5 to e29d0c1 Compare September 7, 2021 22:15

k8s-ci-robot added the area/ipvs label Sep 7, 2021

khenidak force-pushed the health-check-port branch from e29d0c1 to ea34c0b Compare September 9, 2021 17:13

khenidak changed the title ~~WIP: change health-check port to listen to node port addresses~~ change health-check port to listen to node port addresses Sep 9, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 9, 2021

khenidak force-pushed the health-check-port branch from ea34c0b to aa0c88b Compare September 9, 2021 18:34

khenidak force-pushed the health-check-port branch from aa0c88b to aa8078e Compare September 10, 2021 17:06

danwinship reviewed Sep 10, 2021

View reviewed changes

pkg/proxy/healthcheck/healthcheck_test.go Outdated Show resolved Hide resolved

pkg/proxy/healthcheck/service_health.go Outdated Show resolved Hide resolved

khenidak mentioned this pull request Sep 10, 2021

Simplify and de-layer Service REST implementation #96684

Merged

khenidak force-pushed the health-check-port branch from aa8078e to 831a21f Compare September 10, 2021 23:38

k8s-ci-robot assigned danwinship Sep 13, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2021