Skip to content

Commit dfa1234

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 <[email protected]> Upstream-repository: operator-lifecycle-manager Upstream-commit: dc0c564f62d526bae0467d53f439e1c91a17ed8a
1 parent b18c1b3 commit dfa1234

File tree

13 files changed

+349
-190
lines changed

13 files changed

+349
-190
lines changed

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

Lines changed: 44 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,26 @@ 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+
331+
if _, err := i.strategyClient.GetOpClient().ApplyService(svcApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}); err != nil {
332+
log.Errorf("could not apply service %s: %s", *svcApplyConfig.Name, err.Error())
333+
return nil, nil, err
343334
}
344335

345336
// Create signed serving cert
@@ -353,14 +344,14 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
353344
// Create Secret for serving cert
354345
certPEM, privPEM, err := servingPair.ToPEM()
355346
if err != nil {
356-
logger.Warnf("unable to convert serving certificate and private key to PEM format for Service %s", service.GetName())
347+
logger.Warnf("unable to convert serving certificate and private key to PEM format for Service %s", serviceName)
357348
return nil, nil, err
358349
}
359350

360351
// Add olmcahash as a label to the caPEM
361352
caPEM, _, err := ca.ToPEM()
362353
if err != nil {
363-
logger.Warnf("unable to convert CA certificate to PEM format for Service %s", service)
354+
logger.Warnf("unable to convert CA certificate to PEM format for Service %s", serviceName)
364355
return nil, nil, err
365356
}
366357
caHash := certs.PEMSHA256(caPEM)
@@ -373,7 +364,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
373364
},
374365
Type: corev1.SecretTypeTLS,
375366
}
376-
secret.SetName(SecretName(service.GetName()))
367+
secret.SetName(SecretName(serviceName))
377368
secret.SetNamespace(i.owner.GetNamespace())
378369
secret.SetAnnotations(map[string]string{OLMCAHashAnnotationKey: caHash})
379370
secret.SetLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue})
@@ -503,50 +494,25 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
503494
return nil, nil, err
504495
}
505496

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 {
497+
// apply ClusterRoleBinding to system:auth-delegator Role
498+
crbLabels := map[string]string{}
499+
for key, val := range ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind) {
500+
crbLabels[key] = val
501+
}
502+
crbApplyConfig := rbacv1ac.ClusterRoleBinding(serviceName + "-system:auth-delegator").
503+
WithSubjects(rbacv1ac.Subject().
504+
WithKind("ServiceAccount").
505+
WithAPIGroup("").
506+
WithName(depSpec.Template.Spec.ServiceAccountName).
507+
WithNamespace(i.owner.GetNamespace())).
508+
WithRoleRef(rbacv1ac.RoleRef().
509+
WithAPIGroup("rbac.authorization.k8s.io").
510+
WithKind("ClusterRole").
511+
WithName("system:auth-delegator")).
512+
WithLabels(crbLabels)
513+
514+
if _, err = i.strategyClient.GetOpClient().ApplyClusterRoleBinding(crbApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}); err != nil {
515+
log.Errorf("could not apply auth delegator clusterrolebinding %s: %s", *crbApplyConfig.Name, err.Error())
550516
return nil, nil, err
551517
}
552518

@@ -566,7 +532,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
566532
Name: "extension-apiserver-authentication-reader",
567533
},
568534
}
569-
authReaderRoleBinding.SetName(service.GetName() + "-auth-reader")
535+
authReaderRoleBinding.SetName(serviceName + "-auth-reader")
570536
authReaderRoleBinding.SetNamespace(KubeSystem)
571537

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

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

Lines changed: 104 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,23 @@ 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+
185+
mockOpClient.EXPECT().ApplyService(svcApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(&service, nil)
169186

170187
hosts := []string{
171188
fmt.Sprintf("%s.%s", service.GetName(), namespace),
@@ -250,7 +267,22 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
250267
},
251268
}
252269

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

255287
authReaderRoleBinding := &rbacv1.RoleBinding{
256288
Subjects: []rbacv1.Subject{
@@ -375,7 +407,6 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
375407
{
376408
name: "doesn't add duplicate service ownerrefs",
377409
mockExternal: func(mockOpClient *operatorclientmocks.MockClientInterface, fakeLister *operatorlisterfakes.FakeOperatorLister, namespace string, args args) {
378-
mockOpClient.EXPECT().DeleteService(namespace, "test-service", &metav1.DeleteOptions{}).Return(nil)
379410
service := corev1.Service{
380411
ObjectMeta: metav1.ObjectMeta{
381412
Name: "test-service",
@@ -389,7 +420,22 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
389420
Selector: selector(t, "test=label").MatchLabels,
390421
},
391422
}
392-
mockOpClient.EXPECT().CreateService(&service).Return(&service, nil)
423+
portsApplyConfig := []*corev1ac.ServicePortApplyConfiguration{}
424+
for _, p := range args.ports {
425+
ac := corev1ac.ServicePort().
426+
WithName(p.Name).
427+
WithPort(p.Port).
428+
WithTargetPort(p.TargetPort)
429+
portsApplyConfig = append(portsApplyConfig, ac)
430+
}
431+
432+
svcApplyConfig := corev1ac.Service(service.Name, service.Namespace).
433+
WithSpec(corev1ac.ServiceSpec().
434+
WithPorts(portsApplyConfig...).
435+
WithSelector(selector(t, "test=label").MatchLabels)).
436+
WithOwnerReferences(ownerutil.NonBlockingOwnerApplyConfiguration(owner))
437+
438+
mockOpClient.EXPECT().ApplyService(svcApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(&service, nil)
393439

394440
hosts := []string{
395441
fmt.Sprintf("%s.%s", service.GetName(), namespace),
@@ -474,7 +520,23 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
474520
},
475521
}
476522

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

479541
authReaderRoleBinding := &rbacv1.RoleBinding{
480542
Subjects: []rbacv1.Subject{
@@ -591,9 +653,8 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
591653
},
592654
},
593655
{
594-
name: "labels an unlabelled secret if present",
656+
name: "labels an unlabelled secret if present; creates Service and ClusterRoleBinding if not existing",
595657
mockExternal: func(mockOpClient *operatorclientmocks.MockClientInterface, fakeLister *operatorlisterfakes.FakeOperatorLister, namespace string, args args) {
596-
mockOpClient.EXPECT().DeleteService(namespace, "test-service", &metav1.DeleteOptions{}).Return(nil)
597658
service := corev1.Service{
598659
ObjectMeta: metav1.ObjectMeta{
599660
Name: "test-service",
@@ -606,7 +667,23 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
606667
Selector: selector(t, "test=label").MatchLabels,
607668
},
608669
}
609-
mockOpClient.EXPECT().CreateService(&service).Return(&service, nil)
670+
671+
portsApplyConfig := []*corev1ac.ServicePortApplyConfiguration{}
672+
for _, p := range args.ports {
673+
ac := corev1ac.ServicePort().
674+
WithName(p.Name).
675+
WithPort(p.Port).
676+
WithTargetPort(p.TargetPort)
677+
portsApplyConfig = append(portsApplyConfig, ac)
678+
}
679+
680+
svcApplyConfig := corev1ac.Service(service.Name, service.Namespace).
681+
WithSpec(corev1ac.ServiceSpec().
682+
WithPorts(portsApplyConfig...).
683+
WithSelector(selector(t, "test=label").MatchLabels)).
684+
WithOwnerReferences(ownerutil.NonBlockingOwnerApplyConfiguration(&v1alpha1.ClusterServiceVersion{}))
685+
686+
mockOpClient.EXPECT().ApplyService(svcApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(&service, nil)
610687

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

706797
authReaderRoleBinding := &rbacv1.RoleBinding{
707798
Subjects: []rbacv1.Subject{
@@ -724,13 +815,7 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
724815
mockOpClient.EXPECT().UpdateRoleBinding(authReaderRoleBinding).Return(authReaderRoleBinding, nil)
725816
},
726817
state: fakeState{
727-
existingService: &corev1.Service{
728-
ObjectMeta: metav1.ObjectMeta{
729-
OwnerReferences: []metav1.OwnerReference{
730-
ownerutil.NonBlockingOwner(&v1alpha1.ClusterServiceVersion{}),
731-
},
732-
},
733-
},
818+
existingService: nil,
734819
// unlabelled secret won't be in cache
735820
getSecretError: errors.NewNotFound(schema.GroupResource{
736821
Group: "",
@@ -742,9 +827,7 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
742827
existingRoleBinding: &rbacv1.RoleBinding{
743828
ObjectMeta: metav1.ObjectMeta{},
744829
},
745-
existingClusterRoleBinding: &rbacv1.ClusterRoleBinding{
746-
ObjectMeta: metav1.ObjectMeta{},
747-
},
830+
existingClusterRoleBinding: nil,
748831
},
749832
fields: fields{
750833
owner: &v1alpha1.ClusterServiceVersion{},

0 commit comments

Comments
 (0)