-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
add a iptables failure detector, default off #19534
Conversation
tools/istio-iptables/pkg/cmd/root.go
Outdated
@@ -30,10 +32,13 @@ import ( | |||
var rootCmd = &cobra.Command{ | |||
Use: "istio-iptables", | |||
Long: "Script responsible for setting up port forwarding for Istio sidecar.", | |||
Run: func(cmd *cobra.Command, args []string) { | |||
RunE: func(cmd *cobra.Command, args []string) error { | |||
fmt.Println("lambdai") | |||
config := constructConfig() | |||
iptConfigurator := NewIptablesConfigurator(config) |
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.
This is not correct. You can't setup iptables when Istio CNI is enabled, because 1) this container won't run with CAP_NET_ADMIN, and 2) trying to initialize iptables a 2nd time after Istio CNI has already done it will fail.
So your validator has to be in another command, not in the root command.
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.
The idea is to reuse the configurator defined here.
Ideally
cni
executes the iptable setup in the kubelet contextistio-init
executes the validation in the preinit pod- for non-cni, run both
WDYT?
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.
- for non-cni, run both
Why? I don't understand why we would ever need to check an iptables setup that succeeded just before.
We would ever need to do one or the other. Which is why I recommended to do the validation in another command, to make that clear.
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 don't have strong opinion we must validate in non-cni.
But I'd like to ship iptables setup in one binary. Easy to maintain, and adding very little overhead to the binary size.
Isn't that a fact that istio-iptables is already running in both kubelet (for cni) context and istio-init(for non-cni)?
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.
But I'd like to ship iptables setup in one binary.
I never asked for another binary.
I asked for another Cobra command.
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.
@rlenglet Thanks!
I add --skip-rule-apply and --skip-validation.
See the summary
can you update the doc with what you want to do? |
@mandarjog I will update the doc.
Below is the log I got from validation server.
|
Signed-off-by: Yuchen Dai <silentdai@gmail.com> stash Signed-off-by: Yuchen Dai <silentdai@gmail.com> add iptables for test Signed-off-by: Yuchen Dai <silentdai@gmail.com> update v6 and host addr Signed-off-by: Yuchen Dai <silentdai@gmail.com> working get origin dst ipv4 and ipv6 Signed-off-by: Yuchen Dai <silentdai@gmail.com> refactor server Signed-off-by: Yuchen Dai <silentdai@gmail.com> add default validator to iptables command Signed-off-by: Yuchen Dai <silentdai@gmail.com> iptables failure detector in preinit Signed-off-by: Yuchen Dai <silentdai@gmail.com> replace native byte order detection Signed-off-by: Yuchen Dai <silentdai@gmail.com> add tests skip validation by default Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
0cf2aa1
to
5aa1e7a
Compare
@rlenglet What is the blocker for this PR? I disabled default detector by default in istio-iptables binary, so the cni should see no behavior change. The follow PRs will be utilizing it with complete e2e test |
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
/test unit-tests_istio |
The code LGTM. |
/retest integ-istioio-k8s-tests_istio |
/wait |
/retest integ-istioio-k8s-tests_istio |
@howardjohn it take a little bit more than 1 second. Actually the 1 second is the retry since the first request usually fail. I can cut the 1 second in the follow up PR.
|
1s is pretty high. Any time we add to startup is not just slower pod start
, for knative it is actually service latency
One of the benefits of cni is improved startup speed so I hope we don't
regress too much
…On Fri, Jan 10, 2020, 12:39 AM Yuchen Dai ***@***.***> wrote:
@howardjohn <https://github.com/howardjohn> it take a little bit more
than 1 second. Actually the 1 second is the retry since the first request
usually fail. I can cut the 1 second in the follow up PR.
$ time /usr/local/bin/istio-iptables --skip-rule-apply --run-validation ; done
in new validator: 10.12.1.50
Error connecting to 127.0.0.6:15002: dial tcp 127.0.0.1:0->127.0.0.6:15002: connect: connection refused
Listening on 127.0.0.1:15001
Listening on 127.0.0.1:15006
local addr 127.0.0.1:15006
original addr 127.0.0.1:15002
validation passed
real 0m1.101s
user 0m0.002s
sys 0m0.003s
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19534?email_source=notifications&email_token=AAEYGXPGNHQYMQH47HPT6GTQ5AX4JA5CNFSM4JZXTKGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEITDWJI#issuecomment-572930853>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEYGXJGT3XE2L2EQUOTA6TQ5AX4JANCNFSM4JZXTKGA>
.
|
Why does happy path take 1s? |
+1 to @mandarjog's comment. Why does this code ever try to open a connection before it has opened the server socket? Opening the server socket should be the first action. |
Another unfortunate side effect of this issue (not the pr specifically) is that cni may now be slower than non cni. CNi solution will have the same number of moving parts including scheduling the init container plus the time to execute cni initialization. |
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
@mandarjog @rlenglet @howardjohn Added a barrier: validation client runs only when server is ready to serve. I flush some further changes (reuseAddr) in this PR. Let me know if you think it should be excluded. |
Because validation is default off so knative is not impacted if the don't use cni (Do they?) |
People use CNI + knative as far as I know, I think I have seen some mentions of it before |
I ran this locally. ran it a couple 100 times, it seems very stable. 100 runs consistently takes 20s, so .2s each run |
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.
/lgtm
/test unit-tests_istio |
* cni validation Signed-off-by: Yuchen Dai <silentdai@gmail.com> stash Signed-off-by: Yuchen Dai <silentdai@gmail.com> add iptables for test Signed-off-by: Yuchen Dai <silentdai@gmail.com> update v6 and host addr Signed-off-by: Yuchen Dai <silentdai@gmail.com> working get origin dst ipv4 and ipv6 Signed-off-by: Yuchen Dai <silentdai@gmail.com> refactor server Signed-off-by: Yuchen Dai <silentdai@gmail.com> add default validator to iptables command Signed-off-by: Yuchen Dai <silentdai@gmail.com> iptables failure detector in preinit Signed-off-by: Yuchen Dai <silentdai@gmail.com> replace native byte order detection Signed-off-by: Yuchen Dai <silentdai@gmail.com> add tests skip validation by default Signed-off-by: Yuchen Dai <silentdai@gmail.com> * fix rootcmd flag Signed-off-by: Yuchen Dai <silentdai@gmail.com> * lint Signed-off-by: Yuchen Dai <silentdai@gmail.com> * go import Signed-off-by: Yuchen Dai <silentdai@gmail.com> * more lints Signed-off-by: Yuchen Dai <silentdai@gmail.com> * lintlintlint Signed-off-by: Yuchen Dai <silentdai@gmail.com> * go.mod go.sum by make gen Signed-off-by: Yuchen Dai <silentdai@gmail.com> * stash Signed-off-by: Yuchen Dai <silentdai@gmail.com> * stash2 Signed-off-by: Yuchen Dai <silentdai@gmail.com> * formatter Signed-off-by: Yuchen Dai <silentdai@gmail.com> * use simulate outbound server Signed-off-by: Yuchen Dai <silentdai@gmail.com> * ipv4,v6; server and client, both proxyport and inboundport Signed-off-by: Yuchen Dai <silentdai@gmail.com> * exit code Signed-off-by: Yuchen Dai <silentdai@gmail.com> * revert debug Signed-off-by: Yuchen Dai <silentdai@gmail.com> * address comment Signed-off-by: Yuchen Dai <silentdai@gmail.com> * gofmt -w -s Signed-off-by: Yuchen Dai <silentdai@gmail.com> * fix double run Signed-off-by: Yuchen Dai <silentdai@gmail.com> * revert RunOrFail Signed-off-by: Yuchen Dai <silentdai@gmail.com> * make gen Signed-off-by: Yuchen Dai <silentdai@gmail.com> * make gen make lint Signed-off-by: Yuchen Dai <silentdai@gmail.com> * resolve an error where it cannot be error Signed-off-by: Yuchen Dai <silentdai@gmail.com> * syscall reuseaddr Signed-off-by: Yuchen Dai <silentdai@gmail.com> * minor fix Signed-off-by: Yuchen Dai <silentdai@gmail.com> * save 1 second in iptable validation Signed-off-by: Yuchen Dai <silentdai@gmail.com> (cherry picked from commit ae817fc)
* Adding CNI repair to 1.4 * Fixing typo * Reverting some go.mod changes * More gomod changes * Addressing a test failure * Changing dangerous template whitespace deletion * Updating golden file. * Fixing another template error * add a iptables failure detector, default off (#19534) * cni validation Signed-off-by: Yuchen Dai <silentdai@gmail.com> stash Signed-off-by: Yuchen Dai <silentdai@gmail.com> add iptables for test Signed-off-by: Yuchen Dai <silentdai@gmail.com> update v6 and host addr Signed-off-by: Yuchen Dai <silentdai@gmail.com> working get origin dst ipv4 and ipv6 Signed-off-by: Yuchen Dai <silentdai@gmail.com> refactor server Signed-off-by: Yuchen Dai <silentdai@gmail.com> add default validator to iptables command Signed-off-by: Yuchen Dai <silentdai@gmail.com> iptables failure detector in preinit Signed-off-by: Yuchen Dai <silentdai@gmail.com> replace native byte order detection Signed-off-by: Yuchen Dai <silentdai@gmail.com> add tests skip validation by default Signed-off-by: Yuchen Dai <silentdai@gmail.com> * fix rootcmd flag Signed-off-by: Yuchen Dai <silentdai@gmail.com> * lint Signed-off-by: Yuchen Dai <silentdai@gmail.com> * go import Signed-off-by: Yuchen Dai <silentdai@gmail.com> * more lints Signed-off-by: Yuchen Dai <silentdai@gmail.com> * lintlintlint Signed-off-by: Yuchen Dai <silentdai@gmail.com> * go.mod go.sum by make gen Signed-off-by: Yuchen Dai <silentdai@gmail.com> * stash Signed-off-by: Yuchen Dai <silentdai@gmail.com> * stash2 Signed-off-by: Yuchen Dai <silentdai@gmail.com> * formatter Signed-off-by: Yuchen Dai <silentdai@gmail.com> * use simulate outbound server Signed-off-by: Yuchen Dai <silentdai@gmail.com> * ipv4,v6; server and client, both proxyport and inboundport Signed-off-by: Yuchen Dai <silentdai@gmail.com> * exit code Signed-off-by: Yuchen Dai <silentdai@gmail.com> * revert debug Signed-off-by: Yuchen Dai <silentdai@gmail.com> * address comment Signed-off-by: Yuchen Dai <silentdai@gmail.com> * gofmt -w -s Signed-off-by: Yuchen Dai <silentdai@gmail.com> * fix double run Signed-off-by: Yuchen Dai <silentdai@gmail.com> * revert RunOrFail Signed-off-by: Yuchen Dai <silentdai@gmail.com> * make gen Signed-off-by: Yuchen Dai <silentdai@gmail.com> * make gen make lint Signed-off-by: Yuchen Dai <silentdai@gmail.com> * resolve an error where it cannot be error Signed-off-by: Yuchen Dai <silentdai@gmail.com> * syscall reuseaddr Signed-off-by: Yuchen Dai <silentdai@gmail.com> * minor fix Signed-off-by: Yuchen Dai <silentdai@gmail.com> * save 1 second in iptable validation Signed-off-by: Yuchen Dai <silentdai@gmail.com> (cherry picked from commit ae817fc) * Fixing a few cherrypick mismerges * Changing the proxyv2 container to use the istio-iptables *binary* * Adding istio-iptables.sh back as default * Making istio-iptables binary change conditional * Typo * Fixing the distroless container Co-authored-by: Yuchen Dai <silentdai@gmail.com>
* Adding CNI repair to 1.4 * Fixing typo * Reverting some go.mod changes * More gomod changes * Addressing a test failure * Changing dangerous template whitespace deletion * Updating golden file. * Fixing another template error * add a iptables failure detector, default off (istio#19534) * cni validation Signed-off-by: Yuchen Dai <silentdai@gmail.com> stash Signed-off-by: Yuchen Dai <silentdai@gmail.com> add iptables for test Signed-off-by: Yuchen Dai <silentdai@gmail.com> update v6 and host addr Signed-off-by: Yuchen Dai <silentdai@gmail.com> working get origin dst ipv4 and ipv6 Signed-off-by: Yuchen Dai <silentdai@gmail.com> refactor server Signed-off-by: Yuchen Dai <silentdai@gmail.com> add default validator to iptables command Signed-off-by: Yuchen Dai <silentdai@gmail.com> iptables failure detector in preinit Signed-off-by: Yuchen Dai <silentdai@gmail.com> replace native byte order detection Signed-off-by: Yuchen Dai <silentdai@gmail.com> add tests skip validation by default Signed-off-by: Yuchen Dai <silentdai@gmail.com> * fix rootcmd flag Signed-off-by: Yuchen Dai <silentdai@gmail.com> * lint Signed-off-by: Yuchen Dai <silentdai@gmail.com> * go import Signed-off-by: Yuchen Dai <silentdai@gmail.com> * more lints Signed-off-by: Yuchen Dai <silentdai@gmail.com> * lintlintlint Signed-off-by: Yuchen Dai <silentdai@gmail.com> * go.mod go.sum by make gen Signed-off-by: Yuchen Dai <silentdai@gmail.com> * stash Signed-off-by: Yuchen Dai <silentdai@gmail.com> * stash2 Signed-off-by: Yuchen Dai <silentdai@gmail.com> * formatter Signed-off-by: Yuchen Dai <silentdai@gmail.com> * use simulate outbound server Signed-off-by: Yuchen Dai <silentdai@gmail.com> * ipv4,v6; server and client, both proxyport and inboundport Signed-off-by: Yuchen Dai <silentdai@gmail.com> * exit code Signed-off-by: Yuchen Dai <silentdai@gmail.com> * revert debug Signed-off-by: Yuchen Dai <silentdai@gmail.com> * address comment Signed-off-by: Yuchen Dai <silentdai@gmail.com> * gofmt -w -s Signed-off-by: Yuchen Dai <silentdai@gmail.com> * fix double run Signed-off-by: Yuchen Dai <silentdai@gmail.com> * revert RunOrFail Signed-off-by: Yuchen Dai <silentdai@gmail.com> * make gen Signed-off-by: Yuchen Dai <silentdai@gmail.com> * make gen make lint Signed-off-by: Yuchen Dai <silentdai@gmail.com> * resolve an error where it cannot be error Signed-off-by: Yuchen Dai <silentdai@gmail.com> * syscall reuseaddr Signed-off-by: Yuchen Dai <silentdai@gmail.com> * minor fix Signed-off-by: Yuchen Dai <silentdai@gmail.com> * save 1 second in iptable validation Signed-off-by: Yuchen Dai <silentdai@gmail.com> (cherry picked from commit ae817fc) * Fixing a few cherrypick mismerges * Changing the proxyv2 container to use the istio-iptables *binary* * Adding istio-iptables.sh back as default * Making istio-iptables binary change conditional * Typo * Fixing the distroless container Co-authored-by: Yuchen Dai <silentdai@gmail.com>
* Adding CNI repair to 1.4 * Fixing typo * Reverting some go.mod changes * More gomod changes * Addressing a test failure * Changing dangerous template whitespace deletion * Updating golden file. * Fixing another template error * add a iptables failure detector, default off (istio#19534) * cni validation Signed-off-by: Yuchen Dai <silentdai@gmail.com> stash Signed-off-by: Yuchen Dai <silentdai@gmail.com> add iptables for test Signed-off-by: Yuchen Dai <silentdai@gmail.com> update v6 and host addr Signed-off-by: Yuchen Dai <silentdai@gmail.com> working get origin dst ipv4 and ipv6 Signed-off-by: Yuchen Dai <silentdai@gmail.com> refactor server Signed-off-by: Yuchen Dai <silentdai@gmail.com> add default validator to iptables command Signed-off-by: Yuchen Dai <silentdai@gmail.com> iptables failure detector in preinit Signed-off-by: Yuchen Dai <silentdai@gmail.com> replace native byte order detection Signed-off-by: Yuchen Dai <silentdai@gmail.com> add tests skip validation by default Signed-off-by: Yuchen Dai <silentdai@gmail.com> * fix rootcmd flag Signed-off-by: Yuchen Dai <silentdai@gmail.com> * lint Signed-off-by: Yuchen Dai <silentdai@gmail.com> * go import Signed-off-by: Yuchen Dai <silentdai@gmail.com> * more lints Signed-off-by: Yuchen Dai <silentdai@gmail.com> * lintlintlint Signed-off-by: Yuchen Dai <silentdai@gmail.com> * go.mod go.sum by make gen Signed-off-by: Yuchen Dai <silentdai@gmail.com> * stash Signed-off-by: Yuchen Dai <silentdai@gmail.com> * stash2 Signed-off-by: Yuchen Dai <silentdai@gmail.com> * formatter Signed-off-by: Yuchen Dai <silentdai@gmail.com> * use simulate outbound server Signed-off-by: Yuchen Dai <silentdai@gmail.com> * ipv4,v6; server and client, both proxyport and inboundport Signed-off-by: Yuchen Dai <silentdai@gmail.com> * exit code Signed-off-by: Yuchen Dai <silentdai@gmail.com> * revert debug Signed-off-by: Yuchen Dai <silentdai@gmail.com> * address comment Signed-off-by: Yuchen Dai <silentdai@gmail.com> * gofmt -w -s Signed-off-by: Yuchen Dai <silentdai@gmail.com> * fix double run Signed-off-by: Yuchen Dai <silentdai@gmail.com> * revert RunOrFail Signed-off-by: Yuchen Dai <silentdai@gmail.com> * make gen Signed-off-by: Yuchen Dai <silentdai@gmail.com> * make gen make lint Signed-off-by: Yuchen Dai <silentdai@gmail.com> * resolve an error where it cannot be error Signed-off-by: Yuchen Dai <silentdai@gmail.com> * syscall reuseaddr Signed-off-by: Yuchen Dai <silentdai@gmail.com> * minor fix Signed-off-by: Yuchen Dai <silentdai@gmail.com> * save 1 second in iptable validation Signed-off-by: Yuchen Dai <silentdai@gmail.com> (cherry picked from commit ae817fc) * Fixing a few cherrypick mismerges * Changing the proxyv2 container to use the istio-iptables *binary* * Adding istio-iptables.sh back as default * Making istio-iptables binary change conditional * Typo * Fixing the distroless container Co-authored-by: Yuchen Dai <silentdai@gmail.com>
Add a iptables failure detector in go version of iptables
By default it will watch the inbound traffic and recover the original port(15002).
If iptables are set correctly it should see the inbound port as 15001 or 15006 but the original port is 15002.
It should work in both ipv4 and ipv6
The client and server are both in the init container and send traffic to itself through host ip. So it require istio >=1.3 to make this tcp connection capturable.
Add --skip-rule-apply and --skip-validation
A little bit more on --dry-run
An example log of spawn another istio-validation preinit container