Skip to content

Commit

Permalink
Allow non-CSV-owned ServiceAccounts to satisfy CSV requirements.
Browse files Browse the repository at this point in the history 
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 <bluddy@redhat.com>
  • Loading branch information
@benluddy
benluddy committed Mar 18, 2021
1 parent 2cd064d commit 9ed5cd0
Show file tree
Hide file tree
Showing 3 changed files with 287 additions and 14 deletions.
6 changes: 3 additions & 3 deletions pkg/controller/operators/olm/requirements.go
View file
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
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
107 changes: 107 additions & 0 deletions test/e2e/csv_e2e_test.go
View file
Expand Up @@ -126,6 +126,113 @@ 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) {
if err := ctx.Ctx().Client().Get(context.Background(), client.ObjectKeyFromObject(&csv), &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

0 comments on commit 9ed5cd0

Please sign in to comment.