Skip to content

Commit cfda6a3

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #1881 from horis233/check-sa-owner
Bug 1857877: check the service account owner in the requirement
2 parents 271771d + f7dad7d commit cfda6a3

File tree

5 files changed

+212
-25
lines changed

5 files changed

+212
-25
lines changed

‎pkg/controller/operators/olm/operator_test.go

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import (
3434
utilclock "k8s.io/apimachinery/pkg/util/clock"
3535
"k8s.io/apimachinery/pkg/util/diff"
3636
utilerrors "k8s.io/apimachinery/pkg/util/errors"
37+
"k8s.io/apimachinery/pkg/types"
3738
"k8s.io/apimachinery/pkg/util/intstr"
3839
"k8s.io/apimachinery/pkg/util/wait"
3940
k8sfake "k8s.io/client-go/kubernetes/fake"
@@ -513,6 +514,19 @@ func csvWithAnnotations(csv *v1alpha1.ClusterServiceVersion, annotations map[str
513514
return withAnnotations(csv, annotations).(*v1alpha1.ClusterServiceVersion)
514515
}
515516

517+
func withUID(obj runtime.Object, uid types.UID) runtime.Object {
518+
meta, ok := obj.(metav1.Object)
519+
if !ok {
520+
panic("could not find metadata on object")
521+
}
522+
meta.SetUID(uid)
523+
return meta.(runtime.Object)
524+
}
525+
526+
func csvWithUID(csv *v1alpha1.ClusterServiceVersion, uid types.UID) *v1alpha1.ClusterServiceVersion {
527+
return withUID(csv, uid).(*v1alpha1.ClusterServiceVersion)
528+
}
529+
516530
func withLabels(obj runtime.Object, labels map[string]string) runtime.Object {
517531
meta, ok := obj.(metav1.Object)
518532
if !ok {
@@ -960,7 +974,7 @@ func TestTransitionCSV(t *testing.T) {
960974
name: "SingleCSVPendingToFailed/BadStrategyPermissions",
961975
initial: initial{
962976
csvs: []runtime.Object{
963-
csvWithAnnotations(csv("csv1",
977+
csvWithUID(csvWithAnnotations(csv("csv1",
964978
namespace,
965979
"0.0.0",
966980
"",
@@ -981,7 +995,7 @@ func TestTransitionCSV(t *testing.T) {
981995
[]*apiextensionsv1.CustomResourceDefinition{crd("c1", "v1", "g1")},
982996
[]*apiextensionsv1.CustomResourceDefinition{},
983997
v1alpha1.CSVPhasePending,
984-
), defaultTemplateAnnotations),
998+
), defaultTemplateAnnotations), types.UID("csv-uid")),
985999
},
9861000
clientObjs: []runtime.Object{addAnnotation(defaultOperatorGroup, v1.OperatorGroupProvidedAPIsAnnotationKey, "c1.v1.g1")},
9871001
crds: []runtime.Object{
@@ -992,6 +1006,12 @@ func TestTransitionCSV(t *testing.T) {
9921006
ObjectMeta: metav1.ObjectMeta{
9931007
Name: "sa",
9941008
Namespace: namespace,
1009+
OwnerReferences: []metav1.OwnerReference{
1010+
{
1011+
Kind: v1alpha1.ClusterServiceVersionKind,
1012+
UID: "csv-uid",
1013+
},
1014+
},
9951015
},
9961016
},
9971017
},

‎pkg/controller/operators/olm/operatorgroup.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -432,7 +432,7 @@ func (a *Operator) ensureRBACInTargetNamespace(csv *v1alpha1.ClusterServiceVersi
432432
strategyDetailsDeployment.ClusterPermissions = append(strategyDetailsDeployment.ClusterPermissions, p)
433433
}
434434
strategyDetailsDeployment.Permissions = nil
435-
permMet, _, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, corev1.NamespaceAll, csv.GetNamespace())
435+
permMet, _, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, corev1.NamespaceAll, csv)
436436
if err != nil {
437437
return err
438438
}
@@ -669,7 +669,7 @@ func (a *Operator) ensureCSVsInNamespaces(csv *v1alpha1.ClusterServiceVersion, o
669669
}
670670
for _, ns := range targetNamespaces {
671671
// create roles/rolebindings for each target namespace
672-
permMet, _, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, ns, csv.GetNamespace())
672+
permMet, _, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, ns, csv)
673673
if err != nil {
674674
logger.WithError(err).Debug("permission status")
675675
return err

‎pkg/controller/operators/olm/requirements.go

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,15 @@ import (
55
"fmt"
66
"strings"
77

8+
"github.com/coreos/go-semver/semver"
89
"github.com/sirupsen/logrus"
10+
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
11+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
912

10-
"github.com/coreos/go-semver/semver"
1113
"github.com/operator-framework/api/pkg/operators/v1alpha1"
1214
olmErrors "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/errors"
1315
"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
14-
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
15-
16-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
16+
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
1717
)
1818

1919
func (a *Operator) minKubeVersionStatus(name string, minKubeVersion string) (met bool, statuses []v1alpha1.RequirementStatus) {
@@ -237,7 +237,7 @@ func (a *Operator) requirementStatus(strategyDetailsDeployment *v1alpha1.Strateg
237237
}
238238

239239
// permissionStatus checks whether the given CSV's RBAC requirements are met in its namespace
240-
func (a *Operator) permissionStatus(strategyDetailsDeployment *v1alpha1.StrategyDetailsDeployment, ruleChecker install.RuleChecker, targetNamespace, serviceAccountNamespace string) (bool, []v1alpha1.RequirementStatus, error) {
240+
func (a *Operator) permissionStatus(strategyDetailsDeployment *v1alpha1.StrategyDetailsDeployment, ruleChecker install.RuleChecker, targetNamespace string, csv *v1alpha1.ClusterServiceVersion) (bool, []v1alpha1.RequirementStatus, error) {
241241
statusesSet := map[string]v1alpha1.RequirementStatus{}
242242

243243
checkPermissions := func(permissions []v1alpha1.StrategyDeploymentPermissions, namespace string) (bool, error) {
@@ -261,7 +261,7 @@ func (a *Operator) permissionStatus(strategyDetailsDeployment *v1alpha1.Strategy
261261
}
262262

263263
// Ensure the ServiceAccount exists
264-
sa, err := a.opClient.GetServiceAccount(serviceAccountNamespace, perm.ServiceAccountName)
264+
sa, err := a.opClient.GetServiceAccount(csv.GetNamespace(), perm.ServiceAccountName)
265265
if err != nil {
266266
met = false
267267
status.Status = v1alpha1.RequirementStatusReasonNotPresent
@@ -270,6 +270,15 @@ func (a *Operator) permissionStatus(strategyDetailsDeployment *v1alpha1.Strategy
270270
continue
271271
}
272272

273+
// Check if the ServiceAccount is owned by CSV
274+
if len(sa.GetOwnerReferences()) != 0 && !ownerutil.IsOwnedBy(sa, csv) {
275+
met = false
276+
status.Status = v1alpha1.RequirementStatusReasonPresentNotSatisfied
277+
status.Message = "Service account is not owned by this ClusterServiceVersion"
278+
statusesSet[saName] = status
279+
continue
280+
}
281+
273282
// Check if PolicyRules are satisfied
274283
for _, rule := range perm.Rules {
275284
dependent := v1alpha1.DependentStatus{
@@ -365,7 +374,7 @@ func (a *Operator) requirementAndPermissionStatus(csv *v1alpha1.ClusterServiceVe
365374
clusterRoleBindingLister := rbacLister.ClusterRoleBindingLister()
366375

367376
ruleChecker := install.NewCSVRuleChecker(roleLister, roleBindingLister, clusterRoleLister, clusterRoleBindingLister, csv)
368-
permMet, permStatuses, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, csv.GetNamespace(), csv.GetNamespace())
377+
permMet, permStatuses, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, csv.GetNamespace(), csv)
369378
if err != nil {
370379
return false, nil, err
371380
}

‎pkg/controller/operators/olm/requirements_test.go

Lines changed: 157 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
3535
}{
3636
{
3737
description: "AllPermissionsMet",
38-
csv: csv("csv1",
38+
csv: csvWithUID(csv("csv1",
3939
namespace,
4040
"0.0.0",
4141
"",
@@ -68,13 +68,19 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
6868
nil,
6969
nil,
7070
v1alpha1.CSVPhasePending,
71-
),
71+
), types.UID("csv-uid")),
7272
existingObjs: []runtime.Object{
7373
&corev1.ServiceAccount{
7474
ObjectMeta: metav1.ObjectMeta{
7575
Name: "sa",
7676
Namespace: namespace,
7777
UID: types.UID("sa"),
78+
OwnerReferences: []metav1.OwnerReference{
79+
{
80+
Kind: v1alpha1.ClusterServiceVersionKind,
81+
UID: "csv-uid",
82+
},
83+
},
7884
},
7985
},
8086
&rbacv1.Role{
@@ -173,7 +179,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
173179
},
174180
{
175181
description: "OnePermissionNotMet",
176-
csv: csv("csv1",
182+
csv: csvWithUID(csv("csv1",
177183
namespace,
178184
"0.0.0",
179185
"",
@@ -206,13 +212,19 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
206212
nil,
207213
nil,
208214
v1alpha1.CSVPhasePending,
209-
),
215+
), types.UID("csv-uid")),
210216
existingObjs: []runtime.Object{
211217
&corev1.ServiceAccount{
212218
ObjectMeta: metav1.ObjectMeta{
213219
Name: "sa",
214220
Namespace: namespace,
215221
UID: types.UID("sa"),
222+
OwnerReferences: []metav1.OwnerReference{
223+
{
224+
Kind: v1alpha1.ClusterServiceVersionKind,
225+
UID: "csv-uid",
226+
},
227+
},
216228
},
217229
},
218230
&rbacv1.Role{
@@ -309,9 +321,142 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
309321
},
310322
expectedError: nil,
311323
},
324+
{
325+
description: "RequirementNotMet/ServiceAccountOwnerConflict",
326+
csv: csvWithUID(csv("csv1",
327+
namespace,
328+
"0.0.0",
329+
"",
330+
installStrategy(
331+
"csv1-dep",
332+
[]v1alpha1.StrategyDeploymentPermissions{
333+
{
334+
ServiceAccountName: "sa",
335+
Rules: []rbacv1.PolicyRule{
336+
{
337+
APIGroups: []string{""},
338+
Verbs: []string{"*"},
339+
Resources: []string{"donuts"},
340+
},
341+
},
342+
},
343+
},
344+
[]v1alpha1.StrategyDeploymentPermissions{
345+
{
346+
ServiceAccountName: "sa",
347+
Rules: []rbacv1.PolicyRule{
348+
{
349+
Verbs: []string{"get"},
350+
NonResourceURLs: []string{"/osbs"},
351+
},
352+
},
353+
},
354+
},
355+
),
356+
nil,
357+
nil,
358+
v1alpha1.CSVPhasePending,
359+
), types.UID("csv-uid")),
360+
existingObjs: []runtime.Object{
361+
&corev1.ServiceAccount{
362+
ObjectMeta: metav1.ObjectMeta{
363+
Name: "sa",
364+
Namespace: namespace,
365+
UID: types.UID("sa"),
366+
OwnerReferences: []metav1.OwnerReference{
367+
{
368+
Kind: v1alpha1.ClusterServiceVersionKind,
369+
UID: "csv-uid-other",
370+
},
371+
},
372+
},
373+
},
374+
&rbacv1.Role{
375+
ObjectMeta: metav1.ObjectMeta{
376+
Name: "role",
377+
Namespace: namespace,
378+
},
379+
Rules: []rbacv1.PolicyRule{
380+
{
381+
APIGroups: []string{""},
382+
Verbs: []string{"*"},
383+
Resources: []string{"donuts"},
384+
},
385+
},
386+
},
387+
&rbacv1.RoleBinding{
388+
ObjectMeta: metav1.ObjectMeta{
389+
Name: "roleBinding",
390+
Namespace: namespace,
391+
},
392+
Subjects: []rbacv1.Subject{
393+
{
394+
Kind: "ServiceAccount",
395+
APIGroup: "",
396+
Name: "sa",
397+
Namespace: namespace,
398+
},
399+
},
400+
RoleRef: rbacv1.RoleRef{
401+
APIGroup: "rbac.authorization.k8s.io",
402+
Kind: "Role",
403+
Name: "role",
404+
},
405+
},
406+
&rbacv1.ClusterRole{
407+
ObjectMeta: metav1.ObjectMeta{
408+
Name: "clusterRole",
409+
},
410+
Rules: []rbacv1.PolicyRule{
411+
{
412+
Verbs: []string{"get"},
413+
NonResourceURLs: []string{"/osbs"},
414+
},
415+
},
416+
},
417+
&rbacv1.ClusterRoleBinding{
418+
ObjectMeta: metav1.ObjectMeta{
419+
Name: "clusterRoleBinding",
420+
},
421+
Subjects: []rbacv1.Subject{
422+
{
423+
Kind: "ServiceAccount",
424+
APIGroup: "",
425+
Name: "sa",
426+
Namespace: namespace,
427+
},
428+
},
429+
RoleRef: rbacv1.RoleRef{
430+
APIGroup: "rbac.authorization.k8s.io",
431+
Kind: "ClusterRole",
432+
Name: "clusterRole",
433+
},
434+
},
435+
},
436+
existingExtObjs: nil,
437+
met: false,
438+
expectedRequirementStatuses: map[gvkn]v1alpha1.RequirementStatus{
439+
{"", "v1", "ServiceAccount", "sa"}: {
440+
Group: "",
441+
Version: "v1",
442+
Kind: "ServiceAccount",
443+
Name: "sa",
444+
Status: v1alpha1.RequirementStatusReasonPresentNotSatisfied,
445+
Dependents: []v1alpha1.DependentStatus{},
446+
},
447+
{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv1"}: {
448+
Group: "operators.coreos.com",
449+
Version: "v1alpha1",
450+
Kind: "ClusterServiceVersion",
451+
Name: "csv1",
452+
Status: v1alpha1.RequirementStatusReasonPresent,
453+
},
454+
},
455+
expectedError: nil,
456+
},
312457
{
313458
description: "AllRequirementsMet",
314-
csv: csv("csv1",
459+
csv: csvWithUID(csv("csv1",
315460
namespace,
316461
"0.0.0",
317462
"",
@@ -334,13 +479,19 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
334479
[]*apiextensionsv1.CustomResourceDefinition{crd("c1", "v1", "g1")},
335480
[]*apiextensionsv1.CustomResourceDefinition{crd("c2", "v1", "g2")},
336481
v1alpha1.CSVPhasePending,
337-
),
482+
), types.UID("csv-uid")),
338483
existingObjs: []runtime.Object{
339484
&corev1.ServiceAccount{
340485
ObjectMeta: metav1.ObjectMeta{
341486
Name: "sa",
342487
Namespace: namespace,
343488
UID: types.UID("sa"),
489+
OwnerReferences: []metav1.OwnerReference{
490+
{
491+
Kind: v1alpha1.ClusterServiceVersionKind,
492+
UID: "csv-uid",
493+
},
494+
},
344495
},
345496
},
346497
&rbacv1.Role{

0 commit comments

Comments
 (0)