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

Look for update-alternatives before using it #1640

Closed
wants to merge 1 commit into from

Conversation

afbjorklund
Copy link
Contributor

It is only used with dpkg, not with other systems such as
buildroot (that already has iptables legacy as the default)

I experimented with doing an image based on "buildroot",
which uses busybox rather than ubuntu for the packages.

See kubernetes/minikube#6942

Also had issues with "hostname --ip-address", but it seems gone

It is only used with dpkg, not with other systems such as
buildroot (that already has iptables legacy as the default)
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 31, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @afbjorklund. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: afbjorklund
To complete the pull request process, please assign munnerz
You can assign the PR to them by writing /assign @munnerz in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot requested review from aojea and krzyzacy May 31, 2020 15:58
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 31, 2020
@BenTheElder
Copy link
Member

busybox is broken with kubernetes last I checked, e.g. mount works improperly ...

It seems like it would also be possible to place a fake / custom update-alternatives in this modified image.

I'd prefer not to silently not update the iptables to match, failing to do so is a bug and will cause problems without surfacing them as obviously.

@afbjorklund
Copy link
Contributor Author

The image was based on the new minikube.iso, if you want to check it out...

Basically we are doing the equivalent of update-alternatives at build time.

@afbjorklund
Copy link
Contributor Author

afbjorklund commented May 31, 2020

busybox is broken with kubernetes last I checked, e.g. mount works improperly ...

This was only for hosting the image, kubernetes itself runs inside the kindbase/kicbase

EDIT: thinko, but minikube does run kubernetes already so that buildroot config should be ok

It seems like it would also be possible to place a fake / custom update-alternatives in this modified image.

That would work too, we could also do a different check if better (look for symlink etc)

ln -s /bin/true /usr/sbin/update-alternatives

@afbjorklund
Copy link
Contributor Author

afbjorklund commented May 31, 2020

busybox is broken with kubernetes last I checked, e.g. mount works improperly ...

This was only for hosting the image, kubernetes itself runs inside the kindbase/kicbase

Hmm, wrote too fast...

I think we replaced some of the busybox with other variants, like util-linux and friends.

This was indeed intended for the image itself, too many layers to keep them apart :-)

@afbjorklund
Copy link
Contributor Author

afbjorklund commented May 31, 2020

Note that we are still running the docker containers with ubuntu image, just like kind.

This was an experiment, to see if we could have the same OS for both VM and KIC

For minikube it would make maintenance easier, by having less default OS variants...

Another approach would be to make an Ubuntu-based VM, but it would increase footprint

@aojea
Copy link
Contributor

aojea commented May 31, 2020

@afbjorklund
Copy link
Contributor Author

I suppose a fedora-based image is also a possibility, in the sake of diversity (like podman)

@BenTheElder
Copy link
Member

I suppose a fedora-based image is also a possibility, in the sake of diversity (like podman)

That seems like an enormous waste of resources. The OS of the image is an implementation detail, we're definitely not going to maintain against multiple OS-es for the image contents.

As noted in #1640 (comment) this behavior was considered and rejected in the upstream iptables-wrapper, and as mentioned previously failing to set this is a major bug.
Failing to implement this is going to make the image unusable on many hosts soon. Plenty of users are sticking to legacy mode for compatibility, but most large distros are shipping nft mode by default in their stable releases.

@afbjorklund
Copy link
Contributor Author

I suppose a fedora-based image is also a possibility, in the sake of diversity (like podman)

That seems like an enormous waste of resources. The OS of the image is an implementation detail, we're definitely not going to maintain against multiple OS-es for the image contents.

In one aspect you are right, both podman and cri-o are an enormous waste of resources...
It would be much easier to just ignore the CRI and run everything on docker and containerd.

And so far we haven't added any infrastructure for actually testing on CentOS (or Fedora)
So it just breaks as a part of the so called "none" driver, and we let kubeadm worry about it.

There are multiple OS for minikube being used at the moment, but we might change that soon.

But for this issue, I'm just going to add a fake update-alternatives script for Debian compat.

@afbjorklund afbjorklund deleted the busybox branch June 30, 2020 06:07
@afbjorklund
Copy link
Contributor Author

As noted in #1640 (comment) this behavior was considered and rejected in the upstream iptables-wrapper, and as mentioned previously failing to set this is a major bug.

Failing to implement this is going to make the image unusable on many hosts soon. Plenty of users are sticking to legacy mode for compatibility, but most large distros are shipping nft mode by default in their stable releases.

Right now it only has "legacy", but it turns out that it is simple to implement this "update-alternatives" once both binaries are present. It is just a question of where the /usr/sbin/iptables symlink is pointing to, whether it is xtables-legacy-multi or xtables-nft-multi

Just have to set BR2_PACKAGE_IPTABLES_NFTABLES=y, in order to actually build it without --disable-nftables. But then it should be trivial to support a Debian compability script, if you don't want to make it distribution-agnostic (like that code snippet)

https://github.com/kubernetes-sigs/iptables-wrappers/blob/5be13bb4f92d23b3bc963857b163105f00740a6e/iptables-wrapper-installer.sh#L144_L170

# Update links to point to the selected binaries
for cmd in iptables iptables-save iptables-restore ip6tables ip6tables-save ip6tables-restore; do
    rm -f "${sbin}/\${cmd}"
    ln -s "${sbin}/xtables-\${mode}-multi" "${sbin}/\${cmd}"
done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants