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 unchecked error in datapath/linux/ipsec #29423

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Fix unchecked error in datapath/linux/ipsec #29423

merged 1 commit into from
Jan 23, 2024

Conversation

derailed
Copy link
Contributor

@derailed derailed commented Nov 27, 2023 •
edited by pchaigno
Loading

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

Fix unchecked error in datapath/linux/ipsec.go

The following calls are unchecked:

  • deleteNodeIPSecOutRoute
  • replaceNodeIPSecOutRoute

Fixes: #issue-number

Signed-off-by: Fernand Galiana [email protected]

Fix bug that could cause IPsec route change failures to be silent.

@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 27, 2023
@derailed derailed force-pushed the pr/derailed/fix_uncaught_err branch 3 times, most recently from 20e4758 to 933df35 Compare November 27, 2023 23:01
@derailed
Copy link
Contributor Author

/test

@derailed derailed mentioned this pull request Nov 29, 2023
Merged
8 tasks
@derailed derailed marked this pull request as ready for review November 29, 2023 21:32
@derailed derailed requested a review from a team as a code owner November 29, 2023 21:32
@derailed derailed requested a review from pchaigno November 29, 2023 21:32
@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Dec 30, 2023
@derailed derailed removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jan 9, 2024
@derailed derailed force-pushed the pr/derailed/fix_uncaught_err branch 2 times, most recently from 4808f7d to e469205 Compare January 10, 2024 16:42
@derailed
Copy link
Contributor Author

/test

@derailed
Fix unchecked error in datapath/linux/ipsec
bde53b6 
Ensure errs are checked for the calls below:

- deleteNodeIPSecOutRoute
- replaceNodeIPSecOutRoute

Signed-off-by: Fernand Galiana <[email protected]>
@derailed derailed force-pushed the pr/derailed/fix_uncaught_err branch from e469205 to bde53b6 Compare January 12, 2024 17:33
@derailed
Copy link
Contributor Author

/test

pchaigno
pchaigno approved these changes Jan 15, 2024
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Small proposed improvement for the error message. Otherwise LGTM.

pkg/datapath/linux/ipsec.go
@@ -178,7 +178,9 @@ func (n *linuxNodeHandler) enableIPsecIPv4(newNode *nodeTypes.Node, nodeID uint1
}
} else {
remoteCIDR := newNode.IPv4AllocCIDR.IPNet
n.replaceNodeIPSecOutRoute(remoteCIDR)
if err := n.replaceNodeIPSecOutRoute(remoteCIDR); err != nil {
errs = errors.Join(errs, fmt.Errorf("failed to replace ipsec OUT (%q): %w", remoteCIDR.IP, err))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe failed to replace egress route to %q for IPsec: %w?

@pchaigno pchaigno added release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Jan 15, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jan 15, 2024
@pchaigno pchaigno added area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jan 15, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jan 15, 2024
@julianwiedmann julianwiedmann added the feature/ipsec Relates to Cilium's IPsec feature label Jan 18, 2024
@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 Jan 18, 2024
@squeed squeed merged commit 5b04e08 into cilium:main Jan 23, 2024
62 checks passed
@joamaki joamaki mentioned this pull request Jan 30, 2024
Merged
28 tasks
@joamaki joamaki added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. and removed needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Jan 30, 2024
@aanm aanm added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno approved these changes

Assignees
No one assigned
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. feature/ipsec Relates to Cilium's IPsec feature 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@derailed @pchaigno @squeed @joamaki @aanm @julianwiedmann