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

Enable easier configuration of tedge components to connect to an external mqtt broker #1773

Closed
reubenmiller opened this issue Feb 28, 2023 · 6 comments
Assignees
Labels
improvement User value theme:mqtt Theme: mqtt and mosquitto related topics
Milestone

Comments

@reubenmiller
Copy link
Contributor

reubenmiller commented Feb 28, 2023

Is your feature improvement request related to a problem? Please describe.

Configuring thin-edge.io to connect to an external MQTT broker is very difficult as the mqtt/http internal and external bind settings are currently used both for configuring the local MQTT broker and for clients connecting to the broker.

This is a common scenario when running thin-edge.io in a container environment where each thin-edge.io component is running in a separate container. For each container to connect to the MQTT broker, you need to set the same settings as you would when you are setting the mqtt broker binding settings (which is really unexpected for users).

The current settings are very confusing for the users to understand as their semantic meanings are mixed (depending on the component reading the setting).

Currently the following mqtt settings are available, and from the list it is show below:

mqtt.bind_address
mqtt.port
mqtt.external.port
mqtt.external.bind_address
mqtt.external.bind_interface
mqtt.external.capath
mqtt.external.certfile
mqtt.external.keyfile

Describe the solution you'd like

Separate the mqtt client settings from the mqtt bridge/broker settings.

For example, the c8y-log-plugin service will start an mqtt client to connect to a broker where it listens to log file requests. This setting should be independent to any mosquitto broker bridge configuration)

Add support for setting the mqtt client settings which are used by each component to tell the mqtt client which mqtt broker to connect to (host and port). The setting should be independent to any of the "binding" logic, as it enables thin-edge components to be deployed in independent containers.

The following settings should be supported.

Property Default Type Description
mqtt.client.host 127.0.0.1 string (could be a hostname or ip!) Client settings for the MQTT host to communicate with
mqtt.client.port 1883 int? Client settings for the MQTT port to communicate with

⚠️ The mqtt.client.host should be a string and not an IPAddress type so that it will also support hostnames/dns names, as an external mqtt broker is most likely going to be referenced by a dns name or hostname instead of an ip address!

The settings above would translate to the following snippet in the /etc/tedge/tedge.toml file:

[mqtt.client]
host = "127.0.0.1"
port = 1883

In terms of usage of the new mqtt client settings, the host/port should be read from the configuration, and used in the creation of each mqtt client in every service (e.g. tedge-agent, tedge-mapper-*, c8y-log-plugin, c8y-configuration-plugin etc.)

File: plugins/c8y_log_plugin/src/main.rs:90

The following shows a modified version of the code showing the usage when creating the mqtt client

let mqtt_config = mqtt_channel::Config::default()
    .with_session_name(C8Y_LOG_PLUGIN)
    .with_host(mqtt_host)
    .with_port(mqtt_port)
    .with_subscriptions(topics)
    .with_last_will_message(health_status_down_message(C8Y_LOG_PLUGIN));

Describe alternatives you've considered

Additional context

I am aware that this will technically be a breaking change, however since it was not easy to configure an external broker anyway, I don't think the impact will be large. Generally I think the new settings will be easier for users to understand and configure when wanting to connect thin-edge to an external mqtt broker.

@reubenmiller reubenmiller added improvement User value theme:mqtt Theme: mqtt and mosquitto related topics labels Feb 28, 2023
@reubenmiller reubenmiller added this to the 0.10.0 milestone Feb 28, 2023
@didier-wenzek
Copy link
Contributor

Perfect.

In terms of usage of the new mqtt client settings, the host/port should be read from the configuration, and used in the creation of each mqtt client in every service (e.g. tedge-agent, tedge-mapper-*, c8y-log-plugin, c8y-configuration-plugin etc.)

To say the same things in a different manner: The former settings mqtt.bind_address, mqtt.port and mqtt.external.* will then be used only by the tedge connect command to configure the TCP bindings of the mosquitto broker.

@Bravo555
Copy link
Contributor

Bravo555 commented Mar 1, 2023

Working on it. Btw is assigning people to issues limited to people with write access to the repo? Because I'd like to like to assign myself when starting them so it actually shows up on the board during dailies.

Bravo555 added a commit to Bravo555/thin-edge.io that referenced this issue Mar 2, 2023
As described in thin-edge#1773, config settings for addresses and port numbers
were made separate for MQTT clients and brokers, as often connect
and bind addresses are different. This change will help in scenarios
where the MQTT broker is located outside of the local network, e.g. when
using containers or when using a hostname.

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
@albinsuresh
Copy link
Contributor

Even after adding the new host/port settings for the clients, we might still need to do some normalization of the config keys to avoid confusions. The ones that I'd still label "confusing" are mqtt.port and mqtt.external.port. Since both are related to the server binding, it might be better to rename them as mqtt.bind_port and mqtt.external.bind_port keeping them consistent with mqtt.bind_address and mqtt.external.bind_address respectively.

