Skip to content

Change server information endpoint / to only accept GET and HEAD requests #15976

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

Merged
merged 12 commits into from
Mar 5, 2025
Merged

Change server information endpoint / to only accept GET and HEAD requests #15976

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
559d8f4
Change route / to only accept GET and HEAD method
carsonip Mar 3, 2025
16cab2a
Update changelog
carsonip Mar 3, 2025
f13131f
Verbose error message
carsonip Mar 3, 2025
9d4122f
Add to all breaking changes
carsonip Mar 3, 2025
bc091f7
Naming
carsonip Mar 3, 2025
3ee70b7
Remove from openapi spec
carsonip Mar 3, 2025
4e13d0e
Improve docs
carsonip Mar 3, 2025
9a50ac8
Remove changelog to asciidoc
carsonip Mar 4, 2025
c3781f2
Fix test
carsonip Mar 4, 2025
9a55507
Naming
carsonip Mar 5, 2025
8e3ed8c
Merge branch 'main' into root-get-head-methods-only
carsonip Mar 5, 2025
24a50f9
Merge branch 'main' into root-get-head-methods-only
carsonip Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelogs/9.0.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ https://github.com/elastic/apm-server/compare/v\...v9.0.0[View commits]
[float]
==== Breaking Changes
- Change `sampling.tail.storage_limit` default to `0`. While `0` means unlimited local tail-sampling database size, it now enforces a max 80% disk usage on the disk where the data directory is located. Any tail sampling writes after this threshold will be rejected, similar to what happens when tail-sampling database size exceeds a non-0 storage limit. Setting `sampling.tail.storage_limit` to non-0 maintains the existing behavior which limits the tail-sampling database size to `sampling.tail.storage_limit` and does not have the new disk usage threshold check. {pull}15467[15467] {pull}15524[15524]
- Change server information endpoint `/` to only accept GET and HEAD requests, otherwise return HTTP 405 Method Not Allowed.

[float]
==== Deprecations
Expand Down
4 changes: 4 additions & 0 deletions changelogs/all-breaking-changes.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ which limits the tail-sampling database size to `sampling.tail.storage_limit`
and does not have the new disk usage threshold check.
For more details, see https://github.com/elastic/apm-server/pull/15467[PR #15467] and
https://github.com/elastic/apm-server/pull/15524[PR #15524]
- Change server information endpoint `/` to only accept GET and HEAD requests.
Any other methods will return HTTP 405 Method Not Allowed.
This will surface any misconfiguration causing data to be sent to `/` instead of the correct endpoint,
e.g. `/v1/traces` for OTLP/HTTP. [PR #15976]
// end::90-bc[]

// tag::811-bc[]
Expand Down
8 changes: 0 additions & 8 deletions docs/spec/openapi/paths/server_info.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,3 @@ get:
responses:
'200':
$ref: '../components/responses/200_server_info.yaml'
post:
Copy link
Member Author

Choose a reason for hiding this comment

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

[to reviewer] other than openapi spec, we'll also need to remove it from obs docs in a separate PR: https://www.elastic.co/guide/en/observability/current/apm-api-info.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Why would you remove it? I would add a disclaimer that the / endpoint is to be used fr healthcheck or test only, but I think the use case of POST with credentials (to verify credentials) is a nice one to give our user guidance on. I would definitely clarify its purpose though.

Copy link
Member Author

Choose a reason for hiding this comment

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

the intention of this very PR is to remove the "functionality" to POST to /. As such, we are removing the corresponding part in the docs. If we don't remove it in the docs, we'll be left with docs that reference an endpoint that doesn't work.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the use case of POST with credentials (to verify credentials) is a nice one to give our user guidance on

I agree that we should update the docs to state that we can optionally provide credentials on the GET request to get additional info about the server. But I'm not sure if OpenAPI spec supports that.

summary: Get general server information
operationId: postServerHealth
tags:
- server info
responses:
'200':
$ref: '../components/responses/200_server_info.yaml'
57 changes: 51 additions & 6 deletions internal/beater/api/mux_root_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@ package api
import (
"encoding/json"
"net/http"
"net/http/httptest"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/elastic/apm-server/internal/beater/config"
"github.com/elastic/apm-server/internal/beater/headers"
"github.com/elastic/apm-server/internal/beater/monitoringtest"
"github.com/elastic/apm-server/internal/beater/request"
"github.com/elastic/apm-server/internal/version"
)
Expand Down Expand Up @@ -63,10 +65,53 @@ func TestRootHandler_PanicMiddleware(t *testing.T) {
}

func TestRootHandler_MonitoringMiddleware(t *testing.T) {
testMonitoringMiddleware(t, "/", map[string]any{
"http.server." + string(request.IDRequestCount): 1,
"http.server." + string(request.IDResponseCount): 1,
"http.server." + string(request.IDResponseValidCount): 1,
"http.server." + string(request.IDResponseValidOK): 1,
})
validMethodMetrics := map[string]any{
"http.server." + string(request.IDRequestCount): 1,
"http.server." + string(request.IDResponseCount): 1,
"http.server." + string(request.IDResponseValidCount): 1,
"http.server." + string(request.IDResponseValidOK): 1,
"apm-server.root." + string(request.IDRequestCount): 1,
"apm-server.root." + string(request.IDResponseCount): 1,
"apm-server.root." + string(request.IDResponseValidCount): 1,
"apm-server.root." + string(request.IDResponseValidOK): 1,
}
invalidMethodMetrics := map[string]any{
"http.server." + string(request.IDRequestCount): 1,
"http.server." + string(request.IDResponseCount): 1,
"http.server." + string(request.IDResponseErrorsCount): 1,
"http.server." + string(request.IDResponseErrorsMethodNotAllowed): 1,
"apm-server.root." + string(request.IDRequestCount): 1,
"apm-server.root." + string(request.IDResponseCount): 1,
"apm-server.root." + string(request.IDResponseErrorsCount): 1,
"apm-server.root." + string(request.IDResponseErrorsMethodNotAllowed): 1,
}
for _, tc := range []struct {
method string
wantMetrics map[string]any
}{
{
method: http.MethodGet,
wantMetrics: validMethodMetrics,
},
{
method: http.MethodHead,
wantMetrics: validMethodMetrics,
},
{
method: http.MethodPut,
wantMetrics: invalidMethodMetrics,
},
{
method: http.MethodPost,
wantMetrics: invalidMethodMetrics,
},
} {
t.Run(tc.method, func(t *testing.T) {
h, reader := newTestMux(t, config.DefaultConfig())
req := httptest.NewRequest(tc.method, "/", nil)
h.ServeHTTP(httptest.NewRecorder(), req)

monitoringtest.ExpectContainOtelMetrics(t, reader, tc.wantMetrics)
})
}
}
17 changes: 15 additions & 2 deletions internal/beater/api/root/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
package root

import (
"errors"
"net/http"
"time"

"github.com/elastic/beats/v7/libbeat/version"
Expand All @@ -38,10 +40,21 @@ type HandlerConfig struct {

// Handler returns error if route does not exist,
// otherwise returns information about the server. The detail level differs for authenticated and anonymous requests.
// TODO: only allow GET, HEAD requests (breaking change)
func Handler(cfg HandlerConfig) request.Handler {

return func(c *request.Context) {
// Only allow GET, HEAD requests
switch c.Request.Method {
case http.MethodGet, http.MethodHead:
default:
c.Result.SetWithError(
request.IDResponseErrorsMethodNotAllowed,
// include a verbose error message to alert users about a common misconfiguration
errors.New("this is the health check endpoint; did you mean to send data to another endpoint instead?"),
Copy link
Member Author

Choose a reason for hiding this comment

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

[to reviewer] lmk if this is too verbose, I can revert the commit.

$ curl -XPOST -v localhost:8200/
* Host localhost:8200 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:8200...
* connect to ::1 port 8200 from ::1 port 49848 failed: Connection refused
*   Trying 127.0.0.1:8200...
* Connected to localhost (127.0.0.1) port 8200
> POST / HTTP/1.1
> Host: localhost:8200
> User-Agent: curl/8.5.0
> Accept: */*
> 
< HTTP/1.1 405 Method Not Allowed
< Content-Type: application/json
< X-Content-Type-Options: nosniff
< Date: Mon, 03 Mar 2025 16:35:38 GMT
< Content-Length: 129
< 
{
  "error": "method not supported: this is the health check endpoint; did you mean to send data to another endpoint instead?"
}
* Connection #0 to host localhost left intact

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine.

)
c.WriteResult()
return
}

serverInfo := mapstr.M{
"build_date": version.BuildTime().Format(time.RFC3339),
"build_sha": version.Commit(),
Expand Down
Loading