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.

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

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

Merged
merged 12 commits into from
Mar 5, 2025
+67 −17
Merged

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

Show file tree
Hide file tree
Changes from all 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
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
Member

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'
59 changes: 52 additions & 7 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 All @@ -36,7 +38,7 @@ func TestRootHandler_AuthorizationMiddleware(t *testing.T) {
cfg.AgentAuth.SecretToken = "1234"

t.Run("No auth", func(t *testing.T) {
rec, err := requestToMuxerWithPattern(cfg, RootPath)
rec, err := requestToMuxerWithHeader(cfg, RootPath, http.MethodGet, nil)
require.NoError(t, err)
assert.Equal(t, http.StatusOK, rec.Code)
assert.Empty(t, rec.Body.String())
Expand All @@ -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 server information endpoint; did you mean to send data to another endpoint instead?"),
)
c.WriteResult()
return
}

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