Arvados

Ready for review in ccb603fe5a8ca989d6db97cc723ccfcaba2781f5 on branch 18281-login-sync-expired-tokens

• I'm not sure if I'm following correctly, but with this change, the --rotate-tokens flag seems to me to be superfluous: If the tokenfile exists, it checks the token's validity; if it doesn't exist, it'll create a new one no matter if the --rotate-tokens flag was used or not, right?
• Your update will automatically detect token expirations, and I think that's a better approach than having the admin set up the flag on those clusters migrating towards token expiration.
• If my appreciation is correct, I think we can safely remove the flag.

Lucas Di Pentima wrote:

• I'm not sure if I'm following correctly, but with this change, the --rotate-tokens flag seems to me to be superfluous: If the tokenfile exists, it checks the token's validity; if it doesn't exist, it'll create a new one no matter if the --rotate-tokens flag was used or not, right?

Right - but if you give it --rotate-tokens it will force a token rotation, even if there is an existing token and it is valid. I don't know why you'd want to do that, but I guess it could still be useful when you want to force-rotate all the tokens? The comment is misleading though, I've pushed e1c4967befc7b4dd273b3d9d047a4e4262f5ba2f to fix that.

• Your update will automatically detect token expirations, and I think that's a better approach than having the admin set up the flag on those clusters migrating towards token expiration.
• If my appreciation is correct, I think we can safely remove the flag.

If it's OK with you I'd keep it with the updated help text?

Ward Vandewege wrote:

Right - but if you give it --rotate-tokens it will force a token rotation, even if there is an existing token and it is valid. I don't know why you'd want to do that, but I guess it could still be useful when you want to force-rotate all the tokens? The comment is misleading though, I've pushed e1c4967befc7b4dd273b3d9d047a4e4262f5ba2f to fix that.

I kind of remember that the train of thought went something like this: On a expiring tokens installation, pass --token-lifetime N and --rotate-tokens while calling the script every N-X where X is a certain amount of minutes of acceptable grace period. But this doesn't work with auto-expiring tokens. (maybe this got lost when we changed the token expiration approach)

• Your update will automatically detect token expirations, and I think that's a better approach than having the admin set up the flag on those clusters migrating towards token expiration.
• If my appreciation is correct, I think we can safely remove the flag.

If it's OK with you I'd keep it with the updated help text?

I don't think it provides any benefit (other than don't error out on existing installations using the flag) besides what your updates provide, but also it doesn't do any harm to be left there, so it LGTM. :)

Lucas Di Pentima wrote:

I don't think it provides any benefit (other than don't error out on existing installations using the flag) besides what your updates provide, but also it doesn't do any harm to be left there, so it LGTM. :)

Thanks, merged!