From 483feae29157fa47c61a1298c11817ddc6c380c6 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Thu, 13 May 2021 12:02:50 -0600 Subject: [PATCH 01/24] build out lease count (not fully working), start lease list --- vault/expiration.go | 59 +++++++++++++++++++++++++++++++++++ vault/logical_system.go | 26 +++++++++++++++ vault/logical_system_paths.go | 24 ++++++++++++++ 3 files changed, 109 insertions(+) diff --git a/vault/expiration.go b/vault/expiration.go index cee48ae15fe84..c4e7424f2bb09 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -2411,6 +2411,65 @@ func (m *ExpirationManager) markLeaseIrrevocable(ctx context.Context, le *leaseE m.nonexpiring.Delete(le.LeaseID) } +func (m *ExpirationManager) getNamespaceFromLeaseID(ctx context.Context, leaseID string) (*namespace.Namespace, error) { + _, nsID := namespace.SplitIDFromString(leaseID) + leaseNS := namespace.RootNamespace + var err error + if nsID != "" { + leaseNS, err = NamespaceByID(ctx, nsID, m.core) + if err != nil { + return nil, err + } + } + + return leaseNS, nil +} + +// TODO if keep counts as a map, should update the RFC +func (m *ExpirationManager) getIrrevocableLeaseCounts(ctx context.Context, includeChildNamespaces bool) (map[string]interface{}, error) { + requestNS, err := namespace.FromContext(ctx) + if err != nil { + m.logger.Error("could not get namespace from context", "error", err) + return nil, err + } + + numMatchingLeases := 0 + numMatchingLeasesPerMount := make(map[string]int) + m.irrevocable.Range(func(k, v interface{}) bool { + leaseID := k.(string) + leaseInfo := v.(*leaseEntry) + + leaseNS, err := m.getNamespaceFromLeaseID(ctx, leaseID) + if err != nil { + // We probably want to track that an error occured, but continue counting + m.logger.Warn("could not get lease namespace from ID", "error", err) + return true + } + + leaseMatches := (leaseNS == requestNS) || (includeChildNamespaces && leaseNS.HasParent(requestNS)) + if !leaseMatches { + // the lease doesn't meet our criteria, so keep looking + return true + } + + // TODO verify path is kept + if _, ok := numMatchingLeasesPerMount[leaseInfo.Path]; !ok { + numMatchingLeasesPerMount[leaseInfo.Path] = 0 + } + + numMatchingLeases++ + numMatchingLeasesPerMount[leaseInfo.Path]++ + + return true + }) + + resp := make(map[string]interface{}) + resp["lease_count"] = numMatchingLeases + resp["counts"] = numMatchingLeases + + return resp, nil +} + // leaseEntry is used to structure the values the expiration // manager stores. This is used to handle renew and revocation. type leaseEntry struct { diff --git a/vault/logical_system.go b/vault/logical_system.go index fe743cf6541c2..e8afd46d97a19 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -285,6 +285,24 @@ func (b *SystemBackend) handleTidyLeases(ctx context.Context, req *logical.Reque return logical.RespondWithStatusCode(resp, req, http.StatusAccepted) } +func (b *SystemBackend) handleLeaseCount(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { + paramRaw, ok := d.GetOk("include_child_namespaces") + includeChildNamespaces := ok && paramRaw.(bool) + + resp, err := b.Core.expiration.getIrrevocableLeaseCounts(ctx, includeChildNamespaces) + if err != nil { + return nil, err + } + + return &logical.Response{ + Data: resp, + }, nil +} + +func (b *SystemBackend) handleLeaseList(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { + +} + func (b *SystemBackend) handlePluginCatalogTypedList(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { pluginType, err := consts.ParsePluginType(d.Get("type").(string)) if err != nil { @@ -4688,4 +4706,12 @@ This path responds to the following HTTP methods. "Control the collection and reporting of client counts.", "Control the collection and reporting of client counts.", }, + "count-leases": { + "Count of leases associated with this Vault cluster", + "Count of leases associated with this Vault cluster", + }, + "list-leases": { + "List leases associated with this Vault cluster", + "List leases associated with this Vault cluster", + }, } diff --git a/vault/logical_system_paths.go b/vault/logical_system_paths.go index 817ab02ac5940..c9b818301427b 100644 --- a/vault/logical_system_paths.go +++ b/vault/logical_system_paths.go @@ -1210,6 +1210,30 @@ func (b *SystemBackend) leasePaths() []*framework.Path { HelpSynopsis: strings.TrimSpace(sysHelp["tidy_leases"][0]), HelpDescription: strings.TrimSpace(sysHelp["tidy_leases"][1]), }, + + { + Pattern: "leases/count$", + + Callbacks: map[logical.Operation]framework.OperationFunc{ + // currently only works for irrevocable leases with param: type=irrevocable + logical.ReadOperation: b.handleLeaseCount, + }, + + HelpSynopsis: strings.TrimSpace(sysHelp["count-leases"][0]), + HelpDescription: strings.TrimSpace(sysHelp["count-leases"][1]), + }, + + { + Pattern: "leases(/)?$", + + Callbacks: map[logical.Operation]framework.OperationFunc{ + // currently only works for irrevocable leases with param: type=irrevocable + logical.ListOperation: b.handleLeaseList, + }, + + HelpSynopsis: strings.TrimSpace(sysHelp["list-leases"][0]), + HelpDescription: strings.TrimSpace(sysHelp["list-leases"][1]), + }, } } From 9d9f8378cdc247af685afcb3928710f229fc7c05 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Thu, 13 May 2021 13:09:49 -0600 Subject: [PATCH 02/24] build out irrevocable lease list --- vault/expiration.go | 69 ++++++++++++++++++++++++++++++++++++++++- vault/logical_system.go | 27 ++++++++++++++-- 2 files changed, 93 insertions(+), 3 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index c4e7424f2bb09..d504f530f1774 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -73,6 +73,14 @@ const ( genericIrrevocableErrorMessage = "unknown" outOfRetriesMessage = "out of retries" + + // maximum number of irrevocable leases we return to the irrevocable lease + // list API **without** the `force` flag set + maxIrrevocableLeasesToReturn = 10000 +) + +var ( + hitMaxIrrevocableLeases = errors.New("Command cancelled because many irrevocable leases were found. To emit the entire list, re-run the command with the force flag set true.") ) type pendingInfo struct { @@ -2433,8 +2441,8 @@ func (m *ExpirationManager) getIrrevocableLeaseCounts(ctx context.Context, inclu return nil, err } - numMatchingLeases := 0 numMatchingLeasesPerMount := make(map[string]int) + numMatchingLeases := 0 m.irrevocable.Range(func(k, v interface{}) bool { leaseID := k.(string) leaseInfo := v.(*leaseEntry) @@ -2470,6 +2478,65 @@ func (m *ExpirationManager) getIrrevocableLeaseCounts(ctx context.Context, inclu return resp, nil } +type leaseResponse struct { + LeaseID string `json:"lease_id"` + ErrMsg string `json:"error"` +} + +func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeChildNamespaces, force bool) (map[string]interface{}, error) { + requestNS, err := namespace.FromContext(ctx) + if err != nil { + m.logger.Error("could not get namespace from context", "error", err) + return nil, err + } + + // map of mount point : lease info + matchingLeasesPerMount := make(map[string][]*leaseResponse) + numMatchingLeases := 0 + var retErr error + m.irrevocable.Range(func(k, v interface{}) bool { + leaseID := k.(string) + leaseInfo := v.(*leaseEntry) + + leaseNS, err := m.getNamespaceFromLeaseID(ctx, leaseID) + if err != nil { + // We probably want to track that an error occured, but continue counting + m.logger.Warn("could not get lease namespace from ID", "error", err) + return true + } + + leaseMatches := (leaseNS == requestNS) || (includeChildNamespaces && leaseNS.HasParent(requestNS)) + if !leaseMatches { + // the lease doesn't meet our criteria, so keep looking + return true + } + + if !force && (numMatchingLeases >= maxIrrevocableLeasesToReturn) { + m.logger.Warn("hit max irrevocable leases without force flag set") + retErr = hitMaxIrrevocableLeases + return false + } + + numMatchingLeases++ + matchingLeasesPerMount[leaseInfo.Path] = append(matchingLeasesPerMount[leaseInfo.Path], &leaseResponse{ + LeaseID: leaseID, + ErrMsg: leaseInfo.RevokeErr, + }) + + return true + }) + + if retErr != nil { + return nil, retErr + } + + resp := make(map[string]interface{}) + resp["lease_count"] = numMatchingLeases + resp["leases"] = matchingLeasesPerMount + + return resp, nil +} + // leaseEntry is used to structure the values the expiration // manager stores. This is used to handle renew and revocation. type leaseEntry struct { diff --git a/vault/logical_system.go b/vault/logical_system.go index e8afd46d97a19..40c45381676c8 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -286,8 +286,13 @@ func (b *SystemBackend) handleTidyLeases(ctx context.Context, req *logical.Reque } func (b *SystemBackend) handleLeaseCount(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { - paramRaw, ok := d.GetOk("include_child_namespaces") - includeChildNamespaces := ok && paramRaw.(bool) + typeRaw, ok := d.GetOk("type") + if !ok || strings.ToLower(typeRaw.(string)) != "irrevocable" { + return nil, nil + } + + includeChildNamespacesRaw, ok := d.GetOk("include_child_namespaces") + includeChildNamespaces := ok && includeChildNamespacesRaw.(bool) resp, err := b.Core.expiration.getIrrevocableLeaseCounts(ctx, includeChildNamespaces) if err != nil { @@ -300,7 +305,25 @@ func (b *SystemBackend) handleLeaseCount(ctx context.Context, req *logical.Reque } func (b *SystemBackend) handleLeaseList(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { + typeRaw, ok := d.GetOk("type") + if !ok || strings.ToLower(typeRaw.(string)) != "irrevocable" { + return nil, nil + } + + includeChildNamespacesRaw, ok := d.GetOk("include_child_namespaces") + includeChildNamespaces := ok && includeChildNamespacesRaw.(bool) + + forceRaw, ok := d.GetOk("force") + force := ok && forceRaw.(bool) + resp, err := b.Core.expiration.listIrrevocableLeases(ctx, includeChildNamespaces, force) + if err != nil { + return nil, err + } + + return &logical.Response{ + Data: resp, + }, nil } func (b *SystemBackend) handlePluginCatalogTypedList(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { From 41dfabaec84dad9f711ffaab79dbc4febf26f968 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Thu, 13 May 2021 13:26:55 -0600 Subject: [PATCH 03/24] bookkeeping --- changelog/11607.txt | 3 + website/content/api-docs/system/leases.mdx | 65 ++++++++++++++++++++++ 2 files changed, 68 insertions(+) create mode 100644 changelog/11607.txt diff --git a/changelog/11607.txt b/changelog/11607.txt new file mode 100644 index 0000000000000..4404a23d981c2 --- /dev/null +++ b/changelog/11607.txt @@ -0,0 +1,3 @@ +```release-note:improvement +core: add irrevocable lease list and count apis +``` \ No newline at end of file diff --git a/website/content/api-docs/system/leases.mdx b/website/content/api-docs/system/leases.mdx index a7f55f2f338de..7431e4f7a9b44 100644 --- a/website/content/api-docs/system/leases.mdx +++ b/website/content/api-docs/system/leases.mdx @@ -243,3 +243,68 @@ $ curl \ --request POST \ http://127.0.0.1:8200/v1/sys/leases/tidy ``` + +## Irrevocable Leases Counts + +This endpoint returns the total count of irrevocable leases, as well as a count +per mount point. + +This can help determine if particular endpoints are disproportionately +resulting in irrevocable leases. + +This endpoint was added in Vault 1.8. + +### Parameters + +- `type` (string: ) - Specifies the type of lease. Set to "irrevocable" +- `include_child_namespaces` (bool: false) - Specifies if irrevocable leases in + child namespaces should be included in the result + +| Method | Path | +| :----- | :----------------- | +| `GET` | `/sys/leases/count` | + +### Sample Request + +```shell-session +$ curl \ + --header "X-Vault-Token: ..." \ + --request GET \ + http://127.0.0.1:8200/v1/sys/leases/count \ + -d type=irrevocable +``` + +## Irrevocable Leases List + +This endpoint returns the total count of irrevocable leases, as well as a list +of irrevocable leases per mount point - complete with the error that marked +the lease as irrevocable. + +This can help determine if particular endpoints or causes are disproportionately +resulting in irrevocable leases. + +This endpoint was added in Vault 1.8. + +### Parameters + +- `type` (string: ) - Specifies the type of lease. Set to "irrevocable" +- `include_child_namespaces` (bool: false) - Specifies if irrevocable leases in + child namespaces should be included in the result +- `force` (bool: false) - Specifies if the request should complete if there are + more than 10,000 results to return. If this is set false, the client will + send an error specifying that there are many results, and the request must + be retried using the `force` flag. + +| Method | Path | +| :----- | :------------ | +| `LIST` | `/sys/leases` | + +### Sample Request + +```shell-session +$ curl \ + --header "X-Vault-Token: ..." \ + --request LIST \ + http://127.0.0.1:8200/v1/sys/leases \ + -d type=irrevocable +``` From ed4f011d93d05deeb5e034c0f017af90f35d03ff Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Thu, 20 May 2021 09:51:14 -0600 Subject: [PATCH 04/24] test irrevocable lease counts for API/CLI --- vault/expiration.go | 24 ++++++--- vault/expiration_test.go | 112 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 8 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index d504f530f1774..7f447b881eefc 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -269,11 +269,11 @@ func (r *revocationJob) OnFailure(err error) { func expireLeaseStrategyFairsharing(ctx context.Context, m *ExpirationManager, leaseID string, ns *namespace.Namespace) { nsCtx := namespace.ContextWithNamespace(ctx, ns) - var mountAccessor string m.coreStateLock.RLock() mount := m.core.router.MatchingMountEntry(nsCtx, leaseID) m.coreStateLock.RUnlock() + var mountAccessor string if mount == nil { // figure out what this means - if we couldn't find the mount, can we automatically revoke m.logger.Debug("could not find lease path", "lease_id", leaseID) @@ -2445,8 +2445,6 @@ func (m *ExpirationManager) getIrrevocableLeaseCounts(ctx context.Context, inclu numMatchingLeases := 0 m.irrevocable.Range(func(k, v interface{}) bool { leaseID := k.(string) - leaseInfo := v.(*leaseEntry) - leaseNS, err := m.getNamespaceFromLeaseID(ctx, leaseID) if err != nil { // We probably want to track that an error occured, but continue counting @@ -2460,20 +2458,30 @@ func (m *ExpirationManager) getIrrevocableLeaseCounts(ctx context.Context, inclu return true } - // TODO verify path is kept - if _, ok := numMatchingLeasesPerMount[leaseInfo.Path]; !ok { - numMatchingLeasesPerMount[leaseInfo.Path] = 0 + m.coreStateLock.RLock() + mount := m.core.router.MatchingMountEntry(ctx, leaseID) + m.coreStateLock.RUnlock() + + var mountAccessor string + if mount == nil { + mountAccessor = "mount-accessor-not-found" + } else { + mountAccessor = mount.Accessor + } + + if _, ok := numMatchingLeasesPerMount[mountAccessor]; !ok { + numMatchingLeasesPerMount[mountAccessor] = 0 } numMatchingLeases++ - numMatchingLeasesPerMount[leaseInfo.Path]++ + numMatchingLeasesPerMount[mountAccessor]++ return true }) resp := make(map[string]interface{}) resp["lease_count"] = numMatchingLeases - resp["counts"] = numMatchingLeases + resp["counts"] = numMatchingLeasesPerMount return resp, nil } diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 2447332dad770..4b47fecf5c249 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -2869,3 +2869,115 @@ func TestExpiration_unrecoverableErrorMakesIrrevocable(t *testing.T) { } } } + +func addIrrevocableLease(t *testing.T, m *ExpirationManager, pathPrefix string, ns *namespace.Namespace) { + t.Helper() + + uuid, err := uuid.GenerateUUID() + if err != nil { + t.Fatalf("error generating uuid: %v", err) + } + + if ns == nil { + ns = namespace.RootNamespace + } + + le := &leaseEntry{ + LeaseID: pathPrefix + "lease" + uuid, + Path: pathPrefix, + namespace: ns, + IssueTime: time.Now(), + ExpireTime: time.Now().Add(time.Hour), + RevokeErr: "some error message", + } + + m.pendingLock.Lock() + defer m.pendingLock.Unlock() + + if err := m.persistEntry(context.Background(), le); err != nil { + t.Fatalf("error persisting irrevocable lease: %v", err) + } + + m.updatePendingInternal(le) +} + +// set up multiple mounts, and return a mapping of the path to the mount accessor +func mountNoopBackends(t *testing.T, c *Core, paths []string) map[string]string { + t.Helper() + + // enable the noop backend + c.logicalBackends["noop"] = func(ctx context.Context, config *logical.BackendConfig) (logical.Backend, error) { + return &NoopBackend{}, nil + } + + pathToMount := make(map[string]string) + for _, path := range paths { + me := &MountEntry{ + Table: mountTableType, + Path: path, + Type: "noop", + } + err := c.mount(namespace.RootContext(nil), me) + if err != nil { + t.Fatalf("err mounting backend %s: %v", path, err) + } + + mount := c.router.MatchingMountEntry(namespace.RootContext(nil), path) + if mount == nil { + t.Fatalf("couldn't map path to mount") + } + pathToMount[path] = mount.Accessor + } + + return pathToMount +} + +func TestExpiration_getIrrevocableLeaseCounts(t *testing.T) { + c, _, _ := TestCoreUnsealed(t) + + mountPrefixes := []string{"foo/bar/1/", "foo/bar/2/", "foo/bar/3/"} + pathToMount := mountNoopBackends(t, c, mountPrefixes) + + exp := c.expiration + + expectedPerMount := 10 + for i := 0; i < expectedPerMount; i++ { + for _, mountPrefix := range mountPrefixes { + addIrrevocableLease(t, exp, mountPrefix, namespace.RootNamespace) + } + } + + out, err := exp.getIrrevocableLeaseCounts(namespace.RootContext(nil), false) + if err != nil { + t.Fatalf("error getting irrevocable lease counts: %v", err) + } + + countRaw, ok := out["lease_count"] + if !ok { + t.Fatalf("no lease count") + } + + countPerMountRaw, ok := out["counts"] + if !ok { + t.Fatalf("no count per mount") + } + + count := countRaw.(int) + countPerMount := countPerMountRaw.(map[string]int) + + expectedCount := len(mountPrefixes) * expectedPerMount + if count != expectedCount { + t.Errorf("bad count. expected %d, got %d", expectedCount, count) + } + + if len(countPerMount) != len(mountPrefixes) { + t.Fatalf("bad mounts. got %#v, expected %v", countPerMount, mountPrefixes) + } + + for _, mountPrefix := range mountPrefixes { + mountCount := countPerMount[pathToMount[mountPrefix]] + if mountCount != expectedPerMount { + t.Errorf("bad count for prefix %q. expected %d, got %d", mountPrefix, expectedPerMount, mountCount) + } + } +} From e76af8a70c56f827faa69b75f4a959ecc5cbad58 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Thu, 20 May 2021 12:28:33 -0600 Subject: [PATCH 05/24] fix listIrrevocableLeases, test listIrrevocableLeases, cleanup --- vault/expiration.go | 43 +++++++++++------------ vault/expiration_test.go | 74 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 91 insertions(+), 26 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index 7f447b881eefc..105e48e1f431f 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -269,18 +269,7 @@ func (r *revocationJob) OnFailure(err error) { func expireLeaseStrategyFairsharing(ctx context.Context, m *ExpirationManager, leaseID string, ns *namespace.Namespace) { nsCtx := namespace.ContextWithNamespace(ctx, ns) - m.coreStateLock.RLock() - mount := m.core.router.MatchingMountEntry(nsCtx, leaseID) - m.coreStateLock.RUnlock() - - var mountAccessor string - if mount == nil { - // figure out what this means - if we couldn't find the mount, can we automatically revoke - m.logger.Debug("could not find lease path", "lease_id", leaseID) - mountAccessor = "mount-accessor-not-found" - } else { - mountAccessor = mount.Accessor - } + mountAccessor := m.getLeaseMountAccessor(ctx, leaseID) job, err := newRevocationJob(nsCtx, leaseID, ns, m) if err != nil { @@ -2433,6 +2422,21 @@ func (m *ExpirationManager) getNamespaceFromLeaseID(ctx context.Context, leaseID return leaseNS, nil } +func (m *ExpirationManager) getLeaseMountAccessor(ctx context.Context, leaseID string) string { + m.coreStateLock.RLock() + mount := m.core.router.MatchingMountEntry(ctx, leaseID) + m.coreStateLock.RUnlock() + + var mountAccessor string + if mount == nil { + mountAccessor = "mount-accessor-not-found" + } else { + mountAccessor = mount.Accessor + } + + return mountAccessor +} + // TODO if keep counts as a map, should update the RFC func (m *ExpirationManager) getIrrevocableLeaseCounts(ctx context.Context, includeChildNamespaces bool) (map[string]interface{}, error) { requestNS, err := namespace.FromContext(ctx) @@ -2458,16 +2462,7 @@ func (m *ExpirationManager) getIrrevocableLeaseCounts(ctx context.Context, inclu return true } - m.coreStateLock.RLock() - mount := m.core.router.MatchingMountEntry(ctx, leaseID) - m.coreStateLock.RUnlock() - - var mountAccessor string - if mount == nil { - mountAccessor = "mount-accessor-not-found" - } else { - mountAccessor = mount.Accessor - } + mountAccessor := m.getLeaseMountAccessor(ctx, leaseID) if _, ok := numMatchingLeasesPerMount[mountAccessor]; !ok { numMatchingLeasesPerMount[mountAccessor] = 0 @@ -2525,8 +2520,10 @@ func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeCh return false } + mountAccessor := m.getLeaseMountAccessor(ctx, leaseID) + numMatchingLeases++ - matchingLeasesPerMount[leaseInfo.Path] = append(matchingLeasesPerMount[leaseInfo.Path], &leaseResponse{ + matchingLeasesPerMount[mountAccessor] = append(matchingLeasesPerMount[mountAccessor], &leaseResponse{ LeaseID: leaseID, ErrMsg: leaseInfo.RevokeErr, }) diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 4b47fecf5c249..11437dd088f21 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -2870,7 +2870,8 @@ func TestExpiration_unrecoverableErrorMakesIrrevocable(t *testing.T) { } } -func addIrrevocableLease(t *testing.T, m *ExpirationManager, pathPrefix string, ns *namespace.Namespace) { +// returns the lease ID for the lease +func addIrrevocableLease(t *testing.T, m *ExpirationManager, pathPrefix string, ns *namespace.Namespace) string { t.Helper() uuid, err := uuid.GenerateUUID() @@ -2899,6 +2900,8 @@ func addIrrevocableLease(t *testing.T, m *ExpirationManager, pathPrefix string, } m.updatePendingInternal(le) + + return le.LeaseID } // set up multiple mounts, and return a mapping of the path to the mount accessor @@ -2954,12 +2957,12 @@ func TestExpiration_getIrrevocableLeaseCounts(t *testing.T) { countRaw, ok := out["lease_count"] if !ok { - t.Fatalf("no lease count") + t.Fatal("no lease count") } countPerMountRaw, ok := out["counts"] if !ok { - t.Fatalf("no count per mount") + t.Fatal("no count per mount") } count := countRaw.(int) @@ -2981,3 +2984,68 @@ func TestExpiration_getIrrevocableLeaseCounts(t *testing.T) { } } } + +func TestExpiration_listIrrevocableLeases(t *testing.T) { + c, _, _ := TestCoreUnsealed(t) + + mountPrefixes := []string{"foo/bar/1/", "foo/bar/2/", "foo/bar/3/"} + pathToMount := mountNoopBackends(t, c, mountPrefixes) + + exp := c.expiration + + expectedLeasesPerMount := make(map[string][]string) + expectedPerMount := 10 + for i := 0; i < expectedPerMount; i++ { + for _, mountPrefix := range mountPrefixes { + leaseID := addIrrevocableLease(t, exp, mountPrefix, namespace.RootNamespace) + expectedLeasesPerMount[mountPrefix] = append(expectedLeasesPerMount[mountPrefix], leaseID) + } + } + + out, err := exp.listIrrevocableLeases(namespace.RootContext(nil), false, false) + if err != nil { + t.Fatalf("error listing irrevocable leases: %v", err) + } + + countRaw, ok := out["lease_count"] + if !ok { + t.Fatal("no lease count") + } + + leasesRaw, ok := out["leases"] + if !ok { + t.Fatal("no leases") + } + + count := countRaw.(int) + leases := leasesRaw.(map[string][]*leaseResponse) + + expectedCount := len(mountPrefixes) * expectedPerMount + if count != expectedCount { + t.Errorf("bad count. expected %d, got %d", expectedCount, count) + } + + for _, mountPrefix := range mountPrefixes { + mount := pathToMount[mountPrefix] + + // sort both sets of data for easy comparison + sort.Strings(expectedLeasesPerMount[mountPrefix]) + sort.Slice(leases[mount], func(i, j int) bool { + return leases[mount][i].LeaseID < leases[mount][j].LeaseID + }) + + if len(leases[mount]) != expectedPerMount { + t.Fatalf("bad leases for mount prefix %q: expected %d, got %d", mountPrefix, expectedPerMount, len(leases[mount])) + } + + for i, v := range expectedLeasesPerMount[mountPrefix] { + lease := leases[mount][i] + if lease.LeaseID != v { + t.Errorf("bad leaseID for mount prefix %q: expected %s, got %s", mountPrefix, v, lease.LeaseID) + } + if lease.ErrMsg == "" { + t.Errorf("no error message for irrevocable leaseID %q", lease.LeaseID) + } + } + } +} From 6ab77e418c11093ce54eb9f6ead5cff9b9383855 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Thu, 20 May 2021 13:11:51 -0600 Subject: [PATCH 06/24] test expiration API limit --- vault/expiration.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index 105e48e1f431f..4caef0f962a26 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -80,7 +80,7 @@ const ( ) var ( - hitMaxIrrevocableLeases = errors.New("Command cancelled because many irrevocable leases were found. To emit the entire list, re-run the command with the force flag set true.") + errHitMaxIrrevocableLeases = errors.New("Command cancelled because many irrevocable leases were found. To emit the entire list, re-run the command with the force flag set true.") ) type pendingInfo struct { @@ -2516,7 +2516,7 @@ func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeCh if !force && (numMatchingLeases >= maxIrrevocableLeasesToReturn) { m.logger.Warn("hit max irrevocable leases without force flag set") - retErr = hitMaxIrrevocableLeases + retErr = errHitMaxIrrevocableLeases return false } From 61786f5d6932363989627ba6925a8ea592f95b62 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Thu, 20 May 2021 14:41:09 -0600 Subject: [PATCH 07/24] namespace tweaks, test force flag on lease list --- vault/expiration_test.go | 48 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 11437dd088f21..9354e7b1c5a4a 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -2883,9 +2883,14 @@ func addIrrevocableLease(t *testing.T, m *ExpirationManager, pathPrefix string, ns = namespace.RootNamespace } + nsSuffix := "" + if ns != namespace.RootNamespace { + nsSuffix = fmt.Sprintf("/blah.%s", ns.ID) + } + le := &leaseEntry{ - LeaseID: pathPrefix + "lease" + uuid, - Path: pathPrefix, + LeaseID: pathPrefix + "lease" + uuid + nsSuffix, + Path: pathPrefix + nsSuffix, namespace: ns, IssueTime: time.Now(), ExpireTime: time.Now().Add(time.Hour), @@ -3049,3 +3054,42 @@ func TestExpiration_listIrrevocableLeases(t *testing.T) { } } } + +func TestExpiration_listIrrevocableLeases_force(t *testing.T) { + c, _, _ := TestCoreUnsealed(t) + exp := c.expiration + + expectedNumLeases := maxIrrevocableLeasesToReturn + 10 + for i := 0; i < expectedNumLeases; i++ { + addIrrevocableLease(t, exp, "foo/", namespace.RootNamespace) + } + + dataRaw, err := exp.listIrrevocableLeases(namespace.RootContext(nil), false, false) + if err == nil { + t.Fatalf("expected error - more than max number of irrevocable leases") + } + if dataRaw != nil { + t.Fatalf("expected nil data, got %#v", dataRaw) + } + + dataRaw, err = exp.listIrrevocableLeases(namespace.RootContext(nil), false, true) + if err != nil { + t.Fatalf("got error on force list leases: %v", err) + } + if dataRaw == nil { + t.Fatalf("got nil data on force list leases") + } + + numLeasesRaw, ok := dataRaw["lease_count"] + if !ok { + t.Fatalf("lease count data not present") + } + if numLeasesRaw == nil { + t.Fatalf("nil lease count") + } + + numLeases := numLeasesRaw.(int) + if numLeases != expectedNumLeases { + t.Errorf("bad lease count. expected %d, got %d", expectedNumLeases, numLeases) + } +} From 7bc671286b8e099a24c675feaf31675f8dc084ed Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Thu, 20 May 2021 19:22:07 -0600 Subject: [PATCH 08/24] integration test leases/count API, plenty of fixes and improvements --- vault/expiration_test.go | 39 -------- vault/expiration_util.go | 63 +++++++++++++ .../expiration/expiration_test.go | 90 +++++++++++++++++++ vault/logical_system_paths.go | 10 +++ 4 files changed, 163 insertions(+), 39 deletions(-) create mode 100644 vault/external_tests/expiration/expiration_test.go diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 9354e7b1c5a4a..bf3f462bcc6fb 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -2870,45 +2870,6 @@ func TestExpiration_unrecoverableErrorMakesIrrevocable(t *testing.T) { } } -// returns the lease ID for the lease -func addIrrevocableLease(t *testing.T, m *ExpirationManager, pathPrefix string, ns *namespace.Namespace) string { - t.Helper() - - uuid, err := uuid.GenerateUUID() - if err != nil { - t.Fatalf("error generating uuid: %v", err) - } - - if ns == nil { - ns = namespace.RootNamespace - } - - nsSuffix := "" - if ns != namespace.RootNamespace { - nsSuffix = fmt.Sprintf("/blah.%s", ns.ID) - } - - le := &leaseEntry{ - LeaseID: pathPrefix + "lease" + uuid + nsSuffix, - Path: pathPrefix + nsSuffix, - namespace: ns, - IssueTime: time.Now(), - ExpireTime: time.Now().Add(time.Hour), - RevokeErr: "some error message", - } - - m.pendingLock.Lock() - defer m.pendingLock.Unlock() - - if err := m.persistEntry(context.Background(), le); err != nil { - t.Fatalf("error persisting irrevocable lease: %v", err) - } - - m.updatePendingInternal(le) - - return le.LeaseID -} - // set up multiple mounts, and return a mapping of the path to the mount accessor func mountNoopBackends(t *testing.T, c *Core, paths []string) map[string]string { t.Helper() diff --git a/vault/expiration_util.go b/vault/expiration_util.go index eac1703bc6190..660a690890e7c 100644 --- a/vault/expiration_util.go +++ b/vault/expiration_util.go @@ -3,8 +3,12 @@ package vault import ( + "context" "fmt" + "testing" + "time" + uuid "github.com/hashicorp/go-uuid" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/sdk/logical" ) @@ -28,3 +32,62 @@ func (m *ExpirationManager) collectLeases() (map[*namespace.Namespace][]string, leaseCount += len(keys) return existing, leaseCount, nil } + +// add an irrevocable lease for test purposes +// returns the lease ID for the lease +func addIrrevocableLease(t *testing.T, m *ExpirationManager, pathPrefix string, ns *namespace.Namespace) string { + t.Helper() + + uuid, err := uuid.GenerateUUID() + if err != nil { + t.Fatalf("error generating uuid: %v", err) + } + + if ns == nil { + ns = namespace.RootNamespace + } + + nsSuffix := "" + if ns != namespace.RootNamespace { + nsSuffix = fmt.Sprintf("/blah.%s", ns.ID) + } + + le := &leaseEntry{ + LeaseID: pathPrefix + "lease" + uuid + nsSuffix, + Path: pathPrefix + nsSuffix, + namespace: ns, + IssueTime: time.Now(), + ExpireTime: time.Now().Add(time.Hour), + RevokeErr: "some error message", + } + + m.pendingLock.Lock() + defer m.pendingLock.Unlock() + + if err := m.persistEntry(context.Background(), le); err != nil { + t.Fatalf("error persisting irrevocable lease: %v", err) + } + + m.updatePendingInternal(le) + + return le.LeaseID +} + +// InjectIrrevocableLeases injects `count` irrevocable leases (currently to a +// single mount). +// It returns a map of the mount accessor to the number of leases stored there +func (c *Core) InjectIrrevocableLeases(t *testing.T, ctx context.Context, count int) map[string]int { + out := make(map[string]int) + for i := 0; i < count; i++ { + leaseID := addIrrevocableLease(t, c.expiration, "foo/", namespace.RootNamespace) + + mountAccessor := c.expiration.getLeaseMountAccessor(ctx, leaseID) + if _, ok := out[mountAccessor]; !ok { + out[mountAccessor] = 0 + } + + out[mountAccessor]++ + } + + return out +} diff --git a/vault/external_tests/expiration/expiration_test.go b/vault/external_tests/expiration/expiration_test.go new file mode 100644 index 0000000000000..c8e891bc7ae31 --- /dev/null +++ b/vault/external_tests/expiration/expiration_test.go @@ -0,0 +1,90 @@ +package expiration + +import ( + "encoding/json" + "testing" + + "github.com/hashicorp/vault/helper/namespace" + vaulthttp "github.com/hashicorp/vault/http" + "github.com/hashicorp/vault/vault" +) + +func TestExpiration_irrevocableLeaseCountsAPI(t *testing.T) { + cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ + HandlerFunc: vaulthttp.Handler, + NumCores: 1, + }) + cluster.Start() + defer cluster.Cleanup() + + client := cluster.Cores[0].Client + core := cluster.Cores[0].Core + + params := make(map[string][]string) + params["type"] = []string{"irrevocable"} + resp, err := client.Logical().ReadWithData("sys/leases/count", params) + if err != nil { + t.Fatal(err) + } + if resp == nil { + t.Fatal("response is nil") + } + totalLeaseCountRaw, ok := resp.Data["lease_count"] + if !ok { + t.Fatalf("expected 'lease_count' response, got: %#v", resp.Data) + } + + totalLeaseCount, err := totalLeaseCountRaw.(json.Number).Int64() + if err != nil { + t.Fatalf("error extracting lease count: %v", err) + } + if totalLeaseCount != 0 { + t.Errorf("expected no leases, got %d", totalLeaseCount) + } + + expectedNumLeases := 50 + expectedCountsPerMount := core.InjectIrrevocableLeases(t, namespace.RootContext(nil), expectedNumLeases) + + resp, err = client.Logical().ReadWithData("sys/leases/count", params) + if err != nil { + t.Fatal(err) + } + if resp == nil { + t.Fatal("response is nil") + } + totalLeaseCountRaw, ok = resp.Data["lease_count"] + if !ok { + t.Fatalf("expected 'lease_count' response, got: %#v", resp.Data) + } + + totalLeaseCount, err = totalLeaseCountRaw.(json.Number).Int64() + if err != nil { + t.Fatalf("error extracting lease count: %v", err) + } + if totalLeaseCount != int64(expectedNumLeases) { + t.Errorf("expected %d leases, got %d", expectedNumLeases, totalLeaseCount) + } + + countsPerMountRaw, ok := resp.Data["counts"] + if !ok { + t.Fatalf("expected 'counts' response, got %#v", resp.Data) + } + + countsPerMount := countsPerMountRaw.(map[string]interface{}) + for mount, expectedCount := range expectedCountsPerMount { + gotCountRaw, ok := countsPerMount[mount] + if !ok { + t.Errorf("missing mount %q", mount) + continue + } + + gotCount, err := gotCountRaw.(json.Number).Int64() + if err != nil { + t.Errorf("error extracting lease count for mount %q: %v", mount, err) + continue + } + if gotCount != int64(expectedCount) { + t.Errorf("bad count for mount %q: expected: %d, got: %d", mount, expectedCount, gotCount) + } + } +} diff --git a/vault/logical_system_paths.go b/vault/logical_system_paths.go index c9b818301427b..ba33b25f79502 100644 --- a/vault/logical_system_paths.go +++ b/vault/logical_system_paths.go @@ -1213,6 +1213,16 @@ func (b *SystemBackend) leasePaths() []*framework.Path { { Pattern: "leases/count$", + Fields: map[string]*framework.FieldSchema{ + "type": { + Type: framework.TypeString, + Description: "Type of leases to get counts for (currently only supporting irrevocable).", + }, + "include_child_namespaces": { + Type: framework.TypeBool, + Description: "Set true if you want counts for this namespace and its children.", + }, + }, Callbacks: map[logical.Operation]framework.OperationFunc{ // currently only works for irrevocable leases with param: type=irrevocable From e18fad463360c11f3d9cf415ce53c41ed5ee2ba7 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Thu, 20 May 2021 19:44:34 -0600 Subject: [PATCH 09/24] test lease list API, fixes and improvements --- vault/expiration.go | 4 +- .../expiration/expiration_test.go | 100 +++++++++++++++++- vault/logical_system_paths.go | 16 ++- 3 files changed, 112 insertions(+), 8 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index aa090b4764558..986a1e2ed0f78 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -76,7 +76,7 @@ const ( // maximum number of irrevocable leases we return to the irrevocable lease // list API **without** the `force` flag set - maxIrrevocableLeasesToReturn = 10000 + MaxIrrevocableLeasesToReturn = 10000 ) var ( @@ -2521,7 +2521,7 @@ func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeCh return true } - if !force && (numMatchingLeases >= maxIrrevocableLeasesToReturn) { + if !force && (numMatchingLeases >= MaxIrrevocableLeasesToReturn) { m.logger.Warn("hit max irrevocable leases without force flag set") retErr = errHitMaxIrrevocableLeases return false diff --git a/vault/external_tests/expiration/expiration_test.go b/vault/external_tests/expiration/expiration_test.go index c8e891bc7ae31..8a711d3abd4de 100644 --- a/vault/external_tests/expiration/expiration_test.go +++ b/vault/external_tests/expiration/expiration_test.go @@ -42,8 +42,17 @@ func TestExpiration_irrevocableLeaseCountsAPI(t *testing.T) { t.Errorf("expected no leases, got %d", totalLeaseCount) } + countPerMountRaw, ok := resp.Data["counts"] + if !ok { + t.Fatalf("expected 'counts' response, got %#v", resp.Data) + } + countPerMount := countPerMountRaw.(map[string]interface{}) + if len(countPerMount) != 0 { + t.Errorf("expected no mounts with counts, got %#v", countPerMount) + } + expectedNumLeases := 50 - expectedCountsPerMount := core.InjectIrrevocableLeases(t, namespace.RootContext(nil), expectedNumLeases) + expectedCountPerMount := core.InjectIrrevocableLeases(t, namespace.RootContext(nil), expectedNumLeases) resp, err = client.Logical().ReadWithData("sys/leases/count", params) if err != nil { @@ -65,14 +74,14 @@ func TestExpiration_irrevocableLeaseCountsAPI(t *testing.T) { t.Errorf("expected %d leases, got %d", expectedNumLeases, totalLeaseCount) } - countsPerMountRaw, ok := resp.Data["counts"] + countPerMountRaw, ok = resp.Data["counts"] if !ok { t.Fatalf("expected 'counts' response, got %#v", resp.Data) } - countsPerMount := countsPerMountRaw.(map[string]interface{}) - for mount, expectedCount := range expectedCountsPerMount { - gotCountRaw, ok := countsPerMount[mount] + countPerMount = countPerMountRaw.(map[string]interface{}) + for mount, expectedCount := range expectedCountPerMount { + gotCountRaw, ok := countPerMount[mount] if !ok { t.Errorf("missing mount %q", mount) continue @@ -88,3 +97,84 @@ func TestExpiration_irrevocableLeaseCountsAPI(t *testing.T) { } } } + +func TestExpiration_irrevocableLeaseListAPI(t *testing.T) { + cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ + HandlerFunc: vaulthttp.Handler, + NumCores: 1, + }) + cluster.Start() + defer cluster.Cleanup() + + client := cluster.Cores[0].Client + core := cluster.Cores[0].Core + + params := make(map[string][]string) + params["type"] = []string{"irrevocable"} + // TODO update RFC to say that it's a get operation + resp, err := client.Logical().ReadWithData("sys/leases", params) + if err != nil { + t.Fatal(err) + } + if resp == nil { + t.Fatal("response is nil") + } + totalLeaseCountRaw, ok := resp.Data["lease_count"] + if !ok { + t.Fatalf("expected 'lease_count' response, got: %#v", resp.Data) + } + + totalLeaseCount, err := totalLeaseCountRaw.(json.Number).Int64() + if err != nil { + t.Fatalf("error extracting lease count: %v", err) + } + if totalLeaseCount != 0 { + t.Errorf("expected no leases, got %d", totalLeaseCount) + } + + leasesPerMountRaw, ok := resp.Data["leases"] + if !ok { + t.Fatalf("expected 'leases' response, got %#v", resp.Data) + } + leasesPerMount := leasesPerMountRaw.(map[string]interface{}) + if len(leasesPerMount) != 0 { + t.Errorf("expected no mounts with leases, got %#v", leasesPerMount) + } + + // test with a low enough number to not give an error without force flag + expectedNumLeases := 50 + expectedCountsPerMount := core.InjectIrrevocableLeases(t, namespace.RootContext(nil), expectedNumLeases) + + resp, err = client.Logical().ReadWithData("sys/leases", params) + if err != nil { + t.Fatal(err) + } + if resp == nil { + t.Fatal("response is nil") + } + totalLeaseCountRaw, ok = resp.Data["lease_count"] + if !ok { + t.Fatalf("expected 'lease_count' response, got: %#v", resp.Data) + } + + totalLeaseCount, err = totalLeaseCountRaw.(json.Number).Int64() + if err != nil { + t.Fatalf("error extracting lease count: %v", err) + } + if totalLeaseCount != int64(expectedNumLeases) { + t.Errorf("expected %d leases, got %d", expectedNumLeases, totalLeaseCount) + } + + leasesPerMountRaw, ok = resp.Data["leases"] + if !ok { + t.Fatalf("expected 'leases' response, got %#v", resp.Data) + } + + leasesPerMount = leasesPerMountRaw.(map[string]interface{}) + for mount, expectedCount := range expectedCountsPerMount { + leaseCount := len(leasesPerMount[mount].([]interface{})) + if leaseCount != expectedCount { + t.Errorf("bad count for mount %q, expected %d, got %d", mount, expectedCount, leaseCount) + } + } +} diff --git a/vault/logical_system_paths.go b/vault/logical_system_paths.go index ba33b25f79502..9045ec7ca9d3f 100644 --- a/vault/logical_system_paths.go +++ b/vault/logical_system_paths.go @@ -1235,10 +1235,24 @@ func (b *SystemBackend) leasePaths() []*framework.Path { { Pattern: "leases(/)?$", + Fields: map[string]*framework.FieldSchema{ + "type": { + Type: framework.TypeString, + Description: "Type of leases to get counts for (currently only supporting irrevocable).", + }, + "include_child_namespaces": { + Type: framework.TypeBool, + Description: "Set true if you want counts for this namespace and its children.", + }, + "force": { + Type: framework.TypeBool, + Description: "Set true if to get lists containing more than 10,000 entries.", + }, + }, Callbacks: map[logical.Operation]framework.OperationFunc{ // currently only works for irrevocable leases with param: type=irrevocable - logical.ListOperation: b.handleLeaseList, + logical.ReadOperation: b.handleLeaseList, }, HelpSynopsis: strings.TrimSpace(sysHelp["list-leases"][0]), From 61e83161435f4cf16cb25e468a3cfa98fe1f3459 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Thu, 20 May 2021 19:52:52 -0600 Subject: [PATCH 10/24] test force flag for irrevocable lease list API --- vault/expiration.go | 2 +- .../expiration/expiration_test.go | 62 +++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/vault/expiration.go b/vault/expiration.go index 986a1e2ed0f78..eac1346f6fcd5 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -80,7 +80,7 @@ const ( ) var ( - errHitMaxIrrevocableLeases = errors.New("Command cancelled because many irrevocable leases were found. To emit the entire list, re-run the command with the force flag set true.") + errHitMaxIrrevocableLeases = errors.New("Command cancelled because many irrevocable leases were found. To emit the entire list, re-run the command with force set true.") ) type pendingInfo struct { diff --git a/vault/external_tests/expiration/expiration_test.go b/vault/external_tests/expiration/expiration_test.go index 8a711d3abd4de..9d1c9a631c1f6 100644 --- a/vault/external_tests/expiration/expiration_test.go +++ b/vault/external_tests/expiration/expiration_test.go @@ -178,3 +178,65 @@ func TestExpiration_irrevocableLeaseListAPI(t *testing.T) { } } } + +func TestExpiration_irrevocableLeaseListAPI_force(t *testing.T) { + cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ + HandlerFunc: vaulthttp.Handler, + NumCores: 1, + }) + cluster.Start() + defer cluster.Cleanup() + + client := cluster.Cores[0].Client + core := cluster.Cores[0].Core + + // test with a low enough number to not give an error without force flag + expectedNumLeases := vault.MaxIrrevocableLeasesToReturn + 50 + expectedCountsPerMount := core.InjectIrrevocableLeases(t, namespace.RootContext(nil), expectedNumLeases) + + params := make(map[string][]string) + params["type"] = []string{"irrevocable"} + + resp, err := client.Logical().ReadWithData("sys/leases", params) + if err == nil { + t.Fatalf("expected error without force flag") + } + if resp != nil { + t.Errorf("expected nil response, got: %#v", resp) + } + + // now try it with the force flag - we expect no errors and many results + params["force"] = []string{"true"} + resp, err = client.Logical().ReadWithData("sys/leases", params) + if err != nil { + t.Fatalf("unexpected error when using force flag: %v", err) + } + if resp == nil { + t.Fatal("response is nil") + } + totalLeaseCountRaw, ok := resp.Data["lease_count"] + if !ok { + t.Fatalf("expected 'lease_count' response, got: %#v", resp.Data) + } + + totalLeaseCount, err := totalLeaseCountRaw.(json.Number).Int64() + if err != nil { + t.Fatalf("error extracting lease count: %v", err) + } + if totalLeaseCount != int64(expectedNumLeases) { + t.Errorf("expected %d leases, got %d", expectedNumLeases, totalLeaseCount) + } + + leasesPerMountRaw, ok := resp.Data["leases"] + if !ok { + t.Fatalf("expected 'leases' response, got %#v", resp.Data) + } + + leasesPerMount := leasesPerMountRaw.(map[string]interface{}) + for mount, expectedCount := range expectedCountsPerMount { + leaseCount := len(leasesPerMount[mount].([]interface{})) + if leaseCount != expectedCount { + t.Errorf("bad count for mount %q, expected %d, got %d", mount, expectedCount, leaseCount) + } + } +} From d5164e3193ff7cc4b93c13435491104858fdc908 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Fri, 21 May 2021 10:18:20 -0600 Subject: [PATCH 11/24] i guess this wasn't saved on the last refactor... --- vault/expiration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 9b280aea1600b..503091a2e8786 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -3156,7 +3156,7 @@ func TestExpiration_listIrrevocableLeases_force(t *testing.T) { c, _, _ := TestCoreUnsealed(t) exp := c.expiration - expectedNumLeases := maxIrrevocableLeasesToReturn + 10 + expectedNumLeases := MaxIrrevocableLeasesToReturn + 10 for i := 0; i < expectedNumLeases; i++ { addIrrevocableLease(t, exp, "foo/", namespace.RootNamespace) } From fb8b995f23537e7914c8fda697849d0aaf422f22 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Fri, 21 May 2021 10:59:37 -0600 Subject: [PATCH 12/24] fixes and improvements found during my review --- vault/expiration.go | 6 ++++-- vault/external_tests/expiration/expiration_test.go | 13 ++++++++++++- vault/logical_system_paths.go | 7 ++++++- website/content/api-docs/system/leases.mdx | 6 +++--- 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index eac1346f6fcd5..aae1e14bb28ea 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -2417,6 +2417,8 @@ func (m *ExpirationManager) markLeaseIrrevocable(ctx context.Context, le *leaseE func (m *ExpirationManager) getNamespaceFromLeaseID(ctx context.Context, leaseID string) (*namespace.Namespace, error) { _, nsID := namespace.SplitIDFromString(leaseID) + + // avoid re-declaring leaseNS and err with scope inside the if leaseNS := namespace.RootNamespace var err error if nsID != "" { @@ -2444,7 +2446,7 @@ func (m *ExpirationManager) getLeaseMountAccessor(ctx context.Context, leaseID s return mountAccessor } -// TODO if keep counts as a map, should update the RFC +// TODO SW if keep counts as a map, should update the RFC func (m *ExpirationManager) getIrrevocableLeaseCounts(ctx context.Context, includeChildNamespaces bool) (map[string]interface{}, error) { requestNS, err := namespace.FromContext(ctx) if err != nil { @@ -2458,7 +2460,7 @@ func (m *ExpirationManager) getIrrevocableLeaseCounts(ctx context.Context, inclu leaseID := k.(string) leaseNS, err := m.getNamespaceFromLeaseID(ctx, leaseID) if err != nil { - // We probably want to track that an error occured, but continue counting + // We should probably note that an error occured, but continue counting m.logger.Warn("could not get lease namespace from ID", "error", err) return true } diff --git a/vault/external_tests/expiration/expiration_test.go b/vault/external_tests/expiration/expiration_test.go index 9d1c9a631c1f6..19667d3d0723b 100644 --- a/vault/external_tests/expiration/expiration_test.go +++ b/vault/external_tests/expiration/expiration_test.go @@ -80,6 +80,10 @@ func TestExpiration_irrevocableLeaseCountsAPI(t *testing.T) { } countPerMount = countPerMountRaw.(map[string]interface{}) + if len(countPerMount) != len(expectedCountPerMount) { + t.Fatalf("expected %d mounts, got %d: %#v", len(expectedCountPerMount), len(countPerMount), countPerMount) + } + for mount, expectedCount := range expectedCountPerMount { gotCountRaw, ok := countPerMount[mount] if !ok { @@ -111,7 +115,6 @@ func TestExpiration_irrevocableLeaseListAPI(t *testing.T) { params := make(map[string][]string) params["type"] = []string{"irrevocable"} - // TODO update RFC to say that it's a get operation resp, err := client.Logical().ReadWithData("sys/leases", params) if err != nil { t.Fatal(err) @@ -171,6 +174,10 @@ func TestExpiration_irrevocableLeaseListAPI(t *testing.T) { } leasesPerMount = leasesPerMountRaw.(map[string]interface{}) + if len(leasesPerMount) != len(expectedCountsPerMount) { + t.Fatalf("expected %d mounts, got %d: %#v", len(expectedCountsPerMount), len(leasesPerMount), leasesPerMount) + } + for mount, expectedCount := range expectedCountsPerMount { leaseCount := len(leasesPerMount[mount].([]interface{})) if leaseCount != expectedCount { @@ -233,6 +240,10 @@ func TestExpiration_irrevocableLeaseListAPI_force(t *testing.T) { } leasesPerMount := leasesPerMountRaw.(map[string]interface{}) + if len(leasesPerMount) != len(expectedCountsPerMount) { + t.Fatalf("expected %d mounts, got %d: %#v", len(expectedCountsPerMount), len(leasesPerMount), leasesPerMount) + } + for mount, expectedCount := range expectedCountsPerMount { leaseCount := len(leasesPerMount[mount].([]interface{})) if leaseCount != expectedCount { diff --git a/vault/logical_system_paths.go b/vault/logical_system_paths.go index 9045ec7ca9d3f..9374acff78369 100644 --- a/vault/logical_system_paths.go +++ b/vault/logical_system_paths.go @@ -1216,10 +1216,12 @@ func (b *SystemBackend) leasePaths() []*framework.Path { Fields: map[string]*framework.FieldSchema{ "type": { Type: framework.TypeString, + Required: true, Description: "Type of leases to get counts for (currently only supporting irrevocable).", }, "include_child_namespaces": { Type: framework.TypeBool, + Default: false, Description: "Set true if you want counts for this namespace and its children.", }, }, @@ -1238,15 +1240,18 @@ func (b *SystemBackend) leasePaths() []*framework.Path { Fields: map[string]*framework.FieldSchema{ "type": { Type: framework.TypeString, + Required: true, Description: "Type of leases to get counts for (currently only supporting irrevocable).", }, "include_child_namespaces": { Type: framework.TypeBool, + Default: false, Description: "Set true if you want counts for this namespace and its children.", }, "force": { Type: framework.TypeBool, - Description: "Set true if to get lists containing more than 10,000 entries.", + Default: false, + Description: "Set true to get lists containing more than 10,000 entries.", }, }, diff --git a/website/content/api-docs/system/leases.mdx b/website/content/api-docs/system/leases.mdx index 7431e4f7a9b44..9efc3ee085ccc 100644 --- a/website/content/api-docs/system/leases.mdx +++ b/website/content/api-docs/system/leases.mdx @@ -262,7 +262,7 @@ This endpoint was added in Vault 1.8. | Method | Path | | :----- | :----------------- | -| `GET` | `/sys/leases/count` | +| `GET` | `/sys/leases/count`| ### Sample Request @@ -297,14 +297,14 @@ This endpoint was added in Vault 1.8. | Method | Path | | :----- | :------------ | -| `LIST` | `/sys/leases` | +| `GET` | `/sys/leases` | ### Sample Request ```shell-session $ curl \ --header "X-Vault-Token: ..." \ - --request LIST \ + --request GET \ http://127.0.0.1:8200/v1/sys/leases \ -d type=irrevocable ``` From b81ea9b00d94482201c68a18507087a5d8d82253 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Fri, 21 May 2021 16:11:46 -0600 Subject: [PATCH 13/24] better test error msg --- vault/expiration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 503091a2e8786..432b405884f0c 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -3029,7 +3029,7 @@ func mountNoopBackends(t *testing.T, c *Core, paths []string) map[string]string mount := c.router.MatchingMountEntry(namespace.RootContext(nil), path) if mount == nil { - t.Fatalf("couldn't map path to mount") + t.Fatalf("couldn't find mount for path %s", path) } pathToMount[path] = mount.Accessor } From 6114caa14882e6b450afad7c5dcbd037cde9b671 Mon Sep 17 00:00:00 2001 From: swayne275 Date: Tue, 25 May 2021 07:42:19 -0600 Subject: [PATCH 14/24] Update vault/logical_system_paths.go Co-authored-by: Brian Kassouf --- vault/logical_system_paths.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/logical_system_paths.go b/vault/logical_system_paths.go index 9374acff78369..8f3f4c03f8686 100644 --- a/vault/logical_system_paths.go +++ b/vault/logical_system_paths.go @@ -1241,7 +1241,7 @@ func (b *SystemBackend) leasePaths() []*framework.Path { "type": { Type: framework.TypeString, Required: true, - Description: "Type of leases to get counts for (currently only supporting irrevocable).", + Description: "Type of leases to retrieve (currently only supporting irrevocable).", }, "include_child_namespaces": { Type: framework.TypeBool, From 0d61f9cf35875d60dccc75ff9294513cff054956 Mon Sep 17 00:00:00 2001 From: swayne275 Date: Tue, 25 May 2021 11:32:07 -0600 Subject: [PATCH 15/24] Update vault/logical_system_paths.go Co-authored-by: Brian Kassouf --- vault/logical_system_paths.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/logical_system_paths.go b/vault/logical_system_paths.go index 8f3f4c03f8686..4329d009bfb5d 100644 --- a/vault/logical_system_paths.go +++ b/vault/logical_system_paths.go @@ -1246,7 +1246,7 @@ func (b *SystemBackend) leasePaths() []*framework.Path { "include_child_namespaces": { Type: framework.TypeBool, Default: false, - Description: "Set true if you want counts for this namespace and its children.", + Description: "Set true if you want leases for this namespace and its children.", }, "force": { Type: framework.TypeBool, From cb47c4a691fe5c0a57bd0e82dd73ab1b54d552d6 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Tue, 25 May 2021 13:27:28 -0600 Subject: [PATCH 16/24] return warning with data if more than default leases to list without force flag --- vault/expiration.go | 19 ++++------ vault/expiration_test.go | 33 +++++++++++++---- .../expiration/expiration_test.go | 37 +++++++++++++++++-- vault/logical_system.go | 13 +++++-- 4 files changed, 75 insertions(+), 27 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index aae1e14bb28ea..60cd2adc9a479 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -77,10 +77,8 @@ const ( // maximum number of irrevocable leases we return to the irrevocable lease // list API **without** the `force` flag set MaxIrrevocableLeasesToReturn = 10000 -) -var ( - errHitMaxIrrevocableLeases = errors.New("Command cancelled because many irrevocable leases were found. To emit the entire list, re-run the command with force set true.") + MaxIrrevocableLeasesWarning = "Command halted because many irrevocable leases were found. To emit the entire list, re-run the command with force set true." ) type pendingInfo struct { @@ -2495,17 +2493,18 @@ type leaseResponse struct { ErrMsg string `json:"error"` } -func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeChildNamespaces, force bool) (map[string]interface{}, error) { +// returns a warning string, if applicable +func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeChildNamespaces, force bool) (map[string]interface{}, string, error) { requestNS, err := namespace.FromContext(ctx) if err != nil { m.logger.Error("could not get namespace from context", "error", err) - return nil, err + return nil, "", err } // map of mount point : lease info matchingLeasesPerMount := make(map[string][]*leaseResponse) numMatchingLeases := 0 - var retErr error + var warning string m.irrevocable.Range(func(k, v interface{}) bool { leaseID := k.(string) leaseInfo := v.(*leaseEntry) @@ -2525,7 +2524,7 @@ func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeCh if !force && (numMatchingLeases >= MaxIrrevocableLeasesToReturn) { m.logger.Warn("hit max irrevocable leases without force flag set") - retErr = errHitMaxIrrevocableLeases + warning = MaxIrrevocableLeasesWarning return false } @@ -2540,15 +2539,11 @@ func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeCh return true }) - if retErr != nil { - return nil, retErr - } - resp := make(map[string]interface{}) resp["lease_count"] = numMatchingLeases resp["leases"] = matchingLeasesPerMount - return resp, nil + return resp, warning, nil } // leaseEntry is used to structure the values the expiration diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 432b405884f0c..ad72e8b095fbd 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -3104,10 +3104,13 @@ func TestExpiration_listIrrevocableLeases(t *testing.T) { } } - out, err := exp.listIrrevocableLeases(namespace.RootContext(nil), false, false) + out, warn, err := exp.listIrrevocableLeases(namespace.RootContext(nil), false, false) if err != nil { t.Fatalf("error listing irrevocable leases: %v", err) } + if warn != "" { + t.Errorf("expected no warning, got %q", warn) + } countRaw, ok := out["lease_count"] if !ok { @@ -3161,22 +3164,38 @@ func TestExpiration_listIrrevocableLeases_force(t *testing.T) { addIrrevocableLease(t, exp, "foo/", namespace.RootNamespace) } - dataRaw, err := exp.listIrrevocableLeases(namespace.RootContext(nil), false, false) - if err == nil { - t.Fatalf("expected error - more than max number of irrevocable leases") + dataRaw, warn, err := exp.listIrrevocableLeases(namespace.RootContext(nil), false, false) + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + if warn != maxIrrevocableLeasesWarning { + t.Errorf("expected warning %q, got %q", maxIrrevocableLeasesWarning, warn) + } + if dataRaw == nil { + t.Fatal("expected partial data, got nil") } - if dataRaw != nil { - t.Fatalf("expected nil data, got %#v", dataRaw) + + leaseListLength := len(dataRaw["leases"].(map[string][]*leaseResponse)["mount-accessor-not-found"]) + if leaseListLength != MaxIrrevocableLeasesToReturn { + t.Fatalf("expected %d results, got %d", MaxIrrevocableLeasesToReturn, leaseListLength) } - dataRaw, err = exp.listIrrevocableLeases(namespace.RootContext(nil), false, true) + dataRaw, warn, err = exp.listIrrevocableLeases(namespace.RootContext(nil), false, true) if err != nil { t.Fatalf("got error on force list leases: %v", err) } + if warn != "" { + t.Errorf("expected no warning, got %q", warn) + } if dataRaw == nil { t.Fatalf("got nil data on force list leases") } + leaseListLength = len(dataRaw["leases"].(map[string][]*leaseResponse)["mount-accessor-not-found"]) + if leaseListLength != expectedNumLeases { + t.Fatalf("expected %d results, got %d", MaxIrrevocableLeasesToReturn, expectedNumLeases) + } + numLeasesRaw, ok := dataRaw["lease_count"] if !ok { t.Fatalf("lease count data not present") diff --git a/vault/external_tests/expiration/expiration_test.go b/vault/external_tests/expiration/expiration_test.go index 19667d3d0723b..4b3c598232fb5 100644 --- a/vault/external_tests/expiration/expiration_test.go +++ b/vault/external_tests/expiration/expiration_test.go @@ -29,6 +29,11 @@ func TestExpiration_irrevocableLeaseCountsAPI(t *testing.T) { if resp == nil { t.Fatal("response is nil") } + + if len(resp.Warnings) > 0 { + t.Errorf("expected no warnings, got: %v", resp.Warnings) + } + totalLeaseCountRaw, ok := resp.Data["lease_count"] if !ok { t.Fatalf("expected 'lease_count' response, got: %#v", resp.Data) @@ -61,6 +66,11 @@ func TestExpiration_irrevocableLeaseCountsAPI(t *testing.T) { if resp == nil { t.Fatal("response is nil") } + + if len(resp.Warnings) > 0 { + t.Errorf("expected no warnings, got: %v", resp.Warnings) + } + totalLeaseCountRaw, ok = resp.Data["lease_count"] if !ok { t.Fatalf("expected 'lease_count' response, got: %#v", resp.Data) @@ -122,6 +132,11 @@ func TestExpiration_irrevocableLeaseListAPI(t *testing.T) { if resp == nil { t.Fatal("response is nil") } + + if len(resp.Warnings) > 0 { + t.Errorf("expected no warnings, got: %v", resp.Warnings) + } + totalLeaseCountRaw, ok := resp.Data["lease_count"] if !ok { t.Fatalf("expected 'lease_count' response, got: %#v", resp.Data) @@ -155,6 +170,11 @@ func TestExpiration_irrevocableLeaseListAPI(t *testing.T) { if resp == nil { t.Fatal("response is nil") } + + if len(resp.Warnings) > 0 { + t.Errorf("expected no warnings, got: %v", resp.Warnings) + } + totalLeaseCountRaw, ok = resp.Data["lease_count"] if !ok { t.Fatalf("expected 'lease_count' response, got: %#v", resp.Data) @@ -205,11 +225,15 @@ func TestExpiration_irrevocableLeaseListAPI_force(t *testing.T) { params["type"] = []string{"irrevocable"} resp, err := client.Logical().ReadWithData("sys/leases", params) - if err == nil { - t.Fatalf("expected error without force flag") + if err != nil { + t.Fatalf("unexpected error: %v", err) } - if resp != nil { - t.Errorf("expected nil response, got: %#v", resp) + if resp == nil { + t.Error("unexpected nil response") + } + + if len(resp.Warnings) != 1 { + t.Errorf("expected one warning (%q), got: %v", vault.MaxIrrevocableLeasesWarning, resp.Warnings) } // now try it with the force flag - we expect no errors and many results @@ -221,6 +245,11 @@ func TestExpiration_irrevocableLeaseListAPI_force(t *testing.T) { if resp == nil { t.Fatal("response is nil") } + + if len(resp.Warnings) > 0 { + t.Errorf("expected no warnings, got: %v", resp.Warnings) + } + totalLeaseCountRaw, ok := resp.Data["lease_count"] if !ok { t.Fatalf("expected 'lease_count' response, got: %#v", resp.Data) diff --git a/vault/logical_system.go b/vault/logical_system.go index 40c45381676c8..ea0999eac214f 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -316,14 +316,19 @@ func (b *SystemBackend) handleLeaseList(ctx context.Context, req *logical.Reques forceRaw, ok := d.GetOk("force") force := ok && forceRaw.(bool) - resp, err := b.Core.expiration.listIrrevocableLeases(ctx, includeChildNamespaces, force) + leases, warning, err := b.Core.expiration.listIrrevocableLeases(ctx, includeChildNamespaces, force) if err != nil { return nil, err } - return &logical.Response{ - Data: resp, - }, nil + resp := &logical.Response{ + Data: leases, + } + if warning != "" { + resp.AddWarning(warning) + } + + return resp, nil } func (b *SystemBackend) handlePluginCatalogTypedList(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { From e5dafb4cd8c2f3cf76fd25f77ffd491f907968ef Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Tue, 25 May 2021 18:09:08 -0600 Subject: [PATCH 17/24] make api doc more generalized --- website/content/api-docs/system/leases.mdx | 26 +++++++++++----------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/website/content/api-docs/system/leases.mdx b/website/content/api-docs/system/leases.mdx index 9efc3ee085ccc..0b8edd64aefcf 100644 --- a/website/content/api-docs/system/leases.mdx +++ b/website/content/api-docs/system/leases.mdx @@ -244,10 +244,10 @@ $ curl \ http://127.0.0.1:8200/v1/sys/leases/tidy ``` -## Irrevocable Leases Counts +## Lease Counts -This endpoint returns the total count of irrevocable leases, as well as a count -per mount point. +This endpoint returns the total count of a `type` of lease, as well as a count +per mount point. Note that it currently only supports type "irrevocable". This can help determine if particular endpoints are disproportionately resulting in irrevocable leases. @@ -256,9 +256,9 @@ This endpoint was added in Vault 1.8. ### Parameters -- `type` (string: ) - Specifies the type of lease. Set to "irrevocable" -- `include_child_namespaces` (bool: false) - Specifies if irrevocable leases in - child namespaces should be included in the result +- `type` (string: ) - Specifies the type of lease. +- `include_child_namespaces` (bool: false) - Specifies if leases in child + namespaces should be included in the result. | Method | Path | | :----- | :----------------- | @@ -274,11 +274,11 @@ $ curl \ -d type=irrevocable ``` -## Irrevocable Leases List +## Leases List -This endpoint returns the total count of irrevocable leases, as well as a list -of irrevocable leases per mount point - complete with the error that marked -the lease as irrevocable. +This endpoint returns the total count of a `type` of lease, as well as a list +of leases per mount point. Note that it currently only supports type +"irrevocable". This can help determine if particular endpoints or causes are disproportionately resulting in irrevocable leases. @@ -287,9 +287,9 @@ This endpoint was added in Vault 1.8. ### Parameters -- `type` (string: ) - Specifies the type of lease. Set to "irrevocable" -- `include_child_namespaces` (bool: false) - Specifies if irrevocable leases in - child namespaces should be included in the result +- `type` (string: ) - Specifies the type of lease. +- `include_child_namespaces` (bool: false) - Specifies if leases in child + namespaces should be included in the result - `force` (bool: false) - Specifies if the request should complete if there are more than 10,000 results to return. If this is set false, the client will send an error specifying that there are many results, and the request must From 6f2abe7ae805a32b6eda25d12390a626b05c6c85 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Wed, 26 May 2021 12:07:54 -0600 Subject: [PATCH 18/24] list leases in general, not by mount point --- vault/expiration.go | 8 ++- vault/expiration_test.go | 54 ++++++++-------- .../expiration/expiration_test.go | 61 +++++++++++-------- 3 files changed, 69 insertions(+), 54 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index 60cd2adc9a479..a636878c8339e 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -2490,6 +2490,7 @@ func (m *ExpirationManager) getIrrevocableLeaseCounts(ctx context.Context, inclu type leaseResponse struct { LeaseID string `json:"lease_id"` + MountID string `json:"mount_id"` ErrMsg string `json:"error"` } @@ -2502,7 +2503,7 @@ func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeCh } // map of mount point : lease info - matchingLeasesPerMount := make(map[string][]*leaseResponse) + matchingLeases := make([]*leaseResponse, 0) numMatchingLeases := 0 var warning string m.irrevocable.Range(func(k, v interface{}) bool { @@ -2531,8 +2532,9 @@ func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeCh mountAccessor := m.getLeaseMountAccessor(ctx, leaseID) numMatchingLeases++ - matchingLeasesPerMount[mountAccessor] = append(matchingLeasesPerMount[mountAccessor], &leaseResponse{ + matchingLeases = append(matchingLeases, &leaseResponse{ LeaseID: leaseID, + MountID: mountAccessor, ErrMsg: leaseInfo.RevokeErr, }) @@ -2541,7 +2543,7 @@ func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeCh resp := make(map[string]interface{}) resp["lease_count"] = numMatchingLeases - resp["leases"] = matchingLeasesPerMount + resp["leases"] = matchingLeases return resp, warning, nil } diff --git a/vault/expiration_test.go b/vault/expiration_test.go index ad72e8b095fbd..046c75bbb29c0 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -3095,12 +3095,19 @@ func TestExpiration_listIrrevocableLeases(t *testing.T) { exp := c.expiration - expectedLeasesPerMount := make(map[string][]string) + type basicLeaseInfo struct { + id string + mount string + } + expectedLeases := make([]*basicLeaseInfo, 0) expectedPerMount := 10 for i := 0; i < expectedPerMount; i++ { for _, mountPrefix := range mountPrefixes { leaseID := addIrrevocableLease(t, exp, mountPrefix, namespace.RootNamespace) - expectedLeasesPerMount[mountPrefix] = append(expectedLeasesPerMount[mountPrefix], leaseID) + expectedLeases = append(expectedLeases, &basicLeaseInfo{ + id: leaseID, + mount: pathToMount[mountPrefix], + }) } } @@ -3123,34 +3130,29 @@ func TestExpiration_listIrrevocableLeases(t *testing.T) { } count := countRaw.(int) - leases := leasesRaw.(map[string][]*leaseResponse) + leases := leasesRaw.([]*leaseResponse) expectedCount := len(mountPrefixes) * expectedPerMount if count != expectedCount { t.Errorf("bad count. expected %d, got %d", expectedCount, count) } + if len(leases) != len(expectedLeases) { + t.Errorf("bad lease results. expected %d, got %d with values %v", len(expectedLeases), len(leases), leases) + } - for _, mountPrefix := range mountPrefixes { - mount := pathToMount[mountPrefix] - - // sort both sets of data for easy comparison - sort.Strings(expectedLeasesPerMount[mountPrefix]) - sort.Slice(leases[mount], func(i, j int) bool { - return leases[mount][i].LeaseID < leases[mount][j].LeaseID - }) + sort.Slice(expectedLeases, func(i, j int) bool { + return expectedLeases[i].id < expectedLeases[j].id + }) + sort.Slice(leases, func(i, j int) bool { + return leases[i].LeaseID < leases[j].LeaseID + }) - if len(leases[mount]) != expectedPerMount { - t.Fatalf("bad leases for mount prefix %q: expected %d, got %d", mountPrefix, expectedPerMount, len(leases[mount])) + for i, lease := range expectedLeases { + if lease.id != leases[i].LeaseID { + t.Errorf("bad lease id. expected %q, got %q", lease.id, leases[i].LeaseID) } - - for i, v := range expectedLeasesPerMount[mountPrefix] { - lease := leases[mount][i] - if lease.LeaseID != v { - t.Errorf("bad leaseID for mount prefix %q: expected %s, got %s", mountPrefix, v, lease.LeaseID) - } - if lease.ErrMsg == "" { - t.Errorf("no error message for irrevocable leaseID %q", lease.LeaseID) - } + if lease.mount != leases[i].MountID { + t.Errorf("bad mount id. expected %q, got %q", lease.mount, leases[i].MountID) } } } @@ -3168,14 +3170,14 @@ func TestExpiration_listIrrevocableLeases_force(t *testing.T) { if err != nil { t.Fatalf("expected no error, got: %v", err) } - if warn != maxIrrevocableLeasesWarning { - t.Errorf("expected warning %q, got %q", maxIrrevocableLeasesWarning, warn) + if warn != MaxIrrevocableLeasesWarning { + t.Errorf("expected warning %q, got %q", MaxIrrevocableLeasesWarning, warn) } if dataRaw == nil { t.Fatal("expected partial data, got nil") } - leaseListLength := len(dataRaw["leases"].(map[string][]*leaseResponse)["mount-accessor-not-found"]) + leaseListLength := len(dataRaw["leases"].([]*leaseResponse)) if leaseListLength != MaxIrrevocableLeasesToReturn { t.Fatalf("expected %d results, got %d", MaxIrrevocableLeasesToReturn, leaseListLength) } @@ -3191,7 +3193,7 @@ func TestExpiration_listIrrevocableLeases_force(t *testing.T) { t.Fatalf("got nil data on force list leases") } - leaseListLength = len(dataRaw["leases"].(map[string][]*leaseResponse)["mount-accessor-not-found"]) + leaseListLength = len(dataRaw["leases"].([]*leaseResponse)) if leaseListLength != expectedNumLeases { t.Fatalf("expected %d results, got %d", MaxIrrevocableLeasesToReturn, expectedNumLeases) } diff --git a/vault/external_tests/expiration/expiration_test.go b/vault/external_tests/expiration/expiration_test.go index 4b3c598232fb5..e65976097eded 100644 --- a/vault/external_tests/expiration/expiration_test.go +++ b/vault/external_tests/expiration/expiration_test.go @@ -2,6 +2,7 @@ package expiration import ( "encoding/json" + "reflect" "testing" "github.com/hashicorp/vault/helper/namespace" @@ -150,18 +151,18 @@ func TestExpiration_irrevocableLeaseListAPI(t *testing.T) { t.Errorf("expected no leases, got %d", totalLeaseCount) } - leasesPerMountRaw, ok := resp.Data["leases"] + leasesRaw, ok := resp.Data["leases"] if !ok { t.Fatalf("expected 'leases' response, got %#v", resp.Data) } - leasesPerMount := leasesPerMountRaw.(map[string]interface{}) - if len(leasesPerMount) != 0 { - t.Errorf("expected no mounts with leases, got %#v", leasesPerMount) + leases := leasesRaw.([]interface{}) + if len(leases) != 0 { + t.Errorf("expected no mounts with leases, got %#v", leases) } // test with a low enough number to not give an error without force flag expectedNumLeases := 50 - expectedCountsPerMount := core.InjectIrrevocableLeases(t, namespace.RootContext(nil), expectedNumLeases) + expectedCountPerMount := core.InjectIrrevocableLeases(t, namespace.RootContext(nil), expectedNumLeases) resp, err = client.Logical().ReadWithData("sys/leases", params) if err != nil { @@ -188,21 +189,26 @@ func TestExpiration_irrevocableLeaseListAPI(t *testing.T) { t.Errorf("expected %d leases, got %d", expectedNumLeases, totalLeaseCount) } - leasesPerMountRaw, ok = resp.Data["leases"] + leasesRaw, ok = resp.Data["leases"] if !ok { t.Fatalf("expected 'leases' response, got %#v", resp.Data) } - leasesPerMount = leasesPerMountRaw.(map[string]interface{}) - if len(leasesPerMount) != len(expectedCountsPerMount) { - t.Fatalf("expected %d mounts, got %d: %#v", len(expectedCountsPerMount), len(leasesPerMount), leasesPerMount) - } + leases = leasesRaw.([]interface{}) + countPerMount := make(map[string]int) + for _, leaseRaw := range leases { + lease := leaseRaw.(map[string]interface{}) + mount := lease["mount_id"].(string) - for mount, expectedCount := range expectedCountsPerMount { - leaseCount := len(leasesPerMount[mount].([]interface{})) - if leaseCount != expectedCount { - t.Errorf("bad count for mount %q, expected %d, got %d", mount, expectedCount, leaseCount) + if _, ok := countPerMount[mount]; !ok { + countPerMount[mount] = 0 } + + countPerMount[mount]++ + } + + if !reflect.DeepEqual(countPerMount, expectedCountPerMount) { + t.Errorf("bad mount count. expected %v, got %v", expectedCountPerMount, countPerMount) } } @@ -219,7 +225,7 @@ func TestExpiration_irrevocableLeaseListAPI_force(t *testing.T) { // test with a low enough number to not give an error without force flag expectedNumLeases := vault.MaxIrrevocableLeasesToReturn + 50 - expectedCountsPerMount := core.InjectIrrevocableLeases(t, namespace.RootContext(nil), expectedNumLeases) + expectedCountPerMount := core.InjectIrrevocableLeases(t, namespace.RootContext(nil), expectedNumLeases) params := make(map[string][]string) params["type"] = []string{"irrevocable"} @@ -229,7 +235,7 @@ func TestExpiration_irrevocableLeaseListAPI_force(t *testing.T) { t.Fatalf("unexpected error: %v", err) } if resp == nil { - t.Error("unexpected nil response") + t.Fatal("unexpected nil response") } if len(resp.Warnings) != 1 { @@ -263,20 +269,25 @@ func TestExpiration_irrevocableLeaseListAPI_force(t *testing.T) { t.Errorf("expected %d leases, got %d", expectedNumLeases, totalLeaseCount) } - leasesPerMountRaw, ok := resp.Data["leases"] + leasesRaw, ok := resp.Data["leases"] if !ok { t.Fatalf("expected 'leases' response, got %#v", resp.Data) } - leasesPerMount := leasesPerMountRaw.(map[string]interface{}) - if len(leasesPerMount) != len(expectedCountsPerMount) { - t.Fatalf("expected %d mounts, got %d: %#v", len(expectedCountsPerMount), len(leasesPerMount), leasesPerMount) - } + leases := leasesRaw.([]interface{}) + countPerMount := make(map[string]int) + for _, leaseRaw := range leases { + lease := leaseRaw.(map[string]interface{}) + mount := lease["mount_id"].(string) - for mount, expectedCount := range expectedCountsPerMount { - leaseCount := len(leasesPerMount[mount].([]interface{})) - if leaseCount != expectedCount { - t.Errorf("bad count for mount %q, expected %d, got %d", mount, expectedCount, leaseCount) + if _, ok := countPerMount[mount]; !ok { + countPerMount[mount] = 0 } + + countPerMount[mount]++ + } + + if !reflect.DeepEqual(countPerMount, expectedCountPerMount) { + t.Errorf("bad mount count. expected %v, got %v", expectedCountPerMount, countPerMount) } } From da988f93d817f19d1cfe662aadc49ae10368e8a7 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Wed, 26 May 2021 15:15:55 -0600 Subject: [PATCH 19/24] change force flag to include_large_results --- vault/expiration.go | 4 ++-- vault/expiration_test.go | 6 +++--- vault/external_tests/expiration/expiration_test.go | 12 ++++++------ vault/logical_system.go | 6 +++--- vault/logical_system_paths.go | 2 +- website/content/api-docs/system/leases.mdx | 8 ++++---- 6 files changed, 19 insertions(+), 19 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index a636878c8339e..db46ce64864b2 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -2495,7 +2495,7 @@ type leaseResponse struct { } // returns a warning string, if applicable -func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeChildNamespaces, force bool) (map[string]interface{}, string, error) { +func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeChildNamespaces, includeLargeResults bool) (map[string]interface{}, string, error) { requestNS, err := namespace.FromContext(ctx) if err != nil { m.logger.Error("could not get namespace from context", "error", err) @@ -2523,7 +2523,7 @@ func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeCh return true } - if !force && (numMatchingLeases >= MaxIrrevocableLeasesToReturn) { + if !includeLargeResults && (numMatchingLeases >= MaxIrrevocableLeasesToReturn) { m.logger.Warn("hit max irrevocable leases without force flag set") warning = MaxIrrevocableLeasesWarning return false diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 046c75bbb29c0..4483dc9abd3d6 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -3157,7 +3157,7 @@ func TestExpiration_listIrrevocableLeases(t *testing.T) { } } -func TestExpiration_listIrrevocableLeases_force(t *testing.T) { +func TestExpiration_listIrrevocableLeases_includeLargeResults(t *testing.T) { c, _, _ := TestCoreUnsealed(t) exp := c.expiration @@ -3184,13 +3184,13 @@ func TestExpiration_listIrrevocableLeases_force(t *testing.T) { dataRaw, warn, err = exp.listIrrevocableLeases(namespace.RootContext(nil), false, true) if err != nil { - t.Fatalf("got error on force list leases: %v", err) + t.Fatalf("got error on include_large_results: %v", err) } if warn != "" { t.Errorf("expected no warning, got %q", warn) } if dataRaw == nil { - t.Fatalf("got nil data on force list leases") + t.Fatalf("got nil data on include_large_results") } leaseListLength = len(dataRaw["leases"].([]*leaseResponse)) diff --git a/vault/external_tests/expiration/expiration_test.go b/vault/external_tests/expiration/expiration_test.go index e65976097eded..b2e0268e15123 100644 --- a/vault/external_tests/expiration/expiration_test.go +++ b/vault/external_tests/expiration/expiration_test.go @@ -160,7 +160,7 @@ func TestExpiration_irrevocableLeaseListAPI(t *testing.T) { t.Errorf("expected no mounts with leases, got %#v", leases) } - // test with a low enough number to not give an error without force flag + // test with a low enough number to not give an error without include_large_results flag expectedNumLeases := 50 expectedCountPerMount := core.InjectIrrevocableLeases(t, namespace.RootContext(nil), expectedNumLeases) @@ -212,7 +212,7 @@ func TestExpiration_irrevocableLeaseListAPI(t *testing.T) { } } -func TestExpiration_irrevocableLeaseListAPI_force(t *testing.T) { +func TestExpiration_irrevocableLeaseListAPI_includeLargeResults(t *testing.T) { cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ HandlerFunc: vaulthttp.Handler, NumCores: 1, @@ -223,7 +223,7 @@ func TestExpiration_irrevocableLeaseListAPI_force(t *testing.T) { client := cluster.Cores[0].Client core := cluster.Cores[0].Core - // test with a low enough number to not give an error without force flag + // test with a low enough number to not give an error without include_large_results flag expectedNumLeases := vault.MaxIrrevocableLeasesToReturn + 50 expectedCountPerMount := core.InjectIrrevocableLeases(t, namespace.RootContext(nil), expectedNumLeases) @@ -242,11 +242,11 @@ func TestExpiration_irrevocableLeaseListAPI_force(t *testing.T) { t.Errorf("expected one warning (%q), got: %v", vault.MaxIrrevocableLeasesWarning, resp.Warnings) } - // now try it with the force flag - we expect no errors and many results - params["force"] = []string{"true"} + // now try it with the include_large_results flag - we expect no errors and many results + params["include_large_results"] = []string{"true"} resp, err = client.Logical().ReadWithData("sys/leases", params) if err != nil { - t.Fatalf("unexpected error when using force flag: %v", err) + t.Fatalf("unexpected error when using include_large_results flag: %v", err) } if resp == nil { t.Fatal("response is nil") diff --git a/vault/logical_system.go b/vault/logical_system.go index ea0999eac214f..a13a61fe9fe99 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -313,10 +313,10 @@ func (b *SystemBackend) handleLeaseList(ctx context.Context, req *logical.Reques includeChildNamespacesRaw, ok := d.GetOk("include_child_namespaces") includeChildNamespaces := ok && includeChildNamespacesRaw.(bool) - forceRaw, ok := d.GetOk("force") - force := ok && forceRaw.(bool) + includeLargeResultsRaw, ok := d.GetOk("include_large_results") + includeLargeResults := ok && includeLargeResultsRaw.(bool) - leases, warning, err := b.Core.expiration.listIrrevocableLeases(ctx, includeChildNamespaces, force) + leases, warning, err := b.Core.expiration.listIrrevocableLeases(ctx, includeChildNamespaces, includeLargeResults) if err != nil { return nil, err } diff --git a/vault/logical_system_paths.go b/vault/logical_system_paths.go index 4329d009bfb5d..4dc8f1cba8aba 100644 --- a/vault/logical_system_paths.go +++ b/vault/logical_system_paths.go @@ -1248,7 +1248,7 @@ func (b *SystemBackend) leasePaths() []*framework.Path { Default: false, Description: "Set true if you want leases for this namespace and its children.", }, - "force": { + "include_large_results": { Type: framework.TypeBool, Default: false, Description: "Set true to get lists containing more than 10,000 entries.", diff --git a/website/content/api-docs/system/leases.mdx b/website/content/api-docs/system/leases.mdx index 0b8edd64aefcf..efb22e28dde2c 100644 --- a/website/content/api-docs/system/leases.mdx +++ b/website/content/api-docs/system/leases.mdx @@ -290,10 +290,10 @@ This endpoint was added in Vault 1.8. - `type` (string: ) - Specifies the type of lease. - `include_child_namespaces` (bool: false) - Specifies if leases in child namespaces should be included in the result -- `force` (bool: false) - Specifies if the request should complete if there are - more than 10,000 results to return. If this is set false, the client will - send an error specifying that there are many results, and the request must - be retried using the `force` flag. +- `include_large_results` (bool: false) - Specifies if the request should + complete if there are more than 10,000 results to return. If this is set + false, the client will send an error specifying that there are many results, + and the request must be retried using the `force` flag. | Method | Path | | :----- | :------------ | From d06e92305c933c78163a6f6f41bf5347488849cb Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Wed, 26 May 2021 15:48:14 -0600 Subject: [PATCH 20/24] sort leases by LeaseID for consistent API response --- vault/expiration.go | 6 ++++++ vault/expiration_test.go | 4 +--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index db46ce64864b2..d67049781f1eb 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -8,6 +8,7 @@ import ( "math/rand" "os" "path" + "sort" "strconv" "strings" "sync" @@ -2541,6 +2542,11 @@ func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeCh return true }) + // sort the results for consistent API response + sort.Slice(matchingLeases, func(i, j int) bool { + return matchingLeases[i].LeaseID < matchingLeases[j].LeaseID + }) + resp := make(map[string]interface{}) resp["lease_count"] = numMatchingLeases resp["leases"] = matchingLeases diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 4483dc9abd3d6..82177b3e42cb3 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -3140,12 +3140,10 @@ func TestExpiration_listIrrevocableLeases(t *testing.T) { t.Errorf("bad lease results. expected %d, got %d with values %v", len(expectedLeases), len(leases), leases) } + // `leases` is already sorted by lease ID sort.Slice(expectedLeases, func(i, j int) bool { return expectedLeases[i].id < expectedLeases[j].id }) - sort.Slice(leases, func(i, j int) bool { - return leases[i].LeaseID < leases[j].LeaseID - }) for i, lease := range expectedLeases { if lease.id != leases[i].LeaseID { From 33c1d4c4f49414ccfb6633136a2404e22b8b6d94 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Thu, 27 May 2021 16:57:12 -0600 Subject: [PATCH 21/24] switch from bool flag for API limit to string value --- vault/expiration.go | 6 +- vault/expiration_test.go | 12 +-- .../expiration/expiration_test.go | 12 +-- vault/logical_system.go | 38 +++++++- vault/logical_system_paths.go | 8 +- vault/logical_system_test.go | 86 +++++++++++++++++++ website/content/api-docs/system/leases.mdx | 8 +- 7 files changed, 145 insertions(+), 25 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index d67049781f1eb..6b68eb5251dd4 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -2496,7 +2496,9 @@ type leaseResponse struct { } // returns a warning string, if applicable -func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeChildNamespaces, includeLargeResults bool) (map[string]interface{}, string, error) { +// limit specifies how many results to return, and must be >0 +// includeAll specifies if all results should be returned, regardless of limit +func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeChildNamespaces, returnAll bool, limit int) (map[string]interface{}, string, error) { requestNS, err := namespace.FromContext(ctx) if err != nil { m.logger.Error("could not get namespace from context", "error", err) @@ -2524,7 +2526,7 @@ func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeCh return true } - if !includeLargeResults && (numMatchingLeases >= MaxIrrevocableLeasesToReturn) { + if !returnAll && (numMatchingLeases >= limit) { m.logger.Warn("hit max irrevocable leases without force flag set") warning = MaxIrrevocableLeasesWarning return false diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 82177b3e42cb3..f3089ed6d6aa3 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -3111,7 +3111,7 @@ func TestExpiration_listIrrevocableLeases(t *testing.T) { } } - out, warn, err := exp.listIrrevocableLeases(namespace.RootContext(nil), false, false) + out, warn, err := exp.listIrrevocableLeases(namespace.RootContext(nil), false, false, MaxIrrevocableLeasesToReturn) if err != nil { t.Fatalf("error listing irrevocable leases: %v", err) } @@ -3155,7 +3155,7 @@ func TestExpiration_listIrrevocableLeases(t *testing.T) { } } -func TestExpiration_listIrrevocableLeases_includeLargeResults(t *testing.T) { +func TestExpiration_listIrrevocableLeases_includeAll(t *testing.T) { c, _, _ := TestCoreUnsealed(t) exp := c.expiration @@ -3164,7 +3164,7 @@ func TestExpiration_listIrrevocableLeases_includeLargeResults(t *testing.T) { addIrrevocableLease(t, exp, "foo/", namespace.RootNamespace) } - dataRaw, warn, err := exp.listIrrevocableLeases(namespace.RootContext(nil), false, false) + dataRaw, warn, err := exp.listIrrevocableLeases(namespace.RootContext(nil), false, false, MaxIrrevocableLeasesToReturn) if err != nil { t.Fatalf("expected no error, got: %v", err) } @@ -3180,15 +3180,15 @@ func TestExpiration_listIrrevocableLeases_includeLargeResults(t *testing.T) { t.Fatalf("expected %d results, got %d", MaxIrrevocableLeasesToReturn, leaseListLength) } - dataRaw, warn, err = exp.listIrrevocableLeases(namespace.RootContext(nil), false, true) + dataRaw, warn, err = exp.listIrrevocableLeases(namespace.RootContext(nil), false, true, 0) if err != nil { - t.Fatalf("got error on include_large_results: %v", err) + t.Fatalf("got error when using limit=none: %v", err) } if warn != "" { t.Errorf("expected no warning, got %q", warn) } if dataRaw == nil { - t.Fatalf("got nil data on include_large_results") + t.Fatalf("got nil data when using limit=none") } leaseListLength = len(dataRaw["leases"].([]*leaseResponse)) diff --git a/vault/external_tests/expiration/expiration_test.go b/vault/external_tests/expiration/expiration_test.go index b2e0268e15123..761981a42d438 100644 --- a/vault/external_tests/expiration/expiration_test.go +++ b/vault/external_tests/expiration/expiration_test.go @@ -160,7 +160,7 @@ func TestExpiration_irrevocableLeaseListAPI(t *testing.T) { t.Errorf("expected no mounts with leases, got %#v", leases) } - // test with a low enough number to not give an error without include_large_results flag + // test with a low enough number to not give an error without limit set to none expectedNumLeases := 50 expectedCountPerMount := core.InjectIrrevocableLeases(t, namespace.RootContext(nil), expectedNumLeases) @@ -212,7 +212,7 @@ func TestExpiration_irrevocableLeaseListAPI(t *testing.T) { } } -func TestExpiration_irrevocableLeaseListAPI_includeLargeResults(t *testing.T) { +func TestExpiration_irrevocableLeaseListAPI_includeAll(t *testing.T) { cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ HandlerFunc: vaulthttp.Handler, NumCores: 1, @@ -223,7 +223,7 @@ func TestExpiration_irrevocableLeaseListAPI_includeLargeResults(t *testing.T) { client := cluster.Cores[0].Client core := cluster.Cores[0].Core - // test with a low enough number to not give an error without include_large_results flag + // test with a low enough number to not give an error with the default limit expectedNumLeases := vault.MaxIrrevocableLeasesToReturn + 50 expectedCountPerMount := core.InjectIrrevocableLeases(t, namespace.RootContext(nil), expectedNumLeases) @@ -242,11 +242,11 @@ func TestExpiration_irrevocableLeaseListAPI_includeLargeResults(t *testing.T) { t.Errorf("expected one warning (%q), got: %v", vault.MaxIrrevocableLeasesWarning, resp.Warnings) } - // now try it with the include_large_results flag - we expect no errors and many results - params["include_large_results"] = []string{"true"} + // now try it with the no limit on return size - we expect no errors and many results + params["limit"] = []string{"none"} resp, err = client.Logical().ReadWithData("sys/leases", params) if err != nil { - t.Fatalf("unexpected error when using include_large_results flag: %v", err) + t.Fatalf("unexpected error when using limit=none: %v", err) } if resp == nil { t.Fatal("response is nil") diff --git a/vault/logical_system.go b/vault/logical_system.go index a13a61fe9fe99..25c61df625efb 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -304,6 +304,36 @@ func (b *SystemBackend) handleLeaseCount(ctx context.Context, req *logical.Reque }, nil } +func processLimit(d *framework.FieldData) (bool, int, error) { + limitStr := "" + limitRaw, ok := d.GetOk("limit") + if ok { + limitStr = limitRaw.(string) + } + + includeAll := false + maxResults := MaxIrrevocableLeasesToReturn + if limitStr == "" { + // use the defaults + } else if strings.ToLower(limitStr) == "none" { + includeAll = true + } else { + // not having a valid, positive int here is an error + limit, err := strconv.Atoi(limitStr) + if err != nil { + return false, 0, fmt.Errorf("invalid 'limit' provided: %w", err) + } + + if limit < 1 { + return false, 0, fmt.Errorf("limit must be 'none' or a positive integer") + } + + maxResults = limit + } + + return includeAll, maxResults, nil +} + func (b *SystemBackend) handleLeaseList(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { typeRaw, ok := d.GetOk("type") if !ok || strings.ToLower(typeRaw.(string)) != "irrevocable" { @@ -313,10 +343,12 @@ func (b *SystemBackend) handleLeaseList(ctx context.Context, req *logical.Reques includeChildNamespacesRaw, ok := d.GetOk("include_child_namespaces") includeChildNamespaces := ok && includeChildNamespacesRaw.(bool) - includeLargeResultsRaw, ok := d.GetOk("include_large_results") - includeLargeResults := ok && includeLargeResultsRaw.(bool) + includeAll, maxResults, err := processLimit(d) + if err != nil { + return nil, err + } - leases, warning, err := b.Core.expiration.listIrrevocableLeases(ctx, includeChildNamespaces, includeLargeResults) + leases, warning, err := b.Core.expiration.listIrrevocableLeases(ctx, includeChildNamespaces, includeAll, maxResults) if err != nil { return nil, err } diff --git a/vault/logical_system_paths.go b/vault/logical_system_paths.go index 4dc8f1cba8aba..c233a988765ed 100644 --- a/vault/logical_system_paths.go +++ b/vault/logical_system_paths.go @@ -1248,10 +1248,10 @@ func (b *SystemBackend) leasePaths() []*framework.Path { Default: false, Description: "Set true if you want leases for this namespace and its children.", }, - "include_large_results": { - Type: framework.TypeBool, - Default: false, - Description: "Set true to get lists containing more than 10,000 entries.", + "limit": { + Type: framework.TypeString, + Default: "", + Description: "Set to a positive integer of the maximum number of entries to return. If you want all results, set to 'none'. If not set, you will get a maximum of 10,000 results returned.", }, }, diff --git a/vault/logical_system_test.go b/vault/logical_system_test.go index ca235641630d7..7dbb924294944 100644 --- a/vault/logical_system_test.go +++ b/vault/logical_system_test.go @@ -3580,3 +3580,89 @@ func makeStorage(t *testing.T, entries ...*logical.StorageEntry) *logical.InmemS return store } + +func leaseLimitFieldData(limit string) *framework.FieldData { + raw := make(map[string]interface{}) + raw["limit"] = limit + return &framework.FieldData{ + Raw: raw, + Schema: map[string]*framework.FieldSchema{ + "limit": { + Type: framework.TypeString, + Default: "", + Description: "limit return results", + }, + }, + } +} + +func TestProcessLimit(t *testing.T) { + testCases := []struct { + d *framework.FieldData + expectReturnAll bool + expectLimit int + expectErr bool + }{ + { + d: leaseLimitFieldData("500"), + expectReturnAll: false, + expectLimit: 500, + expectErr: false, + }, + { + d: leaseLimitFieldData(""), + expectReturnAll: false, + expectLimit: MaxIrrevocableLeasesToReturn, + expectErr: false, + }, + { + d: leaseLimitFieldData("none"), + expectReturnAll: true, + expectLimit: 10000, + expectErr: false, + }, + { + d: leaseLimitFieldData("NoNe"), + expectReturnAll: true, + expectLimit: 10000, + expectErr: false, + }, + { + d: leaseLimitFieldData("hello_world"), + expectReturnAll: false, + expectLimit: 0, + expectErr: true, + }, + { + d: leaseLimitFieldData("0"), + expectReturnAll: false, + expectLimit: 0, + expectErr: true, + }, + { + d: leaseLimitFieldData("-1"), + expectReturnAll: false, + expectLimit: 0, + expectErr: true, + }, + } + + for i, tc := range testCases { + returnAll, limit, err := processLimit(tc.d) + + if returnAll != tc.expectReturnAll { + t.Errorf("bad return all for test case %d. expected %t, got %t", i, tc.expectReturnAll, returnAll) + } + if limit != tc.expectLimit { + t.Errorf("bad limit for test case %d. expected %d, got %d", i, tc.expectLimit, limit) + } + + haveErr := err != nil + if haveErr != tc.expectErr { + t.Errorf("bad error status for test case %d. expected error: %t, got error: %t", i, tc.expectErr, haveErr) + if err != nil { + t.Errorf("error was: %v", err) + } + } + } +} diff --git a/website/content/api-docs/system/leases.mdx b/website/content/api-docs/system/leases.mdx index efb22e28dde2c..e5fee527ed135 100644 --- a/website/content/api-docs/system/leases.mdx +++ b/website/content/api-docs/system/leases.mdx @@ -290,10 +290,10 @@ This endpoint was added in Vault 1.8. - `type` (string: ) - Specifies the type of lease. - `include_child_namespaces` (bool: false) - Specifies if leases in child namespaces should be included in the result -- `include_large_results` (bool: false) - Specifies if the request should - complete if there are more than 10,000 results to return. If this is set - false, the client will send an error specifying that there are many results, - and the request must be retried using the `force` flag. +- `limit` (string: "") - Specifies the maximum number of leases to return in a + request. To return all results, set to `none`. If not set, this API will + return a maximum of 10,000 leases. If not set to `none` and there exist more + leases than `limit`, the response will include a warning. | Method | Path | | :----- | :------------ | From 23a10820e499497ed8bef397f047ab8734609cc4 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Tue, 1 Jun 2021 10:56:50 -0600 Subject: [PATCH 22/24] sort first by leaseID, then stable sort by expiration --- vault/expiration.go | 19 ++++++++++++------- vault/expiration_test.go | 18 +++++++++--------- vault/expiration_util.go | 23 +++++++++++++++++------ 3 files changed, 38 insertions(+), 22 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index 6b68eb5251dd4..a49797e8ece5a 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -2490,9 +2490,10 @@ func (m *ExpirationManager) getIrrevocableLeaseCounts(ctx context.Context, inclu } type leaseResponse struct { - LeaseID string `json:"lease_id"` - MountID string `json:"mount_id"` - ErrMsg string `json:"error"` + LeaseID string `json:"lease_id"` + MountID string `json:"mount_id"` + ErrMsg string `json:"error"` + expireTime time.Time } // returns a warning string, if applicable @@ -2536,18 +2537,22 @@ func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeCh numMatchingLeases++ matchingLeases = append(matchingLeases, &leaseResponse{ - LeaseID: leaseID, - MountID: mountAccessor, - ErrMsg: leaseInfo.RevokeErr, + LeaseID: leaseID, + MountID: mountAccessor, + ErrMsg: leaseInfo.RevokeErr, + expireTime: leaseInfo.ExpireTime, }) return true }) - // sort the results for consistent API response + // sort the results for consistent API response, with the least fresh leases first in the list sort.Slice(matchingLeases, func(i, j int) bool { return matchingLeases[i].LeaseID < matchingLeases[j].LeaseID }) + sort.SliceStable(matchingLeases, func(i, j int) bool { + return matchingLeases[i].expireTime.Before(matchingLeases[j].expireTime) + }) resp := make(map[string]interface{}) resp["lease_count"] = numMatchingLeases diff --git a/vault/expiration_test.go b/vault/expiration_test.go index f3089ed6d6aa3..82986e3607f02 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -3095,18 +3095,15 @@ func TestExpiration_listIrrevocableLeases(t *testing.T) { exp := c.expiration - type basicLeaseInfo struct { - id string - mount string - } - expectedLeases := make([]*basicLeaseInfo, 0) + expectedLeases := make([]*basicLeaseTestInfo, 0) expectedPerMount := 10 for i := 0; i < expectedPerMount; i++ { for _, mountPrefix := range mountPrefixes { - leaseID := addIrrevocableLease(t, exp, mountPrefix, namespace.RootNamespace) - expectedLeases = append(expectedLeases, &basicLeaseInfo{ - id: leaseID, - mount: pathToMount[mountPrefix], + le := addIrrevocableLease(t, exp, mountPrefix, namespace.RootNamespace) + expectedLeases = append(expectedLeases, &basicLeaseTestInfo{ + id: le.id, + mount: pathToMount[mountPrefix], + expire: le.expire, }) } } @@ -3144,6 +3141,9 @@ func TestExpiration_listIrrevocableLeases(t *testing.T) { sort.Slice(expectedLeases, func(i, j int) bool { return expectedLeases[i].id < expectedLeases[j].id }) + sort.SliceStable(expectedLeases, func(i, j int) bool { + return expectedLeases[i].expire.Before(expectedLeases[j].expire) + }) for i, lease := range expectedLeases { if lease.id != leases[i].LeaseID { diff --git a/vault/expiration_util.go b/vault/expiration_util.go index 660a690890e7c..b284199d31527 100644 --- a/vault/expiration_util.go +++ b/vault/expiration_util.go @@ -5,6 +5,7 @@ package vault import ( "context" "fmt" + "math/rand" "testing" "time" @@ -33,9 +34,15 @@ func (m *ExpirationManager) collectLeases() (map[*namespace.Namespace][]string, return existing, leaseCount, nil } +type basicLeaseTestInfo struct { + id string + mount string + expire time.Time +} + // add an irrevocable lease for test purposes -// returns the lease ID for the lease -func addIrrevocableLease(t *testing.T, m *ExpirationManager, pathPrefix string, ns *namespace.Namespace) string { +// returns the lease ID and expire time +func addIrrevocableLease(t *testing.T, m *ExpirationManager, pathPrefix string, ns *namespace.Namespace) *basicLeaseTestInfo { t.Helper() uuid, err := uuid.GenerateUUID() @@ -52,12 +59,13 @@ func addIrrevocableLease(t *testing.T, m *ExpirationManager, pathPrefix string, nsSuffix = fmt.Sprintf("/blah.%s", ns.ID) } + randomTimeDelta := time.Duration(rand.Int31n(24)) le := &leaseEntry{ LeaseID: pathPrefix + "lease" + uuid + nsSuffix, Path: pathPrefix + nsSuffix, namespace: ns, IssueTime: time.Now(), - ExpireTime: time.Now().Add(time.Hour), + ExpireTime: time.Now().Add(randomTimeDelta * time.Hour), RevokeErr: "some error message", } @@ -70,7 +78,10 @@ func addIrrevocableLease(t *testing.T, m *ExpirationManager, pathPrefix string, m.updatePendingInternal(le) - return le.LeaseID + return &basicLeaseTestInfo{ + id: le.LeaseID, + expire: le.ExpireTime, + } } // InjectIrrevocableLeases injects `count` irrevocable leases (currently to a @@ -79,9 +90,9 @@ func addIrrevocableLease(t *testing.T, m *ExpirationManager, pathPrefix string, func (c *Core) InjectIrrevocableLeases(t *testing.T, ctx context.Context, count int) map[string]int { out := make(map[string]int) for i := 0; i < count; i++ { - leaseID := addIrrevocableLease(t, c.expiration, "foo/", namespace.RootNamespace) + le := addIrrevocableLease(t, c.expiration, "foo/", namespace.RootNamespace) - mountAccessor := c.expiration.getLeaseMountAccessor(ctx, leaseID) + mountAccessor := c.expiration.getLeaseMountAccessor(ctx, le.id) if _, ok := out[mountAccessor]; !ok { out[mountAccessor] = 0 } From 3b618b808fe03a0b533581cc42a4645ed2ff6b82 Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Tue, 1 Jun 2021 11:02:20 -0600 Subject: [PATCH 23/24] move some utils to be in oss and ent --- vault/expiration_util.go | 74 ------------------------------ vault/expiration_util_common.go | 81 +++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 74 deletions(-) create mode 100644 vault/expiration_util_common.go diff --git a/vault/expiration_util.go b/vault/expiration_util.go index b284199d31527..eac1703bc6190 100644 --- a/vault/expiration_util.go +++ b/vault/expiration_util.go @@ -3,13 +3,8 @@ package vault import ( - "context" "fmt" - "math/rand" - "testing" - "time" - uuid "github.com/hashicorp/go-uuid" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/sdk/logical" ) @@ -33,72 +28,3 @@ func (m *ExpirationManager) collectLeases() (map[*namespace.Namespace][]string, leaseCount += len(keys) return existing, leaseCount, nil } - -type basicLeaseTestInfo struct { - id string - mount string - expire time.Time -} - -// add an irrevocable lease for test purposes -// returns the lease ID and expire time -func addIrrevocableLease(t *testing.T, m *ExpirationManager, pathPrefix string, ns *namespace.Namespace) *basicLeaseTestInfo { - t.Helper() - - uuid, err := uuid.GenerateUUID() - if err != nil { - t.Fatalf("error generating uuid: %v", err) - } - - if ns == nil { - ns = namespace.RootNamespace - } - - nsSuffix := "" - if ns != namespace.RootNamespace { - nsSuffix = fmt.Sprintf("/blah.%s", ns.ID) - } - - randomTimeDelta := time.Duration(rand.Int31n(24)) - le := &leaseEntry{ - LeaseID: pathPrefix + "lease" + uuid + nsSuffix, - Path: pathPrefix + nsSuffix, - namespace: ns, - IssueTime: time.Now(), - ExpireTime: time.Now().Add(randomTimeDelta * time.Hour), - RevokeErr: "some error message", - } - - m.pendingLock.Lock() - defer m.pendingLock.Unlock() - - if err := m.persistEntry(context.Background(), le); err != nil { - t.Fatalf("error persisting irrevocable lease: %v", err) - } - - m.updatePendingInternal(le) - - return &basicLeaseTestInfo{ - id: le.LeaseID, - expire: le.ExpireTime, - } -} - -// InjectIrrevocableLeases injects `count` irrevocable leases (currently to a -// single mount). -// It returns a map of the mount accessor to the number of leases stored there -func (c *Core) InjectIrrevocableLeases(t *testing.T, ctx context.Context, count int) map[string]int { - out := make(map[string]int) - for i := 0; i < count; i++ { - le := addIrrevocableLease(t, c.expiration, "foo/", namespace.RootNamespace) - - mountAccessor := c.expiration.getLeaseMountAccessor(ctx, le.id) - if _, ok := out[mountAccessor]; !ok { - out[mountAccessor] = 0 - } - - out[mountAccessor]++ - } - - return out -} diff --git a/vault/expiration_util_common.go b/vault/expiration_util_common.go new file mode 100644 index 0000000000000..de1a4ad2ab103 --- /dev/null +++ b/vault/expiration_util_common.go @@ -0,0 +1,81 @@ +package vault + +import ( + "context" + "fmt" + "math/rand" + "testing" + "time" + + uuid "github.com/hashicorp/go-uuid" + "github.com/hashicorp/vault/helper/namespace" +) + +type basicLeaseTestInfo struct { + id string + mount string + expire time.Time +} + +// add an irrevocable lease for test purposes +// returns the lease ID and expire time +func addIrrevocableLease(t *testing.T, m *ExpirationManager, pathPrefix string, ns *namespace.Namespace) *basicLeaseTestInfo { + t.Helper() + + uuid, err := uuid.GenerateUUID() + if err != nil { + t.Fatalf("error generating uuid: %v", err) + } + + if ns == nil { + ns = namespace.RootNamespace + } + + nsSuffix := "" + if ns != namespace.RootNamespace { + nsSuffix = fmt.Sprintf("/blah.%s", ns.ID) + } + + randomTimeDelta := time.Duration(rand.Int31n(24)) + le := &leaseEntry{ + LeaseID: pathPrefix + "lease" + uuid + nsSuffix, + Path: pathPrefix + nsSuffix, + namespace: ns, + IssueTime: time.Now(), + ExpireTime: time.Now().Add(randomTimeDelta * time.Hour), + RevokeErr: "some error message", + } + + m.pendingLock.Lock() + defer m.pendingLock.Unlock() + + if err := m.persistEntry(context.Background(), le); err != nil { + t.Fatalf("error persisting irrevocable lease: %v", err) + } + + m.updatePendingInternal(le) + + return &basicLeaseTestInfo{ + id: le.LeaseID, + expire: le.ExpireTime, + } +} + +// InjectIrrevocableLeases injects `count` irrevocable leases (currently to a +// single mount). +// It returns a map of the mount accessor to the number of leases stored there +func (c *Core) InjectIrrevocableLeases(t *testing.T, ctx context.Context, count int) map[string]int { + out := make(map[string]int) + for i := 0; i < count; i++ { + le := addIrrevocableLease(t, c.expiration, "foo/", namespace.RootNamespace) + + mountAccessor := c.expiration.getLeaseMountAccessor(ctx, le.id) + if _, ok := out[mountAccessor]; !ok { + out[mountAccessor] = 0 + } + + out[mountAccessor]++ + } + + return out +} From 585caeafff2c6c91a10d77628c2375d58fdb205e Mon Sep 17 00:00:00 2001 From: Stephen Wayne Date: Wed, 2 Jun 2021 09:43:31 -0600 Subject: [PATCH 24/24] improve sort efficiency for API response --- vault/expiration.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index a49797e8ece5a..376c6c1ebe639 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -2546,13 +2546,15 @@ func (m *ExpirationManager) listIrrevocableLeases(ctx context.Context, includeCh return true }) - // sort the results for consistent API response, with the least fresh leases first in the list + // sort the results for consistent API response. we primarily sort on + // increasing expire time, and break ties with increasing lease id sort.Slice(matchingLeases, func(i, j int) bool { + if !matchingLeases[i].expireTime.Equal(matchingLeases[j].expireTime) { + return matchingLeases[i].expireTime.Before(matchingLeases[j].expireTime) + } + return matchingLeases[i].LeaseID < matchingLeases[j].LeaseID }) - sort.SliceStable(matchingLeases, func(i, j int) bool { - return matchingLeases[i].expireTime.Before(matchingLeases[j].expireTime) - }) resp := make(map[string]interface{}) resp["lease_count"] = numMatchingLeases