listener: allow TCP and UDP on the same port

[mattklein123]

Fixes #15562

Risk Level: Low
Testing: Modified unit tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

[mattklein123]

cc @ggreenway @lambdai

[ggreenway]

I think we may be better off adding ipproto (tcp or udp) to the address type, so that equality comparison works as expected. I think most people would intuitively expect that 1.1.1.1:80:TCP != 1.1.1.1:80:UDP. The OS considers them completely separate addresses, so probably our abstraction should also.

[mattklein123]

I think we may be better off adding ipproto (tcp or udp) to the address type, so that equality comparison works as expected. I think most people would intuitively expect that 1.1.1.1:80:TCP != 1.1.1.1:80:UDP. The OS considers them completely separate addresses, so probably our abstraction should also.

OK, I can take a look at this. It will likely be a much larger change.

[ggreenway]

OK, I can take a look at this. It will likely be a much larger change.

Yes, but I think it's the right way to do it. Probably can default to TCP to have a much smaller change. UDP is used far less at this point. Could later make a mechanical change to remove the default and specify TCP in all the places it is needed, if we want that for clarity.

[mattklein123]

@ggreenway I looked at this briefly and I'm not sure that I agree that making the Address interface include TCP/UDP is the right thing to do. It's used in various places that don't care about that at all (DNS, IP tagging, etc.) and I think it's not a great abstraction.

How about this: a wrapper AddressAndProto which contains both the underlying address and the proto, and then define equality on that correctly. Then, we can use that inside the listener manager code and fix effected call sites. I think this will be more targeted and clear. WDYT?

[ggreenway]

@ggreenway I looked at this briefly and I'm not sure that I agree that making the Address interface include TCP/UDP is the right thing to do. It's used in various places that don't care about that at all (DNS, IP tagging, etc.) and I think it's not a great abstraction.

How about this: a wrapper AddressAndProto which contains both the underlying address and the proto, and then define equality on that correctly. Then, we can use that inside the listener manager code and fix effected call sites. I think this will be more targeted and clear. WDYT?

I think we currently use it for two use cases: IP address, or IP:port. Maybe those two should be separate, and TCP/UDP should only be in the IP:port version. But that may turn into an enormous change. Or maybe leave the interfaces as they are, but replace all uses of port with a tuple of port:ipproto, but keep it optional (and defaulting to TCP for now). But i feel like anytime we have a port number, we need to know whether it is tcp or udp; it's not meaningful generically.

[mattklein123]

Or maybe leave the interfaces as they are, but replace all uses of port with a tuple of port:ipproto, but keep it optional (and defaulting to TCP for now). But i feel like anytime we have a port number, we need to know whether it is tcp or udp; it's not meaningful generically.

I still think this is going to be incredibly invasive as there are string versions of address that would need to take into account proto, etc. I agree it's doable but I'm just not sure it's worth the churn. I will keep this going as a side project but given the scope of this change I will put it on hold for now as I don't have the near term time.

/wait

[ggreenway]

Yeah, it may not be worth the churn. I think this change looks good to solve the immediate problem. We can decide in the future if we need a wholistic solution or not.

[mattklein123]

OK, let me open a tracking issue for a better fix.

[mattklein123]

Cross linking #17429 for posterity on a better long term fix.