fix: use fake local network for port-forward tests #11119
Conversation
|
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
There was a problem hiding this comment.
Other than a few concerns and renaming suggestions, I think this looks fine.
Another way we might've been able to remove collisions is to use new ranges for localhost, like 127.0.1.1, 127.0.2.1, etc, each test could reserve one and be quite sure that a freed port isn't re-used.
| "net" | ||
| "strconv" | ||
|
|
||
| "github.com/pion/udp" |
There was a problem hiding this comment.
This package is no longer maintained (repo archived). We should consider switching to github.com/pion/transport/v2/udp if we still need the library.
| // Use pion here so that we get a stream-style net.Conn listener, instead | ||
| // of a packet-oriented connection that can read and write to multiple | ||
| // addresses. | ||
| return udp.Listen(network, &net.UDPAddr{ |
There was a problem hiding this comment.
Are we sure we want to use pion for UDP in all CLI usage going forward? (I understand this is mainly used by port-forward, so this is mainly a future concern about making assumptions based on one use-case).
There was a problem hiding this comment.
I'm not sure, no. We can always refactor later if this doesn't meet the future use case.
Putting UDP into its own method makes faking this interface harder/more complex.
| // osNet is an implementation that call the real OS for networking. | ||
| type osNet struct{} | ||
|
|
||
| func (osNet) Listen(network, address string) (net.Listener, error) { |
There was a problem hiding this comment.
How would we handle situations where you might need to configure dialing via net.Dialer?
Maybe we don't have such use-cases at this time but I'm concerned since we are introducing the field on clibase, which suggests it's the only way one should establish network connections (compared to injecting it into the specific command that needs it).
There was a problem hiding this comment.
Add a GetDialer(options) net.Dialer method to the Net interface, or somesuch.
(compared to injecting it into the specific command that needs it)
I'm not sure what you mean here. How would that suggestion be different than what I've implemented?
There was a problem hiding this comment.
The idea here is that Net is like the stdio and environment fields on Invocation. When you actually run the command from a command line, you get the OS-provided functions, but in testing we can hook various things up to intercept the command interacting with the OS, just like how today we hook stdio into the tests.
There was a problem hiding this comment.
Add a
GetDialer(options) net.Dialermethod to theNetinterface, or somesuch.
I suppose that could work, injecting resolver/dialer for the in-mem stuff 👍🏻.
I'm not sure what you mean here. How would that suggestion be different than what I've implemented?
With that parenthesis, I basically meant not putting the field in clibase/inv until there are more use-cases, i.e. keeping it port-forwarding only for now.
There was a problem hiding this comment.
I'm not sure how we could keep it port-forwarding only without a major refactor. The command handler only gets an Invocation, which is also the only thing we have access to from clitest.New(). There literally isn't anything else to hang it on besides Invocation (module-level vars will wreak havoc in parallel testing).
There was a problem hiding this comment.
There's also context.Context, but I hear you. That's perhaps a bit hidden away.
On macOS you can't bind to these addresses: |
| @@ -160,21 +124,27 @@ func TestPortForward(t *testing.T) { | |||
| inv.Stdin = pty.Input() | |||
| inv.Stdout = pty.Output() | |||
| inv.Stderr = pty.Output() | |||
|
|
|||
| iNet := newInProcNet() | |||
| inv.Net = iNet | |||
There was a problem hiding this comment.
Looks like GitHub lost one of my comments. 😔
I had asked about what i in iNet means (perhaps testNet, fakeNet, memNet, etc would be more descriptive).
I also suggested a name change => s/Proc/Mem/ to better match our other implementations.
Since this got merged already, I won't demand you make the changes (just a small wish 😄)
Soz! When I dip back into this for other networking stuff I'll keep it in mind. |
Fixes #10979
Testing code that listens on a specific port has created a long battle with flakes. Previous attempts to deal with this include opening a listener on a port chosen by the OS, then closing the listener, noting the port and starting the test with that port.
This still flakes, notably in macOS which has a proclivity to reuse ports quickly.
Instead of fighting with the chaos that is an OS networking stack, this PR fakes the host networking in tests.
I've taken a small step here, only faking out the Listen() calls that port-forward makes, but I think over time we should be transitioning all networking the CLI does to an abstract interface so we can fake it. This allows us to run in parallel without flakes and
presents an opportunity to test error paths as well.