Support RFC 3430

[SuperQ]

While it's not common, some devices do support RFC 3430, SNMP over TCP. The upstream gosnmp library supports this, so it should be an easy thing to add.

[Andy1616]

++ for that feature request

[hhromic]

Today I realised that we might need this feature and I am willing to implement it, if there is still interest for it.

Indeed the gosnmp library supports setting tcp for Transport here:
https://github.com/gosnmp/gosnmp/blob/33df32e33fbd043727a4a117a0ef6f2183607113/gosnmp.go#L57-L58

Given that this is pretty much settable together with Target and Port, I propose to add a new optional request query parameter transport to the /snmp scrape endpoint that, when provided, allows to set the underlying Transport attribute in gosnmp. If not provided, then Transport would be left untouched and will default to the current udp default. Therefore, making it 100% backwards compatible. What do you think of this implementation proposal?

[SuperQ]

Not something for a query parameter, but should be part of the auths.

[hhromic]

Hi @SuperQ , thanks for replying!
I'm not very familiar yet with the upcoming auth split, however from what I saw in the migration guide, I'm not sure how the connection transport is related to authentication configuration?

Transport (and host:port target configuration) should not be part of any static auth configuration as the same device can be configured (or re-configured) for both udp/tcp.

In my humble opinion, transport should be actually part of target, for example in the form of a scheme. For example:

• tcp://host:port
• udp://host:port
• host:port (defaults to UDP as of today, not valid URL but for backwards compatibility).

For example in Net SNMP (manpage - Agent Specification section), the syntax is [<transport-specifier>:]<transport-address>.

Or in Keepalived (manpage - snmp_socket keyword), the syntax is a similar [transport:]host[:port].

Perhaps the SNMP Exporter should also use [<transport-specifier>:]<transport-address> ?
This would even make the SNMP Exporter future-proof for other transports such as Unix sockets.

[SuperQ]

The host:port is already set via the target param. We currently default the port to 161 in case there is no port specified in the target. See collector.go.

I would be in favor of URL handling here, since we can do simple operations like:

tcpTarget, tcpTargetFound := strings.CutPrefix("tcp://", target)
udpTarget, udpTargetFound := strings.CutPrefix("udp://", target)

Might be worth breaking out the target string parsing into a function that returns (transport string, target string, port int) or something like that.

[hhromic]

Yes, I'm aware of the target parsing on the collector.go file, that's why I later suggested using URL parsing.
Great that we are on the same page then with that part 👍.

This brings me another follow-up design question. Instead of hard-coding specific transports (udp/tcp), the gosnmp library can actually work with a wider range of transports than only udp/tcp. In fact, another very common transport are Unix sockets.

From: https://github.com/gosnmp/gosnmp/blob/33df32e33fbd043727a4a117a0ef6f2183607113/gosnmp.go#L285-L290

// https://golang.org/pkg/net/#Dial gives acceptable network values as:
//   "tcp", "tcp4" (IPv4-only), "tcp6" (IPv6-only), "udp", "udp4" (IPv4-only),"udp6" (IPv6-only), "ip",
//   "ip4" (IPv4-only), "ip6" (IPv6-only), "unix", "unixgram" and "unixpacket"

If we do proper URL parsing, we can then pass any scheme provided in target down and delegate transport validation to gosnmp. This would allow the SNMP exporter to support any transports that gosnmp supports transparently.

Might be worth breaking out the target string parsing into a function that returns (transport string, target string, port int) or something like that.

I fully agree with that idea and considering the above, I suggest on a function like this:

func parseTarget(target string) (*url.URL, error) {
    // The ugly part here is backwards compatibility.
    // If "target" does not have a scheme:// then we should add a default udp:// one to "target" before parsing.
    // This would require a regex prefix matching if we go the "generalized" route or
    // use simple rule-matching for all the supported schemes.
    return url.ParseRequestURI(target)
}

Then the collector can easily extract the parts or error out if target is invalid:

u, err := parseTarget(target)

transport := u.Scheme
host := u.Hostname()
port := u:Port()

What do you think?

[SuperQ]

Yea, that kind of complexity is why I would prefer to have it be part of the config.

In our new config system we can do something like this:

auths:
  tcp_public_v2:
    community: public
    version: 2
    transport: tcp

We can then pass in all of the transport options directly.

But, we could also go with strings.SplitN(target, "://", 1) and pass whatever prefix string to Transport.

[hhromic]

I really don't think is a good idea to mix dynamic transport configuration with static authentication configs.
The generator would have to provide combinations of udp/tcp configurations (and future ones) for every auth type.

But, we could also go with strings.SplitN(target, "://", 1) and pass whatever prefix string to Transport.

Yeah maybe that is a good idea indeed, to not assume a "general URL" and avoid formal URL parsing which can get tricky.

Basically the contract for the target parameter would be [transport://]host[:port] and nothing else.
This would indeed simplify the parsing (no regex matchings) and is easy to validate.

If you are okay with that, I am happy to implement this.

[SuperQ]

Sounds like a good plan to me.