diff --git a/cmd/ateapi/internal/controlapi/actor_template.go b/cmd/ateapi/internal/controlapi/actor_template.go index 8c72f8d291..e69abc1118 100644 --- a/cmd/ateapi/internal/controlapi/actor_template.go +++ b/cmd/ateapi/internal/controlapi/actor_template.go @@ -357,3 +357,58 @@ func actorTemplateObjectRef(actor *ateapipb.Actor) *ateapipb.ObjectRef { } return &ateapipb.ObjectRef{Atespace: ref.GetAtespace(), Name: ref.GetName()} } + +// ValidateCustom_Container_VolumeMounts rejects two mounts at the same path +// within one container. The list is keyed by volume name (one mount per +// volume), so path uniqueness cannot come from the list-map key +func ValidateCustom_Container_VolumeMounts(_ context.Context, _ operation.Operation, fldPath *field.Path, value, _ []*ateapipb.VolumeMount) field.ErrorList { + var errs field.ErrorList + seen := make(map[string]bool, len(value)) + for i, m := range value { + path := m.GetMountPath() + if path == "" { + continue // required is enforced by tags + } + if seen[path] { + errs = append(errs, field.Duplicate(fldPath.Index(i).Child("mount_path"), path)) + } + // Nested mounts are unsupported (volumes cannot mount onto + // other volumes). + for j := 0; j < i; j++ { + prior := value[j].GetMountPath() + if prior == "" || prior == path { + continue + } + if strings.HasPrefix(path, prior+"/") || strings.HasPrefix(prior, path+"/") { + errs = append(errs, field.Invalid(fldPath.Index(i).Child("mount_path"), path, + fmt.Sprintf("must not nest under or over another mount (%q)", prior))) + } + } + seen[path] = true + } + return errs +} + +// ValidateCustom_CreateActorTemplateRequest_ActorTemplate rejects container +// volume mounts that reference volumes the template does not declare. +func ValidateCustom_CreateActorTemplateRequest_ActorTemplate(_ context.Context, _ operation.Operation, fldPath *field.Path, value, _ *ateapipb.ActorTemplate) field.ErrorList { + declared := make(map[string]bool, len(value.GetVolumes())) + for _, vol := range value.GetVolumes() { + declared[vol.GetName()] = true + } + var errs field.ErrorList + for i, ctr := range value.GetContainers() { + for j, mount := range ctr.GetVolumeMounts() { + name := mount.GetName() + if name == "" { + continue // required is enforced by tags + } + if !declared[name] { + errs = append(errs, field.Invalid( + fldPath.Child("containers").Index(i).Child("volume_mounts").Index(j).Child("name"), + name, "must reference a volume declared in the template")) + } + } + } + return errs +} diff --git a/cmd/ateapi/internal/controlapi/actor_template_test.go b/cmd/ateapi/internal/controlapi/actor_template_test.go index 5ae47b5554..7a810c4954 100644 --- a/cmd/ateapi/internal/controlapi/actor_template_test.go +++ b/cmd/ateapi/internal/controlapi/actor_template_test.go @@ -121,6 +121,19 @@ func TestValidateCreateActorTemplateRequest(t *testing.T) { tmpl.Containers[0].Image = "" })}, field.ErrorList{field.Required(field.NewPath("actor_template", "containers").Index(0).Child("image"), "")}, + }, { + "volume mount referencing a declared volume", + &ateapipb.CreateActorTemplateRequest{ActorTemplate: validActorTemplate(func(tmpl *ateapipb.ActorTemplate) { + tmpl.Volumes = []*ateapipb.Volume{{Name: "data", DurableDir: &ateapipb.DurableDirVolumeSource{}}} + tmpl.Containers[0].VolumeMounts = []*ateapipb.VolumeMount{{Name: "data", MountPath: "/var/data"}} + })}, + nil, + }, { + "volume mount referencing an undeclared volume", + &ateapipb.CreateActorTemplateRequest{ActorTemplate: validActorTemplate(func(tmpl *ateapipb.ActorTemplate) { + tmpl.Containers[0].VolumeMounts = []*ateapipb.VolumeMount{{Name: "ghost-vol", MountPath: "/var/data"}} + })}, + field.ErrorList{field.Invalid(field.NewPath("actor_template", "containers").Index(0).Child("volume_mounts").Index(0).Child("name"), "ghost-vol", "")}, }, { "missing snapshots_config", &ateapipb.CreateActorTemplateRequest{ActorTemplate: validActorTemplate(func(tmpl *ateapipb.ActorTemplate) { @@ -487,6 +500,73 @@ func TestValidateActorTemplate(t *testing.T) { } }, want: field.ErrorList{field.Duplicate(field.NewPath("containers").Index(0).Child("volume_mounts").Index(1), nil)}, + }, { + name: "two volumes at the same path are rejected", + mutate: func(tmpl *ateapipb.ActorTemplate) { + tmpl.Containers[0].VolumeMounts = []*ateapipb.VolumeMount{ + {Name: "data", MountPath: "/var/data"}, + {Name: "other", MountPath: "/var/data"}, + } + }, + want: field.ErrorList{field.Duplicate(field.NewPath("containers").Index(0).Child("volume_mounts").Index(1).Child("mount_path"), nil)}, + }, { + name: "nested mount paths are rejected", + mutate: func(tmpl *ateapipb.ActorTemplate) { + tmpl.Containers[0].VolumeMounts = []*ateapipb.VolumeMount{ + {Name: "data", MountPath: "/data"}, + {Name: "config", MountPath: "/data/config"}, + } + }, + want: field.ErrorList{field.Invalid(field.NewPath("containers").Index(0).Child("volume_mounts").Index(1).Child("mount_path"), nil, "")}, + }, { + name: "deeply nested mount path is rejected", + mutate: func(tmpl *ateapipb.ActorTemplate) { + tmpl.Containers[0].VolumeMounts = []*ateapipb.VolumeMount{ + {Name: "data", MountPath: "/data"}, + {Name: "deep", MountPath: "/data/a/b/c"}, + } + }, + want: field.ErrorList{field.Invalid(field.NewPath("containers").Index(0).Child("volume_mounts").Index(1).Child("mount_path"), nil, "")}, + }, { + name: "nesting is rejected regardless of listing order", + mutate: func(tmpl *ateapipb.ActorTemplate) { + tmpl.Containers[0].VolumeMounts = []*ateapipb.VolumeMount{ + {Name: "deep", MountPath: "/data/a/b/c"}, + {Name: "data", MountPath: "/data"}, + } + }, + want: field.ErrorList{field.Invalid(field.NewPath("containers").Index(0).Child("volume_mounts").Index(1).Child("mount_path"), nil, "")}, + }, { + name: "sibling subpaths under an unmounted ancestor are allowed", + mutate: func(tmpl *ateapipb.ActorTemplate) { + tmpl.Containers[0].VolumeMounts = []*ateapipb.VolumeMount{ + {Name: "data", MountPath: "/data/a"}, + {Name: "other", MountPath: "/data/b"}, + } + }, + }, { + name: "shared segment prefix below the root is allowed", + mutate: func(tmpl *ateapipb.ActorTemplate) { + tmpl.Containers[0].VolumeMounts = []*ateapipb.VolumeMount{ + {Name: "data", MountPath: "/data/a"}, + {Name: "other", MountPath: "/data/ab"}, + } + }, + }, { + name: "the same path in different containers is allowed", + mutate: func(tmpl *ateapipb.ActorTemplate) { + tmpl.Containers = append(tmpl.Containers, &ateapipb.Container{Name: "sidecar", Image: "example.com/side:v1"}) + tmpl.Containers[0].VolumeMounts = []*ateapipb.VolumeMount{{Name: "data", MountPath: "/var/data"}} + tmpl.Containers[1].VolumeMounts = []*ateapipb.VolumeMount{{Name: "data", MountPath: "/var/data"}} + }, + }, { + name: "shared path prefix without nesting is allowed", + mutate: func(tmpl *ateapipb.ActorTemplate) { + tmpl.Containers[0].VolumeMounts = []*ateapipb.VolumeMount{ + {Name: "data", MountPath: "/data"}, + {Name: "other", MountPath: "/database"}, + } + }, }, { name: "two volumes at distinct paths are allowed", mutate: func(tmpl *ateapipb.ActorTemplate) { diff --git a/cmd/ateapi/internal/controlapi/zz_generated.validation.go b/cmd/ateapi/internal/controlapi/zz_generated.validation.go index 4fffe646e4..f5c9074d21 100644 --- a/cmd/ateapi/internal/controlapi/zz_generated.validation.go +++ b/cmd/ateapi/internal/controlapi/zz_generated.validation.go @@ -1688,6 +1688,10 @@ func Validate_Container( if earlyReturn { return // do not proceed } + // custom validation + if e := ValidateCustom_Container_VolumeMounts(ctx, op, fldPath, obj, oldObj); len(e) != 0 { + errs = append(errs, e...) + } // lists with map semantics require unique keys if e := validate.PtrSliceUnique(ctx, op, fldPath, obj, oldObj, func(a *ateapipb.VolumeMount, b *ateapipb.VolumeMount) bool { return a.Name == b.Name }); len(e) != 0 { @@ -2001,6 +2005,10 @@ func Validate_CreateActorTemplateRequest( if earlyReturn { return // do not proceed } + // custom validation + if e := ValidateCustom_CreateActorTemplateRequest_ActorTemplate(ctx, op, fldPath, obj, oldObj); len(e) != 0 { + errs = append(errs, e...) + } // call the type's validation function errs = append(errs, Validate_ActorTemplate(ctx, op, fldPath, obj, oldObj)...) return diff --git a/pkg/proto/ateapipb/ateapi.pb.go b/pkg/proto/ateapipb/ateapi.pb.go index ea5121e4c9..cbe2de546d 100644 --- a/pkg/proto/ateapipb/ateapi.pb.go +++ b/pkg/proto/ateapipb/ateapi.pb.go @@ -2701,17 +2701,15 @@ type Container struct { // // +k8s:optional Readyz *ContainerReadyz `protobuf:"bytes,6,opt,name=readyz,proto3" json:"readyz,omitempty"` - // Each volume may be mounted at most once per container. - // - // TODO: Kubernetes permits mounting a single volume at multiple paths (which - // requires keying by mountPath). We restrict it to one mount per volume (keyed - // by name). Note that keying by name means DV will not catch - // two different volumes mounted to the same path. + // TODO: Kubernetes permits mounting a single volume at multiple paths + // (which requires keying by mountPath). We restrict it to one mount per + // volume (keyed by name). // // +k8s:optional // +k8s:maxItems=32 // +k8s:listType=map // +k8s:listMapKey=name + // +k8s:customValidation # mount_path must be unique within the container VolumeMounts []*VolumeMount `protobuf:"bytes,7,rep,name=volume_mounts,json=volumeMounts,proto3" json:"volume_mounts,omitempty"` // security_context adjusts the container's security settings. Unset leaves // the default capability set. @@ -3984,6 +3982,7 @@ type CreateActorTemplateRequest struct { // version, timestamps) is ignored, as are the status fields. // // +k8s:required + // +k8s:customValidation # volume_mounts must reference declared volumes ActorTemplate *ActorTemplate `protobuf:"bytes,1,opt,name=actor_template,json=actorTemplate,proto3" json:"actor_template,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache diff --git a/pkg/proto/ateapipb/ateapi.proto b/pkg/proto/ateapipb/ateapi.proto index ed94c0aac1..a39498a146 100644 --- a/pkg/proto/ateapipb/ateapi.proto +++ b/pkg/proto/ateapipb/ateapi.proto @@ -923,17 +923,15 @@ message Container { // +k8s:optional ContainerReadyz readyz = 6; - // Each volume may be mounted at most once per container. - // - // TODO: Kubernetes permits mounting a single volume at multiple paths (which - // requires keying by mountPath). We restrict it to one mount per volume (keyed - // by name). Note that keying by name means DV will not catch - // two different volumes mounted to the same path. + // TODO: Kubernetes permits mounting a single volume at multiple paths + // (which requires keying by mountPath). We restrict it to one mount per + // volume (keyed by name). // // +k8s:optional // +k8s:maxItems=32 // +k8s:listType=map // +k8s:listMapKey=name + // +k8s:customValidation # mount_path must be unique within the container repeated VolumeMount volume_mounts = 7; // security_context adjusts the container's security settings. Unset leaves @@ -1236,6 +1234,7 @@ message CreateActorTemplateRequest { // version, timestamps) is ignored, as are the status fields. // // +k8s:required + // +k8s:customValidation # volume_mounts must reference declared volumes ActorTemplate actor_template = 1; }