Skip to content

Commit ce46c64

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #1549 from ecordell/rbac-fix
Bug 1838054: fix(catalog): no operatorgroups in a namespace should be an error when resolving
2 parents d4fe858 + cf6c76e commit ce46c64

File tree

7 files changed

+403
-27
lines changed

7 files changed

+403
-27
lines changed

‎pkg/controller/operators/catalog/operator.go

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1170,6 +1170,28 @@ func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) {
11701170
return
11711171
}
11721172

1173+
querier := o.serviceAccountQuerier.NamespaceQuerier(plan.GetNamespace())
1174+
ref, err := querier()
1175+
if err != nil {
1176+
syncError = fmt.Errorf("attenuated service account query failed - %v", err)
1177+
return
1178+
}
1179+
1180+
if ref != nil {
1181+
out := plan.DeepCopy()
1182+
out.Status.AttenuatedServiceAccountRef = ref
1183+
1184+
if !reflect.DeepEqual(plan, out) {
1185+
if _, updateErr := o.client.OperatorsV1alpha1().InstallPlans(out.GetNamespace()).UpdateStatus(context.TODO(), out, metav1.UpdateOptions{}); updateErr != nil {
1186+
syncError = fmt.Errorf("failed to attach attenuated ServiceAccount to status - %v", updateErr)
1187+
return
1188+
}
1189+
1190+
logger.WithField("attenuated-sa", ref.Name).Info("successfully attached attenuated ServiceAccount to status")
1191+
return
1192+
}
1193+
}
1194+
11731195
// Attempt to unpack bundles before installing
11741196
// Note: This should probably use the attenuated client to prevent users from resolving resources they otherwise don't have access to.
11751197
if len(plan.Status.BundleLookups) > 0 {
@@ -1201,28 +1223,6 @@ func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) {
12011223
}
12021224
}
12031225

1204-
querier := o.serviceAccountQuerier.NamespaceQuerier(plan.GetNamespace())
1205-
ref, err := querier()
1206-
if err != nil {
1207-
syncError = fmt.Errorf("attenuated service account query failed - %v", err)
1208-
return
1209-
}
1210-
1211-
if ref != nil {
1212-
out := plan.DeepCopy()
1213-
out.Status.AttenuatedServiceAccountRef = ref
1214-
1215-
if !reflect.DeepEqual(plan, out) {
1216-
if _, updateErr := o.client.OperatorsV1alpha1().InstallPlans(out.GetNamespace()).UpdateStatus(context.TODO(), out, metav1.UpdateOptions{}); updateErr != nil {
1217-
syncError = fmt.Errorf("failed to attach attenuated ServiceAccount to status - %v", updateErr)
1218-
return
1219-
}
1220-
1221-
logger.WithField("attenuated-sa", ref.Name).Info("successfully attached attenuated ServiceAccount to status")
1222-
return
1223-
}
1224-
}
1225-
12261226
outInstallPlan, syncError := transitionInstallPlanState(logger.Logger, o, *plan, o.now())
12271227

12281228
if syncError != nil {

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,57 @@ func TestTransitionInstallPlan(t *testing.T) {
135135
}
136136
}
137137

138+
func TestSyncInstallPlanUnhappy(t *testing.T) {
139+
namespace := "ns"
140+
141+
tests := []struct {
142+
testName string
143+
in *v1alpha1.InstallPlan
144+
err error
145+
}{
146+
{
147+
testName: "NoStatus",
148+
in: installPlan("p", namespace, v1alpha1.InstallPlanPhaseNone),
149+
err: nil,
150+
},
151+
{
152+
// This checks that installplans are not applied when no operatorgroup is present
153+
testName: "HasSteps/NoOperatorGroup",
154+
in: withSteps(installPlan("p", namespace, v1alpha1.InstallPlanPhaseInstalling, "csv"),
155+
[]*v1alpha1.Step{
156+
{
157+
Resource: v1alpha1.StepResource{
158+
CatalogSource: "catalog",
159+
CatalogSourceNamespace: namespace,
160+
Group: "",
161+
Version: "v1",
162+
Kind: "ServiceAccount",
163+
Name: "sa",
164+
Manifest: toManifest(t, serviceAccount("sa", namespace, "",
165+
objectReference("init secret"))),
166+
},
167+
Status: v1alpha1.StepStatusUnknown,
168+
},
169+
},
170+
),
171+
err: fmt.Errorf("attenuated service account query failed - no operator group found that is managing this namespace"),
172+
},
173+
}
174+
175+
for _, tt := range tests {
176+
t.Run(tt.testName, func(t *testing.T) {
177+
ctx, cancel := context.WithCancel(context.TODO())
178+
defer cancel()
179+
180+
op, err := NewFakeOperator(ctx, namespace, []string{namespace}, withClientObjs(tt.in))
181+
require.NoError(t, err)
182+
183+
err = op.syncInstallPlans(tt.in)
184+
require.Equal(t, tt.err, err)
185+
})
186+
}
187+
}
188+
138189
func TestExecutePlan(t *testing.T) {
139190
namespace := "ns"
140191

‎pkg/lib/scoped/querier.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,27 +35,27 @@ func (f *UserDefinedServiceAccountQuerier) NamespaceQuerier(namespace string) Se
3535
logFieldName: logFieldValue,
3636
})
3737

