Skip to content

lookup SocketKind with sock.type#32

Merged
icgood merged 1 commit intoicgood:mainfrom
tommyvn:main
Nov 30, 2021
Merged

lookup SocketKind with sock.type#32
icgood merged 1 commit intoicgood:mainfrom
tommyvn:main

Conversation

@tommyvn
Copy link
Copy Markdown
Contributor

@tommyvn tommyvn commented Nov 26, 2021

Confusingly sock.proto is not analogous to the PROXY protocol's socket
protocol, rather the sock.type attribute is. In almost all cases
sock.proto will be zero as per
https://docs.python.org/3/library/socket.html#socket.socket

This was resulting in the command proxyprotocol-server --service localhost:8082 localhost:8080 always injecting a PROXY protocol header
with a protocol of int 0 to the downstream server. This PR fixes that,
it now correctly injects int 1 in the above scenario and the other
SocketKind lookups should also work correctly.

Confusingly `sock.proto` is not analogous to the PROXY protocol's socket
protocol, rather the `sock.type` attribute is. In almost all cases
`sock.proto` will be zero as per
https://docs.python.org/3/library/socket.html#socket.socket

This was resulting in the command `proxyprotocol-server --service
localhost:8082 localhost:8080` always injecting a PROXY protocol header
with a protocol of int 0 to the downstream server. This PR fixes that,
it now correctly injects int 1 in the above scenario and the other
`SocketKind` lookups should also work correctly.
@icgood
Copy link
Copy Markdown
Owner

icgood commented Nov 28, 2021

Great catch, thanks!

Would you mind fixing the relevant parts of SocketInfo as well? Specifically this docstring and this other call to socket.proto. Or I can, if you'd prefer.

@icgood
Copy link
Copy Markdown
Owner

icgood commented Nov 30, 2021

Merging, I'll fix SocketInfo in a followup PR.

@icgood icgood merged commit 33ee8ff into icgood:main Nov 30, 2021
@tommyvn
Copy link
Copy Markdown
Contributor Author

tommyvn commented Dec 28, 2021

sorry, I got distracted by xmas prep :( thanks for the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants