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 1955112: Preserve existing ServiceAccount owner references during installs. #2128

Merged
merged 1 commit into from
May 15, 2021
Merged

[release-4.6] Bug 1955112: Preserve existing ServiceAccount owner references during installs. #2128

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
18 changes: 18 additions & 0 deletions pkg/controller/operators/catalog/step_ensurer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ func (o *StepEnsurer) EnsureServiceAccount(namespace string, sa *corev1.ServiceA
return
}
sa.Secrets = preSa.Secrets
sa.OwnerReferences = mergedOwnerReferences(preSa.OwnerReferences, sa.OwnerReferences)

sa.SetNamespace(namespace)

Expand Down Expand Up @@ -349,3 +350,20 @@ func (o *StepEnsurer) EnsureConfigMap(namespace string, configmap *corev1.Config
status = v1alpha1.StepStatusPresent
return
}

func mergedOwnerReferences(in ...[]metav1.OwnerReference) []metav1.OwnerReference {
uniques := make(map[metav1.OwnerReference]struct{})
for _, refs := range in {
for _, ref := range refs {
uniques[ref] = struct{}{}
}
}
if len(uniques) == 0 {
return nil
}
merged := make([]metav1.OwnerReference, 0, len(uniques))
for ref := range uniques {
merged = append(merged, ref)
}
return merged
}
190 changes: 190 additions & 0 deletions pkg/controller/operators/catalog/step_ensurer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
package catalog

import (
"testing"

"github.com/stretchr/testify/assert"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func TestMergedOwnerReferences(t *testing.T) {
var (
True bool = true
False bool = false
)

for _, tc := range []struct {
Name string
In [][]metav1.OwnerReference
Out []metav1.OwnerReference
}{
{
Name: "empty",
},
{
Name: "different uid",
In: [][]metav1.OwnerReference{
{
{
APIVersion: "a",
Kind: "b",
Name: "c",
Controller: &True,
BlockOwnerDeletion: &True,
UID: "x",
},
{
APIVersion: "a",
Kind: "b",
Name: "c",
Controller: &True,
BlockOwnerDeletion: &True,
UID: "y",
},
},
},
Out: []metav1.OwnerReference{
{
APIVersion: "a",
Kind: "b",
Name: "c",
Controller: &True,
BlockOwnerDeletion: &True,
UID: "x",
},
{
APIVersion: "a",
Kind: "b",
Name: "c",
Controller: &True,
BlockOwnerDeletion: &True,
UID: "y",
},
},
},
{
Name: "different controller",
In: [][]metav1.OwnerReference{
{
{
APIVersion: "a",
Kind: "b",
Name: "c",
Controller: &True,
BlockOwnerDeletion: &True,
UID: "x",
},
{
APIVersion: "a",
Kind: "b",
Name: "c",
Controller: &False,
BlockOwnerDeletion: &True,
UID: "x",
},
},
},
Out: []metav1.OwnerReference{
{
APIVersion: "a",
Kind: "b",
Name: "c",
Controller: &True,
BlockOwnerDeletion: &True,
UID: "x",
},
{
APIVersion: "a",
Kind: "b",
Name: "c",
Controller: &False,
BlockOwnerDeletion: &True,
UID: "x",
},
},
},
{
Name: "add owner without uid",
In: [][]metav1.OwnerReference{
{
{
APIVersion: "a",
Kind: "b",
Name: "c-1",
Controller: &False,
BlockOwnerDeletion: &False,
UID: "x",
},
},
{
{
APIVersion: "a",
Kind: "b",
Name: "c-2",
Controller: &False,
BlockOwnerDeletion: &False,
UID: "",
},
},
},
Out: []metav1.OwnerReference{
{
APIVersion: "a",
Kind: "b",
Name: "c-1",
Controller: &False,
BlockOwnerDeletion: &False,
UID: "x",
},
{
APIVersion: "a",
Kind: "b",
Name: "c-2",
Controller: &False,
BlockOwnerDeletion: &False,
UID: "",
},
},
},
{
Name: "duplicates combined",
In: [][]metav1.OwnerReference{
{
{
APIVersion: "a",
Kind: "b",
Name: "c",
Controller: &False,
BlockOwnerDeletion: &False,
UID: "x",
},
},
{
{
APIVersion: "a",
Kind: "b",
Name: "c",
Controller: &False,
BlockOwnerDeletion: &False,
UID: "x",
},
},
},
Out: []metav1.OwnerReference{
{
APIVersion: "a",
Kind: "b",
Name: "c",
Controller: &False,
BlockOwnerDeletion: &False,
UID: "x",
},
},
},
} {
t.Run(tc.Name, func(t *testing.T) {
assert.ElementsMatch(t, tc.Out, mergedOwnerReferences(tc.In...))
})
}
}
1 change: 1 addition & 0 deletions pkg/controller/operators/olm/operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1486,6 +1486,7 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
// Check if we're not ready to install part of the replacement chain yet
if prev := a.isReplacing(out); prev != nil {
if prev.Status.Phase != v1alpha1.CSVPhaseReplacing {
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")
return
}
}
Expand Down
111 changes: 111 additions & 0 deletions test/e2e/installplan_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/apimachinery/pkg/watch"
"k8s.io/client-go/util/retry"
"sigs.k8s.io/controller-runtime/pkg/client"
runtimeclient "sigs.k8s.io/controller-runtime/pkg/client"

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

When("an InstallPlan transfers ownership of a ServiceAccount to a new ClusterServiceVersion", func() {
var (
csv1, csv2 operatorsv1alpha1.ClusterServiceVersion
sa corev1.ServiceAccount
plan operatorsv1alpha1.InstallPlan
)

BeforeEach(func() {
csv1 = newCSV("test-csv-old", testNamespace, "", semver.Version{}, nil, nil, nil)
Expect(ctx.Ctx().Client().Create(context.TODO(), &csv1)).To(Succeed())
csv2 = newCSV("test-csv-new", testNamespace, "", semver.Version{}, nil, nil, nil)
Expect(ctx.Ctx().Client().Create(context.TODO(), &csv2)).To(Succeed())

sa = corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: "test-serviceaccount",
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
Kind: operatorsv1alpha1.ClusterServiceVersionKind,
Name: csv1.GetName(),
UID: csv1.GetUID(),
},
},
},
}
Expect(ctx.Ctx().Client().Create(context.TODO(), &sa)).To(Succeed())

scheme := runtime.NewScheme()
Expect(corev1.AddToScheme(scheme)).To(Succeed())
var manifest bytes.Buffer
Expect(k8sjson.NewSerializer(k8sjson.DefaultMetaFactory, scheme, scheme, false).Encode(&corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: "test-serviceaccount",
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
Kind: operatorsv1alpha1.ClusterServiceVersionKind,
Name: csv2.GetName(),
},
},
},
}, &manifest)).To(Succeed())

