Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vault 1979: Query API for Irrevocable Leases #11607

Merged
merged 28 commits into from Jun 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
483feae
build out lease count (not fully working), start lease list
swayne275 May 13, 2021
9d9f837
build out irrevocable lease list
swayne275 May 13, 2021
41dfaba
bookkeeping
swayne275 May 13, 2021
167ab90
Merge branch 'master' into vault-1979_expr-irrevocable-query-api
swayne275 May 13, 2021
ed4f011
test irrevocable lease counts for API/CLI
swayne275 May 20, 2021
e76af8a
fix listIrrevocableLeases, test listIrrevocableLeases, cleanup
swayne275 May 20, 2021
6ab77e4
test expiration API limit
swayne275 May 20, 2021
61786f5
namespace tweaks, test force flag on lease list
swayne275 May 20, 2021
7bc6712
integration test leases/count API, plenty of fixes and improvements
swayne275 May 21, 2021
a5450c8
Merge branch 'master' into vault-1979_expr-irrevocable-query-api
swayne275 May 21, 2021
e18fad4
test lease list API, fixes and improvements
swayne275 May 21, 2021
61e8316
test force flag for irrevocable lease list API
swayne275 May 21, 2021
d5164e3
i guess this wasn't saved on the last refactor...
swayne275 May 21, 2021
fb8b995
fixes and improvements found during my review
swayne275 May 21, 2021
b81ea9b
better test error msg
swayne275 May 21, 2021
743a32b
Merge branch 'master' into vault-1979_expr-irrevocable-query-api
swayne275 May 25, 2021
6114caa
Update vault/logical_system_paths.go
swayne275 May 25, 2021
0d61f9c
Update vault/logical_system_paths.go
swayne275 May 25, 2021
cb47c4a
return warning with data if more than default leases to list without …
swayne275 May 25, 2021
e5dafb4
make api doc more generalized
swayne275 May 26, 2021
6f2abe7
list leases in general, not by mount point
swayne275 May 26, 2021
da988f9
change force flag to include_large_results
swayne275 May 26, 2021
d06e923
sort leases by LeaseID for consistent API response
swayne275 May 26, 2021
33c1d4c
switch from bool flag for API limit to string value
swayne275 May 27, 2021
23a1082
sort first by leaseID, then stable sort by expiration
swayne275 Jun 1, 2021
3b618b8
move some utils to be in oss and ent
swayne275 Jun 1, 2021
fd2044d
Merge branch 'master' into vault-1979_expr-irrevocable-query-api
swayne275 Jun 1, 2021
585caea
improve sort efficiency for API response
swayne275 Jun 2, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/11607.txt
@@ -0,0 +1,3 @@
```release-note:improvement
core: add irrevocable lease list and count apis
```
169 changes: 157 additions & 12 deletions vault/expiration.go
Expand Up @@ -8,6 +8,7 @@ import (
"math/rand"
"os"
"path"
"sort"
"strconv"
"strings"
"sync"
Expand Down Expand Up @@ -73,6 +74,12 @@ 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

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 {
Expand Down Expand Up @@ -261,18 +268,7 @@ 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()

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 {
Expand Down Expand Up @@ -2418,6 +2414,155 @@ 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)

// avoid re-declaring leaseNS and err with scope inside the if
leaseNS := namespace.RootNamespace
var err error
if nsID != "" {
leaseNS, err = NamespaceByID(ctx, nsID, m.core)
if err != nil {
return nil, err
}
}

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 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 {
m.logger.Error("could not get namespace from context", "error", err)
return nil, err
}

numMatchingLeasesPerMount := make(map[string]int)
numMatchingLeases := 0
m.irrevocable.Range(func(k, v interface{}) bool {
leaseID := k.(string)
leaseNS, err := m.getNamespaceFromLeaseID(ctx, leaseID)
if err != nil {
// 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
}

leaseMatches := (leaseNS == requestNS) || (includeChildNamespaces && leaseNS.HasParent(requestNS))
if !leaseMatches {
// the lease doesn't meet our criteria, so keep looking
return true
}

mountAccessor := m.getLeaseMountAccessor(ctx, leaseID)

if _, ok := numMatchingLeasesPerMount[mountAccessor]; !ok {
numMatchingLeasesPerMount[mountAccessor] = 0
}

numMatchingLeases++
numMatchingLeasesPerMount[mountAccessor]++

return true
})

resp := make(map[string]interface{})
resp["lease_count"] = numMatchingLeases
resp["counts"] = numMatchingLeasesPerMount

return resp, nil
}

type leaseResponse struct {
LeaseID string `json:"lease_id"`
MountID string `json:"mount_id"`
ErrMsg string `json:"error"`
expireTime time.Time
}

// returns a warning string, if applicable
// 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)
return nil, "", err
}

// map of mount point : lease info
matchingLeases := make([]*leaseResponse, 0)
numMatchingLeases := 0
var warning string
m.irrevocable.Range(func(k, v interface{}) bool {
Copy link
Member

Choose a reason for hiding this comment

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

One downside to using a sync.Map here is that the range scans will be unordered. We could sort the leases before returning them, but in the case where there are > MaxIrrevocableLeasesToReturn leases we may return a different set of leases each call. For now should we sort the values before returning them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, i think there's benefit to that consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've also thought that getting away from sync.Map could be nice so that when there are >10,000 results we can tell the client how many to expect (without doing the work of going through all of them)... a story for another day perhaps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 !returnAll && (numMatchingLeases >= limit) {
m.logger.Warn("hit max irrevocable leases without force flag set")
warning = MaxIrrevocableLeasesWarning
return false
}

mountAccessor := m.getLeaseMountAccessor(ctx, leaseID)

numMatchingLeases++
matchingLeases = append(matchingLeases, &leaseResponse{
LeaseID: leaseID,
MountID: mountAccessor,
ErrMsg: leaseInfo.RevokeErr,
expireTime: leaseInfo.ExpireTime,
})

return true
})

// 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is fine, but we could also sort by expiration time then lease id, so the operator can get a sense of how old the oldest irrevocable leases are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that sounds like a good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Other way around on that sort? I think you want to do it in just one pass with the sort predicate testing expiration time and returning if non-equal, then testing leaseid if the expiration times are equal.

Copy link
Contributor Author

@swayne275 swayne275 Jun 1, 2021

Choose a reason for hiding this comment

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

can you clarify how you'd like this sorted? the current code will display the oldest-expiring leases at the top of the list, and if multiple leases have the same expiration time it will then display them in increasing order by the lease id (a comes before b)

i can certainly do it in one slice sort path, but let's verify how we want the results displayed first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we chatted in slack. sorting is correct, but made more efficient here: 585caea

})

resp := make(map[string]interface{})
resp["lease_count"] = numMatchingLeases
resp["leases"] = matchingLeases

return resp, warning, nil
}

// leaseEntry is used to structure the values the expiration
// manager stores. This is used to handle renew and revocation.
type leaseEntry struct {
Expand Down