allow specification of source address for probes (icmp, tcp, dns) #262

thz · 2017-11-16T00:19:00Z

For IPv4 raw socket is used and the source IP address is set in the IP header.
For IPv6 a control message (ancillary data / RFC3542) is passed for specifying
the address.

thz · 2017-11-16T00:21:54Z

Addresses the ICMP part of #3.

thz · 2017-11-16T00:42:22Z

Hmm. Check failure seems to be unrelated: *** Test killed with quit: ran too long (10m0s). while TCP testing.

brian-brazil · 2017-11-16T09:10:32Z

CONFIGURATION.md

 [ payload_size: <int> ]

+# The source IP address.
+[ source_address: <string> ]


Would it be possible to support the interface name too? If there's DHCP involved you may not know the IP.

Should be possible. Resolve the interface name to interface index first from net.InterfaceByName(), then set IfIndex in the ipv6.ControlMessage struct.

@brian-brazil how about a source_iface attribute in another PR?

brian-brazil · 2017-11-16T09:11:48Z

config/config.go

 	PreferredIPProtocol string `yaml:"preferred_ip_protocol,omitempty"` // Defaults to "ip6".
 	PayloadSize         int    `yaml:"payload_size,omitempty"`
 	DontFragment        bool   `yaml:"dont_fragment,omitempty"`
+	SourceAddress       string `yaml:"source_address,omitempty"`


This should apply to all probes

@brian-brazil Okay... Added similar code for tcp, http, dns and moved the config to the module.

thz · 2018-01-06T00:41:38Z

Now also addresses the TCP part of #3.

brian-brazil · 2018-01-08T12:54:53Z

example.yml

        - expect: "^:[^ ]+ 001"
  icmp_example:
    prober: icmp
+    source_address: "127.0.0.1"


it's odd that this is in the probe but preferred_ip_protocol is in the module

True. But your "This should apply to all probes" made me agree and also implement source-address specification for the other probers. Is it possible (from a configuration compatibility perspective) to pull preferred_ip_protocol up to this level as well?

That'd be a breaking change, so best to keep it consistent.

So you propose to move it back into the prober-specific configuration?

brian-brazil · 2018-01-08T12:55:40Z

prober/http.go

+	if ip.IP.To4() == nil {
+		sourceNetwork = "ip6"
+	}
+	// Resolve local bind address


Comments should end with a full stop

brian-brazil · 2018-01-08T12:56:25Z

prober/http.go

+		sourceNetwork = "ip6"
+	}
+	// Resolve local bind address
+	saddr, err := net.ResolveIPAddr(sourceNetwork, module.SourceAddress)


What if there is no source address configured?

Condition is indeed missing here. Will update soon.

brian-brazil · 2018-01-08T12:57:19Z

prober/http.go

+// stage because of many possible RoundTripper wrappings.
+// Adding this to the prometheus config is an alternative
+// to this wrapper.
+func NewHTTPClientWithLocalAddr(cfg *pconfig.HTTPClientConfig, laddr *net.TCPAddr) (*http.Client, error) {


This is duplicating code with common, it'd be better to extend it. Though that code is just about to get redone.

So how should we proceed? Skipping source-address config for http prober and contribute to that common code redoings? Http prober can be made source-address-capable once that work is done.

Waiting for that work to complete is probably best.

brian-brazil · 2018-01-08T12:58:29Z

prober/tcp.go

 	}
