Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 90 additions & 1 deletion src/pkg/cli/compose/fixup.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,13 @@ func FixupServices(ctx context.Context, provider client.Provider, project *compo
}
}

_, managedS3 := svccfg.Extensions["x-defang-s3"]
if managedS3 || IsMinioRepo(repo) {
if err := fixupS3Service(&svccfg, project, provider, accountInfo, upload); err != nil {
return fmt.Errorf("service %q: %w", svccfg.Name, err)
}
}

if len(svccfg.Name) > 16 {
term.Warnf("service %q: service name is longer than 16 characters, you may run into issues with resource name length", svccfg.Name)
}
Expand Down Expand Up @@ -300,6 +307,76 @@ func fixupPostgresService(svccfg *composeTypes.ServiceConfig, provider client.Pr
return nil
}

// fixupS3Service wires the bucket and region into every service that
// depends_on the MinIO anchor, mirroring the model-provider convention in
// wireDependentServices.
//
// Unlike postgres/redis/mongo, this deliberately adds no host port. That HACK
// exists only to earn the service a CNAME, and a CNAME cannot carry an S3
// endpoint: bucket names are globally unique, and TLS SNI would not match a
// private name. The endpoint is the provider's to supply, because only it
// knows the shape — native S3/GCS on AWS and GCP (where the SDK's own default
// endpoint is usually right), and the s3proxy service, on its own port, on
// Azure. So the CLI never synthesizes one.
func fixupS3Service(svccfg *composeTypes.ServiceConfig, project *composeTypes.Project, provider client.Provider, accountInfo *client.AccountInfo, upload UploadMode) error {
s3Extension, managedS3 := svccfg.Extensions["x-defang-s3"]
if _, ok := provider.(*client.PlaygroundProvider); ok && managedS3 && upload != UploadModeEstimate {
term.Warnf("service %q: managed S3 is not supported in the Playground; consider using BYOC (https://s.defang.io/byoc)", svccfg.Name)
}
fixupIngressPorts(svccfg) // the anchor is not a public endpoint

if !managedS3 {
return nil
}

bucket, err := validateS3Store(s3Extension)
if err != nil {
return err
}

// Inject no region when the provider didn't report one: AccountInfo may
// simply have failed (FixupServices treats that as non-fatal and carries on
// with a zero value), and a guessed region is worse than none. It would
// override the SDK's own resolution with a value that is wrong whenever the
// deployment isn't in that guess, and SigV4 then fails at runtime against a
// bucket in the real region. Absent, the SDK resolves the region itself from
// the task environment. This mirrors configureAccessGateway, which sets
// AWS_REGION only when info.Region is non-empty.
envName := strings.ToUpper(svccfg.Name) // TODO: handle characters that are not allowed in env vars, like '-'
wireS3DependentServices(project, svccfg.Name, bucket, accountInfo.Region, envName+"_BUCKET", envName+"_REGION")
return nil
}

// wireS3DependentServices injects the bucket/region env vars into every
// service that depends_on svcName. It never overwrites a value the author
// already set, including an endpoint they point somewhere themselves.
//
// Why two variables rather than one <SVC>_URL, as model providers get: an S3
// endpoint URL cannot carry the bucket. Every mainstream S3 client takes the
// bucket as a per-call API parameter, not as client config, so it has to
// arrive as a variable of its own. The region is separate for the same
// reason — SigV4 needs it, and non-AWS clients don't read AWS_REGION by
// themselves. The endpoint is the one value the CLI does not inject at all:
// on AWS it should be absent, so the SDK uses its own default, and where a
// cloud does need one, only the provider knows it (see fixupS3Service).
func wireS3DependentServices(project *composeTypes.Project, svcName, bucket, region, bucketEnvVar, regionEnvVar string) {
for name, dependency := range project.Services {
if _, ok := dependency.DependsOn[svcName]; !ok {
continue
}
if dependency.Environment == nil {
dependency.Environment = make(composeTypes.MappingWithEquals)
}
if _, ok := dependency.Environment[bucketEnvVar]; !ok {
dependency.Environment[bucketEnvVar] = &bucket
}
if _, ok := dependency.Environment[regionEnvVar]; !ok && region != "" {
dependency.Environment[regionEnvVar] = &region
}
project.Services[name] = dependency
}
}

