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

fqdn: dnsproxy: fix data race in dns proxy implementation #22619

Merged
merged 1 commit into from
Dec 20, 2022
Merged

fqdn: dnsproxy: fix data race in dns proxy implementation #22619

merged 1 commit into from
Dec 20, 2022

Conversation

aspsk
Copy link
Contributor

@aspsk aspsk commented Dec 8, 2022

A recent commit patched dnsproxy to configure a net.Dialer for every outgoing request. However, the dialer was assigned to a single shared copy of dns.Client which lead to data corruption. Create a new dns.Client each time we do a client, so the state is not shared between threads.

Fixes: cf3cc16 ("fqdn: dnsproxy: fix forwarding of the original security identity for TCP")

Signed-off-by: Anton Protopopov aspsk@isovalent.com

Fixes: #22598

Fix a data race in dnsproxy which could lead to DNS requests drops.

@aspsk aspsk requested review from a team as code owners December 8, 2022 13:09
@aspsk aspsk requested a review from jibi December 8, 2022 13:09
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 8, 2022
@aspsk aspsk requested a review from jrajahalme December 8, 2022 13:09
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Dec 8, 2022
@aspsk aspsk added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Dec 8, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 8, 2022
@aspsk
Copy link
Contributor Author

aspsk commented Dec 8, 2022 •
edited by maintainer-s-little-helper bot
Loading

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Timed out after 240.000s.

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4 so I can create one.

Job 'Cilium-PR-K8s-1.16-kernel-4.9' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Timed out after 240.000s.

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.9 so I can create one.

@aspsk aspsk added release-blocker/1.10 release-blocker/1.11 This issue will prevent the release of the next version of Cilium. release-blocker/1.12 This issue will prevent the release of the next version of Cilium. release-blocker/1.13 This issue will prevent the release of the next version of Cilium. backport/1.11 This PR represents a backport for Cilium 1.11.x of a PR that was merged to main. backport/1.12 This PR represents a backport for Cilium 1.12.x of a PR that was merged to main. labels Dec 9, 2022
@aspsk
fqdn: dnsproxy: fix data race in dns proxy implementation
7e102f8 
A recent commit patched dnsproxy to configure a net.Dialer for every outgoing
request. However, the dialer was assigned to a single shared copy of dns.Client
which lead to data corruption. Create a new dns.Client each time we do a
client, so the state is not shared between threads.

Fixes: cf3cc16 ("fqdn: dnsproxy: fix forwarding of the original security identity for TCP")

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
@aspsk aspsk force-pushed the aspsk/pr/fix-data-race-in-dns-code branch from be54e90 to 7e102f8 Compare December 9, 2022 07:36
@aspsk
Copy link
Contributor Author

aspsk commented Dec 9, 2022

/test

@aanm aanm added needs-backport/1.10 needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch and removed kind/community-contribution This was a contribution made by a community member. backport/1.10 backport/1.11 This PR represents a backport for Cilium 1.11.x of a PR that was merged to main. backport/1.12 This PR represents a backport for Cilium 1.12.x of a PR that was merged to main. labels Dec 9, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.12.5 Dec 9, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.18 Dec 9, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.11.12 Dec 9, 2022
@joestringer joestringer added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Dec 22, 2022
@joestringer joestringer mentioned this pull request Dec 22, 2022
Merged
@gentoo-root gentoo-root mentioned this pull request Jan 6, 2023
Merged
@aditighag aditighag mentioned this pull request Jan 10, 2023
Merged
@aditighag aditighag added backport-pending/1.12 backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed needs-backport/1.12 labels Jan 10, 2023
@ldelossa ldelossa mentioned this pull request Jan 24, 2023
Closed
@ldelossa ldelossa mentioned this pull request Jan 24, 2023
Merged
18 tasks
@joestringer joestringer added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Jan 25, 2023
@michi-covalent michi-covalent added this to Needs backport from master in 1.10.20 Jan 26, 2023
@michi-covalent michi-covalent removed this from Needs backport from master in 1.10.19 Jan 26, 2023
@michi-covalent michi-covalent moved this from Needs backport from master to Backport done to v1.11 in 1.11.13 Jan 26, 2023
@michi-covalent michi-covalent mentioned this pull request Jan 26, 2023
Merged
@michi-covalent michi-covalent moved this from Needs backport from master to Backport done to v1.12 in 1.12.6 Jan 26, 2023
@michi-covalent michi-covalent mentioned this pull request Jan 26, 2023
Merged
@qmonnet qmonnet mentioned this pull request Jan 27, 2023
Merged
7 tasks
@joestringer joestringer moved this from Needs backport from master to Backport done to v1.10 in 1.10.20 Feb 13, 2023
This was referenced Feb 13, 2023
Closed
Merged
@aspsk aspsk deleted the aspsk/pr/fix-data-race-in-dns-code branch March 30, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jibi jibi jibi approved these changes

@joestringer joestringer joestringer approved these changes

@christarazi christarazi christarazi approved these changes

@jrajahalme jrajahalme jrajahalme approved these changes

Assignees
No one assigned
Labels
backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.13 This issue will prevent the release of the next version of Cilium. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.10.20
Backport done to v1.10
1.11.13
Backport done to v1.11
1.12.6
Backport done to v1.12
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

race: between pkg/fqdn/dnsproxy.(*DNSProxy).ServeDNS() and miekg/dns.(*Client).getTimeoutForRequest()