Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[release-4.6] Bug 1940649: Allow non-CSV-owned ServiceAccounts to satisfy CSV requirements. #2051

Merged
merged 1 commit into from Mar 25, 2021
Merged

[release-4.6] Bug 1940649: Allow non-CSV-owned ServiceAccounts to satisfy CSV requirements. #2051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkg/controller/operators/olm/requirements.go
View file
Open in desktop
Expand Up @@ -270,11 +270,11 @@ func (a *Operator) permissionStatus(strategyDetailsDeployment *v1alpha1.Strategy
continue
}

// Check if the ServiceAccount is owned by CSV
if len(sa.GetOwnerReferences()) != 0 && !ownerutil.IsOwnedBy(sa, csv) {
// Check SA's ownership
if ownerutil.IsOwnedByKind(sa, v1alpha1.ClusterServiceVersionKind) && !ownerutil.IsOwnedBy(sa, csv) {
met = false
status.Status = v1alpha1.RequirementStatusReasonPresentNotSatisfied
status.Message = "Service account is not owned by this ClusterServiceVersion"
status.Message = "Service account is owned by another ClusterServiceVersion"
statusesSet[saName] = status
continue
}
Expand Down
188 changes: 177 additions & 11 deletions pkg/controller/operators/olm/requirements_test.go
View file
Open in desktop
Expand Up @@ -14,6 +14,7 @@ import (
"k8s.io/apimachinery/pkg/types"

"github.com/operator-framework/api/pkg/operators/v1alpha1"
"github.com/stretchr/testify/assert"
)

func TestRequirementAndPermissionStatus(t *testing.T) {
Expand Down Expand Up @@ -297,7 +298,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
Version: "v1",
Kind: "ServiceAccount",
Name: "sa",
Status: v1alpha1.RequirementStatusReasonPresent,
Status: v1alpha1.RequirementStatusReasonPresentNotSatisfied,
Dependents: []v1alpha1.DependentStatus{
{
Group: "rbac.authorization.k8s.io",
Expand Down Expand Up @@ -437,11 +438,11 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
met: false,
expectedRequirementStatuses: map[gvkn]v1alpha1.RequirementStatus{
{"", "v1", "ServiceAccount", "sa"}: {
Group: "",
Version: "v1",
Kind: "ServiceAccount",
Name: "sa",
Status: v1alpha1.RequirementStatusReasonPresentNotSatisfied,
Group: "",
Version: "v1",
Kind: "ServiceAccount",
Name: "sa",
Status: v1alpha1.RequirementStatusReasonPresentNotSatisfied,
Dependents: []v1alpha1.DependentStatus{},
},
{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv1"}: {
Expand Down Expand Up @@ -641,7 +642,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
Version: "v1",
Kind: "CustomResourceDefinition",
Name: "c1.g1",
Status: v1alpha1.RequirementStatusReasonNotAvailable,
Status: v1alpha1.RequirementStatusReasonNotPresent,
},
{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv1"}: {
Group: "operators.coreos.com",
Expand Down Expand Up @@ -733,6 +734,169 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
},
expectedError: nil,
},
{
description: "RequirementNotMet/StaleServiceAccount",
csv: csvWithUID(csv("csv1",
namespace,
"0.0.0",
"",
installStrategy(
"csv1-dep",
[]v1alpha1.StrategyDeploymentPermissions{
{
ServiceAccountName: "sa",
Rules: []rbacv1.PolicyRule{
{
APIGroups: []string{""},
Verbs: []string{"*"},
Resources: []string{"donuts"},
},
},
},
},
nil,
),
nil,
nil,
v1alpha1.CSVPhasePending,
), types.UID("csv-uid")),
existingObjs: []runtime.Object{
&corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: "sa",
Namespace: namespace,
UID: types.UID("sa"),
OwnerReferences: []metav1.OwnerReference{
{
Kind: v1alpha1.ClusterServiceVersionKind,
UID: "csv-wrong",
},
},
},
},
},
existingExtObjs: nil,
met: false,
expectedRequirementStatuses: map[gvkn]v1alpha1.RequirementStatus{
{"", "v1", "ServiceAccount", "sa"}: {
Version: "v1",
Kind: "ServiceAccount",
Name: "sa",
Status: v1alpha1.RequirementStatusReasonPresentNotSatisfied,
Dependents: []v1alpha1.DependentStatus{},
},
{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv1"}: {
Group: "operators.coreos.com",
Version: "v1alpha1",
Kind: "ClusterServiceVersion",
Name: "csv1",
Status: v1alpha1.RequirementStatusReasonPresent,
},
},
expectedError: nil,
},
{
description: "RequirementMet/ServiceAccountOwnedByNonCSV",
csv: csvWithUID(csv("csv",
namespace,
"0.0.0",
"",
installStrategy(
"csv-dep",
[]v1alpha1.StrategyDeploymentPermissions{
{
ServiceAccountName: "sa",
},
},
nil,
),
nil,
nil,
v1alpha1.CSVPhasePending,
), types.UID("csv-uid")),
existingObjs: []runtime.Object{
&corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: "sa",
Namespace: namespace,
UID: types.UID("sa"),
OwnerReferences: []metav1.OwnerReference{
{
Kind: v1alpha1.SubscriptionKind, // arbitrary non-CSV kind
UID: "non-csv",
},
},
},
},
},
existingExtObjs: nil,
met: true,
expectedRequirementStatuses: map[gvkn]v1alpha1.RequirementStatus{
{"", "v1", "ServiceAccount", "sa"}: {
Version: "v1",
Kind: "ServiceAccount",
Name: "sa",
Status: v1alpha1.RequirementStatusReasonPresent,
Dependents: []v1alpha1.DependentStatus{},
},
{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv"}: {
Group: "operators.coreos.com",
Version: "v1alpha1",
Kind: "ClusterServiceVersion",
Name: "csv",
Status: v1alpha1.RequirementStatusReasonPresent,
},
},
expectedError: nil,
},
{
description: "RequirementMet/ServiceAccountHasNoOwner",
csv: csvWithUID(csv("csv",
namespace,
"0.0.0",
"",
installStrategy(
"csv-dep",
[]v1alpha1.StrategyDeploymentPermissions{
{
ServiceAccountName: "sa",
},
},
nil,
),
nil,
nil,
v1alpha1.CSVPhasePending,
), types.UID("csv-uid")),
existingObjs: []runtime.Object{
&corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: "sa",
Namespace: namespace,
UID: types.UID("sa"),
},
},
},
existingExtObjs: nil,
met: true,
expectedRequirementStatuses: map[gvkn]v1alpha1.RequirementStatus{
{"", "v1", "ServiceAccount", "sa"}: {
Version: "v1",
Kind: "ServiceAccount",
Name: "sa",
Status: v1alpha1.RequirementStatusReasonPresent,
Dependents: []v1alpha1.DependentStatus{},
},
{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv"}: {
Group: "operators.coreos.com",
Version: "v1alpha1",
Kind: "ClusterServiceVersion",
Name: "csv",
Status: v1alpha1.RequirementStatusReasonPresent,
},
},
expectedError: nil,
},
}

