acl: tokens can be created with an optional expiration time #5353
Conversation
| for _, v := range resp.Tokens { | ||
| retrievedTokens = append(retrievedTokens, v.AccessorID) | ||
| } | ||
| require.Subset(t, retrievedTokens, tokens) |
There was a problem hiding this comment.
I removed use of require.Subset in this whole file because it has the potential (as I encountered legitimately) to mask bugs in returning data the function should not.
|
@mkeeler I think I've addressed the points you raised. |
rboyer
force-pushed
the
f-token-expiration
branch
from
c2f761c
to
3229c5b
Compare
rboyer
force-pushed
the
f-token-expiration
branch
2 times, most recently
from
4079224
to
8e9b063
Compare
rboyer
force-pushed
the
f-token-expiration
branch
from
28a87ac
to
c756611
Compare
|
Not sure where this is at but I think it's looking good to me from a not-in-depth view. The TODO items remaining seem important do you intend to do those in this PR or a later one? Would be good to have Matt look closer at the subtleties in the ACL stuff - I can't say I'm confident there are no bugs I've missed there from higher level skimming!
Yeah MemDB (and iradix) is pretty limited - I found this recently we can't do "lower bound" queries in general and so no range scans semantics unless you happen to know the exact value of the first key you care about or want to scan from the start of the index (luckily you do want that in case of next token to expire). One day maybe I'll change that if I ever finish my rewrite of iradix/memdb 😄 . |
If you're referring to the checkboxes at the top, I was considering having a long-lived draft PR of Mostly with these incremental feature branches I'm looking for correctness/performance issues and figure that the final cleanup for the whole overall ACL UX feature set can be handled before it goes into master. |
rboyer
force-pushed
the
f-token-expiration
branch
from
c756611
to
473db0d
Compare
|
I believe I've addressed the outstanding commentary. |
rboyer
force-pushed
the
f-token-expiration
branch
from
ff0f4c0
to
e9439b5
Compare
Note: this is merging into a long lived feature branch, not master. Future PRs in this series may adjust the contents here slightly as needed. These are separated out for digestibility.
I'm mostly looking for early feedback on portions of the overall changeset. The final product that merges into master may end up with small adjustments in further PRs as issues arise.
There are a pair of new memdb indexes used to to keep track of upcoming tokens to expire (one for local tokens and one for global tokens). I would have preferred a single composite index of
(local, expirationTime)but I could not figure out how to find the min/max elements of a composite index when presented with an exact match for one portion of the composite index.