-
Notifications
You must be signed in to change notification settings - Fork 359
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
feat: add master configurations for access token max and default lifespans [DET-10464] #10101
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10101 +/- ##
==========================================
+ Coverage 54.71% 54.72% +0.01%
==========================================
Files 1266 1266
Lines 159970 160013 +43
Branches 3662 3662
==========================================
+ Hits 87525 87568 +43
Misses 72312 72312
Partials 133 133
Flags with carried forward coverage won't be shown. Click here to find out more.
|
✅ Deploy Preview for determined-ui canceled.
|
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.
suggested edits
expiration_in_hours = None | ||
if args.expiration_days is not None: | ||
expiration_in_hours = ( | ||
"-1" if args.expiration_days == -1 else f"{24 * args.expiration_days}h" |
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.
might be worth just making sure that args.expiration_days is an int here, if it's a float not sure what would happen
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.
With the current code, any float input would return an error invalid int value: '5.0'
. I think we should keep it as is.
Another option is to explicitly cast the input to an integer. However, the potential issue with this approach is that it could mislead users into thinking that floating-point values are valid, which might give the impression that fractional days are allowed.
expiration_in_hours = None
if args.expiration_days is not None:
expiration_days = int(args.expiration_days) # Ensure it's an integer
expiration_in_hours = "-1" if expiration_days == -1 else f"{24 * expiration_days}h"
what's the reason for adding both these configs to the master config? i get why a cluster admin would want to set max_expiration, but is default necessary? it's not a big deal, but if it's unlikely to be used it's just a bit more bloat in the master config. |
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.
only reviewed python side, deferring to @stoksc for master stuff
master/internal/config/config.go
Outdated
@@ -111,6 +120,10 @@ func DefaultConfig() *Config { | |||
RsaKeySize: 1024, | |||
}, | |||
AuthZ: *DefaultAuthZConfig(), | |||
Token: TokenConfig{ | |||
MaxLifespanDays: DefaultTokenMaxLifespanDays, |
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 don't think our default should be infinite. Let's make it 30 days and let folks opt-in to these wild infinite lifespan tokens.
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.
leaving this partial review but we're chatting more online
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.
LGTM
========= | ||
|
||
Applies only to Determined Enterprise Edition. Defines default and maximum lifespan settings for | ||
access tokens. These settings allow administrators to control how long access tokens can remain |
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.
reading this it is not clear to me the difference between an "access token" and a regular token, and i'm not sure how a user would know there is a difference, but I dont have an immediate idea of how to fix it.
…spans [DET-10464] (#10101) Co-authored-by: ShreyaLnuHpe <[email protected]> Co-authored-by: Bradley Laney <[email protected]>
…spans [DET-10464] (#10101) Co-authored-by: ShreyaLnuHpe <[email protected]> Co-authored-by: Bradley Laney <[email protected]>
Ticket
DET-10464
Description
Adds default & max lifespan master configurations for access tokens to set global defaults & limits on how long access tokens can be valid for.
You would need to define in master configuration as below:
Constants that are defined:
API endpoint:
POST /api/v1/tokens
No changes in GET and PATCH APIs
CLI changes:
Creating token with indefinite expiration days
Creating token with 30 expiration days
Test Plan
Try below CLI commands
% det token create -e -1
% det token create -e 30
Checklist
docs/release-notes/
See Release Note for details.