Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: add multi listener impl for net.Listener #311

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

aroradaman
Copy link
Member

This adds an implementation of net.Listener which uses synchronous multiplexing to listen on and accept connections from multiple addrs.

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR adds an implementation of net.Listener which uses synchronous multiplexing to listen to and accept connections from multiple addresses.
Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 25, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 25, 2024
@aroradaman
Copy link
Member Author

/cc @thockin @danwinship @aojea

@aroradaman
Copy link
Member Author

For the testing part should I directly add an integration test that listens on net.InterfaceAddrs on multiple ports and then try to connect to server for validation?

@aroradaman
Copy link
Member Author

/cc @uablrek

@aroradaman
Copy link
Member Author

For windows, we might need a goroutine-based async multiplexer as select is not supported.

@aroradaman aroradaman force-pushed the net-multi-listen branch 2 times, most recently from 3327c9c to 6bd4edf Compare May 25, 2024 10:55
net/multi_listen_darwin.go Outdated Show resolved Hide resolved
@aojea
Copy link
Member

aojea commented May 26, 2024

do we really need this?

@aroradaman
Copy link
Member Author

do we really need this?

Currently, for v1alpha2 I spawn multiple helathz and metrics servers in go-routines, this could really help there.

Also, Tim suggested this here

@uablrek
Copy link

uablrek commented May 27, 2024

do we really need this?

My vote is on a library function because of testability. I can imagine some "what if"s that should be tested in (good) unit-tests at one place.

I also want to emphasise my "context" comment:

Also, I think your NewMultiListener should take a context as parameter. net.Listen uses ListenConfig which feels like a work-around for keeping backward compatibility.

@danwinship
Copy link
Contributor

do we really need this?

Currently, for v1alpha2 I spawn multiple helathz and metrics servers in go-routines, this could really help there.

Also, Tim suggested this here

KEP-2438 Dual-Stack API Server needs it too

@danwinship
Copy link
Contributor

But you really shouldn't be implementing this in terms of syscall. Use goroutines. Trust the runtime.

@aroradaman aroradaman force-pushed the net-multi-listen branch 2 times, most recently from 0d98132 to 19a9056 Compare June 11, 2024 14:35
stopCh: make(chan struct{}),
}

for _, address := range addresses {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may need to filter duplicate addresses? or do we want to fail (I think it will fail to create a listener in the same address twice)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the caller be responsible for that (not passing the same address twice).
I can't imagine a case where there is a need to listen on the same address twice, if it is passed it might be because of some bug and we should error out instead of silently ignoring the dup address, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the same area: return an error if the address slice is empty. I think it works like this now: (ml, nil) ia returned, and a subsequent Accept() hangs forever. While this is logical (sort of), it's probably not what the caller intended.

net/multi_listen.go Outdated Show resolved Hide resolved
net/multi_listen.go Outdated Show resolved Hide resolved
@uablrek
Copy link

uablrek commented Jun 12, 2024

You must have support for context. Please see #311 (comment). I will make a better review later.

@uablrek
Copy link

uablrek commented Jun 12, 2024

I think the multi listener must be backward compatible. That means that a call like:

l,e := MultiListen(ctx, []string{":8080"})

must work, and listen on both families, and:

l,e := MultiListen(ctx, []string{"0.0.0.0:8080"})
// or
l,e := MultiListen(ctx, []string{"[::]:8080"})

must also listen on both families.

net/multi_listen.go Outdated Show resolved Hide resolved
_ = ml.Close()
return nil, err
}
switch IPFamilyOf(ParseIPSloppy(host)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd rather see this as an argument, rather than try to figure it out. It seems like an opportunity to get it wrong, when we don't need to.

net/multi_listen.go Show resolved Hide resolved
return ml, nil
}

// Accept is part of net.Listener interface.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would explain here the specific semantics - this will wait for ANY of the sub-listeners to get a connection.

var _ net.Listener = &multiListener{}

// MultiListen returns net.Listener which can listen for and accept
// TCP connections on multiple addresses.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment could be more verbose to explain the semantics


}

// Close is part of net.Listener interface.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment, this will close ALL of the sub-listeners

@uablrek
Copy link

uablrek commented Jun 19, 2024

The multi-listener will have different behavior compared to the standard Listener, as mentioned in #311 (comment). I can't see a way to make them exactly the same, but the differences must be investigated and documented. Otherwise users will assume that they are totally compatible. I can make some tests, but not this week.

