Skip to content

Commit ad235a6

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #2076 from benluddy/serviceaccount-owner-handoff
Preserve existing ServiceAccount owner references during installs.
2 parents 5cd7822 + 1e463a9 commit ad235a6

File tree

4 files changed

+316
-1
lines changed

4 files changed

+316
-1
lines changed

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ func (o *StepEnsurer) EnsureServiceAccount(namespace string, sa *corev1.ServiceA
156156
return
157157
}
158158
sa.Secrets = preSa.Secrets
159+
sa.OwnerReferences = mergedOwnerReferences(preSa.OwnerReferences, sa.OwnerReferences)
159160

160161
sa.SetNamespace(namespace)
161162

@@ -349,3 +350,20 @@ func (o *StepEnsurer) EnsureConfigMap(namespace string, configmap *corev1.Config
349350
status = v1alpha1.StepStatusPresent
350351
return
351352
}
353+
354+
func mergedOwnerReferences(in ...[]metav1.OwnerReference) []metav1.OwnerReference {
355+
uniques := make(map[metav1.OwnerReference]struct{})
356+
for _, refs := range in {
357+
for _, ref := range refs {
358+
uniques[ref] = struct{}{}
359+
}
360+
}
361+
if len(uniques) == 0 {
362+
return nil
363+
}
364+
merged := make([]metav1.OwnerReference, 0, len(uniques))
365+
for ref := range uniques {
366+
merged = append(merged, ref)
367+
}
368+
return merged
369+
}

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

Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,190 @@
1+
package catalog
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
8+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
9+
)
10+
11+
func TestMergedOwnerReferences(t *testing.T) {
12+
var (
13+
True bool = true
14+
False bool = false
15+
)
16+
17+
for _, tc := range []struct {
18+
Name string
19+
In [][]metav1.OwnerReference
20+
Out []metav1.OwnerReference
21+
}{
22+
{
23+
Name: "empty",
24+
},
25+
{
26+
Name: "different uid",
27+
In: [][]metav1.OwnerReference{
28+
{
29+
{
30+
APIVersion: "a",
31+
Kind: "b",
32+
Name: "c",
33+
Controller: &True,
34+
BlockOwnerDeletion: &True,
35+
UID: "x",
36+
},
37+
{
38+
APIVersion: "a",
39+
Kind: "b",
40+
Name: "c",
41+
Controller: &True,
42+
BlockOwnerDeletion: &True,
43+
UID: "y",
44+
},
45+
},
46+
},
47+
Out: []metav1.OwnerReference{
48+
{
49+
APIVersion: "a",
50+
Kind: "b",
51+
Name: "c",
52+
Controller: &True,
53+
BlockOwnerDeletion: &True,
54+
UID: "x",
55+
},
56+
{
57+
APIVersion: "a",
58+
Kind: "b",
59+
Name: "c",
60+
Controller: &True,
61+
BlockOwnerDeletion: &True,
62+
UID: "y",
63+
},
64+
},
65+
},
66+
{
67+
Name: "different controller",
68+
In: [][]metav1.OwnerReference{
69+
{
70+
{
71+
APIVersion: "a",
72+
Kind: "b",
73+
Name: "c",
74+
Controller: &True,
75+
BlockOwnerDeletion: &True,
76+
UID: "x",
77+
},
78+
{
79+
APIVersion: "a",
80+
Kind: "b",
81+
Name: "c",
82+
Controller: &False,
83+
BlockOwnerDeletion: &True,
84+
UID: "x",
85+
},
86+
},
87+
},
88+
Out: []metav1.OwnerReference{
89+
{
90+
APIVersion: "a",
91+
Kind: "b",
92+
Name: "c",
93+
Controller: &True,
94+
BlockOwnerDeletion: &True,
95+
UID: "x",
96+
},
97+
{
98+
APIVersion: "a",
99+
Kind: "b",
100+
Name: "c",
101+
Controller: &False,
102+
BlockOwnerDeletion: &True,
103+
UID: "x",
104+
},
105+
},
106+
},
107+
{
108+
Name: "add owner without uid",
109+
In: [][]metav1.OwnerReference{
110+
{
111+
{
112+
APIVersion: "a",
113+
Kind: "b",
114+
Name: "c-1",
115+
Controller: &False,
116+
BlockOwnerDeletion: &False,
117+
UID: "x",
118+
},
119+
},
120+
{
121+
{
122+
APIVersion: "a",
123+
Kind: "b",
124+
Name: "c-2",
125+
Controller: &False,
126+
BlockOwnerDeletion: &False,
127+
UID: "",
128+
},
129+
},
130+
},
131+
Out: []metav1.OwnerReference{
132+
{
133+
APIVersion: "a",
134+
Kind: "b",
135+
Name: "c-1",
136+
Controller: &False,
137+
BlockOwnerDeletion: &False,
138+
UID: "x",
139+
},
140+
{
141+
APIVersion: "a",
142+
Kind: "b",
143+
Name: "c-2",
144+
Controller: &False,
145+
BlockOwnerDeletion: &False,
146+
UID: "",
147+
},
148+
},
149+
},
150+
{
151+
Name: "duplicates combined",
152+
In: [][]metav1.OwnerReference{
153+
{
154+
{
155+
APIVersion: "a",
156+
Kind: "b",
157+
Name: "c",
158+
Controller: &False,
159+
BlockOwnerDeletion: &False,
160+
UID: "x",
161+
},
162+
},
163+
{
164+
{
165+
APIVersion: "a",
166+
Kind: "b",
167+
Name: "c",
168+
Controller: &False,
169+
BlockOwnerDeletion: &False,
170+
UID: "x",
171+
},
172+
},
173+
},
174+
Out: []metav1.OwnerReference{
175+
{
176+
APIVersion: "a",
177+
Kind: "b",
178+
Name: "c",
179+
Controller: &False,
180+
BlockOwnerDeletion: &False,
181+
UID: "x",
182+
},
183+
},
184+
},
185+
} {
186+
t.Run(tc.Name, func(t *testing.T) {
187+
assert.ElementsMatch(t, tc.Out, mergedOwnerReferences(tc.In...))
188+
})
189+
}
190+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1514,7 +1514,7 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
15141514
// Check if we're not ready to install part of the replacement chain yet
15151515
if prev := a.isReplacing(out); prev != nil {
15161516
if prev.Status.Phase != v1alpha1.CSVPhaseReplacing {
1517-
logger.WithError(fmt.Errorf("CSV being replaced is in phase %s instead of %s", prev.Status.Phase, v1alpha1.CSVPhaseReplacing)).Debug("Unable to replace previous CSV")
1517+
logger.WithError(fmt.Errorf("CSV being replaced is in phase %s instead of %s", prev.Status.Phase, v1alpha1.CSVPhaseReplacing)).Warn("Unable to replace previous CSV")
15181518
return
15191519
}
15201520
}

‎test/e2e/installplan_e2e_test.go

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import (
3131
"k8s.io/apimachinery/pkg/util/wait"
3232
"k8s.io/apimachinery/pkg/watch"
3333
"k8s.io/client-go/util/retry"
34+
"sigs.k8s.io/controller-runtime/pkg/client"
3435
runtimeclient "sigs.k8s.io/controller-runtime/pkg/client"
3536

3637
opver "github.com/operator-framework/api/pkg/lib/version"
@@ -50,6 +51,112 @@ var _ = Describe("Install Plan", func() {
5051
TearDown(testNamespace)
5152
})
5253

54+
When("an InstallPlan transfers ownership of a ServiceAccount to a new ClusterServiceVersion", func() {
55+
var (
56+
csv1, csv2 operatorsv1alpha1.ClusterServiceVersion
57+
sa corev1.ServiceAccount
58+
plan operatorsv1alpha1.InstallPlan
59+
)
60+
61+
BeforeEach(func() {
62+
csv1 = newCSV("test-csv-old", testNamespace, "", semver.Version{}, nil, nil, nil)
63+
Expect(ctx.Ctx().Client().Create(context.TODO(), &csv1)).To(Succeed())
64+
csv2 = newCSV("test-csv-new", testNamespace, "", semver.Version{}, nil, nil, nil)
65+
Expect(ctx.Ctx().Client().Create(context.TODO(), &csv2)).To(Succeed())
66+
67+
sa = corev1.ServiceAccount{
68+
ObjectMeta: metav1.ObjectMeta{
69+
Namespace: testNamespace,
70+
Name: "test-serviceaccount",
71+
OwnerReferences: []metav1.OwnerReference{
72+
{
73+
APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
74+
Kind: operatorsv1alpha1.ClusterServiceVersionKind,
75+
Name: csv1.GetName(),
76+
UID: csv1.GetUID(),
77+
},
78+
},
79+
},
80+
}
81+
Expect(ctx.Ctx().Client().Create(context.TODO(), &sa)).To(Succeed())
82+
83+
scheme := runtime.NewScheme()
84+
Expect(corev1.AddToScheme(scheme)).To(Succeed())
85+
var manifest bytes.Buffer
86+
Expect(k8sjson.NewSerializer(k8sjson.DefaultMetaFactory, scheme, scheme, false).Encode(&corev1.ServiceAccount{
87+
ObjectMeta: metav1.ObjectMeta{
88+
Namespace: testNamespace,
89+
Name: "test-serviceaccount",
90+
OwnerReferences: []metav1.OwnerReference{
91+
{
92+
APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
93+
Kind: operatorsv1alpha1.ClusterServiceVersionKind,
94+
Name: csv2.GetName(),
95+
},
96+
},
97+
},
98+
}, &manifest)).To(Succeed())
99+
100+
plan = operatorsv1alpha1.InstallPlan{
101+
ObjectMeta: metav1.ObjectMeta{
102+
Namespace: testNamespace,
103+
Name: "test-plan",
104+
},
105+
Spec: operatorsv1alpha1.InstallPlanSpec{
106+
Approval: operatorsv1alpha1.ApprovalAutomatic,
107+
Approved: true,
108+
ClusterServiceVersionNames: []string{},
109+
},
110+
Status: operatorsv1alpha1.InstallPlanStatus{
111+
Phase: operatorsv1alpha1.InstallPlanPhaseInstalling,
112+
CatalogSources: []string{},
113+
Plan: []*operatorsv1alpha1.Step{
114+
{
115+
Status: operatorsv1alpha1.StepStatusUnknown,
116+
Resource: operatorsv1alpha1.StepResource{
117+
Name: sa.GetName(),
118+
Version: "v1",
119+
Kind: "ServiceAccount",
120+
Manifest: manifest.String(),
121+
},
122+
},
123+
},
124+
},
125+
}
126+
Expect(ctx.Ctx().Client().Create(context.Background(), &plan)).To(Succeed())
127+
Expect(ctx.Ctx().Client().Status().Update(context.Background(), &plan)).To(Succeed())
128+
})
129+
130+
AfterEach(func() {
131+
Expect(ctx.Ctx().Client().Delete(context.TODO(), &sa)).To(Or(
132+
Succeed(),
133+
WithTransform(k8serrors.IsNotFound, BeTrue()),
134+
))
135+
})
136+
137+
It("preserves owner references to both the old and the new ClusterServiceVersion", func() {
138+
Eventually(func() ([]metav1.OwnerReference, error) {
139+
if err := ctx.Ctx().Client().Get(context.TODO(), client.ObjectKeyFromObject(&sa), &sa); err != nil {
140+
return nil, err
141+
}
142+
return sa.GetOwnerReferences(), nil
143+
}).Should(ContainElements([]metav1.OwnerReference{
144+
{
145+
APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
146+
Kind: operatorsv1alpha1.ClusterServiceVersionKind,
147+
Name: csv1.GetName(),
148+
UID: csv1.GetUID(),
149+
},
150+
{
151+
APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
152+
Kind: operatorsv1alpha1.ClusterServiceVersionKind,
153+
Name: csv2.GetName(),
154+
UID: csv2.GetUID(),
155+
},
156+
}))
157+
})
158+
})
159+
53160
When("a ClusterIP service exists", func() {
54161
var (
55162
service *corev1.Service

0 commit comments

Comments
 (0)