Skip to content

Commit

Permalink
Merge pull request #1160 from openshift-cherrypick-robot/cherry-pick-…
Browse files Browse the repository at this point in the history 
…1123-to-release-4.3

[release-4.3] Bug 1776521: Replacing operator creates duplicate secrets
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot authored Dec 3, 2019
2 parents ba10413 + 928a2fe commit b6b7c85
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 7 deletions.
111 changes: 111 additions & 0 deletions pkg/controller/operators/catalog/operator_test.go
View file
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
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)

// 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
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

0 comments on commit b6b7c85

Please sign in to comment.