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

Replace pkg/rand by standard library math/rand/v2 #32542

Merged
merged 6 commits into from
May 16, 2024

Conversation

tklauser
Copy link
Member

Since Go 1.22 the Go standard library provides the math/rand/v2 package1. Its global generator accessed by top-level functions is unconditionally seeded using a true random value. Moreover, the new package provides a way to construct PRNG sources which can be accessed concurrently.

This allows to drop SafeRand and use the global source again where applicable. SafeRand was originally introduced in commit fac5dde ("rand: add and use concurrency-safe PRNG source") to fix #10988 by providing a concurrency-safe PRNG source other than the global math/rand source which could be used at the time because it can't be interfered with from unrelated packages by means of calling rand.Seed, see #10575.

In some cases the global math/rand/v2 source cannot be used because unit tests rely on seeding the PRNG source to a fixed value for deterministic test runs. In these cases a math/rand/v2 source using fixed values is used, like with SafeRand before.

See commits for details.

Supersedes #29731

Footnotes

  1. https://go.dev/doc/go1.22#math_rand_v2

@tklauser tklauser added kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. kind/tech-debt Technical debt labels May 15, 2024
@tklauser tklauser requested review from a team as code owners May 15, 2024 09:48
@tklauser
Copy link
Member Author

/test

Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Thanks and lgtm ✔️

daemon/cmd/status.go Outdated Show resolved Hide resolved
daemon/cmd/status.go Outdated Show resolved Hide resolved
@tklauser tklauser enabled auto-merge May 15, 2024 10:30
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Changes overall look good to me, thanks!

Just a couple of comments inline. I wonder if it wouldn't be better to convert the few final math/rand imports over to v2, and then add a linter to forbid using math/rand altogether, to prevent possible regressions.

pkg/proxy/proxy.go Outdated Show resolved Hide resolved
pkg/ipam/service/allocator/bitmap.go Outdated Show resolved Hide resolved
pkg/lock/stoppable_waitgroup_test.go Show resolved Hide resolved
@tklauser
Copy link
Member Author

Just a couple of comments inline. I wonder if it wouldn't be better to convert the few final math/rand imports over to v2, and then add a linter to forbid using math/rand altogether, to prevent possible regressions.

Good suggestion! I'll do that in an additional set of commits on this PR given the list of math/rand imports is pretty small.

Since Go 1.22 the Go standard library provides the math/rand/v2
package[^1]. Its global generator accessed by top-level functions is
unconditionally seeded using a true random value. Moreover, the new
package provides a way to construct PRNG sources which can be accessed
concurrently.

This allows to drop SafeRand and use the global source again where
applicable. SafeRand was originally introduced in commit fac5dde ("rand:
add and use concurrency-safe PRNG source") to fix #10988 by providing a
concurrency-safe PRNG source other than the global math/rand source
which could be used at the time because it can't be interfered with from
unrelated packages by means of calling rand.Seed, see #10575.

In some cases the global math/rand/v2 source cannot be used because unit
tests rely on seeding the PRNG source to a fixed value for deterministic
test runs. In these cases a math/rand/v2 source using fixed values is
used, like with SafeRand before.

[^1]: https://go.dev/doc/go1.22#math_rand_v2

Signed-off-by: Tobias Klauser <tobias@cilium.io>
The previous commit changed all users of pkg/rand to use math/rand/v2
from the Go standard library. As a result this package is no longer
needed, should not be used going forward and can thus be removed.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
pkg/rand was removed in the previous commit and we'll rely on
math/rand/v2 from the Go standard library going forward. The script
checking for pkg/rand usage is thus no longer needed.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
Replace use of the standard library math/rand package by math/rand/v2
which provides top-level functions backed by a global generator
unconditionally seeded using a true random value. This avoids having to
create generators manually.

Suggested-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Tobias Klauser <tobias@cilium.io>
Use crypto/rand.Read instead as suggested in
https://pkg.go.dev/math/rand#Read

Signed-off-by: Tobias Klauser <tobias@cilium.io>
Use the depguard linter to disallow using the standard library math/rand
package and use v2 of the package instead.

Suggested-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser tklauser force-pushed the pr/tklauser/switch-to-math-rand-v2 branch from 5ba4edd to 35587ff Compare May 15, 2024 12:45
@tklauser
Copy link
Member Author

/test

@tklauser tklauser disabled auto-merge May 15, 2024 12:48
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

FQDN changes LGTM, thanks! 💯

Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM for ci-structure

@tklauser tklauser enabled auto-merge May 15, 2024 19:26
@tklauser tklauser added this pull request to the merge queue May 16, 2024
@rolinh rolinh removed the request for review from borkmann May 16, 2024 09:09
Merged via the queue into main with commit 376a232 May 16, 2024
270 of 271 checks passed
@tklauser tklauser deleted the pr/tklauser/switch-to-math-rand-v2 branch May 16, 2024 09:17
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup This includes no functional changes. kind/tech-debt Technical debt ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DATA RACE: rand.NewSource is not safe for concurrent usage.
9 participants