Add support for validating the downstream ip of the connection #108
Conversation
|
@pires can you please approve the CI workflow so we can check if the new test being added works fine? thanks! |
|
@dims for you the world, thanks for pinging. |
|
@kmala please review the coverage tests, looks like we need to make sure they don't regress. |
Update and verified locally that the coverage has increased. |
policy.go
Outdated
| @@ -43,7 +44,7 @@ const ( | |||
| // Kubernetes pods local traffic. The def is a policy to use when an upstream | |||
| // address doesn't match the skipHeaderCIDR. | |||
| func SkipProxyHeaderForCIDR(skipHeaderCIDR *net.IPNet, def Policy) PolicyFunc { | |||
| return func(upstream net.Addr) (Policy, error) { | |||
| return func(upstream net.Addr, downstream net.Addr) (Policy, error) { | |||
There was a problem hiding this comment.
This breaks the API and I'm thinking there's no need to do this. upstream is just a placeholder for any net.Addr so unless both upstream and downstream addresses have to be evaluated in a policy, I think we can just keep the API as is, maybe rename upstream to address and the new PolicyFunc just works. WDYT?
There was a problem hiding this comment.
I thought about it and the issue i faced is where this function is called https://github.com/pires/go-proxyproto/blob/main/protocol.go#L71. If we have 2 policy function one calling with the remote and other with the local address, then i felt its not straight forward on handling different policy returned when both the functions are used.
I can change it if we are okay with second function overriding if both are set.
There was a problem hiding this comment.
@pires what do you suggest as i don't see any other option apart from these?
There was a problem hiding this comment.
I'm thinking if anything, we'd split PolicyFunc into DownstreamPolicyFunc and UpstreamPolicyFunc with the same signature. My only strong opinion is not wanting to break the API.
There was a problem hiding this comment.
@emersion got any thoughts on this? 🙏🏻
There was a problem hiding this comment.
Yes, i understand that. Do we want to have one function override the other if both are present or do we want to pick a stricter policy of both(which i think is bit difficult to say which is stricter)
protocol.go
Outdated
| @@ -68,7 +68,7 @@ func (p *Listener) Accept() (net.Conn, error) { | |||
|
|
|||
| proxyHeaderPolicy := USE | |||
| if p.Policy != nil { | |||
| proxyHeaderPolicy, err = p.Policy(conn.RemoteAddr()) | |||
| proxyHeaderPolicy, err = p.Policy(conn.RemoteAddr(), conn.LocalAddr()) | |||
There was a problem hiding this comment.
I see now what the problem is. Let me think a little bit more about this. We'll find agreement.
There was a problem hiding this comment.
@pires did you get a chance to look more into this?
|
If we don't want to break the API, my take would be to add a new
|
The issue is that since the Listener is exported https://github.com/pires/go-proxyproto/blob/main/protocol.go#L25 there is no proper way to enforce that only one of the policy/connpolicy can be specified. We can just ignore Policy if ConnPolicy is specified in the code but that won't be obvious unless someone reads the code or comments. |
|
It should be possible to panic from |
|
@emersion can you please check if the changes look good? |
|
I am thinking the |
Update the field as deprecated. |
fixes #107
Had to change the PolicyFunc mentod signature as i couldn't find a better way to handle policy if we have separate functions for upstream and downstream and if both are present.