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

feat: Add possibility to customize domain for MQTT broker #359

Merged
merged 10 commits into from
Apr 8, 2024

Conversation

ppawlowski
Copy link
Contributor

@ppawlowski ppawlowski commented Apr 3, 2024

Description

This PR adds the forge.broker.domain configuration option to allow running the MQTT broker on a custom domain. It also includes updates to the chart's schema and the documentation update.
The example of the FlowFuse Platform running with the broker on custom domain is available here.

Related Issue(s)

Closes #295

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

@ppawlowski ppawlowski requested a review from hardillb April 3, 2024 10:36
Copy link
Contributor

@hardillb hardillb left a comment

Choose a reason for hiding this comment

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

Functionally looks good, I just have the niggle with re-using domain to mean different things.

@@ -85,6 +85,7 @@ To use STMP to send email
- `forge.broker.enabled` (default `false`)
- `forge.broker.url` URL to access the broker from inside the cluster (default `mqtt://flowforge-broker.[namespace]:1883`)
- `forge.broker.public_url` URL to access the broker from outside the cluster (default `ws://mqtt.[forge.domain]`, uses `wss://` if `forge.https` is `true`)
- `forge.broker.domain` the domain the broker will be hosted on (default `mgtt.[forge.domain]`)
Copy link
Contributor

Choose a reason for hiding this comment

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

My only thought here is that we use forge.domain to mean a domain we will append hostnames to, where as here we are using forge.broker.domain to mean a fully qualified hostname.

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 see the point. How about renaming to forge.broker.subdomain as a subdomain of forge.domain ? It will be less confusing and will probably prevent questions about self-hosting MQTT broker separately from the FlowFuse platform.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm tempted to say forge.broker.hostname and label it as needing to be a FQDN (and possibility improve the forge.entrypoint doc as that is similar and very vague)

@ppawlowski ppawlowski requested a review from hardillb April 8, 2024 08:00
Copy link
Contributor

@hardillb hardillb left a comment

Choose a reason for hiding this comment

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

Looks good, just a single char typo in the docs (I will merge the fix)

helm/flowforge/README.md Outdated Show resolved Hide resolved
@hardillb hardillb merged commit 695dae2 into main Apr 8, 2024
6 checks passed
@hardillb hardillb deleted the feat-custom-broker-domain branch April 8, 2024 15:19
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.

As an OPS I would like to have a possibility to customize domain for the broker
3 participants