The other confusion would be coming from the external client's perspective: What host and port should it use to connect to the broker? The mqtt.client.host and mqtt.client.port or the mqtt.external.bind_address and mqtt.external.bind_port? For external clients, we recommend using the external settings in our documentation here. That will add to additional confusion. If the mqtt.client.host and mqtt.client.port were meant to be used only by local clients running on the same thin-edge device, mqtt.local.host and mqtt.local.port would have been more appropriate, I feel. But again, which one should be used in a containerised env?

@reubenmiller
Copy link
Contributor Author

Even after adding the new host/port settings for the clients, we might still need to do some normalization of the config keys to avoid confusions. The ones that I'd still label "confusing" are mqtt.port and mqtt.external.port. Since both are related to the server binding, it might be better to rename them as mqtt.bind_port and mqtt.external.bind_port keeping them consistent with mqtt.bind_address and mqtt.external.bind_address respectively.

The other confusion would be coming from the external client's perspective: What host and port should it use to connect to the broker? The mqtt.client.host and mqtt.client.port or the mqtt.external.bind_address and mqtt.external.bind_port? For external clients, we recommend using the external settings in our documentation here. That will add to additional confusion. If the mqtt.client.host and mqtt.client.port were meant to be used only by local clients running on the same thin-edge device, mqtt.local.host and mqtt.local.port would have been more appropriate, I feel. But again, which one should be used in a containerised env?

Yes there will be some cleanup required to sort out the binding settings. However the mqtt.client.* settings are intended to not care about having a local or external host as it does not matter from the mqtt client's perspective. The user can freely configure where the mqtt clients should connect to (this works nicely for both local and external mqtt brokers). The mqtt client will be using a default value of 127.0.0.1, so when all of the thin-edge components are running on the same environment, then everything will just work, however it allows us to freely configure the mqtt client settings to work when all the components are not running on the same environment (e.g. in a multi container setup).

The mqtt binding settings are really only required to setup the mqtt broker, so the tedge binary is really the only one that needs access to the bind settings.

Bravo555 added a commit to Bravo555/thin-edge.io that referenced this issue Mar 6, 2023
As described in thin-edge#1773, config settings for addresses and port numbers
were made separate for MQTT clients and brokers, as often connect
and bind addresses are different. This change will help in scenarios
where the MQTT broker is located outside of the local network, e.g. when
using containers or when using a hostname.

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
Bravo555 added a commit to Bravo555/thin-edge.io that referenced this issue Mar 6, 2023
As described in thin-edge#1773, config settings for addresses and port numbers
were made separate for MQTT clients and brokers, as often connect
and bind addresses are different. This change will help in scenarios
where the MQTT broker is located outside of the local network, e.g. when
using containers or when using a hostname.

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
reubenmiller pushed a commit that referenced this issue Mar 6, 2023
* Added impl TryFrom<&str> for IpAddress

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

* Add MQTT client address and port config options

As described in #1773, config settings for addresses and port numbers
were made separate for MQTT clients and brokers, as often connect
and bind addresses are different. This change will help in scenarios
where the MQTT broker is located outside of the local network, e.g. when
using containers or when using a hostname.

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

* Make c8y plugins use client address/port settings

c8y_configuration_plugin, c8y_firmware_plugin, and c8y_log_plugin
didnt's use any MQTT address/host config settings, but were hardcoded to
default to localhost. Now they use MqttClientAddressSetting and
MqttClientPortSetting settings.

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

---------

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
@reubenmiller reubenmiller assigned reubenmiller and unassigned Bravo555 Mar 7, 2023
@reubenmiller
Copy link
Contributor Author

We need to investigate how best to write a test to cover this as it requires configuring an external mqtt broker.

albinsuresh pushed a commit to albinsuresh/thin-edge.io that referenced this issue Mar 13, 2023
…edge#1789)

* Added impl TryFrom<&str> for IpAddress

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

* Add MQTT client address and port config options

As described in thin-edge#1773, config settings for addresses and port numbers
were made separate for MQTT clients and brokers, as often connect
and bind addresses are different. This change will help in scenarios
where the MQTT broker is located outside of the local network, e.g. when
using containers or when using a hostname.

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

* Make c8y plugins use client address/port settings

c8y_configuration_plugin, c8y_firmware_plugin, and c8y_log_plugin
didnt's use any MQTT address/host config settings, but were hardcoded to
default to localhost. Now they use MqttClientAddressSetting and
MqttClientPortSetting settings.

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

---------

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
@reubenmiller
Copy link
Contributor Author

Integration test coverage by #1848

github-actions bot referenced this issue Aug 6, 2023
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
github-actions bot referenced this issue Aug 8, 2023
chore: add maintainer scripts from existing debian packages for later comparison
github-actions bot referenced this issue Aug 10, 2023
github-actions bot referenced this issue Sep 4, 2023
tedgeUrl is now using the device name instead of "main" for the main
device. This change is made on the PR #2108.

Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
Co-authored-by: Didier Wenzek <didier.wenzek@free.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement User value theme:mqtt Theme: mqtt and mosquitto related topics
Projects
None yet
Development

No branches or pull requests

4 participants