Skip to content
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

dashboard: allow server_kwargs to override defaults #2955

Merged
merged 2 commits into from Aug 15, 2019

Conversation

@bmerry
Copy link
Contributor

commented Aug 14, 2019

Closes #2915.

@bmerry

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@martindurant this is what I had in mind. I'm not sure what to do about tests, because from what I can see none of the existing tests in distributed/dashboard/tests are customising service_kwargs. Is it all being tested via the CLI tests? Should I wire up a CLI option for allow_websocket_origin to test it?

@martindurant

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

I don't know about the testing per se, but it does seem useful to allow these overrides on the CLI. It comes with the complexity of inferring types, though, so it may not be worth the hassle.

@mrocklin

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

because from what I can see none of the existing tests in distributed/dashboard/tests are customising service_kwargs

Yeah... our testing of the dashboard isn't the greatest. Honestly, most of the Dask developers don't have much experience testing web systems.

If you have thoughts on how to test this well that would certainly be welcome in order to ensure that this improvement doesn't get removed accidentally in the future.

Should I wire up a CLI option for allow_websocket_origin to test it?

The CLI options are getting somewhat unweildy. I would be more inclined to use configuration for less-frequently-used features like this if possible.

@bmerry

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Yeah... our testing of the dashboard isn't the greatest. Honestly, most of the Dask developers don't have much experience testing web systems.

I'm afraid I'm largely in the same boat.

In theory I don't think this should be that hard to test: pass an overridden allow_websocket_origin, then make an OPTIONS request to the websocket endpoint and check the Access-Control-Allow-Origin header. I'm not sure exactly where such a test belongs in the codebase though.

@mrocklin

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

I'm not sure exactly where such a test belongs in the codebase though.

I would probably use distributed/dashboard/tests/test_scheduler_bokeh.py

Hopefully there are other tests in there that would serve as examples

@mrocklin

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

It's also not a huge deal if you can't test this. It would be nice though.

@bmerry

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

It's also not a huge deal if you can't test this. It would be nice though.

I'll give it a go, but if it turns into a pain I'll abandon the testing.

@bmerry bmerry marked this pull request as ready for review Aug 15, 2019

@bmerry

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

It turns out websocket security doesn't work the way I thought it did (Same Origin Policy and CORS do not apply, and the server has to enforce origin policies), but I've cooked up a test. It might be touch fragile because it has to encode the bokeh-protocol-version into the URL.

@mrocklin

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

It might be touch fragile because it has to encode the bokeh-protocol-version into the URL.

Do you have a sense for how fragile this is or how often it is likely to change? Perhaps a comment here helping the next person along would reduce concerns?

@bmerry

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Do you have a sense for how fragile this is or how often it is likely to change?

The fact that it's still protocol version 1.0 after however many years Bokeh has been around suggests that it's unlikely to change frequently.

@mrocklin

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Sounds sensible. OK, merging. Thanks @bmerry !

@mrocklin mrocklin merged commit 6302175 into dask:master Aug 15, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bmerry bmerry deleted the bmerry:override-websocket-origin branch Aug 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.