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

bump iptables version to v1.8.8 #8416

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

cyclinder
Copy link
Contributor

Description

Currently, the iptables version of calico-node is 1.8.4, The iptables-nft-save -t raw are incompatible. This patch will bump iptables to 1.8.8, which can solve the incompatible issue.

Related issues/PRs

fixes #8403

Todos

  • Tests
  • Documentation
  • Release note

Release Note

Bump iptables version of calico-node to 1.8.8

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

  • docs-pr-required: This change requires a change to the documentation that has not been completed yet.
  • docs-completed: This change has all necessary documentation completed.
  • docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

  • release-note-required: This PR has user-facing changes. Most PRs should have this label.
  • release-note-not-required: This PR has no user-facing changes.

Other optional labels:

  • cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
  • needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

@cyclinder cyclinder requested a review from a team as a code owner January 18, 2024 02:20
@marvin-tigera marvin-tigera added this to the Calico v3.28.0 milestone Jan 18, 2024
@marvin-tigera marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Jan 18, 2024
@cyclinder cyclinder marked this pull request as draft January 18, 2024 02:20
@cyclinder cyclinder force-pushed the bump_iptables_v1.8.9 branch 2 times, most recently from ac1615b to ec3272a Compare January 18, 2024 03:12
@fasaxc
Copy link
Member

fasaxc commented Jan 18, 2024

/sem-approve

@fasaxc
Copy link
Member

fasaxc commented Jan 18, 2024

Thanks a lot @cyclinder, I've kicked off CI.

@fasaxc
Copy link
Member

fasaxc commented Jan 18, 2024

Failing with this error:

#45 ERROR: executor failed running [/bin/sh -c /clean-up-filesystem.sh]: exit code: 1
------
 > [ubi 25/26] RUN /clean-up-filesystem.sh:
#45 10.22 warning: file /usr/share/locale/en_GB/LC_MESSAGES/chkconfig.mo: remove failed: No such file or directory
#45 10.22 warning: file /usr/lib/systemd/systemd-sysv-install: remove failed: No such file or directory
#45 10.50 Failed to disable unit, unit systemd-readahead-replay.service does not exist.
#45 10.50 Failed to disable unit, unit systemd-readahead-collect.service does not exist.
#45 10.53 warning: file /etc/rc.local: remove failed: No such file or directory
#45 10.83 install-info: No such file or directory for /usr/share/info/nettle.info
#45 10.89 install-info: No such file or directory for /usr/share/info/history.info
#45 10.89 install-info: No such file or directory for /usr/share/info/rluserman.info
#45 11.01 warning: file /usr/share/locale/en_GB/LC_MESSAGES/p11-kit.mo: remove failed: No such file or directory
#45 11.85 Binary is missing after RPM cleanup: /usr/sbin/ip6tables
------
error: failed to solve: executor failed running [/bin/sh -c /clean-up-filesystem.sh]: exit code: 1

@fasaxc fasaxc added docs-not-required Docs not required for this change and removed docs-pr-required Change is not yet documented labels Jan 18, 2024
@cyclinder
Copy link
Contributor Author

cyclinder commented Jan 19, 2024

Thanks @fasaxc

Failing with this error:

Yeah, This is the issue I mentioned in #8403 (comment), and I'm trying to fix it now..., so this PR is in draft state. As soon as I fix it, I'll mark it as ready.

@cyclinder
Copy link
Contributor Author

hey @matthewdupre @mazdakn Could you mind give me some advices? curently, I'm stuck at this point. Any suggestions would be appreciated. Thanks!

@cyclinder
Copy link
Contributor Author

cyclinder commented Jan 23, 2024

➜  node git:(6503dfeb6) ✗ docker run --rm -it a21da52eb5b1 bash
[root@73874e0b42fe /]# ls /usr/sbin/ip6tables
/usr/sbin/ip6tables
[root@73874e0b42fe /]# 

[root@ca5c2fb949c6 /]# /usr/sbin/iptables
iptables v1.8.8 (nf_tables): no command specified
Try `iptables -h' or 'iptables --help' for more information.
[root@ca5c2fb949c6 /]# /usr/sbin/ip6tables 
bash: /usr/sbin/ip6tables: No such file or directory
[root@ca5c2fb949c6 /]# ^C
[root@ca5c2fb949c6 /]# cd /etc/alternatives/
[root@ca5c2fb949c6 alternatives]# ls
arptables         arptables-restore      arptables-save-man  ebtables-restore  ip6tables-restore  iptables-restore      python
arptables-helper  arptables-restore-man  ebtables            ebtables-save     ip6tables-save     iptables-save         unversioned-python-man
arptables-man     arptables-save         ebtables-man        ip6tables         iptables           libnssckbi.so.x86_64
[root@ca5c2fb949c6 alternatives]# ./ip6tables
bash: ./ip6tables: No such file or directory
[root@ca5c2fb949c6 alternatives]# ls -l
total 0
lrwxrwxrwx 1 root root 23 Jan 23 13:14 arptables -> /usr/sbin/arptables-nft
lrwxrwxrwx 1 root root 33 Jan 23 13:14 arptables-helper -> /usr/libexec/arptables-nft-helper
lrwxrwxrwx 1 root root 38 Jan 23 13:14 arptables-man -> /usr/share/man/man8/arptables-nft.8.gz
lrwxrwxrwx 1 root root 31 Jan 23 13:14 arptables-restore -> /usr/sbin/arptables-nft-restore
lrwxrwxrwx 1 root root 46 Jan 23 13:14 arptables-restore-man -> /usr/share/man/man8/arptables-nft-restore.8.gz
lrwxrwxrwx 1 root root 28 Jan 23 13:14 arptables-save -> /usr/sbin/arptables-nft-save
lrwxrwxrwx 1 root root 43 Jan 23 13:14 arptables-save-man -> /usr/share/man/man8/arptables-nft-save.8.gz
lrwxrwxrwx 1 root root 22 Jan 23 13:14 ebtables -> /usr/sbin/ebtables-nft
lrwxrwxrwx 1 root root 37 Jan 23 13:14 ebtables-man -> /usr/share/man/man8/ebtables-nft.8.gz
lrwxrwxrwx 1 root root 30 Jan 23 13:14 ebtables-restore -> /usr/sbin/ebtables-nft-restore
lrwxrwxrwx 1 root root 27 Jan 23 13:14 ebtables-save -> /usr/sbin/ebtables-nft-save
lrwxrwxrwx 1 root root 26 Jan 23 13:14 ip6tables -> /usr/sbin/ip6tables-legacy
lrwxrwxrwx 1 root root 31 Jan 23 13:14 ip6tables-restore -> /usr/sbin/ip6tables-nft-restore
lrwxrwxrwx 1 root root 28 Jan 23 13:14 ip6tables-save -> /usr/sbin/ip6tables-nft-save
lrwxrwxrwx 1 root root 22 Jan 23 13:14 iptables -> /usr/sbin/iptables-nft
lrwxrwxrwx 1 root root 30 Jan 23 13:14 iptables-restore -> /usr/sbin/iptables-nft-restore
lrwxrwxrwx 1 root root 27 Jan 23 13:14 iptables-save -> /usr/sbin/iptables-nft-save
lrwxrwxrwx 1 root root 34 Mar 28  2023 libnssckbi.so.x86_64 -> /usr/lib64/pkcs11/p11-kit-trust.so
lrwxrwxrwx 1 root root 22 Jan 17 09:46 python -> /usr/libexec/no-python
lrwxrwxrwx 1 root root 43 Jan 17 09:46 unversioned-python-man -> /usr/share/man/man1/unversioned-python.1.gz
[root@ca5c2fb949c6 alternatives]# ls /usr/sbin/

In fact we can be sure that ip6tables is there, but I wonder why this script is complaining that it has been deleted. but why
lrwxrwxrwx 1 root root 26 Jan 23 13:14 ip6tables -> /usr/sbin/ip6tables-legacy ?

[root@ca5c2fb949c6 alternatives]# ls /usr/sbin/ip6tables-legacy
ls: cannot access '/usr/sbin/ip6tables-legacy': No such file or directory

@coutinhop
Copy link
Contributor

@cyclinder ip6tables is present as a symlink to /usr/sbin/ip6tables-legacy, which is not found, right? On #8403 @defo89 mentioned that the "legacy case is not covered". Is ip6tables-legacy included in version 1.8.4 (what we currently have), but not in 1.8.9? I wonder if there's an extra package that needs to be installed that will include it

@cyclinder
Copy link
Contributor Author

ip6tables is present as a symlink to /usr/sbin/ip6tables-legacy, which is not found, right? On #8403 @defo89 mentioned that the "legacy case is not covered". Is ip6tables-legacy included in version 1.8.4 (what we currently have), but not in 1.8.9?

@coutinhop Yes, We have not found ip6tables-legacy in 1.8.8

I wonder if there's an extra package that needs to be installed that will include it

I'm more inclined to say that the symlink is incorrect, and ip6tables-nft is there. so it should be ip6tables -> /usr/sbin/ip6tables-nft?

@fasaxc
Copy link
Member

fasaxc commented Jan 24, 2024

It think the whole point of that custom build logic is to build the legacy and nft versions of iptables. Normally, RPMs contain one or the other with the same name iptables but we need to install both and select at runtime.

@cyclinder
Copy link
Contributor Author

So it seems that iptables-legacy is not included in our custom compiled iptables...Or it needs to be compiled separately

@matthewdupre
Copy link
Member

We may need to get iptables legacy from a different source to iptables-nft - I think that's OK, and probably inevitable given the deprecation progress of iptables.

@cyclinder
Copy link
Contributor Author

thanks @matthewdupre .yeah, We really should build both legacy and NFT and then let Runtime choose which one to use.
I verified the latest changes locally for me, It works well.

@fasaxc Can you help trigger the CI and then see what happens with the CI?

@cyclinder cyclinder marked this pull request as ready for review January 31, 2024 06:36
@mazdakn
Copy link
Member

mazdakn commented Jan 31, 2024

/sem-approve

Comment on lines 41 to 42
ARG LIBNFTNL_SOURCERPM_URL=${CENTOS_MIRROR_BASE_URL}/BaseOS/Source/SPackages/libnftnl-${LIBNFTNL_VER}.el9.src.rpm
ARG IPTABLES_SOURCERPM_URL=${CENTOS_MIRROR_BASE_URL}/BaseOS/Source/SPackages/iptables-${IPTABLES_VER}.el9.src.rpm
Copy link
Contributor

Choose a reason for hiding this comment

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

arm64 build failed with a 404 on this package, changing the URL to match the amd64 build seems to fix it:

Suggested change
ARG LIBNFTNL_SOURCERPM_URL=${CENTOS_MIRROR_BASE_URL}/BaseOS/Source/SPackages/libnftnl-${LIBNFTNL_VER}.el9.src.rpm
ARG IPTABLES_SOURCERPM_URL=${CENTOS_MIRROR_BASE_URL}/BaseOS/Source/SPackages/iptables-${IPTABLES_VER}.el9.src.rpm
ARG LIBNFTNL_SOURCERPM_URL=${CENTOS_MIRROR_BASE_URL}/BaseOS/source/tree/Packages/libnftnl-${LIBNFTNL_VER}.el9.src.rpm
ARG IPTABLES_SOURCERPM_URL=${CENTOS_MIRROR_BASE_URL}/BaseOS/source/tree/Packages/iptables-${IPTABLES_VER}.el9.src.rpm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I forgot to commit the changes in Dockerfile.arm64, now let me resubmit them!

@coutinhop
Copy link
Contributor

/sem-approve

Copy link
Contributor

@coutinhop coutinhop left a comment

Choose a reason for hiding this comment

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

CI is green and image looks good:

$ make image
(...)
$ docker run -it --rm calico/node sh
sh-4.4# iptables --version
iptables v1.8.8 (legacy)
sh-4.4# iptables-nft --version
iptables v1.8.8 (nf_tables)

Thanks @cyclinder! 🎉

@coutinhop coutinhop merged commit d4896d8 into projectcalico:master Feb 2, 2024
2 checks passed
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
Development

Successfully merging this pull request may close these issues.

option to bump iptables to ≥1.8.8
6 participants