Add duration/count metrics to PKI issue and revoke flows (#13889)

* Add duration/count metrics to PKI issue and revoke flows

* docs, changelog

* tidy

* last tidy

* remove err

* Update callsites

* Simple returns

* Handle the fact that test cases don't have namespaces

* Add mount point to the request

* fmt

* Handle empty mount point, and add it to unit tests

* improvement

* Turns out sign-verbatim is tricky, it can take a role but doesn't have to

* Get around the field schema problem

builtin/logical/pki/backend.go

@@ -2,14 +2,24 @@ package pki
 
 import (
 	"context"
+	"fmt"
 	"strings"
 	"sync"
 	"time"
 
+	"github.com/armon/go-metrics"
+	"github.com/hashicorp/vault/helper/metricsutil"
+	"github.com/hashicorp/vault/helper/namespace"
 	"github.com/hashicorp/vault/sdk/framework"
 	"github.com/hashicorp/vault/sdk/logical"
 )
 
+const (
+	noRole       = 0
+	roleOptional = 1
+	roleRequired = 2
+)
+
 // Factory creates a new backend implementing the logical.Backend interface
 func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend, error) {
 	b := Backend(conf)
@@ -106,7 +116,10 @@ type backend struct {
 	tidyStatus     *tidyStatus
 }
 
-type tidyStatusState int
+type (
+	tidyStatusState int
+	roleOperation   func(ctx context.Context, req *logical.Request, data *framework.FieldData, role *roleEntry) (*logical.Response, error)
+)
 
 const (
 	tidyStatusInactive tidyStatusState = iota
@@ -137,3 +150,60 @@ The PKI backend dynamically generates X509 server and client certificates.
 After mounting this backend, configure the CA using the "pem_bundle" endpoint within
 the "config/" path.
 `
+
+func metricsKey(req *logical.Request, extra ...string) []string {
+	if req == nil || req.MountPoint == "" {
+		return extra
+	}
+	key := make([]string, len(extra)+1)
+	key[0] = req.MountPoint[:len(req.MountPoint)-1]
+	copy(key[1:], extra)
+	return key
+}
+
+func (b *backend) metricsWrap(callType string, roleMode int, ofunc roleOperation) framework.OperationFunc {
+	return func(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
+		key := metricsKey(req, callType)
+		var role *roleEntry
+		var labels []metrics.Label
+		var err error
+
+		var roleName string
+		switch roleMode {
+		case roleRequired:
+			roleName = data.Get("role").(string)
+		case roleOptional:
+			r, ok := data.GetOk("role")
+			if ok {
+				roleName = r.(string)
+			}
+		}
+		if roleMode > noRole {
+			// Get the role
+			role, err = b.getRole(ctx, req.Storage, roleName)
+			if err != nil {
+				return nil, err
+			}
+			if role == nil && roleMode == roleRequired {
+				return logical.ErrorResponse(fmt.Sprintf("unknown role: %s", roleName)), nil
+			}
+			labels = []metrics.Label{{"role", roleName}}
+		}
+
+		ns, err := namespace.FromContext(ctx)
+		if err == nil {
+			labels = append(labels, metricsutil.NamespaceLabel(ns))
+		}
+
+		start := time.Now()
+		defer metrics.MeasureSinceWithLabels(key, start, labels)
+		resp, err := ofunc(ctx, req, data, role)
+
+		if err != nil || resp.IsError() {
+			metrics.IncrCounterWithLabels(append(key, "failure"), 1.0, labels)
+		} else {
+			metrics.IncrCounterWithLabels(key, 1.0, labels)
+		}
+		return resp, err
+	}
+}

builtin/logical/pki/backend_test.go

@@ -1792,10 +1792,11 @@ func TestBackend_PathFetchCertList(t *testing.T) {
 	}
 
 	resp, err := b.HandleRequest(context.Background(), &logical.Request{
-		Operation: logical.UpdateOperation,
+		Operation:  logical.UpdateOperation,
-		Path:      "root/generate/internal",
+		Path:       "root/generate/internal",
-		Storage:   storage,
+		Storage:    storage,
-		Data:      rootData,
+		Data:       rootData,
+		MountPoint: "pki/",
 	})
 	if resp != nil && resp.IsError() {
 		t.Fatalf("failed to generate root, %#v", resp)
@@ -1811,10 +1812,11 @@ func TestBackend_PathFetchCertList(t *testing.T) {
 	}
 
 	resp, err = b.HandleRequest(context.Background(), &logical.Request{
-		Operation: logical.UpdateOperation,
+		Operation:  logical.UpdateOperation,
-		Path:      "config/urls",
+		Path:       "config/urls",
-		Storage:   storage,
+		Storage:    storage,
-		Data:      urlsData,
+		Data:       urlsData,
+		MountPoint: "pki/",
 	})
 	if resp != nil && resp.IsError() {
 		t.Fatalf("failed to config urls, %#v", resp)
@@ -1831,10 +1833,11 @@ func TestBackend_PathFetchCertList(t *testing.T) {
 	}
 
 	resp, err = b.HandleRequest(context.Background(), &logical.Request{
-		Operation: logical.UpdateOperation,
+		Operation:  logical.UpdateOperation,
-		Path:      "roles/test-example",
+		Path:       "roles/test-example",
-		Storage:   storage,
+		Storage:    storage,
-		Data:      roleData,
+		Data:       roleData,
+		MountPoint: "pki/",
 	})
 	if resp != nil && resp.IsError() {
 		t.Fatalf("failed to create a role, %#v", resp)
@@ -1850,10 +1853,11 @@ func TestBackend_PathFetchCertList(t *testing.T) {
 			"common_name": "example.test.com",
 		}
 		resp, err = b.HandleRequest(context.Background(), &logical.Request{
-			Operation: logical.UpdateOperation,
+			Operation:  logical.UpdateOperation,
-			Path:      "issue/test-example",
+			Path:       "issue/test-example",
-			Storage:   storage,
+			Storage:    storage,
-			Data:      certData,
+			Data:       certData,
+			MountPoint: "pki/",
 		})
 		if resp != nil && resp.IsError() {
 			t.Fatalf("failed to issue a cert, %#v", resp)
@@ -1867,9 +1871,10 @@ func TestBackend_PathFetchCertList(t *testing.T) {
 
 	// list certs
 	resp, err = b.HandleRequest(context.Background(), &logical.Request{
-		Operation: logical.ListOperation,
+		Operation:  logical.ListOperation,
-		Path:      "certs",
+		Path:       "certs",
-		Storage:   storage,
+		Storage:    storage,
+		MountPoint: "pki/",
 	})
 	if resp != nil && resp.IsError() {
 		t.Fatalf("failed to list certs, %#v", resp)
@@ -1884,9 +1889,10 @@ func TestBackend_PathFetchCertList(t *testing.T) {
 
 	// list certs/
 	resp, err = b.HandleRequest(context.Background(), &logical.Request{
-		Operation: logical.ListOperation,
+		Operation:  logical.ListOperation,
-		Path:      "certs/",
+		Path:       "certs/",
-		Storage:   storage,
+		Storage:    storage,
+		MountPoint: "pki/",
 	})
 	if resp != nil && resp.IsError() {
 		t.Fatalf("failed to list certs, %#v", resp)
@@ -1919,10 +1925,11 @@ func TestBackend_SignVerbatim(t *testing.T) {
 	}
 
 	resp, err := b.HandleRequest(context.Background(), &logical.Request{
-		Operation: logical.UpdateOperation,
+		Operation:  logical.UpdateOperation,
-		Path:      "root/generate/internal",
+		Path:       "root/generate/internal",
-		Storage:   storage,
+		Storage:    storage,
-		Data:      rootData,
+		Data:       rootData,
+		MountPoint: "pki/",
 	})
 	if resp != nil && resp.IsError() {
 		t.Fatalf("failed to generate root, %#v", *resp)
@@ -1963,6 +1970,7 @@ func TestBackend_SignVerbatim(t *testing.T) {
 		Data: map[string]interface{}{
 			"csr": pemCSR,
 		},
+		MountPoint: "pki/",
 	})
 	if resp != nil && resp.IsError() {
 		t.Fatalf("failed to sign-verbatim basic CSR: %#v", *resp)
@@ -1980,10 +1988,11 @@ func TestBackend_SignVerbatim(t *testing.T) {
 		"max_ttl": "8h",
 	}
 	resp, err = b.HandleRequest(context.Background(), &logical.Request{
-		Operation: logical.UpdateOperation,
+		Operation:  logical.UpdateOperation,
-		Path:      "roles/test",
+		Path:       "roles/test",
-		Storage:   storage,
+		Storage:    storage,
-		Data:      roleData,
+		Data:       roleData,
+		MountPoint: "pki/",
 	})
 	if resp != nil && resp.IsError() {
 		t.Fatalf("failed to create a role, %#v", *resp)
@@ -1999,6 +2008,7 @@ func TestBackend_SignVerbatim(t *testing.T) {
 			"csr": pemCSR,
 			"ttl": "5h",
 		},
+		MountPoint: "pki/",
 	})
 	if resp != nil && resp.IsError() {
 		t.Fatalf("failed to sign-verbatim ttl'd CSR: %#v", *resp)
@@ -2017,6 +2027,7 @@ func TestBackend_SignVerbatim(t *testing.T) {
 			"csr": pemCSR,
 			"ttl": "12h",
 		},
+		MountPoint: "pki/",
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -2053,6 +2064,7 @@ func TestBackend_SignVerbatim(t *testing.T) {
 			"csr":       pemCSR,
 			"not_after": "9999-12-31T23:59:59Z",
 		},
+		MountPoint: "pki/",
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -2088,10 +2100,11 @@ func TestBackend_SignVerbatim(t *testing.T) {
 		"generate_lease": true,
 	}
 	resp, err = b.HandleRequest(context.Background(), &logical.Request{
-		Operation: logical.UpdateOperation,
+		Operation:  logical.UpdateOperation,
-		Path:      "roles/test",
+		Path:       "roles/test",
-		Storage:   storage,
+		Storage:    storage,
-		Data:      roleData,
+		Data:       roleData,
+		MountPoint: "pki/",
 	})
 	if resp != nil && resp.IsError() {
 		t.Fatalf("failed to create a role, %#v", *resp)
@@ -2107,6 +2120,7 @@ func TestBackend_SignVerbatim(t *testing.T) {
 			"csr": pemCSR,
 			"ttl": "5h",
 		},
+		MountPoint: "pki/",
 	})
 	if resp != nil && resp.IsError() {
 		t.Fatalf("failed to sign-verbatim role-leased CSR: %#v", *resp)
@@ -2336,10 +2350,11 @@ func TestBackend_SignSelfIssued(t *testing.T) {
 	}
 
 	resp, err := b.HandleRequest(context.Background(), &logical.Request{
-		Operation: logical.UpdateOperation,
+		Operation:  logical.UpdateOperation,
-		Path:      "root/generate/internal",
+		Path:       "root/generate/internal",
-		Storage:   storage,
+		Storage:    storage,
-		Data:      rootData,
+		Data:       rootData,
+		MountPoint: "pki/",
 	})
 	if resp != nil && resp.IsError() {
 		t.Fatalf("failed to generate root, %#v", *resp)
@@ -2370,6 +2385,7 @@ func TestBackend_SignSelfIssued(t *testing.T) {
 		Data: map[string]interface{}{
 			"certificate": ss,
 		},
+		MountPoint: "pki/",
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -2400,6 +2416,7 @@ func TestBackend_SignSelfIssued(t *testing.T) {
 		Data: map[string]interface{}{
 			"certificate": ss,
 		},
+		MountPoint: "pki/",
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -2419,6 +2436,7 @@ func TestBackend_SignSelfIssued(t *testing.T) {
 		Data: map[string]interface{}{
 			"certificate": ss,
 		},
+		MountPoint: "pki/",
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -2481,10 +2499,11 @@ func TestBackend_SignSelfIssued_DifferentTypes(t *testing.T) {
 	}
 
 	resp, err := b.HandleRequest(context.Background(), &logical.Request{
-		Operation: logical.UpdateOperation,
+		Operation:  logical.UpdateOperation,
-		Path:      "root/generate/internal",
+		Path:       "root/generate/internal",
-		Storage:   storage,
+		Storage:    storage,
-		Data:      rootData,
+		Data:       rootData,
+		MountPoint: "pki/",
 	})
 	if resp != nil && resp.IsError() {
 		t.Fatalf("failed to generate root, %#v", *resp)
@@ -2516,6 +2535,7 @@ func TestBackend_SignSelfIssued_DifferentTypes(t *testing.T) {
 		Data: map[string]interface{}{
 			"certificate": ss,
 		},
+		MountPoint: "pki/",
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -2537,6 +2557,7 @@ func TestBackend_SignSelfIssued_DifferentTypes(t *testing.T) {
 			"certificate": ss,
 			"require_matching_certificate_algorithms": false,
 		},
+		MountPoint: "pki/",
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -2555,6 +2576,7 @@ func TestBackend_SignSelfIssued_DifferentTypes(t *testing.T) {
 			"certificate": ss,
 			"require_matching_certificate_algorithms": true,
 		},
+		MountPoint: "pki/",
 	})
 	if err == nil {
 		t.Fatal("expected error due to mismatched algorithms")

builtin/logical/pki/path_issue_sign.go

@@ -19,7 +19,7 @@ func pathIssue(b *backend) *framework.Path {
 		Pattern: "issue/" + framework.GenericNameRegex("role"),
 
 		Callbacks: map[logical.Operation]framework.OperationFunc{
-			logical.UpdateOperation: b.pathIssue,
+			logical.UpdateOperation: b.metricsWrap("issue", roleRequired, b.pathIssue),
 		},
 
 		HelpSynopsis:    pathIssueHelpSyn,
@@ -35,7 +35,7 @@ func pathSign(b *backend) *framework.Path {
 		Pattern: "sign/" + framework.GenericNameRegex("role"),
 
 		Callbacks: map[logical.Operation]framework.OperationFunc{
-			logical.UpdateOperation: b.pathSign,
+			logical.UpdateOperation: b.metricsWrap("sign", roleRequired, b.pathSign),
 		},
 
 		HelpSynopsis:    pathSignHelpSyn,
@@ -58,7 +58,7 @@ func pathSignVerbatim(b *backend) *framework.Path {
 		Pattern: "sign-verbatim" + framework.OptionalParamRegex("role"),
 
 		Callbacks: map[logical.Operation]framework.OperationFunc{
-			logical.UpdateOperation: b.pathSignVerbatim,
+			logical.UpdateOperation: b.metricsWrap("sign-verbatim", roleOptional, b.pathSignVerbatim),
 		},
 
 		HelpSynopsis:    pathSignHelpSyn,
@@ -106,18 +106,7 @@ this value to an empty list.`,
 
 // pathIssue issues a certificate and private key from given parameters,
 // subject to role restrictions
-func (b *backend) pathIssue(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
+func (b *backend) pathIssue(ctx context.Context, req *logical.Request, data *framework.FieldData, role *roleEntry) (*logical.Response, error) {
-	roleName := data.Get("role").(string)
-
-	// Get the role
-	role, err := b.getRole(ctx, req.Storage, roleName)
-	if err != nil {
-		return nil, err
-	}
-	if role == nil {
-		return logical.ErrorResponse(fmt.Sprintf("unknown role: %s", roleName)), nil
-	}
-
 	if role.KeyType == "any" {
 		return logical.ErrorResponse("role key type \"any\" not allowed for issuing certificates, only signing"), nil
 	}
@@ -127,32 +116,13 @@ func (b *backend) pathIssue(ctx context.Context, req *logical.Request, data *fra
 
 // pathSign issues a certificate from a submitted CSR, subject to role
 // restrictions
-func (b *backend) pathSign(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
+func (b *backend) pathSign(ctx context.Context, req *logical.Request, data *framework.FieldData, role *roleEntry) (*logical.Response, error) {
-	roleName := data.Get("role").(string)
-
-	// Get the role
-	role, err := b.getRole(ctx, req.Storage, roleName)
-	if err != nil {
-		return nil, err
-	}
-	if role == nil {
-		return logical.ErrorResponse(fmt.Sprintf("unknown role: %s", roleName)), nil
-	}
-
 	return b.pathIssueSignCert(ctx, req, data, role, true, false)
 }
 
 // pathSignVerbatim issues a certificate from a submitted CSR, *not* subject to
 // role restrictions
-func (b *backend) pathSignVerbatim(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
+func (b *backend) pathSignVerbatim(ctx context.Context, req *logical.Request, data *framework.FieldData, role *roleEntry) (*logical.Response, error) {
-	roleName := data.Get("role").(string)
-
-	// Get the role if one was specified
-	role, err := b.getRole(ctx, req.Storage, roleName)
-	if err != nil {
-		return nil, err
-	}
-
 	entry := &roleEntry{
 		AllowLocalhost:       true,
 		AllowAnyName:         true,

builtin/logical/pki/path_revoke.go

@@ -23,7 +23,7 @@ hyphen-separated octal`,
 		},
 
 		Callbacks: map[logical.Operation]framework.OperationFunc{
-			logical.UpdateOperation: b.pathRevokeWrite,
+			logical.UpdateOperation: b.metricsWrap("revoke", noRole, b.pathRevokeWrite),
 		},
 
 		HelpSynopsis:    pathRevokeHelpSyn,
@@ -44,7 +44,7 @@ func pathRotateCRL(b *backend) *framework.Path {
 	}
 }
 
-func (b *backend) pathRevokeWrite(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
+func (b *backend) pathRevokeWrite(ctx context.Context, req *logical.Request, data *framework.FieldData, _ *roleEntry) (*logical.Response, error) {
 	serial := data.Get("serial_number").(string)
 	if len(serial) == 0 {
 		return logical.ErrorResponse("The serial number must be provided"), nil

changelog/13889.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+secrets/pki: Add count and duration metrics to PKI issue and revoke calls.
+```

website/content/docs/secrets/pki.mdx

@@ -211,6 +211,12 @@ associated leases, to prevent unintended revocation when not using a token with
 a long enough lifetime. To revoke these certificates, use the `pki/revoke`
 endpoint.
 
+### Telemetry
+
+Beyond Vault's default telemetry around request processing, PKI exposes count and
+ duration metrics for the issue, sign, sign-verbatim, and revoke calls. The 
+metrics keys take the form `mount-path,operation,[failure]` with labels for namespace and role name.
+
 ## Quick Start
 
 #### Mount the backend