Expose the IPVS firewall mark as a label

[dsolsona]

When using IVPS with firewall marks it is interesting to know to which virtual service a backend belongs.

This PR exposes the firewall mark as a label. I've tested this in my local prometheus setup and works as expected. I haven't tested this in a non fwmark setup but the tests cover that use case.

@SuperQ @discordianfish

[SuperQ]

It would be good to have a working example in the fixtures.

[discordianfish]

Agree, in general this looks useful. Thanks! But yeah some fixtures would be great!

[dsolsona]

I've added the new fixtures to include a use case with fw marks support.

Let me know if there's anything else required.

[SuperQ]

After reading over the IPVS procfs code, and trying to understand the IPVS documentation a bit, I'm not sure this is what we need to do to handle this.

[dsolsona]

@SuperQ what do you mean exactly?

The changes to collector/fixtures/proc/net/ip_vs are based on what was added in https://github.com/prometheus/procfs/pull/48/files and also on a real example I got from our infrastructure

ipvsadm output

~# ipvsadm -L -n
IP Virtual Server version 1.2.1 (size=4096)
Prot LocalAddress:Port Scheduler Flags
  -> RemoteAddress:Port           Forward Weight ActiveConn InActConn
FWM  1 wrr
  -> 10.91.70.51:80               Tunnel  100    1          1
  -> 10.91.70.52:80               Tunnel  100    2          1
FWM  100 wrr
  -> 10.90.153.167:443            Tunnel  100    4          5
  -> 10.91.89.34:443              Tunnel  100    4          4
FWM  110 wrr
  -> 10.90.153.172:443            Tunnel  100    0          0
  -> 10.90.153.173:443            Tunnel  100    0          0

proc output

~# cat /proc/net/ip_vs
IP Virtual Server version 1.2.1 (size=4096)
Prot LocalAddress:Port Scheduler Flags
  -> RemoteAddress:Port Forward Weight ActiveConn InActConn
FWM  00000001 wrr
  -> 0A5B4634:0050      Tunnel  100    3          2
  -> 0A5B4633:0050      Tunnel  100    3          2
FWM  00000064 wrr
  -> 0A5A99A7:01BB      Tunnel  100    3          6
  -> 0A5B5922:01BB      Tunnel  100    4          4
FWM  0000006E wrr
  -> 0A5A99AD:01BB      Tunnel  100    0          0
  -> 0A5A99AC:01BB      Tunnel  100    0          0

The node_exporter built from this branch

~# curl -s localhost:9100/metrics |grep -i ipvs
node_exporter_build_info{branch="ipvs_fwmark",goversion="go1.9.2",revision="fde756d93cc8ba23b9e88be9cc3076948e7313b0",version="0.15.0"} 1
# HELP node_ipvs_backend_connections_active The current active connections by local and remote address.
# TYPE node_ipvs_backend_connections_active gauge
node_ipvs_backend_connections_active{local_address="<nil>",local_mark="00000001",local_port="0",proto="FWM",remote_address="10.91.70.51",remote_port="80"} 2
node_ipvs_backend_connections_active{local_address="<nil>",local_mark="00000001",local_port="0",proto="FWM",remote_address="10.91.70.52",remote_port="80"} 3
node_ipvs_backend_connections_active{local_address="<nil>",local_mark="00000064",local_port="0",proto="FWM",remote_address="10.90.153.167",remote_port="443"} 3
node_ipvs_backend_connections_active{local_address="<nil>",local_mark="00000064",local_port="0",proto="FWM",remote_address="10.91.89.34",remote_port="443"} 4
node_ipvs_backend_connections_active{local_address="<nil>",local_mark="0000006E",local_port="0",proto="FWM",remote_address="10.90.153.172",remote_port="443"} 0
node_ipvs_backend_connections_active{local_address="<nil>",local_mark="0000006E",local_port="0",proto="FWM",remote_address="10.90.153.173",remote_port="443"} 0

Am I missing anything?

[SuperQ]

What I'm thinking is that it doesn't seem like the local marks have any association with a local address. Maybe it would be easier to simply map the mark label into the address label?

[dsolsona]

The way I understand it is that those are two different things. In TCP/UDP you have a local address which maps to an IP:PORT combo (or just a single IP). When defining a fwmark service, the concept of local address doesn't exist, you just configure an integer to identify which fwmark to use.

It might seem a bit a weird to re-use the local_address field for something entirely different.

[SuperQ]

Yes, it's tricky. One option is we make these a new metric with different labeling.

For sure, one thing we need to fix is to map the localAddress nil to empty string.

[dsolsona]

I agree with fixing the localAddress being nil, I could try to fix this in the procfs repo.

As for the new metric, I'm not entirely sure it would make sense to have a new metric, it is in fact the same metric :), perhaps renaming the label from local_mark to just fwmark might make more sense.

[discordianfish]

Since it's still tracked as active connection, using the same metric is fine IMO. I also think it's okay to have localDress be empty for these cases, I'd say this isn't that uncommon.
Fixing the in procfs would be nice but IMO no need to block this merge.

[dsolsona]

Thanks @discordianfish!

I'm glad we can get this merged.

[discordianfish]

@SuperQ You're okay with merging this now?

[dsolsona]

🙏 I'd like to have this merged

[discordianfish]

@SuperQ We should decide what to do here. I'm happy to leave this final say in this to you but we should decide something :)

[SuperQ]

Yes, sorry, I've been distracted.

This PR still needs to fix the local_address value nil problem.

[SuperQ]

This needs to be local_address="", not <nil>

[kornrunner]

I'm willing to address this issue, but I believe procfs returns net.IP instance, and this package is invoking .String() method which returns <nil>. If it's okay I can do another PR that will mitigate this issue in current package? Thanks!

[discordianfish]

@dsolsona Can you fix this? I think then we can merge this.

[discordianfish]

@dsolsona Bump.
@SuperQ I assume you'd be okay with merging this once this is fixed, right?

[SuperQ]

Yes, I think we just need to fix up the labeling issue, and it'll be fine.

[discordianfish]

I'm going to close this for now due to lack of activity. Feel free to re-open again once y ou revive this.