Skip to content

server: ensure CNI teardown prevents IP leaks with missing netns #9372

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
Merged
merged 1 commit into from
Aug 5, 2025
Merged

server: ensure CNI teardown prevents IP leaks with missing netns #9372

merged 1 commit into from
Aug 5, 2025

Conversation

@sohankunkerkar
Copy link
Member

This fix leverages CNI cache files to retrieve network attachments and call CNI DEL when network namespace files are missing, preventing IP address leaks.

What type of PR is this?

/kind bug

What this PR does / why we need it:

This will fix the issue mentioned in #9301 (comment)

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

server: ensure CNI teardown prevents IP leaks with missing netns

@sohankunkerkar sohankunkerkar requested a review from mrunalp as a code owner July 31, 2025 14:11
@openshift-ci openshift-ci bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug Categorizes issue or PR as related to a bug. labels Jul 31, 2025
@sohankunkerkar
Copy link
Member Author

@danwinship could you PTAL?

@openshift-ci openshift-ci bot requested review from hasan4791 and klihub July 31, 2025 14:11
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2025
@codecov
Copy link

codecov bot commented Jul 31, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 63.33333% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.30%. Comparing base (17c08c8) to head (e1bf924).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #9372      +/-   ##
==========================================
+ Coverage   67.17%   67.30%   +0.13%     
==========================================
  Files         200      200              
  Lines       27680    27702      +22     
==========================================
+ Hits        18595    18646      +51     
+ Misses       7540     7510      -30     
- Partials     1545     1546       +1
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sohankunkerkar
Copy link
Member Author

https://github.com/cri-o/cri-o/actions/runs/16651679437/job/47126005482?pr=9372
This fix ensures CNI teardown is always called to prevent IP leaks, even when network namespaces are invalid/missing. However, this causes the above test to fail when the CNI plugin is temporarily unavailable (it looks like we need to fix the test?)

@sohankunkerkar sohankunkerkar force-pushed the fix-ip-leaks branch 3 times, most recently from e837b67 to c354bd9 Compare July 31, 2025 19:33
lance5890
lance5890 reviewed Aug 4, 2025
View reviewed changes
@sohankunkerkar
Copy link
Member Author

@cri-o/cri-o-maintainers PTAL

saschagrunert
saschagrunert reviewed Aug 4, 2025
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

Just a nit, otherwise LGTM.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 5, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saschagrunert, sohankunkerkar

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:
  • OWNERS [saschagrunert,sohankunkerkar]

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

@openshift-merge-bot openshift-merge-bot bot merged commit a9243f9 into cri-o:main Aug 5, 2025
73 of 74 checks passed
@sohankunkerkar
Copy link
Member Author

/cherry-pick release-1.33

@openshift-cherrypick-robot
Copy link

@sohankunkerkar: new pull request created: #9382

In response to this:

/cherry-pick release-1.33

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Aug 5, 2025
Merged
This was referenced Aug 8, 2025
Merged
Open
@sohankunkerkar sohankunkerkar mentioned this pull request Aug 28, 2025
Open
@sohankunkerkar sohankunkerkar mentioned this pull request Aug 28, 2025
Open
@github-actions github-actions bot mentioned this pull request Sep 14, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saschagrunert saschagrunert saschagrunert approved these changes

@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp is a code owner

@hasan4791 hasan4791 Awaiting requested review from hasan4791

@klihub klihub Awaiting requested review from klihub

+1 more reviewer

@lance5890 lance5890 lance5890 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@saschagrunert saschagrunert

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sohankunkerkar @openshift-cherrypick-robot @saschagrunert @lance5890