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

Add TLS, QoS and retain options to the MQTT receiver #232

Merged
merged 7 commits into from
Sep 17, 2024

Conversation

alexander-akhmetov
Copy link
Contributor

@alexander-akhmetov alexander-akhmetov commented Aug 22, 2024

Add TLS, QoS and Retain parameters to the MQTT receiver.

Grafana PR: grafana/grafana#92331

Related: grafana/grafana#16858 and #227

@alexander-akhmetov alexander-akhmetov force-pushed the alexander-akhmetov/mqtt-receiver-certs branch from bccfd21 to 454aa94 Compare August 23, 2024 16:11
@alexander-akhmetov alexander-akhmetov marked this pull request as ready for review August 23, 2024 16:18
@alexander-akhmetov alexander-akhmetov requested a review from a team as a code owner August 23, 2024 16:18
Retain bool `json:"retain,omitempty" yaml:"retain,omitempty"`
TLSCACertificate string `json:"tlsCACertificate,omitempty" yaml:"tlsCACertificate,omitempty"`
TLSClientCertificate string `json:"tlsClientCertificate,omitempty" yaml:"tlsClientCertificate,omitempty"`
TLSClientKey string `json:"tlsClientKey,omitempty" yaml:"tlsClientKey,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great if we group these fields into a struct (similar to what upstream does). This schema will let us group fields in UI as well.
There are some problems with nested secrets right now but it should be fixed in grafana/grafana#92035.

Copy link
Contributor

Choose a reason for hiding this comment

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

Later we may reuse it in other integrations such as webhook

@alexander-akhmetov alexander-akhmetov force-pushed the alexander-akhmetov/mqtt-receiver-certs branch from 8fd94d3 to 2a01a54 Compare August 29, 2024 17:34
@alexander-akhmetov alexander-akhmetov force-pushed the alexander-akhmetov/mqtt-receiver-certs branch from 15d5b7b to aa46696 Compare August 30, 2024 17:27
Copy link
Contributor

@yuri-tceretian yuri-tceretian left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@titolins titolins left a comment

Choose a reason for hiding this comment

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

Hey @alexander-akhmetov, hope I'm on time for a last comment here. I'm investigating how to inject TLS config in the webhook receiver and this could help a lot (in the scope of grafana/grafana#9548).

I could also tackle the suggested changes in a separate PR once you merge it, if you'd prefer 👍

receivers/mqtt/mqtt.go Outdated Show resolved Hide resolved
@alexander-akhmetov alexander-akhmetov force-pushed the alexander-akhmetov/mqtt-receiver-certs branch 3 times, most recently from ca50133 to 99614a5 Compare September 12, 2024 11:31
@alexander-akhmetov alexander-akhmetov force-pushed the alexander-akhmetov/mqtt-receiver-certs branch from 99614a5 to c2f1eec Compare September 12, 2024 11:32
Copy link

@titolins titolins left a comment

Choose a reason for hiding this comment

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

🚀

@alexander-akhmetov alexander-akhmetov merged commit 6c25eb6 into main Sep 17, 2024
3 checks passed
@alexander-akhmetov alexander-akhmetov deleted the alexander-akhmetov/mqtt-receiver-certs branch September 17, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants