-
Notifications
You must be signed in to change notification settings - Fork 9k
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
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]