-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Remove artificial ACLTokenMaxTTL limit for configuring acl token expiry #17066
Conversation
@@ -34,7 +34,7 @@ func main() { | |||
sort.Sort(sort.Reverse(sort.StringSlice(cev.EnvoyVersions))) | |||
|
|||
ceVersions := consulEnvoyVersions{ | |||
ConsulVersion: string(cVersion), | |||
ConsulVersion: cVersion, |
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.
This was causing issues with the linter.
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.
It looks like this change has already been applied to main
. You may need to merge/rebase.
@@ -3261,21 +3261,6 @@ func TestACLEndpoint_AuthMethodSet(t *testing.T) { | |||
err := aclEp.AuthMethodSet(&req, &resp) | |||
testutil.RequireErrorContains(t, err, "MaxTokenTTL 1ms cannot be less than") | |||
}) | |||
|
|||
t.Run("Create with MaxTokenTTL too big", func(t *testing.T) { |
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.
The default max is now the max int64 value so removing this test.
ACLTokenMaxExpirationTTL: 24 * time.Hour, | ||
// Duration is stored as an int64. Setting the default max | ||
// to the max possible duration (approx 290 years). | ||
ACLTokenMaxExpirationTTL: 1<<63 - 1, |
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.
Alternatively, we could rework things so that this value is completely removed but I figured it makes sense to allow people to still configure their own max if they want. So just setting the default to the max time.Duration
value.
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.
This is awesome @johnlanda. Congrats on your first PR! I just had one non-blocking nitpick.
@@ -34,7 +34,7 @@ func main() { | |||
sort.Sort(sort.Reverse(sort.StringSlice(cev.EnvoyVersions))) | |||
|
|||
ceVersions := consulEnvoyVersions{ | |||
ConsulVersion: string(cVersion), | |||
ConsulVersion: cVersion, |
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.
It looks like this change has already been applied to main
. You may need to merge/rebase.
aed1cce
to
d8b841e
Compare
Description
The default expiry for an ACL token is never. Currently, when supplying an acl token ttl value the max allowed via the
-expires-ttl
flag is 24 hours.This change sets the default max ttl value for ACL tokens to the max duration value.
Testing & Reproduction steps
command/acl/token/create/token_create_test.go
expires-ttl_short
expires-ttl_long
Links
-expires-ttl flag docs
PR Checklist