Work/fix firewall for localhost

[maldag]

Closes #5918

This fix is mainly an implemention of the suggestion of #5918.

This is a bug in my opinion since it doesn't take the host argument of LocalCluster or SpecCluster into account and always listens to 0.0.0.0

I've tested the code with the following snippet:

import distributed

if __name__ == "__main__":
    #Use either LocalCluster or SpecCluster, works for both
    #cluster=distributed.LocalCluster(host="localhost")
    #print(cluster)
    schedulerprops = {
        "cls": distributed.Scheduler,
        "options": {"dashboard_address": ":8787", "host": "localhost", "port": 0},
    }
    workerprops = {
        "my-worker-1": {"cls": distributed.Nanny, "options": {"nthreads": 8}},
        "my-worker-2": {"cls": distributed.Nanny, "options": {"nthreads": 8}},
    }
    cluster = distributed.SpecCluster(scheduler=schedulerprops, workers=workerprops)
    print(cluster)
    

[GPUtester]

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

[maldag]

OK, there is one test failing for creating the dashboard.
My fix will redirect all empty strings to the given start_address.

If host is "localhost", this should redirect the dashboard also to localhost only IMO.
What should happen here for compatibility?

@pytest.mark.parametrize(
    "host", ["tcp://0.0.0.0", "tcp://127.0.0.1", "tcp://127.0.0.1:38275"]
)
@pytest.mark.parametrize(
    "dashboard_address,expect",
    [
        (None, ("::", "0.0.0.0")),
        ("127.0.0.1:0", ("127.0.0.1",)),
    ],
)
@gen_test()
async def test_dashboard_host(host, dashboard_address, expect):
    """Dashboard is accessible from any host by default, but it can be also bound to
    localhost.
    """
    async with Scheduler(host=host, dashboard_address=dashboard_address) as s:
        sock = first(s.http_server._sockets.values())
        assert sock.getsockname()[0] in expect

The current test case's results are:

FAILED distributed/tests/test_scheduler.py::test_dashboard_host[None-expect0-tcp://127.0.0.1] - AssertionError: assert '127.0.0.1' in ('::', '0.0.0.0')
FAILED distributed/tests/test_scheduler.py::test_dashboard_host[None-expect0-tcp://127.0.0.1:38275] - AssertionError: assert '127.0.0.1' in ('::', '0.0.0.0')
FAILED distributed/tests/test_worker.py::test_service_hosts_match_worker - AssertionError: assert '127.0.0.1' in ('::', '0.0.0.0')

I added commit 9cd91a7 to handle the default case for dashboard address listening to all interfaces

[github-actions]

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

    25 files  ±0      25 suites  ±0   10h 16m 6s ⏱️ + 6m 53s
 4 125 tests ±0   4 005 ✅ +1    111 💤 ±0   9 ❌  - 1 
47 651 runs  ±0  45 497 ✅ +2  2 105 💤  - 1  49 ❌  - 1 

For more details on these failures, see this check.

Results for commit cb9f62c. ± Comparison against base commit b28822b.

♻️ This comment has been updated with latest results.

[jacobtomlinson]

I agree that if host is set and dashboard address is not then the dashboard host should default to match the comm host.

[maldag]

I agree that if host is set and dashboard address is not then the dashboard host should default to match the comm host.

In my last commit I changed it so it stays compatible as it is. If dashboard address is set via SpecCluster and set to ":8787" (to intentionally leave out the ip address) it would now bind to 0.0.0.0 (as prior). If the dashboard_address parameter is not set it would bind to localhost instead

[jacobtomlinson]

That seems good to me. It looks like tests are still failing on distributed/tests/test_worker.py::test_service_hosts_match_worker.

[maldag]

There a still a couple of tests failing. I can't figure out how to reproduce them locally since I don't know all different configurations. Is there someone to look at this? distributed/shuffle/tests/test_rechunk.py

[jacobtomlinson]

@fjetter @hendrikmakait would one of you be able to take a look? I think this is pretty close to getting merged.

[maldag]

So this PR is mainly blocked by this PR #8870

[hendrikmakait]

Thanks, @maldag! CI failures are unrelated and the code looks good to me. I have one small suggestion regarding the documentation.

[hendrikmakait]

Thanks, @maldag, for your first contribution! 🥳