Fix content headers in Health Check Node Port response #26458

cezarygerard · 2023-06-23T11:52:09Z

Fix content headers in Health Check Node Port response.

Before the fix 2 content headers are not returned, because w.WriteHeader(http.StatusOK) must be called after setting the headers map

Fix: Return "Content-Type" and "X-Content-Type-Options" headers from  Health Check Node Port

maintainer-s-little-helper · 2023-06-23T11:52:11Z

Commit 19e3244f61b09fc21bbb67f713685cecc80c56da does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

before the fix 2 content headers are not returned ```release-note Fix: Return "Content-Type" and "X-Content-Type-Options" headers from Health Check Node Port ``` Signed-off-by: Cezary Zawadka <czawadka@google.com>

dlapcevic

/lgtm

borkmann · 2023-06-23T12:30:26Z

/test

aojea · 2023-06-23T12:45:46Z

pkg/service/healthserver/healthserver_test.go

@@ -8,6 +8,7 @@ import (
 	"net/http/httptest"
 	"testing"

+	"github.com/google/go-cmp/cmp"


you need to run gofmt on this file

ake: *** [Makefile:645: precheck] Error 1 Unformatted Go source code: make: *** Waiting for unfinished jobs.... ./pkg/service/healthserver/healthserver_test.go diff ./pkg/service/healthserver/healthserver_test.go.orig ./pkg/service/healthserver/healthserver_test.go --- ./pkg/service/healthserver/healthserver_test.go.orig +++ ./pkg/service/healthserver/healthserver_test.go @@ -8,8 +8,8 @@ "net/http/httptest" "testing" - "github.com/google/go-cmp/cmp" . "github.com/cilium/checkmate" + "github.com/google/go-cmp/cmp" ) type ServiceHealthServerSuite struct{} @@ -94,7 +94,7 @@ assertRespHeader(c, resp, "Content-Type", "application/json") assertRespHeader(c, resp, "X-Content-Type-Options", "nosniff") resp.Body.Close() - +

aojea · 2023-06-23T12:55:26Z

pkg/service/healthserver/healthserver.go

 	if svc.LocalEndpoints == 0 {
 		w.WriteHeader(http.StatusServiceUnavailable)
 	} else {
 		w.WriteHeader(http.StatusOK)
 	}
-	w.Header().Set("Content-Type", "application/json")


These are trailers https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Trailer

https://pkg.go.dev/net/http#example-ResponseWriter-Trailers

I do not know if this is on purpose though

does not look that using trailers instead of headers are on purpose, and using headers is coherent with kube-proxy https://github.com/kubernetes/kubernetes/blob/027ac5a426a261ba6b66a40e79e123e75e9baf5b/pkg/proxy/healthcheck/proxier_health.go#L159-L173 , since this is a "replacement" it makes sense to be consistent.
Also, the failing test indicates that the goal may have been to use headers in the first place

similar code in kube-proxy uses regular headers, not trailers

https://github.com/kubernetes/kubernetes/blob/027ac5a426a261ba6b66a40e79e123e75e9baf5b/pkg/proxy/healthcheck/service_health.go#L236

Signed-off-by: Cezary Zawadka <czawadka@google.com>

aojea · 2023-06-26T11:51:20Z

/lgtm
/assign @borkmann @aanm

borkmann · 2023-06-26T12:11:58Z

/test

gandro

Looks good to me. Did this actually cause issues or how did you find this?

ldelossa · 2023-06-26T14:16:46Z

All required tests passing, and multiple committers approved. Merging 🚀

aojea · 2023-06-26T22:49:57Z

Looks good to me. Did this actually cause issues or how did you find this?

google loadbalancers can use the headers for passing the data of the existing endpoints on the node and this way use weights, the headers were not coming at all 😄

cezarygerard requested a review from a team as a code owner June 23, 2023 11:52

cezarygerard requested a review from ysksuzuki June 23, 2023 11:52

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 23, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jun 23, 2023

cezarygerard force-pushed the main branch from 19e3244 to 892bd58 Compare June 23, 2023 11:54

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jun 23, 2023

Fix content headers in Health Check Node Port response

3f25493

before the fix 2 content headers are not returned ```release-note Fix: Return "Content-Type" and "X-Content-Type-Options" headers from Health Check Node Port ``` Signed-off-by: Cezary Zawadka <czawadka@google.com>

cezarygerard force-pushed the main branch from 892bd58 to 3f25493 Compare June 23, 2023 11:58

dlapcevic approved these changes Jun 23, 2023

View reviewed changes

borkmann approved these changes Jun 23, 2023

View reviewed changes

borkmann added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jun 23, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 23, 2023

aojea reviewed Jun 23, 2023

View reviewed changes

Update healthserver_test.go

af1e736

Signed-off-by: Cezary Zawadka <czawadka@google.com>

borkmann added release-blocker/1.14 This issue will prevent the release of the next version of Cilium. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Jun 26, 2023

maintainer-s-little-helper bot added this to Needs backport from main in 1.13.5 Jun 26, 2023

brb approved these changes Jun 26, 2023

View reviewed changes

ysksuzuki approved these changes Jun 26, 2023

View reviewed changes

gandro approved these changes Jun 26, 2023

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 26, 2023

ldelossa merged commit fc105dd into cilium:main Jun 26, 2023
64 of 65 checks passed

tklauser mentioned this pull request Jun 28, 2023

v1.13 Backports 2023-06-28 #26528

Merged

3 tasks

tklauser added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Jun 28, 2023

maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.13 in 1.13.5 Jun 28, 2023

tklauser added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Jun 28, 2023

maintainer-s-little-helper bot moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.5 Jun 28, 2023

joestringer mentioned this pull request Jun 28, 2023

Prepare for release v1.14.0-rc.0 #26544

Merged

gentoo-root mentioned this pull request Jul 26, 2023

Prepare for release v1.13.5 #27088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix content headers in Health Check Node Port response #26458

Fix content headers in Health Check Node Port response #26458

cezarygerard commented Jun 23, 2023 •

edited

maintainer-s-little-helper bot commented Jun 23, 2023

dlapcevic left a comment

borkmann commented Jun 23, 2023

aojea Jun 23, 2023

cezarygerard Jun 26, 2023

aojea Jun 23, 2023

aojea Jun 26, 2023

cezarygerard Jun 26, 2023

aojea commented Jun 26, 2023

borkmann commented Jun 26, 2023

gandro left a comment

ldelossa commented Jun 26, 2023

aojea commented Jun 26, 2023

Fix content headers in Health Check Node Port response #26458

Fix content headers in Health Check Node Port response #26458

Conversation

cezarygerard commented Jun 23, 2023 • edited

maintainer-s-little-helper bot commented Jun 23, 2023

dlapcevic left a comment

Choose a reason for hiding this comment

borkmann commented Jun 23, 2023

aojea Jun 23, 2023

Choose a reason for hiding this comment

cezarygerard Jun 26, 2023

Choose a reason for hiding this comment

aojea Jun 23, 2023

Choose a reason for hiding this comment

aojea Jun 26, 2023

Choose a reason for hiding this comment

cezarygerard Jun 26, 2023

Choose a reason for hiding this comment

aojea commented Jun 26, 2023

borkmann commented Jun 26, 2023

gandro left a comment

Choose a reason for hiding this comment

ldelossa commented Jun 26, 2023

aojea commented Jun 26, 2023

cezarygerard commented Jun 23, 2023 •

edited