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

kube-proxy service health: add new header with number of local endpoints #118999

Merged
merged 1 commit into from Jul 7, 2023

Conversation

cezarygerard
Copy link
Contributor

@cezarygerard cezarygerard commented Jun 30, 2023

/kind feature

What this PR does / why we need it:

  • add new header "X-Load-Balancing-Endpoint-Weight" returned from service health. Value of the header is number of local endpoints. Header can be used in weighted load balancing. Parsing header for number of endpoints is faster than unmarshalling json from the content body.

  • add missing unit test for new and old headers returned from service health

Special notes for your reviewer:

I am happy to discuss this on SIG Network meeting

Does this PR introduce a user-facing change?

yes, to some extend
kube-proxy service health will return additional http header "X-Load-Balancing-Endpoint-Weight" with number of local endpoints.
no action is required from users

kube-proxy service health returns http header "X-Load-Balancing-Endpoint-Weight" with number of local endpoints. The same information is still available in response body JSON payload.LocalEndpoints.

… endpoints

- add new header "X-Load-Balancing-Endpoint-Weight" returned from service health. Value of the header is number of local endpoints. Header can be used in weighted load balancing. Parsing header for number of endpoints is faster than unmarshalling json from the content body.

- add missing unit test for new and old headers returned from service health
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 30, 2023
@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jun 30, 2023
@k8s-ci-robot k8s-ci-robot added area/kube-proxy sig/network Categorizes an issue or PR as relevant to SIG Network. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 30, 2023
@aojea
Copy link
Member

aojea commented Jun 30, 2023

/assign @thockin @danwinship @khenidak

I talked with Cezary offline about this, and the header seems generic enough and not used anywhere that all new implementations can benefit from it.

It LGTM but waiting for others to have consensus

@thockin
Copy link
Member

thockin commented Jun 30, 2023

I'm OK with this, but open to discussion. We could add a flag to set the header name, but I'd only do that if we have a concrete use-case.

We would want other proxier implementations to adopt this, too, right? Cilium, Antrea, ... who else? @rikatz

Can you bring this to sig-net in an upcoming meeting?

@rikatz
Copy link
Contributor

rikatz commented Jun 30, 2023

should mark @tgraf @antoninbas and probably @caseydavenport for the proxiers of each CNI :)

@danwinship
Copy link
Contributor

danwinship commented Jul 3, 2023

The HealthCheckNodePort check returns a JSON result. We could add this there.
We already include this there:

fmt.Fprint(resp, strings.Trim(dedent.Dedent(fmt.Sprintf(`
	{
		"service": {
			"namespace": %q,
			"name": %q
		},
		"localEndpoints": %d,
		"serviceProxyHealthy": %v
	}
	`, h.name.Namespace, h.name.Name, count, kubeProxyHealthy)), "\n"))

@cezarygerard
Copy link
Contributor Author

@danwinship

I know the value from the header can already be parsed out from the body
but header is easier to parse for Load Balancers

@cezarygerard
Copy link
Contributor Author

One more use case suggested by @thockin is HTTP HEAD method that can be used to check the health.

Current implementation of the service healthchecks supports the HEAD method, I guess this comes for free with "net/http" implementation.

Example from my k8s node:

$curl --head localhost:32478

HTTP/1.1 200 OK
Content-Type: application/json
X-Content-Type-Options: nosniff
Date: Thu, 06 Jul 2023 16:47:07 GMT
Content-Length: 104

cezarygerard added a commit to cezarygerard/website that referenced this pull request Jul 7, 2023
describe Healcth check node port response

add info about new response header  in 1.28 kubernetes/kubernetes#118999

This was discussed on SIG Network on 6th July 2023
cezarygerard added a commit to cezarygerard/website that referenced this pull request Jul 7, 2023
describe Healcth check node port response

add info about new response header  in 1.28 kubernetes/kubernetes#118999

This was discussed on SIG Network on 6th July 2023
@cezarygerard
Copy link
Contributor Author

@danwinship

added draft PR with documentation update to https://kubernetes.io/docs/concepts/services-networking/service/

kubernetes/website#41926

Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 8daa96f2294288354ad2d26baea4272cef690287

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cezarygerard, 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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2023
@k8s-ci-robot k8s-ci-robot merged commit 2d42274 into kubernetes:master Jul 7, 2023
12 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kube-proxy cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/network Categorizes an issue or PR as relevant to SIG Network. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants