Skip to content
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

Use istio-iptables golang implementation instead of bash script #18962

Merged
merged 1 commit into from Nov 27, 2019

Conversation

@abhide
Copy link
Member

abhide commented Nov 14, 2019

Testing all E2E tests that will use golang implementation for istio-iptables
Issue: #13967

@abhide abhide requested a review from howardjohn Nov 14, 2019
@abhide abhide requested a review from istio/wg-test-and-release-maintainers as a code owner Nov 14, 2019
@googlebot googlebot added the cla: yes label Nov 14, 2019
@abhide abhide requested a review from rlenglet Nov 14, 2019
@rlenglet

This comment has been minimized.

Copy link
Member

rlenglet commented Nov 16, 2019

A bunch of tests don't like the additional option...

@abhide

This comment has been minimized.

Copy link
Member Author

abhide commented Nov 16, 2019

A bunch of tests don't like the additional option...

Yeah.. Will take a look

@abhide abhide force-pushed the abhide:13967-p1-test branch from 6aa0e07 to ba1b742 Nov 20, 2019
@abhide

This comment has been minimized.

Copy link
Member Author

abhide commented Nov 21, 2019

Most (almost all) tests failures are related to pods unable to start due to images pulll

image: "istio-testing/pilot:istio-testing-distroless"
image: "istio-testing/mixer:istio-testing-distroless"
image: "istio-testing/galley:istio-testing-distroless"

Looks like the distroless images aren't present or being looked up at wrong location?

@rlenglet @howardjohn Any ideas how to resolve this?

prow/e2e-kind-suite.sh Outdated Show resolved Hide resolved
@abhide abhide force-pushed the abhide:13967-p1-test branch from ba1b742 to 20eae3c Nov 25, 2019
@abhide

This comment has been minimized.

Copy link
Member Author

abhide commented Nov 25, 2019

/retest

@abhide

This comment has been minimized.

Copy link
Member Author

abhide commented Nov 25, 2019

Thanks @howardjohn test results look better than before! Getting there!

@rlenglet

This comment has been minimized.

Copy link
Member

rlenglet commented Nov 26, 2019

This is the test that is failing:

func TestTcpStatefulSets(t *testing.T) {

And this is the particular command that is failing:
cmd := fmt.Sprintf("client --url %s --count %d %s", url, count, extra)
request, err := util.PodExec(tc.Kube.Namespace, pod, "app", cmd, true, tc.Kube.Clusters[cluster])

The app container fails to start for some reason.

kubelet logged this about the statefulset-0 pod:

Nov 25 05:27:39 istio-testing-control-plane kubelet[283]: E1125 05:27:39.314079     283 pod_workers.go:191] Error syncing pod 194a2fd2-249e-4261-be27-6a3aab126a85 ("statefulset-0_istio-system(194a2fd2-249e-4261-be27-6a3aab126a85)"), skipping: failed to "StartContainer" for "app" with ErrImagePull: "rpc error: code = Unknown desc = failed to pull and unpack image \"docker.io/istio-testing/app:istio-testing-distroless\": failed to resolve reference \"docker.io/istio-testing/app:istio-testing-distroless\": pull access denied, repository does not exist or may require authorization: server message: insufficient_scope: authorization failed"
Nov 25 05:27:39 istio-testing-control-plane containerd[83]: time="2019-11-25T05:27:39.478814816Z" level=info msg="ExecSync for \"fdc0b1118c3d3ed55e5190fde59012dd526a7ab19948fa917c26ed2c76704ab3\" with command [/usr/local/bin/galley probe --probe-path=/tmp/healthready --interval=10s] and timeout 1 (s)"
Nov 25 05:27:39 istio-testing-control-plane kubelet[283]: E1125 05:27:39.486508     283 pod_workers.go:191] Error syncing pod 194a2fd2-249e-4261-be27-6a3aab126a85 ("statefulset-0_istio-system(194a2fd2-249e-4261-be27-6a3aab126a85)"), skipping: failed to "StartContainer" for "app" with ImagePullBackOff: "Back-off pulling image \"istio-testing/app:istio-testing-distroless\""
...
Nov 25 05:27:40 istio-testing-control-plane kubelet[283]: E1125 05:27:40.492050     283 pod_workers.go:191] Error syncing pod 194a2fd2-249e-4261-be27-6a3aab126a85 ("statefulset-0_istio-system(194a2fd2-249e-4261-be27-6a3aab126a85)"), skipping: failed to "StartContainer" for "app" with ImagePullBackOff: "Back-off pulling image \"istio-testing/app:istio-testing-distroless\""
...
Nov 25 05:27:55 istio-testing-control-plane kubelet[283]: E1125 05:27:55.985478     283 pod_workers.go:191] Error syncing pod 194a2fd2-249e-4261-be27-6a3aab126a85 ("statefulset-0_istio-system(194a2fd2-249e-4261-be27-6a3aab126a85)"), skipping: failed to "StartContainer" for "app" with ErrImagePull: "rpc error: code = Unknown desc = failed to pull and unpack image \"docker.io/istio-testing/app:istio-testing-distroless\": failed to resolve reference \"docker.io/istio-testing/app:istio-testing-distroless\": pull access denied, repository does not exist or may require authorization: server message: insufficient_scope: authorization failed"
...
Nov 25 05:28:07 istio-testing-control-plane kubelet[283]: E1125 05:28:07.358790     283 pod_workers.go:191] Error syncing pod 194a2fd2-249e-4261-be27-6a3aab126a85 ("statefulset-0_istio-system(194a2fd2-249e-4261-be27-6a3aab126a85)"), skipping: failed to "StartContainer" for "app" with ImagePullBackOff: "Back-off pulling image \"istio-testing/app:istio-testing-distroless\""

etc. etc.

The istio-testing/app:istio-testing-distroless tag is obviously incorrect. -distroless shouldn't be appended to the tag. This is a test bug.

@rlenglet

This comment has been minimized.

Copy link
Member

rlenglet commented Nov 26, 2019

This bug is in

"Tag": tc.Kube.PilotTag(),

I believe this should call tc.Kube.AppTag() instead of tc.Kube.PilotTag(), cf.

"Tag": tc.Kube.AppTag(),
(which didn't fail).

@abhide could you try this fix, and submit a proper PR if this works?

@abhide abhide force-pushed the abhide:13967-p1-test branch from 20eae3c to c4777ab Nov 26, 2019
@abhide abhide requested a review from istio/wg-networking-maintainers as a code owner Nov 26, 2019
@abhide abhide force-pushed the abhide:13967-p1-test branch from c4777ab to 3e339a0 Nov 26, 2019
@abhide

This comment has been minimized.

Copy link
Member Author

abhide commented Nov 26, 2019

/test unit-tests_istio

@abhide abhide force-pushed the abhide:13967-p1-test branch from 3e339a0 to 21bc70e Nov 27, 2019
@istio-testing istio-testing added size/S and removed size/XS labels Nov 27, 2019
@abhide abhide changed the title WIP: Change to trigger CI test that use distroless WIP: Use istio-iptables golang implementation instead of bash script Nov 27, 2019
@abhide abhide force-pushed the abhide:13967-p1-test branch from 21bc70e to e3bb4a9 Nov 27, 2019
Copy link
Member

rlenglet left a comment

This is awesome! Just one nitpick.

tools/istio-docker.mk Show resolved Hide resolved
Copy link
Member

howardjohn left a comment

if we merge this should we copy the bash script in as well for a release as a backup in case there is some issue with the go impl? that way user could just switch the template to reference the shell script instead of waiting for a new release to fix issue

@abhide

This comment has been minimized.

Copy link
Member Author

abhide commented Nov 27, 2019

This bug is in

"Tag": tc.Kube.PilotTag(),

I believe this should call tc.Kube.AppTag() instead of tc.Kube.PilotTag(), cf.

"Tag": tc.Kube.AppTag(),

(which didn't fail).
@abhide could you try this fix, and submit a proper PR if this works?

This is fixed in master branch by: #19252

@howardjohn

This comment has been minimized.

Copy link
Member

howardjohn commented Nov 27, 2019

actually I guess part of the motivation is that we don't have to maintain two implementation so maybe that's not worth it

@abhide

This comment has been minimized.

Copy link
Member Author

abhide commented Nov 27, 2019

if we merge this should we copy the bash script in as well for a release as a backup in case there is some issue with the go impl? that way user could just switch the template to reference the shell script instead of waiting for a new release to fix issue

hmm.. don't we want to move away from bash script? We should fix any issues with golang implementation and get rid of bash script completely.

@rlenglet

This comment has been minimized.

Copy link
Member

rlenglet commented Nov 27, 2019

actually I guess part of the motivation is that we don't have to maintain two implementation so maybe that's not worth it

Exactly what I'm thinking. The Golang implementation is now well covered by unit tests, so this is the implementation we should focus on.

@abhide abhide changed the title WIP: Use istio-iptables golang implementation instead of bash script Use istio-iptables golang implementation instead of bash script Nov 27, 2019
@mandarjog

This comment has been minimized.

Copy link
Contributor

mandarjog commented Nov 27, 2019

Does golang impl already produce identical rules over the input space?
We should totally get rid of bash and direct up tables stuff. How does distroless or non-distroless affect this?

@abhide

This comment has been minimized.

Copy link
Member Author

abhide commented Nov 27, 2019

Does golang impl already produce identical rules over the input space?
We should totally get rid of bash and direct up tables stuff. How does distroless or non-distroless affect this?

@mandarjog Yes, golang produces identical rules.
Also, golang uses iptables-restore instead of individual iptables commands. I tested out distroless there were 2 test setup issues

  1. kubectl cp failed as tar was missing from distroless container
  2. multi-cluster tests had issues. @howardjohn can you add more details for it
@howardjohn

This comment has been minimized.

Copy link
Member

howardjohn commented Nov 27, 2019

Mandar for some more info, distroless has already been using to go impl forever. This PR originally was switching to distroless default just to test the iptables stuff but now has switched to just using the go iptables everywhere

@howardjohn

This comment has been minimized.

Copy link
Member

howardjohn commented Nov 27, 2019

This lgtm, but will give others a chance to review

@kyessenov

This comment has been minimized.

Copy link
Contributor

kyessenov commented Nov 27, 2019

This is wonderful. We can slim the base image to drop C tools and statically link the go binary instead.

@howardjohn

This comment has been minimized.

Copy link
Member

howardjohn commented Nov 27, 2019

@kyessenov I think we still depend on iptables? See the distroless implementation, we copy these over

COPY --from=default /sbin/xtables-multi /sbin/iptables* /sbin/ip6tables* /sbin/ip /sbin/
COPY --from=default /usr/lib/x86_64-linux-gnu/xtables/ /usr/lib/x86_64-linux-gnu/xtables
COPY --from=default /usr/lib/x86_64-linux-gnu/ /usr/lib/x86_64-linux-gnu
COPY --from=default /etc/iproute2 /etc/iproute2

But if we can clean some stuff up, would be great

@kyessenov

This comment has been minimized.

Copy link
Contributor

kyessenov commented Nov 27, 2019

OK, I'm confused. Is the golang implementation just a script wrapper to call out to iptables then?

@abhide

This comment has been minimized.

Copy link
Member Author

abhide commented Nov 27, 2019

@kyessenov I think we still depend on iptables? See the distroless implementation, we copy these over

COPY --from=default /sbin/xtables-multi /sbin/iptables* /sbin/ip6tables* /sbin/ip /sbin/
COPY --from=default /usr/lib/x86_64-linux-gnu/xtables/ /usr/lib/x86_64-linux-gnu/xtables
COPY --from=default /usr/lib/x86_64-linux-gnu/ /usr/lib/x86_64-linux-gnu
COPY --from=default /etc/iproute2 /etc/iproute2

But if we can clean some stuff up, would be great

We will need iptables, iptables-restore, iptables-save (and same for v6)
@kyessenov yes, golang implementation will invoke iptables-restore by default, but can also invoke individual iptables commands.

@kyessenov

This comment has been minimized.

Copy link
Contributor

kyessenov commented Nov 27, 2019

Do we have a plan to address the hack of copying binaries from one image to another? It's not safe to do so since we have varying versions of glibc used to build them.

@rlenglet

This comment has been minimized.

Copy link
Member

rlenglet commented Nov 27, 2019

@abhide please add an entry in the Release 1.5 note draft.

@abhide

This comment has been minimized.

Copy link
Member Author

abhide commented Nov 27, 2019

/test lint_istio

1 similar comment
@abhide

This comment has been minimized.

Copy link
Member Author

abhide commented Nov 27, 2019

/test lint_istio

@istio-testing istio-testing merged commit 7a6ede4 into istio:master Nov 27, 2019
28 of 29 checks passed
28 of 29 checks passed
tide Not mergeable. Retesting: lint_istio
Details
cla/google All necessary CLAs are signed
e2e-bookInfoTests-envoyv2-v1alpha3_istio Job succeeded.
Details
e2e-dashboard_istio Job succeeded.
Details
e2e-mixer-no_auth_istio Job succeeded.
Details
e2e-simpleTests-cni_istio Job succeeded.
Details
e2e-simpleTests-distroless_istio Job succeeded.
Details
e2e-simpleTestsMinProfile_istio Job succeeded.
Details
e2e-simpleTests_istio Job succeeded.
Details
gencheck_istio Job succeeded.
Details
integ-framework-k8s-tests_istio Job succeeded.
Details
integ-framework-local-tests_istio Job succeeded.
Details
integ-galley-k8s-tests_istio Job succeeded.
Details
integ-galley-local-tests_istio Job succeeded.
Details
integ-istioctl-k8s-tests_istio Job succeeded.
Details
integ-istioio-k8s-tests_istio Job succeeded.
Details
integ-mixer-k8s-tests_istio Job succeeded.
Details
integ-new-install-k8s-tests_istio Job succeeded.
Details
integ-pilot-k8s-tests_istio Job succeeded.
Details
integ-pilot-local-tests_istio Job succeeded.
Details
integ-security-k8s-tests_istio Job succeeded.
Details
integ-security-local-tests_istio Job succeeded.
Details
integ-telemetry-k8s-tests_istio Job succeeded.
Details
istio_e2e_cloudfoundry_istio Job succeeded.
Details
lint_istio Job succeeded.
Details
pilot-e2e-envoyv2-v1alpha3_istio Job succeeded.
Details
pilot-multicluster-e2e_istio Job succeeded.
Details
release-test_istio Job succeeded.
Details
unit-tests_istio Job succeeded.
Details
@abhide abhide deleted the abhide:13967-p1-test branch Nov 27, 2019
@abhide

This comment has been minimized.

Copy link
Member Author

abhide commented Nov 27, 2019

@abhide please add an entry in the Release 1.5 note draft.

@rlenglet Added it under Other section

@ccccbjcn

This comment has been minimized.

Copy link

ccccbjcn commented Mar 27, 2020

Do you have any plan to support IPVS? Per the code, we got the iptables being installed by hard coding. If changed to use IPVS, will change tools/istio-iptables code?

@rlenglet

This comment has been minimized.

Copy link
Member

rlenglet commented Mar 27, 2020

@ccccbjcn IPVS doesn't solve the same problem.

@ccccbjcn

This comment has been minimized.

Copy link

ccccbjcn commented Mar 28, 2020

@rlenglet Thanks for your reply. Yes, using IPVS still needs to write code to install the iptables. I mean whether using IPVS to improve performance for large scale APP deployment on a target?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.