plan = operatorsv1alpha1.InstallPlan{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: "test-plan",
},
Spec: operatorsv1alpha1.InstallPlanSpec{
Approval: operatorsv1alpha1.ApprovalAutomatic,
Approved: true,
ClusterServiceVersionNames: []string{},
},
Status: operatorsv1alpha1.InstallPlanStatus{
Phase: operatorsv1alpha1.InstallPlanPhaseInstalling,
CatalogSources: []string{},
Plan: []*operatorsv1alpha1.Step{
{
Status: operatorsv1alpha1.StepStatusUnknown,
Resource: operatorsv1alpha1.StepResource{
Name: sa.GetName(),
Version: "v1",
Kind: "ServiceAccount",
Manifest: manifest.String(),
},
},
},
},
}
Expect(ctx.Ctx().Client().Create(context.Background(), &plan)).To(Succeed())
Expect(ctx.Ctx().Client().Status().Update(context.Background(), &plan)).To(Succeed())
})

AfterEach(func() {
Expect(ctx.Ctx().Client().Delete(context.TODO(), &sa)).To(Or(
Succeed(),
WithTransform(k8serrors.IsNotFound, BeTrue()),
))
})

It("preserves owner references to both the old and the new ClusterServiceVersion", func() {
Eventually(func() ([]metav1.OwnerReference, error) {
key, err := client.ObjectKeyFromObject(&sa)
if err != nil{
return nil, err
}
if err := ctx.Ctx().Client().Get(context.TODO(), key, &sa); err != nil {
return nil, err
}
return sa.GetOwnerReferences(), nil
}).Should(ContainElements([]metav1.OwnerReference{
{
APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
Kind: operatorsv1alpha1.ClusterServiceVersionKind,
Name: csv1.GetName(),
UID: csv1.GetUID(),
},
{
APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
Kind: operatorsv1alpha1.ClusterServiceVersionKind,
Name: csv2.GetName(),
UID: csv2.GetUID(),
},
}))
})
})

When("a ClusterIP service exists", func() {
var (
service *corev1.Service
Expand Down