From 1e4769d5fd60994038894313e170275215c31ea1 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Mon, 9 Jun 2025 17:07:36 -0400 Subject: [PATCH] operatorgroup: ensure clusterroleselectors in clusterrole aggregation rules are sorted (#3593) Signed-off-by: Joe Lanford Upstream-repository: operator-lifecycle-manager Upstream-commit: 3558fae23fb2157270f5f676ddfe5647def8f157 --- .../controller/operators/olm/operatorgroup.go | 26 +++++-- .../operators/olm/operatorgroup_test.go | 68 ++++++++++++++++++- .../controller/operators/olm/operatorgroup.go | 26 +++++-- 3 files changed, 104 insertions(+), 16 deletions(-) diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup.go b/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup.go index ff4f3b35cc..e10fbe6adc 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup.go +++ b/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup.go @@ -19,6 +19,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/sets" operatorsv1 "github.com/operator-framework/api/pkg/operators/v1" "github.com/operator-framework/api/pkg/operators/v1alpha1" @@ -1047,24 +1048,35 @@ func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffi } func (a *Operator) getClusterRoleAggregationRule(apis cache.APISet, suffix string) (*rbacv1.AggregationRule, error) { - var selectors []metav1.LabelSelector + if len(apis) == 0 { + return nil, nil + } + + aggregationLabels := sets.New[string]() for api := range apis { aggregationLabel, err := aggregationLabelFromAPIKey(api, suffix) if err != nil { return nil, err } + aggregationLabels.Insert(aggregationLabel) + } + + // The order of the resulting selectors MUST BE deterministic in order + // to avoid unnecessary writes against the API server where only the order + // is changing. Therefore, we use `sets.List` to iterate. It returns a + // sorted slice of the aggregation labels. + selectors := make([]metav1.LabelSelector, 0, aggregationLabels.Len()) + for _, aggregationLabel := range sets.List(aggregationLabels) { selectors = append(selectors, metav1.LabelSelector{ MatchLabels: map[string]string{ aggregationLabel: "true", }, }) } - if len(selectors) > 0 { - return &rbacv1.AggregationRule{ - ClusterRoleSelectors: selectors, - }, nil - } - return nil, nil + + return &rbacv1.AggregationRule{ + ClusterRoleSelectors: selectors, + }, nil } func (a *Operator) ensureOpGroupClusterRoles(op *operatorsv1.OperatorGroup, apis cache.APISet) error { diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup_test.go b/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup_test.go index bb328c72cc..628e9ec3dd 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup_test.go +++ b/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup_test.go @@ -8,15 +8,17 @@ import ( "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "k8s.io/client-go/metadata/metadatalister" - + rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/client-go/metadata/metadatalister" ktesting "k8s.io/client-go/testing" "github.com/operator-framework/api/pkg/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/cache" + "github.com/operator-framework/operator-registry/pkg/registry" ) func TestCopyToNamespace(t *testing.T) { @@ -407,3 +409,65 @@ func TestCSVCopyPrototype(t *testing.T) { }, }, dst) } + +func TestOperator_getClusterRoleAggregationRule(t *testing.T) { + tests := []struct { + name string + apis cache.APISet + suffix string + want func(*testing.T, *rbacv1.AggregationRule) + wantErr require.ErrorAssertionFunc + }{ + { + name: "no aggregation rule when no APIs", + apis: cache.APISet{}, + suffix: "admin", + want: func(t *testing.T, rule *rbacv1.AggregationRule) { + require.Nil(t, rule) + }, + wantErr: require.NoError, + }, + { + name: "ordered selectors in aggregation rule", + apis: cache.APISet{ + registry.APIKey{Group: "example.com", Version: "v1alpha1", Kind: "Foo"}: {}, + registry.APIKey{Group: "example.com", Version: "v1alpha2", Kind: "Foo"}: {}, + registry.APIKey{Group: "example.com", Version: "v1alpha3", Kind: "Foo"}: {}, + registry.APIKey{Group: "example.com", Version: "v1alpha4", Kind: "Foo"}: {}, + registry.APIKey{Group: "example.com", Version: "v1alpha5", Kind: "Foo"}: {}, + registry.APIKey{Group: "example.com", Version: "v1alpha1", Kind: "Bar"}: {}, + registry.APIKey{Group: "example.com", Version: "v1alpha2", Kind: "Bar"}: {}, + registry.APIKey{Group: "example.com", Version: "v1alpha3", Kind: "Bar"}: {}, + registry.APIKey{Group: "example.com", Version: "v1alpha4", Kind: "Bar"}: {}, + registry.APIKey{Group: "example.com", Version: "v1alpha5", Kind: "Bar"}: {}, + }, + suffix: "admin", + want: func(t *testing.T, rule *rbacv1.AggregationRule) { + getOneKey := func(t *testing.T, m map[string]string) string { + require.Len(t, m, 1) + for k := range m { + return k + } + t.Fatalf("no keys found in map") + return "" + } + + a := getOneKey(t, rule.ClusterRoleSelectors[0].MatchLabels) + for _, selector := range rule.ClusterRoleSelectors[1:] { + b := getOneKey(t, selector.MatchLabels) + require.Lessf(t, a, b, "expected selector match labels keys to be in sorted ascending order") + a = b + } + }, + wantErr: require.NoError, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := &Operator{} + got, err := a.getClusterRoleAggregationRule(tt.apis, tt.suffix) + tt.wantErr(t, err) + tt.want(t, got) + }) + } +} diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup.go index ff4f3b35cc..e10fbe6adc 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup.go +++ b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup.go @@ -19,6 +19,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/sets" operatorsv1 "github.com/operator-framework/api/pkg/operators/v1" "github.com/operator-framework/api/pkg/operators/v1alpha1" @@ -1047,24 +1048,35 @@ func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffi } func (a *Operator) getClusterRoleAggregationRule(apis cache.APISet, suffix string) (*rbacv1.AggregationRule, error) { - var selectors []metav1.LabelSelector + if len(apis) == 0 { + return nil, nil + } + + aggregationLabels := sets.New[string]() for api := range apis { aggregationLabel, err := aggregationLabelFromAPIKey(api, suffix) if err != nil { return nil, err } + aggregationLabels.Insert(aggregationLabel) + } + + // The order of the resulting selectors MUST BE deterministic in order + // to avoid unnecessary writes against the API server where only the order + // is changing. Therefore, we use `sets.List` to iterate. It returns a + // sorted slice of the aggregation labels. + selectors := make([]metav1.LabelSelector, 0, aggregationLabels.Len()) + for _, aggregationLabel := range sets.List(aggregationLabels) { selectors = append(selectors, metav1.LabelSelector{ MatchLabels: map[string]string{ aggregationLabel: "true", }, }) } - if len(selectors) > 0 { - return &rbacv1.AggregationRule{ - ClusterRoleSelectors: selectors, - }, nil - } - return nil, nil + + return &rbacv1.AggregationRule{ + ClusterRoleSelectors: selectors, + }, nil } func (a *Operator) ensureOpGroupClusterRoles(op *operatorsv1.OperatorGroup, apis cache.APISet) error {