Configurable ClientIP utility function

[AlexanderYastrebov]

Is your feature request related to a problem? Please describe.
There are several modules where client IP address is required.

Depending on the setup it can be reliably obtained from:

• http.Request.RemoteAddr attribute
• first X-Forwarded-For header (e.g. when behind a loadbalancer)
• last X-Forwarded-For header

Different modules (predicates, rate limiters, load balancing) use different strategies to obtain client IP address. E.g. there are three predicates Source, SourceFromLast and ClientIP that implement three different ways to obtain client IP.

Ratelimiters can obtain IP address from first X-Forwarded-For header (via XForwardedForLookuper) but not from last, there is also no way to obtain IP address from http.Request.RemoteAddr directly regardless of X-Forwarded-For (similar problem #1614 addresses).

ConsistentHash loadbalancer algorithm always uses first X-Forwarded-For header.

⚠️ It is important that all usecases require client IP address and the proper way to obtain it depends on the Skipper setup or environment

Describe the solution you would like
Define ClientIP utility function that uses implementation configured on startup via command flag, something like:

diff --git a/net/net.go b/net/net.go
index 5e2da71..7535b48 100644
--- a/net/net.go
+++ b/net/net.go
@@ -1,6 +1,7 @@
 package net
 
 import (
+       "fmt"
        "net"
        "net/http"
        "strings"
@@ -61,3 +62,32 @@ func RemoteHostFromLast(r *http.Request) net.IP {
 
        return parse(r.RemoteAddr)
 }
+
+// Returns remote address of the client obtained from http.Request.RemoteAddr
+func hostFromRemoteAddr(r *http.Request) net.IP {
+       h, _, _ := net.SplitHostPort(r.RemoteAddr)
+       return net.ParseIP(h)
+}
+
+var clientIPSource = hostFromRemoteAddr
+
+// Configures source used by ClientIP function to provide client IP address
+func ConfigureClientIPSource(src string) error {
+       switch src {
+       case "XFF":
+               clientIPSource = RemoteHost
+       case "XFFLast":
+               clientIPSource = RemoteHostFromLast
+       case "RemoteAddr":
+               clientIPSource = hostFromRemoteAddr
+       default:
+               return fmt.Errorf("Unsupported client IP source '%s'. Use one of XFF, XFFLast or RemoteAddr", src)
+       }
+       return nil
+}
+
+// Returns client IP address based on configured algorithm
+// Default algorithm is "RemoteAddr"
+func ClientIP(r *http.Request) net.IP {
+       return clientIPSource(r)
+}

and use it everywhere client IP address is meant.

This will ensure the proper implementation is used in all cases and there is no need to repeat algorithm selection in every module that requires client IP.

Describe alternatives you've considered (optional)
Every module supports client IP algorithm selection. When skipper is provided as infrastructure (say ingress) users have to be aware of the differences between algorithms.

Additional context (optional)

• This is a followup of the "Secure" Source predicate which can't be manipulated by the client #1614 discussion
• Instead of new ClientIP an old RemoteHost can be repurposed
• When proposed startup flag is specified
  • Source, SourceFromLast and ClientIP predicates may become aliases and all use the configured algorithm. This ensures routes are safe regardless of predicate used by users.
  • XForwardedForLookuper may use the configured implementation. Also a new ClientIPLookuper could be introduced.
  • For ALB->NLB migration there would be no need in predicate rewriting Duplicate routes and rewrite predicate #1619

Would you like to work on it?
Yes, let's discuss first

[szuecs]

It’s not possible to do it in general.
Think about behind ALB and east-west, these are 2 completely different scenarios and need different predicates to make it work as expected.

The alb->nlb case is wrong, in case we implicitly switch without creating a new skipper stack and I am more in favor of non new stack creation, because of several reasons. One example is cost, another is east west routing will not work as expected, yet another is additional complexity.

[AlexanderYastrebov]

@szuecs Look from users perspective - they don't really want to (or even can not) know in depth all the details and implications of the setup especially when skipper is provided as infrastructure. What they want is to use "client ip" and proper "client ip" definition depends on the setup. In this ticket I propose to configure proper "client ip" for all modules at once.

[szuecs]

@AlexanderYastrebov users perspective, that's why I proposed the PreProcessor!
We can later propose changes via GHE API to all projects that use P1 to switch to P2, no matter what P1 and P2 will be in the future.