cilium-cli/connectivity: don't retry expected-drop actions

Commit 39c5e16 attached WithRetryCondition(WithRetryAll()) to
echo-ingress-l7-named-port so a transient first-packet loss or Envoy
warm-up on the allowed requests no longer fails the test on the first
attempt. Its message assumed the expected-drop actions were unaffected
beyond "only adding retry delay", but that delay is the whole cost: the
scenario's denied actions are silently dropped by policy, so their curl
hits --connect-timeout and exits 28, and --retry re-issues each one three
times with --retry-delay in between. curl has no zero-delay retry mode, so
every denied action pays the full retry budget re-sending a request that is
meant to fail.

On the v1.18 ci-ipsec-upgrade runs this inflated echo-ingress-l7-named-port
from ~52s to ~450s in the concurrent phase and, once the cli bumped to the
release vendoring 39c5e16, pushed the job past its 45 minute limit.

Gate retry-option generation on the action's expected result in the shared
retryCondition.CurlOptions: retrying an expected drop can never turn it into
the expected result, so return no retry options when a drop is expected. The
expectation is resolved in NewAction before the action's Run closure runs, so
a.ExpectingSuccess() is already known at the call sites. This covers every
retry user, not just echo-ingress-l7-named-port: client-egress-l7-set-header
(also PodToPodWithEndpoints + WithRetryAll) and client-egress-tls-sni
(PodToWorld + WithRetryAll) likewise stopped retrying their denied actions.
The scoped conditions (WithRetryDestIP/DestPort/PodLabel) already matched only
the allowed destinations, so their behavior is unchanged.

The allowed requests keep their retries, so the flake 39c5e16 fixed stays
fixed. While here, clone the shared base curl options per action in
podToPodWithEndpoints so a drop action never inherits retry flags appended by
a sibling success action in the same scenario.

This commit was prepared with AIL:3.

Signed-off-by: André Martins <andre@cilium.io>

Author: aanm

cilium-cli/connectivity/check/action.go

@@ -1197,14 +1197,16 @@ func (a *Action) validateMetric(ctx context.Context, node string, result Metrics
 	}
 }
 
