-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix: disable health check request logs (admin) #3496
Conversation
The excluded path is wrong
Codecov Report
@@ Coverage Diff @@
## master #3496 +/- ##
==========================================
- Coverage 76.87% 76.85% -0.03%
==========================================
Files 124 124
Lines 9175 9175
==========================================
- Hits 7053 7051 -2
- Misses 1673 1674 +1
- Partials 449 450 +1
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -201,7 +201,7 @@ func setup(ctx context.Context, d driver.Registry, cmd *cobra.Command) (admin *h | |||
NewMiddlewareFromLogger(d.Logger(), | |||
fmt.Sprintf("hydra/admin: %s", d.Config().IssuerURL(ctx).String())) | |||
if d.Config().DisableHealthAccessLog(config.AdminInterface) { | |||
adminLogger = adminLogger.ExcludePaths("/admin"+healthx.AliveCheckPath, "/admin"+healthx.ReadyCheckPath) | |||
adminLogger = adminLogger.ExcludePaths(healthx.AliveCheckPath, healthx.ReadyCheckPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about this? The path is - if I recall correctly - /admin/health/status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure the path is actually /health/alive
when testing the endpoints manually i get a proper response:
curl localhost:4445/health/alive
{"status":"ok"}
while localhost:4445/admin/health/status
returns a 404.
this path is also used by the helm chart:
https://github.com/ory/k8s/blob/master/helm/charts/hydra/templates/deployment.yaml#L92-L95
The excluded path is wrong
Related issue(s)
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments