Address istio mesh e2e mesh failures #11286

dprotaso · 2021-04-30T19:50:36Z

/area test-and-release
/area networking
/kind cleanup
/kind good-first-issue

Expected Behavior

e2e tests pass when running our e2e tests with istio and the local gateway disabled (mesh only) with mTLS enabled (STRICT). At a minimum we should be skipping tests we know that will fail when strict mTLS is enabled.

This also affects DomainMapping but that's being tracked by knative-extensions/net-istio#562.

Actual Behavior

The following tests will always fail since we have global mTLS turned on (STRICT)

test/e2e: TestSvcToSvcViaActivator/both-disabled
test/e2e: TestSvcToSvcViaActivator/a-disabled
test/e2e: TestCallToPublicService/local_address

Steps to Reproduce the Problem

Ensure your net-istio latest/stable mesh has the right PeerAuthentication to ensure global mTLS is on
ie. [istio-stable] renable global mTLS when in mesh mode knative-extensions/net-istio#617
Once e2e - drop imperative bash yaml for declarative ytt/kapp #11175 merges open a PR with a noop change and run

/test pull-knative-serving-istio-latest-mesh
/test pull-knative-serving-istio-stable-mesh

Additional Info

The text was updated successfully, but these errors were encountered:

dprotaso · 2021-05-03T13:23:42Z

/assign @arturenault

nak3 · 2021-05-10T06:46:40Z

@dprotaso @arturenault The root cause is local-gateway.mesh: "mesh".

The option makes tests failed w/o sidecar injection. The failed tests disable sidecar as:

serving/test/e2e/service_to_service_test.go

Line 123 in d30f0bd

"sidecar.istio.io/inject": strconv.FormatBool(inject),

I verified these tests worked if local-gateway.mesh: "mesh" was dropped by https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/11323/pull-knative-serving-istio-stable-mesh/1391619647853301760

So, we need to change either:

a) Remove local-gateway.mesh: "mesh".
b) Skip tests with w/o sidecar -> app for mesh tests.

arturenault · 2021-05-10T15:15:00Z

Thanks for looking into this @nak3. I think option 2 makes sense. I'll send a PR later today.

dprotaso · 2021-05-11T15:20:12Z

yeah - some of the tests skip disable sidecars so the test will never pass with STRICT mTLS

nak3 · 2021-06-11T10:13:00Z

Now, I am inclined to think "a) Remove local-gateway.mesh: "mesh"".
Even if we skip test with w/o sidecar -> app (by #11331), DomainMapping tests cannot be fixed as knative-extensions/net-istio#562.

So can we remove the option in net-istio https://github.com/knative-sandbox/net-istio/blob/main/third_party/istio-stable/istio-kind-mesh/config-istio.yaml#L11 ? (I kicked the test run in #11503)

cc @ZhiminXiang

dprotaso · 2021-06-11T15:08:53Z

"a) Remove local-gateway.mesh: "mesh""

Then are we really running mesh (with mTLS) ?

ZhiminXiang · 2021-06-11T16:59:59Z

"a) Remove local-gateway.mesh: "mesh""

Then are we really running mesh (with mTLS) ?

I think we use mTLS STRICT for mesh https://github.com/knative-sandbox/net-istio/blob/8a3cf7e23bc14a27218bddaf0a3757c4d64f891a/third_party/istio-latest/extra/istio-mesh.yaml#L9

ZhiminXiang · 2021-06-11T17:07:44Z

So can we remove the option in net-istio https://github.com/knative-sandbox/net-istio/blob/main/third_party/istio-stable/istio-kind-mesh/config-istio.yaml#L11 ? (I kicked the test run in #11503)

cc @ZhiminXiang

I think we can remove the option local-gateway.mesh: "mesh".
If we remove this option, we may want to add an extra test for service-to-service communication when there is no pods for knative-local-gateway service in order to make sure there is no extra hop for service-to-service communication (in case there is any regression in Istio side).

dprotaso · 2021-06-11T19:21:31Z

I'm confused I thought by removing local-gateway.mesh that would required service to service communication to go through an additional hop (a gateway pod) - vs. sidecar to sidecar.

I'd only consider sidecar to sidecar to be 'mesh'

julz · 2021-06-11T19:41:05Z

I'm confused I thought by removing local-gateway.mesh that would required service to service communication to go through an additional hop (a gateway pod) - vs. sidecar to sidecar.

I believe removing this just allows access through the gateway for pods that arent in the mesh, it doesn't require it, and pods with a sidecar will still go pod-to-pod.

ZhiminXiang · 2021-06-11T20:03:54Z

I'm confused I thought by removing local-gateway.mesh that would required service to service communication to go through an additional hop (a gateway pod) - vs. sidecar to sidecar.

I believe removing this just allows access through the gateway for pods that arent in the mesh, it doesn't require it, and pods with a sidecar will still go pod-to-pod.

@julz is right. From my test knative-extensions/net-istio#562 (comment), after removing local-gateway.mesh, the internal communication is still sidecar to sidecar. I think the sidecar of client knows what is the right destination and will directly proxy the requests to the destination.

dprotaso · 2021-06-12T01:16:23Z

ok great :)

