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.

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

Pluggable secret backend #2239

Merged
merged 1 commit into from
Jul 14, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
295 changes: 175 additions & 120 deletions api/specs.pb.go

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions api/specs.proto
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,9 @@ message SecretSpec {
// The currently recognized values are:
// - golang: Go templating
Driver templating = 3;

// Driver is the the secret driver that is used to store the specified secret
Driver driver = 4;
}

// ConfigSpec specifies user-provided configuration files.
Expand Down
14 changes: 14 additions & 0 deletions api/validation/secrets.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package validation

import "fmt"

// MaxSecretSize is the maximum byte length of the `Secret.Spec.Data` field.
const MaxSecretSize = 500 * 1024 // 500KB

// ValidateSecretPayload validates the secret payload size
func ValidateSecretPayload(data []byte) error {
if len(data) >= MaxSecretSize || len(data) < 1 {
return fmt.Errorf("secret data must be larger than 0 and less than %d bytes", MaxSecretSize)
}
return nil
}
11 changes: 10 additions & 1 deletion cmd/swarmctl/secret/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,13 @@ var createCmd = &cobra.Command{
var (
secretData []byte
err error
driver string
)

driver, err = flags.GetString("driver")
if err != nil {
return fmt.Errorf("Error reading secret driver %s", err.Error())
}
if flags.Changed("file") {
filename, err := flags.GetString("file")
if err != nil {
Expand All @@ -35,7 +40,7 @@ var createCmd = &cobra.Command{
if err != nil {
return fmt.Errorf("Error reading from file '%s': %s", filename, err.Error())
}
} else {
} else if driver == "" {
secretData, err = ioutil.ReadAll(os.Stdin)
if err != nil {
return fmt.Errorf("Error reading content from STDIN: %s", err.Error())
Expand All @@ -51,6 +56,9 @@ var createCmd = &cobra.Command{
Annotations: api.Annotations{Name: args[0]},
Data: secretData,
}
if driver != "" {
spec.Driver = &api.Driver{Name: driver}
}

resp, err := client.CreateSecret(common.Context(cmd), &api.CreateSecretRequest{Spec: spec})
if err != nil {
Expand All @@ -63,4 +71,5 @@ var createCmd = &cobra.Command{

func init() {
createCmd.Flags().StringP("file", "f", "", "Rather than read the secret from STDIN, read from the given file")
createCmd.Flags().StringP("driver", "d", "", "Rather than read the secret from STDIN, read the value from an external secret driver")
}
10 changes: 8 additions & 2 deletions cmd/swarmctl/secret/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,21 @@ var (
// Ignore flushing errors - there's nothing we can do.
_ = w.Flush()
}()
common.PrintHeader(w, "ID", "Name", "Created")
common.PrintHeader(w, "ID", "Name", "Driver", "Created")
output = func(s *api.Secret) {
created, err := gogotypes.TimestampFromProto(s.Meta.CreatedAt)
if err != nil {
panic(err)
}
fmt.Fprintf(w, "%s\t%s\t%s\n",
var driver string
if s.Spec.Driver != nil {
driver = s.Spec.Driver.Name
}

fmt.Fprintf(w, "%s\t%s\t%s\t%s\n",
s.ID,
s.Spec.Annotations.Name,
driver,
humanize.Time(created),
)
}
Expand Down
18 changes: 11 additions & 7 deletions manager/controlapi/secret.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

"github.com/Sirupsen/logrus"
"github.com/docker/swarmkit/api"
"github.com/docker/swarmkit/api/validation"
"github.com/docker/swarmkit/identity"
"github.com/docker/swarmkit/log"
"github.com/docker/swarmkit/manager/state/store"
Expand All @@ -14,9 +15,6 @@ import (
"google.golang.org/grpc/codes"
)

// MaxSecretSize is the maximum byte length of the `Secret.Spec.Data` field.
const MaxSecretSize = 500 * 1024 // 500KB

// assumes spec is not nil
func secretFromSecretSpec(spec *api.SecretSpec) *api.Secret {
return &api.Secret{
Expand Down Expand Up @@ -56,7 +54,6 @@ func (s *Server) UpdateSecret(ctx context.Context, request *api.UpdateSecretRequ
if request.SecretID == "" || request.SecretVersion == nil {
return nil, grpc.Errorf(codes.InvalidArgument, errInvalidArgument.Error())
}

var secret *api.Secret
err := s.store.Update(func(tx store.Tx) error {
secret = store.GetSecret(tx, request.SecretID)
Expand Down Expand Up @@ -245,9 +242,16 @@ func validateSecretSpec(spec *api.SecretSpec) error {
if err := validateConfigOrSecretAnnotations(spec.Annotations); err != nil {
return err
}

if len(spec.Data) >= MaxSecretSize || len(spec.Data) < 1 {
return grpc.Errorf(codes.InvalidArgument, "secret data must be larger than 0 and less than %d bytes", MaxSecretSize)
// Check if secret driver is defined
if spec.Driver != nil {
// Ensure secret driver has a name
if spec.Driver.Name == "" {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be tracing the code incorrectly but it seems that we could have a non-nil driver with an empty name? See: https://github.com/docker/swarmkit/pull/2239/files#diff-b7cdf7ddfbe8b31d75bc99e8d2d0fa78R58

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @riyazdf, you are right, I modified the create flow accordingly.

return grpc.Errorf(codes.InvalidArgument, "secret driver must have a name")
}
return nil
}
if err := validation.ValidateSecretPayload(spec.Data); err != nil {
return grpc.Errorf(codes.InvalidArgument, "%s", err.Error())
}
return nil
}
10 changes: 10 additions & 0 deletions manager/controlapi/secret_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,16 @@ func TestValidateSecretSpec(t *testing.T) {
err := validateSecretSpec(good)
assert.NoError(t, err)
}

// Ensure secret driver has a name
spec := createSecretSpec("secret-driver", make([]byte, 1), nil)
spec.Driver = &api.Driver{}
err := validateSecretSpec(spec)
assert.Error(t, err)
assert.Equal(t, codes.InvalidArgument, grpc.Code(err), grpc.ErrorDesc(err))
spec.Driver.Name = "secret-driver"
err = validateSecretSpec(spec)
assert.NoError(t, err)
}

func TestCreateSecret(t *testing.T) {
Expand Down
1 change: 0 additions & 1 deletion manager/controlapi/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
)

var (
errNotImplemented = errors.New("not implemented")
errInvalidArgument = errors.New("invalid argument")
)

Expand Down
44 changes: 38 additions & 6 deletions manager/dispatcher/assignments.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package dispatcher

import (
"fmt"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit: normally there would be a blank line here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aaronlehmann fixed.


"github.com/Sirupsen/logrus"
"github.com/docker/swarmkit/api"
"github.com/docker/swarmkit/api/equality"
"github.com/docker/swarmkit/api/validation"
"github.com/docker/swarmkit/manager/drivers"
"github.com/docker/swarmkit/manager/state/store"
)

Expand All @@ -24,15 +28,16 @@ type typeAndID struct {
}

type assignmentSet struct {
dp *drivers.DriverProvider
tasksMap map[string]*api.Task
tasksUsingDependency map[typeAndID]map[string]struct{}
changes map[typeAndID]*api.AssignmentChange

log *logrus.Entry
log *logrus.Entry
}

func newAssignmentSet(log *logrus.Entry) *assignmentSet {
func newAssignmentSet(log *logrus.Entry, dp *drivers.DriverProvider) *assignmentSet {
return &assignmentSet{
dp: dp,
changes: make(map[typeAndID]*api.AssignmentChange),
tasksMap: make(map[string]*api.Task),
tasksUsingDependency: make(map[typeAndID]map[string]struct{}),
Expand All @@ -53,12 +58,13 @@ func (a *assignmentSet) addTaskDependencies(readTx store.ReadTx, t *api.Task) {
if len(a.tasksUsingDependency[mapKey]) == 0 {
a.tasksUsingDependency[mapKey] = make(map[string]struct{})

secret := store.GetSecret(readTx, secretID)
if secret == nil {
secret, err := a.secret(readTx, secretID)
if err != nil {
a.log.WithFields(logrus.Fields{
"secret.id": secretID,
"secret.name": secretRef.SecretName,
}).Debug("secret not found")
"error": err,
}).Error("failed to fetch secret")
continue
}

Expand Down Expand Up @@ -245,3 +251,29 @@ func (a *assignmentSet) message() api.AssignmentsMessage {

return message
}

// secret populates the secret value from raft store. For external secrets, the value is populated
// from the secret driver.
func (a *assignmentSet) secret(readTx store.ReadTx, secretID string) (*api.Secret, error) {
secret := store.GetSecret(readTx, secretID)
if secret == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is returning any time the local store doesn't have the secret.

I still think it's better to check the local store only if driver was not specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpuguy83 but if a driver was specified, I still need to query the api.Secret object to fetch the api.Driver (to correctly materialize the secret).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see now.
Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow: doesn't this function return if the store.GetSecret returns nil?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The secret metadata is still stored in the raft store and must exist there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@riyazdf, just to clarify, following @cpuguy83 comments, I consolidate all secret fetching functionality to a single function.
The flow:

  1. Fetch the secret (since the secret is required by the task, return error if not found)
  2. If secrets driver is defined, fetch the secret value from the driver (otherwise return the secret value)
    Let me know if additional refactoring is needed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it. Thanks @cpuguy83 and @liron-l for clarifying. :)

return nil, fmt.Errorf("secret not found")
}
if secret.Spec.Driver == nil {
return secret, nil
}
d, err := a.dp.NewSecretDriver(secret.Spec.Driver)
if err != nil {
return nil, err
}
value, err := d.Get(&secret.Spec)
if err != nil {
return nil, err
}
if err := validation.ValidateSecretPayload(value); err != nil {
return nil, err
}
// Assign the secret
secret.Spec.Data = value
return secret, nil
}
7 changes: 5 additions & 2 deletions manager/dispatcher/dispatcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/docker/swarmkit/api/equality"
"github.com/docker/swarmkit/ca"
"github.com/docker/swarmkit/log"
"github.com/docker/swarmkit/manager/drivers"
"github.com/docker/swarmkit/manager/state/store"
"github.com/docker/swarmkit/remotes"
"github.com/docker/swarmkit/watch"
Expand Down Expand Up @@ -125,6 +126,7 @@ type Dispatcher struct {
ctx context.Context
cancel context.CancelFunc
clusterUpdateQueue *watch.Queue
dp *drivers.DriverProvider

taskUpdates map[string]*api.TaskStatus // indexed by task ID
taskUpdatesLock sync.Mutex
Expand All @@ -142,8 +144,9 @@ type Dispatcher struct {
}

// New returns Dispatcher with cluster interface(usually raft.Node).
func New(cluster Cluster, c *Config) *Dispatcher {
func New(cluster Cluster, c *Config, dp *drivers.DriverProvider) *Dispatcher {
d := &Dispatcher{
dp: dp,
nodes: newNodeStore(c.HeartbeatPeriod, c.HeartbeatEpsilon, c.GracePeriodMultiplier, c.RateLimitPeriod),
downNodes: newNodeStore(defaultNodeDownPeriod, 0, 1, 0),
store: cluster.MemoryStore(),
Expand Down Expand Up @@ -836,7 +839,7 @@ func (d *Dispatcher) Assignments(r *api.AssignmentsRequest, stream api.Dispatche
var (
sequence int64
appliesTo string
assignments = newAssignmentSet(log)
assignments = newAssignmentSet(log, d.dp)
)

sendMessage := func(msg api.AssignmentsMessage, assignmentType api.AssignmentsMessage_Type) error {
Expand Down
Loading