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

🐛 Avoid the redirection to /healthz/ when calling /healthz #1134

Merged
merged 1 commit into from Aug 24, 2020

Conversation

0gajun
Copy link
Contributor

@0gajun 0gajun commented Aug 23, 2020

By #1100, subpaths of /healthz/ can be handled.
However, this change introduced the redirection from /healthz to /healthz/ with 301 (Moved Permanently) when accessing /healthz endpoint like following. (Accessing /healthz/ works fine without the redirect.)

→ curl -v -L http://localhost:8080/healthz
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> GET /healthz HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.54.0
> Accept: */*
>
< HTTP/1.1 301 Moved Permanently
< Content-Type: text/html; charset=utf-8
< Location: /healthz/
< Date: Sun, 23 Aug 2020 08:41:59 GMT
< Content-Length: 44
<
* Ignoring the response-body
* Connection #0 to host localhost left intact
* Issue another request to this URL: 'http://localhost:8080/healthz/'
* Found bundle for host localhost: 0x7fe067900c00 [can pipeline]
* Re-using existing connection! (#0) with host localhost
* Connected to localhost (::1) port 8080 (#0)
> GET /healthz/ HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.54.0
> Accept: */*
>
< HTTP/1.1 200 OK
< Content-Type: text/plain; charset=utf-8
< X-Content-Type-Options: nosniff
< Date: Sun, 23 Aug 2020 08:41:59 GMT
< Content-Length: 2
<
* Connection #0 to host localhost left intact
ok%

By this redirect behavior, the current implementation doesn't break anything, but I think that this is unnecessary overhead in the health checking.

So, this PR enables to access /healthz without the redirect with keeping that subpaths of /healthz/ are accessible.

By this PR, /healthz endpoint's response becomes like following.

→ curl -v -L http://localhost:8080/healthz
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> GET /healthz HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.54.0
> Accept: */*
>
< HTTP/1.1 200 OK
< Content-Type: text/plain; charset=utf-8
< X-Content-Type-Options: nosniff
< Date: Sun, 23 Aug 2020 08:59:45 GMT
< Content-Length: 2
<
* Connection #0 to host localhost left intact
ok%

→ curl -v -L http://localhost:8080/healthz/ping
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> GET /healthz/ping HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.54.0
> Accept: */*
>
< HTTP/1.1 200 OK
< Date: Sun, 23 Aug 2020 09:02:27 GMT
< Content-Length: 2
< Content-Type: text/plain; charset=utf-8
<
* Connection #0 to host localhost left intact
ok%

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 23, 2020
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 23, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @0gajun. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 23, 2020
defaultReadinessEndpoint = "/readyz/"
defaultLivenessEndpoint = "/healthz/"
defaultReadinessEndpoint = "/readyz"
defaultLivenessEndpoint = "/healthz"
Copy link
Contributor Author

@0gajun 0gajun Aug 23, 2020

Choose a reason for hiding this comment

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

These values are just default values.

Users can specify other paths here.

if options.LivenessEndpointName == "" {
options.LivenessEndpointName = defaultLivenessEndpoint
}

So, I think it's better to keep endpoint names consistent among readiness, liveness and metrics :)

@0gajun
Copy link
Contributor Author

0gajun commented Aug 23, 2020

/assign @mengqiy

@alvaroaleman
Copy link
Member

/cc @jimmidyson

Copy link
Member

@jimmidyson jimmidyson left a comment

Choose a reason for hiding this comment

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

Nit about no need to trim / in tests but otherwise LGTM

pkg/manager/manager_test.go Outdated Show resolved Hide resolved
pkg/manager/manager_test.go Outdated Show resolved Hide resolved
By kubernetes-sigs#1100,
subpaths of `/healthz/` can be handled.
However, this change introduced the redirection from `/healthz`
to `/healthz/` with 301 (Moved Permanently) when accessing `/healthz`
endpoint. (Accessing `/healthz/` works fine without the redirect.)

This is unnecessary overhead in the health checking.

So, this commit enable to access `/healthz` without the redirect with keeping
that subpaths of `/healthz/` are accessible.
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 24, 2020
@0gajun 0gajun requested a review from jimmidyson August 24, 2020 12:31
@jimmidyson
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 24, 2020
@jimmidyson
Copy link
Member

/approve

Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

Thanks @0gajun
/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 Aug 24, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0gajun, alvaroaleman, jimmidyson

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 Aug 24, 2020
@k8s-ci-robot k8s-ci-robot merged commit fdb6ef2 into kubernetes-sigs:master Aug 24, 2020
@0gajun 0gajun deleted the avoid_redirection branch August 24, 2020 15:34
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants