-
Notifications
You must be signed in to change notification settings - Fork 884
Fix iptables.Exists logic #925
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import ( | |
| "fmt" | ||
| "net" | ||
| "os/exec" | ||
| "regexp" | ||
| "strconv" | ||
| "strings" | ||
| "sync" | ||
|
|
@@ -36,6 +37,7 @@ const ( | |
| var ( | ||
| iptablesPath string | ||
| supportsXlock = false | ||
| supportsCOpt = false | ||
| // used to lock iptables commands if xtables lock is not supported | ||
| bestEffortLock sync.Mutex | ||
| // ErrIptablesNotFound is returned when the rule is not found. | ||
|
|
@@ -60,14 +62,19 @@ func (e ChainError) Error() string { | |
| } | ||
|
|
||
| func initCheck() error { | ||
|
|
||
| if iptablesPath == "" { | ||
| path, err := exec.LookPath("iptables") | ||
| if err != nil { | ||
| return ErrIptablesNotFound | ||
| } | ||
| iptablesPath = path | ||
| supportsXlock = exec.Command(iptablesPath, "--wait", "-L", "-n").Run() == nil | ||
| mj, mn, mc, err := GetVersion() | ||
| if err != nil { | ||
| logrus.Warnf("Failed to read iptables version: %v", err) | ||
| return nil | ||
| } | ||
| supportsCOpt = supportsCOption(mj, mn, mc) | ||
| } | ||
| return nil | ||
| } | ||
|
|
@@ -299,20 +306,19 @@ func Exists(table Table, chain string, rule ...string) bool { | |
| table = Filter | ||
| } | ||
|
|
||
| // iptables -C, --check option was added in v.1.4.11 | ||
| // http://ftp.netfilter.org/pub/iptables/changes-iptables-1.4.11.txt | ||
|
|
||
| // try -C | ||
| // if exit status is 0 then return true, the rule exists | ||
| if _, err := Raw(append([]string{ | ||
| "-t", string(table), "-C", chain}, rule...)...); err == nil { | ||
| return true | ||
| if supportsCOpt { | ||
| // if exit status is 0 then return true, the rule exists | ||
| _, err := Raw(append([]string{"-t", string(table), "-C", chain}, rule...)...) | ||
| return err == nil | ||
| } | ||
|
|
||
| // parse "iptables -S" for the rule (this checks rules in a specific chain | ||
| // in a specific table) | ||
| ruleString := strings.Join(rule, " ") | ||
| ruleString = chain + " " + ruleString | ||
| // parse "iptables -S" for the rule (it checks rules in a specific chain | ||
| // in a specific table and it is very unreliable) | ||
| return existsRaw(table, chain, rule...) | ||
| } | ||
|
|
||
| func existsRaw(table Table, chain string, rule ...string) bool { | ||
| ruleString := fmt.Sprintf("%s %s\n", chain, strings.Join(rule, " ")) | ||
| existingRules, _ := exec.Command(iptablesPath, "-t", string(table), "-S", chain).Output() | ||
|
|
||
| return strings.Contains(string(existingRules), ruleString) | ||
|
|
@@ -380,3 +386,25 @@ func ExistChain(chain string, table Table) bool { | |
| } | ||
| return false | ||
| } | ||
|
|
||
| // GetVersion reads the iptables version numbers | ||
| func GetVersion() (major, minor, micro int, err error) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this function ? It doesnt seem to be used in this PR ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, just in case we want to do a version check in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this again goes to the same discussion on checking for versions vs checking for functionality (we had a few such scenarios for kernel versions and we typically prefer the functionality check). @mrjana WDYT ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets remove this function. There is no use for this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok |
||
| out, err := Raw("--version") | ||
| if err == nil { | ||
| major, minor, micro = parseVersionNumbers(string(out)) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| func parseVersionNumbers(input string) (major, minor, micro int) { | ||
| re := regexp.MustCompile(`v\d*.\d*.\d*`) | ||
| line := re.FindString(input) | ||
| fmt.Sscanf(line, "v%d.%d.%d", &major, &minor, µ) | ||
| return | ||
| } | ||
|
|
||
| // iptables -C, --check option was added in v.1.4.11 | ||
| // http://ftp.netfilter.org/pub/iptables/changes-iptables-1.4.11.txt | ||
| func supportsCOption(mj, mn, mc int) bool { | ||
| return mj > 1 || (mj == 1 && (mn > 4 || (mn == 4 && mc >= 11))) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you adding the
\nto take care of theContainsmatching on partial TARGET ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where ? I dont see a partial TARGET match test here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at line 275 in the test diffs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mavenugo
Actually you are right, I modified the test code but the check at 275 is still testing the old logic.
Will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mavenugo
Took care of it, PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quite frankly, am not too comfortable depending on
\nas a mechanism to guarantee the comparison and it could result in false-negatives depending on the position of the rule itself (and various other unrelated dependencies).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking for the
\nis in fact to avoid a possible match on a rule subset.The existing raw exist check does nothing but looking for the rule string in the output of
iptables -t <table> -S <chain>, which is a list of\nterminated strings, where each rule follows this format:-<Action> <Chain> <rule args>\nGiven the rule format, checking if the byte stream contains
<rule>\nwill guarantee no subset can match.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with
\nas a delimiter, as k8s does https://github.com/kubernetes/kubernetes/blob/master/pkg/util/iptables/iptables.go#L417-L437