func fixupMongoService(svccfg *composeTypes.ServiceConfig, provider client.Provider, upload UploadMode) error {
_, managedMongo := svccfg.Extensions["x-defang-mongodb"]
if _, ok := provider.(*client.PlaygroundProvider); ok && managedMongo && upload != UploadModeEstimate {
Expand Down Expand Up @@ -589,8 +666,16 @@ func modelWithProvider(model, prefix string) string {
return prefix + "/" + model
}

// GetImageRepo returns the lowercase repository of an image reference, without
// its tag or digest: minio/minio, minio/minio:latest and
// minio/minio@sha256:<digest> all yield "minio/minio", so the managed-service
// image checks below recognize a digest-pinned image too. A colon inside the
// registry host is a port, not a tag.
func GetImageRepo(image string) string {
repo, _, _ := strings.Cut(image, ":")
repo, _, _ := strings.Cut(image, "@") // strip the digest, if any
if i := strings.LastIndex(repo, ":"); i > strings.LastIndex(repo, "/") {
repo = repo[:i] // strip the tag, but keep a registry port
}
return strings.ToLower(repo)
}

Expand Down Expand Up @@ -634,3 +719,7 @@ func IsRedisRepo(repo string) bool {
func IsMongoRepo(repo string) bool {
return strings.HasSuffix(repo, "mongo")
}

func IsMinioRepo(repo string) bool {
return strings.HasSuffix(repo, "minio")
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
111 changes: 111 additions & 0 deletions src/pkg/cli/compose/fixup_test.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package compose

import (
"context"
"strings"
"testing"

"github.com/DefangLabs/defang/src/pkg"
"github.com/DefangLabs/defang/src/pkg/cli/client"
"github.com/aws/smithy-go/ptr"
composeTypes "github.com/compose-spec/compose-go/v2/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -359,3 +361,112 @@ func TestModelWithProvider(t *testing.T) {
assert.Equal(t, "vertex_ai/gemini-2.5-flash", modelWithProvider("gemini-2.5-flash", "vertex_ai"))
assert.Equal(t, "vertex_ai/gemini-2.5-flash", modelWithProvider("vertex_ai/gemini-2.5-flash", "vertex_ai"))
}

func TestGetImageRepo(t *testing.T) {
tests := []struct {
image string
want string
}{
{image: "minio/minio", want: "minio/minio"},
{image: "minio/minio:RELEASE.2024-01-01T00-00-00Z", want: "minio/minio"},
{image: "minio/minio@sha256:0000000000000000000000000000000000000000000000000000000000000000", want: "minio/minio"},
{image: "minio/minio:latest@sha256:0000000000000000000000000000000000000000000000000000000000000000", want: "minio/minio"},
{image: "MinIO/MinIO", want: "minio/minio"},
{image: "registry.example.com:5000/minio/minio", want: "registry.example.com:5000/minio/minio"},
{image: "registry.example.com:5000/minio/minio:latest", want: "registry.example.com:5000/minio/minio"},
{image: "", want: ""},
}
for _, tt := range tests {
t.Run(tt.image, func(t *testing.T) {
if got := GetImageRepo(tt.image); got != tt.want {
t.Errorf("GetImageRepo(%q) = %q, want %q", tt.image, got, tt.want)
}
})
}
}

// s3RegionProvider is a MockProvider whose AccountInfo carries a region, so the
// region-present branch of fixupS3Service can be exercised: the shared
// MockProvider reports none.
type s3RegionProvider struct {
client.MockProvider
region string
}

func (p s3RegionProvider) AccountInfo(context.Context) (*client.AccountInfo, error) {
return &client.AccountInfo{Provider: client.ProviderAWS, Region: p.region}, nil
}

func TestFixupS3ServiceRegion(t *testing.T) {
tests := []struct {
name string
region string
preset *string
wantRegion *string // nil = the var must not be injected at all
}{
{
name: "provider reports a region",
region: "eu-west-2",
wantRegion: ptr.String("eu-west-2"),
},
{
// No region means AccountInfo failed or reported none. Guessing one
// would override the SDK's own resolution with a value that breaks
// SigV4 whenever the guess is wrong, so inject nothing.
name: "provider reports no region",
region: "",
wantRegion: nil,
},
{
name: "author's own value wins over the provider's",
region: "eu-west-2",
preset: ptr.String("ap-south-1"),
wantRegion: ptr.String("ap-south-1"),
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
dependent := composeTypes.ServiceConfig{
Name: "app",
DependsOn: composeTypes.DependsOnConfig{"objs": composeTypes.ServiceDependency{}},
Environment: composeTypes.MappingWithEquals{},
}
if tt.preset != nil {
dependent.Environment["OBJS_REGION"] = tt.preset
}
project := &composeTypes.Project{Services: composeTypes.Services{"app": dependent}}
anchor := composeTypes.ServiceConfig{
Name: "objs",
Image: "minio/minio",
Extensions: map[string]any{"x-defang-s3": map[string]any{"bucket": "buzz-media-prod"}},
}

provider := s3RegionProvider{region: tt.region}
info, err := provider.AccountInfo(t.Context())
if err != nil {
t.Fatal(err)
}
if err := fixupS3Service(&anchor, project, provider, info, UploadModeIgnore); err != nil {
t.Fatal(err)
}

env := project.Services["app"].Environment
got, ok := env["OBJS_REGION"]
if tt.wantRegion == nil {
if ok {
t.Errorf("OBJS_REGION = %q, want it not to be injected", *got)
}
} else if !ok {
t.Errorf("OBJS_REGION not injected, want %q", *tt.wantRegion)
} else if *got != *tt.wantRegion {
t.Errorf("OBJS_REGION = %q, want %q", *got, *tt.wantRegion)
}

// The bucket is injected either way; only the region is conditional.
if bucket, ok := env["OBJS_BUCKET"]; !ok || *bucket != "buzz-media-prod" {
t.Errorf("OBJS_BUCKET = %v, want %q", bucket, "buzz-media-prod")
}
})
}
}
2 changes: 1 addition & 1 deletion src/pkg/cli/compose/stateful.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ var statefulImages = []string{
}

func isStatefulImage(image string) bool {
repo := strings.ToLower(strings.SplitN(image, ":", 2)[0])
repo := GetImageRepo(image)
for _, statefulImage := range statefulImages {
if strings.HasSuffix(repo, statefulImage) {
return true
Expand Down
5 changes: 5 additions & 0 deletions src/pkg/cli/compose/stateful_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ func TestIsStatefulImage(t *testing.T) {
image: "docker.io/redis",
expected: true,
},
{
name: "Stateful image pinned by digest",
image: "minio/minio@sha256:0000000000000000000000000000000000000000000000000000000000000000",
expected: true,
},
{
name: "Stateless image",
image: "alpine:latest",
Expand Down
52 changes: 51 additions & 1 deletion src/pkg/cli/compose/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,18 @@ func validateService(svccfg *composeTypes.ServiceConfig, project *composeTypes.P
}
}

if !managedRedis && !managedPostgres && !managedMongodb && isStatefulImage(svccfg.Image) {
s3Extension, managedS3 := svccfg.Extensions["x-defang-s3"]
if managedS3 {
// Ensure the repo is a valid MinIO repo
if !IsMinioRepo(repo) {
term.Warnf("service %q: managed S3 service should use a minio image", svccfg.Name)
}
if _, err = validateS3Store(s3Extension); err != nil {
return fmt.Errorf("service %q: %w", svccfg.Name, err)
}
}

if !managedRedis && !managedPostgres && !managedMongodb && !managedS3 && isStatefulImage(svccfg.Image) {
term.Warnf("service %q: stateful service will lose data on restart; use a managed service instead", svccfg.Name)
}

Expand All @@ -386,6 +397,7 @@ func validateService(svccfg *composeTypes.ServiceConfig, project *composeTypes.P
"x-defang-redis",
"x-defang-postgres",
"x-defang-mongodb",
"x-defang-s3",
"x-defang-llm",
"x-defang-autoscaling",
// Consumed by the CD provider, not the CLI, but still valid and
Expand Down Expand Up @@ -568,6 +580,43 @@ func validateManagedStore(managedStore any) (bool, error) {
}
}

var bucketNameRegex = regexp.MustCompile(`^[a-z0-9][a-z0-9-]*[a-z0-9]$`)

// validateS3Store requires a declared bucket name: bucket names must be
// globally unique, and the CLI injects the endpoint/bucket into dependent
// services' env before anything is provisioned, so a generated name isn't
// known yet. No dots — a dot breaks the wildcard TLS cert on virtual-hosted
// S3 endpoints.
func validateS3Store(managedStore any) (string, error) {
m, ok := managedStore.(map[string]any)
if !ok {
return "", errors.New("'x-defang-s3' requires a 'bucket' name")
}
b, ok := m["bucket"]
if !ok {
return "", errors.New("'x-defang-s3' requires a 'bucket' name")
}
bucket, ok := b.(string)
if !ok {
return "", errors.New("'bucket' must be a string")
}
if len(bucket) < 3 || len(bucket) > 63 {
return "", fmt.Errorf("'bucket' %q must be between 3 and 63 characters", bucket)
}
if strings.Contains(bucket, ".") {
return "", fmt.Errorf("'bucket' %q must not contain dots", bucket)
}
if !bucketNameRegex.MatchString(bucket) {
return "", fmt.Errorf("'bucket' %q must use only lowercase letters, digits and hyphens, and start/end with a letter or digit", bucket)
}
if downtime, ok := m["allow-downtime"]; ok {
if _, ok := downtime.(bool); !ok {
return "", errors.New("'allow-downtime' must be a boolean")
}
}
return bucket, nil
}

func IsComputeService(service *composeTypes.ServiceConfig) bool {
if service.Extensions == nil {
return true
Expand All @@ -576,5 +625,6 @@ func IsComputeService(service *composeTypes.ServiceConfig) bool {
return service.Extensions["x-defang-static-files"] == nil &&
service.Extensions["x-defang-redis"] == nil &&
service.Extensions["x-defang-mongodb"] == nil &&
service.Extensions["x-defang-s3"] == nil &&
service.Extensions["x-defang-postgres"] == nil
}
Loading
Loading