Keep the outdated phase until the runner spec itself changes

apis/actions.github.com/v1alpha1/autoscalinglistener_types.go

@@ -21,8 +21,39 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+// AutoscalingListenerPhase describes whether the listener should be running.
+//
+// It is part of the spec rather than the status because it is a desired state
+// written by the AutoscalingRunnerSet controller, not an observation made by the
+// AutoscalingListener controller about itself.
+type AutoscalingListenerPhase string
+
+const (
+	// AutoscalingListenerPhaseRunning is the default. The listener controller
+	// creates and maintains the listener pod and everything it depends on.
+	AutoscalingListenerPhaseRunning AutoscalingListenerPhase = "Running"
+
+	// AutoscalingListenerPhaseStopped switches the listener off without
+	// deleting it. The listener controller tears the pod and the rest of the
+	// child resources down, so no further jobs are acquired, but the
+	// AutoscalingListener object stays as the record of a scale set that is
+	// meant to come back. Moving the phase to Running is what starts it again.
+	AutoscalingListenerPhaseStopped AutoscalingListenerPhase = "Stopped"
+)
+
+// Stopped reports whether the phase switches the listener off. The zero value
+// means Running, so listeners written before this field existed keep working.
+func (p AutoscalingListenerPhase) Stopped() bool {
+	return p == AutoscalingListenerPhaseStopped
+}
+
 // AutoscalingListenerSpec defines the desired state of AutoscalingListener
 type AutoscalingListenerSpec struct {
+	// Phase controls whether the listener runs. Empty means Running.
+	// +kubebuilder:validation:Enum=Running;Stopped
+	// +optional
+	Phase AutoscalingListenerPhase `json:"phase,omitempty"`
+
 	// +optional
 	GitHubConfigURL string `json:"githubConfigUrl,omitempty"`
 
@@ -94,6 +125,7 @@ type AutoscalingListenerStatus struct{}
 // +kubebuilder:printcolumn:JSONPath=".spec.githubConfigUrl",name=GitHub Configure URL,type=string
 // +kubebuilder:printcolumn:JSONPath=".spec.autoscalingRunnerSetNamespace",name=AutoscalingRunnerSet Namespace,type=string
 // +kubebuilder:printcolumn:JSONPath=".spec.autoscalingRunnerSetName",name=AutoscalingRunnerSet Name,type=string
+// +kubebuilder:printcolumn:JSONPath=".spec.phase",name=Phase,type=string
 
 // AutoscalingListener is the Schema for the autoscalinglisteners API
 type AutoscalingListener struct {

controllers/actions.github.com/autoscalinglistener_controller.go

@@ -148,6 +148,26 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl.
 		return ctrl.Result{}, err
 	}
 
+	// A stopped listener keeps its object, spec and finalizer, but owns nothing:
+	// the AutoscalingRunnerSet parks it this way instead of deleting it so the
+	// scale set stops acquiring jobs without re-registering with the Actions
+	// service when it comes back. Patching the phase back to Running falls
+	// through to the regular reconcile below, which rebuilds the children.
+	if autoscalingListener.Spec.Phase.Stopped() {
+		log.Info("Listener is stopped, cleaning up its resources")
+		requeue, err := r.cleanupResources(ctx, &autoscalingListener, log)
+		if err != nil {
+			log.Error(err, "Failed to clean up the resources of a stopped listener")
+			return ctrl.Result{}, err
+		}
+		if requeue {
+			return ctrl.Result{RequeueAfter: time.Second}, nil
+		}
+
+		log.Info("Listener is stopped and all of its resources are cleaned up")
+		return ctrl.Result{}, nil
+	}
+
 	// Make sure the runner scale set listener service account is created for the listener pod in the controller namespace
 	var serviceAccount corev1.ServiceAccount
 	err := r.Get(
@@ -621,24 +641,29 @@ func (r *AutoscalingListenerReconciler) cleanupResources(ctx context.Context, au
 		return false, fmt.Errorf("failed to get listener config secret: %w", err)
 	}
 
-	if autoscalingListener.Spec.Proxy != nil {
-		logger.Info("Cleaning up the listener proxy secret")
-		proxySecret := new(corev1.Secret)
-		err = r.Get(ctx, types.NamespacedName{Name: proxyListenerSecretName(autoscalingListener), Namespace: autoscalingListener.Namespace}, proxySecret)
-		switch {
-		case err == nil:
-			if proxySecret.DeletionTimestamp.IsZero() {
-				logger.Info("Deleting the listener proxy secret")
-				if err := r.Delete(ctx, proxySecret); err != nil {
-					return false, fmt.Errorf("failed to delete listener proxy secret: %w", err)
-				}
+	// The proxy secret is deleted whatever the current spec says about a proxy.
+	// Its name is derived from the listener rather than from the spec, and the
+	// spec of a stopped listener is updated in place as the AutoscalingRunnerSet
+	// is edited, so asking the spec would let a user who removes the proxy while
+	// the scale set is switched off erase the only signal that the old secret is
+	// there. It holds credentials, and a stopped listener is never deleted, so
+	// nothing else would ever collect it.
+	logger.Info("Cleaning up the listener proxy secret")
+	proxySecret := new(corev1.Secret)
+	err = r.Get(ctx, types.NamespacedName{Name: proxyListenerSecretName(autoscalingListener), Namespace: autoscalingListener.Namespace}, proxySecret)
+	switch {
+	case err == nil:
+		if proxySecret.DeletionTimestamp.IsZero() {
+			logger.Info("Deleting the listener proxy secret")
+			if err := r.Delete(ctx, proxySecret); err != nil {
+				return false, fmt.Errorf("failed to delete listener proxy secret: %w", err)
 			}
-			requeue = true
-		case !kerrors.IsNotFound(err):
-			return false, fmt.Errorf("failed to get listener proxy secret: %w", err)
 		}
-		logger.Info("Listener proxy secret is deleted")
+		requeue = true
+	case !kerrors.IsNotFound(err):
+		return false, fmt.Errorf("failed to get listener proxy secret: %w", err)
 	}
+	logger.Info("Listener proxy secret is deleted")
 
 	listenerRoleBinding := new(rbacv1.RoleBinding)
 	err = r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: autoscalingListener.Name}, listenerRoleBinding)

controllers/actions.github.com/autoscalinglistener_controller_test.go

@@ -646,6 +646,94 @@ var _ = Describe("Test AutoScalingListener controller", func() {
 			).Should(BeEquivalentTo(oldSecretUID), "Config secret should persist (not be re-created)")
 		})
 	})
+
+	Context("When the listener is stopped", func() {
+		listenerKey := func() client.ObjectKey {
+			return client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Namespace}
+		}
+
+		// The child resources are the listener's whole footprint: the pod that
+		// acquires jobs, the config secret holding its credentials, and the RBAC
+		// it runs under.
+		//
+		// A pod counts as removed once its deletion has been requested. envtest
+		// runs no kubelet, so nothing confirms the delete and the pod lingers
+		// Terminating for its whole 60s grace period; the controller has already
+		// done everything it can at that point.
+		expectChildResources := func(exist bool) {
+			GinkgoHelper()
+
+			Eventually(
+				func(g Gomega) {
+					pod := new(corev1.Pod)
+					err := k8sClient.Get(ctx, listenerKey(), pod)
+					if exist {
+						g.Expect(err).NotTo(HaveOccurred(), "pod should exist")
+						g.Expect(pod.DeletionTimestamp).To(BeNil(), "pod should not be terminating")
+					} else {
+						g.Expect(kerrors.IsNotFound(err) || (err == nil && pod.DeletionTimestamp != nil)).
+							To(BeTrue(), "pod should be removed while the listener is stopped")
+					}
+
+					for _, resource := range []struct {
+						name   string
+						key    client.ObjectKey
+						object client.Object
+					}{
+						{"config secret", client.ObjectKey{Name: scaleSetListenerConfigName(autoscalingListener), Namespace: autoscalingListener.Namespace}, new(corev1.Secret)},
+						{"service account", listenerKey(), new(corev1.ServiceAccount)},
+						{"role", client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, new(rbacv1.Role)},
+						{"role binding", client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, new(rbacv1.RoleBinding)},
+					} {
+						err := k8sClient.Get(ctx, resource.key, resource.object)
+						if exist {
+							g.Expect(err).NotTo(HaveOccurred(), "%s should exist", resource.name)
+							continue
+						}
+						g.Expect(kerrors.IsNotFound(err)).To(BeTrue(), "%s should be removed while the listener is stopped", resource.name)
+					}
+				},
+				autoscalingListenerTestTimeout,
+				autoscalingListenerTestInterval,
+			).Should(Succeed())
+		}
+
+		patchPhase := func(phase v1alpha1.AutoscalingListenerPhase) {
+			GinkgoHelper()
+
+			listener := new(v1alpha1.AutoscalingListener)
+			Expect(k8sClient.Get(ctx, listenerKey(), listener)).To(Succeed())
+			original := listener.DeepCopy()
+			listener.Spec.Phase = phase
+			Expect(k8sClient.Patch(ctx, listener, client.MergeFrom(original))).To(Succeed(), "failed to patch the listener phase")
+		}
+
+		It("removes the child resources but keeps the listener, and rebuilds them when started again", func() {
+			expectChildResources(true)
+
+			patchPhase(v1alpha1.AutoscalingListenerPhaseStopped)
+
+			expectChildResources(false)
+
+			// The listener itself is the record of a scale set that is meant to
+			// come back, so it survives with its finalizer and spec intact.
+			Consistently(
+				func(g Gomega) {
+					listener := new(v1alpha1.AutoscalingListener)
+					g.Expect(k8sClient.Get(ctx, listenerKey(), listener)).To(Succeed())
+					g.Expect(listener.DeletionTimestamp).To(BeNil(), "a stopped listener must not be deleted")
+					g.Expect(listener.Finalizers).To(ContainElement(autoscalingListenerFinalizerName))
+					g.Expect(listener.Spec.Phase).To(Equal(v1alpha1.AutoscalingListenerPhaseStopped))
+				},
+				2*time.Second,
+				autoscalingListenerTestInterval,
+			).Should(Succeed())
+
+			patchPhase(v1alpha1.AutoscalingListenerPhaseRunning)
+
+			expectChildResources(true)
+		})
+	})
 })
 
 var _ = Describe("Test AutoScalingListener customization", func() {

controllers/actions.github.com/autoscalinglistener_proxy_cleanup_test.go

@@ -0,0 +1,79 @@
+package actionsgithubcom
+
+import (
+	"context"
+	"testing"
+
+	"github.com/go-logr/logr"
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	kerrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	"github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1"
+)
+
+// The proxy secret holds credentials and is owned by the listener, so deleting
+// the listener collects it. A stopped listener is not deleted, which makes
+// cleanupResources the only thing that will ever remove it.
+//
+// Its name is derived from the listener, not from the spec, so cleanup must not
+// ask the spec whether it exists. The spec of a parked listener is updated in
+// place as the AutoscalingRunnerSet is edited, so a user who removes the proxy
+// while the scale set is switched off would otherwise erase the only signal that
+// the old secret is there, and leave the credentials behind indefinitely.
+func TestCleanupResourcesDeletesTheProxySecretWithoutAskingTheSpec(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, v1alpha1.AddToScheme(scheme))
+
+	const namespace = "arc-system"
+
+	listener := &v1alpha1.AutoscalingListener{
+		ObjectMeta: metav1.ObjectMeta{Name: "test-listener", Namespace: namespace},
+		Spec: v1alpha1.AutoscalingListenerSpec{
+			AutoscalingRunnerSetNamespace: "arc-runners",
+			AutoscalingRunnerSetName:      "test-ars",
+			Phase:                         v1alpha1.AutoscalingListenerPhaseStopped,
+			// The proxy has been removed from the scale set since the secret was
+			// created, and the parked listener has already taken that edit.
+			Proxy: nil,
+		},
+	}
+	proxySecret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      proxyListenerSecretName(listener),
+			Namespace: namespace,
+		},
+		Data: map[string][]byte{"password": []byte("secret")},
+	}
+
+	fakeClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(listener, proxySecret).
+		Build()
+
+	reconciler := &AutoscalingListenerReconciler{
+		Client: fakeClient,
+		Scheme: scheme,
+		Log:    logr.Discard(),
+	}
+
+	_, err := reconciler.cleanupResources(context.Background(), listener, logr.Discard())
+	require.NoError(t, err)
+
+	err = fakeClient.Get(
+		context.Background(),
+		types.NamespacedName{Name: proxySecret.Name, Namespace: namespace},
+		new(corev1.Secret),
+	)
+	require.True(
+		t,
+		kerrors.IsNotFound(err),
+		"the proxy secret must be removed with the rest of the listener's resources, whatever the current spec says about a proxy",
+	)
+}