Skip to content

Commit b375c4e

Browse files
benluddybenluddy
committed
Allow non-CSV-owned ServiceAccounts to satisfy CSV requirements.
The existing check, intended to prevent a race condition that could occur during CSV upgrades, marks a ServiceAccount as NotPresent when it has at least one owner but is not owned by the current CSV. It's expected for the ServiceAccount "default" not to be have an ownerreference to a CSV, so the check can be relaxed to only consider owners of kind ClusterServiceVersion. This also combines two ServiceAccount checks that were independently added to address the same race condition into a single check. Signed-off-by: Ben Luddy <[email protected]>
1 parent 3787874 commit b375c4e

File tree

3 files changed

+212
-42
lines changed

3 files changed

+212
-42
lines changed

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

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -271,18 +271,9 @@ func (a *Operator) permissionStatus(strategyDetailsDeployment *v1alpha1.Strategy
271271
}
272272
// Check SA's ownership
273273
if ownerutil.IsOwnedByKind(sa, v1alpha1.ClusterServiceVersionKind) && !ownerutil.IsOwnedBy(sa, csv) {
274-
met = false
275-
status.Status = v1alpha1.RequirementStatusReasonNotPresent
276-
status.Message = "Service account is stale"
277-
statusesSet[saName] = status
278-
continue
279-
}
280-
281-
// Check if the ServiceAccount is owned by CSV
282-
if len(sa.GetOwnerReferences()) != 0 && !ownerutil.IsOwnedBy(sa, csv) {
283274
met = false
284275
status.Status = v1alpha1.RequirementStatusReasonPresentNotSatisfied
285-
status.Message = "Service account is not owned by this ClusterServiceVersion"
276+
status.Message = "Service account is owned by another ClusterServiceVersion"
286277
statusesSet[saName] = status
287278
continue
288279
}

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

Lines changed: 104 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"k8s.io/apimachinery/pkg/types"
1515

1616
"github.com/operator-framework/api/pkg/operators/v1alpha1"
17+
"github.com/stretchr/testify/assert"
1718
)
1819

