Conversation
There was a problem hiding this comment.
are you adding the \n to take care of the Contains matching on partial TARGET ?
There was a problem hiding this comment.
I added a test for it.
There was a problem hiding this comment.
where ? I dont see a partial TARGET match test here.
There was a problem hiding this comment.
Look at line 275 in the test diffs
There was a problem hiding this comment.
@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.
quite frankly, am not too comfortable depending on \n as 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.
Looking for the \n is 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 \n terminated strings, where each rule follows this format:
-<Action> <Chain> <rule args>\n
Given the rule format, checking if the byte stream contains <rule>\n will guarantee no subset can match.
There was a problem hiding this comment.
I'm ok with \n as a delimiter, as k8s does https://github.com/kubernetes/kubernetes/blob/master/pkg/util/iptables/iptables.go#L417-L437
- Fixed exists to attempt a raw exists check only when "iptables -C ..." execution returns error becasue of "unsupported option" - Fixed raw exists to not match substring - Added GetVersion method Signed-off-by: Alessandro Boch <aboch@docker.com>
|
@chenchun I think I took care of your comment. PTAL when you get a chance. Thanks. |
|
LGTM |
|
LGTM. @mrjana can you please confirm if you are okay for us to check the iptables version as we do in this PR ? I know we had a similar discussion regarding checking for kernel version vs functionality. |
|
Yes let's not do any static version checks on any software. Check for the functionality. If the check is made only for test code, the whole versioning check code can move to the *_test file. |
|
@mrjana The iptables support for the |
5 participants
Signed-off-by: Alessandro Boch aboch@docker.com