-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
WIP: Add websocket browser_redirect_url config option #1671
base: main
Are you sure you want to change the base?
Conversation
If an HTTP(S) request is received without the Upgrade: header, then if this new option is set then we'll assume it's a browser and issue a redirect. There's no flexibility based upon the passed in URL; this should help with redirecting people to dashboards or other tooling.
I left in that there's logging of an error for such a request, but the logging doesn't appear to pull in the source address. So two refinements suggest themselves:
Either or both of these seem worthwhile. 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.
In general I like this, but we also need to support operator driven versions where the redirect URL is contained in the operator JWT. /cc @matthiashanel
@derekcollison will add a corresponding jwt field to v2 |
We should make sure we can eventually handle this properly with multi-operator setups. |
@derekcollison the way I would go about handling this in a multi operator setup is by calling out one of the operators to be operating the server. |
I like that approach.. |
@derekcollison @matthiashanel I am a bit confused. This parameter that @philpennock added is for situations where something connects to the Websocket port that is detected as likely not being a client (since it does not have the proper http headers). So obviously the option cannot be linked to a specific account (since there is no auth at the point the error is detected). |
Yes operator JWT, and I think anything we can do to skinny down the actual server config in operator mode is a good thing. Much rather update operator JWT then all configs for all servers etc. |
Since this is a websocket port, I am not sure why we do an HTTP redirect - websocket URLs for one are ws(s) - if an HTTP client hits the ws URL using a browser, redirecting doesn't seem like the right strat. |
TBH, if we are going to do this here, then why aren't we doing a similar assertion on if a browser is pointed to the nats service port or if a nats client is pointed to the monitoring port. |
@philpennock Is this still relevant? |
If an HTTP(S) request is received without the Upgrade: header, then if this new
option is set then we'll assume it's a browser and issue a redirect.
There's no flexibility based upon the passed in URL; this should help with
redirecting people to dashboards or other tooling.
There's no documentation and no tests, this should not be merged as-is, the PR
exists for people to discuss, or replace with something better.
/cc @nats-io/core