feat: Cleanup orphaned Istio gateway leases

[Trevor Williams (trevorwilliams2025)]

Summary

Add automatic cleanup of orphaned AKS-managed Istio gateway leader-election
leases after mesh upgrades complete and reach a stable state.

Addresses: Azure/AKS#5862

Problem

During Istio mesh upgrades, AKS leaves behind orphaned gateway leader-election
leases (istio-gateway-deployment-asm-*, istio-gateway-status-leader-asm-*)
for retired revisions. These leases are harmless but accumulate and clutter the
cluster. Until AKS adds proper garbage collection via owner references, we need
to clean them up ourselves.

Solution

Post-upgrade reconciliation that safely deletes retired gateway leases by:

• Running only when the mesh is fully stable:
  • Cluster provisioning has completed (ProvisioningState == "Succeeded")
  • No upgrade is in progress
  • Exactly one mesh revision is installed
  • That revision matches the configured target
• Deleting only leases whose revision is no longer installed
• Treating NotFound errors as benign (idempotent operation)
• Logging errors but not blocking the upgrade (non-fatal)

Changes

• leases.go (new): ReconcileRetiredGatewayLeases() function
• leases_test.go (new): Unit + integration tests
• upgrade.go: Call reconciliation after stable state verification in:
  • runReconcile() (no upgrade needed)
  • runCanaryPostInstall() (after canary completes)
  • runCleanupAndUpgrade() (after cleanup verification)

Testing

• All existing tests pass
• New test coverage for:
  • Lease deletion (both formats)
  • Preservation of active leases
  • Skipping during unstable states (upgrade in progress, wrong revision, etc.)

Deployment

• ARO-Tools only — no ARO-HCP config changes needed
• ARO-HCP will pick up via dependency bump
• Safe to deploy anytime; will be a no-op if no retired leases exist
• Future: When AKS adds proper GC, this code becomes a harmless no-op

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: trevorwilliams2025
Once this PR has been reviewed and has the lgtm label, please assign raelga for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

Hi Trevor Williams (@trevorwilliams2025). Thanks for your PR.

I'm waiting for a Azure member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Tip

We noticed you've done this a few times! Consider joining the org to skip this step and gain /lgtm and other bot rights. We recommend asking approvers on your previous PRs to sponsor you.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[Copilot]

Pull request overview

Adds post-upgrade cleanup for orphaned AKS Istio gateway leases once the mesh is stable.

Changes:

• Adds lease discovery, filtering, deletion, and NotFound handling.
• Integrates cleanup into upgrade and reconciliation flows.
• Adds tests and updates workspace checksums.
• upgrade.go should log ARM stability-read failures and skip cleanup rather than failing otherwise successful flows.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tools/istio-upgrade/pkg/istio/upgrade.go	Integrates stable-state lease reconciliation.
tools/istio-upgrade/pkg/istio/upgrade_test.go	Updates reconciliation test calls.
tools/istio-upgrade/pkg/istio/leases.go	Implements retired gateway lease cleanup.
tools/istio-upgrade/pkg/istio/leases_test.go	Tests lease cleanup and stability gating.
go.work.sum	Updates workspace dependency checksums.

Suppressed comments (5)

tools/istio-upgrade/pkg/istio/leases_test.go:86

