- Notifications
You must be signed in to change notification settings - Fork927
fix: use fake local network for port-forward tests#11119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Current dependencies on/for this PR: Thisstack of pull requests is managed byGraphite. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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, like127.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This package is no longer maintained (repo archived). We should consider switching togithub.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How would we handle situations where you might need to configure dialing vianet.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Add aGetDialer(options) net.Dialer method to theNet 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The idea here is thatNet is like the stdio and environment fields onInvocation. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure how we could keep it port-forwarding only without a major refactor. The command handler only gets anInvocation, which is also the only thing we have access to fromclitest.New(). There literally isn't anything else to hang it on besidesInvocation (module-levelvars will wreak havoc in parallel testing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There's alsocontext.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks like GitHub lost one of my comments. 😔
I had asked about whati iniNet means (perhapstestNet,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.