Skip to content

Commit e8b2548

Browse files
authored
Fix ambient multi-cluster integration tests (#58466)
It appears that TestServerSideLB is flaky in multi-network setup. I think there are two contributing factors for that: 1. With two clusters we have twice as many backends to hit, so the same 10 attempts may not always be enough 2. Specifically when waypoints are involved, we may be affected by istio/istio#58039, though after running the test locally, it does not seem critical. Aside from that, I also cleaned up some of the skips that were added when we just started running ambient integration tests in multi-cluster environment. As things stand now there are just three known reasons why we should skip a test in ambient multi-cluster: 1. When in test workload behind a sidecar proxy tries to talk to a workload behind ztunnel (but not other way around) - the fix for that is tracked in istio/istio#57878, but given that interoperability between ambient and sidecar is not officially supported, it's ok to skip those tests. 2. We skip TestServiceDynamicEnroll because it's flaky in general and running it in single cluster mode as well as in multi-cluster mode just increases the chances of hitting a flake for somewhat unclear benefits at the moment; there is a ticket open to fix the test - istio/istio#58228. 3. We skip TestTrafficSplit at the moment because it relies on subsetting and we lose that information when we cross network boundary; it's debatable if we want to support this feature at all in multi-cluster, but for now we track it in istio/istio#58140. All other tests should be running and passing in ambient multi-cluster mode. Fixes #58020 Fixes #58080 Fixes #56228 Signed-off-by: Mikhail Krinkin <[email protected]>
1 parent da480ff commit e8b2548

File tree

1 file changed

+7
-25
lines changed

1 file changed

+7
-25
lines changed

tests/integration/ambient/baseline_test.go

Lines changed: 7 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ func TestServices(t *testing.T) {
179179
opt.Check = tcpValidator
180180
}
181181

182-
if src.Config().HasSidecar() && t.Settings().AmbientMultiNetwork {
182+
if t.Settings().AmbientMultiNetwork && src.Config().HasSidecar() && !dst.Config().HasSidecar() {
183183
t.Skip("https://github.com/istio/istio/issues/57878")
184184
}
185185

@@ -355,8 +355,9 @@ func TestServerSideLB(t *testing.T) {
355355
shouldBalance := dst.Config().HasServiceAddressedWaypointProxy()
356356
// Istio client will not reuse connections for HTTP/1.1
357357
opt.HTTP.HTTP2 = true
358-
// Make sure we make multiple calls
359-
opt.Count = 10
358+
// Make sure we make multiple calls and scale the number of calls with the number of clusters as well.
359+
// We need to make sure that we run enough requests to cover all the backends we want to hit.
360+
opt.Count = 10 * len(t.AllClusters())
360361
c := singleHost
361362
if shouldBalance {
362363
c = multipleHost
@@ -950,19 +951,6 @@ spec:
950951
})
951952
}
952953

953-
func sidecarInjected(t framework.TestContext, es ...echo.Target) bool {
954-
t.Helper()
955-
956-
for _, e := range es {
957-
for _, w := range e.WorkloadsOrFail(t) {
958-
if w.Sidecar() != nil {
959-
return true
960-
}
961-
}
962-
}
963-
return false
964-
}
965-
966954
func TestAuthorizationL4(t *testing.T) {
967955
framework.NewTest(t).Run(func(t framework.TestContext) {
968956
applyDrainingWorkaround(t)
@@ -972,7 +960,7 @@ func TestAuthorizationL4(t *testing.T) {
972960
t.Skip("this test is only for L4/TCP")
973961
}
974962

975-
if t.Settings().AmbientMultiNetwork && sidecarInjected(t, src) && !sidecarInjected(t, dst) {
963+
if t.Settings().AmbientMultiNetwork && src.Config().HasSidecar() && !dst.Config().HasSidecar() {
976964
// Currently sidecars cannot talk to ambient endpoints on a remote network.
977965
//
978966
// Sidecars can't use double-HBONE and therefore cannot use ambient E/W gateways.
@@ -1029,7 +1017,7 @@ func TestAuthorizationL4(t *testing.T) {
10291017

10301018
for _, tc := range authzCases {
10311019
t.NewSubTest(tc.name).Run(func(t framework.TestContext) {
1032-
if t.Settings().AmbientMultiNetwork && (src.Config().HasSidecar() || dst.Config().HasSidecar()) {
1020+
if t.Settings().AmbientMultiNetwork && src.Config().HasSidecar() && !dst.Config().HasSidecar() {
10331021
// Sidecar + Ambient is not supported
10341022
t.Skip("https://github.com/istio/istio/issues/57878")
10351023
}
@@ -1141,9 +1129,6 @@ func TestAuthorizationGateway(t *testing.T) {
11411129
Check: check.OK(),
11421130
To: dst,
11431131
}
1144-
if t.Settings().AmbientMultiNetwork {
1145-
t.Skip("https://github.com/istio/istio/issues/57878")
1146-
}
11471132
f(t, istio.DefaultIngressOrFail(t, t), dst, opt)
11481133
})
11491134
}
@@ -2881,7 +2866,7 @@ func runTestContextForCalls(
28812866
t.NewSubTestf("from %v %v", src.Config().Cluster.Name(), src.Config().Service).Run(func(t framework.TestContext) {
28822867
for _, dst := range getAllInstancesByServiceName() {
28832868
t.NewSubTestf("to all %v", dst.Config().Service).Run(func(t framework.TestContext) {
2884-
if t.Settings().AmbientMultiNetwork && (src.Config().HasSidecar() || dst.Config().HasSidecar()) {
2869+
if t.Settings().AmbientMultiNetwork && src.Config().HasSidecar() && !dst.Config().HasSidecar() {
28852870
// Skip sidecar to sidecar in multinetwork, as they will use the east-west gateway which is not tested here.
28862871
t.Skip("https://github.com/istio/istio/issues/57878")
28872872
}
@@ -3803,9 +3788,6 @@ spec:
38033788
func TestWaypointWithSidecarBackend(t *testing.T) {
38043789
framework.NewTest(t).
38053790
Run(func(t framework.TestContext) {
3806-
if t.Settings().AmbientMultiNetwork {
3807-
t.Skip("https://github.com/istio/istio/issues/57878")
3808-
}
38093791
// Ensure we go through the waypoint (verified by modifying the request) and that we are doing mTLS.
38103792
t.ConfigIstio().
38113793
Eval(apps.Namespace.Name(), apps.Namespace.Name(), `apiVersion: gateway.networking.k8s.io/v1

0 commit comments

Comments
 (0)