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

Bug 1769030: Replacing operator creates duplicate secrets #1123

Merged
merged 1 commit into from
Dec 2, 2019
Merged

Bug 1769030: Replacing operator creates duplicate secrets #1123

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 111 additions & 0 deletions pkg/controller/operators/catalog/operator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,96 @@ func TestExecutePlan(t *testing.T) {
want: []runtime.Object{service("service", namespace), csv("csv", namespace, nil, nil)},
err: nil,
},
{
testName: "CreateServiceAccount",
in: withSteps(installPlan("p", namespace, v1alpha1.InstallPlanPhaseInstalling, "csv"),
[]*v1alpha1.Step{
{
Resource: v1alpha1.StepResource{
CatalogSource: "catalog",
CatalogSourceNamespace: namespace,
Group: "",
Version: "v1",
Kind: "ServiceAccount",
Name: "sa",
Manifest: toManifest(serviceAccount("sa", namespace, "",
objectReference("init secret"))),
},
Status: v1alpha1.StepStatusUnknown,
},
},
),
want: []runtime.Object{serviceAccount("sa", namespace, "", objectReference("init secret"))},
err: nil,
},
{
testName: "UpdateServiceAccountWithSameFields",
in: withSteps(installPlan("p", namespace, v1alpha1.InstallPlanPhaseInstalling, "csv"),
[]*v1alpha1.Step{
{
Resource: v1alpha1.StepResource{
CatalogSource: "catalog",
CatalogSourceNamespace: namespace,
Group: "",
Version: "v1",
Kind: "ServiceAccount",
Name: "sa",
Manifest: toManifest(serviceAccount("sa", namespace, "name",
objectReference("init secret"))),
},
Status: v1alpha1.StepStatusUnknown,
},
{
Resource: v1alpha1.StepResource{
CatalogSource: "catalog",
CatalogSourceNamespace: namespace,
Group: "",
Version: "v1",
Kind: "ServiceAccount",
Name: "sa",
Manifest: toManifest(serviceAccount("sa", namespace, "name", nil)),
},
Status: v1alpha1.StepStatusUnknown,
},
},
),
want: []runtime.Object{serviceAccount("sa", namespace, "name", objectReference("init secret"))},
err: nil,
},
{
testName: "UpdateServiceAccountWithDiffFields",
in: withSteps(installPlan("p", namespace, v1alpha1.InstallPlanPhaseInstalling, "csv"),
[]*v1alpha1.Step{
{
Resource: v1alpha1.StepResource{
CatalogSource: "catalog",
CatalogSourceNamespace: namespace,
Group: "",
Version: "v1",
Kind: "ServiceAccount",
Name: "sa",
Manifest: toManifest(serviceAccount("sa", namespace, "old_name",
objectReference("init secret"))),
},
Status: v1alpha1.StepStatusUnknown,
},
{
Resource: v1alpha1.StepResource{
CatalogSource: "catalog",
CatalogSourceNamespace: namespace,
Group: "",
Version: "v1",
Kind: "ServiceAccount",
Name: "sa",
Manifest: toManifest(serviceAccount("sa", namespace, "new_name", nil)),
},
Status: v1alpha1.StepStatusUnknown,
},
},
),
want: []runtime.Object{serviceAccount("sa", namespace, "new_name", objectReference("init secret"))},
err: nil,
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -411,6 +501,8 @@ func TestExecutePlan(t *testing.T) {
fetched, err = op.opClient.GetRoleBinding(namespace, o.GetName())
case *corev1.ServiceAccount:
fetched, err = op.opClient.GetServiceAccount(namespace, o.GetName())
case *corev1.Secret:
fetched, err = op.opClient.GetSecret(namespace, o.GetName())
case *corev1.Service:
fetched, err = op.opClient.GetService(namespace, o.GetName())
case *v1alpha1.ClusterServiceVersion:
Expand Down Expand Up @@ -1010,6 +1102,25 @@ func service(name, namespace string) *corev1.Service {
}
}

func serviceAccount(name, namespace, generateName string, secretRef *corev1.ObjectReference) *corev1.ServiceAccount {
if secretRef == nil {
return &corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace, GenerateName: generateName},
}
}
return &corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace, GenerateName: generateName},
Secrets: []corev1.ObjectReference{*secretRef},
}
}

