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: Team Broker #484

Merged
merged 18 commits into from
Oct 31, 2024
Merged

feat: Team Broker #484

merged 18 commits into from
Oct 31, 2024

Conversation

hardillb
Copy link
Contributor

part of FlowFuse/flowfuse#1350

Description

Add the teamBroker config.

Related Issue(s)

FlowFuse/flowfuse#1350

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

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

@hardillb hardillb self-assigned this Oct 25, 2024
@hardillb hardillb changed the title feature: Team Broker feat: Team Broker Oct 25, 2024
@@ -0,0 +1,240 @@
{{- if and ( eq .Values.forge.broker.enabled true) ( eq .Values.forge.broker.teamBroker.enabled true ) -}}
apiVersion: apps.emqx.io/v2beta1
Copy link
Contributor

Choose a reason for hiding this comment

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

With Capabilities we should check if this API exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like

{{ if .Capabilities.APIVersions.Has('apps.emqx.io/v2beta1') }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will need to add --api-versions=apps.emqx.io/v2beta1 to the template tests

}
}
dashboard {
default_password = topSecret
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be linked to the value from a Secret?

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 don't think so as it's a direct embed into the config file, the idea is that you should change it on first login.

The defaults are to not expose the dashboard that uses this password externally, but it could be accessed internally to the cluster (e.g. I am currently doing kubectl port forwarding)

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI It seems like it is possible to do it via environmental variables EMQX_DASHBOARD__DEFAULT_PASSWORD for the password and EMQX_DASHBOARD__DEFAULT_USERNAME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will need to test if we can inject individual env vars and not wipe out the ones already set on the pods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes, made, but will need to test

@ppawlowski
Copy link
Contributor

ppawlowski commented Oct 28, 2024

Can we take the introduction of a new broker as an opportunity and move broker definitions away from forge key in favour of a dedicated top-level broker key?
This kind of separation is a common approach for Helm charts which provide multiple services.

@hardillb
Copy link
Contributor Author

Can we take the introduction of a new broker as an opportunity and move broker definitions away from forge key in favour of a dedicated top-level broker key?

I'll have a look

Co-authored-by: PPawlowski <ppawlowski@users.noreply.github.com>
@hardillb
Copy link
Contributor Author

I think I also need to add an existing secret option for the emqx stuff

@hardillb
Copy link
Contributor Author

Marking this as ready for review so we can get it merged tomorrow and use it for the dedicated env.

There may be somethings we need to fix for testing (e.g. the apis thing) or things we put off to a follow up PR

@hardillb hardillb marked this pull request as ready for review October 30, 2024 16:52
@hardillb
Copy link
Contributor Author

@ppawlowski OK, this should be good, the only bit outstanding is the question about having to add --api-versions=apps.emqx.io/v2beta1 to helm template if testing with emqx chart

@ppawlowski ppawlowski merged commit 619118f into main Oct 31, 2024
6 checks passed
@ppawlowski ppawlowski deleted the emqx branch October 31, 2024 12:56
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