• The stability gate has four independent conditions, but this test exercises only UpgradeInProgress. A regression in provisioning state, revision count, or the installed-revision/target match could then permit deletion without being caught; add cases for each remaining condition and assert the lease is preserved.

	t.Run("skips while mesh is not stable", func(t *testing.T) {
		ctx := logr.NewContext(context.Background(), testr.New(t))
		client := fake.NewSimpleClientset(
			&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: istioSystemNamespace}},
			gatewayLease("istio-gateway-deployment-asm-1-28"),
		)

		err := reconcileRetiredGatewayLeases(
			ctx,
			logr.FromContextOrDiscard(ctx),
			&fakeAKSClient{
				clusterInfo: &ClusterInfo{ProvisioningState: "Succeeded"},
				meshProfile: &MeshProfile{Revisions: []string{"asm-1-29"}},
				upgradeInfo: &MeshUpgradeInfo{UpgradeInProgress: true},
			},
			NewKubeClientFromInterface(client),
			DefaultUpgradeOptions(),
			"asm-1-29",

tools/istio-upgrade/pkg/istio/leases_test.go:67

• The tests cover only successful deletes, not the explicit idempotency and failure branches below. Add fake-client reactors for a delete that races and returns NotFound, plus a non-NotFound delete/list error, so the benign handling and error propagation cannot regress.

func TestReconcileRetiredGatewayLeases(t *testing.T) {
	t.Run("deletes only retired gateway lease formats", func(t *testing.T) {
		client := fake.NewSimpleClientset(
			&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: istioSystemNamespace}},
			gatewayLease("istio-gateway-deployment-asm-1-28"),
			gatewayLease("istio-gateway-status-leader-asm-1-28"),
			gatewayLease("istio-gateway-deployment-asm-1-29"),
			gatewayLease("some-other-lease"),
		)

		err := ReconcileRetiredGatewayLeases(
			context.Background(),
			NewKubeClientFromInterface(client),
			[]string{"asm-1-29"},
		)
		require.NoError(t, err)

		_, err = client.CoordinationV1().Leases(istioSystemNamespace).Get(
			context.Background(), "istio-gateway-deployment-asm-1-28", metav1.GetOptions{})
		assert.True(t, apierrors.IsNotFound(err))

		_, err = client.CoordinationV1().Leases(istioSystemNamespace).Get(
			context.Background(), "istio-gateway-deployment-asm-1-29", metav1.GetOptions{})
		require.NoError(t, err)

		_, err = client.CoordinationV1().Leases(istioSystemNamespace).Get(
			context.Background(), "some-other-lease", metav1.GetOptions{})
		require.NoError(t, err)
	})

tools/istio-upgrade/pkg/istio/leases_test.go:44

• The test creates the retired status-leader lease but never verifies that it is deleted; an implementation that failed to match this second supported format would still pass. Add an IsNotFound assertion for that lease so the advertised support for both formats is actually covered.

			gatewayLease("istio-gateway-status-leader-asm-1-28"),

tools/istio-upgrade/pkg/istio/upgrade.go:694

• This branch has the same non-fatality problem: a transient failure listing upgrade targets turns an otherwise completed upgrade into an error. Since cleanup is optional, log the failed stability check and return nil (without deleting leases) instead of propagating it to the upgrade pipeline.

		return fmt.Errorf("get Istio upgrade state before retired lease reconciliation: %w", err)

tools/istio-upgrade/pkg/istio/upgrade.go:702

• The new stability test exercises only UpgradeInProgress; it does not cover the other three independent guards here (ProvisioningState, exactly one installed revision, and revision matching target). These are safety-critical because a regression could delete leases during a provisioning transition or while the configured revision is not the installed one, so add table-driven cases that assert the retired lease remains for each condition.

	if clusterInfo.ProvisioningState != "Succeeded" ||
		upgradeInfo.UpgradeInProgress ||
		len(meshProfile.Revisions) != 1 ||
		meshProfile.Revisions[0] != target {

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

Update lease RBAC documentation/permissions and cover the promised delete error paths.

Review details

Suppressed comments (2)

tools/istio-upgrade/pkg/istio/leases.go:47

• This adds list/delete access to coordination.k8s.io/leases, but the command's documented Kubernetes permission set in tools/istio-upgrade/cmd/run/cmd.go:22-26 does not include leases. With that least-privilege kubeconfig, the new list will be forbidden and this helper will silently log and skip cleanup on every run, so the feature will not work; update the deployed RBAC/permission documentation alongside this change.

	leases, err := kubeClient.client.CoordinationV1().
		Leases(istioSystemNamespace).
		List(ctx, metav1.ListOptions{})

tools/istio-upgrade/pkg/istio/leases.go:68

• The idempotent/non-fatal delete contract is not covered: the tests never make a lease disappear between List/Delete to exercise IsNotFound, nor make Delete return another error while asserting reconciliation continues. These branches are part of the promised safety behavior, so add focused fake-client reactor tests before relying on this cleanup in upgrades.

		if err := kubeClient.client.CoordinationV1().
			Leases(istioSystemNamespace).
			Delete(ctx, lease.Name, metav1.DeleteOptions{}); err != nil {
			if apierrors.IsNotFound(err) {
				continue

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite