Skip to content

Commit d6b4760

Browse files
dtfranzdtfranz
committed
Modify installation steps for ClusterRoleBindings and Services to use SSA in order to avoid race conditions.
Signed-off-by: Daniel Franz <[email protected]>
1 parent 9f42a6f commit d6b4760

File tree

8 files changed

+279
-116
lines changed

8 files changed

+279
-116
lines changed

‎pkg/controller/install/certresources.go

+48-82
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import (
1212
apierrors "k8s.io/apimachinery/pkg/api/errors"
1313
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1414
"k8s.io/apimachinery/pkg/util/intstr"
15+
corev1ac "k8s.io/client-go/applyconfigurations/core/v1"
16+
rbacv1ac "k8s.io/client-go/applyconfigurations/rbac/v1"
1517

1618
"github.com/operator-framework/api/pkg/operators/v1alpha1"
1719
"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/certs"
@@ -241,43 +243,33 @@ func CalculateCertRotatesAt(certExpirationTime time.Time) time.Time {
241243
func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deploymentName string, ca *certs.KeyPair, expiration time.Time, depSpec appsv1.DeploymentSpec, ports []corev1.ServicePort) (*appsv1.DeploymentSpec, []byte, error) {
242244
logger := log.WithFields(log.Fields{})
243245

244-
// Create a service for the deployment
245-
service := &corev1.Service{
246-
Spec: corev1.ServiceSpec{
247-
Ports: ports,
248-
Selector: depSpec.Selector.MatchLabels,
249-
},
250-
}
251-
service.SetName(ServiceName(deploymentName))
252-
service.SetNamespace(i.owner.GetNamespace())
253-
ownerutil.AddNonBlockingOwner(service, i.owner)
254-
service.SetLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue})
255-
256-
existingService, err := i.strategyClient.GetOpLister().CoreV1().ServiceLister().Services(i.owner.GetNamespace()).Get(service.GetName())
257-
if err == nil {
258-
if !ownerutil.Adoptable(i.owner, existingService.GetOwnerReferences()) {
259-
return nil, nil, fmt.Errorf("service %s not safe to replace: extraneous ownerreferences found", service.GetName())
260-
}
261-
service.SetOwnerReferences(existingService.GetOwnerReferences())
262-
263-
// Delete the Service to replace
264-
deleteErr := i.strategyClient.GetOpClient().DeleteService(service.GetNamespace(), service.GetName(), &metav1.DeleteOptions{})
265-
if deleteErr != nil && !apierrors.IsNotFound(deleteErr) {
266-
return nil, nil, fmt.Errorf("could not delete existing service %s", service.GetName())
267-
}
268-
}
269-
270-
// Attempt to create the Service
271-
_, err = i.strategyClient.GetOpClient().CreateService(service)
272-
if err != nil {
273-
logger.Warnf("could not create service %s", service.GetName())
274-
return nil, nil, fmt.Errorf("could not create service %s: %s", service.GetName(), err.Error())
246+
// apply Service
247+
serviceName := ServiceName(deploymentName)
248+
portsApplyConfig := []*corev1ac.ServicePortApplyConfiguration{}
249+
for _, p := range ports {
250+
ac := corev1ac.ServicePort().
251+
WithName(p.Name).
252+
WithPort(p.Port).
253+
WithTargetPort(p.TargetPort)
254+
portsApplyConfig = append(portsApplyConfig, ac)
255+
}
256+
257+
svcApplyConfig := corev1ac.Service(serviceName, i.owner.GetNamespace()).
258+
WithSpec(corev1ac.ServiceSpec().
259+
WithPorts(portsApplyConfig...).
260+
WithSelector(depSpec.Selector.MatchLabels)).
261+
WithOwnerReferences(ownerutil.NonBlockingOwnerApplyConfiguration(i.owner)).
262+
WithLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue})
263+
264+
if _, err := i.strategyClient.GetOpClient().ApplyService(svcApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}); err != nil {
265+
log.Errorf("could not apply service %s: %s", *svcApplyConfig.Name, err.Error())
266+
return nil, nil, err
275267
}
276268

277269
// Create signed serving cert
278270
hosts := []string{
279-
fmt.Sprintf("%s.%s", service.GetName(), i.owner.GetNamespace()),
280-
fmt.Sprintf("%s.%s.svc", service.GetName(), i.owner.GetNamespace()),
271+
fmt.Sprintf("%s.%s", serviceName, i.owner.GetNamespace()),
272+
fmt.Sprintf("%s.%s.svc", serviceName, i.owner.GetNamespace()),
281273
}
282274
servingPair, err := certGenerator.Generate(expiration, Organization, ca, hosts)
283275
if err != nil {
@@ -288,14 +280,14 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
288280
// Create Secret for serving cert
289281
certPEM, privPEM, err := servingPair.ToPEM()
290282
if err != nil {
291-
logger.Warnf("unable to convert serving certificate and private key to PEM format for Service %s", service.GetName())
283+
logger.Warnf("unable to convert serving certificate and private key to PEM format for Service %s", serviceName)
292284
return nil, nil, err
293285
}
294286

295287
// Add olmcahash as a label to the caPEM
296288
caPEM, _, err := ca.ToPEM()
297289
if err != nil {
298-
logger.Warnf("unable to convert CA certificate to PEM format for Service %s", service)
290+
logger.Warnf("unable to convert CA certificate to PEM format for Service %s", serviceName)
299291
return nil, nil, err
300292
}
301293
caHash := certs.PEMSHA256(caPEM)
@@ -308,7 +300,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
308300
},
309301
Type: corev1.SecretTypeTLS,
310302
}
311-
secret.SetName(SecretName(service.GetName()))
303+
secret.SetName(SecretName(serviceName))
312304
secret.SetNamespace(i.owner.GetNamespace())
313305
secret.SetAnnotations(map[string]string{OLMCAHashAnnotationKey: caHash})
314306
secret.SetLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue})
@@ -440,51 +432,25 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
440432
return nil, nil, err
441433
}
442434

443-
// create ClusterRoleBinding to system:auth-delegator Role
444-
authDelegatorClusterRoleBinding := &rbacv1.ClusterRoleBinding{
445-
Subjects: []rbacv1.Subject{
446-
{
447-
Kind: "ServiceAccount",
448-
APIGroup: "",
449-
Name: depSpec.Template.Spec.ServiceAccountName,
450-
Namespace: i.owner.GetNamespace(),
451-
},
452-
},
453-
RoleRef: rbacv1.RoleRef{
454-
APIGroup: "rbac.authorization.k8s.io",
455-
Kind: "ClusterRole",
456-
Name: "system:auth-delegator",
457-
},
458-
}
459-
authDelegatorClusterRoleBinding.SetName(AuthDelegatorClusterRoleBindingName(service.GetName()))
460-
authDelegatorClusterRoleBinding.SetLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue})
461-
462-
existingAuthDelegatorClusterRoleBinding, err := i.strategyClient.GetOpLister().RbacV1().ClusterRoleBindingLister().Get(authDelegatorClusterRoleBinding.GetName())
463-
if err == nil {
464-
// Check if the only owners are this CSV or in this CSV's replacement chain.
465-
if ownerutil.AdoptableLabels(existingAuthDelegatorClusterRoleBinding.GetLabels(), true, i.owner) {
466-
logger.WithFields(log.Fields{"obj": "authDelegatorCRB", "labels": existingAuthDelegatorClusterRoleBinding.GetLabels()}).Debug("adopting")
467-
if err := ownerutil.AddOwnerLabels(authDelegatorClusterRoleBinding, i.owner); err != nil {
468-
return nil, nil, err
469-
}
470-
}
471-
472-
// Attempt an update.
473-
if _, err := i.strategyClient.GetOpClient().UpdateClusterRoleBinding(authDelegatorClusterRoleBinding); err != nil {
474-
logger.Warnf("could not update auth delegator clusterrolebinding %s", authDelegatorClusterRoleBinding.GetName())
475-
return nil, nil, err
476-
}
477-
} else if apierrors.IsNotFound(err) {
478-
// Create the role.
479-
if err := ownerutil.AddOwnerLabels(authDelegatorClusterRoleBinding, i.owner); err != nil {
480-
return nil, nil, err
481-
}
482-
_, err = i.strategyClient.GetOpClient().CreateClusterRoleBinding(authDelegatorClusterRoleBinding)
483-
if err != nil {
484-
log.Warnf("could not create auth delegator clusterrolebinding %s", authDelegatorClusterRoleBinding.GetName())
485-
return nil, nil, err
486-
}
487-
} else {
435+
// apply ClusterRoleBinding to system:auth-delegator Role
436+
crbLabels := map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}
437+
for key, val := range ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind) {
438+
crbLabels[key] = val
439+
}
440+
crbApplyConfig := rbacv1ac.ClusterRoleBinding(AuthDelegatorClusterRoleBindingName(serviceName)).
441+
WithSubjects(rbacv1ac.Subject().
442+
WithKind("ServiceAccount").
443+
WithAPIGroup("").
444+
WithName(depSpec.Template.Spec.ServiceAccountName).
445+
WithNamespace(i.owner.GetNamespace())).
446+
WithRoleRef(rbacv1ac.RoleRef().
447+
WithAPIGroup("rbac.authorization.k8s.io").
448+
WithKind("ClusterRole").
449+
WithName("system:auth-delegator")).
450+
WithLabels(crbLabels)
451+
452+
if _, err = i.strategyClient.GetOpClient().ApplyClusterRoleBinding(crbApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}); err != nil {
453+
log.Errorf("could not apply auth delegator clusterrolebinding %s: %s", *crbApplyConfig.Name, err.Error())
488454
return nil, nil, err
489455
}
490456

