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

server: exempt healthcheck endpoint from authentication check #24945

Merged
merged 1 commit into from
May 16, 2018

Conversation

vilterp
Copy link
Contributor

@vilterp vilterp commented Apr 19, 2018

Otherwise, you get the red "connection lost" banner until you log in.

This will have no effect until the auth mux is put on the request path (#24944).

Fixes #24942

@vilterp vilterp requested review from couchand, mrtracy and a team April 19, 2018 22:38
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@vilterp vilterp removed the request for review from a team April 19, 2018 22:39
@mrtracy
Copy link
Contributor

mrtracy commented Apr 21, 2018

This is not the correct way to do this. In server/server.go, you'll see a block that registers path/handler combinations with the top level http mux, with lines such as:

s.mux.Handle(statusPrefix, authHandler)
s.mux.Handle(authPrefix, gwMux)

adminPrefix currently goes to authHandler, but add this line:

s.mux.Handle("/_admin/v1/health", gwMux)

And it will bypass the auth handler for just that one endpoint.


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@couchand
Copy link
Contributor

:lgtm:


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@couchand
Copy link
Contributor

bors r+

@craig
Copy link
Contributor

craig bot commented May 15, 2018

Build failed (retrying...)

@couchand
Copy link
Contributor

bors r+

@craig
Copy link
Contributor

craig bot commented May 15, 2018

Build failed (retrying...)

@vilterp
Copy link
Contributor Author

vilterp commented May 15, 2018 via email

@couchand couchand force-pushed the exempt-healthcheck branch from 4d14bf3 to 7c2d6f4 Compare May 16, 2018 20:37
@couchand
Copy link
Contributor

Rebased off of #25195 and updated TestHealthAPI to reflect the change.

@couchand couchand force-pushed the exempt-healthcheck branch from 7c2d6f4 to f0eab2e Compare May 16, 2018 20:51
@couchand
Copy link
Contributor

bors r+

craig bot pushed a commit that referenced this pull request May 16, 2018
24945: server: exempt healthcheck endpoint from authentication check r=couchand a=vilterp

Otherwise, you get the red "connection lost" banner until you log in.

This will have no effect until the auth mux is put on the request path (#24944).

Fixes #24942 

25581: storage: fix use of context with closed trace r=a-robinson a=a-robinson

Fixes #25575

Release note: None

Co-authored-by: Pete Vilter <[email protected]>
Co-authored-by: Alex Robinson <[email protected]>
@craig
Copy link
Contributor

craig bot commented May 16, 2018

Build succeeded

@craig craig bot merged commit f0eab2e into cockroachdb:master May 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants