From 3a4a0ea37767a8d46aa4b935ede61499995ad361 Mon Sep 17 00:00:00 2001 From: Daniel Franz Date: Mon, 25 Mar 2024 13:30:34 -0700 Subject: [PATCH] RoleBinding SSA (#3190) Switch to SSA for RoleBindings during install (as we do now for Services and ClusterRoleBindings) to avoid issues with race conditions and/or failures to retrieve resources due to missing labels. Signed-off-by: Daniel Franz Upstream-repository: operator-lifecycle-manager Upstream-commit: 6b2b933f7fb5dd1fd6fe7da967f3f22a27c4253c --- .../pkg/controller/install/certresources.go | 59 +++++-------------- .../controller/install/certresources_test.go | 42 ++++++++++++- .../controller/operators/olm/operator_test.go | 4 +- .../pkg/lib/operatorclient/client.go | 1 + .../operatorclientmocks/mock_client.go | 30 ++++++++++ .../pkg/lib/operatorclient/rolebinding.go | 6 ++ .../pkg/controller/install/certresources.go | 59 +++++-------------- .../pkg/lib/operatorclient/client.go | 1 + .../pkg/lib/operatorclient/rolebinding.go | 6 ++ 9 files changed, 116 insertions(+), 92 deletions(-) diff --git a/staging/operator-lifecycle-manager/pkg/controller/install/certresources.go b/staging/operator-lifecycle-manager/pkg/controller/install/certresources.go index 25f4fd3f82..d0dc664cc7 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/install/certresources.go +++ b/staging/operator-lifecycle-manager/pkg/controller/install/certresources.go @@ -463,53 +463,24 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo return nil, nil, err } - // Create RoleBinding to extension-apiserver-authentication-reader Role in the kube-system namespace. - authReaderRoleBinding := &rbacv1.RoleBinding{ - Subjects: []rbacv1.Subject{ - { - Kind: "ServiceAccount", - APIGroup: "", - Name: depSpec.Template.Spec.ServiceAccountName, - Namespace: i.owner.GetNamespace(), - }, - }, - RoleRef: rbacv1.RoleRef{ - APIGroup: "rbac.authorization.k8s.io", - Kind: "Role", - Name: "extension-apiserver-authentication-reader", - }, - } - authReaderRoleBinding.SetName(AuthReaderRoleBindingName(serviceName)) - authReaderRoleBinding.SetNamespace(KubeSystem) - authReaderRoleBinding.SetLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}) + // Apply RoleBinding to extension-apiserver-authentication-reader Role in the kube-system namespace. + authReaderRoleBindingApplyConfig := rbacv1ac.RoleBinding(AuthReaderRoleBindingName(serviceName), KubeSystem). + WithLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}). + WithSubjects(rbacv1ac.Subject(). + WithKind("ServiceAccount"). + WithAPIGroup(""). + WithName(depSpec.Template.Spec.ServiceAccountName). + WithNamespace(i.owner.GetNamespace())). + WithRoleRef(rbacv1ac.RoleRef(). + WithAPIGroup("rbac.authorization.k8s.io"). + WithKind("Role"). + WithName("extension-apiserver-authentication-reader")) - existingAuthReaderRoleBinding, err := i.strategyClient.GetOpLister().RbacV1().RoleBindingLister().RoleBindings(KubeSystem).Get(authReaderRoleBinding.GetName()) - if err == nil { - // Check if the only owners are this CSV or in this CSV's replacement chain. - if ownerutil.AdoptableLabels(existingAuthReaderRoleBinding.GetLabels(), true, i.owner) { - logger.WithFields(log.Fields{"obj": "existingAuthReaderRB", "labels": existingAuthReaderRoleBinding.GetLabels()}).Debug("adopting") - if err := ownerutil.AddOwnerLabels(authReaderRoleBinding, i.owner); err != nil { - return nil, nil, err - } - } - // Attempt an update. - if _, err := i.strategyClient.GetOpClient().UpdateRoleBinding(authReaderRoleBinding); err != nil { - logger.Warnf("could not update auth reader role binding %s", authReaderRoleBinding.GetName()) - return nil, nil, err - } - } else if apierrors.IsNotFound(err) { - // Create the role. - if err := ownerutil.AddOwnerLabels(authReaderRoleBinding, i.owner); err != nil { - return nil, nil, err - } - _, err = i.strategyClient.GetOpClient().CreateRoleBinding(authReaderRoleBinding) - if err != nil { - log.Warnf("could not create auth reader role binding %s", authReaderRoleBinding.GetName()) - return nil, nil, err - } - } else { + if _, err = i.strategyClient.GetOpClient().ApplyRoleBinding(authReaderRoleBindingApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}); err != nil { + log.Errorf("could not apply auth reader rolebinding %s: %s", *authReaderRoleBindingApplyConfig.Name, err.Error()) return nil, nil, err } + AddDefaultCertVolumeAndVolumeMounts(&depSpec, secret.GetName()) // Setting the olm hash label forces a rollout and ensures that the new secret diff --git a/staging/operator-lifecycle-manager/pkg/controller/install/certresources_test.go b/staging/operator-lifecycle-manager/pkg/controller/install/certresources_test.go index 2682e98ffa..eb31c550d4 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/install/certresources_test.go +++ b/staging/operator-lifecycle-manager/pkg/controller/install/certresources_test.go @@ -309,7 +309,19 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) { authReaderRoleBinding.SetNamespace(KubeSystem) authReaderRoleBinding.SetLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}) - mockOpClient.EXPECT().UpdateRoleBinding(authReaderRoleBinding).Return(authReaderRoleBinding, nil) + authReaderRoleBindingApplyConfig := rbacv1ac.RoleBinding(AuthReaderRoleBindingName(service.GetName()), KubeSystem). + WithLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}). + WithSubjects(rbacv1ac.Subject(). + WithKind("ServiceAccount"). + WithAPIGroup(""). + WithName(args.depSpec.Template.Spec.ServiceAccountName). + WithNamespace(namespace)). + WithRoleRef(rbacv1ac.RoleRef(). + WithAPIGroup("rbac.authorization.k8s.io"). + WithKind("Role"). + WithName("extension-apiserver-authentication-reader")) + + mockOpClient.EXPECT().ApplyRoleBinding(authReaderRoleBindingApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(authReaderRoleBinding, nil) }, state: fakeState{ existingService: &corev1.Service{ @@ -569,7 +581,19 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) { authReaderRoleBinding.SetNamespace(KubeSystem) authReaderRoleBinding.SetLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}) - mockOpClient.EXPECT().UpdateRoleBinding(authReaderRoleBinding).Return(authReaderRoleBinding, nil) + authReaderRoleBindingApplyConfig := rbacv1ac.RoleBinding(AuthReaderRoleBindingName(service.GetName()), KubeSystem). + WithLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}). + WithSubjects(rbacv1ac.Subject(). + WithKind("ServiceAccount"). + WithAPIGroup(""). + WithName(args.depSpec.Template.Spec.ServiceAccountName). + WithNamespace(namespace)). + WithRoleRef(rbacv1ac.RoleRef(). + WithAPIGroup("rbac.authorization.k8s.io"). + WithKind("Role"). + WithName("extension-apiserver-authentication-reader")) + + mockOpClient.EXPECT().ApplyRoleBinding(authReaderRoleBindingApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(authReaderRoleBinding, nil) }, state: fakeState{ existingService: &corev1.Service{ @@ -831,7 +855,19 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) { authReaderRoleBinding.SetNamespace(KubeSystem) authReaderRoleBinding.SetLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}) - mockOpClient.EXPECT().UpdateRoleBinding(authReaderRoleBinding).Return(authReaderRoleBinding, nil) + authReaderRoleBindingApplyConfig := rbacv1ac.RoleBinding(AuthReaderRoleBindingName(service.GetName()), KubeSystem). + WithLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}). + WithSubjects(rbacv1ac.Subject(). + WithKind("ServiceAccount"). + WithAPIGroup(""). + WithName(args.depSpec.Template.Spec.ServiceAccountName). + WithNamespace(namespace)). + WithRoleRef(rbacv1ac.RoleRef(). + WithAPIGroup("rbac.authorization.k8s.io"). + WithKind("Role"). + WithName("extension-apiserver-authentication-reader")) + + mockOpClient.EXPECT().ApplyRoleBinding(authReaderRoleBindingApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(authReaderRoleBinding, nil) }, state: fakeState{ existingService: nil, diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator_test.go b/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator_test.go index 79cd22cc79..e5bf822a9e 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator_test.go +++ b/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator_test.go @@ -1511,6 +1511,7 @@ func TestTransitionCSV(t *testing.T) { // Note: Ideally we would not pre-create these objects, but fake client does not support // creation through SSA, see issue here: https://github.com/kubernetes/kubernetes/issues/115598 // Once resolved, these objects and others in this file may be removed. + roleBinding("a1-service-auth-reader", "kube-system", "extension-apiserver-authentication-reader", "sa", namespace), service("a1-service", namespace, "a1", 80), clusterRoleBinding("a1-service-system:auth-delegator", "system:auth-delegator", "sa", namespace), }, @@ -5985,8 +5986,9 @@ func TestCARotation(t *testing.T) { ), defaultTemplateAnnotations), apis("a1.v1.a1Kind"), nil), }, clientObjs: []runtime.Object{addAnnotation(defaultOperatorGroup, operatorsv1.OperatorGroupProvidedAPIsAnnotationKey, "c1.v1.g1,a1Kind.v1.a1")}, - // The service and clusterRoleBinding have been added here as a workaround to fake client not supporting SSA + // The rolebinding, service, and clusterRoleBinding have been added here as a workaround to fake client not supporting SSA objs: []runtime.Object{ + roleBinding("a1-service-auth-reader", "kube-system", "extension-apiserver-authentication-reader", "sa", namespace), service("a1-service", namespace, "a1", 80, ownerReference), clusterRoleBinding("a1-service-system:auth-delegator", "system:auth-delegator", "sa", namespace), }, diff --git a/staging/operator-lifecycle-manager/pkg/lib/operatorclient/client.go b/staging/operator-lifecycle-manager/pkg/lib/operatorclient/client.go index a7c27ae07b..cc89369fb7 100644 --- a/staging/operator-lifecycle-manager/pkg/lib/operatorclient/client.go +++ b/staging/operator-lifecycle-manager/pkg/lib/operatorclient/client.go @@ -94,6 +94,7 @@ type RoleClient interface { // RoleBindingClient contains methods for manipulating RoleBindings. type RoleBindingClient interface { + ApplyRoleBinding(applyConfig *rbacv1ac.RoleBindingApplyConfiguration, applyOptions metav1.ApplyOptions) (*rbacv1.RoleBinding, error) CreateRoleBinding(*rbacv1.RoleBinding) (*rbacv1.RoleBinding, error) GetRoleBinding(namespace, name string) (*rbacv1.RoleBinding, error) UpdateRoleBinding(modified *rbacv1.RoleBinding) (*rbacv1.RoleBinding, error) diff --git a/staging/operator-lifecycle-manager/pkg/lib/operatorclient/operatorclientmocks/mock_client.go b/staging/operator-lifecycle-manager/pkg/lib/operatorclient/operatorclientmocks/mock_client.go index 99ad7bc9df..b82217b8d1 100644 --- a/staging/operator-lifecycle-manager/pkg/lib/operatorclient/operatorclientmocks/mock_client.go +++ b/staging/operator-lifecycle-manager/pkg/lib/operatorclient/operatorclientmocks/mock_client.go @@ -89,6 +89,21 @@ func (mr *MockClientInterfaceMockRecorder) ApplyClusterRoleBinding(applyConfig, return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ApplyClusterRoleBinding", reflect.TypeOf((*MockClientInterface)(nil).ApplyClusterRoleBinding), applyConfig, applyOptions) } +// ApplyRoleBinding mocks base method. +func (m *MockClientInterface) ApplyRoleBinding(applyConfig *v14.RoleBindingApplyConfiguration, applyOptions v12.ApplyOptions) (*v11.RoleBinding, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ApplyRoleBinding", applyConfig, applyOptions) + ret0, _ := ret[0].(*v11.RoleBinding) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ApplyRoleBinding indicates an expected call of ApplyRoleBinding. +func (mr *MockClientInterfaceMockRecorder) ApplyRoleBinding(applyConfig, applyOptions interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ApplyRoleBinding", reflect.TypeOf((*MockClientInterface)(nil).ApplyRoleBinding), applyConfig, applyOptions) +} + // ApplyService mocks base method. func (m *MockClientInterface) ApplyService(arg0 *v13.ServiceApplyConfiguration, arg1 v12.ApplyOptions) (*v10.Service, error) { m.ctrl.T.Helper() @@ -1607,6 +1622,21 @@ func (m *MockRoleBindingClient) EXPECT() *MockRoleBindingClientMockRecorder { return m.recorder } +// ApplyRoleBinding mocks base method. +func (m *MockRoleBindingClient) ApplyRoleBinding(applyConfig *v14.RoleBindingApplyConfiguration, applyOptions v12.ApplyOptions) (*v11.RoleBinding, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ApplyRoleBinding", applyConfig, applyOptions) + ret0, _ := ret[0].(*v11.RoleBinding) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ApplyRoleBinding indicates an expected call of ApplyRoleBinding. +func (mr *MockRoleBindingClientMockRecorder) ApplyRoleBinding(applyConfig, applyOptions interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ApplyRoleBinding", reflect.TypeOf((*MockRoleBindingClient)(nil).ApplyRoleBinding), applyConfig, applyOptions) +} + // CreateRoleBinding mocks base method. func (m *MockRoleBindingClient) CreateRoleBinding(arg0 *v11.RoleBinding) (*v11.RoleBinding, error) { m.ctrl.T.Helper() diff --git a/staging/operator-lifecycle-manager/pkg/lib/operatorclient/rolebinding.go b/staging/operator-lifecycle-manager/pkg/lib/operatorclient/rolebinding.go index 2cfc5434db..44fc637c1c 100644 --- a/staging/operator-lifecycle-manager/pkg/lib/operatorclient/rolebinding.go +++ b/staging/operator-lifecycle-manager/pkg/lib/operatorclient/rolebinding.go @@ -7,9 +7,15 @@ import ( rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + acv1 "k8s.io/client-go/applyconfigurations/rbac/v1" "k8s.io/klog" ) +// ApplyRoleBinding applies the roleBinding. +func (c *Client) ApplyRoleBinding(applyConfig *acv1.RoleBindingApplyConfiguration, applyOptions metav1.ApplyOptions) (*rbacv1.RoleBinding, error) { + return c.RbacV1().RoleBindings(*applyConfig.Namespace).Apply(context.TODO(), applyConfig, applyOptions) +} + // CreateRoleBinding creates the roleBinding. func (c *Client) CreateRoleBinding(ig *rbacv1.RoleBinding) (*rbacv1.RoleBinding, error) { return c.RbacV1().RoleBindings(ig.GetNamespace()).Create(context.TODO(), ig, metav1.CreateOptions{}) diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install/certresources.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install/certresources.go index 25f4fd3f82..d0dc664cc7 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install/certresources.go +++ b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install/certresources.go @@ -463,53 +463,24 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo return nil, nil, err } - // Create RoleBinding to extension-apiserver-authentication-reader Role in the kube-system namespace. - authReaderRoleBinding := &rbacv1.RoleBinding{ - Subjects: []rbacv1.Subject{ - { - Kind: "ServiceAccount", - APIGroup: "", - Name: depSpec.Template.Spec.ServiceAccountName, - Namespace: i.owner.GetNamespace(), - }, - }, - RoleRef: rbacv1.RoleRef{ - APIGroup: "rbac.authorization.k8s.io", - Kind: "Role", - Name: "extension-apiserver-authentication-reader", - }, - } - authReaderRoleBinding.SetName(AuthReaderRoleBindingName(serviceName)) - authReaderRoleBinding.SetNamespace(KubeSystem) - authReaderRoleBinding.SetLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}) + // Apply RoleBinding to extension-apiserver-authentication-reader Role in the kube-system namespace. + authReaderRoleBindingApplyConfig := rbacv1ac.RoleBinding(AuthReaderRoleBindingName(serviceName), KubeSystem). + WithLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}). + WithSubjects(rbacv1ac.Subject(). + WithKind("ServiceAccount"). + WithAPIGroup(""). + WithName(depSpec.Template.Spec.ServiceAccountName). + WithNamespace(i.owner.GetNamespace())). + WithRoleRef(rbacv1ac.RoleRef(). + WithAPIGroup("rbac.authorization.k8s.io"). + WithKind("Role"). + WithName("extension-apiserver-authentication-reader")) - existingAuthReaderRoleBinding, err := i.strategyClient.GetOpLister().RbacV1().RoleBindingLister().RoleBindings(KubeSystem).Get(authReaderRoleBinding.GetName()) - if err == nil { - // Check if the only owners are this CSV or in this CSV's replacement chain. - if ownerutil.AdoptableLabels(existingAuthReaderRoleBinding.GetLabels(), true, i.owner) { - logger.WithFields(log.Fields{"obj": "existingAuthReaderRB", "labels": existingAuthReaderRoleBinding.GetLabels()}).Debug("adopting") - if err := ownerutil.AddOwnerLabels(authReaderRoleBinding, i.owner); err != nil { - return nil, nil, err - } - } - // Attempt an update. - if _, err := i.strategyClient.GetOpClient().UpdateRoleBinding(authReaderRoleBinding); err != nil { - logger.Warnf("could not update auth reader role binding %s", authReaderRoleBinding.GetName()) - return nil, nil, err - } - } else if apierrors.IsNotFound(err) { - // Create the role. - if err := ownerutil.AddOwnerLabels(authReaderRoleBinding, i.owner); err != nil { - return nil, nil, err - } - _, err = i.strategyClient.GetOpClient().CreateRoleBinding(authReaderRoleBinding) - if err != nil { - log.Warnf("could not create auth reader role binding %s", authReaderRoleBinding.GetName()) - return nil, nil, err - } - } else { + if _, err = i.strategyClient.GetOpClient().ApplyRoleBinding(authReaderRoleBindingApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}); err != nil { + log.Errorf("could not apply auth reader rolebinding %s: %s", *authReaderRoleBindingApplyConfig.Name, err.Error()) return nil, nil, err } + AddDefaultCertVolumeAndVolumeMounts(&depSpec, secret.GetName()) // Setting the olm hash label forces a rollout and ensures that the new secret diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient/client.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient/client.go index a7c27ae07b..cc89369fb7 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient/client.go +++ b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient/client.go @@ -94,6 +94,7 @@ type RoleClient interface { // RoleBindingClient contains methods for manipulating RoleBindings. type RoleBindingClient interface { + ApplyRoleBinding(applyConfig *rbacv1ac.RoleBindingApplyConfiguration, applyOptions metav1.ApplyOptions) (*rbacv1.RoleBinding, error) CreateRoleBinding(*rbacv1.RoleBinding) (*rbacv1.RoleBinding, error) GetRoleBinding(namespace, name string) (*rbacv1.RoleBinding, error) UpdateRoleBinding(modified *rbacv1.RoleBinding) (*rbacv1.RoleBinding, error) diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient/rolebinding.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient/rolebinding.go index 2cfc5434db..44fc637c1c 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient/rolebinding.go +++ b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient/rolebinding.go @@ -7,9 +7,15 @@ import ( rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + acv1 "k8s.io/client-go/applyconfigurations/rbac/v1" "k8s.io/klog" ) +// ApplyRoleBinding applies the roleBinding. +func (c *Client) ApplyRoleBinding(applyConfig *acv1.RoleBindingApplyConfiguration, applyOptions metav1.ApplyOptions) (*rbacv1.RoleBinding, error) { + return c.RbacV1().RoleBindings(*applyConfig.Namespace).Apply(context.TODO(), applyConfig, applyOptions) +} + // CreateRoleBinding creates the roleBinding. func (c *Client) CreateRoleBinding(ig *rbacv1.RoleBinding) (*rbacv1.RoleBinding, error) { return c.RbacV1().RoleBindings(ig.GetNamespace()).Create(context.TODO(), ig, metav1.CreateOptions{})