-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Upgrade pq to fix connection failure cleanup bug (v1.8.0 => v1.10.3) #12413
Conversation
Fixes #12415 |
It might be related to this particular symptom in Azure, #12415 (comment) |
I don't maintain this repository, but I just wanted to drop in and add a few comments that may help this move along. |
@ianferguson Thanks for this! Can you please update to v1.10.3 and resolve the |
cd7bd03
to
d43574b
Compare
@kalafut i've reset my branch and run
to regenerate this commit -- let me know if I can do anything else, and thanks for looking at this PR! |
@@ -719,8 +719,6 @@ github.com/hashicorp/vault-plugin-auth-jwt v0.10.1 h1:7hvGSiICXpmp7Ras5glxVVxTDg | |||
github.com/hashicorp/vault-plugin-auth-jwt v0.10.1/go.mod h1:3KxfehLIM7zH19+O8jHJ/QJsLGRzSKRqjsesOJmBuoI= | |||
github.com/hashicorp/vault-plugin-auth-kerberos v0.4.0 h1:7M7/DbFsUoOMBd2/R48ZNj4PM3Gdsg0dGcbMOdt5z1Q= | |||
github.com/hashicorp/vault-plugin-auth-kerberos v0.4.0/go.mod h1:h+7pLm4Z2EeKHOGPefX0bGzdUQCMBUlvM/BpSMNgTFw= | |||
github.com/hashicorp/vault-plugin-auth-kubernetes v0.11.1-0.20210921194437-e5af6ccd8add h1:Spwfyp4obQ6MhXWCsYHiAlNsehb8PCVciF1vMZqn3so= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks unrelated to the dep updates. Can we rebase/merge origin/main
once more? I just merged a PR that synced up the deps recently which should take this change out of the PR.
I don't see other deps being pulled in, but can we do the dep update without the -u
option just in case, so that we don't end up pulling transitive dep updates?
$ go get github.com/lib/pq
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This came from running go mod tidy
and occurs on the head of main still for me:
~/g/h/vault (main)> git fetch
~/g/h/vault (main)> git merge --ff-only
Already up to date.
~/g/h/vault (main)> go mod tidy
~/g/h/vault (main)> git diff
diff --git a/go.sum b/go.sum
index 70ded0821..70754edc1 100644
--- a/go.sum
+++ b/go.sum
@@ -719,8 +719,6 @@ github.com/hashicorp/vault-plugin-auth-jwt v0.10.1 h1:7hvGSiICXpmp7Ras5glxVVxTDg
github.com/hashicorp/vault-plugin-auth-jwt v0.10.1/go.mod h1:3KxfehLIM7zH19+O8jHJ/QJsLGRzSKRqjsesOJmBuoI=
github.com/hashicorp/vault-plugin-auth-kerberos v0.4.0 h1:7M7/DbFsUoOMBd2/R48ZNj4PM3Gdsg0dGcbMOdt5z1Q=
github.com/hashicorp/vault-plugin-auth-kerberos v0.4.0/go.mod h1:h+7pLm4Z2EeKHOGPefX0bGzdUQCMBUlvM/BpSMNgTFw=
-github.com/hashicorp/vault-plugin-auth-kubernetes v0.11.1-0.20210921194437-e5af6ccd8add h1:Spwfyp4obQ6MhXWCsYHiAlNsehb8PCVciF1vMZqn3so=
-github.com/hashicorp/vault-plugin-auth-kubernetes v0.11.1-0.20210921194437-e5af6ccd8add/go.mod h1:Q13bq4paoPWW+bsSq2seyiLPQkFl5vrb+vIwwLDlQ8M=
github.com/hashicorp/vault-plugin-auth-kubernetes v0.11.1-0.20210929181055-821e911b1751 h1:wICfRtupLijLDjQ/8GGnEOvpDzxGK1pwd1OQBZFQOr0=
github.com/hashicorp/vault-plugin-auth-kubernetes v0.11.1-0.20210929181055-821e911b1751/go.mod h1:Q13bq4paoPWW+bsSq2seyiLPQkFl5vrb+vIwwLDlQ8M=
github.com/hashicorp/vault-plugin-auth-oci v0.8.0 h1:qYtVYsQlVnqqlCVqZ+CAiFEXuYJqUQCuqcWQVELybZY=
I can rewrite the commits to drop the go mod tidy
but had added that call based on this comment's suggestion: #12413 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's okay. It's just removing the older checksum format for the github.com/hashicorp/vault-plugin-auth-kubernetes
package. You can see the latest version of the plugin that's in use remains just below these changes with the same Q13bq4paoPWW+bsSq2seyiLPQkFl5vrb+vIwwLDlQ8M=
value:
github.com/hashicorp/vault-plugin-auth-kubernetes v0.11.1-0.20210929181055-821e911b1751/go.mod h1:Q13bq4paoPWW+bsSq2seyiLPQkFl5vrb+vIwwLDlQ8M=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What version of go
are you on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go version
go version go1.17.1 darwin/amd64
it is honoring the go1.16 directive in the go.mod file and not making 2 requires blocks ala 1.17 -- if it is an issue let me know and I'll remove commit d43574b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we should be running go mod tidy
. It's odd that this removal didn't apply already in my recently commit which I also ran go mod tidy
against. I was hoping to clean this up so that it'd make backporting and conflict resolution a little easier, but we can also address this on our end.
Can we also include a changelog file that mentions the bug fix as part of this PR? Here is a sample of the formatting for a changelog entry file (the name of the file should be the PR number). Something along the lines of:
storage/postgres: Update postgres library to properly clean up connection pool on write errors
database/postgres: Update postgres library to properly clean up connection pool on write errors
Sorry for the numerous back-and-forths on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added changelog/12413.txt
in 2b37f57
…12413) * Upgrade pq to fix connection failure cleanup bug (v1.8.0 => v1.10.3) * Run go mod tidy after `go get -u github.com/lib/pq` * include changelog/12413.txt # Conflicts: # go.sum
…12413) * Upgrade pq to fix connection failure cleanup bug (v1.8.0 => v1.10.3) * Run go mod tidy after `go get -u github.com/lib/pq` * include changelog/12413.txt # Conflicts: # go.sum
…12413) * Upgrade pq to fix connection failure cleanup bug (v1.8.0 => v1.10.3) * Run go mod tidy after `go get -u github.com/lib/pq` * include changelog/12413.txt # Conflicts: # go.mod # go.sum
…12413) (#12725) * Upgrade pq to fix connection failure cleanup bug (v1.8.0 => v1.10.3) * Run go mod tidy after `go get -u github.com/lib/pq` * include changelog/12413.txt # Conflicts: # go.sum Co-authored-by: Ian Ferguson <[email protected]>
…12413) (#12727) * Upgrade pq to fix connection failure cleanup bug (v1.8.0 => v1.10.3) * Run go mod tidy after `go get -u github.com/lib/pq` * include changelog/12413.txt # Conflicts: # go.sum Co-authored-by: Ian Ferguson <[email protected]>
…12413) (#12728) * Upgrade pq to fix connection failure cleanup bug (v1.8.0 => v1.10.3) * Run go mod tidy after `go get -u github.com/lib/pq` * include changelog/12413.txt # Conflicts: # go.mod # go.sum Co-authored-by: Ian Ferguson <[email protected]>
We've encountered an issue when using TLS enabled posgres connections
Azure Database for PostgreSQL
w/ golang 1.15 or newer and lib/pq versions prior to v1.9.0.When this issue occurs, connections will fail, but not be purged from the lib/pq pool and will continue to be used by the application unsuccessfully until the application (Vault in this case) is restarted.
This issue appeared with the update from go 1.14 to go 1.15 because of this change in the
crypto/tls
package (go release notes):In particular, the returned errors are now of type
*tls.permamentError
instead of*net.OpError
, and thus do not match this check inlib/pq
, so the connection is not marked as "bad".lib/pq#1013 (part of v1.9.0) fixes the issue by marking the connection as bad for any write error where no data was written.
This PR was generated by running:
from the root of this repository.