Using gobgp_exporter with a dns address

[kurojishi]

Hi,
seems like the validation for hostnames it's not happy with not using ip addresses.

scrubbing-engine-gobgp-exporter | ts=2023-12-22T14:29:17.620Z caller=main.go:218 level=error msg="failed to init properly" error="invalid IP address in gobgp:50051"

while trying to use the dns:// like in https://github.com/greenpau/gobgp_exporter/blob/main/pkg/gobgp_exporter/router_node.go#L96 prefix returns another error

scrubbing-engine-gobgp-exporter  | ts=2023-12-22T14:38:11.123Z caller=main.go:218 level=error msg="failed to init properly" error="address dns://gobgp:50051: too many colons in address"

Now this logic seems a bit weird to me and i want to make sure this is intended before sending out a patch especially cause the test have a lot of bad cases, for rejection but none of the good ones for ensuring that positive cases don't get rejected

https://github.com/greenpau/gobgp_exporter/blob/main/pkg/gobgp_exporter/gobgp_exporter_test.go#L35

and adding in the {address: "dns://localhost:50051", ok: true}, in the list does get it rejected.

But if i read this correctly is rejecting anything that is not an IP address

SplitHostPort tries to split in host/port but any colon before the port address will throw the too many colons error (https://cs.opensource.google/go/go/+/refs/tags/go1.21.5:src/net/ipsock.go;l=164)

so the dns:// that is saying it's valid will always throw an error, and if there's any host it will only trigger the first branch of the if/then/else statament so it's impossible for a dns name to pass.

	host, strport, err := net.SplitHostPort(s)
	if err != nil {
		return err
	} else if host != "" {
		if addr := net.ParseIP(host); addr == nil {
			return fmt.Errorf("invalid IP address in %s", s)
		}
	} else if !strings.HasPrefix(s, "dns://") {
		return fmt.Errorf("invalid address format in %s", s)
	} else {
		// "dns://" prefix for hostname is allowed per go grpc documentation
		// see https://pkg.go.dev/google.golang.org/grpc#DialContext
		idx := strings.LastIndex(s, ":")
		host = s[0:idx]
		strport = s[idx+1:]
	}

Now what was your intention for valid endpoints for gobgp? i think it make sense to give the option to give a full fledged URI and then validate, i can send the patch if that's the case and i can improve the validation method.

[greenpau]

Now what was your intention for valid endpoints for gobgp? i think it make sense to give the option to give a full fledged URI and then validate, i can send the patch if that's the case and i can improve the validation method.

@kurojishi , it has been a while ... I don't remember the reasons. If you feel it needs to be modified, I am all ears.

cc: @mrueg , @icedream , @vj396

[icedream]

On our end, we don't use hostnames or dns: for specifying an address at all. I can attempt to derive the intention from the code.

My interpretation is the code is supposed to be compatible with the common dns: address input that the gRPC dialing routine also accepts, as well as simple IP:port. In its current state...

• for input without a scheme, it only works with IPs because net.ParseIP is explicitly called on the hostname. That's probably intentional.
• the dns: scheme check would indeed never properly trigger as the err check takes priority.
• other schemes that the gRPC document defines are not considered at all and would fail, e.g. the explicit ipv4:/ipv6:/unix socket syntax.

It would make sense to first check for dns: (or really any scheme the gRPC document defines, maybe with net/url package or check for substring :/) before falling back to parsing as IP:port. Whether we also want to add our own DNS resolving to that separately, or whether we want to simply forward the address string to gRPC dialing if it contains any scheme, I leave that open to discussion. I personally think it's not needed since gRPC already provides a framework for resolving addresses like that, it just needs documentation once the dns: scheme check is fixed up.

[kurojishi]

Sorry for the delays, holidays and whatsnots

Imho instead of doing a lot of this manually using

https://pkg.go.dev/net/url#ParseRequestURI

then checking on the parts is more effective

objections?

[kurojishi]

#30 here it is, a bit of a different behavior, but it tested it now and works properly on docker

[kurojishi]

High, anyone can give a check to the related MR?