Skip to content

Commit d434c7e

Browse files
dtfranzdtfranz
committed
Modify installation steps for ClusterRoleBindings and Services to use SSA in order to avoid race conditions. (#3182)
Signed-off-by: Daniel Franz <dfranz@redhat.com> Upstream-repository: operator-lifecycle-manager Upstream-commit: dc0c564f62d526bae0467d53f439e1c91a17ed8a
1 parent b18c1b3 commit d434c7e

File tree

13 files changed

+354
-190
lines changed

13 files changed

+354
-190
lines changed

‎staging/operator-lifecycle-manager/pkg/controller/install/certresources.go‎

Lines changed: 45 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ import (
1313
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1414
"k8s.io/apimachinery/pkg/util/intstr"
1515
"k8s.io/apimachinery/pkg/util/sets"
16+
corev1ac "k8s.io/client-go/applyconfigurations/core/v1"
17+
rbacv1ac "k8s.io/client-go/applyconfigurations/rbac/v1"
1618

1719
"github.com/operator-framework/api/pkg/operators/v1alpha1"
1820
"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/certs"
@@ -309,37 +311,27 @@ func CalculateCertRotatesAt(certExpirationTime time.Time) time.Time {
309311
func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deploymentName string, ca *certs.KeyPair, expiration time.Time, depSpec appsv1.DeploymentSpec, ports []corev1.ServicePort) (*appsv1.DeploymentSpec, []byte, error) {
310312
logger := log.WithFields(log.Fields{})
311313

312-
// Create a service for the deployment
313-
service := &corev1.Service{
314-
Spec: corev1.ServiceSpec{
315-
Ports: ports,
316-
Selector: depSpec.Selector.MatchLabels,
317-
},
318-
}
314+
// apply Service
319315
serviceName := ServiceName(deploymentName)
320-
service.SetName(serviceName)
321-
service.SetNamespace(i.owner.GetNamespace())
322-
ownerutil.AddNonBlockingOwner(service, i.owner)
323-
324-
existingService, err := i.strategyClient.GetOpLister().CoreV1().ServiceLister().Services(i.owner.GetNamespace()).Get(service.GetName())
325-
if err == nil {
326-
if !ownerutil.Adoptable(i.owner, existingService.GetOwnerReferences()) {
327-
return nil, nil, fmt.Errorf("service %s not safe to replace: extraneous ownerreferences found", service.GetName())
328-
}
329-
service.SetOwnerReferences(existingService.GetOwnerReferences())
330-
331-
// Delete the Service to replace
332-
deleteErr := i.strategyClient.GetOpClient().DeleteService(service.GetNamespace(), service.GetName(), &metav1.DeleteOptions{})
333-
if deleteErr != nil && !apierrors.IsNotFound(deleteErr) {
334-
return nil, nil, fmt.Errorf("could not delete existing service %s", service.GetName())
335-
}
336-
}
337-
338-
// Attempt to create the Service
339-
_, err = i.strategyClient.GetOpClient().CreateService(service)
340-
if err != nil {
341-
logger.Warnf("could not create service %s", service.GetName())
342-
return nil, nil, fmt.Errorf("could not create service %s: %s", service.GetName(), err.Error())
316+
portsApplyConfig := []*corev1ac.ServicePortApplyConfiguration{}
317+
for _, p := range ports {
318+
ac := corev1ac.ServicePort().
319+
WithName(p.Name).
320+
WithPort(p.Port).
321+
WithTargetPort(p.TargetPort)
322+
portsApplyConfig = append(portsApplyConfig, ac)
323+
}
324+
325+
svcApplyConfig := corev1ac.Service(serviceName, i.owner.GetNamespace()).
326+
WithSpec(corev1ac.ServiceSpec().
327+
WithPorts(portsApplyConfig...).
328+
WithSelector(depSpec.Selector.MatchLabels)).
329+
WithOwnerReferences(ownerutil.NonBlockingOwnerApplyConfiguration(i.owner)).
330+
WithLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue})
331+
332+
if _, err := i.strategyClient.GetOpClient().ApplyService(svcApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}); err != nil {
333+
log.Errorf("could not apply service %s: %s", *svcApplyConfig.Name, err.Error())
334+
return nil, nil, err
343335
}
344336

345337
// Create signed serving cert
@@ -353,14 +345,14 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
353345
// Create Secret for serving cert
354346
certPEM, privPEM, err := servingPair.ToPEM()
355347
if err != nil {
356-
logger.Warnf("unable to convert serving certificate and private key to PEM format for Service %s", service.GetName())
348+
logger.Warnf("unable to convert serving certificate and private key to PEM format for Service %s", serviceName)
357349
return nil, nil, err
358350
}
359351

360352
// Add olmcahash as a label to the caPEM
361353
caPEM, _, err := ca.ToPEM()
362354
if err != nil {
363-
logger.Warnf("unable to convert CA certificate to PEM format for Service %s", service)
355+
logger.Warnf("unable to convert CA certificate to PEM format for Service %s", serviceName)
364356
return nil, nil, err
365357
}
366358
caHash := certs.PEMSHA256(caPEM)
@@ -373,7 +365,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
373365
},
374366
Type: corev1.SecretTypeTLS,
375367
}
376-
secret.SetName(SecretName(service.GetName()))
368+
secret.SetName(SecretName(serviceName))
377369
secret.SetNamespace(i.owner.GetNamespace())
378370
secret.SetAnnotations(map[string]string{OLMCAHashAnnotationKey: caHash})
379371
secret.SetLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue})
@@ -503,50 +495,25 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
503495
return nil, nil, err
504496
}
505497

