From 1a200e4236c91ae0d6c7884586efe8249dde6e50 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Tue, 29 Sep 2026 09:13:33 +0200 Subject: [PATCH] Fix annotation merging for labels-only resource metadata (#4689) --- .../values.yaml | 1 + charts/gha-runner-scale-set/values.yaml | 1 + .../autoscalinglistener_metadata_test.go | 103 ++++++++++++ .../ephemeralrunnerset_metadata_test.go | 86 ++++++++++ .../actions.github.com/resourcebuilder.go | 7 +- .../resourcebuilder_metadata_test.go | 152 ++++++++++++++++++ 6 files changed, 347 insertions(+), 3 deletions(-) create mode 100644 controllers/actions.github.com/autoscalinglistener_metadata_test.go create mode 100644 controllers/actions.github.com/ephemeralrunnerset_metadata_test.go create mode 100644 controllers/actions.github.com/resourcebuilder_metadata_test.go diff --git a/charts/gha-runner-scale-set-experimental/values.yaml b/charts/gha-runner-scale-set-experimental/values.yaml index 406061c9..303f643c 100644 --- a/charts/gha-runner-scale-set-experimental/values.yaml +++ b/charts/gha-runner-scale-set-experimental/values.yaml @@ -100,6 +100,7 @@ secretResolution: # runnerMountPath: /usr/local/share/ca-certificates/ ## Resource object allows modifying resources created by the chart itself +## Labels and annotations are independently optional for each resource. resource: # Specifies metadata that will be applied to all resources managed by ARC all: diff --git a/charts/gha-runner-scale-set/values.yaml b/charts/gha-runner-scale-set/values.yaml index c1c0f29d..0b286577 100644 --- a/charts/gha-runner-scale-set/values.yaml +++ b/charts/gha-runner-scale-set/values.yaml @@ -481,6 +481,7 @@ namespaceOverride: "" ## If you want more fine-grained control over annotations applied to particular resource created by this chart, ## you can use `resourceMeta`. +## Labels and annotations are independently optional for each resource. ## Order of applying labels and annotations is: ## 1. Apply labels/annotations globally, using `annotations` and `labels` field ## 2. Apply `resourceMeta` labels/annotations diff --git a/controllers/actions.github.com/autoscalinglistener_metadata_test.go b/controllers/actions.github.com/autoscalinglistener_metadata_test.go new file mode 100644 index 00000000..fc252147 --- /dev/null +++ b/controllers/actions.github.com/autoscalinglistener_metadata_test.go @@ -0,0 +1,103 @@ +package actionsgithubcom + +import ( + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + scalefake "github.com/actions/actions-runner-controller/controllers/actions.github.com/multiclient/fake" + "github.com/actions/actions-runner-controller/controllers/actions.github.com/secretresolver" + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestListenerRestoresMissingAnnotations(t *testing.T) { + for _, tt := range []struct { + name string + object client.Object + }{ + {name: "service account", object: &corev1.ServiceAccount{}}, + {name: "role", object: &rbacv1.Role{}}, + {name: "role binding", object: &rbacv1.RoleBinding{}}, + {name: "config secret", object: &corev1.Secret{}}, + {name: "pod", object: &corev1.Pod{}}, + } { + t.Run(tt.name, func(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, rbacv1.AddToScheme(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + metadata := &v1alpha1.ResourceMeta{Annotations: map[string]string{"example.com/required": "value"}} + listener := &v1alpha1.AutoscalingListener{ + ObjectMeta: metav1.ObjectMeta{Name: "test-listener", Namespace: "test-ns"}, + Spec: v1alpha1.AutoscalingListenerSpec{ + GitHubConfigURL: "https://github.com/org/repo", + GitHubConfigSecret: "auth", + RunnerScaleSetID: 1, + AutoscalingRunnerSetName: "test-set", + AutoscalingRunnerSetNamespace: "test-ns", + EphemeralRunnerSetName: "test-set", + Image: "listener:latest", + ServiceAccountMetadata: metadata.DeepCopy(), + RoleMetadata: metadata.DeepCopy(), + RoleBindingMetadata: metadata.DeepCopy(), + ConfigSecretMetadata: metadata.DeepCopy(), + Template: &corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Annotations: metadata.DeepCopy().Annotations}, + }, + }, + } + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects( + listener, + &v1alpha1.AutoscalingRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-set", Namespace: listener.Namespace}, + Spec: v1alpha1.AutoscalingRunnerSetSpec{ + GitHubConfigUrl: listener.Spec.GitHubConfigURL, GitHubConfigSecret: "auth", + }, + }, + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "auth", Namespace: listener.Namespace}, + Data: map[string][]byte{"github_token": []byte("test-token")}, + }, + ).Build() + r := &AutoscalingListenerReconciler{ + Client: c, Scheme: scheme, Log: logr.Discard(), ListenerMetricsAddr: "0", + ResourceBuilder: ResourceBuilder{ + Scheme: scheme, ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(c, scalefake.NewMultiClient()), + }, + } + req := ctrl.Request{NamespacedName: client.ObjectKeyFromObject(listener)} + for range 6 { + _, err := r.Reconcile(t.Context(), req) + require.NoError(t, err) + } + key := req.NamespacedName + if _, ok := tt.object.(*corev1.Secret); ok { + key.Name = scaleSetListenerConfigName(listener) + } + require.NoError(t, c.Get(t.Context(), key, tt.object)) + require.Equal(t, "value", tt.object.GetAnnotations()["example.com/required"]) + tt.object.SetAnnotations(nil) + require.NoError(t, c.Update(t.Context(), tt.object)) + + // Removing the pod's config-version annotation requires replacement; + // the other resources restore their annotations with a patch. + for range 6 { + require.NotPanics(t, func() { + _, err := r.Reconcile(t.Context(), req) + require.NoError(t, err) + }) + } + require.NoError(t, c.Get(t.Context(), key, tt.object)) + assert.Equal(t, "value", tt.object.GetAnnotations()["example.com/required"]) + }) + } +} diff --git a/controllers/actions.github.com/ephemeralrunnerset_metadata_test.go b/controllers/actions.github.com/ephemeralrunnerset_metadata_test.go new file mode 100644 index 00000000..11108467 --- /dev/null +++ b/controllers/actions.github.com/ephemeralrunnerset_metadata_test.go @@ -0,0 +1,86 @@ +package actionsgithubcom + +import ( + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestScaleUpWithOptionalRunnerMetadata(t *testing.T) { + for _, tt := range []struct { + name string + metadata *v1alpha1.ResourceMeta + }{ + {name: "omitted"}, + {name: "empty", metadata: &v1alpha1.ResourceMeta{}}, + {name: "labels only", metadata: &v1alpha1.ResourceMeta{Labels: map[string]string{"example.com/custom": "label"}}}, + {name: "annotations only", metadata: &v1alpha1.ResourceMeta{Annotations: map[string]string{"example.com/custom": "annotation"}}}, + {name: "both", metadata: &v1alpha1.ResourceMeta{ + Labels: map[string]string{"example.com/custom": "label"}, + Annotations: map[string]string{"example.com/custom": "annotation", AnnotationKeyPatchID: "999"}, + }}, + } { + t.Run(tt.name, func(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + set := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-set", Namespace: "test-ns", UID: "test-set", + Finalizers: []string{EphemeralRunnerSetFinalizerName}, + }, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + Replicas: 3, PatchID: 1, + EphemeralRunnerMetadata: tt.metadata, + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/org/repo", + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "runner", Image: "runner:latest"}}, + }}, + }, + }, + } + original := set.DeepCopy() + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(set). + WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}). + WithIndex(&v1alpha1.EphemeralRunner{}, resourceOwnerKey, newGroupVersionOwnerKindIndexer("EphemeralRunnerSet")). + Build() + r := &EphemeralRunnerSetReconciler{ + Client: c, APIReader: c, Log: logr.Discard(), Scheme: scheme, + ResourceBuilder: ResourceBuilder{Scheme: scheme, ResourceCache: newTestResourceCache()}, + } + + // Reconcile exercises the errgroup workers, whose panics cannot be + // caught by controller-runtime's recovery around Reconcile itself. + _, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(set)}) + require.NoError(t, err) + var runners v1alpha1.EphemeralRunnerList + require.NoError(t, c.List(t.Context(), &runners)) + require.Len(t, runners.Items, set.Spec.Replicas) + for _, runner := range runners.Items { + assert.Equal(t, "1", runner.Annotations[AnnotationKeyPatchID]) + assert.Equal(t, "0", runner.Annotations[AnnotationKeyActionableRevision]) + require.NotNil(t, metav1.GetControllerOf(&runner)) + assert.Equal(t, set.UID, metav1.GetControllerOf(&runner).UID) + if tt.metadata != nil { + for key, value := range tt.metadata.Labels { + assert.Equal(t, value, runner.Labels[key]) + } + assert.Equal(t, tt.metadata.Annotations["example.com/custom"], runner.Annotations["example.com/custom"]) + } + } + assert.Equal(t, original.Spec, set.Spec) + assert.Equal(t, original.Labels, set.Labels) + assert.Equal(t, original.Annotations, set.Annotations) + }) + } +} diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index 8a68e9a3..db821789 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -1110,7 +1110,8 @@ func (b *ResourceBuilder) mergeAnnotations(base, overwrite map[string]string) ma if base == nil && overwrite == nil { return nil } - base = maps.Clone(base) - maps.Copy(base, overwrite) - return base + mergedAnnotations := make(map[string]string, len(base)+len(overwrite)) + maps.Copy(mergedAnnotations, base) + maps.Copy(mergedAnnotations, overwrite) + return mergedAnnotations } diff --git a/controllers/actions.github.com/resourcebuilder_metadata_test.go b/controllers/actions.github.com/resourcebuilder_metadata_test.go new file mode 100644 index 00000000..9a722cb2 --- /dev/null +++ b/controllers/actions.github.com/resourcebuilder_metadata_test.go @@ -0,0 +1,152 @@ +package actionsgithubcom + +import ( + "maps" + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1/appconfig" + "github.com/actions/scaleset" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func TestMergeAnnotations(t *testing.T) { + tests := []struct { + name string + base map[string]string + overwrite map[string]string + want map[string]string + }{ + {name: "both nil"}, + {name: "nil base and empty overwrite", overwrite: map[string]string{}, want: map[string]string{}}, + {name: "empty base and nil overwrite", base: map[string]string{}, want: map[string]string{}}, + {name: "both empty", base: map[string]string{}, overwrite: map[string]string{}, want: map[string]string{}}, + { + name: "nil base", overwrite: map[string]string{"generated": "value"}, + want: map[string]string{"generated": "value"}, + }, + { + name: "empty base", base: map[string]string{}, overwrite: map[string]string{"generated": "value"}, + want: map[string]string{"generated": "value"}, + }, + { + name: "nil overwrite", base: map[string]string{"custom": "value"}, + want: map[string]string{"custom": "value"}, + }, + { + name: "empty overwrite", base: map[string]string{"custom": "value"}, overwrite: map[string]string{}, + want: map[string]string{"custom": "value"}, + }, + { + name: "overwrite takes precedence", + base: map[string]string{"custom": "value", "reserved": "user"}, + overwrite: map[string]string{"generated": "value", "reserved": "controller"}, + want: map[string]string{"custom": "value", "generated": "value", "reserved": "controller"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + base, overwrite := maps.Clone(tt.base), maps.Clone(tt.overwrite) + var b ResourceBuilder + var merged map[string]string + require.NotPanics(t, func() { merged = b.mergeAnnotations(tt.base, tt.overwrite) }) + require.Equal(t, tt.want, merged) + assert.Equal(t, base, tt.base) + assert.Equal(t, overwrite, tt.overwrite) + + for key := range merged { + merged[key] = "changed" + } + if merged != nil { + merged["new"] = "value" + } + assert.Equal(t, base, tt.base, "the result must not alias the base") + assert.Equal(t, overwrite, tt.overwrite, "the result must not alias the overwrite") + }) + } +} + +func TestMetadataPropagationWithoutAnnotations(t *testing.T) { + for _, tt := range []struct { + name string + annotations map[string]string + }{ + {name: "omitted annotations"}, + {name: "empty annotations", annotations: map[string]string{}}, + } { + t.Run(tt.name, func(t *testing.T) { + metadata := &v1alpha1.ResourceMeta{ + Labels: map[string]string{"example.com/custom": "value"}, + Annotations: tt.annotations, + } + ars := &v1alpha1.AutoscalingRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-set", Namespace: "test-ns", Generation: 7, + Annotations: map[string]string{ + runnerScaleSetIDAnnotationKey: "1", + AnnotationKeyGitHubRunnerGroupName: "test-group", + AnnotationKeyGitHubRunnerScaleSetName: "test-set", + }, + }, + Spec: v1alpha1.AutoscalingRunnerSetSpec{ + GitHubConfigUrl: "https://github.com/org/repo", + AutoscalingListenerMetadata: metadata.DeepCopy(), + ListenerServiceAccountMetadata: metadata.DeepCopy(), + ListenerRoleMetadata: metadata.DeepCopy(), + ListenerRoleBindingMetadata: metadata.DeepCopy(), + ListenerConfigSecretMetadata: metadata.DeepCopy(), + EphemeralRunnerSetMetadata: metadata.DeepCopy(), + EphemeralRunnerMetadata: metadata.DeepCopy(), + EphemeralRunnerConfigSecretMetadata: metadata.DeepCopy(), + Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "runner", Image: "runner:latest"}}, + }}, + }, + } + original := ars.DeepCopy() + b := ResourceBuilder{ResourceCache: newTestResourceCache()} + var ers *v1alpha1.EphemeralRunnerSet + require.NotPanics(t, func() { + var err error + ers, err = b.newEphemeralRunnerSet(ars) + require.NoError(t, err) + }) + listener, err := b.newAutoscalingListener(ars, ers, ars.Namespace, "listener:latest", nil) + require.NoError(t, err) + sa, err := b.newScaleSetListenerServiceAccount(listener) + require.NoError(t, err) + role := b.newScaleSetListenerRole(listener) + binding := b.newScaleSetListenerRoleBinding(listener, role, sa) + config, err := b.newScaleSetListenerConfig(listener, &appconfig.AppConfig{Token: "test-token"}, nil, "") + require.NoError(t, err) + listenerPod, err := b.newScaleSetListenerPod(listener, config, sa, role, binding, nil) + require.NoError(t, err) + ers.Spec.PatchID = 3 + ers.Spec.ActionableRevision = 2 + runner, err := b.newEphemeralRunner(ers) + require.NoError(t, err) + runner.Name = "test-runner" + jit, err := b.newEphemeralRunnerJitSecret(runner, &scaleset.RunnerScaleSetJitRunnerConfig{ + Runner: &scaleset.RunnerReference{ID: 1, Name: runner.Name, RunnerScaleSetID: 1}, + }) + require.NoError(t, err) + runnerPod, err := b.newEphemeralRunnerPod(runner, jit) + require.NoError(t, err) + + for _, obj := range []client.Object{ers, listener, sa, role, binding, config, listenerPod, runner, jit, runnerPod} { + assert.Equal(t, "value", obj.GetLabels()["example.com/custom"], "%T labels", obj) + assert.NotContains(t, obj.GetAnnotations(), "example.com/custom", "%T annotations", obj) + } + assert.Equal(t, "7", ers.Annotations[AnnotationKeyAutoscalingRunnerSetGeneration]) + assert.Equal(t, "test-group", ers.Annotations[AnnotationKeyGitHubRunnerGroupName]) + assert.Equal(t, "test-set", ers.Annotations[AnnotationKeyGitHubRunnerScaleSetName]) + assert.Equal(t, "3", runner.Annotations[AnnotationKeyPatchID]) + assert.Equal(t, "2", runner.Annotations[AnnotationKeyActionableRevision]) + assert.Equal(t, original, ars, "building resources must not mutate the input metadata") + }) + } +}