Merge pull request 'fix(apps): compare app env against the activated revision in diffs' (#23) from fix/app-env-diff into main
9 files · +284 -38
M docs/config/apps.md +2 -2
| @@ -120,8 +120,8 @@ A deploy recreates a service whose image digest, spec, environment, or secret va | |||
| 120 | 120 | ||
| 121 | 121 | ## Environment and Secrets | |
| 122 | 122 | ||
| 123 | - | - `[env]` is app-wide public env injected into all services. Each key must be disjoint from every `[services.<name>.secrets]` key in the app. A change to `[env]` changes every service, so the next deploy recreates all of them, databases included. Prefer `[services.<name>.env]` for values only one service needs. | |
| 124 | - | - `[services.<name>.env]` is public env for that service only. A key here overrides the same key in `[env]` for this service. Each key must be disjoint from the same service's `[services.<name>.secrets]` keys. Values follow the `[env]` rules: no newlines, 1 byte to 64 KiB, no secret references. Values are stored in revisions and shown in plain text, so use secrets for anything sensitive. A change shows as `service/<name>/env` in diffs and applies on the next deploy. | |
| 123 | + | - `[env]` is app-wide public env injected into all services. Each key must be disjoint from every `[services.<name>.secrets]` key in the app. A change to `[env]` changes every service, so the next deploy recreates all of them, databases included. A change shows as `env/<KEY>` in diffs, one entry per added, removed, or changed key (names only, never values), and blocks `apps deploy --service`. Prefer `[services.<name>.env]` for values only one service needs. | |
| 124 | + | - `[services.<name>.env]` is public env for that service only. A key here overrides the same key in `[env]` for this service. Each key must be disjoint from the same service's `[services.<name>.secrets]` keys. Values follow the `[env]` rules: no newlines, 1 byte to 64 KiB, no secret references. Values are stored in revisions and shown in plain text, so use secrets for anything sensitive. A change shows as `service/<name>/env/<KEY>` in diffs and applies on the next deploy. | |
| 125 | 125 | - `[services.<name>.secrets]` maps ENV var name to service-local secret name. Values are written with `gordon apps secrets set` and stay in pass under `gordon/apps/<uuid>/<service>/<name>`. Running containers keep the values they were created with; `gordon apps deploy` applies new values. `restart` does not. | |
| 126 | 126 | - A secret entry is either a secret name string (env delivery) or a table `{ name = "...", as = "env" | "file" }`. `as` defaults to `env`; any other key or value is rejected. | |
| 127 | 127 | - `as = "file"` delivers the value as a read-only file (mode `0444`) at `/run/secrets/<ENV>` and sets `<ENV>_FILE=/run/secrets/<ENV>` instead of `<ENV>`. The value never appears in the container environment, `docker inspect`, or `/proc/1/environ`. Use it for images that support the `*_FILE` convention. | |
| @@ -120,8 +120,8 @@ A deploy recreates a service whose image digest, spec, environment, or secret va | |||||
| 120 | 120 | ||||
| 121 | ## Environment and Secrets | 121 | ## Environment and Secrets | ||
| 122 | 122 | ||||
| 123 | - | - `[env]` is app-wide public env injected into all services. Each key must be disjoint from every `[services.<name>.secrets]` key in the app. A change to `[env]` changes every service, so the next deploy recreates all of them, databases included. Prefer `[services.<name>.env]` for values only one service needs. | 123 | + | - `[env]` is app-wide public env injected into all services. Each key must be disjoint from every `[services.<name>.secrets]` key in the app. A change to `[env]` changes every service, so the next deploy recreates all of them, databases included. A change shows as `env/<KEY>` in diffs, one entry per added, removed, or changed key (names only, never values), and blocks `apps deploy --service`. Prefer `[services.<name>.env]` for values only one service needs. |
| 124 | - | - `[services.<name>.env]` is public env for that service only. A key here overrides the same key in `[env]` for this service. Each key must be disjoint from the same service's `[services.<name>.secrets]` keys. Values follow the `[env]` rules: no newlines, 1 byte to 64 KiB, no secret references. Values are stored in revisions and shown in plain text, so use secrets for anything sensitive. A change shows as `service/<name>/env` in diffs and applies on the next deploy. | 124 | + | - `[services.<name>.env]` is public env for that service only. A key here overrides the same key in `[env]` for this service. Each key must be disjoint from the same service's `[services.<name>.secrets]` keys. Values follow the `[env]` rules: no newlines, 1 byte to 64 KiB, no secret references. Values are stored in revisions and shown in plain text, so use secrets for anything sensitive. A change shows as `service/<name>/env/<KEY>` in diffs and applies on the next deploy. |
| 125 | - `[services.<name>.secrets]` maps ENV var name to service-local secret name. Values are written with `gordon apps secrets set` and stay in pass under `gordon/apps/<uuid>/<service>/<name>`. Running containers keep the values they were created with; `gordon apps deploy` applies new values. `restart` does not. | 125 | - `[services.<name>.secrets]` maps ENV var name to service-local secret name. Values are written with `gordon apps secrets set` and stay in pass under `gordon/apps/<uuid>/<service>/<name>`. Running containers keep the values they were created with; `gordon apps deploy` applies new values. `restart` does not. | ||
| 126 | - A secret entry is either a secret name string (env delivery) or a table `{ name = "...", as = "env" | "file" }`. `as` defaults to `env`; any other key or value is rejected. | 126 | - A secret entry is either a secret name string (env delivery) or a table `{ name = "...", as = "env" | "file" }`. `as` defaults to `env`; any other key or value is rejected. | ||
| 127 | - `as = "file"` delivers the value as a read-only file (mode `0444`) at `/run/secrets/<ENV>` and sets `<ENV>_FILE=/run/secrets/<ENV>` instead of `<ENV>`. The value never appears in the container environment, `docker inspect`, or `/proc/1/environ`. Use it for images that support the `*_FILE` convention. | 127 | - `as = "file"` delivers the value as a read-only file (mode `0444`) at `/run/secrets/<ENV>` and sets `<ENV>_FILE=/run/secrets/<ENV>` instead of `<ENV>`. The value never appears in the container environment, `docker inspect`, or `/proc/1/environ`. Use it for images that support the `*_FILE` convention. | ||
M internal/boundaries/out/app_state.go +3 -0
| @@ -151,6 +151,9 @@ type AppApplyStore interface { | |||
| 151 | 151 | LoadCheckpoint(ctx context.Context) (domain.AppStoreCheckpoint, error) | |
| 152 | 152 | LoadDesired(ctx context.Context, app string) (domain.AppDesiredRevision, bool, error) | |
| 153 | 153 | LoadActive(ctx context.Context, app string) (domain.AppActive, bool, error) | |
| 154 | + | // LoadRevision reads the app env of the revisions ACTIVE services were | |
| 155 | + | // activated from, so diffs compare against what actually runs. | |
| 156 | + | LoadRevision(ctx context.Context, app, revision string) (domain.AppDesiredRevision, error) | |
| 154 | 157 | AcceptApply(ctx context.Context, intent domain.AppApplyIntent) error | |
| 155 | 158 | } | |
| 156 | 159 | ||
| @@ -151,6 +151,9 @@ type AppApplyStore interface { | |||||
| 151 | LoadCheckpoint(ctx context.Context) (domain.AppStoreCheckpoint, error) | 151 | LoadCheckpoint(ctx context.Context) (domain.AppStoreCheckpoint, error) | ||
| 152 | LoadDesired(ctx context.Context, app string) (domain.AppDesiredRevision, bool, error) | 152 | LoadDesired(ctx context.Context, app string) (domain.AppDesiredRevision, bool, error) | ||
| 153 | LoadActive(ctx context.Context, app string) (domain.AppActive, bool, error) | 153 | LoadActive(ctx context.Context, app string) (domain.AppActive, bool, error) | ||
| 154 | + | // LoadRevision reads the app env of the revisions ACTIVE services were | |||
| 155 | + | // activated from, so diffs compare against what actually runs. | |||
| 156 | + | LoadRevision(ctx context.Context, app, revision string) (domain.AppDesiredRevision, error) | |||
| 154 | AcceptApply(ctx context.Context, intent domain.AppApplyIntent) error | 157 | AcceptApply(ctx context.Context, intent domain.AppApplyIntent) error | ||
| 155 | } | 158 | } | ||
| 156 | 159 | ||||
M internal/domain/app_spec.go +78 -14
| @@ -1013,9 +1013,7 @@ func DiffAppSpec(desired, effective AppSpec) AppDiff { | |||
| 1013 | 1013 | } | |
| 1014 | 1014 | diff.Changed = append(diff.Changed, diffService(name, desiredSvc, effectiveSvc)...) | |
| 1015 | 1015 | } | |
| 1016 | - | if !equalStringMaps(desired.Env, effective.Env) { | |
| 1017 | - | diff.Changed = append(diff.Changed, "env") | |
| 1018 | - | } | |
| 1016 | + | diff.Changed = append(diff.Changed, diffEnvKeys("env/", desired.Env, effective.Env)...) | |
| 1019 | 1017 | diff.Changed = append(diff.Changed, diffNetworks(desired.Networks, effective.Networks)...) | |
| 1020 | 1018 | sort.Strings(diff.Added) | |
| 1021 | 1019 | sort.Strings(diff.Removed) | |
| @@ -1097,9 +1095,7 @@ func diffService(name string, desired, effective AppService) []string { | |||
| 1097 | 1095 | } else if interfacesChanged(desired, effective) { | |
| 1098 | 1096 | changed = append(changed, "service/"+name+"/interfaces") | |
| 1099 | 1097 | } | |
| 1100 | - | if !equalStringMaps(desired.Env, effective.Env) { | |
| 1101 | - | changed = append(changed, "service/"+name+"/env") | |
| 1102 | - | } | |
| 1098 | + | changed = append(changed, diffEnvKeys("service/"+name+"/env/", desired.Env, effective.Env)...) | |
| 1103 | 1099 | changed = appendSecretChanges(changed, desired, effective) | |
| 1104 | 1100 | if !equalVolumes(desired.Volumes, effective.Volumes) { | |
| 1105 | 1101 | changed = append(changed, "service/"+name+"/volumes") | |
| @@ -1181,17 +1177,85 @@ func httpInterfacesEqual(a, b AppHTTPInterface) bool { | |||
| 1181 | 1177 | return a == b | |
| 1182 | 1178 | } | |
| 1183 | 1179 | ||
| 1184 | - | // equalStringMaps compares string maps. | |
| 1185 | - | func equalStringMaps(a, b map[string]string) bool { | |
| 1186 | - | if len(a) != len(b) { | |
| 1187 | - | return false | |
| 1180 | + | // diffEnvKeys returns prefix+KEY for every key added, removed, or changed | |
| 1181 | + | // between two env maps. Only key names appear, never values. | |
| 1182 | + | func diffEnvKeys(prefix string, desired, effective map[string]string) []string { | |
| 1183 | + | var changed []string | |
| 1184 | + | for key, value := range desired { | |
| 1185 | + | if previous, ok := effective[key]; !ok || previous != value { | |
| 1186 | + | changed = append(changed, prefix+key) | |
| 1187 | + | } | |
| 1188 | 1188 | } | |
| 1189 | - | for k, v := range a { | |
| 1190 | - | if b[k] != v { | |
| 1191 | - | return false | |
| 1189 | + | for key := range effective { | |
| 1190 | + | if _, ok := desired[key]; !ok { | |
| 1191 | + | changed = append(changed, prefix+key) | |
| 1192 | + | } | |
| 1193 | + | } | |
| 1194 | + | sort.Strings(changed) | |
| 1195 | + | return changed | |
| 1196 | + | } | |
| 1197 | + | ||
| 1198 | + | // ActiveAppSpec rebuilds the effective AppSpec view of ACTIVE for diffing. | |
| 1199 | + | // ACTIVE stores per-service specs but not the app-level [env], so | |
| 1200 | + | // revisionEnv returns the app env of the revision a service was activated | |
| 1201 | + | // from (nil when that revision is gone). When services run from different | |
| 1202 | + | // revisions, every key that is stale in any of them is reported, so a diff | |
| 1203 | + | // never hides a service still running a stale app env. | |
| 1204 | + | func ActiveAppSpec( | |
| 1205 | + | active AppActive, | |
| 1206 | + | desiredEnv map[string]string, | |
| 1207 | + | revisionEnv func(revision string) (map[string]string, error), | |
| 1208 | + | ) (AppSpec, error) { | |
| 1209 | + | spec := AppSpec{Name: active.App, Env: map[string]string{}, Networks: append([]AppSharedNetwork(nil), active.Networks...)} | |
| 1210 | + | names := slices.Sorted(maps.Keys(active.Services)) | |
| 1211 | + | for _, name := range names { | |
| 1212 | + | svc := active.Services[name].Spec | |
| 1213 | + | svc.Name = name | |
| 1214 | + | spec.Services = append(spec.Services, svc) | |
| 1215 | + | } | |
| 1216 | + | if len(names) == 0 { | |
| 1217 | + | return spec, nil | |
| 1218 | + | } | |
| 1219 | + | spec.Env = maps.Clone(desiredEnv) | |
| 1220 | + | if spec.Env == nil { | |
| 1221 | + | spec.Env = map[string]string{} | |
| 1222 | + | } | |
| 1223 | + | checked := map[string]struct{}{} | |
| 1224 | + | for _, name := range names { | |
| 1225 | + | revision := active.Services[name].EffectiveRevision | |
| 1226 | + | if _, done := checked[revision]; done { | |
| 1227 | + | continue | |
| 1228 | + | } | |
| 1229 | + | checked[revision] = struct{}{} | |
| 1230 | + | var env map[string]string | |
| 1231 | + | if revision != "" { | |
| 1232 | + | var err error | |
| 1233 | + | if env, err = revisionEnv(revision); err != nil { | |
| 1234 | + | return AppSpec{}, err | |
| 1235 | + | } | |
| 1236 | + | } | |
| 1237 | + | mergeStaleEnv(spec.Env, desiredEnv, env) | |
| 1238 | + | } | |
| 1239 | + | return spec, nil | |
| 1240 | + | } | |
| 1241 | + | ||
| 1242 | + | // mergeStaleEnv folds one running env into effective, which starts as a | |
| 1243 | + | // copy of desired. Every key whose running value differs from desired is | |
| 1244 | + | // overwritten (or deleted when not running), so it stays different from | |
| 1245 | + | // desired once any revision marks it stale; matching keys are untouched. | |
| 1246 | + | func mergeStaleEnv(effective, desired, running map[string]string) { | |
| 1247 | + | for key, value := range desired { | |
| 1248 | + | if runningValue, ok := running[key]; !ok { | |
| 1249 | + | delete(effective, key) | |
| 1250 | + | } else if runningValue != value { | |
| 1251 | + | effective[key] = runningValue | |
| 1252 | + | } | |
| 1253 | + | } | |
| 1254 | + | for key, value := range running { | |
| 1255 | + | if _, ok := desired[key]; !ok { | |
| 1256 | + | effective[key] = value | |
| 1192 | 1257 | } | |
| 1193 | 1258 | } | |
| 1194 | - | return true | |
| 1195 | 1259 | } | |
| 1196 | 1260 | ||
| 1197 | 1261 | // equalVolumes compares volume slices by value. | |
| @@ -1013,9 +1013,7 @@ func DiffAppSpec(desired, effective AppSpec) AppDiff { | |||||
| 1013 | } | 1013 | } | ||
| 1014 | diff.Changed = append(diff.Changed, diffService(name, desiredSvc, effectiveSvc)...) | 1014 | diff.Changed = append(diff.Changed, diffService(name, desiredSvc, effectiveSvc)...) | ||
| 1015 | } | 1015 | } | ||
| 1016 | - | if !equalStringMaps(desired.Env, effective.Env) { | 1016 | + | diff.Changed = append(diff.Changed, diffEnvKeys("env/", desired.Env, effective.Env)...) |
| 1017 | - | diff.Changed = append(diff.Changed, "env") | |||
| 1018 | - | } | |||
| 1019 | diff.Changed = append(diff.Changed, diffNetworks(desired.Networks, effective.Networks)...) | 1017 | diff.Changed = append(diff.Changed, diffNetworks(desired.Networks, effective.Networks)...) | ||
| 1020 | sort.Strings(diff.Added) | 1018 | sort.Strings(diff.Added) | ||
| 1021 | sort.Strings(diff.Removed) | 1019 | sort.Strings(diff.Removed) | ||
| @@ -1097,9 +1095,7 @@ func diffService(name string, desired, effective AppService) []string { | |||||
| 1097 | } else if interfacesChanged(desired, effective) { | 1095 | } else if interfacesChanged(desired, effective) { | ||
| 1098 | changed = append(changed, "service/"+name+"/interfaces") | 1096 | changed = append(changed, "service/"+name+"/interfaces") | ||
| 1099 | } | 1097 | } | ||
| 1100 | - | if !equalStringMaps(desired.Env, effective.Env) { | 1098 | + | changed = append(changed, diffEnvKeys("service/"+name+"/env/", desired.Env, effective.Env)...) |
| 1101 | - | changed = append(changed, "service/"+name+"/env") | |||
| 1102 | - | } | |||
| 1103 | changed = appendSecretChanges(changed, desired, effective) | 1099 | changed = appendSecretChanges(changed, desired, effective) | ||
| 1104 | if !equalVolumes(desired.Volumes, effective.Volumes) { | 1100 | if !equalVolumes(desired.Volumes, effective.Volumes) { | ||
| 1105 | changed = append(changed, "service/"+name+"/volumes") | 1101 | changed = append(changed, "service/"+name+"/volumes") | ||
| @@ -1181,17 +1177,85 @@ func httpInterfacesEqual(a, b AppHTTPInterface) bool { | |||||
| 1181 | return a == b | 1177 | return a == b | ||
| 1182 | } | 1178 | } | ||
| 1183 | 1179 | ||||
| 1184 | - | // equalStringMaps compares string maps. | 1180 | + | // diffEnvKeys returns prefix+KEY for every key added, removed, or changed |
| 1185 | - | func equalStringMaps(a, b map[string]string) bool { | 1181 | + | // between two env maps. Only key names appear, never values. |
| 1186 | - | if len(a) != len(b) { | 1182 | + | func diffEnvKeys(prefix string, desired, effective map[string]string) []string { |
| 1187 | - | return false | 1183 | + | var changed []string |
| 1184 | + | for key, value := range desired { | |||
| 1185 | + | if previous, ok := effective[key]; !ok || previous != value { | |||
| 1186 | + | changed = append(changed, prefix+key) | |||
| 1187 | + | } | |||
| 1188 | } | 1188 | } | ||
| 1189 | - | for k, v := range a { | 1189 | + | for key := range effective { |
| 1190 | - | if b[k] != v { | 1190 | + | if _, ok := desired[key]; !ok { |
| 1191 | - | return false | 1191 | + | changed = append(changed, prefix+key) |
| 1192 | + | } | |||
| 1193 | + | } | |||
| 1194 | + | sort.Strings(changed) | |||
| 1195 | + | return changed | |||
| 1196 | + | } | |||
| 1197 | + | ||||
| 1198 | + | // ActiveAppSpec rebuilds the effective AppSpec view of ACTIVE for diffing. | |||
| 1199 | + | // ACTIVE stores per-service specs but not the app-level [env], so | |||
| 1200 | + | // revisionEnv returns the app env of the revision a service was activated | |||
| 1201 | + | // from (nil when that revision is gone). When services run from different | |||
| 1202 | + | // revisions, every key that is stale in any of them is reported, so a diff | |||
| 1203 | + | // never hides a service still running a stale app env. | |||
| 1204 | + | func ActiveAppSpec( | |||
| 1205 | + | active AppActive, | |||
| 1206 | + | desiredEnv map[string]string, | |||
| 1207 | + | revisionEnv func(revision string) (map[string]string, error), | |||
| 1208 | + | ) (AppSpec, error) { | |||
| 1209 | + | spec := AppSpec{Name: active.App, Env: map[string]string{}, Networks: append([]AppSharedNetwork(nil), active.Networks...)} | |||
| 1210 | + | names := slices.Sorted(maps.Keys(active.Services)) | |||
| 1211 | + | for _, name := range names { | |||
| 1212 | + | svc := active.Services[name].Spec | |||
| 1213 | + | svc.Name = name | |||
| 1214 | + | spec.Services = append(spec.Services, svc) | |||
| 1215 | + | } | |||
| 1216 | + | if len(names) == 0 { | |||
| 1217 | + | return spec, nil | |||
| 1218 | + | } | |||
| 1219 | + | spec.Env = maps.Clone(desiredEnv) | |||
| 1220 | + | if spec.Env == nil { | |||
| 1221 | + | spec.Env = map[string]string{} | |||
| 1222 | + | } | |||
| 1223 | + | checked := map[string]struct{}{} | |||
| 1224 | + | for _, name := range names { | |||
| 1225 | + | revision := active.Services[name].EffectiveRevision | |||
| 1226 | + | if _, done := checked[revision]; done { | |||
| 1227 | + | continue | |||
| 1228 | + | } | |||
| 1229 | + | checked[revision] = struct{}{} | |||
| 1230 | + | var env map[string]string | |||
| 1231 | + | if revision != "" { | |||
| 1232 | + | var err error | |||
| 1233 | + | if env, err = revisionEnv(revision); err != nil { | |||
| 1234 | + | return AppSpec{}, err | |||
| 1235 | + | } | |||
| 1236 | + | } | |||
| 1237 | + | mergeStaleEnv(spec.Env, desiredEnv, env) | |||
| 1238 | + | } | |||
| 1239 | + | return spec, nil | |||
| 1240 | + | } | |||
| 1241 | + | ||||
| 1242 | + | // mergeStaleEnv folds one running env into effective, which starts as a | |||
| 1243 | + | // copy of desired. Every key whose running value differs from desired is | |||
| 1244 | + | // overwritten (or deleted when not running), so it stays different from | |||
| 1245 | + | // desired once any revision marks it stale; matching keys are untouched. | |||
| 1246 | + | func mergeStaleEnv(effective, desired, running map[string]string) { | |||
| 1247 | + | for key, value := range desired { | |||
| 1248 | + | if runningValue, ok := running[key]; !ok { | |||
| 1249 | + | delete(effective, key) | |||
| 1250 | + | } else if runningValue != value { | |||
| 1251 | + | effective[key] = runningValue | |||
| 1252 | + | } | |||
| 1253 | + | } | |||
| 1254 | + | for key, value := range running { | |||
| 1255 | + | if _, ok := desired[key]; !ok { | |||
| 1256 | + | effective[key] = value | |||
| 1192 | } | 1257 | } | ||
| 1193 | } | 1258 | } | ||
| 1194 | - | return true | |||
| 1195 | } | 1259 | } | ||
| 1196 | 1260 | ||||
| 1197 | // equalVolumes compares volume slices by value. | 1261 | // equalVolumes compares volume slices by value. | ||
M internal/domain/app_spec_test.go +94 -4
| @@ -141,7 +141,7 @@ func TestDiffAppSpec_DeterministicAndRedacted(t *testing.T) { | |||
| 141 | 141 | assert.Contains(t, diff.Changed, "network/shared/added") | |
| 142 | 142 | assert.Contains(t, diff.Changed, "service/web/secret/database-url-v2") | |
| 143 | 143 | assert.Contains(t, diff.Changed, "service/web/secret/database-url") | |
| 144 | - | assert.Contains(t, diff.Changed, "env") | |
| 144 | + | assert.Contains(t, diff.Changed, "env/NEW_KEY") | |
| 145 | 145 | // Sorted output. | |
| 146 | 146 | assert.IsIncreasing(t, diff.Changed) | |
| 147 | 147 | // No secret values leak: only names/paths. | |
| @@ -196,13 +196,103 @@ func TestDiffAppSpec_ServiceEnv(t *testing.T) { | |||
| 196 | 196 | changed := validSpec() | |
| 197 | 197 | changed.Services[0].Env = map[string]string{"LOG": "debug"} | |
| 198 | 198 | ||
| 199 | - | assert.Equal(t, []string{"service/web/env"}, domain.DiffAppSpec(changed, base).Changed) | |
| 200 | - | assert.Equal(t, []string{"service/web/env"}, domain.DiffAppSpec(base, changed).Changed) | |
| 199 | + | assert.Equal(t, []string{"service/web/env/LOG"}, domain.DiffAppSpec(changed, base).Changed) | |
| 200 | + | assert.Equal(t, []string{"service/web/env/LOG"}, domain.DiffAppSpec(base, changed).Changed) | |
| 201 | 201 | assert.Empty(t, domain.DiffAppSpec(changed, changed).Changed) | |
| 202 | 202 | assert.False(t, domain.SameAppService(changed.Services[0], base.Services[0])) | |
| 203 | 203 | ||
| 204 | 204 | changed.Services[0].Env["LOG"] = "info" | |
| 205 | 205 | other := validSpec() | |
| 206 | 206 | other.Services[0].Env = map[string]string{"LOG": "debug"} | |
| 207 | - | assert.Equal(t, []string{"service/web/env"}, domain.DiffAppSpec(changed, other).Changed) | |
| 207 | + | assert.Equal(t, []string{"service/web/env/LOG"}, domain.DiffAppSpec(changed, other).Changed) | |
| 208 | + | } | |
| 209 | + | ||
| 210 | + | func TestDiffAppSpec_EnvListsChangedKeyNamesOnly(t *testing.T) { | |
| 211 | + | before := validSpec() | |
| 212 | + | before.Env = map[string]string{"KEEP": "1", "EDIT": "old-value", "DROP": "x"} | |
| 213 | + | after := validSpec() | |
| 214 | + | after.Env = map[string]string{"KEEP": "1", "EDIT": "new-value", "ADD": "y"} | |
| 215 | + | ||
| 216 | + | diff := domain.DiffAppSpec(after, before) | |
| 217 | + | assert.Equal(t, []string{"env/ADD", "env/DROP", "env/EDIT"}, diff.Changed) | |
| 218 | + | for _, entry := range diff.Changed { | |
| 219 | + | assert.NotContains(t, entry, "value") | |
| 220 | + | } | |
| 221 | + | } | |
| 222 | + | ||
| 223 | + | func TestActiveAppSpec_UsesActivatedRevisionEnv(t *testing.T) { | |
| 224 | + | web := validSpec().Services[0] | |
| 225 | + | worker := web | |
| 226 | + | worker.Name = "worker" | |
| 227 | + | active := domain.AppActive{ | |
| 228 | + | App: "blog", | |
| 229 | + | Services: map[string]domain.AppEffectiveService{ | |
| 230 | + | "web": {EffectiveRevision: "rev-1", Spec: web}, | |
| 231 | + | "worker": {EffectiveRevision: "rev-2", Spec: worker}, | |
| 232 | + | }, | |
| 233 | + | } | |
| 234 | + | envs := map[string]map[string]string{ | |
| 235 | + | "rev-1": {"MODE": "prod"}, | |
| 236 | + | "rev-2": {"MODE": "old"}, | |
| 237 | + | } | |
| 238 | + | load := func(revision string) (map[string]string, error) { return envs[revision], nil } | |
| 239 | + | ||
| 240 | + | // Image-only change: desired env equals what every service runs. | |
| 241 | + | envs["rev-2"] = map[string]string{"MODE": "prod"} | |
| 242 | + | spec, err := domain.ActiveAppSpec(active, map[string]string{"MODE": "prod"}, load) | |
| 243 | + | require.NoError(t, err) | |
| 244 | + | assert.Equal(t, map[string]string{"MODE": "prod"}, spec.Env) | |
| 245 | + | assert.Equal(t, []string{"web", "worker"}, []string{spec.Services[0].Name, spec.Services[1].Name}) | |
| 246 | + | ||
| 247 | + | // One service still runs a stale env: the diff must not hide it. | |
| 248 | + | envs["rev-2"] = map[string]string{"MODE": "old"} | |
| 249 | + | spec, err = domain.ActiveAppSpec(active, map[string]string{"MODE": "prod"}, load) | |
| 250 | + | require.NoError(t, err) | |
| 251 | + | assert.Equal(t, map[string]string{"MODE": "old"}, spec.Env) | |
| 252 | + | ||
| 253 | + | // A garbage-collected revision counts as an empty env. | |
| 254 | + | spec, err = domain.ActiveAppSpec(active, map[string]string{"MODE": "prod"}, func(string) (map[string]string, error) { return nil, nil }) | |
| 255 | + | require.NoError(t, err) | |
| 256 | + | assert.Empty(t, spec.Env) | |
| 257 | + | ||
| 258 | + | _, err = domain.ActiveAppSpec(active, nil, func(string) (map[string]string, error) { return nil, errors.New("boom") }) | |
| 259 | + | require.Error(t, err) | |
| 260 | + | } | |
| 261 | + | ||
| 262 | + | func TestActiveAppSpec_ReportsStaleKeysFromEveryRevision(t *testing.T) { | |
| 263 | + | web := validSpec().Services[0] | |
| 264 | + | worker := web | |
| 265 | + | worker.Name = "worker" | |
| 266 | + | active := domain.AppActive{ | |
| 267 | + | App: "blog", | |
| 268 | + | Services: map[string]domain.AppEffectiveService{ | |
| 269 | + | "web": {EffectiveRevision: "rev-1", Spec: web}, | |
| 270 | + | "worker": {EffectiveRevision: "rev-2", Spec: worker}, | |
| 271 | + | }, | |
| 272 | + | } | |
| 273 | + | envs := map[string]map[string]string{ | |
| 274 | + | "rev-1": {"A": "old", "B": "new", "C": "same"}, | |
| 275 | + | "rev-2": {"A": "new", "C": "same", "D": "gone"}, | |
| 276 | + | } | |
| 277 | + | desired := domain.AppSpec{Name: "blog", Env: map[string]string{"A": "new", "B": "new", "C": "same"}} | |
| 278 | + | desired.Services = []domain.AppService{web, worker} | |
| 279 | + | ||
| 280 | + | effective, err := domain.ActiveAppSpec(active, desired.Env, func(revision string) (map[string]string, error) { | |
| 281 | + | return envs[revision], nil | |
| 282 | + | }) | |
| 283 | + | require.NoError(t, err) | |
| 284 | + | ||
| 285 | + | diff := domain.DiffAppSpec(desired, effective) | |
| 286 | + | assert.Equal(t, []string{"env/A", "env/B", "env/D"}, diff.Changed) | |
| 287 | + | assert.Equal(t, map[string]string{"A": "new", "B": "new", "C": "same"}, desired.Env, "desired env must not be mutated") | |
| 288 | + | } | |
| 289 | + | ||
| 290 | + | func TestActiveAppSpec_EmptyActiveHasEmptyEnv(t *testing.T) { | |
| 291 | + | spec, err := domain.ActiveAppSpec(domain.AppActive{App: "blog"}, map[string]string{"A": "x"}, func(string) (map[string]string, error) { | |
| 292 | + | t.Fatal("no revision should be loaded") | |
| 293 | + | return nil, nil | |
| 294 | + | }) | |
| 295 | + | require.NoError(t, err) | |
| 296 | + | assert.Empty(t, spec.Env) | |
| 297 | + | assert.NotNil(t, spec.Env) | |
| 208 | 298 | } | |
| @@ -141,7 +141,7 @@ func TestDiffAppSpec_DeterministicAndRedacted(t *testing.T) { | |||||
| 141 | assert.Contains(t, diff.Changed, "network/shared/added") | 141 | assert.Contains(t, diff.Changed, "network/shared/added") | ||
| 142 | assert.Contains(t, diff.Changed, "service/web/secret/database-url-v2") | 142 | assert.Contains(t, diff.Changed, "service/web/secret/database-url-v2") | ||
| 143 | assert.Contains(t, diff.Changed, "service/web/secret/database-url") | 143 | assert.Contains(t, diff.Changed, "service/web/secret/database-url") | ||
| 144 | - | assert.Contains(t, diff.Changed, "env") | 144 | + | assert.Contains(t, diff.Changed, "env/NEW_KEY") |
| 145 | // Sorted output. | 145 | // Sorted output. | ||
| 146 | assert.IsIncreasing(t, diff.Changed) | 146 | assert.IsIncreasing(t, diff.Changed) | ||
| 147 | // No secret values leak: only names/paths. | 147 | // No secret values leak: only names/paths. | ||
| @@ -196,13 +196,103 @@ func TestDiffAppSpec_ServiceEnv(t *testing.T) { | |||||
| 196 | changed := validSpec() | 196 | changed := validSpec() | ||
| 197 | changed.Services[0].Env = map[string]string{"LOG": "debug"} | 197 | changed.Services[0].Env = map[string]string{"LOG": "debug"} | ||
| 198 | 198 | ||||
| 199 | - | assert.Equal(t, []string{"service/web/env"}, domain.DiffAppSpec(changed, base).Changed) | 199 | + | assert.Equal(t, []string{"service/web/env/LOG"}, domain.DiffAppSpec(changed, base).Changed) |
| 200 | - | assert.Equal(t, []string{"service/web/env"}, domain.DiffAppSpec(base, changed).Changed) | 200 | + | assert.Equal(t, []string{"service/web/env/LOG"}, domain.DiffAppSpec(base, changed).Changed) |
| 201 | assert.Empty(t, domain.DiffAppSpec(changed, changed).Changed) | 201 | assert.Empty(t, domain.DiffAppSpec(changed, changed).Changed) | ||
| 202 | assert.False(t, domain.SameAppService(changed.Services[0], base.Services[0])) | 202 | assert.False(t, domain.SameAppService(changed.Services[0], base.Services[0])) | ||
| 203 | 203 | ||||
| 204 | changed.Services[0].Env["LOG"] = "info" | 204 | changed.Services[0].Env["LOG"] = "info" | ||
| 205 | other := validSpec() | 205 | other := validSpec() | ||
| 206 | other.Services[0].Env = map[string]string{"LOG": "debug"} | 206 | other.Services[0].Env = map[string]string{"LOG": "debug"} | ||
| 207 | - | assert.Equal(t, []string{"service/web/env"}, domain.DiffAppSpec(changed, other).Changed) | 207 | + | assert.Equal(t, []string{"service/web/env/LOG"}, domain.DiffAppSpec(changed, other).Changed) |
| 208 | + | } | |||
| 209 | + | ||||
| 210 | + | func TestDiffAppSpec_EnvListsChangedKeyNamesOnly(t *testing.T) { | |||
| 211 | + | before := validSpec() | |||
| 212 | + | before.Env = map[string]string{"KEEP": "1", "EDIT": "old-value", "DROP": "x"} | |||
| 213 | + | after := validSpec() | |||
| 214 | + | after.Env = map[string]string{"KEEP": "1", "EDIT": "new-value", "ADD": "y"} | |||
| 215 | + | ||||
| 216 | + | diff := domain.DiffAppSpec(after, before) | |||
| 217 | + | assert.Equal(t, []string{"env/ADD", "env/DROP", "env/EDIT"}, diff.Changed) | |||
| 218 | + | for _, entry := range diff.Changed { | |||
| 219 | + | assert.NotContains(t, entry, "value") | |||
| 220 | + | } | |||
| 221 | + | } | |||
| 222 | + | ||||
| 223 | + | func TestActiveAppSpec_UsesActivatedRevisionEnv(t *testing.T) { | |||
| 224 | + | web := validSpec().Services[0] | |||
| 225 | + | worker := web | |||
| 226 | + | worker.Name = "worker" | |||
| 227 | + | active := domain.AppActive{ | |||
| 228 | + | App: "blog", | |||
| 229 | + | Services: map[string]domain.AppEffectiveService{ | |||
| 230 | + | "web": {EffectiveRevision: "rev-1", Spec: web}, | |||
| 231 | + | "worker": {EffectiveRevision: "rev-2", Spec: worker}, | |||
| 232 | + | }, | |||
| 233 | + | } | |||
| 234 | + | envs := map[string]map[string]string{ | |||
| 235 | + | "rev-1": {"MODE": "prod"}, | |||
| 236 | + | "rev-2": {"MODE": "old"}, | |||
| 237 | + | } | |||
| 238 | + | load := func(revision string) (map[string]string, error) { return envs[revision], nil } | |||
| 239 | + | ||||
| 240 | + | // Image-only change: desired env equals what every service runs. | |||
| 241 | + | envs["rev-2"] = map[string]string{"MODE": "prod"} | |||
| 242 | + | spec, err := domain.ActiveAppSpec(active, map[string]string{"MODE": "prod"}, load) | |||
| 243 | + | require.NoError(t, err) | |||
| 244 | + | assert.Equal(t, map[string]string{"MODE": "prod"}, spec.Env) | |||
| 245 | + | assert.Equal(t, []string{"web", "worker"}, []string{spec.Services[0].Name, spec.Services[1].Name}) | |||
| 246 | + | ||||
| 247 | + | // One service still runs a stale env: the diff must not hide it. | |||
| 248 | + | envs["rev-2"] = map[string]string{"MODE": "old"} | |||
| 249 | + | spec, err = domain.ActiveAppSpec(active, map[string]string{"MODE": "prod"}, load) | |||
| 250 | + | require.NoError(t, err) | |||
| 251 | + | assert.Equal(t, map[string]string{"MODE": "old"}, spec.Env) | |||
| 252 | + | ||||
| 253 | + | // A garbage-collected revision counts as an empty env. | |||
| 254 | + | spec, err = domain.ActiveAppSpec(active, map[string]string{"MODE": "prod"}, func(string) (map[string]string, error) { return nil, nil }) | |||
| 255 | + | require.NoError(t, err) | |||
| 256 | + | assert.Empty(t, spec.Env) | |||
| 257 | + | ||||
| 258 | + | _, err = domain.ActiveAppSpec(active, nil, func(string) (map[string]string, error) { return nil, errors.New("boom") }) | |||
| 259 | + | require.Error(t, err) | |||
| 260 | + | } | |||
| 261 | + | ||||
| 262 | + | func TestActiveAppSpec_ReportsStaleKeysFromEveryRevision(t *testing.T) { | |||
| 263 | + | web := validSpec().Services[0] | |||
| 264 | + | worker := web | |||
| 265 | + | worker.Name = "worker" | |||
| 266 | + | active := domain.AppActive{ | |||
| 267 | + | App: "blog", | |||
| 268 | + | Services: map[string]domain.AppEffectiveService{ | |||
| 269 | + | "web": {EffectiveRevision: "rev-1", Spec: web}, | |||
| 270 | + | "worker": {EffectiveRevision: "rev-2", Spec: worker}, | |||
| 271 | + | }, | |||
| 272 | + | } | |||
| 273 | + | envs := map[string]map[string]string{ | |||
| 274 | + | "rev-1": {"A": "old", "B": "new", "C": "same"}, | |||
| 275 | + | "rev-2": {"A": "new", "C": "same", "D": "gone"}, | |||
| 276 | + | } | |||
| 277 | + | desired := domain.AppSpec{Name: "blog", Env: map[string]string{"A": "new", "B": "new", "C": "same"}} | |||
| 278 | + | desired.Services = []domain.AppService{web, worker} | |||
| 279 | + | ||||
| 280 | + | effective, err := domain.ActiveAppSpec(active, desired.Env, func(revision string) (map[string]string, error) { | |||
| 281 | + | return envs[revision], nil | |||
| 282 | + | }) | |||
| 283 | + | require.NoError(t, err) | |||
| 284 | + | ||||
| 285 | + | diff := domain.DiffAppSpec(desired, effective) | |||
| 286 | + | assert.Equal(t, []string{"env/A", "env/B", "env/D"}, diff.Changed) | |||
| 287 | + | assert.Equal(t, map[string]string{"A": "new", "B": "new", "C": "same"}, desired.Env, "desired env must not be mutated") | |||
| 288 | + | } | |||
| 289 | + | ||||
| 290 | + | func TestActiveAppSpec_EmptyActiveHasEmptyEnv(t *testing.T) { | |||
| 291 | + | spec, err := domain.ActiveAppSpec(domain.AppActive{App: "blog"}, map[string]string{"A": "x"}, func(string) (map[string]string, error) { | |||
| 292 | + | t.Fatal("no revision should be loaded") | |||
| 293 | + | return nil, nil | |||
| 294 | + | }) | |||
| 295 | + | require.NoError(t, err) | |||
| 296 | + | assert.Empty(t, spec.Env) | |||
| 297 | + | assert.NotNil(t, spec.Env) | |||
| 208 | } | 298 | } | ||
M internal/usecase/apps/app_service.go +5 -1
| @@ -402,7 +402,11 @@ func (s *AppServiceImpl) Diff(ctx context.Context, app string) (domain.AppDiff, | |||
| 402 | 402 | if err != nil { | |
| 403 | 403 | return domain.AppDiff{}, err | |
| 404 | 404 | } | |
| 405 | - | return domain.DiffAppSpec(desired.Spec, activeSpec(active)), nil | |
| 405 | + | effective, err := activeSpec(ctx, s.store, active, desired.Spec.Env) | |
| 406 | + | if err != nil { | |
| 407 | + | return domain.AppDiff{}, err | |
| 408 | + | } | |
| 409 | + | return domain.DiffAppSpec(desired.Spec, effective), nil | |
| 406 | 410 | } | |
| 407 | 411 | ||
| 408 | 412 | // Deploy implements in.AppService. It enrolls the request hand-off with the | |
| @@ -402,7 +402,11 @@ func (s *AppServiceImpl) Diff(ctx context.Context, app string) (domain.AppDiff, | |||||
| 402 | if err != nil { | 402 | if err != nil { | ||
| 403 | return domain.AppDiff{}, err | 403 | return domain.AppDiff{}, err | ||
| 404 | } | 404 | } | ||
| 405 | - | return domain.DiffAppSpec(desired.Spec, activeSpec(active)), nil | 405 | + | effective, err := activeSpec(ctx, s.store, active, desired.Spec.Env) |
| 406 | + | if err != nil { | |||
| 407 | + | return domain.AppDiff{}, err | |||
| 408 | + | } | |||
| 409 | + | return domain.DiffAppSpec(desired.Spec, effective), nil | |||
| 406 | } | 410 | } | ||
| 407 | 411 | ||||
| 408 | // Deploy implements in.AppService. It enrolls the request hand-off with the | 412 | // Deploy implements in.AppService. It enrolls the request hand-off with the | ||
M internal/usecase/apps/service.go +27 -9
| @@ -8,6 +8,7 @@ import ( | |||
| 8 | 8 | "context" | |
| 9 | 9 | "crypto/sha256" | |
| 10 | 10 | "encoding/hex" | |
| 11 | + | "errors" | |
| 11 | 12 | "fmt" | |
| 12 | 13 | "sync" | |
| 13 | 14 | "time" | |
| @@ -359,7 +360,11 @@ func (s *Service) prepareApply(ctx context.Context, spec domain.AppSpec) (*apply | |||
| 359 | 360 | if err := domain.CheckReservations(checkpoint.Reservations, reservations, spec.Name); err != nil { | |
| 360 | 361 | return nil, err | |
| 361 | 362 | } | |
| 362 | - | diff := domain.DiffAppSpec(spec, activeSpec(active)) | |
| 363 | + | effective, err := activeSpec(ctx, s.store, active, spec.Env) | |
| 364 | + | if err != nil { | |
| 365 | + | return nil, err | |
| 366 | + | } | |
| 367 | + | diff := domain.DiffAppSpec(spec, effective) | |
| 363 | 368 | if hasDesired && specsEqual(desired.Spec, spec) { | |
| 364 | 369 | return &applyPrepared{desired: desired, diff: diff, reservations: reservations, noop: true}, nil | |
| 365 | 370 | } | |
| @@ -395,15 +400,28 @@ func (s *Service) persistApply(ctx context.Context, spec domain.AppSpec, source | |||
| 395 | 400 | }, nil, nil | |
| 396 | 401 | } | |
| 397 | 402 | ||
| 398 | - | // activeSpec rebuilds an AppSpec view from effective definitions for diffing. | |
| 399 | - | func activeSpec(active domain.AppActive) domain.AppSpec { | |
| 400 | - | spec := domain.AppSpec{Name: active.App, Env: map[string]string{}, Networks: append([]domain.AppSharedNetwork(nil), active.Networks...)} | |
| 401 | - | for name, svc := range active.Services { | |
| 402 | - | svcSpec := svc.Spec | |
| 403 | - | svcSpec.Name = name | |
| 404 | - | spec.Services = append(spec.Services, svcSpec) | |
| 403 | + | // revisionLoader is the store subset activeSpec needs. | |
| 404 | + | type revisionLoader interface { | |
| 405 | + | LoadRevision(ctx context.Context, app, revision string) (domain.AppDesiredRevision, error) | |
| 406 | + | } | |
| 407 | + | ||
| 408 | + | // activeSpec rebuilds an AppSpec view from effective definitions for | |
| 409 | + | // diffing, including the app env of the revisions services run from. | |
| 410 | + | func activeSpec(ctx context.Context, store revisionLoader, active domain.AppActive, desiredEnv map[string]string) (domain.AppSpec, error) { | |
| 411 | + | spec, err := domain.ActiveAppSpec(active, desiredEnv, func(revision string) (map[string]string, error) { | |
| 412 | + | rev, err := store.LoadRevision(ctx, active.App, revision) | |
| 413 | + | if errors.Is(err, domain.ErrAppRevisionNotFound) { | |
| 414 | + | return nil, nil | |
| 415 | + | } | |
| 416 | + | if err != nil { | |
| 417 | + | return nil, err | |
| 418 | + | } | |
| 419 | + | return rev.Spec.Env, nil | |
| 420 | + | }) | |
| 421 | + | if err != nil { | |
| 422 | + | return domain.AppSpec{}, fmt.Errorf("apps: rebuild active spec: %w", err) | |
| 405 | 423 | } | |
| 406 | - | return spec | |
| 424 | + | return spec, nil | |
| 407 | 425 | } | |
| 408 | 426 | ||
| 409 | 427 | // specsEqual compares normalized specs for no-op detection. | |
| @@ -8,6 +8,7 @@ import ( | |||||
| 8 | "context" | 8 | "context" | ||
| 9 | "crypto/sha256" | 9 | "crypto/sha256" | ||
| 10 | "encoding/hex" | 10 | "encoding/hex" | ||
| 11 | + | "errors" | |||
| 11 | "fmt" | 12 | "fmt" | ||
| 12 | "sync" | 13 | "sync" | ||
| 13 | "time" | 14 | "time" | ||
| @@ -359,7 +360,11 @@ func (s *Service) prepareApply(ctx context.Context, spec domain.AppSpec) (*apply | |||||
| 359 | if err := domain.CheckReservations(checkpoint.Reservations, reservations, spec.Name); err != nil { | 360 | if err := domain.CheckReservations(checkpoint.Reservations, reservations, spec.Name); err != nil { | ||
| 360 | return nil, err | 361 | return nil, err | ||
| 361 | } | 362 | } | ||
| 362 | - | diff := domain.DiffAppSpec(spec, activeSpec(active)) | 363 | + | effective, err := activeSpec(ctx, s.store, active, spec.Env) |
| 364 | + | if err != nil { | |||
| 365 | + | return nil, err | |||
| 366 | + | } | |||
| 367 | + | diff := domain.DiffAppSpec(spec, effective) | |||
| 363 | if hasDesired && specsEqual(desired.Spec, spec) { | 368 | if hasDesired && specsEqual(desired.Spec, spec) { | ||
| 364 | return &applyPrepared{desired: desired, diff: diff, reservations: reservations, noop: true}, nil | 369 | return &applyPrepared{desired: desired, diff: diff, reservations: reservations, noop: true}, nil | ||
| 365 | } | 370 | } | ||
| @@ -395,15 +400,28 @@ func (s *Service) persistApply(ctx context.Context, spec domain.AppSpec, source | |||||
| 395 | }, nil, nil | 400 | }, nil, nil | ||
| 396 | } | 401 | } | ||
| 397 | 402 | ||||
| 398 | - | // activeSpec rebuilds an AppSpec view from effective definitions for diffing. | 403 | + | // revisionLoader is the store subset activeSpec needs. |
| 399 | - | func activeSpec(active domain.AppActive) domain.AppSpec { | 404 | + | type revisionLoader interface { |
| 400 | - | spec := domain.AppSpec{Name: active.App, Env: map[string]string{}, Networks: append([]domain.AppSharedNetwork(nil), active.Networks...)} | 405 | + | LoadRevision(ctx context.Context, app, revision string) (domain.AppDesiredRevision, error) |
| 401 | - | for name, svc := range active.Services { | 406 | + | } |
| 402 | - | svcSpec := svc.Spec | 407 | + | |
| 403 | - | svcSpec.Name = name | 408 | + | // activeSpec rebuilds an AppSpec view from effective definitions for |
| 404 | - | spec.Services = append(spec.Services, svcSpec) | 409 | + | // diffing, including the app env of the revisions services run from. |
| 410 | + | func activeSpec(ctx context.Context, store revisionLoader, active domain.AppActive, desiredEnv map[string]string) (domain.AppSpec, error) { | |||
| 411 | + | spec, err := domain.ActiveAppSpec(active, desiredEnv, func(revision string) (map[string]string, error) { | |||
| 412 | + | rev, err := store.LoadRevision(ctx, active.App, revision) | |||
| 413 | + | if errors.Is(err, domain.ErrAppRevisionNotFound) { | |||
| 414 | + | return nil, nil | |||
| 415 | + | } | |||
| 416 | + | if err != nil { | |||
| 417 | + | return nil, err | |||
| 418 | + | } | |||
| 419 | + | return rev.Spec.Env, nil | |||
| 420 | + | }) | |||
| 421 | + | if err != nil { | |||
| 422 | + | return domain.AppSpec{}, fmt.Errorf("apps: rebuild active spec: %w", err) | |||
| 405 | } | 423 | } | ||
| 406 | - | return spec | 424 | + | return spec, nil |
| 407 | } | 425 | } | ||
| 408 | 426 | ||||
| 409 | // specsEqual compares normalized specs for no-op detection. | 427 | // specsEqual compares normalized specs for no-op detection. | ||
M internal/usecase/apps/service_test.go +27 -0
| @@ -373,3 +373,30 @@ func TestReservationsFor_DelegatesToDomain(t *testing.T) { | |||
| 373 | 373 | assert.Equal(t, "tcp", reservations[1].Proto) | |
| 374 | 374 | assert.Equal(t, "dual", reservations[1].IP) | |
| 375 | 375 | } | |
| 376 | + | ||
| 377 | + | // TestApply_ImageOnlyDiffWithAppEnv reproduces issue 22: with a non-empty | |
| 378 | + | // [env], an image-only change must not report the app env as changed. | |
| 379 | + | func TestApply_ImageOnlyDiffWithAppEnv(t *testing.T) { | |
| 380 | + | ctx := context.Background() | |
| 381 | + | store := outmocks.NewMockAppState(t) | |
| 382 | + | svc := apps.NewService(store, zerowrap.Default()) | |
| 383 | + | spec := testSpec("blog") | |
| 384 | + | spec.Env = map[string]string{"MODE": "prod"} | |
| 385 | + | previous := testSpec("blog") | |
| 386 | + | previous.Env = map[string]string{"MODE": "prod"} | |
| 387 | + | spec.Services[0].Image = "img:2" | |
| 388 | + | ||
| 389 | + | store.EXPECT().Recover(mock.Anything).Return(nil).Once() | |
| 390 | + | store.EXPECT().LoadCheckpoint(mock.Anything).Return(domain.AppStoreCheckpoint{}, nil).Once() | |
| 391 | + | store.EXPECT().LoadDesired(mock.Anything, "blog").Return(domain.AppDesiredRevision{Revision: "rev-1", App: "blog", Spec: previous}, true, nil).Once() | |
| 392 | + | store.EXPECT().LoadActive(mock.Anything, "blog").Return(domain.AppActive{ | |
| 393 | + | App: "blog", | |
| 394 | + | Services: map[string]domain.AppEffectiveService{"web": {EffectiveRevision: "rev-1", Spec: previous.Services[0]}}, | |
| 395 | + | }, true, nil).Once() | |
| 396 | + | store.EXPECT().LoadRevision(mock.Anything, "blog", "rev-1").Return(domain.AppDesiredRevision{Revision: "rev-1", App: "blog", Spec: previous}, nil).Once() | |
| 397 | + | ||
| 398 | + | _, dry, err := svc.Apply(ctx, spec, []byte("manifest"), true) | |
| 399 | + | require.NoError(t, err) | |
| 400 | + | require.NotNil(t, dry) | |
| 401 | + | assert.Equal(t, []string{"service/web/image"}, dry.Diff.Changed) | |
| 402 | + | } | |
| @@ -373,3 +373,30 @@ func TestReservationsFor_DelegatesToDomain(t *testing.T) { | |||||
| 373 | assert.Equal(t, "tcp", reservations[1].Proto) | 373 | assert.Equal(t, "tcp", reservations[1].Proto) | ||
| 374 | assert.Equal(t, "dual", reservations[1].IP) | 374 | assert.Equal(t, "dual", reservations[1].IP) | ||
| 375 | } | 375 | } | ||
| 376 | + | ||||
| 377 | + | // TestApply_ImageOnlyDiffWithAppEnv reproduces issue 22: with a non-empty | |||
| 378 | + | // [env], an image-only change must not report the app env as changed. | |||
| 379 | + | func TestApply_ImageOnlyDiffWithAppEnv(t *testing.T) { | |||
| 380 | + | ctx := context.Background() | |||
| 381 | + | store := outmocks.NewMockAppState(t) | |||
| 382 | + | svc := apps.NewService(store, zerowrap.Default()) | |||
| 383 | + | spec := testSpec("blog") | |||
| 384 | + | spec.Env = map[string]string{"MODE": "prod"} | |||
| 385 | + | previous := testSpec("blog") | |||
| 386 | + | previous.Env = map[string]string{"MODE": "prod"} | |||
| 387 | + | spec.Services[0].Image = "img:2" | |||
| 388 | + | ||||
| 389 | + | store.EXPECT().Recover(mock.Anything).Return(nil).Once() | |||
| 390 | + | store.EXPECT().LoadCheckpoint(mock.Anything).Return(domain.AppStoreCheckpoint{}, nil).Once() | |||
| 391 | + | store.EXPECT().LoadDesired(mock.Anything, "blog").Return(domain.AppDesiredRevision{Revision: "rev-1", App: "blog", Spec: previous}, true, nil).Once() | |||
| 392 | + | store.EXPECT().LoadActive(mock.Anything, "blog").Return(domain.AppActive{ | |||
| 393 | + | App: "blog", | |||
| 394 | + | Services: map[string]domain.AppEffectiveService{"web": {EffectiveRevision: "rev-1", Spec: previous.Services[0]}}, | |||
| 395 | + | }, true, nil).Once() | |||
| 396 | + | store.EXPECT().LoadRevision(mock.Anything, "blog", "rev-1").Return(domain.AppDesiredRevision{Revision: "rev-1", App: "blog", Spec: previous}, nil).Once() | |||
| 397 | + | ||||
| 398 | + | _, dry, err := svc.Apply(ctx, spec, []byte("manifest"), true) | |||
| 399 | + | require.NoError(t, err) | |||
| 400 | + | require.NotNil(t, dry) | |||
| 401 | + | assert.Equal(t, []string{"service/web/image"}, dry.Diff.Changed) | |||
| 402 | + | } | |||
M internal/usecase/deployment/preflight_gate_test.go +31 -0
| @@ -2,6 +2,7 @@ package deployment_test | |||
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | + | "maps" | |
| 5 | 6 | "testing" | |
| 6 | 7 | "time" | |
| 7 | 8 | ||
| @@ -340,3 +341,33 @@ func TestDeploy_UnsupportedEngineFailsBeforeMutation(t *testing.T) { | |||
| 340 | 341 | assertNoDeployMutation(t, runtime, state) | |
| 341 | 342 | assertPreflightGateJournal(t, *saved) | |
| 342 | 343 | } | |
| 344 | + | ||
| 345 | + | // TestStartDeploy_TargetedImageChangeWithUnchangedAppEnv reproduces issue | |
| 346 | + | // 22: an app with a non-empty [env] whose only change is one service image | |
| 347 | + | // must accept a service-targeted deploy. | |
| 348 | + | func TestStartDeploy_TargetedImageChangeWithUnchangedAppEnv(t *testing.T) { | |
| 349 | + | ctx := context.Background() | |
| 350 | + | state, runtime, images, secrets := mockDeps(t) | |
| 351 | + | web := webService() | |
| 352 | + | env := map[string]string{"MODE": "prod"} | |
| 353 | + | active := domain.AppActive{ | |
| 354 | + | App: "blog", ConvergedRevision: "rev-1", Converged: true, | |
| 355 | + | Services: map[string]domain.AppEffectiveService{"web": {EffectiveRevision: "rev-1", Spec: web}}, | |
| 356 | + | } | |
| 357 | + | next := web | |
| 358 | + | next.Image = "img:2" | |
| 359 | + | rev := domain.AppDesiredRevision{App: "blog", Revision: "rev-2", Spec: domain.AppSpec{Name: "blog", Env: env, Services: []domain.AppService{next}}} | |
| 360 | + | ||
| 361 | + | state.EXPECT().Recover(mock.Anything).Return(nil) | |
| 362 | + | state.EXPECT().LoadRevision(mock.Anything, "blog", "rev-2").Return(rev, nil).Once() | |
| 363 | + | state.EXPECT().LoadActive(mock.Anything, "blog").Return(active, true, nil).Once() | |
| 364 | + | state.EXPECT().LoadRevision(mock.Anything, "blog", "rev-1").Return(domain.AppDesiredRevision{ | |
| 365 | + | App: "blog", Revision: "rev-1", Spec: domain.AppSpec{Name: "blog", Env: maps.Clone(env), Services: []domain.AppService{web}}, | |
| 366 | + | }, nil).Once() | |
| 367 | + | state.EXPECT().SaveOperation(mock.Anything, mock.Anything).Return(nil).Once() | |
| 368 | + | ||
| 369 | + | svc := keyedService(t, state, runtime, images, secrets) | |
| 370 | + | started, err := svc.StartDeploy(ctx, deployment.DeployInput{App: "blog", Revision: "rev-2", Service: "web"}) | |
| 371 | + | require.NoError(t, err) | |
| 372 | + | require.True(t, started.Owned) | |
| 373 | + | } | |
| @@ -2,6 +2,7 @@ package deployment_test | |||||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | + | "maps" | |||
| 5 | "testing" | 6 | "testing" | ||
| 6 | "time" | 7 | "time" | ||
| 7 | 8 | ||||
| @@ -340,3 +341,33 @@ func TestDeploy_UnsupportedEngineFailsBeforeMutation(t *testing.T) { | |||||
| 340 | assertNoDeployMutation(t, runtime, state) | 341 | assertNoDeployMutation(t, runtime, state) | ||
| 341 | assertPreflightGateJournal(t, *saved) | 342 | assertPreflightGateJournal(t, *saved) | ||
| 342 | } | 343 | } | ||
| 344 | + | ||||
| 345 | + | // TestStartDeploy_TargetedImageChangeWithUnchangedAppEnv reproduces issue | |||
| 346 | + | // 22: an app with a non-empty [env] whose only change is one service image | |||
| 347 | + | // must accept a service-targeted deploy. | |||
| 348 | + | func TestStartDeploy_TargetedImageChangeWithUnchangedAppEnv(t *testing.T) { | |||
| 349 | + | ctx := context.Background() | |||
| 350 | + | state, runtime, images, secrets := mockDeps(t) | |||
| 351 | + | web := webService() | |||
| 352 | + | env := map[string]string{"MODE": "prod"} | |||
| 353 | + | active := domain.AppActive{ | |||
| 354 | + | App: "blog", ConvergedRevision: "rev-1", Converged: true, | |||
| 355 | + | Services: map[string]domain.AppEffectiveService{"web": {EffectiveRevision: "rev-1", Spec: web}}, | |||
| 356 | + | } | |||
| 357 | + | next := web | |||
| 358 | + | next.Image = "img:2" | |||
| 359 | + | rev := domain.AppDesiredRevision{App: "blog", Revision: "rev-2", Spec: domain.AppSpec{Name: "blog", Env: env, Services: []domain.AppService{next}}} | |||
| 360 | + | ||||
| 361 | + | state.EXPECT().Recover(mock.Anything).Return(nil) | |||
| 362 | + | state.EXPECT().LoadRevision(mock.Anything, "blog", "rev-2").Return(rev, nil).Once() | |||
| 363 | + | state.EXPECT().LoadActive(mock.Anything, "blog").Return(active, true, nil).Once() | |||
| 364 | + | state.EXPECT().LoadRevision(mock.Anything, "blog", "rev-1").Return(domain.AppDesiredRevision{ | |||
| 365 | + | App: "blog", Revision: "rev-1", Spec: domain.AppSpec{Name: "blog", Env: maps.Clone(env), Services: []domain.AppService{web}}, | |||
| 366 | + | }, nil).Once() | |||
| 367 | + | state.EXPECT().SaveOperation(mock.Anything, mock.Anything).Return(nil).Once() | |||
| 368 | + | ||||
| 369 | + | svc := keyedService(t, state, runtime, images, secrets) | |||
| 370 | + | started, err := svc.StartDeploy(ctx, deployment.DeployInput{App: "blog", Revision: "rev-2", Service: "web"}) | |||
| 371 | + | require.NoError(t, err) | |||
| 372 | + | require.True(t, started.Owned) | |||
| 373 | + | } | |||
M internal/usecase/deployment/service.go +17 -8
| @@ -653,7 +653,13 @@ func (s *Service) checkConverged(ctx context.Context, app, service string, rev d | |||
| 653 | 653 | if active.Converged && active.ConvergedRevision == rev.Revision { | |
| 654 | 654 | return nil | |
| 655 | 655 | } | |
| 656 | - | diff := domain.DiffAppSpec(rev.Spec, effectiveAppSpec(active)) | |
| 656 | + | effective, err := domain.ActiveAppSpec(active, rev.Spec.Env, func(revision string) (map[string]string, error) { | |
| 657 | + | return s.revisionEnv(ctx, app, revision) | |
| 658 | + | }) | |
| 659 | + | if err != nil { | |
| 660 | + | return fmt.Errorf("deployment: rebuild effective spec: %w", err) | |
| 661 | + | } | |
| 662 | + | diff := domain.DiffAppSpec(rev.Spec, effective) | |
| 657 | 663 | prefix := "service/" + service + "/" | |
| 658 | 664 | if len(diff.Added) > 0 { | |
| 659 | 665 | return targetedDivergenceError(rev.Revision, diff.Added[0]+" (added)") | |
| @@ -669,14 +675,17 @@ func (s *Service) checkConverged(ctx context.Context, app, service string, rev d | |||
| 669 | 675 | return nil | |
| 670 | 676 | } | |
| 671 | 677 | ||
| 672 | - | func effectiveAppSpec(active domain.AppActive) domain.AppSpec { | |
| 673 | - | spec := domain.AppSpec{Name: active.App, Networks: append([]domain.AppSharedNetwork(nil), active.Networks...)} | |
| 674 | - | for name, service := range active.Services { | |
| 675 | - | effective := service.Spec | |
| 676 | - | effective.Name = name | |
| 677 | - | spec.Services = append(spec.Services, effective) | |
| 678 | + | // revisionEnv returns the app env of a stored revision, or nil when the | |
| 679 | + | // revision was garbage-collected. | |
| 680 | + | func (s *Service) revisionEnv(ctx context.Context, app, revision string) (map[string]string, error) { | |
| 681 | + | rev, err := s.deps.State.LoadRevision(ctx, app, revision) | |
| 682 | + | if errors.Is(err, domain.ErrAppRevisionNotFound) { | |
| 683 | + | return nil, nil | |
| 684 | + | } | |
| 685 | + | if err != nil { | |
| 686 | + | return nil, err | |
| 678 | 687 | } | |
| 679 | - | return spec | |
| 688 | + | return rev.Spec.Env, nil | |
| 680 | 689 | } | |
| 681 | 690 | ||
| 682 | 691 | func targetedDivergenceError(revision, path string) error { | |
| @@ -653,7 +653,13 @@ func (s *Service) checkConverged(ctx context.Context, app, service string, rev d | |||||
| 653 | if active.Converged && active.ConvergedRevision == rev.Revision { | 653 | if active.Converged && active.ConvergedRevision == rev.Revision { | ||
| 654 | return nil | 654 | return nil | ||
| 655 | } | 655 | } | ||
| 656 | - | diff := domain.DiffAppSpec(rev.Spec, effectiveAppSpec(active)) | 656 | + | effective, err := domain.ActiveAppSpec(active, rev.Spec.Env, func(revision string) (map[string]string, error) { |
| 657 | + | return s.revisionEnv(ctx, app, revision) | |||
| 658 | + | }) | |||
| 659 | + | if err != nil { | |||
| 660 | + | return fmt.Errorf("deployment: rebuild effective spec: %w", err) | |||
| 661 | + | } | |||
| 662 | + | diff := domain.DiffAppSpec(rev.Spec, effective) | |||
| 657 | prefix := "service/" + service + "/" | 663 | prefix := "service/" + service + "/" | ||
| 658 | if len(diff.Added) > 0 { | 664 | if len(diff.Added) > 0 { | ||
| 659 | return targetedDivergenceError(rev.Revision, diff.Added[0]+" (added)") | 665 | return targetedDivergenceError(rev.Revision, diff.Added[0]+" (added)") | ||
| @@ -669,14 +675,17 @@ func (s *Service) checkConverged(ctx context.Context, app, service string, rev d | |||||
| 669 | return nil | 675 | return nil | ||
| 670 | } | 676 | } | ||
| 671 | 677 | ||||
| 672 | - | func effectiveAppSpec(active domain.AppActive) domain.AppSpec { | 678 | + | // revisionEnv returns the app env of a stored revision, or nil when the |
| 673 | - | spec := domain.AppSpec{Name: active.App, Networks: append([]domain.AppSharedNetwork(nil), active.Networks...)} | 679 | + | // revision was garbage-collected. |
| 674 | - | for name, service := range active.Services { | 680 | + | func (s *Service) revisionEnv(ctx context.Context, app, revision string) (map[string]string, error) { |
| 675 | - | effective := service.Spec | 681 | + | rev, err := s.deps.State.LoadRevision(ctx, app, revision) |
| 676 | - | effective.Name = name | 682 | + | if errors.Is(err, domain.ErrAppRevisionNotFound) { |
| 677 | - | spec.Services = append(spec.Services, effective) | 683 | + | return nil, nil |
| 684 | + | } | |||
| 685 | + | if err != nil { | |||
| 686 | + | return nil, err | |||
| 678 | } | 687 | } | ||
| 679 | - | return spec | 688 | + | return rev.Spec.Env, nil |
| 680 | } | 689 | } | ||
| 681 | 690 | ||||
| 682 | func targetedDivergenceError(revision, path string) error { | 691 | func targetedDivergenceError(revision, path string) error { | ||