506-
// create ClusterRoleBinding to system:auth-delegator Role
507-
authDelegatorClusterRoleBinding := &rbacv1.ClusterRoleBinding{
508-
Subjects: []rbacv1.Subject{
509-
{
510-
Kind: "ServiceAccount",
511-
APIGroup: "",
512-
Name: depSpec.Template.Spec.ServiceAccountName,
513-
Namespace: i.owner.GetNamespace(),
514-
},
515-
},
516-
RoleRef: rbacv1.RoleRef{
517-
APIGroup: "rbac.authorization.k8s.io",
518-
Kind: "ClusterRole",
519-
Name: "system:auth-delegator",
520-
},
521-
}
522-
authDelegatorClusterRoleBinding.SetName(service.GetName() + "-system:auth-delegator")
523-
524-
existingAuthDelegatorClusterRoleBinding, err := i.strategyClient.GetOpLister().RbacV1().ClusterRoleBindingLister().Get(authDelegatorClusterRoleBinding.GetName())
525-
if err == nil {
526-
// Check if the only owners are this CSV or in this CSV's replacement chain.
527-
if ownerutil.AdoptableLabels(existingAuthDelegatorClusterRoleBinding.GetLabels(), true, i.owner) {
528-
logger.WithFields(log.Fields{"obj": "authDelegatorCRB", "labels": existingAuthDelegatorClusterRoleBinding.GetLabels()}).Debug("adopting")
529-
if err := ownerutil.AddOwnerLabels(authDelegatorClusterRoleBinding, i.owner); err != nil {
530-
return nil, nil, err
531-
}
532-
}
533-
534-
// Attempt an update.
535-
if _, err := i.strategyClient.GetOpClient().UpdateClusterRoleBinding(authDelegatorClusterRoleBinding); err != nil {
536-
logger.Warnf("could not update auth delegator clusterrolebinding %s", authDelegatorClusterRoleBinding.GetName())
537-
return nil, nil, err
538-
}
539-
} else if apierrors.IsNotFound(err) {
540-
// Create the role.
541-
if err := ownerutil.AddOwnerLabels(authDelegatorClusterRoleBinding, i.owner); err != nil {
542-
return nil, nil, err
543-
}
544-
_, err = i.strategyClient.GetOpClient().CreateClusterRoleBinding(authDelegatorClusterRoleBinding)
545-
if err != nil {
546-
log.Warnf("could not create auth delegator clusterrolebinding %s", authDelegatorClusterRoleBinding.GetName())
547-
return nil, nil, err
548-
}
549-
} else {
498+
// apply ClusterRoleBinding to system:auth-delegator Role
499+
crbLabels := map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}
500+
for key, val := range ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind) {
501+
crbLabels[key] = val
502+
}
503+
crbApplyConfig := rbacv1ac.ClusterRoleBinding(serviceName + "-system:auth-delegator").
504+
WithSubjects(rbacv1ac.Subject().
505+
WithKind("ServiceAccount").
506+
WithAPIGroup("").
507+
WithName(depSpec.Template.Spec.ServiceAccountName).
508+
WithNamespace(i.owner.GetNamespace())).
509+
WithRoleRef(rbacv1ac.RoleRef().
510+
WithAPIGroup("rbac.authorization.k8s.io").
511+
WithKind("ClusterRole").
512+
WithName("system:auth-delegator")).
513+
WithLabels(crbLabels)
514+
515+
if _, err = i.strategyClient.GetOpClient().ApplyClusterRoleBinding(crbApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}); err != nil {
516+
log.Errorf("could not apply auth delegator clusterrolebinding %s: %s", *crbApplyConfig.Name, err.Error())
550517
return nil, nil, err
551518
}
552519

