-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
VAULT-1564 report in-flight requests #13024
Changes from 7 commits
3463e90
6420c0c
2c3fefe
e2031da
29c5f24
17a4928
18ab7da
660ff96
a06c4a7
b2979d5
5914acd
f418b0e
c181d15
16ed14a
e9f2312
d7682a5
2ad56c2
814128b
6ecb241
ec22517
8a6350e
b9dad8c
e3079f0
96e5c71
03153e7
da8a1e0
18d70ba
2a53824
9893d2e
d565b64
97c4cf3
5649a60
a0bec8d
087fb00
5ed569b
8c8e5d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
```release-note:feature | ||
**Report in-flight requests**:Adding a trace capability to show in-flight requests, and a new gauge metric to show the total number of in-flight requests | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ import ( | |
"github.com/hashicorp/go-secure-stdlib/gatedwriter" | ||
"github.com/hashicorp/go-secure-stdlib/strutil" | ||
"github.com/hashicorp/vault/api" | ||
"github.com/hashicorp/vault/sdk/helper/jsonutil" | ||
"github.com/hashicorp/vault/sdk/helper/logging" | ||
"github.com/hashicorp/vault/sdk/version" | ||
"github.com/mholt/archiver" | ||
|
@@ -106,6 +107,7 @@ type DebugCommand struct { | |
metricsCollection []map[string]interface{} | ||
replicationStatusCollection []map[string]interface{} | ||
serverStatusCollection []map[string]interface{} | ||
inFlightReqStatusCollection []map[string]interface{} | ||
|
||
// cachedClient holds the client retrieved during preflight | ||
cachedClient *api.Client | ||
|
@@ -480,7 +482,7 @@ func (c *DebugCommand) preflight(rawArgs []string) (string, error) { | |
} | ||
|
||
func (c *DebugCommand) defaultTargets() []string { | ||
return []string{"config", "host", "metrics", "pprof", "replication-status", "server-status", "log"} | ||
return []string{"config", "host", "requests", "metrics", "pprof", "replication-status", "server-status", "log"} | ||
} | ||
|
||
func (c *DebugCommand) captureStaticTargets() error { | ||
|
@@ -492,6 +494,7 @@ func (c *DebugCommand) captureStaticTargets() error { | |
if err != nil { | ||
c.captureError("config", err) | ||
c.logger.Error("config: error capturing config state", "error", err) | ||
return nil | ||
} | ||
|
||
if resp != nil && resp.Data != nil { | ||
|
@@ -580,6 +583,16 @@ func (c *DebugCommand) capturePollingTargets() error { | |
}) | ||
} | ||
|
||
// Collect in-flight request status if target is specified | ||
if strutil.StrListContains(c.flagTargets, "requests") { | ||
g.Add(func() error { | ||
c.collectInFlightRequestStatus(ctx) | ||
return nil | ||
}, func(error) { | ||
cancelFunc() | ||
}) | ||
} | ||
|
||
if strutil.StrListContains(c.flagTargets, "log") { | ||
g.Add(func() error { | ||
c.writeLogs(ctx) | ||
|
@@ -611,7 +624,9 @@ func (c *DebugCommand) capturePollingTargets() error { | |
if err := c.persistCollection(c.hostInfoCollection, "host_info.json"); err != nil { | ||
c.UI.Error(fmt.Sprintf("Error writing data to %s: %v", "host_info.json", err)) | ||
} | ||
|
||
if err := c.persistCollection(c.inFlightReqStatusCollection, "requests.json"); err != nil { | ||
c.UI.Error(fmt.Sprintf("Error writing data to %s: %v", "requests.json", err)) | ||
} | ||
return nil | ||
} | ||
|
||
|
@@ -635,13 +650,15 @@ func (c *DebugCommand) collectHostInfo(ctx context.Context) { | |
resp, err := c.cachedClient.RawRequestWithContext(ctx, r) | ||
if err != nil { | ||
c.captureError("host", err) | ||
return | ||
} | ||
if resp != nil { | ||
defer resp.Body.Close() | ||
|
||
secret, err := api.ParseSecret(resp.Body) | ||
if err != nil { | ||
c.captureError("host", err) | ||
return | ||
} | ||
if secret != nil && secret.Data != nil { | ||
hostEntry := secret.Data | ||
|
@@ -829,13 +846,15 @@ func (c *DebugCommand) collectReplicationStatus(ctx context.Context) { | |
resp, err := c.cachedClient.RawRequestWithContext(ctx, r) | ||
if err != nil { | ||
c.captureError("replication-status", err) | ||
return | ||
} | ||
if resp != nil { | ||
defer resp.Body.Close() | ||
|
||
secret, err := api.ParseSecret(resp.Body) | ||
if err != nil { | ||
c.captureError("replication-status", err) | ||
return | ||
} | ||
if secret != nil && secret.Data != nil { | ||
replicationEntry := secret.Data | ||
|
@@ -865,10 +884,12 @@ func (c *DebugCommand) collectServerStatus(ctx context.Context) { | |
healthInfo, err := c.cachedClient.Sys().Health() | ||
if err != nil { | ||
c.captureError("server-status.health", err) | ||
return | ||
} | ||
sealInfo, err := c.cachedClient.Sys().SealStatus() | ||
if err != nil { | ||
c.captureError("server-status.seal", err) | ||
return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here too. |
||
} | ||
|
||
statusEntry := map[string]interface{}{ | ||
|
@@ -880,6 +901,50 @@ func (c *DebugCommand) collectServerStatus(ctx context.Context) { | |
} | ||
} | ||
|
||
func (c *DebugCommand) collectInFlightRequestStatus(ctx context.Context) { | ||
|
||
idxCount := 0 | ||
intervalTicker := time.Tick(c.flagInterval) | ||
|
||
for { | ||
if idxCount > 0 { | ||
select { | ||
case <-ctx.Done(): | ||
return | ||
case <-intervalTicker: | ||
} | ||
} | ||
|
||
c.logger.Info("capturing in-flight request status", "count", idxCount) | ||
idxCount++ | ||
|
||
req := c.cachedClient.NewRequest("GET", "/v1/sys/in-flight-req") | ||
resp, err := c.cachedClient.RawRequestWithContext(ctx, req) | ||
if err != nil { | ||
c.captureError("inFlightReq-status", err) | ||
ncabatoff marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return | ||
} | ||
|
||
var data map[string]interface{} | ||
if resp != nil { | ||
defer resp.Body.Close() | ||
err = jsonutil.DecodeJSONFromReader(resp.Body, &data) | ||
if err != nil { | ||
c.captureError("inFlightReq-status", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like the first arg to captureError is the target, which we're now calling |
||
return | ||
} | ||
|
||
if data != nil && len(data) > 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need this conditional do we? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, there should always be at least one entry which is the /v1/sys/in-flight-req related info |
||
statusEntry := map[string]interface{}{ | ||
"timestamp": time.Now().UTC(), | ||
"in_flight_requests": data, | ||
} | ||
c.inFlightReqStatusCollection = append(c.inFlightReqStatusCollection, statusEntry) | ||
} | ||
} | ||
} | ||
} | ||
|
||
// persistCollection writes the collected data for a particular target onto the | ||
// specified file. If the collection is empty, it returns immediately. | ||
func (c *DebugCommand) persistCollection(collection []map[string]interface{}, outFile string) error { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
storage "inmem" {} | ||
listener "tcp" { | ||
address = "127.0.0.1:8200" | ||
tls_disable = true | ||
profiling { | ||
unauthenticated_in_flight_request_access = true | ||
} | ||
} | ||
disable_mlock = true |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ import ( | |
"encoding/json" | ||
"errors" | ||
"fmt" | ||
"github.com/hashicorp/go-uuid" | ||
ncabatoff marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"io" | ||
"io/fs" | ||
"io/ioutil" | ||
|
@@ -196,6 +197,11 @@ func Handler(props *vault.HandlerProperties) http.Handler { | |
mux.Handle("/v1/sys/pprof/", handleLogicalNoForward(core)) | ||
} | ||
|
||
if props.ListenerConfig != nil && props.ListenerConfig.Profiling.UnauthenticatedInFlightAccess { | ||
mux.Handle("/v1/sys/in-flight-req", handleUnAuthenticatedInFlightRequest(core)) | ||
} else { | ||
mux.Handle("/v1/sys/in-flight-req", handleLogicalNoForward(core)) | ||
} | ||
additionalRoutes(mux, core) | ||
} | ||
|
||
|
@@ -389,6 +395,27 @@ func wrapGenericHandler(core *vault.Core, h http.Handler, props *vault.HandlerPr | |
hostname, _ := os.Hostname() | ||
|
||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
|
||
// The uuid for the request is going to be generated when a logical | ||
// request is generated. But, here we generate one to be able to track | ||
// in-flight requests | ||
inFlightReqID, err := uuid.GenerateUUID() | ||
if err != nil { | ||
respondError(w, http.StatusInternalServerError, fmt.Errorf("failed to generate an identifier for the in-flight request")) | ||
} | ||
core.StoreInFlightReqData( | ||
inFlightReqID, | ||
&vault.InFlightReqData { | ||
StartTime: time.Now(), | ||
ClientRemoteAddr: r.RemoteAddr, | ||
ReqPath: r.URL.Path, | ||
}) | ||
if err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What error are we handling here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, thanks for catching this! |
||
respondError(w, http.StatusBadRequest, err) | ||
} | ||
// deleting the in-flight request entry | ||
defer core.DeleteInFlightReqData(inFlightReqID) | ||
|
||
// This block needs to be here so that upon sending SIGHUP, custom response | ||
// headers are also reloaded into the handlers. | ||
if props.ListenerConfig != nil { | ||
|
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.
Although it's true that an error on the health endpoint will probably mean one on the seal-status endpoint too, I'd rather we didn't assume that. Can you revert this change please?