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

downgrade go-ldap client to v3.4.4 due to race conditions in tests #23103

Merged
merged 2 commits into from
Sep 14, 2023

Conversation

raymonstah
Copy link
Contributor

@raymonstah raymonstah commented Sep 14, 2023

Follow up to #22185, which introduced the cap/ldap library, and bumped the go-ldap library version to v3.4.5. Unfortunately, this caused some test failures due to race conditions.

This PR downgrades go-ldap to v3.4.4 to avoid the race conditions.

Example of data race:

WARNING: DATA RACE
Read at 0x00c00d6a6480 by goroutine 108756:
  github.com/go-ldap/ldap/v3.(*Conn).Close()
      /home/runner/go/pkg/mod/github.com/go-ldap/ldap/[email protected]/conn.go:291 +0x1bd
  github.com/go-ldap/ldap/v3.(*Conn).reader.func1()
      /home/runner/go/pkg/mod/github.com/go-ldap/ldap/[email protected]/conn.go:582 +0x104
  runtime.deferreturn()
      /home/runner/actions-runner/_work/_tool/go/1.21.1/x64/src/runtime/panic.go:477 +0x30
  github.com/go-ldap/ldap/v3.(*Conn).Start.func1()
      /home/runner/go/pkg/mod/github.com/go-ldap/ldap/[email protected]/conn.go:267 +0x33

Previous write at 0x00c00d6a6480 by goroutine 83821:
  sync/atomic.StoreInt64()
      /home/runner/actions-runner/_work/_tool/go/1.21.1/x64/src/runtime/race_amd64.s:237 +0xb
  sync/atomic.StoreInt64()
      <autogenerated>:1 +0x15
  github.com/hashicorp/cap/ldap.(*Client).Authenticate()
      /home/runner/go/pkg/mod/github.com/hashicorp/cap/[email protected]/client.go:228 +0x204

@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Sep 14, 2023
@raymonstah raymonstah added this to the 1.16 milestone Sep 14, 2023
@github-actions
Copy link

CI Results:
All Go tests succeeded! ✅

@raymonstah raymonstah marked this pull request as ready for review September 14, 2023 22:51
@raymonstah raymonstah requested review from ncabatoff and a team September 14, 2023 22:51
@raymonstah raymonstah enabled auto-merge (squash) September 14, 2023 22:58
Copy link
Contributor

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

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

Thanks!

@raymonstah raymonstah merged commit 25221fe into main Sep 14, 2023
147 of 156 checks passed
@raymonstah raymonstah deleted the VAULT-20040/raymond/downgrade-go-ldap-to-v3.4.4 branch September 14, 2023 23:18
@github-actions
Copy link

Build Results:
All builds succeeded! ✅

@raymonstah
Copy link
Contributor Author

There's a fix in go-ldap/ldap#465.
Will wait for a v3.4.6 release before introducing back into Vault

@cpuschma
Copy link

cpuschma commented Sep 16, 2023

One of the maintainers of go-ldap here. Sorry for the race condition and thank you for the PR @raymonstah. Version v3.4.6 has been released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants