Support using MX records for DNS discovery #10099
Conversation
|
I would require a port. I expect that nowadays 587 is most used. |
|
Not really, 25 is still the standard for email delivery rather than submission; so if you publish MX records you're telling people a service is available on port 25, so the default makes sense. |
|
Reminder: In a week (on 2022-01-12), I'll cut a new release (v2.33). I think this PR would be great to go into the release, so it would be good if the review could be concluded soon. |
|
How can we phrase the documentation to say that the MX port can still be overwritten? |
|
I changed it to just be quite explicit: # The port number used if the query type is not SRV. For MX set to override the
# default (25).
[ port: <int>]wdyt? |
|
I have been thinking about this and I am really not sure we should make an execption for MX records to have a default port. Just like we do not default to 80 for A records. This adds complexity and most users would use an exporter rather that something directly on port 25. From a user perspective, it is easy to add port: 25 in the config if required. |
|
I think the difference there is A doesn't imply HTTP, unlike MX which does imply SMTP. But also I don't really mind if you prefer to make port required, I suspect this is a bit niche anyway and having 25 documented in the config doesn't hurt, even if anything other than 25 is very unlikely. |
|
Yes, my preference is no default port. Note that Postfix exporter runs on 9154, and Dovecot exporter on 9166. |
discovery/dns/dns.go
Outdated
| case "A", "AAAA": | ||
| if c.Port == 0 { | ||
| return errors.New("a port is required in DNS-SD configs for all record types except SRV") | ||
| return errors.New("a port is required in DNS-SD configs for all record types except SRV and MX") |
There was a problem hiding this comment.
This change is no longer necessary
discovery/dns/dns.go
Outdated
| @@ -99,10 +101,10 @@ func (c *SDConfig) UnmarshalYAML(unmarshal func(interface{}) error) error { | |||
| return errors.New("DNS-SD config must contain at least one SRV record name") | |||
| } | |||
| switch strings.ToUpper(c.Type) { | |||
| case "SRV": | |||
| case "SRV", "MX": | |||
There was a problem hiding this comment.
This change is no longer necessary
discovery/dns/dns.go
Outdated
| @@ -129,18 +131,21 @@ func NewDiscovery(conf SDConfig, logger log.Logger) *Discovery { | |||
| } | |||
|
|
|||
| qtype := dns.TypeSRV | |||
| port := conf.Port | |||
There was a problem hiding this comment.
This change is no longer necessary
| @@ -40,6 +40,8 @@ const ( | |||
| dnsSrvRecordPrefix = model.MetaLabelPrefix + "dns_srv_record_" | |||
| dnsSrvRecordTargetLabel = dnsSrvRecordPrefix + "target" | |||
| dnsSrvRecordPortLabel = dnsSrvRecordPrefix + "port" | |||
| dnsMxRecordPrefix = model.MetaLabelPrefix + "dns_mx_record_" | |||
| dnsMxRecordTargetLabel = dnsMxRecordPrefix + "target" | |||
There was a problem hiding this comment.
What about we also expose the priority?
|
Friendly ping :) |
It's currently possible to use blackbox_exporter to probe MX records themselves. However it's not possible to do an end-to-end test, like is possible with SRV records. This makes it possible to use MX records as a source of hostnames in the same way as SRV records. Signed-off-by: David Leadbeater <[email protected]>
|
This LGTM, can you rebase? |
It's currently possible to use blackbox_exporter to probe MX records themselves. However it's not possible to do an end-to-end test, like is possible with SRV records. This makes it possible to use MX records as a source of hostnames in the same way as SRV records. Signed-off-by: David Leadbeater <[email protected]>
It's currently possible to use blackbox_exporter to probe MX records
themselves. However it's not possible to do an end-to-end test, like is
possible with SRV records. This makes it possible to use MX records as a
source of hostnames in the same way as SRV records.
Signed-off-by: David Leadbeater [email protected]