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

backport-2.1: pgwire: cancel authentication work when the network connection is closed #36231

Merged
merged 1 commit into from
Apr 6, 2019

Conversation

andreimatei
Copy link
Contributor

backport of #35776 and #36221
cc @cockroachdb/release

Before this patch we would not react to a pgwire connection closing
while we were in the process of authenticating the connection's user.
That's bad particularly when authentication is blocked forever because
of data unavailability. Before this patch, goroutines blocked on
authentication would just leak if the client closed the corresponding
connection, leading to OOMs for folks.
Generally, we react to connection cancelation: a dedicated goroutine is
constantly responsible for reading from the network and canceling the
command processor's context if the connection drops. However,
authentication was special in that it happened before the connection
entered this reader/processor-goroutine mode. Authentication did it's
own reading and writing from the net conn.

This patch extends authentication to fit into the 2-goroutine model. The
conn goroutine takes responsibility for reading all the password
information and passing it to an authenticator which runs on the command
processing goroutine. As in all other cases, if the connection is
dropped, the authenticator's ctx will be canceled.
This required abstracting the conn's interaction with the authenticator
through an interface. The authenticator's interactions with the
connection were already abstracted through an interface for GSS API
reasons. That interface is adapted. A "pipe" metaphor is used to model
the interactions between the conn and the authenticator.

Release note: bug fix: memory and goroutines used for authenticating
connections opened while the cluster is unavailable (e.g. network
partition) no longer leak when the client closes said connections.

@andreimatei andreimatei requested review from nvanbenschoten and a team March 27, 2019 18:23
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@nvanbenschoten
Copy link
Member

Do you mind commenting on where this backport was clean and where you had to make changes? This will need more scrutiny than #36177.

@andreimatei
Copy link
Contributor Author

It was not a clean cherry-pick because 2.1 didn't have the kerberos auth method and all the refactorings that happened when that went in. But it wasn't too terrible.

Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 8 of 8 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@andreimatei andreimatei force-pushed the backport2.1-35776 branch 3 times, most recently from 85d5cf9 to 897e0f3 Compare April 5, 2019 23:12
Before this patch we would not react to a pgwire connection closing
while we were in the process of authenticating the connection's user.
That's bad particularly when authentication is blocked forever because
of data unavailability. Before this patch, goroutines blocked on
authentication would just leak if the client closed the corresponding
connection, leading to OOMs for folks.
Generally, we react to connection cancelation: a dedicated goroutine is
constantly responsible for reading from the network and canceling the
command processor's context if the connection drops. However,
authentication was special in that it happened before the connection
entered this reader/processor-goroutine mode. Authentication did it's
own reading and writing from the net conn.

This patch extends authentication to fit into the 2-goroutine model. The
conn goroutine takes responsibility for reading all the password
information and passing it to an authenticator which runs on the command
processing goroutine. As in all other cases, if the connection is
dropped, the authenticator's ctx will be canceled.
This required abstracting the conn's interaction with the authenticator
through an interface. The authenticator's interactions with the
connection were already abstracted through an interface for GSS API
reasons. That interface is adapted. A "pipe" metaphor is used to model
the interactions between the conn and the authenticator.

Release note: bug fix: memory and goroutines used for authenticating
connections opened while the cluster is unavailable (e.g. network
partition) no longer leak when the client closes said connections.
@andreimatei andreimatei merged commit 304007d into cockroachdb:release-2.1 Apr 6, 2019
@andreimatei andreimatei deleted the backport2.1-35776 branch April 6, 2019 01:09
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.

None yet

3 participants