From b557ea1eee3972e3b927894748da76171071753d Mon Sep 17 00:00:00 2001 From: Andy Goldstein Date: Fri, 9 Nov 2018 13:39:56 -0500 Subject: [PATCH] Remove default token from all service accounts Instead of only removing the default token from a service account when it already exists in the cluster, always remove it. If the service account already exists, continue to do the merging logic. Signed-off-by: Andy Goldstein --- pkg/cmd/server/plugin/plugin.go | 5 + pkg/restore/merge_service_account.go | 11 --- pkg/restore/merge_service_account_test.go | 27 +++-- pkg/restore/service_account_action.go | 79 +++++++++++++++ pkg/restore/service_account_action_test.go | 110 +++++++++++++++++++++ 5 files changed, 207 insertions(+), 25 deletions(-) create mode 100644 pkg/restore/service_account_action.go create mode 100644 pkg/restore/service_account_action_test.go diff --git a/pkg/cmd/server/plugin/plugin.go b/pkg/cmd/server/plugin/plugin.go index c289ee4b75..966df8c0af 100644 --- a/pkg/cmd/server/plugin/plugin.go +++ b/pkg/cmd/server/plugin/plugin.go @@ -54,6 +54,7 @@ func NewCommand(f client.Factory) *cobra.Command { RegisterRestoreItemAction("pod", newPodRestoreItemAction). RegisterRestoreItemAction("restic", newResticRestoreItemAction). RegisterRestoreItemAction("service", newServiceRestoreItemAction). + RegisterRestoreItemAction("serviceaccount", newServiceAccountRestoreItemAction). Serve() }, } @@ -133,3 +134,7 @@ func newResticRestoreItemAction(logger logrus.FieldLogger) (interface{}, error) func newServiceRestoreItemAction(logger logrus.FieldLogger) (interface{}, error) { return restore.NewServiceAction(logger), nil } + +func newServiceAccountRestoreItemAction(logger logrus.FieldLogger) (interface{}, error) { + return restore.NewServiceAccountAction(logger), nil +} diff --git a/pkg/restore/merge_service_account.go b/pkg/restore/merge_service_account.go index ae55bcf405..dcbe030763 100644 --- a/pkg/restore/merge_service_account.go +++ b/pkg/restore/merge_service_account.go @@ -17,7 +17,6 @@ package restore import ( "encoding/json" - "strings" jsonpatch "github.com/evanphx/json-patch" "github.com/pkg/errors" @@ -30,7 +29,6 @@ import ( ) // mergeServiceAccount takes a backed up serviceaccount and merges attributes into the current in-cluster service account. -// The default token secret from the backed up serviceaccount will be ignored in favor of the one already present. // Labels and Annotations on the backed up version but not on the in-cluster version will be merged. If a key is specified in both, the in-cluster version is retained. func mergeServiceAccounts(fromCluster, fromBackup *unstructured.Unstructured) (*unstructured.Unstructured, error) { desired := new(corev1api.ServiceAccount) @@ -44,15 +42,6 @@ func mergeServiceAccounts(fromCluster, fromBackup *unstructured.Unstructured) (* return nil, errors.Wrap(err, "unable to convert from backed up service account unstructured to serviceaccount") } - for i := len(backupSA.Secrets) - 1; i >= 0; i-- { - secret := &backupSA.Secrets[i] - if strings.HasPrefix(secret.Name, backupSA.Name+"-token-") { - // Copy all secrets *except* -token- - backupSA.Secrets = append(backupSA.Secrets[:i], backupSA.Secrets[i+1:]...) - break - } - } - desired.Secrets = mergeObjectReferenceSlices(desired.Secrets, backupSA.Secrets) desired.ImagePullSecrets = mergeLocalObjectReferenceSlices(desired.ImagePullSecrets, backupSA.ImagePullSecrets) diff --git a/pkg/restore/merge_service_account_test.go b/pkg/restore/merge_service_account_test.go index e3d425da91..655ec205c1 100644 --- a/pkg/restore/merge_service_account_test.go +++ b/pkg/restore/merge_service_account_test.go @@ -21,6 +21,7 @@ import ( "unicode" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" corev1api "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -503,7 +504,7 @@ func TestMergeServiceAccountBasic(t *testing.T) { expectedErr bool }{ { - name: "only default tokens present", + name: "only default token", fromCluster: arktest.UnstructuredOrDie( `{ "apiVersion": "v1", @@ -517,6 +518,8 @@ func TestMergeServiceAccountBasic(t *testing.T) { ] }`, ), + // fromBackup doesn't have the default token because it is expected to already have been removed + // by the service account action fromBackup: arktest.UnstructuredOrDie( `{ "kind": "ServiceAccount", @@ -525,9 +528,7 @@ func TestMergeServiceAccountBasic(t *testing.T) { "namespace": "ns1", "name": "default" }, - "secrets": [ - { "name": "default-token-xzy12" } - ] + "secrets": [] }`, ), expectedRes: arktest.UnstructuredOrDie( @@ -561,7 +562,8 @@ func TestMergeServiceAccountBasic(t *testing.T) { ] }`, ), - + // fromBackup doesn't have the default token because it is expected to already have been removed + // by the service account action fromBackup: arktest.UnstructuredOrDie( `{ "kind": "ServiceAccount", @@ -571,7 +573,6 @@ func TestMergeServiceAccountBasic(t *testing.T) { "name": "default" }, "secrets": [ - { "name": "default-token-xzy12" }, { "name": "my-old-secret" }, { "name": "secrete"} ] @@ -621,7 +622,8 @@ func TestMergeServiceAccountBasic(t *testing.T) { ] }`, ), - + // fromBackup doesn't have the default token because it is expected to already have been removed + // by the service account action fromBackup: arktest.UnstructuredOrDie( `{ "kind": "ServiceAccount", @@ -645,9 +647,7 @@ func TestMergeServiceAccountBasic(t *testing.T) { "a6": "v6" } }, - "secrets": [ - { "name": "default-token-xzy12" } - ] + "secrets": [] }`, ), expectedRes: arktest.UnstructuredOrDie( @@ -682,11 +682,10 @@ func TestMergeServiceAccountBasic(t *testing.T) { } for _, test := range tests { - t.Run(test.name, func(b *testing.T) { + t.Run(test.name, func(t *testing.T) { result, err := mergeServiceAccounts(test.fromCluster, test.fromBackup) - if err != nil { - assert.Equal(t, test.expectedRes, result) - } + require.NoError(t, err) + assert.Equal(t, test.expectedRes, result) }) } } diff --git a/pkg/restore/service_account_action.go b/pkg/restore/service_account_action.go new file mode 100644 index 0000000000..8734ae4320 --- /dev/null +++ b/pkg/restore/service_account_action.go @@ -0,0 +1,79 @@ +/* +Copyright 2018 the Heptio Ark contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package restore + +import ( + "strings" + + "github.com/pkg/errors" + "github.com/sirupsen/logrus" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + + api "github.com/heptio/ark/pkg/apis/ark/v1" + "github.com/heptio/ark/pkg/util/kube" +) + +type serviceAccountAction struct { + logger logrus.FieldLogger +} + +func NewServiceAccountAction(logger logrus.FieldLogger) ItemAction { + return &serviceAccountAction{logger: logger} +} + +func (a *serviceAccountAction) AppliesTo() (ResourceSelector, error) { + return ResourceSelector{ + IncludedResources: []string{"serviceaccounts"}, + }, nil +} + +func (a *serviceAccountAction) Execute(obj runtime.Unstructured, restore *api.Restore) (runtime.Unstructured, error, error) { + a.logger.Info("Executing serviceAccountAction") + defer a.logger.Info("Done executing serviceAccountAction") + + var serviceAccount corev1.ServiceAccount + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), &serviceAccount); err != nil { + return nil, nil, errors.Wrap(err, "unable to convert serviceaccount from runtime.Unstructured") + } + + log := a.logger.WithField("serviceaccount", kube.NamespaceAndName(&serviceAccount)) + + log.Debug("Checking secrets") + check := serviceAccount.Name + "-token-" + for i := len(serviceAccount.Secrets) - 1; i >= 0; i-- { + secret := &serviceAccount.Secrets[i] + log.Debugf("Checking if secret %s matches %s", secret.Name, check) + + if strings.HasPrefix(secret.Name, check) { + // Copy all secrets *except* -token- + log.Debug("Match found - excluding this secret") + serviceAccount.Secrets = append(serviceAccount.Secrets[:i], serviceAccount.Secrets[i+1:]...) + break + } else { + log.Debug("No match found - including this secret") + } + } + + res, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&serviceAccount) + if err != nil { + return nil, nil, errors.Wrap(err, "unable to convert serviceaccount to runtime.Unstructured") + } + + return &unstructured.Unstructured{Object: res}, nil, nil +} diff --git a/pkg/restore/service_account_action_test.go b/pkg/restore/service_account_action_test.go new file mode 100644 index 0000000000..f8c46d9bc4 --- /dev/null +++ b/pkg/restore/service_account_action_test.go @@ -0,0 +1,110 @@ +/* +Copyright 2018 the Heptio Ark contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package restore + +import ( + "sort" + "testing" + + "github.com/heptio/ark/pkg/util/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" +) + +func TestServiceAccountActionAppliesTo(t *testing.T) { + action := NewServiceAccountAction(test.NewLogger()) + actual, err := action.AppliesTo() + require.NoError(t, err) + assert.Equal(t, ResourceSelector{IncludedResources: []string{"serviceaccounts"}}, actual) +} + +func TestServiceAccountActionExecute(t *testing.T) { + tests := []struct { + name string + secrets []string + expected []string + }{ + { + name: "no secrets", + secrets: []string{}, + expected: []string{}, + }, + { + name: "no match", + secrets: []string{"a", "bar-TOKN-nomatch", "baz"}, + expected: []string{"a", "bar-TOKN-nomatch", "baz"}, + }, + { + name: "match - first", + secrets: []string{"bar-token-a1b2c", "a", "baz"}, + expected: []string{"a", "baz"}, + }, + { + name: "match - middle", + secrets: []string{"a", "bar-token-a1b2c", "baz"}, + expected: []string{"a", "baz"}, + }, + { + name: "match - end", + secrets: []string{"a", "baz", "bar-token-a1b2c"}, + expected: []string{"a", "baz"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + sa := corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "foo", + Name: "bar", + }, + } + + for _, secret := range tc.secrets { + sa.Secrets = append(sa.Secrets, corev1.ObjectReference{ + Name: secret, + }) + } + + saUnstructured, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&sa) + require.NoError(t, err) + + action := NewServiceAccountAction(test.NewLogger()) + res, warning, err := action.Execute(&unstructured.Unstructured{Object: saUnstructured}, nil) + require.NoError(t, warning) + require.NoError(t, err) + + var resSA *corev1.ServiceAccount + err = runtime.DefaultUnstructuredConverter.FromUnstructured(res.UnstructuredContent(), &resSA) + require.NoError(t, err) + + actual := []string{} + for _, secret := range resSA.Secrets { + actual = append(actual, secret.Name) + } + + sort.Strings(tc.expected) + sort.Strings(actual) + + assert.Equal(t, tc.expected, actual) + }) + } +}