mirror of
https://github.com/actions-runner-controller/actions-runner-controller.git
synced 2026-09-30 02:35:57 +02:00
Fix annotation merging for labels-only resource metadata (#4689)
This commit is contained in:
Nikola Jokic
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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"])
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
})
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user