nak3 · 2021-06-14T12:59:43Z

So we will proceed this issue by the following steps:

Do not enable local-gateway.mesh in e2e (remove it from net-istio).
Consider (open an issue/send a mail knative-user@) to deprecate local-gateway.mesh option. (The deprecation was mentioned in DomainMapping does not work with local gateway "mesh" mode knative-extensions/net-istio#562 (comment). And we should deprecate it as we no longer test it.)
Add a new test to verify svc-to-svc in mesh does not use local gateway.

I will send the PRs so please help your reviews 🙏

arturenault · 2021-06-14T16:50:18Z

Thanks all, sorry for dropping the ball on the previous PR (got busy with other things and then went on vacation for 2 weeks). Sounds like we're going with a different solution and assign this issue to kenjiro.

/assign @nak3

nak3 · 2021-06-21T09:47:09Z

So, the last monster is scale-200 test 😅

#11552 verified that all test passed with -short option (it runs scale-10 instead of scale-200) so I think we can use the -short options for somewhere.

I think we have a few options for the solution like:

🟢 ... test will pass.
🔴 ... test will fail.

option 1. Use short option for pre-submit test.

🟢 /test pull-knative-serving-istio-stable-mesh (w/ -short)
🔴 periodic job (test grid)

option 2. Add new job trigger `/test pull-knative-serving-istio-stable-mesh-short` to run mesh w/ -short.

🟢 /test pull-knative-serving-istio-stable-mesh-short (w/ -short)
🔴 periodic job (test grid)
🔴 /test pull-knative-serving-istio-stable-mesh

nak3 · 2021-06-21T09:49:11Z

And I am thinking that option 2 is good for now.
(option 1 will make confusion as pre-submit passed all tests but test grid always red. Someone wonder what is the difference...)

julz · 2021-06-21T13:38:50Z

Option 2 seems reasonable to me too fwiw

(I would love, at some point, separately, to figure out if we can tweak the timeouts on the full scale tests in mesh mode to have it pass, though -- it's obviously not ideal if we have no scale tests for mesh mode long-term since I imagine people who use mesh might have relatively big clusters/workloads)

dprotaso · 2021-06-21T14:21:57Z

To @julz's point is configuring the timeouts for mesh an option easier option - ie. make them test flags

dprotaso · 2021-06-21T14:24:49Z

aside: a caveat is these timings would be sensitive to GKE performance and it could regress again. I think these scale tests should be spun up on GCE/kubeadm were we have more control of the machine types of the API server

But we've only been bitten by this once so maybe not worth doing so yet

This patch adds short option to e2e test and support it on scale test. Please see knative#11286 for the discussion.

* Add short option to scale test This patch adds short option to e2e test and support it on scale test. Please see #11286 for the discussion. * Temporary enable short option. * Revert "Temporary enable short option." This reverts commit 52eed09. * Use short option for all test when enabled

nak3 · 2021-06-28T04:21:08Z

Now, mesh test fails with only scale-200. HA test and TestHPAAutoscaleUpDownUp fails sometime but it was also caused by scale-200 test 😓 (It can be fixed by tweaking the test order as #11552).

Should we close this issue? Or keep open until scale-200 test could be fixed?

julz · 2021-06-28T08:30:08Z

Should we close this issue? Or keep open until scale-200 test could be fixed?

Personal opinion: I reckon since we now have consistently green tests (🎉🎉!) we should close this issue but we should create another one to track re-enabling the fill scale-200 (unless we already did obviously), because we do want to do that one way or another

dprotaso · 2021-06-28T13:59:20Z

testgrid still shows red for me - is there a new job?

https://testgrid.k8s.io/r/knative-own-testgrid/serving

nak3 · 2021-06-29T04:03:22Z

The mesh test's failure is scale-200.

knative/test-infra#2817 added pull-knative-serving-istio-stable-mesh-short but it is just for a manual trigger or updaate of third_party/istio-latest/net-istio.yaml.
Should we add or replace stable-mesh-short for the test grid?

dprotaso · 2021-07-05T17:41:20Z

I guess I'm trying to answer the question:

Where do I look or what tests do I run to ensure the mesh case is working as expected.

If we know scale-200 is going to fail should we use skip it for the mesh case and decide after whether it's possible to fix?

nak3 · 2021-08-28T00:44:31Z

stable-mesh-short passes all test.
Removing/decreasing scale-200 for mesh (not short) was already proposed before but #5989 was not accepted.

knative-prow-robot added area/test-and-release It flags unit/e2e/conformance/perf test issues for product features area/networking kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/good-first-issue labels Apr 30, 2021

This was referenced Apr 30, 2021

e2e - drop imperative bash yaml for declarative ytt/kapp #11175

Merged

[Automated] Update net-istio nightly #11287

Merged

knative-prow-robot assigned arturenault May 3, 2021

nak3 mentioned this issue May 10, 2021

wip: Enable sidecar injection for net-istio controller #11323

Closed

arturenault mentioned this issue May 11, 2021

Skip tests where A isn't injected and start injecting for public service call test #11331

Closed

knative-prow-robot assigned nak3 Jun 14, 2021

nak3 added a commit to nak3/serving that referenced this issue Jun 22, 2021

Add short option to scale test

0adc297

This patch adds short option to e2e test and support it on scale test. Please see knative#11286 for the discussion.

nak3 mentioned this issue Jun 22, 2021

Add short option to scale test #11571

Merged

arturenault removed their assignment Jul 8, 2021

dprotaso added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. and removed kind/good-first-issue labels Jul 23, 2021

nak3 closed this as completed Aug 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address istio mesh e2e mesh failures #11286

Address istio mesh e2e mesh failures #11286

dprotaso commented Apr 30, 2021 •

edited

Loading

dprotaso commented May 3, 2021

nak3 commented May 10, 2021 •

edited

Loading

arturenault commented May 10, 2021

dprotaso commented May 11, 2021 •

edited

Loading

nak3 commented Jun 11, 2021

dprotaso commented Jun 11, 2021

ZhiminXiang commented Jun 11, 2021

ZhiminXiang commented Jun 11, 2021

dprotaso commented Jun 11, 2021

julz commented Jun 11, 2021

ZhiminXiang commented Jun 11, 2021

dprotaso commented Jun 12, 2021

nak3 commented Jun 14, 2021

arturenault commented Jun 14, 2021

nak3 commented Jun 21, 2021

nak3 commented Jun 21, 2021

julz commented Jun 21, 2021

dprotaso commented Jun 21, 2021

dprotaso commented Jun 21, 2021 •

edited

Loading

nak3 commented Jun 28, 2021

julz commented Jun 28, 2021

dprotaso commented Jun 28, 2021

nak3 commented Jun 29, 2021

dprotaso commented Jul 5, 2021

nak3 commented Aug 28, 2021 •

edited

Loading

Address istio mesh e2e mesh failures #11286

Address istio mesh e2e mesh failures #11286

Comments

dprotaso commented Apr 30, 2021 • edited Loading

Expected Behavior

Actual Behavior

Steps to Reproduce the Problem

Additional Info

dprotaso commented May 3, 2021

nak3 commented May 10, 2021 • edited Loading

arturenault commented May 10, 2021

dprotaso commented May 11, 2021 • edited Loading

nak3 commented Jun 11, 2021

dprotaso commented Jun 11, 2021

ZhiminXiang commented Jun 11, 2021

ZhiminXiang commented Jun 11, 2021

dprotaso commented Jun 11, 2021

julz commented Jun 11, 2021

ZhiminXiang commented Jun 11, 2021

dprotaso commented Jun 12, 2021

nak3 commented Jun 14, 2021

arturenault commented Jun 14, 2021

nak3 commented Jun 21, 2021

option 1. Use short option for pre-submit test.

option 2. Add new job trigger /test pull-knative-serving-istio-stable-mesh-short to run mesh w/ -short.

nak3 commented Jun 21, 2021

julz commented Jun 21, 2021

dprotaso commented Jun 21, 2021

dprotaso commented Jun 21, 2021 • edited Loading

nak3 commented Jun 28, 2021

julz commented Jun 28, 2021

dprotaso commented Jun 28, 2021

nak3 commented Jun 29, 2021

dprotaso commented Jul 5, 2021

nak3 commented Aug 28, 2021 • edited Loading

dprotaso commented Apr 30, 2021 •

edited

Loading

nak3 commented May 10, 2021 •

edited

Loading

dprotaso commented May 11, 2021 •

edited

Loading

option 2. Add new job trigger `/test pull-knative-serving-istio-stable-mesh-short` to run mesh w/ -short.

dprotaso commented Jun 21, 2021 •

edited

Loading

nak3 commented Aug 28, 2021 •

edited

Loading