search

TheThingsNetwork / lorawan-stack

The Things Stack, an Open Source LoRaWAN Network Server
https://www.thethingsindustries.com/stack/
Apache License 2.0
975 stars 306 forks source link

Report gateway IP address in gs.gateway.connect event #5491

Closed furtiman closed 2 years ago

furtiman commented 2 years ago

Summary

Add gateway IP address to gs.gateway.connect event. Can be reliably determined for UDP, Basic Station and gRPC gateways but not for MQTT gateways.

Why do we need this?

To be able to use / display IP of the gateway to users.

What is already there? What do you see now?

evtGatewayConnect not containing IP address

What is missing? What do you want to see?

IP address added to payload of the event.

Environment

3.19.2

How do you propose to implement this?

Add extra payload and change event data type in gs.gateway.connect event (pkg/gatewayserver/observability)

How do you propose to test this?

UT?

Can you do this yourself and submit a Pull Request?

cc @johanstokking will put you for now

htdvisser commented 2 years ago

I think we can try adding the gateway IP address to the GatewayConnectionStats message.

The only question is if/how this is going to work when The Things Stack is behind a load balancer / proxy. Then we can't reliably determine the original IP address (unless we add PROXY protocol support).

KrishnaIyer commented 2 years ago

The only question is if/how this is going to work when The Things Stack is behind a load balancer / proxy. Then we can't reliably determine the original IP address (unless we add PROXY protocol support).

Indeed and additionally, if gateways are using a VPN then this won't work either. This is also one of the reasons we decided not to use IP addresses for location data.

I believe that this should be a sub-component of the broader gateway management issue; https://github.com/TheThingsIndustries/lorawan-stack/issues/2694

johanstokking commented 2 years ago

@htdvisser I think we can try adding the gateway IP address to the GatewayConnectionStats message.

Yes, for it to be requested from GS, for the Console. We don't have to put the IP address in every stats update (to store or events to NOC); only on connect. We could add a initial GatewayConnectionStats message as event payload to gs.gateway.connect (currently there's no payload), and include the IP address there.

@htdvisser The only question is if/how this is going to work when The Things Stack is behind a load balancer / proxy. Then we can't reliably determine the original IP address (unless we add PROXY protocol support).

It becomes a bit deployment specific, but in case of UDP we already have the source IP address (even if AWS NLB is in between). In case of Basic Station and MQTT, indeed we'd need Proxy Protocol support. But I would argue that having the source IP address is useful anyhow.

@KrishnaIyer I believe that this should be a sub-component of the broader gateway management issue; TheThingsIndustries/lorawan-stack#2694

How so? The use case here is to show the gateway's IP address in the NOC and potentially Console.

htdvisser commented 2 years ago

It becomes a bit deployment specific, but in case of UDP we already have the source IP address (even if AWS NLB is in between). In case of Basic Station and MQTT, indeed we'd need Proxy Protocol support.

Indeed, in our AWS ECS deployments we should be able to determine the source IP address, also for MQTT, which uses pretty much the same AWS NLB settings as UDP. For Basic Station, Envoy should also get the correct source IP from AWS NLB, and add an X-Forwarded-For header that we can read in The Things Stack.

The problem is mostly with other (unknown) proxy configurations, but I suppose we can just get away with saying that this feature only works reliably in officially supported deployment models.

johanstokking commented 2 years ago

The problem is mostly with other (unknown) proxy configurations, but I suppose we can just get away with saying that this feature only works reliably in officially supported deployment models.

Yep. Including not using a load balancer, which isn't really needed with TTSOS anyway.

So we the origin IP already for UDP, Basic Station, GRPC and MQTT. The only thing is that the frontends need to provide the IP address nicely, probably with a new context key.

johanstokking commented 2 years ago

@KrishnaIyer please assign a version milestone or we discuss in next weekly call.

KrishnaIyer commented 2 years ago

I've added v3.20.2 taking holidays into account.

johanstokking commented 2 years ago

Cool. Implementation proposal:

  1. Define new context value or just net.Addr
  2. Let the I/O frontends set the value on the context before calling Connect() (just like rights are set in certain frontends)
  3. The frontends use various ways to get the IP address:
    • UDP: let connect() (of pkg/gatewayserver/io/udp.(*srv)) take the *net.UDPAddr so it can put that in a derived context before calling Connect
    • MQTT: let Connect() (of pkg/gatewayserver/io/mqtt.(*connection)) put the RemoteAddr and Transport from the given auth info in a new struct that implements net.Addr
    • gRPC: get the Peer
    • Websockets: use the remote addr and override with proxy headers. It looks like the proxy middleware already sets X-Real-IP to the desired value
  4. Declare event data for gs.gateway.connect (also make sure to declare the event data type in the event definition) with a type that includes the address and transport strings. These come straight from net.Addr.
  5. Add the address and transport to the GatewayConnectionStats so they get stored by GS and they can be retrieved by the Console etc. This value needs to be stored just once as it doesn't change. It therefore doesn't need to be in the new gs.gateway.connection.stats event either.
KrishnaIyer commented 2 years ago

Proposal looks good to me. However, I think that adding the new struct (or net.Addr) as an argument to Connect seems better to me instead of passing it in the context.