Closed kmala closed 2 months ago
@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.
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.
If we don't want to break the API, my take would be to add a new ConnPolicy
callback:
Policy
and ConnPolicy
can be specified, not both, to avoid having to pick between the two.ConnPolicy
takes the net.Conn
so that it can fetch any details it needs about the connection. Alternatively, it takes a ConnPolicyOptions
struct which carry the IP addresses and can be extended later with more fields.Only one of Policy and ConnPolicy can be specified, not both, to avoid having to pick between the two.
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 Accept
if the library user provides both. Not ideal, but still better than silently ignoring one of the callbacks IMHO.
@emersion can you please check if the changes look good?
I am thinking the ConnPolicy
should become the new API, since it seems to encapsulate what Policy
is doing today, and we'd mark Policy
as deprecated and to be removed in a future release. WDYT?
I am thinking the
ConnPolicy
should become the new API, since it seems to encapsulate whatPolicy
is doing today, and we'd markPolicy
as deprecated and to be removed in a future release. WDYT?
Update the field as deprecated.
@pires @emersion can you please review when you get chance ?
Thanks a ton for this contribution. We'll be releasing a new minor for introducing the new behavior and announce the deprecation of PolicyFunc
🖖🏻
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.