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.

Sign up for GitHub

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

Jump to bottom

Update usages of http.ResponseWriter.WriteHeader to use http.Error #69013

Merged
merged 1 commit into from
Oct 23, 2018
Merged

Update usages of http.ResponseWriter.WriteHeader to use http.Error #69013

merged 1 commit into from
Oct 23, 2018

Conversation

ibrasho
Copy link
Contributor

@ibrasho ibrasho commented Sep 24, 2018 •
edited
Loading

What this PR does / why we need it:
Replaces most usages of http.ResponseWrite.WriteHeader with http.Error to ensure Content-Type and X-Content-Type-Options headers are set.

Which issue(s) this PR fixes
fixes #69011

Special notes for your reviewer:

  • Should we add a message in the empty responses?
  • Any other places that should be updated?

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. area/apiserver needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. area/kubeadm area/kubectl area/kubelet sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 24, 2018
@ibrasho ibrasho changed the title Update usages of http.ResponseWriter.WriteCode to use http.Error Update usages of http.ResponseWriter.WriteHeader to use http.Error Sep 24, 2018
@neolit123
Copy link
Member

@ibrasho
thanks!

/ok-to-test
/kind cleanup

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 25, 2018
@BenTheElder
Copy link
Member

please run (and commit) hack/update-bazel.sh or hack/update-all.sh. thanks for working on this!

@ibrasho ibrasho force-pushed the switch-to-http.Error branch 4 times, most recently from 217d7db to 448c878 Compare September 25, 2018 19:43
@k8s-ci-robot k8s-ci-robot added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Sep 25, 2018
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 10, 2018
@ibrasho ibrasho force-pushed the switch-to-http.Error branch 2 times, most recently from 5530727 to e412cba Compare October 12, 2018 14:51
@ibrasho
Copy link
Contributor Author

ibrasho commented Oct 12, 2018

Seems like the failures were flakes.
I'm squashing and rebasing again.

@k8s-ci-robot k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Oct 12, 2018
@ibrasho ibrasho force-pushed the switch-to-http.Error branch from e412cba to 51f070e Compare October 12, 2018 14:54
@tallclair
Copy link
Member

/lgtm

Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2018
liggitt
liggitt reviewed Oct 12, 2018
View reviewed changes
pkg/proxy/healthcheck/healthcheck.go Outdated
@@ -211,7 +211,7 @@ func (h hcHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request) {

resp.Header().Set("Content-Type", "application/json")
if count == 0 {
resp.WriteHeader(http.StatusServiceUnavailable)
http.Error(resp, "", http.StatusServiceUnavailable)
Copy link
Member

Choose a reason for hiding this comment

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

this resets the content type and makes the following json an invalid response, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

localEndpoints will be equal to 0 in case of an error. Is that the right behavior?

Copy link
Member

Choose a reason for hiding this comment

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

localEndpoints will be equal to 0 in case of an error. Is that the right behavior?

that would be a good question for @kubernetes/sig-network-api-reviews, but my leaning would be toward keeping the current behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the PR to keep the current behavior here.

liggitt
liggitt reviewed Oct 12, 2018
View reviewed changes
pkg/proxy/healthcheck/healthcheck.go Outdated
@@ -339,7 +339,7 @@ func (h healthzHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request) {

resp.Header().Set("Content-Type", "application/json")
if !lastUpdated.IsZero() && currentTime.After(lastUpdated.Add(h.hs.healthTimeout)) {
resp.WriteHeader(http.StatusServiceUnavailable)
http.Error(resp, "", http.StatusServiceUnavailable)
Copy link
Member

Choose a reason for hiding this comment

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

same here, we shouldn't send json after resetting to text/plain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same thing here. In case of an error, does the current response make sense?

Copy link
Member

Choose a reason for hiding this comment

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

will defer to @kubernetes/sig-network-api-reviews as well, or we can restore current behavior and work through a change here in a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the PR to keep the current behavior here.

@tallclair
Copy link
Member

/lgtm cancel
Agree with @liggitt 's comments.

@k8s-ci-robot k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 12, 2018
@ibrasho ibrasho force-pushed the switch-to-http.Error branch from 51f070e to 039e0fa Compare October 14, 2018 02:45
@ibrasho
Copy link
Contributor Author

ibrasho commented Oct 14, 2018

I've updated the PR to keep the current behavior in pkg/proxy/healthcheck/healthcheck.go as per @liggitt comments.

@ibrasho ibrasho force-pushed the switch-to-http.Error branch from 039e0fa to 31a6f85 Compare October 14, 2018 02:52
@ibrasho
Update usages of http.ResponseWriter.WriteHeader to use http.Error
2fb3ba7 
Signed-off-by: Ibrahim AshShohail <me@ibrasho.com>
@ibrasho ibrasho force-pushed the switch-to-http.Error branch from 31a6f85 to 2fb3ba7 Compare October 14, 2018 02:58
@liggitt
Copy link
Member

liggitt commented Oct 15, 2018

/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 Oct 15, 2018
@tallclair
Copy link
Member

BTW, top-level pkg/ approval is required because pkg/probe is missing an owners file. I opened #69822 to add one.

@smarterclayton
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ibrasho, liggitt, smarterclayton

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 Oct 23, 2018
@k8s-ci-robot k8s-ci-robot merged commit dad0768 into kubernetes:master Oct 23, 2018
@liggitt liggitt mentioned this pull request Oct 25, 2018
Closed
@ibrasho ibrasho mentioned this pull request Jan 14, 2019
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tallclair tallclair tallclair left review comments

@liggitt liggitt liggitt left review comments

@BenTheElder BenTheElder BenTheElder left review comments

@bowei bowei Awaiting requested review from bowei

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

Assignees

@liggitt liggitt

@smarterclayton smarterclayton

@tallclair tallclair

@jennybuckley jennybuckley

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/code-generation area/kubeadm area/kubectl area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use http.Error() instead of writing responses directly
9 participants
@ibrasho @neolit123 @BenTheElder @fedebongio @tallclair @liggitt @smarterclayton @k8s-ci-robot @jennybuckley