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.

Sign up for GitHub

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

Jump to bottom

Fix a racing issue in client-go UpdateTransportConfig #80284

Merged
merged 1 commit into from
Nov 12, 2019
Merged

Fix a racing issue in client-go UpdateTransportConfig #80284

merged 1 commit into from
Nov 12, 2019

Conversation

danielqsj
Copy link
Contributor

What type of PR is this?

/kind bug
/sig api-machinery

What this PR does / why we need it:

in k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go UpdateTransportConfig(), this assignment statement will cause data race.

kubernetes/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go

Line 194 in 5de4134

a.onRotate = d.CloseAll

Need acquire lock before to avoid it.

Which issue(s) this PR fixes:

Fixes #80269

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix a racing issue in client-go UpdateTransportConfig.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Jul 18, 2019
@loginakhil
Copy link

@danielqsj is it possible to add some tests so that these kind of bugs wont happen in future ?
I am not that expert in kubernetes code base, but its possible with unit tests running in parallel to capture such race conditions.

@roycaihw
Copy link
Member

/cc @jpbetz

@k8s-ci-robot k8s-ci-robot requested a review from jpbetz July 18, 2019 20:06
@jpbetz
Copy link
Contributor

jpbetz commented Jul 18, 2019

@danielqsj Thanks for finding this. Just to confirm, are all of the other functions in this file correct? I did a quick pass and they all look okay, but it would be great if you'd check it carefully as well.

@danielqsj
Copy link
Contributor Author

@jpbetz Other functions in this file look good.

@danielqsj
Copy link
Contributor Author

/retest

@jpbetz
Copy link
Contributor

jpbetz commented Jul 19, 2019

/lgtm

Thanks for fixing this!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 19, 2019
@danielqsj
Copy link
Contributor Author

Add sig-auth approvers for approving.
@kubernetes/sig-auth-proposals

@k8s-ci-robot k8s-ci-robot added the kind/design Categorizes issue or PR as related to design. label Jul 22, 2019
@mikedanese mikedanese added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed kind/design Categorizes issue or PR as related to design. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 24, 2019
@mikedanese mikedanese self-assigned this Jul 24, 2019
@mikedanese
Copy link
Member

mikedanese commented Jul 24, 2019 •
edited
Loading

This looks ok, but would like to refer it back to the linked bug.

/approve

@mikedanese mikedanese removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2019
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 24, 2019
@liggitt
Copy link
Member

liggitt commented Aug 28, 2019

/hold
for resolution of #80284 (comment)

@liggitt
Copy link
Member

liggitt commented Sep 26, 2019

I'd also like to see a test demonstrating the bug this is fixing, if possible

@danielqsj
Copy link
Contributor Author

@liggitt thanks for your reviewing. PR updated but I have not find an easy way to write this test case immediately.

liggitt
liggitt reviewed Oct 14, 2019
View reviewed changes
Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

I have not find an easy way to write this test case

I would expect a unit test that starts two goroutines that do the following and lets them run for a few seconds would probably demonstrate the bug on master:

  • make requests in a loop
  • call UpdateTransportConfig in a loop

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go Outdated Show resolved Hide resolved
@dims
Copy link
Member

dims commented Nov 10, 2019

/milestone v1.17

(issue #80269 is in 1.17, PR was not)

@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Nov 10, 2019
@josiahbjorgaard
Copy link
Contributor

@danielqsj Will this likely be merged by this Thursday, 5PM pacific time and close the referenced issue? Otherwise, should we move it and the related issue to v1.18?

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 11, 2019
@danielqsj
Copy link
Contributor Author

@liggitt I add a test case, but I cannot reproduce this issues by it. Need your help. Thanks

@danielqsj
Copy link
Contributor Author

@liggitt thanks, PR updated, PTAL

@liggitt
Copy link
Member

liggitt commented Nov 12, 2019

a few small final comments, then squash down to a single commit

@danielqsj
Copy link
Contributor Author

@liggitt great thanks. PR updated and squashed. :)

@liggitt
Copy link
Member

liggitt commented Nov 12, 2019

/lgtm
/approve
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2019
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielqsj, liggitt, mikedanese

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@danielqsj
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit 48ddf3b into kubernetes:master Nov 12, 2019
@liggitt liggitt mentioned this pull request Jun 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikedanese mikedanese mikedanese left review comments

@liggitt liggitt liggitt left review comments

@lavalamp lavalamp Awaiting requested review from lavalamp

@mbohlool mbohlool Awaiting requested review from mbohlool

@jpbetz jpbetz Awaiting requested review from jpbetz

Assignees

@jpbetz jpbetz

@liggitt liggitt

@awly awly

@mikedanese mikedanese

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.17
Development

Successfully merging this pull request may close these issues.

data race in client go
10 participants
@danielqsj @loginakhil @roycaihw @jpbetz @mikedanese @liggitt @dims @josiahbjorgaard @k8s-ci-robot @awly