1df31dd fix(deployment): never replace another container's secret files · gordon

M internal/adapters/out/secretfiles/store.go +6 -6


@@ -69,7 +69,10 @@ func (s *Store) Dir() string { return s.dir }
69	69		// Close releases the base directory handle.
70	70		func (s *Store) Close() error { return s.root.Close() }
71	71
72		-	// Write replaces the container's directory with one 0444 file per key.
	72	+	// Write creates the container's directory with one 0444 file per key. The
	73	+	// directory must not exist yet: container names are unique per create, so
	74	+	// an existing directory belongs to another container and is never touched.
	75	+	// Crash leftovers are removed by the sweeps, not here.
73	76		func (s *Store) Write(_ context.Context, app, container string, values map[string]string) (map[string]string, error) {
74	77		if err := validateSet(app, container); err != nil {
75	78		return nil, err

@@ -86,8 +89,8 @@ func (s *Store) Write(_ context.Context, app, container string, values map[strin
86	89		return nil, fmt.Errorf("secret files: chmod app directory: %w", err)
87	90		}
88	91		rel := filepath.Join(app, container)
89		-	if err := s.root.RemoveAll(rel); err != nil {
90		-	return nil, fmt.Errorf("secret files: clear container directory: %w", err)
	92	+	if err := s.root.Mkdir(rel, dirMode); err != nil {
	93	+	return nil, fmt.Errorf("secret files: create container directory: %w", err)
91	94		}
92	95		paths, err := s.writeFiles(rel, values)
93	96		if err != nil {

@@ -98,9 +101,6 @@ func (s *Store) Write(_ context.Context, app, container string, values map[strin
98	101		}
99	102
100	103		func (s *Store) writeFiles(rel string, values map[string]string) (map[string]string, error) {
101		-	if err := s.root.Mkdir(rel, dirMode); err != nil {
102		-	return nil, fmt.Errorf("secret files: create container directory: %w", err)
103		-	}
104	104		if err := s.root.Chmod(rel, dirMode); err != nil {
105	105		return nil, fmt.Errorf("secret files: chmod container directory: %w", err)
106	106		}

 // Close releases the base directory handle.
 func (s *Store) Close() error { return s.root.Close() }
-// Write replaces the container's directory with one 0444 file per key.
+// Write creates the container's directory with one 0444 file per key. The
+// directory must not exist yet: container names are unique per create, so
+// an existing directory belongs to another container and is never touched.
+// Crash leftovers are removed by the sweeps, not here.
 func (s *Store) Write(_ context.Context, app, container string, values map[string]string) (map[string]string, error) {
 	if err := validateSet(app, container); err != nil {
 		return nil, err
 		return nil, fmt.Errorf("secret files: chmod app directory: %w", err)
 	}
 	rel := filepath.Join(app, container)
-	if err := s.root.RemoveAll(rel); err != nil {
+	if err := s.root.Mkdir(rel, dirMode); err != nil {
-		return nil, fmt.Errorf("secret files: clear container directory: %w", err)
+		return nil, fmt.Errorf("secret files: create container directory: %w", err)
 	}
 	paths, err := s.writeFiles(rel, values)
 	if err != nil {
 }
 func (s *Store) writeFiles(rel string, values map[string]string) (map[string]string, error) {
-	if err := s.root.Mkdir(rel, dirMode); err != nil {
-		return nil, fmt.Errorf("secret files: create container directory: %w", err)
-	}
 	if err := s.root.Chmod(rel, dirMode); err != nil {
 		return nil, fmt.Errorf("secret files: chmod container directory: %w", err)
 	}

M internal/adapters/out/secretfiles/store_test.go +7 -7


@@ -49,19 +49,19 @@ func TestStore_WriteModesAndRead(t *testing.T) {
49	49		assert.Equal(t, "s3cr3t", got)
50	50		}
51	51
52		-	func TestStore_WriteReplacesStaleKeys(t *testing.T) {
	52	+	func TestStore_WriteNeverReplacesExistingContainer(t *testing.T) {
53	53		store, _ := newStore(t)
54	54		ctx := context.Background()
55		-	_, err := store.Write(ctx, "blog", "c1", map[string]string{"OLD": "x"})
56		-	require.NoError(t, err)
57		-	_, err = store.Write(ctx, "blog", "c1", map[string]string{"NEW": "y"})
	55	+	_, err := store.Write(ctx, "blog", "c1", map[string]string{"K": "live"})
58	56		require.NoError(t, err)
59	57
60		-	_, err = store.Read(ctx, "blog", "c1", "OLD")
	58	+	_, err = store.Write(ctx, "blog", "c1", map[string]string{"K": "other"})
61	59		require.Error(t, err)
62		-	got, err := store.Read(ctx, "blog", "c1", "NEW")
	60	+	assert.NotContains(t, err.Error(), store.Dir())
	61	+
	62	+	got, err := store.Read(ctx, "blog", "c1", "K")
63	63		require.NoError(t, err)
64		-	assert.Equal(t, "y", got)
	64	+	assert.Equal(t, "live", got, "a live container's files stay intact")
65	65		}
66	66
67	67		func TestStore_RemoveAndList(t *testing.T) {

 	assert.Equal(t, "s3cr3t", got)
 }
-func TestStore_WriteReplacesStaleKeys(t *testing.T) {
+func TestStore_WriteNeverReplacesExistingContainer(t *testing.T) {
 	store, _ := newStore(t)
 	ctx := context.Background()
-	_, err := store.Write(ctx, "blog", "c1", map[string]string{"OLD": "x"})
+	_, err := store.Write(ctx, "blog", "c1", map[string]string{"K": "live"})
-	require.NoError(t, err)
-	_, err = store.Write(ctx, "blog", "c1", map[string]string{"NEW": "y"})
 	require.NoError(t, err)
-	_, err = store.Read(ctx, "blog", "c1", "OLD")
+	_, err = store.Write(ctx, "blog", "c1", map[string]string{"K": "other"})
 	require.Error(t, err)
-	got, err := store.Read(ctx, "blog", "c1", "NEW")
+	assert.NotContains(t, err.Error(), store.Dir())
+	got, err := store.Read(ctx, "blog", "c1", "K")
 	require.NoError(t, err)
-	assert.Equal(t, "y", got)
+	assert.Equal(t, "live", got, "a live container's files stay intact")
 }
 func TestStore_RemoveAndList(t *testing.T) {

M internal/boundaries/out/secret_files.go +3 -2


@@ -8,8 +8,9 @@ type SecretFileSet struct{ App, Container string }
8	8		// SecretFileStore persists per-container secret files on the host. Values
9	9		// cross this boundary in memory only; nothing is logged.
10	10		type SecretFileStore interface {
11		-	// Write replaces the container's directory with one 0444 file per key
12		-	// and returns key -> absolute host path.
	11	+	// Write creates the container's directory with one 0444 file per key
	12	+	// and returns key -> absolute host path. It fails when the directory
	13	+	// already exists, so another container's files are never replaced.
13	14		Write(ctx context.Context, app, container string, values map[string]string) (map[string]string, error)
14	15		// Read returns one file's content.
15	16		Read(ctx context.Context, app, container, key string) (string, error)

 // SecretFileStore persists per-container secret files on the host. Values
 // cross this boundary in memory only; nothing is logged.
 type SecretFileStore interface {
-	// Write replaces the container's directory with one 0444 file per key
+	// Write creates the container's directory with one 0444 file per key
-	// and returns key -> absolute host path.
+	// and returns key -> absolute host path. It fails when the directory
+	// already exists, so another container's files are never replaced.
 	Write(ctx context.Context, app, container string, values map[string]string) (map[string]string, error)
 	// Read returns one file's content.
 	Read(ctx context.Context, app, container, key string) (string, error)

M internal/domain/app_secret_files.go +5 -3


@@ -22,7 +22,12 @@ func (s AppService) SecretAsFile(key string) bool {
22	22		}
23	23
24	24		// validateSecretFiles checks the file-delivered secret keys of one service.
	25	+	// <KEY>_FILE collisions with public env are checked by
	26	+	// checkEnvSecretKeyDisjoint, which sees both app and service env.
25	27		func (s *AppService) validateSecretFiles() error {
	28	+	if !slices.IsSorted(s.SecretFiles) {
	29	+	return fmt.Errorf("%w: service %q secret file keys must be sorted", ErrInvalidAppSpec, s.Name)
	30	+	}
26	31		seen := make(map[string]struct{}, len(s.SecretFiles))
27	32		for _, key := range s.SecretFiles {
28	33		if _, ok := s.Secrets[key]; !ok {

@@ -36,9 +41,6 @@ func (s *AppService) validateSecretFiles() error {
36	41		if _, ok := s.Secrets[fileKey]; ok {
37	42		return fmt.Errorf("%w: service %q secret key %q collides with a secret file key", ErrInvalidAppSpec, s.Name, fileKey)
38	43		}
39		-	if _, ok := s.Env[fileKey]; ok {
40		-	return fmt.Errorf("%w: service %q env key %q collides with a secret file key", ErrInvalidAppSpec, s.Name, fileKey)
41		-	}
42	44		}
43	45		if len(s.SecretFiles) == 0 {
44	46		return nil

 }
 // validateSecretFiles checks the file-delivered secret keys of one service.
+// <KEY>_FILE collisions with public env are checked by
+// checkEnvSecretKeyDisjoint, which sees both app and service env.
 func (s *AppService) validateSecretFiles() error {
+	if !slices.IsSorted(s.SecretFiles) {
+		return fmt.Errorf("%w: service %q secret file keys must be sorted", ErrInvalidAppSpec, s.Name)
+	}
 	seen := make(map[string]struct{}, len(s.SecretFiles))
 	for _, key := range s.SecretFiles {
 		if _, ok := s.Secrets[key]; !ok {
 		if _, ok := s.Secrets[fileKey]; ok {
 			return fmt.Errorf("%w: service %q secret key %q collides with a secret file key", ErrInvalidAppSpec, s.Name, fileKey)
 		}
-		if _, ok := s.Env[fileKey]; ok {
-			return fmt.Errorf("%w: service %q env key %q collides with a secret file key", ErrInvalidAppSpec, s.Name, fileKey)
-		}
 	}
 	if len(s.SecretFiles) == 0 {
 		return nil

M internal/domain/app_secret_files_test.go +4 -0


@@ -30,6 +30,10 @@ func TestAppSpec_ValidateSecretFilesErrors(t *testing.T) {
30	30		wantErr string
31	31		}{
32	32		{"not a secret", func(s *domain.AppSpec) { s.Services[0].SecretFiles = []string{"OTHER"} }, "not a declared secret"},
	33	+	{"unsorted", func(s *domain.AppSpec) {
	34	+	s.Services[0].Secrets["API"] = "api"
	35	+	s.Services[0].SecretFiles = []string{"DATABASE_URL", "API"}
	36	+	}, "must be sorted"},
33	37		{"duplicate", func(s *domain.AppSpec) {
34	38		s.Services[0].SecretFiles = []string{"DATABASE_URL", "DATABASE_URL"}
35	39		}, "duplicated"},


@@ -30,6 +30,10 @@ func TestAppSpec_ValidateSecretFilesErrors(t *testing.T) {
30	wantErr string	30		wantErr string
31	}{	31		}{
32	{"not a secret", func(s *domain.AppSpec) { s.Services[0].SecretFiles = []string{"OTHER"} }, "not a declared secret"},	32		{"not a secret", func(s *domain.AppSpec) { s.Services[0].SecretFiles = []string{"OTHER"} }, "not a declared secret"},
		33	+	{"unsorted", func(s *domain.AppSpec) {
		34	+	s.Services[0].Secrets["API"] = "api"
		35	+	s.Services[0].SecretFiles = []string{"DATABASE_URL", "API"}
		36	+	}, "must be sorted"},
33	{"duplicate", func(s *domain.AppSpec) {	37		{"duplicate", func(s *domain.AppSpec) {
34	s.Services[0].SecretFiles = []string{"DATABASE_URL", "DATABASE_URL"}	38		s.Services[0].SecretFiles = []string{"DATABASE_URL", "DATABASE_URL"}
35	}, "duplicated"},	39		}, "duplicated"},

M internal/usecase/deployment/secret_files.go +3 -1


@@ -4,6 +4,8 @@ import (
4	4		"context"
5	5		"errors"
6	6		"fmt"
	7	+
	8	+	"github.com/bnema/gordon/internal/boundaries/out"
7	9		)
8	10
9	11		// sweepAppSecretFiles removes the secret file directories of app whose

@@ -33,7 +35,7 @@ func (s *Service) sweepSecretFiles(ctx context.Context, filter string) error {
33	35		return fmt.Errorf("deployment: list secret files: %w", err)
34	36		}
35	37		if filter != "" {
36		-	kept := sets[:0]
	38	+	kept := make([]out.SecretFileSet, 0, len(sets))
37	39		for _, set := range sets {
38	40		if set.App == filter {
39	41		kept = append(kept, set)

 	"context"
 	"errors"
 	"fmt"
+	"github.com/bnema/gordon/internal/boundaries/out"
 )
 // sweepAppSecretFiles removes the secret file directories of app whose
 		return fmt.Errorf("deployment: list secret files: %w", err)
 	}
 	if filter != "" {
-		kept := sets[:0]
+		kept := make([]out.SecretFileSet, 0, len(sets))
 		for _, set := range sets {
 			if set.App == filter {
 				kept = append(kept, set)

M internal/usecase/deployment/secret_files_internal_test.go +51 -0


@@ -102,6 +102,34 @@ func TestCreateWithSecretFiles_CreateFailureRemovesFiles(t *testing.T) {
102	102		assert.NotContains(t, err.Error(), path)
103	103		}
104	104
	105	+	func TestCreateWithSecretFiles_WriteFailureSkipsCreate(t *testing.T) {
	106	+	store := outmocks.NewMockSecretFileStore(t)
	107	+	runtime := outmocks.NewMockContainerRuntime(t)
	108	+	svc := NewService(Deps{Runtime: runtime, SecretFiles: store}, zerowrap.Default())
	109	+
	110	+	store.EXPECT().Write(mock.Anything, "blog", "c", mock.Anything).Return(nil, errors.New("disk full")).Once()
	111	+
	112	+	_, err := svc.createWithSecretFiles(context.Background(), "blog", "web", &domain.ContainerConfig{Name: "c"}, map[string]string{"B": testFileSecretValue})
	113	+	require.Error(t, err)
	114	+	assert.NotContains(t, err.Error(), testFileSecretValue)
	115	+	runtime.AssertNotCalled(t, "CreateContainer", mock.Anything, mock.Anything)
	116	+	}
	117	+
	118	+	func TestCreateWithSecretFiles_CleanupFailureKeepsCreateError(t *testing.T) {
	119	+	store := outmocks.NewMockSecretFileStore(t)
	120	+	runtime := outmocks.NewMockContainerRuntime(t)
	121	+	svc := NewService(Deps{Runtime: runtime, SecretFiles: store}, zerowrap.Default())
	122	+
	123	+	store.EXPECT().Write(mock.Anything, "blog", "c", mock.Anything).
	124	+	Return(map[string]string{"B": testSecretFileBase + "/blog/c/B"}, nil).Once()
	125	+	runtime.EXPECT().CreateContainer(mock.Anything, mock.Anything).Return(nil, errors.New("create failed")).Once()
	126	+	store.EXPECT().Remove(mock.Anything, "blog", "c").Return(errors.New("busy")).Once()
	127	+
	128	+	_, err := svc.createWithSecretFiles(context.Background(), "blog", "web", &domain.ContainerConfig{Name: "c"}, map[string]string{"B": "v"})
	129	+	require.Error(t, err)
	130	+	assert.Contains(t, err.Error(), "runtime error redacted")
	131	+	}
	132	+
105	133		func TestCreateWithSecretFiles_NilStoreRefuses(t *testing.T) {
106	134		runtime := outmocks.NewMockContainerRuntime(t)
107	135		svc := NewService(Deps{Runtime: runtime}, zerowrap.Default())

@@ -209,6 +237,29 @@ func TestSweepSecretFiles(t *testing.T) {
209	237		svc := NewService(Deps{}, zerowrap.Default())
210	238		require.NoError(t, svc.SweepSecretFiles(context.Background()))
211	239		})
	240	+
	241	+	t.Run("runtime list error removes nothing", func(t *testing.T) {
	242	+	store := outmocks.NewMockSecretFileStore(t)
	243	+	runtime := outmocks.NewMockContainerRuntime(t)
	244	+	store.EXPECT().List(mock.Anything).Return(sets, nil).Once()
	245	+	runtime.EXPECT().ListContainers(mock.Anything, true).Return(nil, errors.New("daemon down")).Once()
	246	+
	247	+	svc := NewService(Deps{Runtime: runtime, SecretFiles: store}, zerowrap.Default())
	248	+	require.Error(t, svc.SweepSecretFiles(context.Background()))
	249	+	store.AssertNotCalled(t, "Remove", mock.Anything, mock.Anything, mock.Anything)
	250	+	})
	251	+
	252	+	t.Run("partial remove failure continues", func(t *testing.T) {
	253	+	store := outmocks.NewMockSecretFileStore(t)
	254	+	runtime := outmocks.NewMockContainerRuntime(t)
	255	+	store.EXPECT().List(mock.Anything).Return(sets, nil).Once()
	256	+	runtime.EXPECT().ListContainers(mock.Anything, true).Return(live, nil).Once()
	257	+	store.EXPECT().Remove(mock.Anything, "blog", "gordon-blog--web--a").Return(errors.New("busy")).Once()
	258	+	store.EXPECT().Remove(mock.Anything, "other", "gordon-other--x--c").Return(nil).Once()
	259	+
	260	+	svc := NewService(Deps{Runtime: runtime, SecretFiles: store}, zerowrap.Default())
	261	+	require.Error(t, svc.SweepSecretFiles(context.Background()))
	262	+	})
212	263		}
213	264
214	265		func TestRetireContainer_SweepsSecretFiles(t *testing.T) {


@@ -102,6 +102,34 @@ func TestCreateWithSecretFiles_CreateFailureRemovesFiles(t *testing.T) {
102	assert.NotContains(t, err.Error(), path)	102		assert.NotContains(t, err.Error(), path)
103	}	103		}
104		104
		105	+	func TestCreateWithSecretFiles_WriteFailureSkipsCreate(t *testing.T) {
		106	+	store := outmocks.NewMockSecretFileStore(t)
		107	+	runtime := outmocks.NewMockContainerRuntime(t)
		108	+	svc := NewService(Deps{Runtime: runtime, SecretFiles: store}, zerowrap.Default())
		109	+
		110	+	store.EXPECT().Write(mock.Anything, "blog", "c", mock.Anything).Return(nil, errors.New("disk full")).Once()
		111	+
		112	+	_, err := svc.createWithSecretFiles(context.Background(), "blog", "web", &domain.ContainerConfig{Name: "c"}, map[string]string{"B": testFileSecretValue})
		113	+	require.Error(t, err)
		114	+	assert.NotContains(t, err.Error(), testFileSecretValue)
		115	+	runtime.AssertNotCalled(t, "CreateContainer", mock.Anything, mock.Anything)
		116	+	}
		117	+
		118	+	func TestCreateWithSecretFiles_CleanupFailureKeepsCreateError(t *testing.T) {
		119	+	store := outmocks.NewMockSecretFileStore(t)
		120	+	runtime := outmocks.NewMockContainerRuntime(t)
		121	+	svc := NewService(Deps{Runtime: runtime, SecretFiles: store}, zerowrap.Default())
		122	+
		123	+	store.EXPECT().Write(mock.Anything, "blog", "c", mock.Anything).
		124	+	Return(map[string]string{"B": testSecretFileBase + "/blog/c/B"}, nil).Once()
		125	+	runtime.EXPECT().CreateContainer(mock.Anything, mock.Anything).Return(nil, errors.New("create failed")).Once()
		126	+	store.EXPECT().Remove(mock.Anything, "blog", "c").Return(errors.New("busy")).Once()
		127	+
		128	+	_, err := svc.createWithSecretFiles(context.Background(), "blog", "web", &domain.ContainerConfig{Name: "c"}, map[string]string{"B": "v"})
		129	+	require.Error(t, err)
		130	+	assert.Contains(t, err.Error(), "runtime error redacted")
		131	+	}
		132	+
105	func TestCreateWithSecretFiles_NilStoreRefuses(t *testing.T) {	133		func TestCreateWithSecretFiles_NilStoreRefuses(t *testing.T) {
106	runtime := outmocks.NewMockContainerRuntime(t)	134		runtime := outmocks.NewMockContainerRuntime(t)
107	svc := NewService(Deps{Runtime: runtime}, zerowrap.Default())	135		svc := NewService(Deps{Runtime: runtime}, zerowrap.Default())

@@ -209,6 +237,29 @@ func TestSweepSecretFiles(t *testing.T) {
209	svc := NewService(Deps{}, zerowrap.Default())	237		svc := NewService(Deps{}, zerowrap.Default())
210	require.NoError(t, svc.SweepSecretFiles(context.Background()))	238		require.NoError(t, svc.SweepSecretFiles(context.Background()))
211	})	239		})
		240	+
		241	+	t.Run("runtime list error removes nothing", func(t *testing.T) {
		242	+	store := outmocks.NewMockSecretFileStore(t)
		243	+	runtime := outmocks.NewMockContainerRuntime(t)
		244	+	store.EXPECT().List(mock.Anything).Return(sets, nil).Once()
		245	+	runtime.EXPECT().ListContainers(mock.Anything, true).Return(nil, errors.New("daemon down")).Once()
		246	+
		247	+	svc := NewService(Deps{Runtime: runtime, SecretFiles: store}, zerowrap.Default())
		248	+	require.Error(t, svc.SweepSecretFiles(context.Background()))
		249	+	store.AssertNotCalled(t, "Remove", mock.Anything, mock.Anything, mock.Anything)
		250	+	})
		251	+
		252	+	t.Run("partial remove failure continues", func(t *testing.T) {
		253	+	store := outmocks.NewMockSecretFileStore(t)
		254	+	runtime := outmocks.NewMockContainerRuntime(t)
		255	+	store.EXPECT().List(mock.Anything).Return(sets, nil).Once()
		256	+	runtime.EXPECT().ListContainers(mock.Anything, true).Return(live, nil).Once()
		257	+	store.EXPECT().Remove(mock.Anything, "blog", "gordon-blog--web--a").Return(errors.New("busy")).Once()
		258	+	store.EXPECT().Remove(mock.Anything, "other", "gordon-other--x--c").Return(nil).Once()
		259	+
		260	+	svc := NewService(Deps{Runtime: runtime, SecretFiles: store}, zerowrap.Default())
		261	+	require.Error(t, svc.SweepSecretFiles(context.Background()))
		262	+	})
212	}	263		}
213		264
214	func TestRetireContainer_SweepsSecretFiles(t *testing.T) {	265		func TestRetireContainer_SweepsSecretFiles(t *testing.T) {