-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Vault 1979: Query API for Irrevocable Leases #11607
Changes from all commits
483feae
9d9f837
41dfaba
167ab90
ed4f011
e76af8a
6ab77e4
61786f5
7bc6712
a5450c8
e18fad4
61e8316
d5164e3
fb8b995
b81ea9b
743a32b
6114caa
0d61f9c
cb47c4a
e5dafb4
6f2abe7
da988f9
d06e923
33c1d4c
23a1082
3b618b8
fd2044d
585caea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
```release-note:improvement | ||
core: add irrevocable lease list and count apis | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import ( | |
"math/rand" | ||
"os" | ||
"path" | ||
"sort" | ||
"strconv" | ||
"strings" | ||
"sync" | ||
|
@@ -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 { | ||
|
@@ -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 { | ||
|
@@ -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 { | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah that sounds like a good idea There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i think there's benefit to that consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 perhapsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d06e923