Skip to content

Commit b6067cb

Browse files
author
Per Goncalves da Silva
committed
revert to predictable hashes
Signed-off-by: Per Goncalves da Silva <[email protected]>
1 parent d04b6a7 commit b6067cb

File tree

2 files changed

+58
-113
lines changed

2 files changed

+58
-113
lines changed

Diff for: pkg/controller/operators/olm/operator_test.go

+12-24
Original file line numberDiff line numberDiff line change
@@ -298,18 +298,6 @@ func NewFakeOperator(ctx context.Context, options ...fakeOperatorOption) (*Opera
298298
k8sClientFake.Resources = apiResourcesForObjects(append(config.extObjs, config.regObjs...))
299299
k8sClientFake.PrependReactor("*", "*", clienttesting.ReactionFunc(func(action clienttesting.Action) (bool, runtime.Object, error) {
300300
*config.actionLog = append(*config.actionLog, action)
301-
switch action.GetVerb() {
302-
case "create":
303-
a := action.(clienttesting.CreateAction)
304-
m := a.GetObject().(metav1.Object)
305-
306-
// create a name if generateName is set
307-
if m.GetGenerateName() != "" {
308-
m.SetName(m.GetGenerateName() + "xxxxx")
309-
m.SetGenerateName("")
310-
return false, a.GetObject(), nil
311-
}
312-
}
313301
return false, nil, nil
314302
}))
315303
apiextensionsFake := apiextensionsfake.NewSimpleClientset(config.extObjs...)
@@ -4566,7 +4554,7 @@ func TestSyncOperatorGroups(t *testing.T) {
45664554
&rbacv1.ClusterRole{
45674555
ObjectMeta: metav1.ObjectMeta{
45684556
ResourceVersion: "",
4569-
Name: "olm.og.operator-group-1.admin-xxxxx",
4557+
Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU",
45704558
Labels: map[string]string{
45714559
"olm.owner": "operator-group-1",
45724560
"olm.owner.namespace": "operator-ns",
@@ -4577,7 +4565,7 @@ func TestSyncOperatorGroups(t *testing.T) {
45774565
&rbacv1.ClusterRole{
45784566
ObjectMeta: metav1.ObjectMeta{
45794567
ResourceVersion: "",
4580-
Name: "olm.og.operator-group-1.edit-xxxxx",
4568+
Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od",
45814569
Labels: map[string]string{
45824570
"olm.owner": "operator-group-1",
45834571
"olm.owner.namespace": "operator-ns",
@@ -4588,7 +4576,7 @@ func TestSyncOperatorGroups(t *testing.T) {
45884576
&rbacv1.ClusterRole{
45894577
ObjectMeta: metav1.ObjectMeta{
45904578
ResourceVersion: "",
4591-
Name: "olm.og.operator-group-1.view-xxxxx",
4579+
Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr",
45924580
Labels: map[string]string{
45934581
"olm.owner": "operator-group-1",
45944582
"olm.owner.namespace": "operator-ns",
@@ -4670,7 +4658,7 @@ func TestSyncOperatorGroups(t *testing.T) {
46704658
&rbacv1.ClusterRole{
46714659
ObjectMeta: metav1.ObjectMeta{
46724660
ResourceVersion: "",
4673-
Name: "olm.og.operator-group-1.admin-xxxxx",
4661+
Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU",
46744662
Labels: map[string]string{
46754663
"olm.owner": "operator-group-1",
46764664
"olm.owner.namespace": "operator-ns",
@@ -4681,7 +4669,7 @@ func TestSyncOperatorGroups(t *testing.T) {
46814669
&rbacv1.ClusterRole{
46824670
ObjectMeta: metav1.ObjectMeta{
46834671
ResourceVersion: "",
4684-
Name: "olm.og.operator-group-1.edit-xxxxx",
4672+
Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od",
46854673
Labels: map[string]string{
46864674
"olm.owner": "operator-group-1",
46874675
"olm.owner.namespace": "operator-ns",
@@ -4692,7 +4680,7 @@ func TestSyncOperatorGroups(t *testing.T) {
46924680
&rbacv1.ClusterRole{
46934681
ObjectMeta: metav1.ObjectMeta{
46944682
ResourceVersion: "",
4695-
Name: "olm.og.operator-group-1.view-xxxxx",
4683+
Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr",
46964684
Labels: map[string]string{
46974685
"olm.owner": "operator-group-1",
46984686
"olm.owner.namespace": "operator-ns",
@@ -4766,7 +4754,7 @@ func TestSyncOperatorGroups(t *testing.T) {
47664754
&rbacv1.ClusterRole{
47674755
ObjectMeta: metav1.ObjectMeta{
47684756
ResourceVersion: "",
4769-
Name: "olm.og.operator-group-1.admin-xxxxx",
4757+
Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU",
47704758
Labels: map[string]string{
47714759
"olm.owner": "operator-group-1",
47724760
"olm.owner.namespace": "operator-ns",
@@ -4777,7 +4765,7 @@ func TestSyncOperatorGroups(t *testing.T) {
47774765
&rbacv1.ClusterRole{
47784766
ObjectMeta: metav1.ObjectMeta{
47794767
ResourceVersion: "",
4780-
Name: "olm.og.operator-group-1.edit-xxxxx",
4768+
Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od",
47814769
Labels: map[string]string{
47824770
"olm.owner": "operator-group-1",
47834771
"olm.owner.namespace": "operator-ns",
@@ -4788,7 +4776,7 @@ func TestSyncOperatorGroups(t *testing.T) {
47884776
&rbacv1.ClusterRole{
47894777
ObjectMeta: metav1.ObjectMeta{
47904778
ResourceVersion: "",
4791-
Name: "olm.og.operator-group-1.view-xxxxx",
4779+
Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr",
47924780
Labels: map[string]string{
47934781
"olm.owner": "operator-group-1",
47944782
"olm.owner.namespace": "operator-ns",
@@ -4806,7 +4794,7 @@ func TestSyncOperatorGroups(t *testing.T) {
48064794
&rbacv1.ClusterRole{
48074795
ObjectMeta: metav1.ObjectMeta{
48084796
ResourceVersion: "",
4809-
Name: "olm.og.operator-group-1.admin-xxxxx",
4797+
Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU",
48104798
Labels: map[string]string{
48114799
"olm.owner": "operator-group-1",
48124800
"olm.owner.namespace": "operator-ns",
@@ -4817,7 +4805,7 @@ func TestSyncOperatorGroups(t *testing.T) {
48174805
&rbacv1.ClusterRole{
48184806
ObjectMeta: metav1.ObjectMeta{
48194807
ResourceVersion: "",
4820-
Name: "olm.og.operator-group-1.edit-xxxxx",
4808+
Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od",
48214809
Labels: map[string]string{
48224810
"olm.owner": "operator-group-1",
48234811
"olm.owner.namespace": "operator-ns",
@@ -4828,7 +4816,7 @@ func TestSyncOperatorGroups(t *testing.T) {
48284816
&rbacv1.ClusterRole{
48294817
ObjectMeta: metav1.ObjectMeta{
48304818
ResourceVersion: "",
4831-
Name: "olm.og.operator-group-1.view-xxxxx",
4819+
Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr",
48324820
Labels: map[string]string{
48334821
"olm.owner": "operator-group-1",
48344822
"olm.owner.namespace": "operator-ns",

Diff for: pkg/controller/operators/olm/operatorgroup.go

+46-89
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,11 @@ package olm
22

33
import (
44
"context"
5+
"crypto/sha256"
56
"fmt"
67
"hash/fnv"
7-
"math"
8+
"math/big"
89
"reflect"
9-
"regexp"
10-
"sort"
1110
"strings"
1211

1312
"github.com/sirupsen/logrus"
@@ -41,8 +40,9 @@ const (
4140
// kubeResourceNameLimit is the maximum length of a Kubernetes resource name
4241
kubeResourceNameLimit = 253
4342

44-
// resourceRandomPrefixLengh is the length of the random prefix added to the resource name
45-
resourceRandomPrefixLength = 5
43+
// operatorGroupClusterRoleNameFmt template for ClusterRole names owned by OperatorGroups\
44+
// e.g. olm.og.my-group.admin-<hash>
45+
operatorGroupClusterRoleNameFmt = "olm.og.%s.%s-%s"
4646
)
4747

4848
var (
@@ -994,30 +994,31 @@ func (a *Operator) updateNamespaceList(op *operatorsv1.OperatorGroup) ([]string,
994994
return namespaceList, nil
995995
}
996996

997-
func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, roleType string, apis cache.APISet) error {
997+
func (a *Operator) getClusterRoleName(op *operatorsv1.OperatorGroup, roleType string) (string, error) {
998+
roleSuffix := hash(fmt.Sprintf("%s/%s/%s", op.GetNamespace(), op.GetName(), roleType))
999+
// calculate how many characters are left for the operator group name
1000+
nameLimit := kubeResourceNameLimit - len(strings.Replace(operatorGroupClusterRoleNameFmt, "%s", "", -1)) - len(roleType) - len(roleSuffix)
1001+
if len(op.GetName()) < nameLimit {
1002+
return fmt.Sprintf(operatorGroupClusterRoleNameFmt, op.GetName(), roleType, roleSuffix), nil
1003+
}
1004+
return fmt.Sprintf(operatorGroupClusterRoleNameFmt, op.GetName()[:nameLimit], roleType, roleSuffix), nil
1005+
}
1006+
1007+
func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffix string, apis cache.APISet) error {
9981008
// create target cluster role spec
9991009
var clusterRole *rbacv1.ClusterRole
1000-
1001-
// to respect kubernetes resource length limitations we need to truncate the operator group name
1002-
template := "olm.og.%s.%s-" // final name will look like, e.g. olm.og.my-og.admin-pll5s
1003-
numTemplateChars := len(strings.Replace(template, "%s", "", -1))
1004-
roleTypeLength := len(roleType)
1005-
1006-
// the operator group component of the name must be limited by the resource name length limit
1007-
// minus the number of characters used up by the other components of the name
1008-
nameLimit := kubeResourceNameLimit - numTemplateChars - roleTypeLength - resourceRandomPrefixLength
1009-
operatorGroupName := op.GetName()[:int(math.Min(float64(nameLimit), float64(len(op.GetName()))))]
1010-
1011-
clusterRoleName := fmt.Sprintf(template, operatorGroupName, roleType)
1012-
aggregationRule, err := a.getClusterRoleAggregationRule(apis, roleType)
1010+
clusterRoleName, err := a.getClusterRoleName(op, suffix)
1011+
if err != nil {
1012+
return err
1013+
}
1014+
aggregationRule, err := a.getClusterRoleAggregationRule(apis, suffix)
10131015
if err != nil {
10141016
return err
10151017
}
10161018

1017-
// if we'll be creating a fresh cluster role, use generateName to avoid name collisions
10181019
clusterRole = &rbacv1.ClusterRole{
10191020
ObjectMeta: metav1.ObjectMeta{
1020-
GenerateName: clusterRoleName,
1021+
Name: clusterRoleName,
10211022
},
10221023
AggregationRule: aggregationRule,
10231024
}
@@ -1026,74 +1027,32 @@ func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, roleT
10261027
return err
10271028
}
10281029

1029-
// list current cluster roles owned by this operator group
1030-
clusterRoles, err := a.lister.RbacV1().ClusterRoleLister().List(ownerutil.OperatorGroupOwnerSelector(op))
1031-
if err != nil {
1030+
// get existing cluster role for this level (suffix: admin, edit, view))
1031+
existingRole, err := a.lister.RbacV1().ClusterRoleLister().Get(clusterRoleName)
1032+
if err != nil && !apierrors.IsNotFound(err) {
10321033
return err
10331034
}
10341035

1035-
// find the cluster role that matches the template and is of the correct type
1036-
var existingClusterRoles []*rbacv1.ClusterRole
1037-
re, ok := ogClusterRoleNameRegExMap[roleType]
1038-
if !ok {
1039-
return fmt.Errorf("no regex found for role type: %s", roleType)
1040-
}
1041-
for _, cr := range clusterRoles {
1042-
if re.FindStringSubmatch(cr.GetName()) != nil {
1043-
existingClusterRoles = append(existingClusterRoles, cr)
1044-
}
1045-
}
1046-
1047-
switch len(existingClusterRoles) {
1048-
// if no cluster role exists, create one
1049-
case 0:
1050-
a.logger.Infof("Creating cluster role: %s owned by operator group: %s/%s", clusterRole.GetGenerateName(), op.GetNamespace(), op.GetName())
1051-
if _, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(context.TODO(), clusterRole, metav1.CreateOptions{}); err != nil {
1052-
// name collision, try again with a new name in the next reconcile
1053-
a.logger.WithError(err).Errorf("Create cluster role failed: %v", clusterRole)
1054-
}
1055-
return err
1056-
// if an existing cluster role resource is found, update it if necessary
1057-
case 1:
1058-
existingClusterRole := existingClusterRoles[0].DeepCopy()
1059-
// the cluster role will need to be updated if the aggregation rules have changed - otherwise, nothing to do
1060-
// the resource is guaranteed to have the correct ownership labels because we reached this part of the code
1061-
if !reflect.DeepEqual(existingClusterRole.AggregationRule, aggregationRule) {
1062-
if _, err := a.opClient.UpdateClusterRole(existingClusterRole); err != nil {
1063-
a.logger.WithError(err).Errorf("Update existing cluster role failed: %v", clusterRole)
1064-
}
1036+
if existingRole != nil {
1037+
// if the existing role conforms to the naming convention, check for skew
1038+
cp := existingRole.DeepCopy()
1039+
if err := ownerutil.AddOwnerLabels(cp, op); err != nil {
10651040
return err
10661041
}
1067-
// the inherent race condition created by listing to check if a resource exists and then creating it
1068-
// and the fact that we are using generateName means that it is possible for multiple cluster roles of the same
1069-
// role type to be created for the same operator group. In this case, we'll disambiguate by keeping the oldest
1070-
// cluster role (by creation timestamp - tie-breaking by name) and deleting the others
1071-
default:
1072-
a.logger.Warnf("multiple (%d) cluster roles of type %s owned by operator group %s/%s found", len(existingClusterRoles), roleType, op.GetNamespace(), op.GetName())
1073-
// sort by creation timestamp and tie-break by name
1074-
sort.Slice(existingClusterRoles, func(i, j int) bool {
1075-
creationTimeI := existingClusterRoles[i].GetCreationTimestamp()
1076-
creationTimeJ := existingClusterRoles[j].GetCreationTimestamp()
1077-
if creationTimeI.Equal(&creationTimeJ) {
1078-
return strings.Compare(existingClusterRoles[i].GetName(), existingClusterRoles[j].GetName()) < 0
1079-
}
1080-
return creationTimeI.Before(&creationTimeJ)
1081-
})
10821042

1083-
// delete all but the oldest cluster role
1084-
a.logger.Infof("keeping cluster role: %s owned by operator group: %s/%s and deleting %d others", existingClusterRoles[0].GetName(), op.GetNamespace(), op.GetName(), len(existingClusterRoles)-1)
1085-
for _, cr := range existingClusterRoles[1:] {
1086-
a.logger.Infof("deleting cluster role: %s owned by operator group: %s/%s - there may be only one", cr.GetName(), op.GetNamespace(), op.GetName())
1087-
if err := a.opClient.DeleteClusterRole(cr.GetName(), &metav1.DeleteOptions{}); err != nil {
1088-
a.logger.WithError(err).Errorf("Delete cluster role failed: %v", cr)
1089-
return err
1090-
}
1043+
// ensure that the labels and aggregation rules are correct
1044+
if labels.Equals(existingRole.Labels, cp.Labels) && reflect.DeepEqual(existingRole.AggregationRule, aggregationRule) {
1045+
return nil
10911046
}
1092-
1093-
// now that we're (hopefully) down to one cluster role, re-reconcile
1094-
return fmt.Errorf("multiple cluster roles of type %s owned by operator group %s/%s found", roleType, op.GetNamespace(), op.GetName())
1047+
if _, err := a.opClient.UpdateClusterRole(clusterRole); err != nil {
1048+
a.logger.WithError(err).Errorf("update existing cluster role failed: %v", clusterRole)
1049+
}
1050+
return err
10951051
}
1096-
return nil
1052+
1053+
a.logger.Infof("creating cluster role: %s owned by operator group: %s/%s", clusterRole.GetName(), op.GetNamespace(), op.GetName())
1054+
_, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(context.TODO(), clusterRole, metav1.CreateOptions{})
1055+
return err
10971056
}
10981057

10991058
func (a *Operator) getClusterRoleAggregationRule(apis cache.APISet, suffix string) (*rbacv1.AggregationRule, error) {
@@ -1212,14 +1171,12 @@ func csvCopyPrototype(src, dst *v1alpha1.ClusterServiceVersion) {
12121171
dst.Status.Message = fmt.Sprintf("The operator is running in %s but is managing this namespace", src.GetNamespace())
12131172
}
12141173

1215-
// ogClusterRoleNameRegEx returns a regexp for the naming format used for cluster roles owned by operator groups
1216-
// for a particular role type e.g. olm.og.my-og.admin-pll2k (where pll2k is a random suffix and admin is the role type)
1217-
var ogClusterRoleNameRegExMap = map[string]*regexp.Regexp{
1218-
"admin": ogClusterRoleNameRegEx("admin"),
1219-
"edit": ogClusterRoleNameRegEx("edit"),
1220-
"view": ogClusterRoleNameRegEx("view"),
1174+
func hash(s string) string {
1175+
return toBase62(sha256.Sum224([]byte(s)))
12211176
}
12221177

1223-
func ogClusterRoleNameRegEx(roleType string) *regexp.Regexp {
1224-
return regexp.MustCompile(fmt.Sprintf(`^olm\.og\.[^\.]+\.%s-[a-z0-9]{5}$`, roleType))
1178+
func toBase62(hash [28]byte) string {
1179+
var i big.Int
1180+
i.SetBytes(hash[:])
1181+
return i.Text(62)
12251182
}

0 commit comments

Comments
 (0)