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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

peteski22
Copy link

@peteski22 peteski22 commented Jun 14, 2024

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).

@pires
Copy link
Owner

pires commented Jun 28, 2024

Can you, please, rebase?

@peteski22 peteski22 force-pushed the peteski22/dont-error-listener-on-invalid-upstream branch from d96e12c to 956f8fe Compare July 5, 2024 15:36
@peteski22
Copy link
Author

Can you, please, rebase?

@pires done now, thanks.

@coveralls
Copy link

Coverage Status

coverage: 94.924% (-0.2%) from 95.119%
when pulling 956f8fe on peteski22:peteski22/dont-error-listener-on-invalid-upstream
into b718e7c on pires:main.

@camaeel
Copy link

camaeel commented Jul 16, 2024

@pires any chance for merging this one?

@drakkan drakkan mentioned this pull request Aug 10, 2024
@pires
Copy link
Owner

pires commented Aug 12, 2024

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

@peteski22
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants