diff --git a/src/cmd/cli/command/compose.go b/src/cmd/cli/command/compose.go index 53d7167de..e9d5e5360 100644 --- a/src/cmd/cli/command/compose.go +++ b/src/cmd/cli/command/compose.go @@ -553,6 +553,71 @@ func makeComposeConfigCmd() *cobra.Command { } } +func makeComposeFixCmd() *cobra.Command { + var dryRun bool + cmd := &cobra.Command{ + Use: "fix", + Args: cobra.NoArgs, + Short: "Apply safe mechanical fixes to a Compose file", + RunE: func(cmd *cobra.Command, args []string) error { + loader := newLoaderForCommand(cmd) + + project, loadErr := loader.LoadProject(cmd.Context()) + if loadErr != nil { + return handleInvalidComposeFileErr(cmd.Context(), loadErr) + } + + fixes := compose.FixProject(project) + printFixResults(fixes) + if dryRun || len(fixes) == 0 { + return nil + } + + data, err := compose.MarshalYAML(project) + if err != nil { + return err + } + term.Println() + _, err = term.Print(string(data)) + return err + }, + } + cmd.Flags().BoolVar(&dryRun, "dry-run", false, "show fixes without outputting YAML") + return cmd +} + +func printFixResults(fixes []compose.FixResult) { + if len(fixes) == 0 { + term.Info("No fixes needed.") + return + } + term.Println("Fixes applied:") + for _, fix := range fixes { + term.Printf(" service %q: %s\n", fix.Service, describeFixResult(fix)) + } + term.Printf("\n%d fix(es) applied.\n", len(fixes)) +} + +func describeFixResult(fix compose.FixResult) string { + switch fix.Action { + case "removed": + return fmt.Sprintf("removed unsupported directive: %s (%s)", fix.Field, fix.Reason) + case "changed": + if fix.Before != "" { + return fmt.Sprintf("changed %s from %q to %q (%s)", fix.Field, fix.Before, fix.After, fix.Reason) + } + return fmt.Sprintf("changed %s to %q (%s)", fix.Field, fix.After, fix.Reason) + default: + if fix.Field == "mode" { + return fmt.Sprintf("added mode: %s to %s", fix.After, fix.Reason) + } + if fix.Reason != "" { + return fmt.Sprintf("added %s: %s (%s)", fix.Field, fix.After, fix.Reason) + } + return fmt.Sprintf("added %s: %s", fix.Field, fix.After) + } +} + func makeComposePsCmd() *cobra.Command { getServicesCmd := &cobra.Command{ Use: "ps", @@ -763,6 +828,7 @@ services: composeCmd.PersistentFlags().StringVar(&byoc.DefangPulumiBackend, "pulumi-backend", "", `specify an alternate Pulumi backend URL or "pulumi-cloud"`) composeCmd.AddCommand(makeComposeUpCmd()) composeCmd.AddCommand(makeComposeConfigCmd()) + composeCmd.AddCommand(makeComposeFixCmd()) composeCmd.AddCommand(makeComposeDownCmd()) composeCmd.AddCommand(makeComposePsCmd()) composeCmd.AddCommand(makeLogsCmd()) diff --git a/src/pkg/cli/compose/fix.go b/src/pkg/cli/compose/fix.go new file mode 100644 index 000000000..b3aaf72bd --- /dev/null +++ b/src/pkg/cli/compose/fix.go @@ -0,0 +1,194 @@ +package compose + +import ( + "fmt" + "sort" + + composeTypes "github.com/compose-spec/compose-go/v2/types" +) + +const defaultRestartPolicy = "unless-stopped" + +type FixResult struct { + Service string + Field string + Action string // "added", "removed", "changed" + Before string + After string + Reason string +} + +func FixProject(project *Project) []FixResult { + if project == nil { + return nil + } + + var results []FixResult + for _, name := range sortedServiceNames(project) { + service := project.Services[name] + results = append(results, fixService(&service)...) + project.Services[name] = service + } + return results +} + +func sortedServiceNames(project *Project) []string { + names := make([]string, 0, len(project.Services)) + for name := range project.Services { + names = append(names, name) + } + sort.Strings(names) + return names +} + +func fixService(service *composeTypes.ServiceConfig) []FixResult { + var results []FixResult + repo := GetImageRepo(service.Image) + isManagedStoreImage := IsPostgresRepo(repo) || IsRedisRepo(repo) || IsMongoRepo(repo) + + results = append(results, fixPorts(service, isManagedStoreImage)...) + results = append(results, fixLimitsToReservations(service)...) + results = append(results, fixRestart(service)...) + results = append(results, fixUnsupportedDirectives(service)...) + + return results +} + +func fixPorts(service *composeTypes.ServiceConfig, isManagedStoreImage bool) []FixResult { + var results []FixResult + for i := range service.Ports { + port := &service.Ports[i] + if port.Mode != "" { + continue + } + + mode := Mode_INGRESS + reason := "" + if port.Protocol == Protocol_UDP { + mode = Mode_HOST + reason = "UDP port" + } else if isManagedStoreImage { + mode = Mode_HOST + reason = "database image" + } + port.Mode = mode + results = append(results, FixResult{ + Service: service.Name, + Field: "mode", + Action: "added", + After: mode, + Reason: portReason(port.Target, reason), + }) + } + return results +} + +func portReason(target uint32, reason string) string { + if reason == "" { + return fmt.Sprintf("port %d", target) + } + return fmt.Sprintf("port %d (%s)", target, reason) +} + +func fixLimitsToReservations(service *composeTypes.ServiceConfig) []FixResult { + if service.Deploy == nil { + return nil + } + if service.Deploy.Resources.Limits == nil || service.Deploy.Resources.Reservations != nil { + return nil + } + limits := *service.Deploy.Resources.Limits + service.Deploy.Resources.Reservations = &limits + return []FixResult{{ + Service: service.Name, + Field: "deploy.resources.reservations", + Action: "added", + After: "copied from deploy.resources.limits", + Reason: "Defang uses reservations for scheduling, not limits", + }} +} + +func fixRestart(service *composeTypes.ServiceConfig) []FixResult { + restart := restartFromDeployPolicy(service) + if restart == "" && isSupportedRestart(service.Restart) { + return nil + } + + before := service.Restart + if restart == "" { + restart = defaultRestartPolicy + } + service.Restart = restart + + reason := "unsupported restart policy" + if service.Deploy != nil && service.Deploy.RestartPolicy != nil { + reason = "deploy.restart_policy is unsupported; converted to service-level restart" + service.Deploy.RestartPolicy = nil + } else if before == "" { + reason = "missing restart policy" + } + + action := "changed" + if before == "" { + action = "added" + } + return []FixResult{{ + Service: service.Name, + Field: "restart", + Action: action, + Before: before, + After: restart, + Reason: reason, + }} +} + +func restartFromDeployPolicy(service *composeTypes.ServiceConfig) string { + if service.Deploy == nil || service.Deploy.RestartPolicy == nil { + return "" + } + switch service.Deploy.RestartPolicy.Condition { + case "", "any": + return "always" + default: + return defaultRestartPolicy + } +} + +func isSupportedRestart(restart string) bool { + return restart == "always" || restart == defaultRestartPolicy +} + +func fixUnsupportedDirectives(service *composeTypes.ServiceConfig) []FixResult { + var results []FixResult + if len(service.DNS) != 0 { + service.DNS = nil + results = append(results, removedDirective(service.Name, "dns")) + } + if len(service.DNSSearch) != 0 { + service.DNSSearch = nil + results = append(results, removedDirective(service.Name, "dns_search")) + } + if len(service.Devices) != 0 { + service.Devices = nil + results = append(results, removedDirective(service.Name, "devices")) + } + if len(service.DeviceCgroupRules) != 0 { + service.DeviceCgroupRules = nil + results = append(results, removedDirective(service.Name, "device_cgroup_rules")) + } + if len(service.GroupAdd) != 0 { + service.GroupAdd = nil + results = append(results, removedDirective(service.Name, "group_add")) + } + return results +} + +func removedDirective(service, field string) FixResult { + return FixResult{ + Service: service, + Field: field, + Action: "removed", + Before: "present", + Reason: "unsupported directive", + } +} diff --git a/src/pkg/cli/compose/fix_test.go b/src/pkg/cli/compose/fix_test.go new file mode 100644 index 000000000..87078a914 --- /dev/null +++ b/src/pkg/cli/compose/fix_test.go @@ -0,0 +1,197 @@ +package compose + +import ( + "reflect" + "testing" + + "github.com/DefangLabs/defang/src/pkg/modes" + composeTypes "github.com/compose-spec/compose-go/v2/types" +) + +func TestFixProject(t *testing.T) { + tests := []struct { + name string + project *Project + want []FixResult + check func(*testing.T, *Project) + }{ + { + name: "web service port mode and restart", + project: &Project{Services: Services{ + "web": { + Name: "web", + Image: "nginx", + Ports: []composeTypes.ServicePortConfig{{Target: 8080}}, + }, + }}, + want: []FixResult{ + {Service: "web", Field: "mode", Action: "added", After: Mode_INGRESS, Reason: "port 8080"}, + {Service: "web", Field: "restart", Action: "added", After: defaultRestartPolicy, Reason: "missing restart policy"}, + }, + check: func(t *testing.T, project *Project) { + service := project.Services["web"] + if service.Ports[0].Mode != Mode_INGRESS { + t.Fatalf("port mode = %q, want %q", service.Ports[0].Mode, Mode_INGRESS) + } + if service.Restart != defaultRestartPolicy { + t.Fatalf("restart = %q, want %q", service.Restart, defaultRestartPolicy) + } + }, + }, + { + name: "managed postgres defaults to host mode", + project: &Project{Services: Services{ + "db": { + Name: "db", + Image: "postgres:16", + Ports: []composeTypes.ServicePortConfig{{Target: 5432}}, + }, + }}, + want: []FixResult{ + {Service: "db", Field: "mode", Action: "added", After: Mode_HOST, Reason: "port 5432 (database image)"}, + {Service: "db", Field: "restart", Action: "added", After: defaultRestartPolicy, Reason: "missing restart policy"}, + }, + check: func(t *testing.T, project *Project) { + service := project.Services["db"] + if service.Ports[0].Mode != Mode_HOST { + t.Fatalf("port mode = %q, want %q", service.Ports[0].Mode, Mode_HOST) + } + }, + }, + { + name: "limits copied to reservations", + project: &Project{Services: Services{ + "api": { + Name: "api", + Image: "api", + Restart: defaultRestartPolicy, + Deploy: &composeTypes.DeployConfig{Resources: composeTypes.Resources{ + Limits: &composeTypes.Resource{MemoryBytes: 1024 * MiB}, + }}, + }, + }}, + want: []FixResult{ + {Service: "api", Field: "deploy.resources.reservations", Action: "added", After: "copied from deploy.resources.limits", Reason: "Defang uses reservations for scheduling, not limits"}, + }, + check: func(t *testing.T, project *Project) { + service := project.Services["api"] + if service.Deploy.Resources.Reservations == nil { + t.Fatal("reservations were not added") + } + if service.Deploy.Resources.Reservations.MemoryBytes != 1024*MiB { + t.Fatalf("memory = %d, want %d", service.Deploy.Resources.Reservations.MemoryBytes, 1024*MiB) + } + }, + }, + { + name: "unsupported directives removed", + project: &Project{Services: Services{ + "worker": { + Name: "worker", + Image: "worker", + Restart: defaultRestartPolicy, + DNS: composeTypes.StringList{"1.1.1.1"}, + DNSSearch: composeTypes.StringList{"example.com"}, + Devices: []composeTypes.DeviceMapping{{Source: "/dev/null", Target: "/dev/null"}}, + DeviceCgroupRules: []string{"c 1:3 mr"}, + GroupAdd: []string{"audio"}, + }, + }}, + want: []FixResult{ + {Service: "worker", Field: "dns", Action: "removed", Before: "present", Reason: "unsupported directive"}, + {Service: "worker", Field: "dns_search", Action: "removed", Before: "present", Reason: "unsupported directive"}, + {Service: "worker", Field: "devices", Action: "removed", Before: "present", Reason: "unsupported directive"}, + {Service: "worker", Field: "device_cgroup_rules", Action: "removed", Before: "present", Reason: "unsupported directive"}, + {Service: "worker", Field: "group_add", Action: "removed", Before: "present", Reason: "unsupported directive"}, + }, + check: func(t *testing.T, project *Project) { + service := project.Services["worker"] + if len(service.DNS) != 0 || len(service.DNSSearch) != 0 || len(service.Devices) != 0 || len(service.DeviceCgroupRules) != 0 || len(service.GroupAdd) != 0 { + t.Fatal("unsupported directives were not removed") + } + }, + }, + { + name: "deploy restart policy converted to service restart", + project: &Project{Services: Services{ + "api": { + Name: "api", + Image: "api", + Deploy: &composeTypes.DeployConfig{ + RestartPolicy: &composeTypes.RestartPolicy{Condition: "any"}, + }, + }, + }}, + want: []FixResult{ + {Service: "api", Field: "restart", Action: "added", Before: "", After: "always", Reason: "deploy.restart_policy is unsupported; converted to service-level restart"}, + }, + check: func(t *testing.T, project *Project) { + service := project.Services["api"] + if service.Restart != "always" { + t.Fatalf("restart = %q, want %q", service.Restart, "always") + } + if service.Deploy.RestartPolicy != nil { + t.Fatal("deploy.restart_policy was not removed") + } + }, + }, + { + name: "udp port defaults to host mode", + project: &Project{Services: Services{ + "dns": { + Name: "dns", + Image: "coredns", + Restart: "always", + Ports: []composeTypes.ServicePortConfig{{Target: 53, Protocol: Protocol_UDP}}, + }, + }}, + want: []FixResult{ + {Service: "dns", Field: "mode", Action: "added", After: Mode_HOST, Reason: "port 53 (UDP port)"}, + }, + check: func(t *testing.T, project *Project) { + service := project.Services["dns"] + if service.Ports[0].Mode != Mode_HOST { + t.Fatalf("port mode = %q, want %q", service.Ports[0].Mode, Mode_HOST) + } + }, + }, + { + name: "no fixes needed", + project: &Project{Services: Services{ + "app": { + Name: "app", + Image: "myapp", + Restart: "always", + Ports: []composeTypes.ServicePortConfig{{Target: 80, Mode: Mode_INGRESS}}, + }, + }}, + want: nil, + check: func(t *testing.T, project *Project) {}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := FixProject(tt.project) + if !reflect.DeepEqual(got, tt.want) { + t.Fatalf("FixProject() = %#v, want %#v", got, tt.want) + } + tt.check(t, tt.project) + }) + } +} + +func TestFixProjectOutputValidates(t *testing.T) { + loader := NewLoader(WithPath("../../../testdata/compose-fix/compose.yaml")) + project, err := loader.LoadProject(t.Context()) + if err != nil { + t.Fatalf("LoadProject() failed: %v", err) + } + + if fixes := FixProject(project); len(fixes) == 0 { + t.Fatal("expected fixes") + } + if err := ValidateProject(project, modes.ModeUnspecified); err != nil { + t.Fatalf("ValidateProject() after FixProject() failed: %v", err) + } +} diff --git a/src/testdata/compose-fix/compose.yaml b/src/testdata/compose-fix/compose.yaml new file mode 100644 index 000000000..e24f8632d --- /dev/null +++ b/src/testdata/compose-fix/compose.yaml @@ -0,0 +1,12 @@ +name: compose-fix +services: + web: + image: nginx + ports: + - target: 8080 + dns: + - 1.1.1.1 + db: + image: postgres:16 + ports: + - target: 5432 diff --git a/src/testdata/compose-fix/compose.yaml.fixup b/src/testdata/compose-fix/compose.yaml.fixup new file mode 100644 index 000000000..16f9a6fba --- /dev/null +++ b/src/testdata/compose-fix/compose.yaml.fixup @@ -0,0 +1,36 @@ +{ + "db": { + "command": null, + "entrypoint": null, + "image": "postgres:16", + "networks": { + "default": null + }, + "ports": [ + { + "mode": "host", + "target": 5432, + "protocol": "tcp" + } + ] + }, + "web": { + "command": null, + "dns": [ + "1.1.1.1" + ], + "entrypoint": null, + "image": "nginx", + "networks": { + "default": null + }, + "ports": [ + { + "mode": "ingress", + "target": 8080, + "protocol": "tcp", + "app_protocol": "http" + } + ] + } +} \ No newline at end of file diff --git a/src/testdata/compose-fix/compose.yaml.golden b/src/testdata/compose-fix/compose.yaml.golden new file mode 100644 index 000000000..ea34d6304 --- /dev/null +++ b/src/testdata/compose-fix/compose.yaml.golden @@ -0,0 +1,23 @@ +name: compose-fix +services: + db: + image: postgres:16 + networks: + default: null + ports: + - mode: ingress + target: 5432 + protocol: tcp + web: + dns: + - 1.1.1.1 + image: nginx + networks: + default: null + ports: + - mode: ingress + target: 8080 + protocol: tcp +networks: + default: + name: compose-fix_default diff --git a/src/testdata/compose-fix/compose.yaml.warnings b/src/testdata/compose-fix/compose.yaml.warnings new file mode 100644 index 000000000..d31dc75d6 --- /dev/null +++ b/src/testdata/compose-fix/compose.yaml.warnings @@ -0,0 +1,3 @@ + ! service "db": missing memory reservation; using provider-specific defaults. Specify deploy.resources.reservations.memory to avoid out-of-memory errors + ! service "db": stateful service will lose data on restart; use a managed service instead +Error: service "web": unsupported compose directive: dns