cilium-cli: skip identity-less endpoints in policy-revision wait

After applying a policy, the connectivity test waits for every endpoint on each
Cilium node to realize the new revision before running the scenario. The wait
polls "cilium endpoint list" and counts an endpoint as not-ready whenever its
realized policy revision is missing or below the target. That count includes
every endpoint on the node, not just the ones the policy was applied to.

A pod that was just scheduled onto the node shows up in the list before it has
been assigned a security identity. The agent reports such an endpoint with a nil
policy status (GetPolicyModel returns nil while SecurityIdentity is nil) and a
nil identity, so it is counted as "not realized" even though it has computed no
policy at all and is enforcing no revision. Under --test-concurrency>1 a sibling
test namespace is frequently creating a pod on the shared node, so there is
almost always such an endpoint in flight and the wait can never reach zero: it
burns the entire PolicyWaitTimeout and then fails setup, either with "N
endpoints have not yet realized policy revision R" or, when the deadline lands
during the endpoint-list exec, with an opaque "context deadline exceeded". On
AKS with IPsec the extra regeneration latency and high concurrency make a fresh
uninitialized endpoint almost always present during the poll. This is what
cilium#46877 was chasing when it dropped the node-wide Ready requirement, but the
residual uninitialized case remained.

Skip endpoints that have no security identity yet (nil Status.Identity). An
endpoint without an identity has computed no policy, so skipping it cannot let
the test run against an endpoint still enforcing an old policy; once it gets an
identity, regeneration computes policy at the current revision and it is counted
again. Endpoints that do have an identity are counted as before, so a genuinely
lagging endpoint still holds the wait. This corrects what is counted rather than
bumping the timeout, which would only hide a truly stuck endpoint.

Add a table-driven unit test over the counting logic covering the nil-identity
skip, the identity-with-stale-revision count, and the identity-at-revision
no-count cases.

Seen in https://github.com/cilium/cilium/actions/runs/29349504650

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

Author: aanm

cilium-cli/connectivity/check/policy.go

@@ -21,6 +21,7 @@ import (
 	policyv1alpha2 "sigs.k8s.io/network-policy-api/apis/v1alpha2"
 
 	flowpb "github.com/cilium/cilium/api/v1/flow"
+	"github.com/cilium/cilium/api/v1/models"
 	"github.com/cilium/cilium/cilium-cli/defaults"
 	"github.com/cilium/cilium/cilium-cli/k8s"
 	"github.com/cilium/cilium/cilium-cli/utils/features"
@@ -129,23 +130,34 @@ func waitCiliumPolicyRevision(ctx context.Context, pod Pod, rev int, timeout tim
 }
 
 // endpointsBelowPolicyRevision returns how many endpoints managed by the given
-// Cilium pod have not yet realized the given policy revision. Endpoints with an
-// unknown realized revision are counted as not ready.
+// Cilium pod have not yet realized the given policy revision.
 func endpointsBelowPolicyRevision(ctx context.Context, pod Pod, rev int) (int, error) {
 	eps, err := pod.K8sClient.CiliumDbgEndpoints(ctx, pod.Pod.Namespace, pod.Pod.Name)
 	if err != nil {
 		return 0, err
 	}
+	return countEndpointsBelowPolicyRevision(eps, rev), nil
+}
 
+// countEndpointsBelowPolicyRevision counts endpoints that have not yet realized
+// the given policy revision. Endpoints without a security identity yet are
+// skipped: they have computed no policy at all, so they enforce no revision we
+// could be waiting on, and once they get an identity regeneration computes
+// policy at the current revision. Endpoints that do have an identity but a
+// missing or older realized revision are counted as not ready.
+func countEndpointsBelowPolicyRevision(eps []*models.Endpoint, rev int) int {
 	notReady := 0
 	for _, ep := range eps {
-		if ep.Status == nil || ep.Status.Policy == nil ||
+		if ep.Status == nil || ep.Status.Identity == nil {
+			continue
+		}
+		if ep.Status.Policy == nil ||
 			ep.Status.Policy.Realized == nil ||
 			ep.Status.Policy.Realized.PolicyRevision < int64(rev) {
 			notReady++
 		}
 	}
-	return notReady, nil
+	return notReady
 }
 
 type policy interface {

cilium-cli/connectivity/check/policy_test.go

@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright Authors of Cilium
+
+package check
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/cilium/cilium/api/v1/models"
+)
+
+func TestCountEndpointsBelowPolicyRevision(t *testing.T) {
+	identity := &models.Identity{ID: 12345}
+
+	realized := func(rev int64) *models.EndpointStatus {
+		return &models.EndpointStatus{
+			Identity: identity,
+			Policy: &models.EndpointPolicyStatus{
+				Realized: &models.EndpointPolicy{PolicyRevision: rev},
+			},
+		}
+	}
+
+	tests := []struct {
+		name string
+		eps  []*models.Endpoint
+		rev  int
+		want int
+	}{
+		{
+			name: "no endpoints",
+			eps:  nil,
+			rev:  10,
+			want: 0,
+		},
+		{
+			name: "identity-less endpoint is skipped",
+			eps: []*models.Endpoint{
+				{Status: &models.EndpointStatus{Identity: nil}},
+			},
+			rev:  10,
+			want: 0,
+		},
+		{
+			name: "nil status is skipped",
+			eps: []*models.Endpoint{
+				{Status: nil},
+			},
+			rev:  10,
+			want: 0,
+		},
+		{
+			name: "identity endpoint with stale realized revision is counted",
+			eps: []*models.Endpoint{
+				{Status: realized(9)},
+			},
+			rev:  10,
+			want: 1,
+		},
+		{
+			name: "identity endpoint with nil policy is counted",
+			eps: []*models.Endpoint{
+				{Status: &models.EndpointStatus{Identity: identity, Policy: nil}},
+			},
+			rev:  10,
+			want: 1,
+		},
+		{
+			name: "identity endpoint with nil realized is counted",
+			eps: []*models.Endpoint{
+				{Status: &models.EndpointStatus{
+					Identity: identity,
+					Policy:   &models.EndpointPolicyStatus{Realized: nil},
+				}},
+			},
+			rev:  10,
+			want: 1,
+		},
+		{
+			name: "identity endpoint at target revision is not counted",
+			eps: []*models.Endpoint{
+				{Status: realized(10)},
+			},
+			rev:  10,
+			want: 0,
+		},
+		{
+			name: "identity endpoint ahead of target revision is not counted",
+			eps: []*models.Endpoint{
+				{Status: realized(11)},
+			},
+			rev:  10,
+			want: 0,
+		},
+		{
+			name: "mix: only identity-bearing lagging endpoints are counted",
+			eps: []*models.Endpoint{
+				{Status: realized(11)},                               // ahead, not counted
+				{Status: realized(9)},                                // stale, counted
+				{Status: &models.EndpointStatus{Identity: nil}},      // no identity, skipped
+				{Status: nil},                                        // no status, skipped
+				{Status: &models.EndpointStatus{Identity: identity}}, // identity, nil policy, counted
+			},
+			rev:  10,
+			want: 2,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, countEndpointsBelowPolicyRevision(tt.eps, tt.rev))
+		})
+	}
+}