38-
return QueryServiceAccountFromNamespace(logger, f.crclient, namespace)
38+
return queryServiceAccountFromNamespace(logger, f.crclient, namespace)
3939
}
4040

4141
return querierFunc
4242
}
4343

4444
// QueryServiceAccountFromNamespace will return the reference to a service account
4545
// associated with the operator group for the given namespace.
46-
// - If no operator group is found in the namespace, both reference and err are set to nil.
46+
// - If no operator group is found in the namespace, an error is returned.
4747
// - If an operator group found is not managing the namespace then it is ignored.
4848
// - If no operator group is managing this namespace then both reference and err are set to nil.
4949
// - If more than one operator group are managing this namespace then an error is thrown.
50-
func QueryServiceAccountFromNamespace(logger *logrus.Entry, crclient versioned.Interface, namespace string) (reference *corev1.ObjectReference, err error) {
50+
func queryServiceAccountFromNamespace(logger *logrus.Entry, crclient versioned.Interface, namespace string) (reference *corev1.ObjectReference, err error) {
5151
// TODO: use a lister instead of a noncached client here.
5252
list, err := crclient.OperatorsV1().OperatorGroups(namespace).List(context.TODO(), metav1.ListOptions{})
5353
if err != nil {
5454
return
5555
}
5656

5757
if len(list.Items) == 0 {
58-
logger.Warnf("list query returned an empty list")
58+
err = fmt.Errorf("no operator group found that is managing this namespace")
5959
return
6060
}
6161

@@ -70,7 +70,7 @@ func QueryServiceAccountFromNamespace(logger *logrus.Entry, crclient versioned.I
7070
}
7171

7272
if len(groups) == 0 {
73-
logger.Warn("no operator group found that is managing this namespace")
73+
err = fmt.Errorf("no operator group found that is managing this namespace")
7474
return
7575
}
7676

‎pkg/lib/scoped/querier_test.go

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
package scoped
2+
3+
import (
4+
"fmt"
5+
"testing"
6+
7+
"github.com/sirupsen/logrus/hooks/test"
8+
"github.com/stretchr/testify/require"
9+
corev1 "k8s.io/api/core/v1"
10+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
11+
12+
v1 "github.com/operator-framework/api/pkg/operators/v1"
13+
"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned"
14+
"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake"
15+
)
16+
17+
func TestUserDefinedServiceAccountQuerier(t *testing.T) {
18+
tests := []struct {
19+
name string
20+
crclient versioned.Interface
21+
namespace string
22+
wantReference *corev1.ObjectReference
23+
wantErr bool
24+
err error
25+
}{
26+
{
27+
name: "NoOperatorGroup",
28+
crclient: fake.NewSimpleClientset(),
29+
namespace: "ns",
30+
wantErr: true,
31+
err: fmt.Errorf("no operator group found that is managing this namespace"),
32+
},
33+
{
34+
name: "OperatorGroup/NamespaceNotInSpec",
35+
crclient: fake.NewSimpleClientset(&v1.OperatorGroup{
36+
ObjectMeta: metav1.ObjectMeta{
37+
Name: "og",
38+
Namespace: "ns",
39+
},
40+
Spec: v1.OperatorGroupSpec{
41+
TargetNamespaces: []string{"other"},
42+
},
43+
}),
44+
namespace: "ns",
45+
wantErr: true,
46+
err: fmt.Errorf("no operator group found that is managing this namespace"),
47+
},
48+
{
49+
name: "OperatorGroup/NamespaceNotInStatus",
50+
crclient: fake.NewSimpleClientset(&v1.OperatorGroup{
51+
ObjectMeta: metav1.ObjectMeta{
52+
Name: "og",
53+
Namespace: "ns",
54+
},
55+
Spec: v1.OperatorGroupSpec{
56+
TargetNamespaces: []string{"ns"},
57+
},
58+
}),
59+
namespace: "ns",
60+
wantErr: true,
61+
err: fmt.Errorf("no operator group found that is managing this namespace"),
62+
},
63+
{
64+
name: "OperatorGroup/Multiple",
65+
crclient: fake.NewSimpleClientset(
66+
&v1.OperatorGroup{
67+
ObjectMeta: metav1.ObjectMeta{
68+
Name: "og",
69+
Namespace: "ns",
70+
},
71+
Spec: v1.OperatorGroupSpec{
72+
TargetNamespaces: []string{"ns"},
73+
},
74+
Status: v1.OperatorGroupStatus{
75+
Namespaces: []string{"ns"},
76+
},
77+
},
78+
&v1.OperatorGroup{
79+
ObjectMeta: metav1.ObjectMeta{
80+
Name: "og2",
81+
Namespace: "ns",
82+
},
83+
Spec: v1.OperatorGroupSpec{
84+
TargetNamespaces: []string{"ns"},
85+
},
86+
Status: v1.OperatorGroupStatus{
87+
Namespaces: []string{"ns"},
88+
},
89+
},
90+
),
91+
namespace: "ns",
92+
wantErr: true,
93+
err: fmt.Errorf("more than one operator group(s) are managing this namespace count=2"),
94+
},
95+
{
96+
name: "OperatorGroup/NamespaceInStatus/NoSA",
97+
crclient: fake.NewSimpleClientset(&v1.OperatorGroup{
98+
ObjectMeta: metav1.ObjectMeta{
99+
Name: "og",
100+
Namespace: "ns",
101+
},
102+
Spec: v1.OperatorGroupSpec{
103+
TargetNamespaces: []string{"ns"},
104+
},
105+
Status: v1.OperatorGroupStatus{
106+
Namespaces: []string{"ns"},
107+
},
108+
}),
109+
namespace: "ns",
110+
wantErr: false,
111+
err: nil,
112+
},
113+
{
114+
name: "OperatorGroup/NamespaceInStatus/ServiceAccountRef",
115+
crclient: fake.NewSimpleClientset(&v1.OperatorGroup{
116+
ObjectMeta: metav1.ObjectMeta{
117+
Name: "og",
118+
Namespace: "ns",
119+
},
120+
Spec: v1.OperatorGroupSpec{
121+
TargetNamespaces: []string{"ns"},
122+
ServiceAccountName: "sa",
123+
},
124+
Status: v1.OperatorGroupStatus{
125+
Namespaces: []string{"ns"},
126+
ServiceAccountRef: &corev1.ObjectReference{
127+
Kind: "ServiceAccount",
128+
Namespace: "ns",
129+
Name: "sa",
130+
},
131+
},
132+
}),
133+
namespace: "ns",
134+
wantErr: false,
135+
err: nil,
136+
wantReference: &corev1.ObjectReference{
137+
Kind: "ServiceAccount",
138+
Namespace: "ns",
139+
Name: "sa",
140+
},
141+
},
142+
}
143+
for _, tt := range tests {
144+
t.Run(tt.name, func(t *testing.T) {
145+
logger, _ := test.NewNullLogger()
146+
f := &UserDefinedServiceAccountQuerier{
147+
crclient: tt.crclient,
148+
logger: logger,
149+
}
150+
gotReference, err := f.NamespaceQuerier(tt.namespace)()
151+
if tt.wantErr {
152+
require.Equal(t, tt.err, err)
153+
} else {
154+
require.Nil(t, tt.err)
155+
}
156+
require.Equal(t, tt.wantReference, gotReference)
157+
})
158+
}
159+
}

0 commit comments

Comments
 (0)