diff --git a/cmd/uc/service/run.go b/cmd/uc/service/run.go index 7fb1f69a..838bcad1 100644 --- a/cmd/uc/service/run.go +++ b/cmd/uc/service/run.go @@ -208,6 +208,9 @@ func prepareServiceSpec(opts runOptions) (api.ServiceSpec, error) { if err != nil { return spec, err } + if err = api.ValidateLabels(labels); err != nil { + return spec, fmt.Errorf("invalid service labels: %w", err) + } containerLabels, err := parseLabels(opts.containerLabels) if err != nil { return spec, err diff --git a/internal/machine/docker/service.go b/internal/machine/docker/service.go index 1f169235..47e70db5 100644 --- a/internal/machine/docker/service.go +++ b/internal/machine/docker/service.go @@ -166,7 +166,7 @@ func (s *Service) InspectServiceContainer(ctx context.Context, nameOrID string) // It applies the CPU and memory limits to the container and updates the service spec stored for the container. func (s *Service) UpdateServiceContainer(ctx context.Context, id string, opts api.UpdateContainerOptions) error { if opts.ServiceLabels != nil { - if err := api.ValidateLabels(*opts.ServiceLabels); err != nil { + if err := api.ValidateLabelKeys(*opts.ServiceLabels); err != nil { return errdefs.ErrInvalidArgument.WithMessage(fmt.Sprintf("invalid service labels: %v", err)) } } diff --git a/pkg/api/service.go b/pkg/api/service.go index d8d910ff..8ece9b78 100644 --- a/pkg/api/service.go +++ b/pkg/api/service.go @@ -78,8 +78,8 @@ type ServiceSpec struct { Configs []ConfigSpec // Container defines the desired state of each container in the service. Container ContainerSpec - // Labels are user-defined key-value metadata attached to the service. Unlike container labels, they're not - // applied to Docker containers. Keys must not use the reserved "uncloud." prefix. + // Labels are key-value metadata attached to the service. Unlike container labels, they're not applied to Docker + // containers. The "uncloud." key prefix is reserved for labels set by Uncloud. Labels map[string]string // Mode is the replication mode of the service. Default is ServiceModeReplicated if empty. Mode string @@ -161,7 +161,9 @@ func (s *ServiceSpec) Validate() error { if err := s.Container.Validate(); err != nil { return err } - if err := ValidateLabels(s.Labels); err != nil { + // We want to allow reserved uncloud. labels in the service spec, because we can't distinguish between user-defined + // in a Compose file and those set by a uc command. + if err := ValidateLabelKeys(s.Labels); err != nil { return fmt.Errorf("invalid service labels: %w", err) } @@ -444,9 +446,8 @@ func (s *ContainerSpec) Clone() ContainerSpec { return spec } -// ValidateLabels checks that label keys are non-empty, don't contain whitespace, and don't use the reserved Uncloud -// prefixes. -func ValidateLabels(labels map[string]string) error { +// ValidateLabelKeys checks that label keys are non-empty and don't contain whitespace. +func ValidateLabelKeys(labels map[string]string) error { for k := range labels { if strings.TrimSpace(k) == "" { return fmt.Errorf("label key must not be empty") @@ -454,6 +455,17 @@ func ValidateLabels(labels map[string]string) error { if strings.ContainsFunc(k, unicode.IsSpace) { return fmt.Errorf("label '%s' must not contain whitespace", k) } + } + return nil +} + +// ValidateLabels checks that label keys are valid and don't use the reserved Uncloud prefixes. Use it for labels +// provided by users and for container labels. +func ValidateLabels(labels map[string]string) error { + if err := ValidateLabelKeys(labels); err != nil { + return err + } + for k := range labels { if strings.HasPrefix(k, LabelReservedPrefix) || strings.HasPrefix(k, LabelDaemonReservedPrefix) { return fmt.Errorf("label '%s' cannot use the reserved prefix '%s' or '%s'", k, LabelReservedPrefix, LabelDaemonReservedPrefix) diff --git a/pkg/api/service_test.go b/pkg/api/service_test.go index d07c7e61..13361320 100644 --- a/pkg/api/service_test.go +++ b/pkg/api/service_test.go @@ -82,8 +82,6 @@ func TestServiceSpec_Validate_Labels(t *testing.T) { {name: "trailing space in key", labels: map[string]string{"tier ": "bar"}, wantErr: "must not contain whitespace"}, {name: "tab in key", labels: map[string]string{"my\tlabel": "bar"}, wantErr: "must not contain whitespace"}, {name: "newline in key", labels: map[string]string{"my\nlabel": "bar"}, wantErr: "must not contain whitespace"}, - {name: "reserved prefix", labels: map[string]string{LabelManaged: ""}, wantErr: "reserved prefix"}, - {name: "reserved daemon prefix", labels: map[string]string{"uncloudd.foo": "bar"}, wantErr: "reserved prefix"}, } for _, tt := range tests { t.Run(tt.name+"/service", func(t *testing.T) { @@ -111,6 +109,38 @@ func TestServiceSpec_Validate_Labels(t *testing.T) { } } +func TestServiceSpec_Validate_ReservedLabels(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + labels map[string]string + }{ + {name: "reserved prefix", labels: map[string]string{LabelManaged: ""}}, + {name: "reserved daemon prefix", labels: map[string]string{"uncloudd.foo": "bar"}}, + {name: "managed service", labels: map[string]string{ + LabelReservedPrefix + "service.managed": "", + LabelReservedPrefix + "custom.key": "value", + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Uncloud sets its own labels on services, so a service spec can have them. + spec := ServiceSpec{Container: ContainerSpec{Image: "nginx:latest"}, Labels: tt.labels} + require.NoError(t, spec.Validate()) + + // Container labels and labels provided by users can't use the reserved prefixes. + spec = ServiceSpec{Container: ContainerSpec{Image: "nginx:latest", Labels: tt.labels}} + err := spec.Validate() + require.ErrorContains(t, err, "invalid container labels") + require.ErrorContains(t, err, "reserved prefix") + require.ErrorContains(t, ValidateLabels(tt.labels), "reserved prefix") + }) + } +} + func TestServiceSpec_Validate_CaddyAndPorts(t *testing.T) { tests := []struct { name string diff --git a/pkg/client/compose/service.go b/pkg/client/compose/service.go index 7838a79d..e01f3dda 100644 --- a/pkg/client/compose/service.go +++ b/pkg/client/compose/service.go @@ -130,6 +130,9 @@ func ServiceSpecFromCompose(project *types.Project, serviceName string) (api.Ser } if len(service.Deploy.Labels) > 0 { + if err = api.ValidateLabels(service.Deploy.Labels); err != nil { + return spec, fmt.Errorf("invalid deploy.labels: %w", err) + } spec.Labels = maps.Clone(service.Deploy.Labels) } } diff --git a/pkg/client/compose/service_test.go b/pkg/client/compose/service_test.go index a8d4b681..7e54f4ca 100644 --- a/pkg/client/compose/service_test.go +++ b/pkg/client/compose/service_test.go @@ -950,6 +950,42 @@ services: } } +func TestServiceSpecFromCompose_InvalidServiceLabels(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + label string + wantErr string + }{ + {name: "reserved prefix", label: api.LabelManaged, wantErr: "reserved prefix"}, + {name: "reserved daemon prefix", label: "uncloudd.foo", wantErr: "reserved prefix"}, + {name: "managed service", label: api.LabelReservedPrefix + "service.managed", wantErr: "reserved prefix"}, + {name: "whitespace in key", label: "my label", wantErr: "must not contain whitespace"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + composeYAML := ` +services: + web: + image: nginx + deploy: + labels: + - ` + tt.label + `=value +` + project, err := LoadProjectFromContent(context.Background(), composeYAML) + require.NoError(t, err) + + _, err = ServiceSpecFromCompose(project, "web") + require.ErrorContains(t, err, "invalid deploy.labels") + require.ErrorContains(t, err, tt.wantErr) + }) + } +} + func TestServiceSpecFromCompose_UpdateConfig(t *testing.T) { t.Parallel()