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

add TLS support to HalfCloser interface #6216

Merged
merged 5 commits into from
Aug 12, 2019
Merged

add TLS support to HalfCloser interface #6216

merged 5 commits into from
Aug 12, 2019

Conversation

mikemorris
Copy link
Contributor

This relates to a known issue documented in #6203, and may fix TLS connections to half-close properly.

When I started writing this I had assumed CloseWrite would exist on the returned net.Conn object and wouldn't need to cast in agent/consul/snapshot_endpoint.go, looking back may want to re-implement HalfCloser, just over either connection type now?

https://github.com/hashicorp/consul/blob/02a52ef22881bcf98184a9604294aacfaf846c09/agent/consul/snapshot_endpoint.go#L225-L244

Refs golang/go#8579

@mikemorris mikemorris requested a review from a team July 25, 2019 16:12
@mikemorris mikemorris force-pushed the conn-close-write branch 2 times, most recently from a17b2de to 38bd383 Compare July 25, 2019 16:29
@banks
Copy link
Member

banks commented Aug 1, 2019

looking back may want to re-implement HalfCloser, just over either connection type now?

Yeah I think the old model was fine and we should just return the half closer for tls too. This PR is actually not bad and might even be closer to what I'd have written in the first place but I think it would be a smaller change to keep HalfCloser and make it work for TLS instead.

The (tiny, theoretical) benefit of the interface is that if we ever have other conn types (unlikey) we wouldn't have to change this code to make it work...

Directly calls net.TCPConn.CloseWrite or mtls.Conn.CloseWrite, which was added in https://go-review.googlesource.com/c/go/+/31318/
@mikemorris mikemorris changed the title remove HalfCloser interface add TLS support to HalfCloser interface Aug 6, 2019
@mikemorris
Copy link
Contributor Author

This should be ready for review again - any suggestions on if/how/where I should write a test for this?

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Nice! Seems like a simpler change.

I can't think of a good way to test this sadly without either changing the code much more to plumb in dummy diallers or some elaborate, flaky integration test and I don't think either is a good use of time.

agent/pool/pool.go Outdated Show resolved Hide resolved
agent/pool/pool.go Outdated Show resolved Hide resolved
mikemorris and others added 2 commits August 12, 2019 12:16
@mikemorris mikemorris merged commit 61206fd into master Aug 12, 2019
@mikemorris mikemorris deleted the conn-close-write branch August 12, 2019 16:47
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.

2 participants