-func (a *Action) expectingSuccess() bool {
+// ExpectingSuccess reports whether the action's registered expectation is a
+// successful command (exit code 0), as opposed to an expected drop.
+func (a *Action) ExpectingSuccess() bool {
 	return a.expectedExitCode() == ExitCode(0)
 }
 
 func (a *Action) CurlCommandWithOutput(peer TestPeer, opts ...string) []string {
-	return a.test.ctx.CurlCommandWithOutput(peer, a.IPFamily(), a.expectingSuccess(), opts)
+	return a.test.ctx.CurlCommandWithOutput(peer, a.IPFamily(), a.ExpectingSuccess(), opts)
 }
 
 func (a *Action) CurlCommand(peer TestPeer, opts ...string) []string {
-	return a.test.ctx.CurlCommand(peer, a.IPFamily(), a.expectingSuccess(), opts)
+	return a.test.ctx.CurlCommand(peer, a.IPFamily(), a.ExpectingSuccess(), opts)
 }

cilium-cli/connectivity/tests/common.go

@@ -71,10 +71,18 @@ type retryCondition struct {
 }
 
 // CurlOptions returns curl retry option or empty slice depending on retry conditions
-func (rc *retryCondition) CurlOptions(peer check.TestPeer, ipFam features.IPFamily, pod check.Pod, params check.Parameters) []string {
+func (rc *retryCondition) CurlOptions(peer check.TestPeer, ipFam features.IPFamily, pod check.Pod, params check.Parameters, expectSuccess bool) []string {
 	if params.Retry == 0 {
 		return []string{}
 	}
+	// Never retry an action that is expected to be dropped. curl cannot retry
+	// without waiting between attempts, so retrying a denied request just adds
+	// retry delay while it re-issues traffic meant to fail; the retry condition
+	// exists to paper over transient failures of the allowed requests, not the
+	// denied ones.
+	if !expectSuccess {
+		return []string{}
+	}
 	if !rc.all && rc.destIP == "" && rc.destPort == 0 {
 		return []string{}
 	}

cilium-cli/connectivity/tests/common_test.go

@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright Authors of Cilium
+
+package tests
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/cilium/cilium/cilium-cli/connectivity/check"
+	"github.com/cilium/cilium/cilium-cli/utils/features"
+)
+
+func TestRetryConditionCurlOptions(t *testing.T) {
+	params := check.Parameters{Retry: 3, RetryDelay: 3 * time.Second}
+	ep := check.HTTPEndpoint("ep", "http://192.0.2.1:80/public")
+	pod := check.Pod{Pod: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"kind": "client"}}}}
+
+	newRC := func(opts ...RetryOption) *retryCondition {
+		rc := &retryCondition{}
+		for _, o := range opts {
+			o(rc)
+		}
+		return rc
+	}
+
+	retryOpts := []string{"--retry", "3", "--retry-all-errors", "--retry-delay", "3"}
+
+	// A drop is expected: no retry flags regardless of the retry condition,
+	// because curl always waits between retries and retrying a request meant to
+	// fail only adds retry delay.
+	assert.Empty(t, newRC(WithRetryAll()).CurlOptions(ep, features.IPFamilyV4, pod, params, false),
+		"WithRetryAll must not emit retry options when a drop is expected")
+	assert.Empty(t, newRC(WithRetryDestPort(80)).CurlOptions(ep, features.IPFamilyV4, pod, params, false),
+		"scoped condition must not emit retry options when a drop is expected")
+
+	// Success expected + WithRetryAll: emit the retry flags.
+	assert.Equal(t, retryOpts, newRC(WithRetryAll()).CurlOptions(ep, features.IPFamilyV4, pod, params, true))
+
+	// Retry disabled globally: never emit, even when success is expected.
+	assert.Empty(t, newRC(WithRetryAll()).CurlOptions(ep, features.IPFamilyV4, pod,
+		check.Parameters{Retry: 0}, true))
+
+	// Success expected but no retry condition set: nothing to emit.
+	assert.Empty(t, newRC().CurlOptions(ep, features.IPFamilyV4, pod, params, true))
+
+	// Matching dest-port condition + success: emit.
+	assert.Equal(t, retryOpts, newRC(WithRetryDestPort(80)).CurlOptions(ep, features.IPFamilyV4, pod, params, true))
+
+	// Non-matching dest-port condition: no retry even on success.
+	assert.Empty(t, newRC(WithRetryDestPort(8080)).CurlOptions(ep, features.IPFamilyV4, pod, params, true))
+}

cilium-cli/connectivity/tests/pod.go

@@ -10,6 +10,7 @@ import (
 	"net"
 	"net/netip"
 	"regexp"
+	"slices"
 	"strconv"
 	"strings"
 
@@ -164,8 +165,9 @@ func (s *podToPodWithEndpoints) curlEndpoints(ctx context.Context, t *check.Test
 		ep := check.HTTPEndpointWithLabels(epName, url, echo.Labels())
 
 		t.NewAction(s, epName, client, ep, ipFam).Run(func(a *check.Action) {
-			curlOpts = append(curlOpts, s.retryCondition.CurlOptions(ep, ipFam, *client, ct.Params())...)
-			a.ExecInPod(ctx, a.CurlCommand(ep, curlOpts...))
+			opts := slices.Clone(curlOpts)
+			opts = append(opts, s.retryCondition.CurlOptions(ep, ipFam, *client, ct.Params(), a.ExpectingSuccess())...)
+			a.ExecInPod(ctx, a.CurlCommand(ep, opts...))
 
 			a.ValidateFlows(ctx, client, a.GetEgressRequirements(check.FlowParameters{}))
 			a.ValidateFlows(ctx, ep, a.GetIngressRequirements(check.FlowParameters{}))
@@ -178,8 +180,8 @@ func (s *podToPodWithEndpoints) curlEndpoints(ctx context.Context, t *check.Test
 			labels["X-Very-Secret-Token"] = "42"
 			ep = check.HTTPEndpointWithLabels(epName, url, labels)
 			t.NewAction(s, epName, client, ep, ipFam).Run(func(a *check.Action) {
-				opts := make([]string, 0, len(curlOpts)+2)
-				opts = append(opts, curlOpts...)
+				opts := slices.Clone(curlOpts)
+				opts = append(opts, s.retryCondition.CurlOptions(ep, ipFam, *client, ct.Params(), a.ExpectingSuccess())...)
 				opts = append(opts, "-H", "X-Very-Secret-Token: 42")
 
 				a.ExecInPod(ctx, a.CurlCommand(ep, opts...))

cilium-cli/connectivity/tests/to-cidr.go

@@ -54,7 +54,7 @@ func (s *podToCIDR) Run(ctx context.Context, t *check.Test) {
 		var i int
 		for _, src := range ct.ClientPods() {
 			t.NewAction(s, fmt.Sprintf("%s-%d", ep.Name(), i), &src, ep, features.GetIPFamily(ip)).Run(func(a *check.Action) {
-				opts := s.rc.CurlOptions(ep, features.GetIPFamily(ip), src, ct.Params())
+				opts := s.rc.CurlOptions(ep, features.GetIPFamily(ip), src, ct.Params(), a.ExpectingSuccess())
 				a.ExecInPod(ctx, a.CurlCommand(ep, opts...))
 
 				a.ValidateFlows(ctx, src, a.GetEgressRequirements(check.FlowParameters{

cilium-cli/connectivity/tests/world.go

@@ -103,25 +103,25 @@ func (s *podToWorld) Run(ctx context.Context, t *check.Test) {
 			}
 
 			// With http, over port 80.
-			httpOpts := s.rc.CurlOptions(http, ipFam, client, ct.Params())
-			httpOpts = append(httpOpts, s.curlOptionalFakeDNS(extTarget, ipFam, t.Context().Params())...)
 			t.NewAction(s, fmt.Sprintf("http-to-%s-%s-%d", extTarget, ipFam, i), &client, http, ipFam).Run(func(a *check.Action) {
+				httpOpts := s.rc.CurlOptions(http, ipFam, client, ct.Params(), a.ExpectingSuccess())
+				httpOpts = append(httpOpts, s.curlOptionalFakeDNS(extTarget, ipFam, t.Context().Params())...)
 				a.ExecInPod(ctx, a.CurlCommand(http, httpOpts...))
 				a.ValidateFlows(ctx, client, a.GetEgressRequirements(fp))
 			})
 
 			// With https, over port 443.
-			httpsOpts := s.rc.CurlOptions(https, ipFam, client, ct.Params())
-			httpsOpts = append(httpsOpts, s.curlOptionalFakeDNS(extTarget, ipFam, t.Context().Params())...)
 			t.NewAction(s, fmt.Sprintf("https-to-%s-%s-%d", extTarget, ipFam, i), &client, https, ipFam).Run(func(a *check.Action) {
+				httpsOpts := s.rc.CurlOptions(https, ipFam, client, ct.Params(), a.ExpectingSuccess())
+				httpsOpts = append(httpsOpts, s.curlOptionalFakeDNS(extTarget, ipFam, t.Context().Params())...)
 				a.ExecInPod(ctx, a.CurlCommand(https, httpsOpts...))
 				a.ValidateFlows(ctx, client, a.GetEgressRequirements(fp))
 			})
 
 			// With https, over port 443, index.html.
-			httpsindexOpts := s.rc.CurlOptions(httpsindex, ipFam, client, ct.Params())
-			httpsindexOpts = append(httpsindexOpts, s.curlOptionalFakeDNS(extTarget, ipFam, t.Context().Params())...)
 			t.NewAction(s, fmt.Sprintf("https-to-%s-index-%s-%d", extTarget, ipFam, i), &client, httpsindex, ipFam).Run(func(a *check.Action) {
+				httpsindexOpts := s.rc.CurlOptions(httpsindex, ipFam, client, ct.Params(), a.ExpectingSuccess())
+				httpsindexOpts = append(httpsindexOpts, s.curlOptionalFakeDNS(extTarget, ipFam, t.Context().Params())...)
 				a.ExecInPod(ctx, a.CurlCommand(httpsindex, httpsindexOpts...))
 				a.ValidateFlows(ctx, client, a.GetEgressRequirements(fp))
 			})