-
Notifications
You must be signed in to change notification settings - Fork 7k
[Core] Token Auth minor UX Improvements #58998
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
Conversation
Signed-off-by: sampan <sampan@anyscale.com>
Signed-off-by: sampan <sampan@anyscale.com>
| try: | ||
| ensure_token_if_auth_enabled(system_config, create_token_if_missing=False) | ||
| except ray.exceptions.AuthenticationError: | ||
| raise RuntimeError( |
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.
@edoakes I removed this as this was throwing a nested stack trace:
ray start --head
Usage stats collection is enabled. To disable this, add `--disable-usage-stats` to the command that starts the cluster, or run the following command: `ray disable-usage-stats` before starting the cluster. See https://docs.ray.io/en/master/cluster/usage-stats.html for more details.
Local node IP: 172.31.5.49
Traceback (most recent call last):
File "/home/ubuntu/clone/ray/python/ray/scripts/scripts.py", line 946, in start
ensure_token_if_auth_enabled(system_config, create_token_if_missing=False)
File "/home/ubuntu/clone/ray/python/ray/_private/authentication/authentication_token_setup.py", line 101, in ensure_token_if_auth_enabled
raise AuthenticationError(
ray.exceptions.AuthenticationError: Token authentication is enabled but no authentication token was found.. Ensure that you have `RAY_AUTH_MODE=token` set and the token for the cluster is available as the `RAY_AUTH_TOKEN` environment variable or a local file. For more information, see: https://docs.ray.io/en/latest/ray-security/auth.html
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/ubuntu/.conda/envs/ray-dev/bin/ray", line 7, in <module>
sys.exit(main())
File "/home/ubuntu/clone/ray/python/ray/scripts/scripts.py", line 2822, in main
return cli()
File "/home/ubuntu/.conda/envs/ray-dev/lib/python3.10/site-packages/click/core.py", line 1442, in __call__
return self.main(*args, **kwargs)
File "/home/ubuntu/.conda/envs/ray-dev/lib/python3.10/site-packages/click/core.py", line 1363, in main
rv = self.invoke(ctx)
File "/home/ubuntu/.conda/envs/ray-dev/lib/python3.10/site-packages/click/core.py", line 1830, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/home/ubuntu/.conda/envs/ray-dev/lib/python3.10/site-packages/click/core.py", line 1226, in invoke
return ctx.invoke(self.callback, **ctx.params)
File "/home/ubuntu/.conda/envs/ray-dev/lib/python3.10/site-packages/click/core.py", line 794, in invoke
return callback(*args, **kwargs)
File "/home/ubuntu/clone/ray/python/ray/autoscaler/_private/cli_logger.py", line 823, in wrapper
return f(*args, **kwargs)
File "/home/ubuntu/clone/ray/python/ray/scripts/scripts.py", line 948, in start
raise RuntimeError(
RuntimeError: Failed to load authentication token. To generate a token for local development, use `ray get-auth-token --generate`. For remote clusters, ensure that the token is propagated to all nodes of the cluster when token authentication is enabled.
I feel exposing the AuthenticationError to users may be better than catching and throwing a new runtime exception. please let me know your thoughts
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 have included the message from the runtime error in the AuthenticationError so the message will still be quite helpful
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.
Code Review
This pull request introduces several valuable UX improvements for token authentication. The changes enhance error messages to be more informative, improve logging for token loading, and add crucial integration tests for the state API. The implementation is solid, and the new tests provide good coverage. I have a couple of minor suggestions to improve code style and robustness.
Signed-off-by: sampan <sampan@anyscale.com>
## Description - convert debug logs in authentication_token_loader to info so that users are aware of where the token being used is being loaded from - When we raise the `AuthenticationError`, if RAY_AUTH_MODE is not set to token we should explicitly print that in the error message - in error messages suggest storing tokens in filesystem instead of env - add state api tests in test_token_auth_integration.py --------- Signed-off-by: sampan <sampan@anyscale.com> Co-authored-by: sampan <sampan@anyscale.com>
Description
AuthenticationError, if RAY_AUTH_MODE is not set to token we should explicitly print that in the error message