Skip to content

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
@akhilles akhilles requested a review from youngnick 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
@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 labels 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 labels Dec 5, 2023
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Dec 6, 2023
skmatti pushed a commit to skmatti/cilium that referenced this pull request Jul 24, 2024
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".

Bug: b/310226307
Upstream-PR: cilium#29462
Change-Id: I24fddcc5e271063edf8f30ca58d8738c75af63df
Signed-off-by: Akhil Velagapudi <4@4khil.com>
Reviewed-on: https://gke-internal-review.googlesource.com/c/third_party/cilium/+/888710
Tested-by: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
Unit-Verified: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
Reviewed-by: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
Reviewed-by: Akhil Velagapudi <avelagap@google.com>
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
No open projects
Status: Released
Development

Successfully merging this pull request may close these issues.

Loopback CNI is not copied atomically
4 participants