Remove iptables >1.4.11 specific functionality #2067
Conversation
|
I am a little buried right now, but I promise I will get to this ASAP - I On Wed, Oct 29, 2014 at 7:37 PM, Haney Maxwell notifications@github.com
|
|
No worries! Thanks for looking at it. |
| for _, line := range strings.Split(string(out), "\n") { | ||
| // Check that this is a rule for the correct chain, and that it has | ||
| // the correct number of argument (+2 for "-A <chain name>") | ||
| if strings.HasPrefix(line, fmt.Sprintf("-A %s", string(chain))) && |
There was a problem hiding this comment.
Note: This whole function is doing roughly structural set equality. If there's a way to do set equality in golang that I'm missing, that would probably be more elegant.
There was a problem hiding this comment.
We have pkg/util/set.go which has StringSet. Maybe you can check the prefix, check the number of args, split this into a StringSet, and add a StringSet.Equal() function.
You have to check the length before StringSet because some argument can come through in duplicate (eg -m tcp -p tcp => [-m, -p, tcp]).
Even this is not quite right, because some args, like ! change the meaning of a rule.
-A KUBE-PROXY ! -d 10.0.0.1/32 -p tcp -m tcp --dport 443 -j REDIRECT --to-ports 34089
...is the same length and StringSet as...
-A KUBE-PROXY -d 10.0.0.1/32 -p tcp -m tcp ! --dport 34089 -j REDIRECT --to-ports 443
...but they mean very different things.
|
Just worked through how feasible it would be to upgrade our iptables with some of our ops guys in detail. It looks like it would be a fairly large project, due to incompatibilities between how RHEL6-style distros package iptables and how more recent distributions do, and a number of challenges around shared libraries. |
| // Check that this is a rule for the correct chain, and that it has | ||
| // the correct number of argument (+2 for "-A <chain name>") | ||
| if strings.HasPrefix(line, fmt.Sprintf("-A %s", string(chain))) && | ||
| len(strings.Split(line, " ")) == len(args)+2 { |
hmrm
force-pushed
the
iptables-compatibility
branch
from
08ce5ed
to
8b7a113
Compare
| func (runner *runner) checkRule(table Table, chain Chain, args ...string) (bool, error) { | ||
| checkPresent, err := getIptablesHasCheckCommand(runner.exec) | ||
| if err != nil { | ||
| return false, fmt.Errorf("Error checking iptables version: %v", err) |
There was a problem hiding this comment.
Is this desired behavior? We could instead have it default to using "-C" if the version check fails (e.g. if iptables decides to change it's --version output at some point in the future).
There was a problem hiding this comment.
I'm fine with falling back on assuming -C, but please do log something significant.
hmrm
force-pushed
the
iptables-compatibility
branch
2 times, most recently
from
6fe80e2
to
1b7480e
Compare
|
It looks like Travis succeeded with 1.3, failed with 1.2 on a connection issue with etcd. I'm guessing just flakiness? |
|
@thockin How does this look? |
| // Occasionally iptables-save will emit trailing whitespace, | ||
| // which is picked up as an empty flag by Split. | ||
| // This solves both that, and the potential of double spaces being emitted. | ||
| splitLineWithEmpties := strings.Split(line, " ") |
There was a problem hiding this comment.
There was a problem hiding this comment.
That's a really nice function! done.
hmrm
force-pushed
the
iptables-compatibility
branch
2 times, most recently
from
93394b3
to
b4378ab
Compare
hmrm
force-pushed
the
iptables-compatibility
branch
from
b4378ab
to
5d69faa
Compare
|
LGTM @lavalamp can commit in daytime hours, I am out the rest of this week. Thanks for persevering! |
|
Thanks for bearing with me despite the my weak golang! The day when I have kubernetes running in production draws ever closer! |
|
Thanks for this guys! 🙇 |
Remove iptables >1.4.11 specific functionality
|
yes thank you, can't wait to see this in 0.5.3 |
Scientific Linux 6 (and likely other similar distributions such as CentOS 6) ship with iptables 1.4.7, which doesn't support the -C flag.
Looking at examples of how other systems are handling this, Saltstack actually parses the help document to figure out if "--check" is present, and then either uses it (if present), or does something similar to this patch (if absent): https://github.com/saltstack/salt/blob/d50fdcb456ab69407610c5fa2abab68605af8084/salt/modules/iptables.py#L462
@thockin This is a continuation of #1979, now with ~100% more actually working, less bash, and a bit more testability.
I'm getting the same results for the e2e tests for this and master (using vagrant to run it): basic.sh and guestbook.sh passing and the others failing.
Edit: After some confusion, it looks like I'm mistaken about this requiring a kernel upgrade. I've removed that sentence.