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

Fix the auto iptables detection if ip_tables.ko preloaded on RHEL/CentOS 8 #7111

Merged

Conversation

yankay
Copy link
Contributor

@yankay yankay commented Dec 20, 2022

Description

The auto iptables detection logic is updated by the Kubernetes KEP 3178 , the 'iptables-wrapper' can decide which iptables backend to use based on which one kubelet used.

So that the issue can be fixed by the new 'iptables-wrapper' logic. The PR is to update it with the new logic:

from:
https://github.com/kubernetes-sigs/iptables-wrappers/blob/v1/iptables-wrapper-installer.sh#L130
to:
https://github.com/kubernetes-sigs/iptables-wrappers/blob/master/iptables-wrapper-installer.sh#L107

Some additional information:

Related issues/PRs

fixes #2322
fixes #3709

Todos

  • Tests
  • Documentation
  • Release note

Release Note

Fix the auto iptables detection if ip_tables.ko preloaded on RHEL/CentOS 8

Reminder for the reviewer

  • cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.

@yankay yankay requested a review from a team as a code owner December 20, 2022 03:44
@marvin-tigera marvin-tigera added this to the Calico v3.25.0 milestone Dec 20, 2022
@marvin-tigera marvin-tigera added docs-pr-required Change is not yet documented release-note-required Change has user-facing impact (no matter how small) labels Dec 20, 2022
@CLAassistant
Copy link

CLAassistant commented Dec 20, 2022

CLA assistant check
All committers have signed the CLA.

@yankay yankay force-pushed the fix-the-auto-iptables-detection branch from bf58af5 to b292088 Compare December 20, 2022 03:49
@cyclinder
Copy link
Contributor

Hi @caseydavenport @song-jiang , can you please take a look ? Thanks, I think it's make a sense. 😄

@mgleung mgleung modified the milestones: Calico v3.25.0, Calico v3.26.0 Jan 5, 2023
@yankay
Copy link
Contributor Author

yankay commented Jan 12, 2023

HI @mazdakn, would you please review the PR :-)

@mazdakn
Copy link
Member

mazdakn commented Jan 12, 2023

@yankay sure, I'll review it in the next few days.

@yankay
Copy link
Contributor Author

yankay commented Jan 12, 2023

@yankay sure, I'll review it in the next few days.

Thank you very much

Copy link
Member

@fasaxc fasaxc left a comment

Choose a reason for hiding this comment

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

Using the hints seems sensible but I don't think we should change the defaulting behaviour if no hints are detected. I notice that a bunch of the error tests have been flipped to default to nft now, for example but I'd expect that if iptbales-nft fails (presumably because we're on a kernel with no nftables support!) then we should default to legacy mode.

@yankay yankay force-pushed the fix-the-auto-iptables-detection branch from b292088 to b390e1e Compare January 16, 2023 09:23
@yankay
Copy link
Contributor Author

yankay commented Jan 16, 2023

Thank you very much @fasaxc ,
The PR has changed the default from the NTF to the legacy mode at https://github.com/projectcalico/calico/pull/7111/files#diff-95c897a9aab2c42c0e5ccfa753ed1caa528d8736e94699740f9d1f3d00b408e9R330

Would you please review it again :-)

@yankay yankay requested a review from fasaxc January 28, 2023 03:24
@fasaxc
Copy link
Member

fasaxc commented Jan 31, 2023

/sem-approve

Copy link
Member

@fasaxc fasaxc left a comment

Choose a reason for hiding this comment

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

LGTM. Noticed there was a change to a couple of existing tests due to the removal of the

if legacyLines >= 10

thanks that's OK now that nft is widely adopted

@yankay
Copy link
Contributor Author

yankay commented Feb 1, 2023

LGTM. Noticed there was a change to a couple of existing tests due to the removal of the

if legacyLines >= 10

thanks that's OK now that nft is widely adopted

Thank you very much for the review :-)

@cyclinder
Copy link
Contributor

cyclinder commented Feb 2, 2023

Hi @fasaxc , ci/semaphoreci/pr is falling, Does this affect the PR merge? What else is needed to merge into this PR?

Thanks.

@mgleung
Copy link
Contributor

mgleung commented Mar 1, 2023

/sem-approve

@mgleung mgleung added the docs-not-required Docs not required for this change label Mar 1, 2023
@marvin-tigera marvin-tigera removed the docs-pr-required Change is not yet documented label Mar 1, 2023
@mgleung mgleung merged commit fa4d1b1 into projectcalico:master Mar 8, 2023
@yankay
Copy link
Contributor Author

yankay commented Mar 9, 2023

Thanks@mazdakn @fasaxc @mgleung @cyclinder for the PR review :-)

marvin-tigera added a commit that referenced this pull request Apr 25, 2023
…rigin-release-v3.25

Automated cherry pick of #7111: Fix the auto iptables detection if ip_tables.ko preloaded on RHEL/CentOS 8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required Docs not required for this change release-note-required Change has user-facing impact (no matter how small)
Projects
None yet
7 participants