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

broker: Fix parsing of listener configuration #72

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sjlongland
Copy link
Contributor

  • Better validate the bind address to both support IPv6 interface
    addresses and to ensure the address is syntactically valid.
  • Use enumerations to describe the type of listener.

@sjlongland
Copy link
Contributor Author

Noted omission in the tests is anything involving SSL: this is because to do so, I'd need to create a dummy CA and certificates for test purposes.

Copy link
Member

@FlorianLudwig FlorianLudwig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the direction this is taking, thanks!

amqtt/broker.py Outdated Show resolved Hide resolved
amqtt/broker.py Outdated Show resolved Hide resolved
assert lc.address == "::"
assert lc.port == 1883


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a test with an adress but no port is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, I'll add one.

"""
Test we can decode a raw port number in 'bind'
"""
lc = ListenerConfig(ListenerType.TCP, bind="1883")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would assume this is an error. As a bind without a colon would be the ip not adress? I thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered the IP address as being optional but the port number mandatory. If the address is omitted, it follows the standard Python network library convention of meaning "bind to all interfaces" (like :: would, but IPv6 and IPv4).

I included it as a short-hand for having to write [::]:1883 or 0.0.0.0:1883. :1883 syntax is kept though as I figured there may be people that still rely on that syntax as a "bind all" in existing configurations.

sjlongland added a commit to sjlongland/amqtt that referenced this pull request Aug 17, 2021
sjlongland added a commit to sjlongland/amqtt that referenced this pull request Aug 17, 2021
sjlongland added a commit to sjlongland/amqtt that referenced this pull request Aug 17, 2021
Former as per suggestion
Yakifo#72 (comment)

Latter because I'm not sure how the `asyncio.create_server` and similar
libraries react to being given a host name.  (Maybe it does a reverse
DNS look-up, maybe not… do users really want to trust that reverse DNS
gets the right IP?)
@sjlongland
Copy link
Contributor Author

sjlongland commented Aug 17, 2021

Run $HOME/.poetry/bin/poetry run flake8 tests/ amqtt/ --count --statistics --show-source --ignore=E501,W503,W605,E722
amqtt/broker.py:15:1: F811 redefinition of unused 'Enum' from line 5
from enum import Enum
^
1     F811 redefinition of unused 'Enum' from line 5
1
Error: Process completed with exit code 1.

That error confuses me… line 15 is blank?

…and Enum appears on exactly two lines:

So no idea where it's getting that from.

Update… Github Actions didn't tell me this was after a merge…

sjlongland added a commit to sjlongland/amqtt that referenced this pull request Aug 17, 2021
sjlongland added a commit to sjlongland/amqtt that referenced this pull request Aug 17, 2021
sjlongland added a commit to sjlongland/amqtt that referenced this pull request Aug 17, 2021
Former as per suggestion
Yakifo#72 (comment)

Latter because I'm not sure how the `asyncio.create_server` and similar
libraries react to being given a host name.  (Maybe it does a reverse
DNS look-up, maybe not… do users really want to trust that reverse DNS
gets the right IP?)
@sjlongland
Copy link
Contributor Author

Food for thought related to this: in my application I'm using aiohttp as the HTTP server for the main application providing a REST API. I then expose real-time updates via MQTT over websockets.

aiohttp supports creation and hosting of websockets. Right now for the user to access MQTT I have three choices:

  • bind amqtt server to a separate port, expose that via the firewall and have my application direct the user to the new port (fine for development, not sure about this for production use though)
  • bind amqtt server to a separate private port, configure my application's front-end reverse proxy (nginx) to also forward that second port to some URI namespace.
  • bind amqtt server to a separate private port, configure some sort of reverse proxy within aiohttp to forward the MQTT/websocket traffic to amqtt.

I'm wondering if that ListenerType enumeration shouldn't have a plugin option. This would require the user write a plug-in that implements the same interface as the tcp and ws listener types. Thus, in my application, I could implement a plug-in that creates a websocket using aiohttp calls and binds it to a path in my server's namespace. I'm thinking in this situation, things like the bind option become optional since bind makes no sense when you're not actually binding to a network socket directly.

Anyway, probably this is in the scope of a second related pull-request.

@FlorianLudwig
Copy link
Member

Anyway, probably this is in the scope of a second related pull-request.

I've made a ticket out of your post here: #73 so we can discuss the architecture there

The former emits the stack trace, which greatly helps in identifying
_WHERE_ an exception is raised, not just WHAT the exception is.
@sjlongland
Copy link
Contributor Author

I'm revisiting old issues… and spotted this one.

Things have moved ahead a lot in the master branch (now called main), rebasing is a bit of a mess. So I've scaled this PR back a bit, focussed largely on the grievance I had at the time: IPv6 literals weren't parsed correctly.

I've shelved the other changes, maybe if there's interest we can cherry-pick them back in. This change set defines a function that abstracts the splitting of IP/port, handling IP addresses (v4 and v6), host names, and blank interface names; and decodes the port number to an integer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants