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

plugins/cilium-cni: Install loopback atomically #29462

Merged
merged 1 commit into from
Nov 29, 2023
Merged

Conversation

akhilles
Copy link
Contributor

If copying of the loopback binary is interrupted, then a truncated version will exist on the node. The node can't recover from this state even if the pod is restarted because install-plugin.sh won't overwrite the existing loopback file.

To fix, install loopback atomically using a cp + mv.

This change also removes the unnecessary deletion of "${BIN_NAME}.new". This is a no-op because the temporary copy destination is prefixed with a dot: ".${BIN_NAME}.new".

Fixes: #29461

Install loopback CNI atomically to protect against aborted copy

If copying of the loopback binary is interrupted, then a truncated
version will exist on the node. The node can't recover from this state
even if the pod is restarted because install-plugin.sh won't overwrite
the existing loopback file.

To fix, install loopback atomically using a cp + mv.

This change also removes the unnecessary deletion of "${BIN_NAME}.new".
This is a no-op because the temporary copy destination is prefixed with
a dot: ".${BIN_NAME}.new".

Signed-off-by: Akhil Velagapudi <4@4khil.com>
@akhilles akhilles requested a review from a team as a code owner November 28, 2023 20:39
@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 Nov 28, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Nov 28, 2023
@youngnick youngnick requested a review from aanm November 29, 2023 06:51
Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM but I'd like to see what @aanm thinks here as well.

@youngnick youngnick added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Nov 29, 2023
@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 Nov 29, 2023
@youngnick
Copy link
Contributor

/test

@akhilles
Copy link
Contributor Author

Any chance this can be back-ported to v1.12+ as it's a bug fix? We actually encountered this issue in v1.12.

@aanm aanm added release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Nov 29, 2023
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

cc @squeed FYI this cool bug

@aanm aanm enabled auto-merge November 29, 2023 20:35
@aanm aanm added this pull request to the merge queue Nov 29, 2023
@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 Nov 29, 2023
Merged via the queue into cilium:main with commit 7398ea3 Nov 29, 2023
62 of 63 checks passed
@aanm
Copy link
Member

aanm commented Nov 29, 2023

Any chance this can be back-ported to v1.12+ as it's a bug fix? We actually encountered this issue in v1.12.

@akhilles According the backports criteria, 1.12 would security related bug fixes. We can mark it for 1.14.

@aanm aanm added affects/v1.12 This issue affects v1.12 branch affects/v1.13 This issue affects v1.13 branch affects/v1.14 This issue affects v1.14 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Nov 29, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.5 Nov 29, 2023
@nbusseneau nbusseneau mentioned this pull request Dec 5, 2023
10 tasks
@nbusseneau nbusseneau added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Dec 5, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.5 Dec 5, 2023
@github-actions github-actions bot removed the backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. label Dec 6, 2023
@github-actions github-actions bot added the backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. label Dec 6, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Backport pending to v1.14 in 1.14.5 Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/v1.12 This issue affects v1.12 branch affects/v1.13 This issue affects v1.13 branch affects/v1.14 This issue affects v1.14 branch backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loopback CNI is not copied atomically
4 participants