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

codegen: fix DeepCopy races #2322

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

howardjohn
Copy link
Member

Fixes istio/api#3019

The deepcopygen we use is from Kubernetes. Its not quite suitable for our types due to our usage of protobuf, which has limitations on dereferencing pointer types. This causes race conditions in concurrent usage of the DeepCopy function.

This is possible to trigger with real Istio (found by running Istio in a live cluster with -race, though our current tests happen to not trigger things I guess), as well as other usage of istio/client-go (as the reporter found).

I ran istio/istio with this change and found all tests continue to pass, and running in a live cluster no longer fails

Fixes istio/api#3019

The deepcopygen we use is from Kubernetes. Its not quite suitable for
our types due to our usage of protobuf, which has limitations on
dereferencing pointer types. This causes race conditions in concurrent
usage of the DeepCopy function.

This is possible to trigger with real Istio (found by running Istio in a
live cluster with `-race`, though our current tests happen to not
trigger things I guess), as well as other usage of istio/client-go (as
the reporter found).

I ran istio/istio with this change and found all tests continue to pass,
and running in a live cluster no longer fails
@howardjohn howardjohn added the cherrypick/release-1.24 Set this label on a PR to auto-merge it to the release-1.24 branch label Dec 12, 2024
@howardjohn howardjohn requested a review from a team as a code owner December 12, 2024 18:46
@istio-policy-bot
Copy link

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

@istio-policy-bot
Copy link

😊 Welcome @howardjohn! This is either your first contribution to the Istio client-go repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick/release-1.24 Set this label on a PR to auto-merge it to the release-1.24 branch size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DeepCopy for generated API types is not safe for concurrent use
4 participants