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 iptables legacy mode when using buster #7381

Closed
wants to merge 2 commits into from

Conversation

zetaab
Copy link
Member

@zetaab zetaab commented Aug 6, 2019

fixes #7379

There are two ways to do this: nodeup OR insert in imagebuilder. However, at least I would like to have support in all buster images - that is why I implemented it to nodeup. Imagebuilder does not support platforms like openstack.

/assign @justinsb
/kind bug

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 6, 2019
@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 6, 2019
@zetaab
Copy link
Member Author

zetaab commented Aug 6, 2019

This needs cherrypick to 1.14 and 1.13.

@zetaab
Copy link
Member Author

zetaab commented Aug 6, 2019

/test pull-kops-e2e-kubernetes-aws

@zetaab
Copy link
Member Author

zetaab commented Aug 9, 2019

@justinsb could you check this PR before doing next 1.14 beta release. This is needed there

@zetaab
Copy link
Member Author

zetaab commented Aug 14, 2019

/assign @mikesplain

could you Mike check this?

@justinsb
Copy link
Member

As I commented on the issue, this is indeed problematic.

I do wonder whether nodeup runs in time. It's presumably possible for something to beat nodeup and run some iptables commands (for example, if docker was preinstalled), which based on reading the k8s issue would result in problems also.

Going to seek clarification on the k8s issue (kubernetes/kubernetes#71305)...

@zetaab
Copy link
Member Author

zetaab commented Aug 16, 2019

I actually have been using buster image by adding these commands to IG additionalUserData. I think additionalUserData is executed earlier than this firewall.go?

  additionalUserData:
  - name: busterfix.sh
    type: text/x-shellscript
    content: |
      #!/bin/sh
      update-alternatives --set iptables /usr/sbin/iptables-legacy
      update-alternatives --set ip6tables /usr/sbin/ip6tables-legacy
      update-alternatives --set arptables /usr/sbin/arptables-legacy
      update-alternatives --set ebtables /usr/sbin/ebtables-legacy

@zetaab
Copy link
Member Author

zetaab commented Sep 8, 2019

@justinsb any progress in this?

@nvanheuverzwijn
Copy link
Contributor

After reading carefully what the problem is, I will try to explain to newcomers what the problem is.

iptables binary is the center of the problem. iptables binary can either modify the kernel nf_table or ip_table but never both. Modifying both is undefined behavior from what I understand.

Debian stretch uses iptables v 1.6 which modifies kernel ip_table. Debian buster uses iptables 1.8 which modify nf_table.

The problem is this : If your host is under debian buster, docker daemon, kubelet, etc (every binary that run on the host) will use the host's iptables, which is 1.8, which modifies kernel nf_table. At this point, everything is all fine. However, when pods, kube-proxy in this case, required by kube-system starts, it uses iptable version 1.6 which modify your host ip_tables and not nf_table which introduce undefined behavior.

TL;DR; using a host OS with iptable version 1.8 coupled with pods using iptable version 1.6 results in buggy kubernetes cluster.

Right now, @zetaab solution is what seem to be the correct solution. It is also what is being documented on kubernetes website (see https://github.com/kubernetes/website/pull/16271/files) usage of debian buster. Because there is no easy solution for this, the strategy seems to be asking k8s operators to document how to update the host OS that uses iptable1.8

@nvanheuverzwijn
Copy link
Contributor

@zetaab
This might be interesting : kubernetes/test-infra#14812
In these tests, he seems to only use iptables-legacy and ip6tables-legacy, not arptables-legacy or ebtables-legacy because "these are not installed" (see kubernetes/test-infra#14813) so maybe we should be careful with arptables and ebtables.

@nvanheuverzwijn
Copy link
Contributor

I actually have been using buster image by adding these commands to IG additionalUserData. I think additionalUserData is executed earlier than this firewall.go?

I think you are right. Kops add things in userdata to download and configure nodeup I think. Using additionalUserData is the way to go in my opinion. Is it the earliest we can do before any iptables command gets fired ?

@hakman
Copy link
Member

hakman commented Nov 4, 2019

I did some testing using the official Debian images from https://wiki.debian.org/Cloud/AmazonEC2Image/Buster.
The arptables and ebtables packages were not installed by default.

May be a good idea to also make sure that arptables and ebtables are installed during setup.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 27, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zetaab

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

The pull request process is described 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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 27, 2019
@zetaab
Copy link
Member Author

zetaab commented Nov 27, 2019

/hold

it might be that this is not needed anymore in kube 1.17 kubernetes/kubernetes#82966

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2019
@k8s-ci-robot
Copy link
Contributor

@zetaab: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kops-verify-staticcheck 65bbbda link /test pull-kops-verify-staticcheck

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@olemarkus
Copy link
Member

Cilium has no plans for supporting nftables anytime soon. So cilium users will need something like this anyway. Would be nice if this one goes in, but perhaps add an API field controlling if legacy or nft iptables should be used.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 3, 2020
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 3, 2020
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. 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.

Debian buster iptables not compatible with k8s
8 participants