-
Notifications
You must be signed in to change notification settings - Fork 35
Remove chain jump rules as part of cleanup #4
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
Conversation
|
The cleanup rules here never worked. Just cleaning our own rules up will add some complexity and potentially paper over more fundamental underlying issues. @adleong and I chatted and agreed that the correct solution is to detect if any of our rules still exist. If they do exist, error out in a reasonable fashion and produce a sane error message to help the users dig into what's going on. |
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 updated structure and output format is great!
As we discussed, I think making this error out if existing Linkerd rules are detected is a good idea.
|
@grampelberg Nice refactor 👍 $ iptables-save | grep A.OUTPUT
-A OUTPUT -m comment --comment "proxy-init/install-proxy-init-output/1574720803" -j PROXY_INIT_OUTPUT
$ iptables -t nat -D OUTPUT -j PROXY_INIT_OUTPUT -m comment --comment proxy-init/install-proxy-init-prerouting/1574303985
iptables: No chain/target/match by that name.
$ iptables -t nat -D OUTPUT -j PROXY_INIT_OUTPUT -m comment --comment proxy-init/install-proxy-init-output/1574720803
# successSo that'd require parsing the |
|
@alpeb yeah, there's no guarantee those are the only two rules as well. It'd be pretty easy with bash and way more complex than I'd like with how this is currently written. |
Signed-off-by: Charles Pretzer <charles@buoyant.io>
e13ddb2 to
b0bd9d4
Compare
cpretzer
left a comment
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.
Tested this locally against main and it's working as expected.
|
@alpeb What do you think is the right next step on this one? The comments above are correct in that this implementation is incomplete and chain rules can be left dangling or will be duplicated with different timestamps, which sounds like a different PR that will require some thought around the implementation. |
|
@cpretzer I think we can rescue all the code cleanup made in this PR, removing the rules cleanup added int |
Signed-off-by: Charles Pretzer <charles@buoyant.io>
Signed-off-by: Charles Pretzer <charles@buoyant.io>
39e211f to
f937777
Compare
|
@alpeb this is updated with the feedback you provided |
alpeb
left a comment
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.
Thanks @cpretzer (and @grampelberg) , this looks good to me 👍
Followup to #4 `iptables-save` doesn't accept the `-w` argument, which is causing the script to error, when used in CNI.
Followup to #4 `iptables-save` doesn't accept the `-w` argument, which is causing the script to error, when used in CNI. Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>
* iptables rules were failing in CNI when using --use-wait-flag Followup to #4 `iptables-save` doesn't accept the `-w` argument, which is causing the script to error, when used in CNI. Also, properly escape nsenter args. Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>
Signed-off-by: Steve Jenson <stevej@buoyant.io>
* modifying import paths and making a temporary copy of testutil/annotations.go Signed-off-by: Steve Jenson <stevej@buoyant.io> * removed testutil, dockerized cni installer tests now pass Signed-off-by: Steve Jenson <stevej@buoyant.io> * moving internal to pkg/linkerd-, removing Dockerfile until fixed, changining imports, removing linkerd2 k8s client with client-go Signed-off-by: Steve Jenson <stevej@buoyant.io> * gofmt install-cni_test.go Signed-off-by: Steve Jenson <stevej@buoyant.io> * go mod updates Signed-off-by: Steve Jenson <stevej@buoyant.io> * adding pkg to Docker image Signed-off-by: Steve Jenson <stevej@buoyant.io> * updating dev from v32 to v35 for go Signed-off-by: Steve Jenson <stevej@buoyant.io> * moving back to old dev image Signed-off-by: Steve Jenson <stevej@buoyant.io> * use dev:v32-go for go lint workflow Signed-off-by: Steve Jenson <stevej@buoyant.io> * fixing linter complaints Signed-off-by: Steve Jenson <stevej@buoyant.io> * fixing linter complaints Signed-off-by: Steve Jenson <stevej@buoyant.io> * turning off noisy lint #1 Signed-off-by: Steve Jenson <stevej@buoyant.io> * turning off noisy lint #2 Signed-off-by: Steve Jenson <stevej@buoyant.io> * turning off noisy lint #3 Signed-off-by: Steve Jenson <stevej@buoyant.io> * turning off noisy lint #4 Signed-off-by: Steve Jenson <stevej@buoyant.io> * turning off noisy lint #5 Signed-off-by: Steve Jenson <stevej@buoyant.io> * turning off noisy lint #6 Signed-off-by: Steve Jenson <stevej@buoyant.io> * Replace pkg/ with internal/ (#148) * Replace pkg/ with internal/ There's no need for a public library export. We can share code within this repo via the `internal` directory. * simplify package names Signed-off-by: Oliver Gould <ver@buoyant.io> * adding internal back. whoopsie Signed-off-by: Steve Jenson <stevej@buoyant.io> * bumping dev go version Signed-off-by: Steve Jenson <stevej@buoyant.io> * replace deprecated ioutil functions with io functions. Signed-off-by: Steve Jenson <stevej@buoyant.io> * increasing timeout to help with linter issues, adding verbose Signed-off-by: Steve Jenson <stevej@buoyant.io> * replace TODO with literals, wait for the linter to complain so we can give it the magic incantation to sleep now Signed-off-by: Steve Jenson <stevej@buoyant.io> * more linter Signed-off-by: Steve Jenson <stevej@buoyant.io> * gofmt Signed-off-by: Steve Jenson <stevej@buoyant.io> * swap position of comment and argument as the linter has an opinion here, too Signed-off-by: Steve Jenson <stevej@buoyant.io> * Update cni-plugin/main.go Co-authored-by: Alejandro Pedraza <alejandro@buoyant.io> * Update cni-plugin/main.go Co-authored-by: Alejandro Pedraza <alejandro@buoyant.io> * Update cni-plugin/main.go Co-authored-by: Alejandro Pedraza <alejandro@buoyant.io> * Update cni-plugin/main.go Co-authored-by: Alejandro Pedraza <alejandro@buoyant.io> * simplify lint call Signed-off-by: Steve Jenson <stevej@buoyant.io> * removed unneeded abstraction Signed-off-by: Steve Jenson <stevej@buoyant.io> * linter for cni-plugin and all go code Signed-off-by: Steve Jenson <stevej@buoyant.io> * giving flags to go linter Signed-off-by: Steve Jenson <stevej@buoyant.io> * run the test on the moved internal package Signed-off-by: Steve Jenson <stevej@buoyant.io> * adding keys back for annotation lookup Signed-off-by: Steve Jenson <stevej@buoyant.io> Signed-off-by: Steve Jenson <stevej@buoyant.io> Signed-off-by: Oliver Gould <ver@buoyant.io> Co-authored-by: Oliver Gould <ver@buoyant.io> Co-authored-by: Alejandro Pedraza <alejandro@buoyant.io>
* modifying import paths and making a temporary copy of testutil/annotations.go Signed-off-by: Steve Jenson <stevej@buoyant.io> * removed testutil, dockerized cni installer tests now pass Signed-off-by: Steve Jenson <stevej@buoyant.io> * moving internal to pkg/linkerd-, removing Dockerfile until fixed, changining imports, removing linkerd2 k8s client with client-go Signed-off-by: Steve Jenson <stevej@buoyant.io> * gofmt install-cni_test.go Signed-off-by: Steve Jenson <stevej@buoyant.io> * go mod updates Signed-off-by: Steve Jenson <stevej@buoyant.io> * adding pkg to Docker image Signed-off-by: Steve Jenson <stevej@buoyant.io> * updating dev from v32 to v35 for go Signed-off-by: Steve Jenson <stevej@buoyant.io> * moving back to old dev image Signed-off-by: Steve Jenson <stevej@buoyant.io> * use dev:v32-go for go lint workflow Signed-off-by: Steve Jenson <stevej@buoyant.io> * fixing linter complaints Signed-off-by: Steve Jenson <stevej@buoyant.io> * fixing linter complaints Signed-off-by: Steve Jenson <stevej@buoyant.io> * turning off noisy lint #1 Signed-off-by: Steve Jenson <stevej@buoyant.io> * turning off noisy lint #2 Signed-off-by: Steve Jenson <stevej@buoyant.io> * turning off noisy lint #3 Signed-off-by: Steve Jenson <stevej@buoyant.io> * turning off noisy lint #4 Signed-off-by: Steve Jenson <stevej@buoyant.io> * turning off noisy lint #5 Signed-off-by: Steve Jenson <stevej@buoyant.io> * turning off noisy lint #6 Signed-off-by: Steve Jenson <stevej@buoyant.io> * adding in Dockerfile, just rules for building, and a workflow for testing the cni-plugin installer script Signed-off-by: Steve Jenson <stevej@buoyant.io> * remember to setup docker Signed-off-by: Steve Jenson <stevej@buoyant.io> * remember to setup docker-qemu Signed-off-by: Steve Jenson <stevej@buoyant.io> * where is docker? Signed-off-by: Steve Jenson <stevej@buoyant.io> * back to a named ubuntu version, removing devcontainer Signed-off-by: Steve Jenson <stevej@buoyant.io> * we need just Signed-off-by: Steve Jenson <stevej@buoyant.io> * WIP import of CNI plugin integration test environment. does not run due to image pull errors. Signed-off-by: Steve Jenson <stevej@buoyant.io> * rewriting just rules to match new rules Signed-off-by: Steve Jenson <stevej@buoyant.io> * bumping dev version, renaming smoke test Signed-off-by: Steve Jenson <stevej@buoyant.io> * WIP for running smoke tests Signed-off-by: Steve Jenson <stevej@buoyant.io> * go workflow fix Signed-off-by: Steve Jenson <stevej@buoyant.io> * also rid ourselves of ioutil in this branch Signed-off-by: Steve Jenson <stevej@buoyant.io> * adding a second, passing test Signed-off-by: Steve Jenson <stevej@buoyant.io> * let's run the test in CI Signed-off-by: Steve Jenson <stevej@buoyant.io> * name the test properly for CI to run it Signed-off-by: Steve Jenson <stevej@buoyant.io> * made the installer integration tests more in-line with the other integration tests Signed-off-by: Steve Jenson <stevej@buoyant.io> * cni-plugin integration test workflow Signed-off-by: Steve Jenson <stevej@buoyant.io> * breaking up steps, renaming test Signed-off-by: Steve Jenson <stevej@buoyant.io> * just Signed-off-by: Steve Jenson <stevej@buoyant.io> * bringing changes from linkerd2 over Signed-off-by: Steve Jenson <stevej@buoyant.io> * tests running within cni-plugin context Signed-off-by: Steve Jenson <stevej@buoyant.io> * create service account and don't delete so we can inspect Signed-off-by: Steve Jenson <stevej@buoyant.io> * fix namespaces, use matei's k3d/k3s mountPaths in the hopes that CNI will run in our pod. Signed-off-by: Steve Jenson <stevej@buoyant.io> * WIP for debugging why CNI won't run in my own pods despite everything looking normal Signed-off-by: Steve Jenson <stevej@buoyant.io> * adding whitespace, fixing comment, removing unneeded variable Signed-off-by: Steve Jenson <stevej@buoyant.io> * fixing some small things Signed-off-by: Steve Jenson <stevej@buoyant.io> * improving Dockerfile, going back to edge for linkerd-cni Signed-off-by: Steve Jenson <stevej@buoyant.io> * cleaned up Dockerfile Signed-off-by: Steve Jenson <stevej@buoyant.io> * using --link for 50% size improvement Signed-off-by: Steve Jenson <stevej@buoyant.io> * cleanup unusued functions, run network-validator before test suite Signed-off-by: Steve Jenson <stevej@buoyant.io> * remove qemu setup, add comment about log level Signed-off-by: Steve Jenson <stevej@buoyant.io> * add wiring to see cni-net-dir and check for kubeconfig Signed-off-by: Steve Jenson <stevej@buoyant.io> * checking that linkerd-cni is the last plugin in the conflist Signed-off-by: Steve Jenson <stevej@buoyant.io> * renaming smoke_test to flannel Signed-off-by: Steve Jenson <stevej@buoyant.io> * rename files, update justfile * name a test file _test so the test runner will run my test Signed-off-by: Steve Jenson <stevej@buoyant.io> * renaming to flannel Signed-off-by: Steve Jenson <stevej@buoyant.io> * remove hardcoded filename Signed-off-by: Steve Jenson <stevej@buoyant.io> * clarified comment Signed-off-by: Steve Jenson <stevej@buoyant.io> * fixed merge conflict error Signed-off-by: Steve Jenson <stevej@buoyant.io> * fix log levels Signed-off-by: Steve Jenson <stevej@buoyant.io> * fix a log level Signed-off-by: Steve Jenson <stevej@buoyant.io> * run test on all files in ./cni-plugin Signed-off-by: Steve Jenson <stevej@buoyant.io> * hcomment explaining why there's no ENTRYPOINT Signed-off-by: Steve Jenson <stevej@buoyant.io> * use a map instead of an array for simplicity Signed-off-by: Steve Jenson <stevej@buoyant.io> * abstract which integration test subdirectory gets used, add internal to ensure those packages are tested Signed-off-by: Steve Jenson <stevej@buoyant.io> * go.yml is already running these tests are there no integration tests in there to run Signed-off-by: Steve Jenson <stevej@buoyant.io> * breaking up a line Signed-off-by: Steve Jenson <stevej@buoyant.io> * renaming SUBDIRECTORY to SCENARIO and renaming a run just target to flannel to signify that this is the rule to crib for other scenarios Signed-off-by: Steve Jenson <stevej@buoyant.io> * better error handling of the cleanup() function, print more diagnostic information if linkerd-cni rollout fails Signed-off-by: Steve Jenson <stevej@buoyant.io> * add error handling for describe ds and logs Signed-off-by: Steve Jenson <stevej@buoyant.io> Signed-off-by: Steve Jenson <stevej@buoyant.io>
This does a couple things:
nsentercode a little so that the log output includes the commands runiptables-saveto get all rules instead of just the nat tableHere's the new output: