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

tedge config set mqtt.BROKERURL not provided #520

Closed
mbay-ODW opened this issue Oct 20, 2021 · 8 comments
Closed

tedge config set mqtt.BROKERURL not provided #520

mbay-ODW opened this issue Oct 20, 2021 · 8 comments
Labels
feature Change request theme:cli Theme: cli related topics theme:configuration Theme: Configuration management

Comments

@mbay-ODW
Copy link

Is your feature request related to a problem? Please describe.
We tried an multi container setup where MQTT Broker and sedge-mapper are running in two different containers. Within the mapper the MQTT Broker address can not be configured.

Describe the solution you'd like
Use mqtt.url= as a new key value pair for config.

@mbay-ODW mbay-ODW added the feature Change request label Oct 20, 2021
@didier-wenzek
Copy link
Contributor

I have a mix feeling here.

  • On one side, I understand your need and it would be a small change to the config and the mapper.
  • On the other side, this has side effects on the tedge connect command that will have to act on a remote host.
  • Somehow this parameter is only required by the mapper.

So I don't see that as a long term solution.

  • We need first to decouple thin-edge from systemd,
  • and then, to ease how thin-edge can be deployed over containers or inter-connected devices.

@mbay-ODW
Copy link
Author

The reason for the request ist that some of the customers already do have and use their own MQTT Broker. In terms of management its thus easier to use the existing one instead of needing another one just for thin-edge.

I do agree that in terms of security and e.g. credential handling some design effort is needed. However this scenario does happen out in the field.

@didier-wenzek
Copy link
Contributor

Connecting to an MQTT broker deployed independently is definitely something that should be feasible.

@mbay-ODW
Copy link
Author

mbay-ODW commented Nov 5, 2021

Is the function tedge config set mqtt.external.bind_address meant for that?

@didier-wenzek
Copy link
Contributor

The mqtt.external.bind_address is related but not what you expect.

  • This setting creates a second listener so the MQTT bus will accept both local and remote connections. However the tedge-mapper will still try to connect to the local bus.
  • In the multi-container use-case, the MQTT broker could be started with such a setting, but the mapper would require a parameter to connect its neighbor container. Something that cannot currently be done.

@mbay-ODW
Copy link
Author

mbay-ODW commented Nov 11, 2021

Could you point us to the files where this needs to be added, we would fork here probably. We searched for "mqtt.port" and did find out that these settings seems to be done in several rust files such that it was not clear to us where to add that function in detail.

@didier-wenzek:
Would it be here:

Link

mqtt_options.broker_address("IP");

Would that do the trick?

@didier-wenzek
Copy link
Contributor

If you wish to add this feature, I see two approaches.

Add a --host parameter to the tedge_mapper daemon.

  • This is done with a new field of the MapperOpt struct
  • This parameter will have to be passed to the start function of all the mappers implementation.
  • And used in the create_mapper function.

The second step might be a bit tedious.

Add a mqtt.host parameter in tedge config.

  • Add a new setting in the tedge_config along the lines of what has been done for mqtt.port.
  • Use that setting in the mqtt_config function of the mapper.

The first step impacts numerous files but is not difficult.

@reubenmiller reubenmiller added theme:configuration Theme: Configuration management theme:cli Theme: cli related topics labels Dec 1, 2022
@reubenmiller
Copy link
Contributor

Good news, this is solved by #1773. The feature has been merged into main already, and is under official verification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Change request theme:cli Theme: cli related topics theme:configuration Theme: Configuration management
Projects
None yet
Development

No branches or pull requests

3 participants