Skip to content

Commit 0262af0

Browse files
ecordellecordell
committed
fix(catalog): no operatorgroups in a namespace should be an error when resolving
1 parent 1849f65 commit 0262af0

File tree

7 files changed

+398
-27
lines changed

7 files changed

+398
-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+
}

‎test/e2e/installplan_e2e_test.go

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2621,7 +2621,75 @@ var _ = Describe("Install Plan", func() {
26212621
require.NoError(GinkgoT(), err)
26222622
require.Equal(GinkgoT(), 1, len(ips.Items), "If this test fails it should be taken seriously and not treated as a flake. \n%v", ips.Items)
26232623
})
2624+
2625+
It("without an operatorgroup", func() {
2626+
defer cleaner.NotifyTestComplete(true)
2627+
2628+
log := func(s string) {
2629+
GinkgoT().Logf("%s: %s", time.Now().Format("15:04:05.9999"), s)
2630+
}
2631+
2632+
ns := &corev1.Namespace{}
2633+
ns.SetName(genName("ns-"))
2634+
2635+
c := newKubeClient()
2636+
crc := newCRClient()
2637+
2638+
// Create a namespace
2639+
ns, err := c.KubernetesInterface().CoreV1().Namespaces().Create(context.TODO(), ns, metav1.CreateOptions{})
2640+
require.NoError(GinkgoT(), err)
2641+
deleteOpts := &metav1.DeleteOptions{}
2642+
defer func() {
2643+
require.NoError(GinkgoT(), c.KubernetesInterface().CoreV1().Namespaces().Delete(context.TODO(), ns.GetName(), *deleteOpts))
2644+
}()
2645+
2646+
mainPackageName := genName("nginx-")
2647+
mainPackageStable := fmt.Sprintf("%s-stable", mainPackageName)
2648+
stableChannel := "stable"
2649+
mainNamedStrategy := newNginxInstallStrategy(genName("dep-"), nil, nil)
2650+
mainCSV := newCSV(mainPackageStable, ns.GetName(), "", semver.MustParse("0.1.0"), nil, nil, mainNamedStrategy)
2651+
2652+
defer func() {
2653+
require.NoError(GinkgoT(), crc.OperatorsV1alpha1().Subscriptions(ns.GetName()).DeleteCollection(context.TODO(), metav1.DeleteOptions{}, metav1.ListOptions{}))
2654+
}()
2655+
2656+
mainCatalogName := genName("mock-ocs-main-")
2657+
mainManifests := []registry.PackageManifest{
2658+
{
2659+
PackageName: mainPackageName,
2660+
Channels: []registry.PackageChannel{
2661+
{Name: stableChannel, CurrentCSVName: mainPackageStable},
2662+
},
2663+
DefaultChannelName: stableChannel,
2664+
},
2665+
}
2666+
2667+
// Create the main catalog source
2668+
_, cleanupMainCatalogSource := createInternalCatalogSource(c, crc, mainCatalogName, ns.GetName(), mainManifests, nil, []operatorsv1alpha1.ClusterServiceVersion{mainCSV})
2669+
defer cleanupMainCatalogSource()
2670+
2671+
// Attempt to get the catalog source before creating install plan
2672+
_, err = fetchCatalogSourceOnStatus(crc, mainCatalogName, ns.GetName(), catalogSourceRegistryPodSynced)
2673+
require.NoError(GinkgoT(), err)
26242674

2675+
subscriptionName := genName("sub-nginx-")
2676+
subscriptionCleanup := createSubscriptionForCatalog(crc, ns.GetName(), subscriptionName, mainCatalogName, mainPackageName, stableChannel, "", operatorsv1alpha1.ApprovalAutomatic)
2677+
defer subscriptionCleanup()
2678+
2679+
subscription, err := fetchSubscription(crc, ns.GetName(), subscriptionName, subscriptionHasInstallPlanChecker)
2680+
require.NoError(GinkgoT(), err)
2681+
require.NotNil(GinkgoT(), subscription)
2682+
2683+
installPlanName := subscription.Status.InstallPlanRef.Name
2684+
2685+
fetchedInstallPlan, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, installPlanName, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseInstalling))
2686+
require.NoError(GinkgoT(), err)
2687+
log(fmt.Sprintf("Install plan %s fetched with status %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase))
2688+
2689+
fetchedInstallPlan, err = fetchInstallPlanWithNamespace(GinkgoT(), crc, installPlanName, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseInstalling))
2690+
require.NoError(GinkgoT(), err)
2691+
log(fmt.Sprintf("Install plan %s fetched with status %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase))
2692+
})
26252693
})
26262694

26272695
type checkInstallPlanFunc func(fip *operatorsv1alpha1.InstallPlan) bool

0 commit comments

Comments
 (0)