for _, test := range tests {
Expand All @@ -749,7 +913,8 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
require.Error(t, err)
require.EqualError(t, test.expectedError, err.Error())
}
require.Equal(t, test.met, met)
assert := assert.New(t)
assert.Equal(test.met, met)

for _, status := range statuses {
key := gvkn{
Expand All @@ -760,14 +925,15 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
}

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

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

require.Len(t, test.expectedRequirementStatuses, 0, "not all expected permission requirement statuses were found")
assert.Len(test.expectedRequirementStatuses, 0, "not all expected permission requirement statuses were found")
})
}
}
Expand Down
111 changes: 111 additions & 0 deletions test/e2e/csv_e2e_test.go
View file
Open in desktop
Expand Up @@ -126,6 +126,117 @@ var _ = Describe("CSV", func() {
})
})

When("a csv requires a serviceaccount solely owned by a non-csv", func() {
var (
cm corev1.ConfigMap
sa corev1.ServiceAccount
csv v1alpha1.ClusterServiceVersion
)

BeforeEach(func() {
cm = corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
GenerateName: "cm-",
Namespace: testNamespace,
},
}
Expect(ctx.Ctx().Client().Create(context.Background(), &cm)).To(Succeed())

sa = corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
GenerateName: "sa-",
Namespace: testNamespace,
OwnerReferences: []metav1.OwnerReference{
{
Name: cm.GetName(),
APIVersion: corev1.SchemeGroupVersion.String(),
Kind: "ConfigMap",
UID: cm.GetUID(),
},
},
},
}
Expect(ctx.Ctx().Client().Create(context.Background(), &sa)).To(Succeed())

csv = v1alpha1.ClusterServiceVersion{
TypeMeta: metav1.TypeMeta{
Kind: v1alpha1.ClusterServiceVersionKind,
APIVersion: v1alpha1.ClusterServiceVersionAPIVersion,
},
ObjectMeta: metav1.ObjectMeta{
GenerateName: "csv-",
Namespace: testNamespace,
},
Spec: v1alpha1.ClusterServiceVersionSpec{
InstallStrategy: v1alpha1.NamedInstallStrategy{
StrategyName: v1alpha1.InstallStrategyNameDeployment,
StrategySpec: v1alpha1.StrategyDetailsDeployment{
DeploymentSpecs: []v1alpha1.StrategyDeploymentSpec{
{
Name: "foo",
Spec: appsv1.DeploymentSpec{
Selector: &metav1.LabelSelector{
MatchLabels: map[string]string{"app": "foo"},
},
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{"app": "foo"},
},
Spec: corev1.PodSpec{Containers: []corev1.Container{
{
Name: genName("foo"),
Image: *dummyImage,
},
}},
},
},
},
},
Permissions: []v1alpha1.StrategyDeploymentPermissions{
{
ServiceAccountName: sa.GetName(),
Rules: []rbacv1.PolicyRule{},
},
},
},
},
InstallModes: []v1alpha1.InstallMode{
{
Type: v1alpha1.InstallModeTypeAllNamespaces,
Supported: true,
},
},
},
}
Expect(ctx.Ctx().Client().Create(context.Background(), &csv)).To(Succeed())
})

AfterEach(func() {
if cm.GetName() != "" {
Expect(ctx.Ctx().Client().Delete(context.Background(), &cm)).To(Succeed())
}
})

It("considers the serviceaccount requirement satisfied", func() {
Eventually(func() (v1alpha1.StatusReason, error) {
key, err := client.ObjectKeyFromObject(&csv)
if err != nil {
return "", err
}
if err := ctx.Ctx().Client().Get(context.Background(), key, &csv); err != nil {
return "", err
}
for _, requirement := range csv.Status.RequirementStatus {
if requirement.Name != sa.GetName() {
continue
}
return requirement.Status, nil
}
return "", fmt.Errorf("missing expected requirement %q", sa.GetName())
}).Should(Equal(v1alpha1.RequirementStatusReasonPresent))
})
})

It("create with unmet requirements min kube version", func() {
c := newKubeClient()
crc := newCRClient()
Expand Down