func objectReference(name string) *corev1.ObjectReference {
if name == "" {
return &corev1.ObjectReference{}
}
return &corev1.ObjectReference{Name: name}
}

func toManifest(obj runtime.Object) string {
raw, _ := json.Marshal(obj)
return string(raw)
Expand Down
29 changes: 22 additions & 7 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 @@ -3,15 +3,17 @@ package catalog
import (
"fmt"

"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned"
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
errorwrap "github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
apiequality "k8s.io/apimachinery/pkg/api/equality"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned"
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
)

func newStepEnsurer(kubeClient operatorclient.ClientInterface, crClient versioned.Interface) *StepEnsurer {
Expand Down Expand Up @@ -116,11 +118,24 @@ func (o *StepEnsurer) EnsureServiceAccount(namespace string, sa *corev1.ServiceA
return
}

sa.SetNamespace(namespace)
if _, updateErr := o.kubeClient.UpdateServiceAccount(sa); updateErr != nil {
err = errorwrap.Wrapf(updateErr, "error updating service account: %s", sa.GetName())
// Carrying secrets through the service account update.
preSa, getErr := o.kubeClient.KubernetesInterface().CoreV1().ServiceAccounts(namespace).Get(sa.Name,
metav1.GetOptions{})
if getErr != nil {
err = errorwrap.Wrapf(getErr, "error getting older version of service account: %s", sa.GetName())
return
}
sa.Secrets = preSa.Secrets

sa.SetNamespace(namespace)

Bowenislandsong marked this conversation as resolved.
Show resolved Hide resolved
// Use DeepDerivative to check if new SA is the same as the old SA. If no field is changed, we skip the update call.
if !apiequality.Semantic.DeepDerivative(sa, preSa) {
if _, updateErr := o.kubeClient.UpdateServiceAccount(sa); updateErr != nil {
err = errorwrap.Wrapf(updateErr, "error updating service account: %s", sa.GetName())
return
}
}

status = v1alpha1.StepStatusPresent
return
Expand Down
11 changes: 11 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 @@ -9,6 +9,7 @@ import (
"time"

"github.com/blang/semver"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
appsv1 "k8s.io/api/apps/v1"
authorizationv1 "k8s.io/api/authorization/v1"
Expand Down Expand Up @@ -1610,6 +1611,9 @@ func TestUpdateCatalogForSubscription(t *testing.T) {
},
}

oldSecrets, err := c.KubernetesInterface().CoreV1().Secrets(testNamespace).List(metav1.ListOptions{})
require.NoError(t, err, "error listing secrets")

// Create the catalog sources
updatedNamedStrategy := newNginxInstallStrategy(genName("dep-"), updatedPermissions, updatedClusterPermissions)
updatedCSV := newCSV(mainPackageStable+"-next", testNamespace, mainCSV.GetName(), semver.MustParse("0.2.0"), []apiextensions.CustomResourceDefinition{mainCRD}, nil, updatedNamedStrategy)
Expand Down Expand Up @@ -1640,6 +1644,13 @@ func TestUpdateCatalogForSubscription(t *testing.T) {
_, err = awaitCSV(t, crc, testNamespace, updatedCSV.GetName(), csvSucceededChecker)
require.NoError(t, err)

newSecrets, err := c.KubernetesInterface().CoreV1().Secrets(testNamespace).List(metav1.ListOptions{})
require.NoError(t, err, "error listing secrets")
// Assert that the number of secrets is not increased from updating service account as part of the install plan,
assert.EqualValues(t, len(oldSecrets.Items), len(newSecrets.Items))
// And that the secret list is indeed updated.
assert.Equal(t, oldSecrets.Items, newSecrets.Items)

// Wait for ServiceAccount to not have access anymore
err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
res, err := c.KubernetesInterface().AuthorizationV1().SubjectAccessReviews().Create(&authorizationv1.SubjectAccessReview{
Expand Down