Skip to content

Commit 0ec7c6e

Browse files
author
Aaron Lehmann
authored
Merge pull request moby#1695 from stevvooe/secrets-decoupled
secrets: decouple secrets from agent package
2 parents ff68c46 + 66c7383 commit 0ec7c6e

11 files changed

Lines changed: 88 additions & 64 deletions

File tree

‎agent/agent_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ func (e *NoopExecutor) Configure(ctx context.Context, node *api.Node) error {
2626
return nil
2727
}
2828

29-
func (e *NoopExecutor) Controller(t *api.Task, secrets exec.SecretProvider) (exec.Controller, error) {
29+
func (e *NoopExecutor) Controller(t *api.Task) (exec.Controller, error) {
3030
return nil, exec.ErrRuntimeUnsupported
3131
}
3232

‎agent/exec/container/adapter.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,10 @@ import (
2626
type containerAdapter struct {
2727
client engineapi.APIClient
2828
container *containerConfig
29-
secrets exec.SecretProvider
29+
secrets exec.SecretGetter
3030
}
3131

32-
func newContainerAdapter(client engineapi.APIClient, task *api.Task, secrets exec.SecretProvider) (*containerAdapter, error) {
32+
func newContainerAdapter(client engineapi.APIClient, task *api.Task, secrets exec.SecretGetter) (*containerAdapter, error) {
3333
ctnr, err := newContainerConfig(task)
3434
if err != nil {
3535
return nil, err

‎agent/exec/container/controller.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ type controller struct {
3131
var _ exec.Controller = &controller{}
3232

3333
// newController returns a docker exec controller for the provided task.
34-
func newController(client engineapi.APIClient, task *api.Task, secrets exec.SecretProvider) (exec.Controller, error) {
34+
func newController(client engineapi.APIClient, task *api.Task, secrets exec.SecretGetter) (exec.Controller, error) {
3535
adapter, err := newContainerAdapter(client, task, secrets)
3636
if err != nil {
3737
return nil, err

‎agent/exec/container/executor.go‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,18 +6,21 @@ import (
66

77
engineapi "github.com/docker/docker/client"
88
"github.com/docker/swarmkit/agent/exec"
9+
"github.com/docker/swarmkit/agent/secrets"
910
"github.com/docker/swarmkit/api"
1011
"golang.org/x/net/context"
1112
)
1213

1314
type executor struct {
14-
client engineapi.APIClient
15+
client engineapi.APIClient
16+
secrets exec.SecretsManager
1517
}
1618

1719
// NewExecutor returns an executor from the docker client.
1820
func NewExecutor(client engineapi.APIClient) exec.Executor {
1921
return &executor{
20-
client: client,
22+
client: client,
23+
secrets: secrets.NewManager(),
2124
}
2225
}
2326

@@ -86,8 +89,8 @@ func (e *executor) Configure(ctx context.Context, node *api.Node) error {
8689
}
8790

8891
// Controller returns a docker container controller.
89-
func (e *executor) Controller(t *api.Task, secrets exec.SecretProvider) (exec.Controller, error) {
90-
ctlr, err := newController(e.client, t, secrets)
92+
func (e *executor) Controller(t *api.Task) (exec.Controller, error) {
93+
ctlr, err := newController(e.client, t, secrets.Restrict(e.secrets, t))
9194
if err != nil {
9295
return nil, err
9396
}
@@ -99,6 +102,10 @@ func (e *executor) SetNetworkBootstrapKeys([]*api.EncryptionKey) error {
99102
return nil
100103
}
101104

105+
func (e *executor) Secrets() exec.SecretsManager {
106+
return e.secrets
107+
}
108+
102109
type sortedPlugins []api.PluginDescription
103110

104111
func (sp sortedPlugins) Len() int { return len(sp) }

‎agent/exec/controller.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,14 +59,14 @@ type ContainerStatuser interface {
5959
//
6060
// Unlike Do, if an error is returned, the status should still be reported. The
6161
// error merely reports the failure at getting the controller.
62-
func Resolve(ctx context.Context, task *api.Task, secrets SecretProvider, executor Executor) (Controller, *api.TaskStatus, error) {
62+
func Resolve(ctx context.Context, task *api.Task, executor Executor) (Controller, *api.TaskStatus, error) {
6363
status := task.Status.Copy()
6464

6565
defer func() {
6666
logStateChange(ctx, task.DesiredState, task.Status.State, status.State)
6767
}()
6868

69-
ctlr, err := executor.Controller(task, secrets)
69+
ctlr, err := executor.Controller(task)
7070

7171
// depending on the tasks state, a failed controller resolution has varying
7272
// impact. The following expresses that impact.

‎agent/exec/controller_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,21 +23,21 @@ func TestResolve(t *testing.T) {
2323
task = newTestTask(t, api.TaskStateAssigned, api.TaskStateRunning)
2424
)
2525

26-
_, status, err := Resolve(ctx, task, nil, executor)
26+
_, status, err := Resolve(ctx, task, executor)
2727
assert.NoError(t, err)
2828
assert.Equal(t, api.TaskStateAccepted, status.State)
2929
assert.Equal(t, "accepted", status.Message)
3030

3131
task.Status = *status
3232
// now, we get no status update.
33-
_, status, err = Resolve(ctx, task, nil, executor)
33+
_, status, err = Resolve(ctx, task, executor)
3434
assert.NoError(t, err)
3535
assert.Equal(t, task.Status, *status)
3636

3737
// now test an error causing rejection
3838
executor.err = errors.New("some error")
3939
task = newTestTask(t, api.TaskStateAssigned, api.TaskStateRunning)
40-
_, status, err = Resolve(ctx, task, nil, executor)
40+
_, status, err = Resolve(ctx, task, executor)
4141
assert.Equal(t, executor.err, err)
4242
assert.Equal(t, api.TaskStateRejected, status.State)
4343

@@ -46,7 +46,7 @@ func TestResolve(t *testing.T) {
4646
// touched.
4747
task.Status = *status
4848
executor.err = nil
49-
_, status, err = Resolve(ctx, task, nil, executor)
49+
_, status, err = Resolve(ctx, task, executor)
5050
assert.NoError(t, err)
5151
assert.Equal(t, task.Status, *status)
5252
}
@@ -411,6 +411,6 @@ type mockExecutor struct {
411411
err error
412412
}
413413

414-
func (m *mockExecutor) Controller(t *api.Task, secrets SecretProvider) (Controller, error) {
414+
func (m *mockExecutor) Controller(t *api.Task) (Controller, error) {
415415
return nil, m.err
416416
}

‎agent/exec/executor.go‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,31 @@ type Executor interface {
1515
Configure(ctx context.Context, node *api.Node) error
1616

1717
// Controller provides a controller for the given task.
18-
Controller(t *api.Task, secrets SecretProvider) (Controller, error)
18+
Controller(t *api.Task) (Controller, error)
1919

2020
// SetNetworkBootstrapKeys passes the symmetric keys from the
2121
// manager to the executor.
2222
SetNetworkBootstrapKeys([]*api.EncryptionKey) error
2323
}
2424

25-
// SecretProvider contains secret data necessary for the Controller.
26-
type SecretProvider interface {
25+
// SecretsProvider is implemented by objects that can store secrets, typically
26+
// an executor.
27+
type SecretsProvider interface {
28+
Secrets() SecretsManager
29+
}
30+
31+
// SecretGetter contains secret data necessary for the Controller.
32+
type SecretGetter interface {
2733
// Get returns the the secret with a specific secret ID, if available.
2834
// When the secret is not available, the return will be nil.
2935
Get(secretID string) *api.Secret
3036
}
37+
38+
// SecretsManager is the interface for secret storage and updates.
39+
type SecretsManager interface {
40+
SecretGetter
41+
42+
Add(secrets ...api.Secret) // add one or more secrets
43+
Remove(secrets []string) // remove the secrets by ID
44+
Reset() // remove all secrets
45+
}

agent/secrets.go renamed to agent/secrets/secrets.go

Lines changed: 23 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package agent
1+
package secrets
22

33
import (
44
"sync"
@@ -14,7 +14,8 @@ type secrets struct {
1414
m map[string]*api.Secret
1515
}
1616

17-
func newSecrets() *secrets {
17+
// NewManager returns a place to store secrets.
18+
func NewManager() exec.SecretsManager {
1819
return &secrets{
1920
m: make(map[string]*api.Secret),
2021
}
@@ -31,7 +32,7 @@ func (s *secrets) Get(secretID string) *api.Secret {
3132
}
3233

3334
// add adds one or more secrets to the secret map
34-
func (s *secrets) add(secrets ...api.Secret) {
35+
func (s *secrets) Add(secrets ...api.Secret) {
3536
s.mu.Lock()
3637
defer s.mu.Unlock()
3738
for _, secret := range secrets {
@@ -41,7 +42,7 @@ func (s *secrets) add(secrets ...api.Secret) {
4142

4243
// remove removes one or more secrets by ID from the secret map. Succeeds
4344
// whether or not the given IDs are in the map.
44-
func (s *secrets) remove(secrets []string) {
45+
func (s *secrets) Remove(secrets []string) {
4546
s.mu.Lock()
4647
defer s.mu.Unlock()
4748
for _, secret := range secrets {
@@ -50,46 +51,37 @@ func (s *secrets) remove(secrets []string) {
5051
}
5152

5253
// reset removes all the secrets
53-
func (s *secrets) reset() {
54+
func (s *secrets) Reset() {
5455
s.mu.Lock()
5556
defer s.mu.Unlock()
5657
s.m = make(map[string]*api.Secret)
5758
}
5859

59-
func (s *secrets) filter(secretIDs []string) map[string]*api.Secret {
60-
s.mu.RLock()
61-
defer s.mu.RUnlock()
62-
filteredSecrets := make(map[string]*api.Secret)
63-
64-
for _, secretID := range secretIDs {
65-
if s, ok := s.m[secretID]; ok {
66-
filteredSecrets[secretID] = s.Copy()
67-
}
68-
}
69-
70-
return filteredSecrets
60+
// taskRestrictedSecretsProvider restricts the ids to the task.
61+
type taskRestrictedSecretsProvider struct {
62+
secrets exec.SecretGetter
63+
secretIDs map[string]struct{} // allow list of secret ids
7164
}
7265

73-
// getStore returns ta Store with only the secrets corresponding to the IDs
74-
// that are passed in.
75-
func (s *secrets) getStore(secretIDs []string) exec.SecretProvider {
76-
return &secrets{
77-
m: s.filter(secretIDs),
66+
func (sp *taskRestrictedSecretsProvider) Get(secretID string) *api.Secret {
67+
if _, ok := sp.secretIDs[secretID]; !ok {
68+
return nil
7869
}
70+
71+
return sp.secrets.Get(secretID)
7972
}
8073

81-
// getStoreForTask returns only the secrets needed by a specific Task
82-
func (s *secrets) getStoreForTask(task *api.Task) exec.SecretProvider {
83-
var secretIDs []string
74+
// Restrict provides a getter that only allows access to the secrets
75+
// referenced by the task.
76+
func Restrict(secrets exec.SecretGetter, t *api.Task) exec.SecretGetter {
77+
sids := map[string]struct{}{}
8478

85-
container := task.Spec.GetContainer()
79+
container := t.Spec.GetContainer()
8680
if container != nil {
87-
for _, secretRef := range container.Secrets {
88-
secretIDs = append(secretIDs, secretRef.SecretID)
81+
for _, ref := range container.Secrets {
82+
sids[ref.SecretID] = struct{}{}
8983
}
9084
}
9185

92-
return &secrets{
93-
m: s.filter(secretIDs),
94-
}
86+
return &taskRestrictedSecretsProvider{secrets: secrets, secretIDs: sids}
9587
}

‎agent/worker.go‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ type worker struct {
4242
db *bolt.DB
4343
executor exec.Executor
4444
listeners map[*statusReporterKey]struct{}
45-
secrets *secrets
4645

4746
taskManagers map[string]*taskManager
4847
mu sync.RWMutex
@@ -54,7 +53,6 @@ func newWorker(db *bolt.DB, executor exec.Executor) *worker {
5453
executor: executor,
5554
listeners: make(map[*statusReporterKey]struct{}),
5655
taskManagers: make(map[string]*taskManager),
57-
secrets: newSecrets(),
5856
}
5957
}
6058

@@ -255,6 +253,15 @@ func reconcileTaskState(ctx context.Context, w *worker, assignments []*api.Assig
255253
}
256254

257255
func reconcileSecrets(ctx context.Context, w *worker, assignments []*api.AssignmentChange, fullSnapshot bool) error {
256+
var secrets exec.SecretsManager
257+
provider, ok := w.executor.(exec.SecretsProvider)
258+
if !ok {
259+
log.G(ctx).Warn("secrets update ignored; executor does not support secrets")
260+
return nil
261+
}
262+
263+
secrets = provider.Secrets()
264+
258265
var (
259266
updatedSecrets []api.Secret
260267
removedSecrets []string
@@ -278,11 +285,11 @@ func reconcileSecrets(ctx context.Context, w *worker, assignments []*api.Assignm
278285

279286
// If this was a complete set of secrets, we're going to clear the secrets map and add all of them
280287
if fullSnapshot {
281-
w.secrets.reset()
288+
secrets.Reset()
282289
} else {
283-
w.secrets.remove(removedSecrets)
290+
secrets.Remove(removedSecrets)
284291
}
285-
w.secrets.add(updatedSecrets...)
292+
secrets.Add(updatedSecrets...)
286293

287294
return nil
288295
}
@@ -337,9 +344,8 @@ func (w *worker) taskManager(ctx context.Context, tx *bolt.Tx, task *api.Task) (
337344

338345
func (w *worker) newTaskManager(ctx context.Context, tx *bolt.Tx, task *api.Task) (*taskManager, error) {
339346
ctx = log.WithLogger(ctx, log.G(ctx).WithField("task.id", task.ID))
340-
secrets := w.secrets.getStoreForTask(task)
341347

342-
ctlr, status, err := exec.Resolve(ctx, task, secrets, w.executor)
348+
ctlr, status, err := exec.Resolve(ctx, task, w.executor)
343349
if err := w.updateTaskStatus(ctx, tx, task.ID, status); err != nil {
344350
log.G(ctx).WithError(err).Error("error updating task status after controller resolution")
345351
}

‎agent/worker_test.go‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"github.com/Sirupsen/logrus"
77
"github.com/boltdb/bolt"
88
"github.com/docker/swarmkit/agent/exec"
9+
"github.com/docker/swarmkit/agent/secrets"
910
"github.com/docker/swarmkit/api"
1011
"github.com/docker/swarmkit/log"
1112
"github.com/stretchr/testify/assert"
@@ -17,7 +18,7 @@ func TestWorkerAssign(t *testing.T) {
1718
defer cleanup()
1819

1920
ctx := context.Background()
20-
executor := &mockExecutor{t: t}
21+
executor := &mockExecutor{t: t, secrets: secrets.NewManager()}
2122
worker := newWorker(db, executor)
2223
reporter := statusReporterFunc(func(ctx context.Context, taskID string, status *api.TaskStatus) error {
2324
log.G(ctx).WithFields(logrus.Fields{"task.id": taskID, "status": status}).Info("status update received")
@@ -138,9 +139,8 @@ func TestWorkerAssign(t *testing.T) {
138139

139140
assert.Equal(t, testcase.expectedTasks, tasks)
140141
assert.Equal(t, testcase.expectedAssigned, assigned)
141-
assert.Len(t, worker.secrets.m, len(testcase.expectedSecrets))
142142
for _, secret := range testcase.expectedSecrets {
143-
assert.NotNil(t, worker.secrets.Get(secret.ID))
143+
assert.NotNil(t, executor.secrets.Get(secret.ID))
144144
}
145145
}
146146
}
@@ -150,7 +150,7 @@ func TestWorkerUpdate(t *testing.T) {
150150
defer cleanup()
151151

152152
ctx := context.Background()
153-
executor := &mockExecutor{t: t}
153+
executor := &mockExecutor{t: t, secrets: secrets.NewManager()}
154154
worker := newWorker(db, executor)
155155
reporter := statusReporterFunc(func(ctx context.Context, taskID string, status *api.TaskStatus) error {
156156
log.G(ctx).WithFields(logrus.Fields{"task.id": taskID, "status": status}).Info("status update received")
@@ -350,9 +350,8 @@ func TestWorkerUpdate(t *testing.T) {
350350

351351
assert.Equal(t, testcase.expectedTasks, tasks)
352352
assert.Equal(t, testcase.expectedAssigned, assigned)
353-
assert.Len(t, worker.secrets.m, len(testcase.expectedSecrets))
354353
for _, secret := range testcase.expectedSecrets {
355-
assert.NotNil(t, worker.secrets.Get(secret.ID))
354+
assert.NotNil(t, executor.secrets.Get(secret.ID))
356355
}
357356
}
358357
}
@@ -376,8 +375,13 @@ func (mtc *mockTaskController) Close() error {
376375
type mockExecutor struct {
377376
t *testing.T
378377
exec.Executor
378+
secrets exec.SecretsManager
379379
}
380380

381-
func (m *mockExecutor) Controller(task *api.Task, secrets exec.SecretProvider) (exec.Controller, error) {
381+
func (m *mockExecutor) Controller(task *api.Task) (exec.Controller, error) {
382382
return &mockTaskController{t: m.t, task: task}, nil
383383
}
384+
385+
func (m *mockExecutor) Secrets() exec.SecretsManager {
386+
return m.secrets
387+
}

0 commit comments

Comments
 (0)