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

Merged

Conversation

peteski22
Copy link
Contributor

@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
Contributor Author

Can you, please, rebase?

@pires done now, thanks.

@coveralls
Copy link

coveralls commented Jul 11, 2024

Coverage Status

coverage: 94.136% (+0.03%) from 94.111%
when pulling d2dad29 on peteski22:peteski22/dont-error-listener-on-invalid-upstream
into cd8a402 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
Contributor 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.

@peteski22
Copy link
Contributor Author

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

@pires
Copy link
Owner

pires commented Oct 8, 2024

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

@pires pires added this to the 0.8 milestone Oct 8, 2024
@peteski22 peteski22 force-pushed the peteski22/dont-error-listener-on-invalid-upstream branch from d032c9c to d2dad29 Compare October 8, 2024 12:45
@peteski22
Copy link
Contributor Author

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? 🤞🏼

@pires
Copy link
Owner

pires commented Oct 8, 2024

Superb! Thanks a ton.

Closes #117

@pires pires merged commit bac82fd into pires:main Oct 8, 2024
7 checks passed
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