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

Use manual test to ensure iptables-* binaries are present #1880

Merged

Conversation

thomasferrandiz
Copy link
Contributor

@thomasferrandiz thomasferrandiz commented Feb 21, 2024

The sanity check in iptables-wrapper-installer.sh is important in case a version of the base image for some arch doesn't include all the needed iptables-related files.
But it fails for multi-arch build so we have to use a manual test.

Description

Todos

  • Tests
  • Documentation
  • Release note

Release Note

None required

The sanity check in iptables-wrapper-installer.sh doesn't work
for multi-arch images so we need to disable it.
@thomasferrandiz thomasferrandiz changed the title remove no-sanity-check flag when building flannel image Use manual test to ensure iptables-* binaries are present Feb 23, 2024
Copy link
Collaborator

@manuelbuil manuelbuil left a comment

Choose a reason for hiding this comment

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

why does it fail in multi-arch?

@@ -33,6 +33,9 @@ COPY --from=build /build/dist/flanneld /opt/bin/flanneld
COPY dist/mk-docker-opts.sh /opt/bin/
COPY --from=build /iptables-wrapper/iptables-wrapper-installer.sh /
COPY --from=build /iptables-wrapper/bin/iptables-wrapper /
# check manually that iptables-legacy and iptables-nft are present since
# iptables-wrapper-installer.sh sanity check doesn't work for multi-arch build
Copy link
Collaborator

Choose a reason for hiding this comment

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

but if you look at the last line, it says --no-sanity-check, are we really doing the sanity check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no because it doesn't work.
Instead I use which to check that iptables-nft and iptables-legacy exist in the image

@thomasferrandiz
Copy link
Contributor Author

in a multi-arch build, --no-sanity-check fails for all arch except amd64 even when iptables-nft and iptables-legacy are present

why does it fail in multi-arch?

the script actually tries to run iptables-legacy and iptables-nft.
In non amd64 arch, iptables-nft --version fails with protocol unsupported, I guess because we are using qemu.

@manuelbuil
Copy link
Collaborator

in a multi-arch build, --no-sanity-check fails for all arch except amd64 even when iptables-nft and iptables-legacy are present

why does it fail in multi-arch?

the script actually tries to run iptables-legacy and iptables-nft. In non amd64 arch, iptables-nft --version fails with protocol unsupported, I guess because we are using qemu.

Thanks for the explanation. Do you think it still makes sense to keep it then?

@thomasferrandiz
Copy link
Contributor Author

I wasn't clear above.
The script fails when no-sanity-check is not present, so we need to pass the flag to avoid running the test that fails in multi-arch build.

@thomasferrandiz thomasferrandiz merged commit 001b36d into flannel-io:master Feb 26, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants