Fix canonical URL when setting either port of host from the CLI

[ran-ka]

Description

Please describe your pull request.

A small bug fix to set the right canonical_url when specifying web_port or web_host using the CLI

[taybin]

Should this be if web_port is not None and web_host is not None:?
Otherwise we could end up with http://:8000 or http://myhost:

[taybin]

Also do we need to think about supporting https?

[ran-ka]

1. I don't believe we need to replace the "or" with an "and". These CLI configurations override the ones in the app configuration (look here: https://faust.readthedocs.io/en/latest/userguide/settings.html) under web_host, web_port, and canonical_url... so, I think the order of configuration is:
   a) Use the provided app configuration (web_host, web_port, and canonical_url...)
   b) If non are provided, use the default (i.e. port = 6066, host = socket.gethostname(), canonical_url= f"http://{web_host}:{web_port}"
   c) Override these values by the CLI param --web_host --web_port (and after this PR the canonical_url as well :-) )

So when the CLI configures the app, it's guaranteed that web_port and web_host have some value

You can test this by setting web_port = 1111 for the app and then start Faust:
faust worker -A main.app --web-port 8001 -l info

and verify that the port is indeed 8001 and not 1111

2. Maybe, at the moment, the use case that I am working on is table_route (i.e. routing requests between workers), so as long as all the workes are in the same subnet we are good. If you can think of a use case where workers are in different networks, external or with security risks, then I guess we need to consider HTTPS as well. What do you think?

[taybin]

That makes sense. Maybe if someone needs cross-network support they can use nginx to wrap it. Or that can be a separate PR in the future.

[ondrejchmelar]

This seems to have broken elsewhere in 6.10. We only set web_host and web_port when initializing faust.App, now it needs to be set via CLI params as well, otherwise default value is used.

[ran-ka]

Thanks, @ondrejchmelar can you please provide more details?
Unless I am missing something, this change ^^ shouldn't modify the App web_host or web_port if you are not using the worker CLI.

[ran-ka]

OK, I've looked at it a bit closer

Version 0.6.9

Looking at this line: web_host CLI config, it looks like when running the Faust worker from a command line, web_host always gets a value (either the default value or the given param)

So this code: checking if web_host is not NULL is a bit confusing.

And actually App.conf.web_host is always overwritten by the worker by this value

I've managed to replicate this in version 0.6.9:
running
faust -A main.app worker -l info (without any params)
And setting the App web_port = 80 and web_host = "localhost"
gives:
web_port = 80, web_host = "MY LOCAL MAC NAME", canincal_url = http://localhost:80

So this behaviour existed also in 0.6.9

Version 0.7.0
Didn't change this behaviour, but just fixed the canonical_url to be consistent with web_host.

running
faust -A main.app worker -l info (without any params)
And setting the App web_port = 80 and web_host = "localhost"
gives:
web_port = 80, web_host = "MY LOCAL MAC NAME", canincal_url = http://{MY LOCAL MAC NAME}:80

@ondrejchmelar does it make sense?

[ran-ka]

@ondrejchmelar can you please see if this PR resolves your issue:
PR 210

[ondrejchmelar]

@ran-ka Yep, that seems right, I would expect it to behave the same way as web-port. Thanks!