1920
func TestRequirementAndPermissionStatus(t *testing.T) {
@@ -297,7 +298,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
297298
Version: "v1",
298299
Kind: "ServiceAccount",
299300
Name: "sa",
300-
Status: v1alpha1.RequirementStatusReasonPresent,
301+
Status: v1alpha1.RequirementStatusReasonPresentNotSatisfied,
301302
Dependents: []v1alpha1.DependentStatus{
302303
{
303304
Group: "rbac.authorization.k8s.io",
@@ -641,7 +642,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
641642
Version: "v1",
642643
Kind: "CustomResourceDefinition",
643644
Name: "c1.g1",
644-
Status: v1alpha1.RequirementStatusReasonNotAvailable,
645+
Status: v1alpha1.RequirementStatusReasonNotPresent,
645646
},
646647
{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv1"}: {
647648
Group: "operators.coreos.com",
@@ -773,55 +774,124 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
773774
},
774775
},
775776
},
776-
&rbacv1.Role{
777-
ObjectMeta: metav1.ObjectMeta{
778-
Name: "role",
779-
Namespace: namespace,
780-
},
781-
Rules: []rbacv1.PolicyRule{
777+
},
778+
existingExtObjs: nil,
779+
met: false,
780+
expectedRequirementStatuses: map[gvkn]v1alpha1.RequirementStatus{
781+
{"", "v1", "ServiceAccount", "sa"}: {
782+
Version: "v1",
783+
Kind: "ServiceAccount",
784+
Name: "sa",
785+
Status: v1alpha1.RequirementStatusReasonPresentNotSatisfied,
786+
Dependents: []v1alpha1.DependentStatus{},
787+
},
788+
{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv1"}: {
789+
Group: "operators.coreos.com",
790+
Version: "v1alpha1",
791+
Kind: "ClusterServiceVersion",
792+
Name: "csv1",
793+
Status: v1alpha1.RequirementStatusReasonPresent,
794+
},
795+
},
796+
expectedError: nil,
797+
},
798+
{
799+
description: "RequirementMet/ServiceAccountOwnedByNonCSV",
800+
csv: csvWithUID(csv("csv",
801+
namespace,
802+
"0.0.0",
803+
"",
804+
installStrategy(
805+
"csv-dep",
806+
[]v1alpha1.StrategyDeploymentPermissions{
782807
{
783-
APIGroups: []string{""},
784-
Verbs: []string{"*"},
785-
Resources: []string{"donuts"},
808+
ServiceAccountName: "sa",
786809
},
787810
},
788-
},
789-
&rbacv1.RoleBinding{
811+
nil,
812+
),
813+
nil,
814+
nil,
815+
v1alpha1.CSVPhasePending,
816+
), types.UID("csv-uid")),
817+
existingObjs: []runtime.Object{
818+
&corev1.ServiceAccount{
790819
ObjectMeta: metav1.ObjectMeta{
791-
Name: "roleBinding",
820+
Name: "sa",
792821
Namespace: namespace,
822+
UID: types.UID("sa"),
823+
OwnerReferences: []metav1.OwnerReference{
824+
{
825+
Kind: v1alpha1.SubscriptionKind, // arbitrary non-CSV kind
826+
UID: "non-csv",
827+
},
828+
},
793829
},
794-
Subjects: []rbacv1.Subject{
830+
},
831+
},
832+
existingExtObjs: nil,
833+
met: true,
834+
expectedRequirementStatuses: map[gvkn]v1alpha1.RequirementStatus{
835+
{"", "v1", "ServiceAccount", "sa"}: {
836+
Version: "v1",
837+
Kind: "ServiceAccount",
838+
Name: "sa",
839+
Status: v1alpha1.RequirementStatusReasonPresent,
840+
Dependents: []v1alpha1.DependentStatus{},
841+
},
842+
{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv"}: {
843+
Group: "operators.coreos.com",
844+
Version: "v1alpha1",
845+
Kind: "ClusterServiceVersion",
846+
Name: "csv",
847+
Status: v1alpha1.RequirementStatusReasonPresent,
848+
},
849+
},
850+
expectedError: nil,
851+
},
852+
{
853+
description: "RequirementMet/ServiceAccountHasNoOwner",
854+
csv: csvWithUID(csv("csv",
855+
namespace,
856+
"0.0.0",
857+
"",
858+
installStrategy(
859+
"csv-dep",
860+
[]v1alpha1.StrategyDeploymentPermissions{
795861
{
796-
Kind: "ServiceAccount",
797-
APIGroup: "",
798-
Name: "sa",
799-
Namespace: namespace,
862+
ServiceAccountName: "sa",
800863
},
801864
},
802-
RoleRef: rbacv1.RoleRef{
803-
APIGroup: "rbac.authorization.k8s.io",
804-
Kind: "Role",
805-
Name: "role",
865+
nil,
866+
),
867+
nil,
868+
nil,
869+
v1alpha1.CSVPhasePending,
870+
), types.UID("csv-uid")),
871+
existingObjs: []runtime.Object{
872+
&corev1.ServiceAccount{
873+
ObjectMeta: metav1.ObjectMeta{
874+
Name: "sa",
875+
Namespace: namespace,
876+
UID: types.UID("sa"),
806877
},
807878
},
808879
},
809880
existingExtObjs: nil,
810-
met: false,
881+
met: true,
811882
expectedRequirementStatuses: map[gvkn]v1alpha1.RequirementStatus{
812883
{"", "v1", "ServiceAccount", "sa"}: {
813-
Group: "",
814884
Version: "v1",
815885
Kind: "ServiceAccount",
816886
Name: "sa",
817-
Status: v1alpha1.RequirementStatusReasonNotPresent,
887+
Status: v1alpha1.RequirementStatusReasonPresent,
818888
Dependents: []v1alpha1.DependentStatus{},
819889
},
820-
{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv1"}: {
890+
{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv"}: {
821891
Group: "operators.coreos.com",
822892
Version: "v1alpha1",
823893
Kind: "ClusterServiceVersion",
824-
Name: "csv1",
894+
Name: "csv",
825895
Status: v1alpha1.RequirementStatusReasonPresent,
826896
},
827897
},
@@ -843,7 +913,8 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
843913
require.Error(t, err)
844914
require.EqualError(t, test.expectedError, err.Error())
845915
}
846-
require.Equal(t, test.met, met)
916+
assert := assert.New(t)
917+
assert.Equal(test.met, met)
847918

848919
for _, status := range statuses {
849920
key := gvkn{
@@ -854,14 +925,15 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
854925
}
855926

856927
expected, ok := test.expectedRequirementStatuses[key]
857-
require.True(t, ok, fmt.Sprintf("permission requirement status %+v found but not expected", key))
858-
require.Len(t, status.Dependents, len(expected.Dependents), "number of dependents is not what was expected")
928+
assert.True(ok, fmt.Sprintf("permission requirement status %+v found but not expected", key))
929+
assert.Equal(expected.Status, status.Status)
930+
assert.Len(status.Dependents, len(expected.Dependents), "number of dependents is not what was expected")
859931

860932
// Delete the requirement status to mark as found
861933
delete(test.expectedRequirementStatuses, key)
862934
}
863935

864-
require.Len(t, test.expectedRequirementStatuses, 0, "not all expected permission requirement statuses were found")
936+
assert.Len(test.expectedRequirementStatuses, 0, "not all expected permission requirement statuses were found")
865937
})
866938
}
867939
}

