Skip to content

Commit 8ccfdb9

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #2029 from benluddy/sa-present-when-owned-by-non-csv
Bug 1935909: Allow non-CSV-owned ServiceAccounts to satisfy CSV requirements.
2 parents 3787874 + b375c4e commit 8ccfdb9

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)