Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Mount Propagation as a Optional Feature #15758

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
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
4 changes: 4 additions & 0 deletions cmd/schema-tweak/overrides.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,10 @@ func revSpecOverrides(prefixPath string) []entry {
"mountPath",
"subPath",
),
featureFlagFields: []flagField{{
name: "mountPropagation",
flag: config.FeaturePodSpecMountPropagation,
}},
}, {
path: "volumes",
allowedFields: sets.New(
Expand Down
4 changes: 4 additions & 0 deletions config/core/300-resources/configuration.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -926,6 +926,10 @@ spec:
Path within the container at which the volume should be mounted. Must
not contain ':'.
type: string
mountPropagation:
description: |-
This is accessible behind a feature flag - kubernetes.podspec-mount-propagation
type: string
name:
description: This must match the Name of a Volume.
type: string
Expand Down
4 changes: 4 additions & 0 deletions config/core/300-resources/revision.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -902,6 +902,10 @@ spec:
Path within the container at which the volume should be mounted. Must
not contain ':'.
type: string
mountPropagation:
description: |-
This is accessible behind a feature flag - kubernetes.podspec-mount-propagation
type: string
name:
description: This must match the Name of a Volume.
type: string
Expand Down
4 changes: 4 additions & 0 deletions config/core/300-resources/service.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,10 @@ spec:
Path within the container at which the volume should be mounted. Must
not contain ':'.
type: string
mountPropagation:
description: |-
This is accessible behind a feature flag - kubernetes.podspec-mount-propagation
type: string
name:
description: This must match the Name of a Volume.
type: string
Expand Down
7 changes: 6 additions & 1 deletion config/core/configmaps/features.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ metadata:
app.kubernetes.io/component: controller
app.kubernetes.io/version: devel
annotations:
knative.dev/example-checksum: "63a13754"
knative.dev/example-checksum: "f3931300"
data:
_example: |-
################################
Expand Down Expand Up @@ -223,6 +223,11 @@ data:
# 2. Disabled: disabling write access for persistent volumes
kubernetes.podspec-persistent-volume-write: "disabled"

# Controls whether volume mount propagation support is enabled or not.
# 1. Enabled: enabling volume mount propagation support
# 2. Disabled: disabling volume mount propagation support
kubernetes.podspec-mount-propagation: "disabled"

# Controls if the queue proxy podInfo feature is enabled, allowed or disabled
#
# This feature should be enabled/allowed when using queue proxy Options (Extensions)
Expand Down
4 changes: 4 additions & 0 deletions pkg/apis/config/features.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ const (
FeaturePodSpecHostPID = "kubernetes.podspec-hostpid"
FeaturePodSpecHostPath = "kubernetes.podspec-volumes-hostpath"
FeaturePodSpecInitContainers = "kubernetes.podspec-init-containers"
FeaturePodSpecMountPropagation = "kubernetes.podspec-mount-propagation"
FeaturePodSpecNodeSelector = "kubernetes.podspec-nodeselector"
FeaturePodSpecPVClaim = "kubernetes.podspec-persistent-volume-claim"
FeaturePodSpecPriorityClassName = "kubernetes.podspec-priorityclassname"
Expand Down Expand Up @@ -99,6 +100,7 @@ func defaultFeaturesConfig() *Features {
PodSpecTolerations: Disabled,
PodSpecVolumesEmptyDir: Enabled,
PodSpecVolumesHostPath: Disabled,
PodSpecVolumeMountPropagation: Disabled,
PodSpecPersistentVolumeClaim: Disabled,
PodSpecPersistentVolumeWrite: Disabled,
QueueProxyMountPodInfo: Disabled,
Expand Down Expand Up @@ -139,6 +141,7 @@ func NewFeaturesConfigFromMap(data map[string]string) (*Features, error) {
asFlag(FeaturePodSpecHostPID, &nc.PodSpecHostPID),
asFlag(FeaturePodSpecHostPath, &nc.PodSpecVolumesHostPath),
asFlag(FeaturePodSpecInitContainers, &nc.PodSpecInitContainers),
asFlag(FeaturePodSpecMountPropagation, &nc.PodSpecVolumeMountPropagation),
asFlag(FeaturePodSpecNodeSelector, &nc.PodSpecNodeSelector),
asFlag(FeaturePodSpecPVClaim, &nc.PodSpecPersistentVolumeClaim),
asFlag(FeaturePodSpecPriorityClassName, &nc.PodSpecPriorityClassName),
Expand Down Expand Up @@ -181,6 +184,7 @@ type Features struct {
PodSpecTolerations Flag
PodSpecVolumesEmptyDir Flag
PodSpecVolumesHostPath Flag
PodSpecVolumeMountPropagation Flag
PodSpecInitContainers Flag
PodSpecPersistentVolumeClaim Flag
PodSpecPersistentVolumeWrite Flag
Expand Down
18 changes: 18 additions & 0 deletions pkg/apis/config/features_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,24 @@ func TestFeaturesConfiguration(t *testing.T) {
data: map[string]string{
"kubernetes.podspec-persistent-volume-claim": "Enabled",
},
}, {
name: "kubernetes.podspec-mount-propagation Disabled",
wantErr: false,
wantFeatures: defaultWith(&Features{
PodSpecVolumeMountPropagation: Disabled,
}),
data: map[string]string{
"kubernetes.podspec-mount-propagation": "Disabled",
},
}, {
name: "kubernetes.podspec-mount-propagation Enabled",
wantErr: false,
wantFeatures: defaultWith(&Features{
PodSpecVolumeMountPropagation: Enabled,
}),
data: map[string]string{
"kubernetes.podspec-mount-propagation": "Enabled",
},
}, {
name: "kubernetes.podspec-persistent-volume-write Disabled",
wantErr: false,
Expand Down
10 changes: 8 additions & 2 deletions pkg/apis/serving/fieldmask.go
Original file line number Diff line number Diff line change
Expand Up @@ -333,22 +333,28 @@ func ContainerMask(in *corev1.Container) *corev1.Container {
// VolumeMountMask performs a _shallow_ copy of the Kubernetes VolumeMount object to a new
// Kubernetes VolumeMount object bringing over only the fields allowed in the Knative API. This
// does not validate the contents or the bounds of the provided fields.
func VolumeMountMask(in *corev1.VolumeMount) *corev1.VolumeMount {
func VolumeMountMask(ctx context.Context, in *corev1.VolumeMount) *corev1.VolumeMount {
if in == nil {
return nil
}

cfg := config.FromContextOrDefaults(ctx)
out := new(corev1.VolumeMount)

// Allowed fields
out.Name = in.Name
out.ReadOnly = in.ReadOnly
out.MountPath = in.MountPath
out.SubPath = in.SubPath
if cfg.Features.PodSpecVolumeMountPropagation != config.Disabled {
out.MountPropagation = in.MountPropagation
} else {
out.MountPropagation = nil
}

// Disallowed fields
// This list is unnecessary, but added here for clarity
out.MountPropagation = nil
out.RecursiveReadOnly = nil

return out
}
Expand Down
51 changes: 48 additions & 3 deletions pkg/apis/serving/fieldmask_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,8 @@ func TestContainerMask(t *testing.T) {
}

func TestVolumeMountMask(t *testing.T) {
mode := corev1.MountPropagationBidirectional
mode := corev1.MountPropagationHostToContainer
ctx := context.Background()

want := &corev1.VolumeMount{
Name: "foo",
Expand All @@ -331,7 +332,51 @@ func TestVolumeMountMask(t *testing.T) {
MountPropagation: &mode,
}

got := VolumeMountMask(in)
got := VolumeMountMask(ctx, in)

if &want == &got {
t.Error("Input and output share addresses. Want different addresses")
}

if diff, err := kmp.SafeDiff(want, got); err != nil {
t.Error("Got error comparing output, err =", err)
} else if diff != "" {
t.Error("VolumeMountMask (-want, +got):", diff)
}

if got = VolumeMountMask(ctx, nil); got != nil {
t.Errorf("VolumeMountMask(nil) = %v, want: nil", got)
}
}

func TestVolumeMountMask_FeatMountPropagation(t *testing.T) {
mode := corev1.MountPropagationHostToContainer
rrr := corev1.RecursiveReadOnlyEnabled
ctx := config.ToContext(context.Background(),
&config.Config{
Features: &config.Features{
PodSpecVolumeMountPropagation: config.Enabled,
},
},
)

want := &corev1.VolumeMount{
Name: "foo",
ReadOnly: true,
MountPath: "/foo/bar",
SubPath: "baz",
MountPropagation: &mode,
}
in := &corev1.VolumeMount{
Name: "foo",
ReadOnly: true,
MountPath: "/foo/bar",
SubPath: "baz",
MountPropagation: &mode,
RecursiveReadOnly: &rrr,
}

got := VolumeMountMask(ctx, in)

if &want == &got {
t.Error("Input and output share addresses. Want different addresses")
Expand All @@ -343,7 +388,7 @@ func TestVolumeMountMask(t *testing.T) {
t.Error("VolumeMountMask (-want, +got):", diff)
}

if got = VolumeMountMask(nil); got != nil {
if got = VolumeMountMask(ctx, nil); got != nil {
t.Errorf("VolumeMountMask(nil) = %v, want: nil", got)
}
}
Expand Down
21 changes: 18 additions & 3 deletions pkg/apis/serving/k8s_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,8 @@
errs = errs.Also(apis.ErrInvalidValue(container.TerminationMessagePolicy, "terminationMessagePolicy"))
}
// VolumeMounts
errs = errs.Also(validateVolumeMounts(container.VolumeMounts, volumes).ViaField("volumeMounts"))
isPrivilegedContainer := container.SecurityContext != nil && container.SecurityContext.Privileged != nil && *container.SecurityContext.Privileged
errs = errs.Also(validateVolumeMounts(ctx, container.VolumeMounts, volumes, isPrivilegedContainer).ViaField("volumeMounts"))

return errs
}
Expand Down Expand Up @@ -659,15 +660,16 @@
return errs
}

func validateVolumeMounts(mounts []corev1.VolumeMount, volumes map[string]corev1.Volume) *apis.FieldError {
func validateVolumeMounts(ctx context.Context, mounts []corev1.VolumeMount, volumes map[string]corev1.Volume, isPrivilegedContainer bool) *apis.FieldError {

Check failure on line 663 in pkg/apis/serving/k8s_validation.go

View workflow job for this annotation

GitHub Actions / style / Golang / Lint

`validateVolumeMounts` - `isPrivilegedContainer` is unused (unparam)
var errs *apis.FieldError
// Check that volume mounts match names in "volumes", that "volumes" has 100%
// coverage, and the field restrictions.
features := config.FromContextOrDefaults(ctx).Features
seenName := make(sets.Set[string], len(mounts))
seenMountPath := make(sets.Set[string], len(mounts))
for i := range mounts {
vm := mounts[i]
errs = errs.Also(apis.CheckDisallowedFields(vm, *VolumeMountMask(&vm)).ViaIndex(i))
errs = errs.Also(apis.CheckDisallowedFields(vm, *VolumeMountMask(ctx, &vm)).ViaIndex(i))
// This effectively checks that Name is non-empty because Volume name must be non-empty.
if _, ok := volumes[vm.Name]; !ok {
errs = errs.Also((&apis.FieldError{
Expand Down Expand Up @@ -699,6 +701,19 @@
Paths: []string{"readOnly"},
}).ViaIndex(i))
}
if vm.MountPropagation != nil {
if features.PodSpecVolumeMountPropagation != config.Enabled {
errs = errs.Also((&apis.FieldError{
Message: fmt.Sprintf("Volume Mount Propagation support is disabled, but found volume mount %s with mount propagation", vm.Name),
}).ViaIndex(i))
}
if *vm.MountPropagation != corev1.MountPropagationNone && *vm.MountPropagation != corev1.MountPropagationHostToContainer {
errs = errs.Also((&apis.FieldError{
Message: "mount propagation should be set to None or HostToContainer",
Paths: []string{"mountPropagation"},
}).ViaIndex(i))
}
}

if volumes[vm.Name].PersistentVolumeClaim != nil {
if volumes[vm.Name].PersistentVolumeClaim.ReadOnly && !vm.ReadOnly {
Expand Down
Loading
Loading