‎test/e2e/csv_e2e_test.go

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,113 @@ var _ = Describe("ClusterServiceVersion", func() {
127127
})
128128
})
129129

130+
When("a csv requires a serviceaccount solely owned by a non-csv", func() {
131+
var (
132+
cm corev1.ConfigMap
133+
sa corev1.ServiceAccount
134+
csv v1alpha1.ClusterServiceVersion
135+
)
136+
137+
BeforeEach(func() {
138+
cm = corev1.ConfigMap{
139+
ObjectMeta: metav1.ObjectMeta{
140+
GenerateName: "cm-",
141+
Namespace: testNamespace,
142+
},
143+
}
144+
Expect(ctx.Ctx().Client().Create(context.Background(), &cm)).To(Succeed())
145+
146+
sa = corev1.ServiceAccount{
147+
ObjectMeta: metav1.ObjectMeta{
148+
GenerateName: "sa-",
149+
Namespace: testNamespace,
150+
OwnerReferences: []metav1.OwnerReference{
151+
{
152+
Name: cm.GetName(),
153+
APIVersion: corev1.SchemeGroupVersion.String(),
154+
Kind: "ConfigMap",
155+
UID: cm.GetUID(),
156+
},
157+
},
158+
},
159+
}
160+
Expect(ctx.Ctx().Client().Create(context.Background(), &sa)).To(Succeed())
161+
162+
csv = v1alpha1.ClusterServiceVersion{
163+
TypeMeta: metav1.TypeMeta{
164+
Kind: v1alpha1.ClusterServiceVersionKind,
165+
APIVersion: v1alpha1.ClusterServiceVersionAPIVersion,
166+
},
167+
ObjectMeta: metav1.ObjectMeta{
168+
GenerateName: "csv-",
169+
Namespace: testNamespace,
170+
},
171+
Spec: v1alpha1.ClusterServiceVersionSpec{
172+
InstallStrategy: v1alpha1.NamedInstallStrategy{
173+
StrategyName: v1alpha1.InstallStrategyNameDeployment,
174+
StrategySpec: v1alpha1.StrategyDetailsDeployment{
175+
DeploymentSpecs: []v1alpha1.StrategyDeploymentSpec{
176+
{
177+
Name: "foo",
178+
Spec: appsv1.DeploymentSpec{
179+
Selector: &metav1.LabelSelector{
180+
MatchLabels: map[string]string{"app": "foo"},
181+
},
182+
Template: corev1.PodTemplateSpec{
183+
ObjectMeta: metav1.ObjectMeta{
184+
Labels: map[string]string{"app": "foo"},
185+
},
186+
Spec: corev1.PodSpec{Containers: []corev1.Container{
187+
{
188+
Name: genName("foo"),
189+
Image: *dummyImage,
190+
},
191+
}},
192+
},
193+
},
194+
},
195+
},
196+
Permissions: []v1alpha1.StrategyDeploymentPermissions{
197+
{
198+
ServiceAccountName: sa.GetName(),
199+
Rules: []rbacv1.PolicyRule{},
200+
},
201+
},
202+
},
203+
},
204+
InstallModes: []v1alpha1.InstallMode{
205+
{
206+
Type: v1alpha1.InstallModeTypeAllNamespaces,
207+
Supported: true,
208+
},
209+
},
210+
},
211+
}
212+
Expect(ctx.Ctx().Client().Create(context.Background(), &csv)).To(Succeed())
213+
})
214+
215+
AfterEach(func() {
216+
if cm.GetName() != "" {
217+
Expect(ctx.Ctx().Client().Delete(context.Background(), &cm)).To(Succeed())
218+
}
219+
})
220+
221+
It("considers the serviceaccount requirement satisfied", func() {
222+
Eventually(func() (v1alpha1.StatusReason, error) {
223+
if err := ctx.Ctx().Client().Get(context.Background(), client.ObjectKeyFromObject(&csv), &csv); err != nil {
224+
return "", err
225+
}
226+
for _, requirement := range csv.Status.RequirementStatus {
227+
if requirement.Name != sa.GetName() {
228+
continue
229+
}
230+
return requirement.Status, nil
231+
}
232+
return "", fmt.Errorf("missing expected requirement %q", sa.GetName())
233+
}).Should(Equal(v1alpha1.RequirementStatusReasonPresent))
234+
})
235+
})
236+
130237
It("create with unmet requirements min kube version", func() {
131238

132239
depName := genName("dep-")

0 commit comments

Comments
 (0)