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

image/runtime: Update iptables-wrapper script #19937

Closed
wants to merge 2 commits into from
Closed

image/runtime: Update iptables-wrapper script #19937

wants to merge 2 commits into from

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented May 24, 2022 •
edited

Description

The original iptables-wrapper script is coming from 1, however,
this was spinned off to 2 in k8s upstream repo. This commit is
to get the latest iptables-wrapper script.

Signed-off-by: Tam Mach tam.mach@cilium.io

Testing

This is spin-off from #19852 so that we can decide to backports these changes (e.g. ubuntu upgrade vs iptables-wrapper update) independently.

@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 May 24, 2022
@sayboras sayboras temporarily deployed to release-base-images May 24, 2022 11:13 Inactive
@sayboras sayboras temporarily deployed to release-base-images May 24, 2022 11:13 Inactive
@sayboras sayboras added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label May 24, 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 May 24, 2022
@sayboras sayboras temporarily deployed to release-base-images May 24, 2022 11:21 Inactive
@sayboras sayboras temporarily deployed to release-base-images May 24, 2022 11:21 Inactive
@sayboras sayboras force-pushed the tam/update-iptables-wrapper branch from 35afe21 to adc6ad8 Compare May 24, 2022 11:48
@sayboras sayboras temporarily deployed to release-base-images May 24, 2022 11:48 Inactive
@sayboras sayboras temporarily deployed to release-base-images May 24, 2022 11:48 Inactive
@sayboras sayboras marked this pull request as ready for review May 24, 2022 11:50
@sayboras sayboras requested review from a team as code owners May 24, 2022 11:50
@sayboras
Copy link
Member Author

/test

@sayboras
Copy link
Member Author

/test-runtime

@sayboras sayboras mentioned this pull request May 27, 2022
Closed
@sayboras
image/runtime: Update iptables-wrapper script
c5c20dc 
The original iptables-wrapper script is coming from [1], however,
this was spinned off to [2] in k8s upstream repo. This commit is
to get the latest iptables-wrapper script.

[1]: kubernetes/kubernetes#82966
[2]: https://github.com/kubernetes-sigs/iptables-wrappers/blob/master/iptables-wrapper-installer.sh
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras sayboras force-pushed the tam/update-iptables-wrapper branch from adc6ad8 to a521b3a Compare May 31, 2022 09:03
@sayboras sayboras temporarily deployed to release-base-images May 31, 2022 09:04 Inactive
@sayboras sayboras temporarily deployed to release-base-images May 31, 2022 09:04 Inactive
@sayboras
update cilium-{runtime,builder}
f52acd5 
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras sayboras force-pushed the tam/update-iptables-wrapper branch from a521b3a to f52acd5 Compare May 31, 2022 09:14
@sayboras sayboras temporarily deployed to release-base-images May 31, 2022 09:14 Inactive
@sayboras sayboras temporarily deployed to release-base-images May 31, 2022 09:14 Inactive
@sayboras
Copy link
Member Author

/test

christarazi
christarazi reviewed May 31, 2022
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

Changes LGTM, I'm not sure how useful it is to review the actual iptables script. Do we feel that CI will give us a good enough signal to avoid regressions? Do we still feel this is risky?

@tommyp1ckles
Copy link
Contributor

What's the context for why the script was changed upstream?

@sayboras
Copy link
Member Author

sayboras commented Jun 8, 2022

What's the context for why the script was changed upstream?

Sorry for late reply. As per my understanding, this script was not maintained and updated for quite sometime. The recent update in upstream is to keep up with latest k8s deployment:

  • most of k8s deployment is with linux distribution with recent iptables version (e.g. 1.8.7)
  • NFT mode is preferred compared to legacy mode.

https://github.com/kubernetes-sigs/iptables-wrappers/commits/master/iptables-wrapper-installer.sh

@sayboras
Copy link
Member Author

sayboras commented Jun 8, 2022 •
edited

Changes LGTM, I'm not sure how useful it is to review the actual iptables script. Do we feel that CI will give us a good enough signal to avoid regressions? Do we still feel this is risky?

I think CI is giving us some confidence for sure, but it's hard to have complete confidence on iptables 😢 . Can we have it as part of 1.12 release, and then will backport to older version if required ? This is based on my assumption that users who plan to use 1.12 will most likely to have recent OS/Kernel/k8s versions, so the risk is becoming less.

@sayboras
Copy link
Member Author

sayboras commented Jun 9, 2022

Closed this one as we are considering other approach to make sure kubelet and cilium are using the same iptables mode.

@sayboras sayboras closed this Jun 9, 2022
@sayboras sayboras deleted the tam/update-iptables-wrapper branch November 1, 2022 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christarazi christarazi christarazi left review comments

@tommyp1ckles tommyp1ckles Awaiting requested review from tommyp1ckles tommyp1ckles was automatically assigned from cilium/build

Assignees
No one assigned
Labels
release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sayboras @tommyp1ckles @christarazi