@@ -566,7 +533,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
566533
Name: "extension-apiserver-authentication-reader",
567534
},
568535
}
569-
authReaderRoleBinding.SetName(service.GetName() + "-auth-reader")
536+
authReaderRoleBinding.SetName(serviceName + "-auth-reader")
570537
authReaderRoleBinding.SetNamespace(KubeSystem)
571538

572539
existingAuthReaderRoleBinding, err := i.strategyClient.GetOpLister().RbacV1().RoleBindingLister().RoleBindings(KubeSystem).Get(authReaderRoleBinding.GetName())

‎staging/operator-lifecycle-manager/pkg/controller/install/certresources_test.go‎

Lines changed: 107 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ import (
1515
"k8s.io/apimachinery/pkg/api/errors"
1616
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1717
"k8s.io/apimachinery/pkg/runtime/schema"
18+
corev1ac "k8s.io/client-go/applyconfigurations/core/v1"
19+
rbacv1ac "k8s.io/client-go/applyconfigurations/rbac/v1"
1820

1921
"github.com/operator-framework/api/pkg/operators/v1alpha1"
2022
"github.com/operator-framework/operator-lifecycle-manager/pkg/api/wrappers"
@@ -152,7 +154,6 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
152154
{
153155
name: "adds certs to deployment spec",
154156
mockExternal: func(mockOpClient *operatorclientmocks.MockClientInterface, fakeLister *operatorlisterfakes.FakeOperatorLister, namespace string, args args) {
155-
mockOpClient.EXPECT().DeleteService(namespace, "test-service", &metav1.DeleteOptions{}).Return(nil)
156157
service := corev1.Service{
157158
ObjectMeta: metav1.ObjectMeta{
158159
Name: "test-service",
@@ -165,7 +166,24 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
165166
Selector: selector(t, "test=label").MatchLabels,
166167
},
167168
}
168-
mockOpClient.EXPECT().CreateService(&service).Return(&service, nil)
169+
170+
portsApplyConfig := []*corev1ac.ServicePortApplyConfiguration{}
171+
for _, p := range args.ports {
172+
ac := corev1ac.ServicePort().
173+
WithName(p.Name).
174+
WithPort(p.Port).
175+
WithTargetPort(p.TargetPort)
176+
portsApplyConfig = append(portsApplyConfig, ac)
177+
}
178+
179+
svcApplyConfig := corev1ac.Service(service.Name, service.Namespace).
180+
WithSpec(corev1ac.ServiceSpec().
181+
WithPorts(portsApplyConfig...).
182+
WithSelector(selector(t, "test=label").MatchLabels)).
183+
WithOwnerReferences(ownerutil.NonBlockingOwnerApplyConfiguration(&v1alpha1.ClusterServiceVersion{})).
184+
WithLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue})
185+
186+
mockOpClient.EXPECT().ApplyService(svcApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(&service, nil)
169187

170188
hosts := []string{
171189
fmt.Sprintf("%s.%s", service.GetName(), namespace),
@@ -250,7 +268,22 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
250268
},
251269
}
252270

253-
mockOpClient.EXPECT().UpdateClusterRoleBinding(authDelegatorClusterRoleBinding).Return(authDelegatorClusterRoleBinding, nil)
271+
crbLabels := map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}
272+
for key, val := range ownerutil.OwnerLabel(ownerutil.Owner(&v1alpha1.ClusterServiceVersion{}), owner.GetObjectKind().GroupVersionKind().Kind) {
273+
crbLabels[key] = val
274+
}
275+
crbApplyConfig := rbacv1ac.ClusterRoleBinding(service.GetName() + "-system:auth-delegator").
276+
WithSubjects(rbacv1ac.Subject().
277+
WithKind("ServiceAccount").
278+
WithAPIGroup("").
279+
WithName(args.depSpec.Template.Spec.ServiceAccountName).
280+
WithNamespace("")). // Empty owner with no namespace
281+
WithRoleRef(rbacv1ac.RoleRef().
282+
WithAPIGroup("rbac.authorization.k8s.io").
283+
WithKind("ClusterRole").
284+
WithName("system:auth-delegator")).
285+
WithLabels(crbLabels)
286+
mockOpClient.EXPECT().ApplyClusterRoleBinding(crbApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(authDelegatorClusterRoleBinding, nil)
254287

255288
authReaderRoleBinding := &rbacv1.RoleBinding{
256289
Subjects: []rbacv1.Subject{
@@ -375,7 +408,6 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
375408
{
376409
name: "doesn't add duplicate service ownerrefs",
377410
mockExternal: func(mockOpClient *operatorclientmocks.MockClientInterface, fakeLister *operatorlisterfakes.FakeOperatorLister, namespace string, args args) {
378-
mockOpClient.EXPECT().DeleteService(namespace, "test-service", &metav1.DeleteOptions{}).Return(nil)
379411
service := corev1.Service{
380412
ObjectMeta: metav1.ObjectMeta{
381413
Name: "test-service",
@@ -389,7 +421,23 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
389421
Selector: selector(t, "test=label").MatchLabels,
390422
},
391423
}
392-
mockOpClient.EXPECT().CreateService(&service).Return(&service, nil)
424+
portsApplyConfig := []*corev1ac.ServicePortApplyConfiguration{}
425+
for _, p := range args.ports {
426+
ac := corev1ac.ServicePort().
427+
WithName(p.Name).
428+
WithPort(p.Port).
429+
WithTargetPort(p.TargetPort)
430+
portsApplyConfig = append(portsApplyConfig, ac)
431+
}
432+
433+
svcApplyConfig := corev1ac.Service(service.Name, service.Namespace).
434+
WithSpec(corev1ac.ServiceSpec().
435+
WithPorts(portsApplyConfig...).
436+
WithSelector(selector(t, "test=label").MatchLabels)).
437+
WithOwnerReferences(ownerutil.NonBlockingOwnerApplyConfiguration(owner)).
438+
WithLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue})
439+
440+
mockOpClient.EXPECT().ApplyService(svcApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(&service, nil)
393441

394442
hosts := []string{
395443
fmt.Sprintf("%s.%s", service.GetName(), namespace),
@@ -474,7 +522,23 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
474522
},
475523
}
476524

477-
mockOpClient.EXPECT().UpdateClusterRoleBinding(authDelegatorClusterRoleBinding).Return(authDelegatorClusterRoleBinding, nil)
525+
crbLabels := map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}
526+
for key, val := range ownerutil.OwnerLabel(owner, owner.GetObjectKind().GroupVersionKind().Kind) {
527+
crbLabels[key] = val
528+
}
529+
crbApplyConfig := rbacv1ac.ClusterRoleBinding(service.GetName() + "-system:auth-delegator").
530+
WithSubjects(rbacv1ac.Subject().
531+
WithKind("ServiceAccount").
532+
WithAPIGroup("").
533+
WithName("test-sa").
534+
WithNamespace(namespace)).
535+
WithRoleRef(rbacv1ac.RoleRef().
536+
WithAPIGroup("rbac.authorization.k8s.io").
537+
WithKind("ClusterRole").
538+
WithName("system:auth-delegator")).
539+
WithLabels(crbLabels)
540+
541+
mockOpClient.EXPECT().ApplyClusterRoleBinding(crbApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(authDelegatorClusterRoleBinding, nil)
478542

479543
authReaderRoleBinding := &rbacv1.RoleBinding{
480544
Subjects: []rbacv1.Subject{
@@ -591,9 +655,8 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
591655
},
592656
},
593657
{
594-
name: "labels an unlabelled secret if present",
658+
name: "labels an unlabelled secret if present; creates Service and ClusterRoleBinding if not existing",
595659
mockExternal: func(mockOpClient *operatorclientmocks.MockClientInterface, fakeLister *operatorlisterfakes.FakeOperatorLister, namespace string, args args) {
596-
mockOpClient.EXPECT().DeleteService(namespace, "test-service", &metav1.DeleteOptions{}).Return(nil)
597660
service := corev1.Service{
598661
ObjectMeta: metav1.ObjectMeta{
599662
Name: "test-service",
@@ -606,7 +669,24 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
606669
Selector: selector(t, "test=label").MatchLabels,
607670
},
608671
}
609-
mockOpClient.EXPECT().CreateService(&service).Return(&service, nil)
672+
673+
portsApplyConfig := []*corev1ac.ServicePortApplyConfiguration{}
674+
for _, p := range args.ports {
675+
ac := corev1ac.ServicePort().
676+
WithName(p.Name).
677+
WithPort(p.Port).
678+
WithTargetPort(p.TargetPort)
679+
portsApplyConfig = append(portsApplyConfig, ac)
680+
}
681+
682+
svcApplyConfig := corev1ac.Service(service.Name, service.Namespace).
683+
WithSpec(corev1ac.ServiceSpec().
684+
WithPorts(portsApplyConfig...).
685+
WithSelector(selector(t, "test=label").MatchLabels)).
686+
WithOwnerReferences(ownerutil.NonBlockingOwnerApplyConfiguration(&v1alpha1.ClusterServiceVersion{})).
687+
WithLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue})
688+
689+
mockOpClient.EXPECT().ApplyService(svcApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(&service, nil)
610690

611691
hosts := []string{
612692
fmt.Sprintf("%s.%s", service.GetName(), namespace),
@@ -700,8 +780,22 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
700780
Name: "system:auth-delegator",
701781
},
702782
}
703-
704-
mockOpClient.EXPECT().UpdateClusterRoleBinding(authDelegatorClusterRoleBinding).Return(authDelegatorClusterRoleBinding, nil)
783+
crbLabels := map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}
784+
for key, val := range ownerutil.OwnerLabel(ownerutil.Owner(&v1alpha1.ClusterServiceVersion{}), owner.GetObjectKind().GroupVersionKind().Kind) {
785+
crbLabels[key] = val
786+
}
787+
crbApplyConfig := rbacv1ac.ClusterRoleBinding(service.GetName() + "-system:auth-delegator").
788+
WithSubjects(rbacv1ac.Subject().WithKind("ServiceAccount").
789+
WithAPIGroup("").
790+
WithName("test-sa").
791+
WithNamespace(namespace)).
792+
WithRoleRef(rbacv1ac.RoleRef().
793+
WithAPIGroup("rbac.authorization.k8s.io").
794+
WithKind("ClusterRole").
795+
WithName("system:auth-delegator")).
796+
WithLabels(crbLabels)
797+
798+
mockOpClient.EXPECT().ApplyClusterRoleBinding(crbApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(authDelegatorClusterRoleBinding, nil)
705799

706800
authReaderRoleBinding := &rbacv1.RoleBinding{
707801
Subjects: []rbacv1.Subject{
@@ -724,13 +818,7 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
724818
mockOpClient.EXPECT().UpdateRoleBinding(authReaderRoleBinding).Return(authReaderRoleBinding, nil)
725819
},
726820
state: fakeState{
727-
existingService: &corev1.Service{
728-
ObjectMeta: metav1.ObjectMeta{
729-
OwnerReferences: []metav1.OwnerReference{
730-
ownerutil.NonBlockingOwner(&v1alpha1.ClusterServiceVersion{}),
731-
},
732-
},
733-
},
821+
existingService: nil,
734822
// unlabelled secret won't be in cache
735823
getSecretError: errors.NewNotFound(schema.GroupResource{
736824
Group: "",
@@ -742,9 +830,7 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
742830
existingRoleBinding: &rbacv1.RoleBinding{
743831
ObjectMeta: metav1.ObjectMeta{},
744832
},
745-
existingClusterRoleBinding: &rbacv1.ClusterRoleBinding{
746-
ObjectMeta: metav1.ObjectMeta{},
747-
},
833+
existingClusterRoleBinding: nil,
748834
},
749835
fields: fields{
750836
owner: &v1alpha1.ClusterServiceVersion{},

0 commit comments

Comments
 (0)