metrics: Add nil check for metricsHandler in AdminMetrics.serveHTTP#7553
Open
Br1an67 wants to merge 2 commits intocaddyserver:masterfrom
Open
metrics: Add nil check for metricsHandler in AdminMetrics.serveHTTP#7553Br1an67 wants to merge 2 commits intocaddyserver:masterfrom
Br1an67 wants to merge 2 commits intocaddyserver:masterfrom
Conversation
Prevents panic when the admin metrics endpoint is accessed before the module is fully provisioned. Returns a proper API error instead of crashing.
|
|
Member
|
Should we be provisioning Also can you sign the CLA? Thanks! |
Instead of adding a nil check for metricsHandler, address the root cause by provisioning admin router modules before calling Routes(). This ensures all handler state is initialized before routes are registered on the mux. Merge newAdminHandler and provisionAdminRouters into a single step, removing the two-phase setup where routes were registered first and modules provisioned later. The AdminConfig.routers field is no longer needed since provisioning happens inline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7079
Summary
This PR fixes a panic that occurs in the admin metrics endpoint when the
AdminMetricsmodule'sserveHTTPmethod is called before the module is fully provisioned. The panic was causing a crash loop when invalid TLS configurations were POST-ed via the admin API.Root Cause
The
serveHTTPmethod calledm.metricsHandler.ServeHTTP(w, r)without checking ifmetricsHandlerwas nil. This field is only set during theProvisionphase, but routes are registered before provisioning completes. If a request hits the endpoint before provisioning finishes (or if provisioning fails), a nil pointer dereference causes a panic.Changes
m.metricsHandlerin theserveHTTPmethodcaddy.APIErrorwith HTTP status 500 instead of panickingTesting
go build ./...Assistance Disclosure
The fix was authored with the assistance of Claude. The solution was verified to be correct through build and test execution.