+
+	if len(module.SourceAddress) > 0 {
+		saddr, err := net.ResolveIPAddr(sourceNetwork, module.SourceAddress)


Why do you resolve this here and not elsewhere? I don't think there's any need to resolve it.

When module.SourceAddress is an IP-Address already no resolving will happen. In that case net.ResolveIPAddr will just convert to an net.IPAddr. I did not want to prevent names being used as module.SourceAddress so resolving would be necessary.
As to "why here not elsewhere". dialTcp() is the place where the target is resolved as well (chooseProtocol) and resolving in the dial function seems to be aligned with golang dialers in general. Which place do you think would be better suited for resolving?

I'm saying that in some probes you support dns names and in others you don't. I'm suggesting not supporting it anywhere.

Ahh! Actually resolving is in for DNS, TCP and HTTP. Only ICMP (initial PR) is using net.ParseIp.
The resolving also solves the question of using IPv4 or IPv6 which also depends on the target (You need a local v6 address for a v6 target but a v4 address for a v4 taget).
A DNS name could provide v4 and v6 address. Going with IP-addresses only in configuration would require
us to provide both (v4/v6). That is why I concluded that using a resolve-step to be a good idea.
When only an IP address is specified in config there is no resolving happening but only IP parsing.

Relying on DNS to handle v4 vs v6 is a bit odd to me. I'd expect the user to provide only the address type that they need. Users requiring this are going to have a good idea of how networking works.

thz · 2018-01-21T20:01:32Z

@brian-brazil please have a look.
Source-IP-address resolving is removed. HTTP probe changes reverted (delayed for common code restructure and support).
I have kept the commits for review, and would squash after feedback.

brian-brazil · 2018-01-22T12:54:10Z

prober/dns.go

+	// Use configured SourceIPAddress.
+	if len(module.DNS.SourceIPAddress) > 0 {
+		if srcip := net.ParseIP(module.DNS.SourceIPAddress); srcip == nil {
+			level.Warn(logger).Log("msg", "Error parsing source ip address", "srcip", module.DNS.SourceIPAddress)


This should fail the probe

Changed to Error and failure in all probes.

brian-brazil · 2018-01-22T12:54:35Z

prober/dns.go

+
+	// Use configured SourceIPAddress.
+	if len(module.DNS.SourceIPAddress) > 0 {
+		if srcip := net.ParseIP(module.DNS.SourceIPAddress); srcip == nil {


srcIP would be clearer

brian-brazil · 2018-01-22T12:56:55Z

prober/icmp.go

-		level.Warn(logger).Log("msg", "Error writing to socket", "err", err)
-		return
+	if socket6 != nil && len(module.ICMP.SourceIPAddress) > 0 {
+		// Set source address for IPv6.


Why are you handling this in a different place than for v4?

Checking for len() > 0 and parse errors is now in common code.
The actual control message is v6 specific.

thz · 2018-01-22T22:56:27Z

@brian-brazil, last review feedback is addressed. Commits are squashed. Please have a look.

brian-brazil · 2018-01-23T12:11:06Z

prober/icmp.go

+		s, err := net.ListenPacket("ip4:icmp", "0.0.0.0")
+		if err != nil {
+			level.Error(logger).Log("msg", "Error listening to socket:", "err", err)
+			return


Explicitly returning false would be clearer.

I agree. That is not my code. I just moved it. I am fine with doing a cleanup commit but would separate it from this PR.

brian-brazil · 2018-01-25T13:23:05Z

prober/icmp.go

+			level.Error(logger).Log("msg", "Error parsing source ip address", "srcIP", module.ICMP.SourceIPAddress)
+			return false
+		}
+		level.Info(logger).Log("msg", "Using source address:", "srcIP", srcIP)


There shouldn't be colons in the msg

thz · 2018-01-25T14:43:34Z

@brian-brazil : removed.

brian-brazil · 2018-01-26T09:34:51Z

Are you sure you pushed your change? I'm still seeing 2 of those.

This adds the configuration option source_ip_address to modules ICMP, TCP and DNS which changes the probe's local address.

thz · 2018-01-26T10:56:19Z

@brian-brazil removed two more : and pushed.

brian-brazil · 2018-01-26T11:51:33Z

Thanks!

This is a logical continuation of prometheus#262.

This is a logical continuation of prometheus#262. Signed-off-by: Ivan Babrou <[email protected]>

bobrik · 2021-03-17T01:51:27Z

I opened #765 for http probe support.

This is a logical continuation of prometheus#262. Signed-off-by: Ivan Babrou <[email protected]>

brian-brazil reviewed Nov 16, 2017

View reviewed changes

thz force-pushed the icmp-src-addr branch from de67283 to 5a29d9b Compare January 6, 2018 00:28

thz changed the title ~~prober/icmp: allow specification of source address~~ allow specification of source address for probes (icmp, tcp, http, dns) Jan 6, 2018

brian-brazil reviewed Jan 8, 2018

View reviewed changes

thz changed the title ~~allow specification of source address for probes (icmp, tcp, http, dns)~~ allow specification of source address for probes (icmp, tcp, dns) Jan 21, 2018

thz force-pushed the icmp-src-addr branch from 84bf845 to 62aeefb Compare January 21, 2018 19:29

brian-brazil reviewed Jan 22, 2018

View reviewed changes

update vendored miekg/dns for Client.Dialer

120c04f

thz force-pushed the icmp-src-addr branch from 62aeefb to 714d253 Compare January 22, 2018 22:42

brian-brazil reviewed Jan 23, 2018

View reviewed changes

brian-brazil reviewed Jan 25, 2018

View reviewed changes

thz force-pushed the icmp-src-addr branch from 714d253 to a29fecb Compare January 25, 2018 13:51

[probers] Allow configuration of source addresses.

d1b06fc

This adds the configuration option source_ip_address to modules ICMP, TCP and DNS which changes the probe's local address.

thz force-pushed the icmp-src-addr branch from a29fecb to d1b06fc Compare January 26, 2018 10:32

brian-brazil merged commit 469a0c8 into prometheus:master Jan 26, 2018

bobrik added a commit to bobrik/blackbox_exporter that referenced this pull request Mar 17, 2021

Add source_ip_address support to http probes

82bdc7e

This is a logical continuation of prometheus#262.

bobrik added a commit to bobrik/blackbox_exporter that referenced this pull request Mar 17, 2021

Add source_ip_address support to http probes

92a35ea

This is a logical continuation of prometheus#262. Signed-off-by: Ivan Babrou <[email protected]>

bobrik mentioned this pull request Mar 17, 2021

Add source_ip_address support to http probes #765

Closed

bobrik added a commit to bobrik/blackbox_exporter that referenced this pull request Mar 17, 2021

Add source_ip_address support to http probes

a9aea9c

This is a logical continuation of prometheus#262. Signed-off-by: Ivan Babrou <[email protected]>

bobrik mentioned this pull request Mar 17, 2021

Add source_ip_address support to http probes #766

Closed

bobrik added a commit to bobrik/blackbox_exporter that referenced this pull request Mar 18, 2021

Add source_ip_address support to http probes

0dce89e

This is a logical continuation of prometheus#262. Signed-off-by: Ivan Babrou <[email protected]>

bobrik added a commit to bobrik/blackbox_exporter that referenced this pull request Mar 18, 2021

Add source_ip_address support to http probes

4e9ff53

This is a logical continuation of prometheus#262. Signed-off-by: Ivan Babrou <[email protected]>

This was referenced Jul 7, 2023

Support source IP interface for ICMP probe #775

Closed

allow specification of source address for probes (http, grpc) #1096

Open

allow specification of source address for probes (http, grpc) #1097

Open

allow specification of source address for probes (icmp, tcp, dns) #262

allow specification of source address for probes (icmp, tcp, dns) #262

Uh oh!

Conversation

thz commented Nov 16, 2017

Uh oh!

thz commented Nov 16, 2017

Uh oh!

thz commented Nov 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thz commented Jan 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thz commented Jan 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thz commented Jan 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thz commented Jan 25, 2018

Uh oh!

brian-brazil commented Jan 26, 2018

Uh oh!