Related is how TCP backlog works in Linux. I thought I knew (I didn't), and have been hit by half-open connection problems at least once (in ~20y, so it is very rare).

net/multi_listen.go Outdated Show resolved Hide resolved
net/multi_listen.go Outdated Show resolved Hide resolved
net/multi_listen.go Outdated Show resolved Hide resolved
net/multi_listen.go Outdated Show resolved Hide resolved
net/multi_listen.go Outdated Show resolved Hide resolved
net/multi_listen.go Outdated Show resolved Hide resolved
net/multi_listen.go Show resolved Hide resolved

var _ net.Listener = &fakeListener{}

func TestMultiListen(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need some tests of accept-while-close, close-with-pending-connections, etc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and some tests of HTTP serving on a multi-listener

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and some tests of HTTP serving on a multi-listener

do you mean an integration test that uses real HTTP servers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can still be a unit test (since this repo doesn't really have integration tests). Just use :0 for a random port. Create 2 or 3 such ports, pass the multi-listener to http.Serve and then connect to each of the ports that were allocated, ensuring they get the correct HTTP response.

@aroradaman
Copy link
Member Author

Actually, I don't think you have to store it. Just create the Listerners with something like:

l,e := net.ListenConfig{}.Listen(ctx, network, address)

Then I suppose all Accept() will exit with an error if the ctx is canceled

@uablrek golang/go#50861

@uablrek
Copy link

uablrek commented Jun 23, 2024

Actually, I don't think you have to store it. Just create the Listerners with something like:

l,e := net.ListenConfig{}.Listen(ctx, network, address)

Then I suppose all Accept() will exit with an error if the ctx is canceled

@uablrek golang/go#50861

Is there any way to break a blocking Accept() (except close the Listener)??

@danwinship
Copy link
Contributor

OK, so...

We could do:

MultiListen(ctx context.Context, network string, addresses []string) (net.Listener, error)

where the semantics are basically:

for i := range addresses {
        lc.Listen(ctx, network, addresses[i])
}

with no added intelligence.

If we want per-address network fields, then I think a more logical API would be drop MultiListen and instead have the caller do

ml := netutils.NewMultiListener()
err := ml.Listen(ctx, "tcp4", "10.0.0.0:80")
if err != nil ...
err = ml.Listen(ctx, "tcp6", "[::]:80")
if err != nil ...
...

I think both of these are equally unambiguous, though the latter is more flexible.

I can imagine that for some callers, being able to separately listen on "tcp4", "0.0.0.0:80" and "tcp6", "[::]:80" might make more sense given their internal organization?

Regardless of which of the above approaches we go to, the other question is: what should happen if you listen on both "tcp", "0.0.0.0:80" and "tcp", "[::]:80" (which is one of the bugs that got us here, kubernetes/kubernetes#114702)? Should MultiListener DTRT here, or should it just do literally what you asked, and end up returning an error (meaning that it's up to callers to know better than to do that).

@thockin
Copy link
Member

thockin commented Jun 24, 2024

Taking (or building) a list of individual Listener is about as flexible as gets. It's not what I meant, but I can see the appeal. Given that the primary consumer of this API will be kubernetes, I suspect that's more flexible that we need, but it seems low-cost to do it that way. I see now the argument that dual-stack is a primary motivator, so a single network argument for a list of IPs may be pointless.

WRT zero addresses, if we make the caller call Listen(), then it should be on them to not collide - we should not be too smart. Effectively this becomes an accept-mux - we need to nail those semantics.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 24, 2024
net/multi_listen.go Show resolved Hide resolved
net/multi_listen.go Outdated Show resolved Hide resolved
net/multi_listen.go Outdated Show resolved Hide resolved
net/multi_listen.go Outdated Show resolved Hide resolved
net/multi_listen.go Outdated Show resolved Hide resolved
net/multi_listen.go Outdated Show resolved Hide resolved
@aroradaman aroradaman force-pushed the net-multi-listen branch 3 times, most recently from d6eb366 to 32b2fda Compare June 29, 2024 19:27
This adds an implementation of net.Listener which listens
on and accepts connections from multiple addresses.

Signed-off-by: Daman Arora <aroradaman@gmail.com>
Co-authored-by: Tim Hockin <thockin@google.com>
// This assumes that ANY error from Accept() will terminate the
// sub-listener. We could maybe be more precise, but it
// doesn't seem necessary.
terminate := err != nil
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thockin should I just return the error to the consumer instead of closing the listener here?
The behaviour of multiListener will be similar to net.LIstener if we return the error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, I think we need to return it up thru the channel so the main Accpet can return it

Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aroradaman, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2024
@k8s-ci-robot k8s-ci-robot merged commit d4aae2b into kubernetes:master Jul 10, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants