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

match https://github.com/kubernetes-sigs/iptables-wrappers/blob/maste… #2289

Merged
merged 2 commits into from
Jun 30, 2021

Conversation

BenTheElder
Copy link
Member

…r/iptables-wrapper-installer.sh selection logic

see: #2271 (comment)

this is one of the only deviations in functionality from the wrapper there.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 28, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder

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 requested review from krzyzacy and munnerz May 28, 2021 08:51
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 28, 2021
@BenTheElder
Copy link
Member Author

note: since I haven't bumped the base this doesn't change anything in CI, this is just for review. namely @aojea 🙃

@aojea
Copy link
Contributor

aojea commented May 28, 2021

yeah, Dan Winship make it work for the case of users that have both rules on the system

@BenTheElder
Copy link
Member Author

@BenTheElder
Copy link
Member Author

@aojea
Copy link
Contributor

aojea commented May 28, 2021

https://github.com/kubernetes/release/blob/0539f9b737ef056d6007b991bd25b92a8706eb7e/images/build/debian-iptables/buster/iptables-wrapper#L28-L38 it has not been updated in kube-proxy

I wonder if that may explain recent bugs open about kube-proxy delays on endpoints 🤔

@aojea
Copy link
Contributor

aojea commented May 31, 2021

@danwinship it seems that there are 2 detection modes:

  1. num lines legacy > 10
  2. num lines legacy > num lines nft

which one do you think is better?
https://github.com/kubernetes-sigs/iptables-wrappers/blob/97b01f43a8e8db07840fc4b95e833a37c0d36b12/iptables-wrapper-installer.sh#L112-L141

@danwinship
Copy link
Contributor

The version of the script in the iptables-wrappers repo is best. You ideally want to check num lines legacy > num lines nft because that's more accurate, but you don't want to do that if you have the buggy version of iptables-nft that will hang and need to be timed out and killed. So the installer in iptables-wrappers does the good way if you have a good iptables-nft binary, and the less good way if you have a bad binary

@BenTheElder
Copy link
Member Author

I filed kubernetes/release#2106 to correct the kube-proxy base image to the second version, we should be on iptables >= 1.8.5 there so we don't need the bug workaround version.

@BenTheElder
Copy link
Member Author

This PR also updates kind to that variant.

@aojea
Copy link
Contributor

aojea commented Jun 4, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2021
@BenTheElder
Copy link
Member Author

/retest

@aojea
Copy link
Contributor

aojea commented Jun 4, 2021

/test pull-kind-e2e-kubernetes-1-20

@BenTheElder
Copy link
Member Author

we should push new image(s) anyhow
/hold

@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 Jun 4, 2021
@BenTheElder BenTheElder force-pushed the iptables-selection branch from 90e6f7f to 9bbcde3 Compare June 5, 2021 00:05
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2021
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@BenTheElder
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2021
@BenTheElder
Copy link
Member Author

need to fix #2313 first

@BenTheElder
Copy link
Member Author

will rebase on #2320

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2021
@BenTheElder
Copy link
Member Author

BenTheElder commented Jun 29, 2021

thought we'd be landing other prioritized changes but since we're not yet, rebased this one.
edit: we did land #2320 though, which was pretty critical.

@BenTheElder
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 29, 2021
@BenTheElder BenTheElder added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 29, 2021
@BenTheElder
Copy link
Member Author

re-applying as this is just a rebase

@k8s-ci-robot k8s-ci-robot merged commit 8ca3399 into kubernetes-sigs:main Jun 30, 2021
@BenTheElder BenTheElder deleted the iptables-selection branch June 30, 2021 00:11
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants