Skip to content

Commit

Permalink
Merge pull request #1715 from diogomonica/graceful-secret-removal
Browse files Browse the repository at this point in the history 
Graceful secret removal
  • Loading branch information
@aluzzardi
aluzzardi authored Nov 4, 2016
2 parents 233a11f + f67b6d7 commit 5afda15
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 25 deletions.
30 changes: 30 additions & 0 deletions manager/controlapi/secret.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package controlapi

import (
"regexp"
"strings"

"github.com/Sirupsen/logrus"
"github.com/docker/distribution/digest"
Expand Down Expand Up @@ -191,13 +192,42 @@ func (s *Server) CreateSecret(ctx context.Context, request *api.CreateSecretRequ
// RemoveSecret removes the secret referenced by `RemoveSecretRequest.ID`.
// - Returns `InvalidArgument` if `RemoveSecretRequest.ID` is empty.
// - Returns `NotFound` if the a secret named `RemoveSecretRequest.ID` is not found.
// - Returns `SecretInUse` if the secret is currently in use
// - Returns an error if the deletion fails.
func (s *Server) RemoveSecret(ctx context.Context, request *api.RemoveSecretRequest) (*api.RemoveSecretResponse, error) {
if request.SecretID == "" {
return nil, grpc.Errorf(codes.InvalidArgument, "secret ID must be provided")
}

err := s.store.Update(func(tx store.Tx) error {
// Check if the secret exists
secret := store.GetSecret(tx, request.SecretID)
if secret == nil {
return grpc.Errorf(codes.NotFound, "could not find secret %s", request.SecretID)
}

// Check if any services currently reference this secret, return error if so
services, err := store.FindServices(tx, store.ByReferencedSecretID(request.SecretID))
if err != nil {
return grpc.Errorf(codes.Internal, "could not find services using secret %s: %v", request.SecretID, err)
}

if len(services) != 0 {
serviceNames := make([]string, 0, len(services))
for _, service := range services {
serviceNames = append(serviceNames, service.Spec.Annotations.Name)
}

secretName := secret.Spec.Annotations.Name
serviceNameStr := strings.Join(serviceNames, ", ")
serviceStr := "services"
if len(serviceNames) == 1 {
serviceStr = "service"
}

return grpc.Errorf(codes.InvalidArgument, "secret '%s' is in use by the following %s: %v", secretName, serviceStr, serviceNameStr)
}

return store.DeleteSecret(tx, request.SecretID)
})
switch err {
Expand Down
103 changes: 79 additions & 24 deletions manager/controlapi/secret_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,11 @@ func TestValidateSecretSpec(t *testing.T) {
}

func TestCreateSecret(t *testing.T) {
s := newTestServer(t)
ts := newTestServer(t)
defer ts.Stop()

// ---- creating a secret with an invalid spec fails, thus checking that CreateSecret validates the spec ----
_, err := s.Client.CreateSecret(context.Background(), &api.CreateSecretRequest{Spec: createSecretSpec("", nil, nil)})
_, err := ts.Client.CreateSecret(context.Background(), &api.CreateSecretRequest{Spec: createSecretSpec("", nil, nil)})
assert.Error(t, err)
assert.Equal(t, codes.InvalidArgument, grpc.Code(err), grpc.ErrorDesc(err))

Expand All @@ -113,7 +114,7 @@ func TestCreateSecret(t *testing.T) {
creationSpec := createSecretSpec("name", data, nil)
validSpecRequest := api.CreateSecretRequest{Spec: creationSpec}

resp, err := s.Client.CreateSecret(context.Background(), &validSpecRequest)
resp, err := ts.Client.CreateSecret(context.Background(), &validSpecRequest)
assert.NoError(t, err)
assert.NotNil(t, resp)
assert.NotNil(t, resp.Secret)
Expand All @@ -124,39 +125,40 @@ func TestCreateSecret(t *testing.T) {

// for sanity, check that the stored secret still has the secret data
var storedSecret *api.Secret
s.Store.View(func(tx store.ReadTx) {
ts.Store.View(func(tx store.ReadTx) {
storedSecret = store.GetSecret(tx, resp.Secret.ID)
})
assert.NotNil(t, storedSecret)
assert.Equal(t, data, storedSecret.Spec.Data)

// ---- creating a secret with the same name, even if it's the exact same spec, fails due to a name conflict ----
_, err = s.Client.CreateSecret(context.Background(), &validSpecRequest)
_, err = ts.Client.CreateSecret(context.Background(), &validSpecRequest)
assert.Error(t, err)
assert.Equal(t, codes.AlreadyExists, grpc.Code(err), grpc.ErrorDesc(err))
}

func TestGetSecret(t *testing.T) {
s := newTestServer(t)
ts := newTestServer(t)
defer ts.Stop()

// ---- getting a secret without providing an ID results in an InvalidArgument ----
_, err := s.Client.GetSecret(context.Background(), &api.GetSecretRequest{})
_, err := ts.Client.GetSecret(context.Background(), &api.GetSecretRequest{})
assert.Error(t, err)
assert.Equal(t, codes.InvalidArgument, grpc.Code(err), grpc.ErrorDesc(err))

// ---- getting a non-existent secret fails with NotFound ----
_, err = s.Client.GetSecret(context.Background(), &api.GetSecretRequest{SecretID: "12345"})
_, err = ts.Client.GetSecret(context.Background(), &api.GetSecretRequest{SecretID: "12345"})
assert.Error(t, err)
assert.Equal(t, codes.NotFound, grpc.Code(err), grpc.ErrorDesc(err))

// ---- getting an existing secret returns the secret with all the private data cleaned ----
secret := secretFromSecretSpec(createSecretSpec("name", []byte("data"), nil))
err = s.Store.Update(func(tx store.Tx) error {
err = ts.Store.Update(func(tx store.Tx) error {
return store.CreateSecret(tx, secret)
})
assert.NoError(t, err)

resp, err := s.Client.GetSecret(context.Background(), &api.GetSecretRequest{SecretID: secret.ID})
resp, err := ts.Client.GetSecret(context.Background(), &api.GetSecretRequest{SecretID: secret.ID})
assert.NoError(t, err)
assert.NotNil(t, resp)
assert.NotNil(t, resp.Secret)
Expand All @@ -168,29 +170,31 @@ func TestGetSecret(t *testing.T) {
}

func TestUpdateSecret(t *testing.T) {
s := newTestServer(t)
ts := newTestServer(t)
defer ts.Stop()

// Add a secret to the store to update
secret := secretFromSecretSpec(createSecretSpec("name", []byte("data"), map[string]string{"mod2": "0", "mod4": "0"}))
err := s.Store.Update(func(tx store.Tx) error {
err := ts.Store.Update(func(tx store.Tx) error {
return store.CreateSecret(tx, secret)
})
assert.NoError(t, err)

// ---- updating a secret without providing an ID results in an InvalidArgument ----
_, err = s.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{})
_, err = ts.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{})
assert.Error(t, err)
assert.Equal(t, codes.InvalidArgument, grpc.Code(err), grpc.ErrorDesc(err))

// ---- getting a non-existent secret fails with NotFound ----
_, err = s.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{SecretID: "1234adsaa", SecretVersion: &api.Version{Index: 1}})
_, err = ts.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{SecretID: "1234adsaa", SecretVersion: &api.Version{Index: 1}})
assert.Error(t, err)
assert.Equal(t, codes.NotFound, grpc.Code(err), grpc.ErrorDesc(err))

// ---- updating an existing secret's labels returns the secret with all the private data cleaned ----
newLabels := map[string]string{"mod2": "0", "mod4": "0", "mod6": "0"}
secret.Spec.Annotations.Labels = newLabels
secret.Spec.Data = nil
resp, err := s.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{
resp, err := ts.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{
SecretID: secret.ID,
Spec: &secret.Spec,
SecretVersion: &secret.Meta.Version,
Expand All @@ -203,40 +207,91 @@ func TestUpdateSecret(t *testing.T) {

// ---- updating an existing secret's data returns an error ----
secret.Spec.Data = []byte{1}
resp, err = s.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{
resp, err = ts.Client.UpdateSecret(context.Background(), &api.UpdateSecretRequest{
SecretID: secret.ID,
Spec: &secret.Spec,
SecretVersion: &secret.Meta.Version,
})
assert.Equal(t, codes.InvalidArgument, grpc.Code(err), grpc.ErrorDesc(err))
}

func TestRemoveSecret(t *testing.T) {
s := newTestServer(t)
func TestRemoveUnusedSecret(t *testing.T) {
ts := newTestServer(t)
defer ts.Stop()

// ---- removing a secret without providing an ID results in an InvalidArgument ----
_, err := s.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{})
// removing a secret without providing an ID results in an InvalidArgument
_, err := ts.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{})
assert.Error(t, err)
assert.Equal(t, codes.InvalidArgument, grpc.Code(err), grpc.ErrorDesc(err))

// ---- removing a secret that exists succeeds ----
// removing a secret that exists succeeds
secret := secretFromSecretSpec(createSecretSpec("name", []byte("data"), nil))
err = s.Store.Update(func(tx store.Tx) error {
err = ts.Store.Update(func(tx store.Tx) error {
return store.CreateSecret(tx, secret)
})
assert.NoError(t, err)

resp, err := s.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{SecretID: secret.ID})
resp, err := ts.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{SecretID: secret.ID})
assert.NoError(t, err)
assert.Equal(t, api.RemoveSecretResponse{}, *resp)

// ---- it was really removed because attempting to remove it again fails with a NotFound ----
_, err = s.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{SecretID: secret.ID})
_, err = ts.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{SecretID: secret.ID})
assert.Error(t, err)
assert.Equal(t, codes.NotFound, grpc.Code(err), grpc.ErrorDesc(err))

}

func TestRemoveUsedSecret(t *testing.T) {
ts := newTestServer(t)
defer ts.Stop()

// Create two secrets
data := []byte("secret")
creationSpec := createSecretSpec("secretID1", data, nil)
resp, err := ts.Client.CreateSecret(context.Background(), &api.CreateSecretRequest{Spec: creationSpec})
assert.NoError(t, err)
creationSpec2 := createSecretSpec("secretID2", data, nil)
resp2, err := ts.Client.CreateSecret(context.Background(), &api.CreateSecretRequest{Spec: creationSpec2})
assert.NoError(t, err)

// Create a service that uses a secret
service := createSpec("service1", "image", 1)
secretRefs := []*api.SecretReference{
{
SecretName: resp.Secret.Spec.Annotations.Name,
SecretID: resp.Secret.ID,
Target: &api.SecretReference_File{
File: &api.SecretReference_FileTarget{
Name: "target.txt",
},
},
},
}
service.Task.GetContainer().Secrets = secretRefs
_, err = ts.Client.CreateService(context.Background(), &api.CreateServiceRequest{Spec: service})
assert.NoError(t, err)

service2 := createSpec("service2", "image", 1)
service2.Task.GetContainer().Secrets = secretRefs
_, err = ts.Client.CreateService(context.Background(), &api.CreateServiceRequest{Spec: service2})
assert.NoError(t, err)

// removing a secret that exists but is in use fails
_, err = ts.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{SecretID: resp.Secret.ID})
assert.Equal(t, codes.InvalidArgument, grpc.Code(err), grpc.ErrorDesc(err))
assert.Regexp(t, "service[1-2], service[1-2]", grpc.ErrorDesc(err))

// removing a secret that exists but is not in use succeeds
_, err = ts.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{SecretID: resp2.Secret.ID})
assert.NoError(t, err)

// it was really removed because attempting to remove it again fails with a NotFound
_, err = ts.Client.RemoveSecret(context.Background(), &api.RemoveSecretRequest{SecretID: resp2.Secret.ID})
assert.Error(t, err)
assert.Equal(t, codes.NotFound, grpc.Code(err), grpc.ErrorDesc(err))
}

func TestListSecrets(t *testing.T) {
s := newTestServer(t)

Expand Down
2 changes: 1 addition & 1 deletion manager/controlapi/service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ func TestSecretValidation(t *testing.T) {
assert.NoError(t, err)

// test secret References with invalid filenames
invalidFileNames := []string{"../secretfile.txt", "../../secretfile.txt", "file../.txt"}
invalidFileNames := []string{"../secretfile.txt", "../../secretfile.txt", "file../.txt", "subdir/file.txt"}
for i, invalidName := range invalidFileNames {
secretRef := createSecret(t, ts, invalidName, invalidName)

Expand Down

0 comments on commit 5afda15

Please sign in to comment.