go-proxyproto icon indicating copy to clipboard operation
go-proxyproto copied to clipboard

Protocol: Keep listener listening if we don't trust the upstream address

Open peteski22 opened this issue 1 year ago • 5 comments

This PR is designed to prevent listeners being stopped when an error is returned, if the upstream connection address is not trusted (ErrInvalidUpstream). Instead, we continue to close the connection but now the Accept method has a for loop to continue looking for other connections to accept.

In using this library we discovered that the listener Accept method returning an error caused the listener to be closed and never reopened when trying to serve HTTP endpoints.

The change was based on github.com/armon/go-proxyproto/ which does something similar with the loop and checking for a particular type of error.

Notes:

See: net/http/server.go => Serve(l net.Listener)

https://cs.opensource.google/go/go/+/master:src/net/http/server.go;l=3333-3351

rw, err := l.Accept()
if err != nil {
	if srv.shuttingDown() {
		return ErrServerClosed
	}
	if ne, ok := err.(net.Error); ok && ne.Temporary() {
		if tempDelay == 0 {
			tempDelay = 5 * time.Millisecond
		} else {
			tempDelay *= 2
		}
		if max := 1 * time.Second; tempDelay > max {
			tempDelay = max
		}
		srv.logf("http: Accept error: %v; retrying in %v", err, tempDelay)
		time.Sleep(tempDelay)
		continue
	}
	return err
}

Above we end up returning the err at the end, which stops the Serve method (so prevents us listening).

peteski22 avatar Jun 14 '24 10:06 peteski22

Can you, please, rebase?

pires avatar Jun 28 '24 15:06 pires

Can you, please, rebase?

@pires done now, thanks.

peteski22 avatar Jul 05 '24 15:07 peteski22

Coverage Status

coverage: 94.136% (+0.03%) from 94.111% when pulling d2dad2966cbf055fc4ba411960f2ce83cae2de4c on peteski22:peteski22/dont-error-listener-on-invalid-upstream into cd8a402dd849b034057d57f49aea2e370f3624f8 on pires:main.

coveralls avatar Jul 11 '24 08:07 coveralls

@pires any chance for merging this one?

camaeel avatar Jul 16 '24 19:07 camaeel

Sorry for the delay. Can we have some tests proving the changes work as expected?

pires avatar Aug 12 '24 12:08 pires

@pires no worries, I've added a test to show the listener stays open when experiencing the invalid upstream error but closes on others.

peteski22 avatar Sep 06 '24 08:09 peteski22

@pires there doesn't seem to be any issues now other than linting which this PR didn't introduce. Would you consider merging it? 🙏🏼

peteski22 avatar Oct 03 '24 07:10 peteski22

Unfortunately, your PR history can't be rebase due to conflicts. I've fixed them but would like for you to re-author the commit so get proper recognition 🙏🏻 https://github.com/pires/go-proxyproto/pull/117/#issuecomment-2399563695

pires avatar Oct 08 '24 11:10 pires

Unfortunately, your PR history can't be rebase due to conflicts. I've fixed them but would like for you to re-author the commit so get proper recognition 🙏🏻 #117

Hi @pires thanks for the message, it's much appreciated. I've rebased everything from upstream/main now so I'm hoping it should look right and this PR can be merged now? 🤞🏼

peteski22 avatar Oct 08 '24 12:10 peteski22

Superb! Thanks a ton.

Closes #117

pires avatar Oct 08 '24 13:10 pires