@@ -504,7 +470,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
504470
Name: "extension-apiserver-authentication-reader",
505471
},
506472
}
507-
authReaderRoleBinding.SetName(AuthReaderRoleBindingName(service.GetName()))
473+
authReaderRoleBinding.SetName(AuthReaderRoleBindingName(serviceName))
508474
authReaderRoleBinding.SetNamespace(KubeSystem)
509475
authReaderRoleBinding.SetLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue})
510476

‎pkg/controller/install/certresources_test.go

+107-21
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ import (
1616
"k8s.io/apimachinery/pkg/api/errors"
1717
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1818
"k8s.io/apimachinery/pkg/runtime/schema"
19+
corev1ac "k8s.io/client-go/applyconfigurations/core/v1"
20+
rbacv1ac "k8s.io/client-go/applyconfigurations/rbac/v1"
1921

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

172190
hosts := []string{
173191
fmt.Sprintf("%s.%s", service.GetName(), namespace),
@@ -255,7 +273,22 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
255273
},
256274
}
257275

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

260293
authReaderRoleBinding := &rbacv1.RoleBinding{
261294
Subjects: []rbacv1.Subject{
@@ -381,7 +414,6 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
381414
{
382415
name: "doesn't add duplicate service ownerrefs",
383416
mockExternal: func(mockOpClient *operatorclientmocks.MockClientInterface, fakeLister *operatorlisterfakes.FakeOperatorLister, namespace string, args args) {
384-
mockOpClient.EXPECT().DeleteService(namespace, "test-service", &metav1.DeleteOptions{}).Return(nil)
385417
service := corev1.Service{
386418
ObjectMeta: metav1.ObjectMeta{
387419
Name: "test-service",
@@ -396,7 +428,23 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
396428
Selector: selector(t, "test=label").MatchLabels,
397429
},
398430
}
399-
mockOpClient.EXPECT().CreateService(&service).Return(&service, nil)
431+
portsApplyConfig := []*corev1ac.ServicePortApplyConfiguration{}
432+
for _, p := range args.ports {
433+
ac := corev1ac.ServicePort().
434+
WithName(p.Name).
435+
WithPort(p.Port).
436+
WithTargetPort(p.TargetPort)
437+
portsApplyConfig = append(portsApplyConfig, ac)
438+
}
439+
440+
svcApplyConfig := corev1ac.Service(service.Name, service.Namespace).
441+
WithSpec(corev1ac.ServiceSpec().
442+
WithPorts(portsApplyConfig...).
443+
WithSelector(selector(t, "test=label").MatchLabels)).
444+
WithOwnerReferences(ownerutil.NonBlockingOwnerApplyConfiguration(owner)).
445+
WithLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue})
446+
447+
mockOpClient.EXPECT().ApplyService(svcApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(&service, nil)
400448

401449
hosts := []string{
402450
fmt.Sprintf("%s.%s", service.GetName(), namespace),
@@ -484,7 +532,23 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
484532
},
485533
}
486534

487-
mockOpClient.EXPECT().UpdateClusterRoleBinding(authDelegatorClusterRoleBinding).Return(authDelegatorClusterRoleBinding, nil)
535+
crbLabels := map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}
536+
for key, val := range ownerutil.OwnerLabel(owner, owner.GetObjectKind().GroupVersionKind().Kind) {
537+
crbLabels[key] = val
538+
}
539+
crbApplyConfig := rbacv1ac.ClusterRoleBinding(service.GetName() + "-system:auth-delegator").
540+
WithSubjects(rbacv1ac.Subject().
541+
WithKind("ServiceAccount").
542+
WithAPIGroup("").
543+
WithName("test-sa").
544+
WithNamespace(namespace)).
545+
WithRoleRef(rbacv1ac.RoleRef().
546+
WithAPIGroup("rbac.authorization.k8s.io").
547+
WithKind("ClusterRole").
548+
WithName("system:auth-delegator")).
549+
WithLabels(crbLabels)
550+
551+
mockOpClient.EXPECT().ApplyClusterRoleBinding(crbApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(authDelegatorClusterRoleBinding, nil)
488552

489553
authReaderRoleBinding := &rbacv1.RoleBinding{
490554
Subjects: []rbacv1.Subject{
@@ -602,9 +666,8 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
602666
},
603667
},
604668
{
605-
name: "labels an unlabelled secret if present",
669+
name: "labels an unlabelled secret if present; creates Service and ClusterRoleBinding if not existing",
606670
mockExternal: func(mockOpClient *operatorclientmocks.MockClientInterface, fakeLister *operatorlisterfakes.FakeOperatorLister, namespace string, args args) {
607-
mockOpClient.EXPECT().DeleteService(namespace, "test-service", &metav1.DeleteOptions{}).Return(nil)
608671
service := corev1.Service{
609672
ObjectMeta: metav1.ObjectMeta{
610673
Name: "test-service",
@@ -618,7 +681,24 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
618681
Selector: selector(t, "test=label").MatchLabels,
619682
},
620683
}
621-
mockOpClient.EXPECT().CreateService(&service).Return(&service, nil)
684+
685+
portsApplyConfig := []*corev1ac.ServicePortApplyConfiguration{}
686+
for _, p := range args.ports {
687+
ac := corev1ac.ServicePort().
688+
WithName(p.Name).
689+
WithPort(p.Port).
690+
WithTargetPort(p.TargetPort)
691+
portsApplyConfig = append(portsApplyConfig, ac)
692+
}
693+
694+
svcApplyConfig := corev1ac.Service(service.Name, service.Namespace).
695+
WithSpec(corev1ac.ServiceSpec().
696+
WithPorts(portsApplyConfig...).
697+
WithSelector(selector(t, "test=label").MatchLabels)).
698+
WithOwnerReferences(ownerutil.NonBlockingOwnerApplyConfiguration(&v1alpha1.ClusterServiceVersion{})).
699+
WithLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue})
700+
701+
mockOpClient.EXPECT().ApplyService(svcApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(&service, nil)
622702

623703
hosts := []string{
624704
fmt.Sprintf("%s.%s", service.GetName(), namespace),
@@ -715,8 +795,22 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
715795
Name: "system:auth-delegator",
716796
},
717797
}
718-
719-
mockOpClient.EXPECT().UpdateClusterRoleBinding(authDelegatorClusterRoleBinding).Return(authDelegatorClusterRoleBinding, nil)
798+
crbLabels := map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}
799+
for key, val := range ownerutil.OwnerLabel(ownerutil.Owner(&v1alpha1.ClusterServiceVersion{}), owner.GetObjectKind().GroupVersionKind().Kind) {
800+
crbLabels[key] = val
801+
}
802+
crbApplyConfig := rbacv1ac.ClusterRoleBinding(AuthDelegatorClusterRoleBindingName(service.GetName())).
803+
WithSubjects(rbacv1ac.Subject().WithKind("ServiceAccount").
804+
WithAPIGroup("").
805+
WithName("test-sa").
806+
WithNamespace(namespace)).
807+
WithRoleRef(rbacv1ac.RoleRef().
808+
WithAPIGroup("rbac.authorization.k8s.io").
809+
WithKind("ClusterRole").
810+
WithName("system:auth-delegator")).
811+
WithLabels(crbLabels)
812+
813+
mockOpClient.EXPECT().ApplyClusterRoleBinding(crbApplyConfig, metav1.ApplyOptions{Force: true, FieldManager: "olm.install"}).Return(authDelegatorClusterRoleBinding, nil)
720814

721815
authReaderRoleBinding := &rbacv1.RoleBinding{
722816
Subjects: []rbacv1.Subject{
@@ -740,13 +834,7 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
740834
mockOpClient.EXPECT().UpdateRoleBinding(authReaderRoleBinding).Return(authReaderRoleBinding, nil)
741835
},
742836
state: fakeState{
743-
existingService: &corev1.Service{
744-
ObjectMeta: metav1.ObjectMeta{
745-
OwnerReferences: []metav1.OwnerReference{
746-
ownerutil.NonBlockingOwner(&v1alpha1.ClusterServiceVersion{}),
747-
},
748-
},
749-
},
837+
existingService: nil,
750838
// unlabelled secret won't be in cache
751839
getSecretError: errors.NewNotFound(schema.GroupResource{
752840
Group: "",
@@ -758,9 +846,7 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
758846
existingRoleBinding: &rbacv1.RoleBinding{
759847
ObjectMeta: metav1.ObjectMeta{},
760848
},
761-
existingClusterRoleBinding: &rbacv1.ClusterRoleBinding{
762-
ObjectMeta: metav1.ObjectMeta{},
763-
},
849+
existingClusterRoleBinding: nil,
764850
},
765851
fields: fields{
766852
owner: &v1alpha1.ClusterServiceVersion{},

0 commit comments

Comments
 (0)