[Backport] [Release 1.1.2] [SRVKS-881] pvc support (#1069)

* Add pvc support (knative#12458) * add pvc support * fix header * fix fmt * simplify implementation * fixes * fix msg * improve test names * nits * more fixes * Add pvc e2e test (knative#12547) * add pvc e2e test * fmt * fix comment * fix pvc ns * add fsGroup * add flag * fix typo * fixes * fix style * skip pvc setup in upgrade tests * Rename test image to volumes from emptydir (#1052) * fix volumes path & dockerfile * run pvc test * enable alpha Co-authored-by: Kenjiro Nakayama <nakayamakenjiro@gmail.com>
openshift · Mar 15, 2022 · 1a52e71 · 1a52e71
1 parent aa81214
commit 1a52e71
Show file tree

Hide file tree

Showing 24 changed files with 540 additions and 36 deletions.
diff --git a/config/core/configmaps/features.yaml b/config/core/configmaps/features.yaml
@@ -23,7 +23,7 @@ metadata:
     app.kubernetes.io/version: devel
     serving.knative.dev/release: devel
   annotations:
-    knative.dev/example-checksum: "2897f625"
+    knative.dev/example-checksum: "d9e300ba"
 data:
   _example: |-
     ################################
@@ -149,3 +149,13 @@ data:
     # 1. Enabled: enabling init containers support
     # 2. Disabled: disabling init containers support
     kubernetes.podspec-init-containers: "disabled"
+
+    # Controls whether persistent volume claim support is enabled or not.
+    # 1. Enabled: enabling persistent volume claim support
+    # 2. Disabled: disabling persistent volume claim support
+    kubernetes.podspec-persistent-volume-claim: "disabled"
+
+    # Controls whether write access for persistent volumes is enabled or not.
+    # 1. Enabled: enabling write access for persistent volumes
+    # 2. Disabled: disabling write access for persistent volumes
+    kubernetes.podspec-persistent-volume-write: "disabled"
diff --git a/...r/knative-test-images/emptydir/Dockerfile → ...or/knative-test-images/volumes/Dockerfile b/...r/knative-test-images/emptydir/Dockerfile → ...or/knative-test-images/volumes/Dockerfile
@@ -2,5 +2,5 @@
 FROM openshift/origin-base
 USER 65532
 
-ADD emptydir /ko-app/emptydir
-ENTRYPOINT ["/ko-app/emptydir"]
+ADD volumes /ko-app/volumes
+ENTRYPOINT ["/ko-app/volumes"]
diff --git a/openshift/e2e-common.sh b/openshift/e2e-common.sh
@@ -15,7 +15,7 @@ readonly OLM_NAMESPACE="openshift-marketplace"
 if [ -n "$OPENSHIFT_BUILD_NAMESPACE" ]; then
   TEST_IMAGE_TEMPLATE=$(cat <<-END
 {{- with .Name }}
-{{- if eq . "emptydir"}}$KNATIVE_SERVING_TEST_EMPTYDIR{{end -}}
+{{- if eq . "volumes"}}$KNATIVE_SERVING_TEST_VOLUMES{{end -}}
 {{- if eq . "readiness"}}$KNATIVE_SERVING_TEST_READINESS{{end -}}
 {{- if eq . "pizzaplanetv1"}}$KNATIVE_SERVING_TEST_PIZZAPLANETV1{{end -}}
 {{- if eq . "pizzaplanetv2"}}$KNATIVE_SERVING_TEST_PIZZAPLANETV2{{end -}}
@@ -377,6 +377,22 @@ function run_e2e_tests(){
     --skip-cleanup-on-fail \
     --resolvabledomain || failed=1
 
+  # Run PVC test
+  oc -n ${SYSTEM_NAMESPACE} patch knativeserving/knative-serving --type=merge --patch='{"spec": {"config": { "features": {"kubernetes.podspec-persistent-volume-claim": "enabled"}}}}' || fail_test
+  oc -n ${SYSTEM_NAMESPACE} patch knativeserving/knative-serving --type=merge --patch='{"spec": {"config": { "features": {"kubernetes.podspec-persistent-volume-write": "enabled"}}}}' || fail_test
+  oc -n ${SYSTEM_NAMESPACE} patch knativeserving/knative-serving --type=merge --patch='{"spec": {"config": { "features": {"kubernetes.podspec-securitycontext": "enabled"}}}}' || fail_test
+  oc apply -f ./test/config/pvc/pvc.yaml || return $?
+  go_test_e2e -timeout=5m ./test/e2e/pvc \
+    --kubeconfig "$KUBECONFIG" \
+    --imagetemplate "$TEST_IMAGE_TEMPLATE" \
+    --enable-alpha \
+    --https \
+    --skip-cleanup-on-fail \
+    --resolvabledomain || failed=1
+  oc -n ${SYSTEM_NAMESPACE} patch knativeserving/knative-serving --type=merge --patch='{"spec": {"config": { "features": {"kubernetes.podspec-persistent-volume-claim": "disabled"}}}}' || fail_test
+  oc -n ${SYSTEM_NAMESPACE} patch knativeserving/knative-serving --type=merge --patch='{"spec": {"config": { "features": {"kubernetes.podspec-persistent-volume-write": "disabled"}}}}' || fail_test
+  oc -n ${SYSTEM_NAMESPACE} patch knativeserving/knative-serving --type=merge --patch='{"spec": {"config": { "features": {"kubernetes.podspec-securitycontext": "disabled"}}}}' || fail_test
+
   # Run the helloworld test with an image pulled into the internal registry.
   local image_to_tag=$KNATIVE_SERVING_TEST_HELLOWORLD
   oc tag -n serving-tests "$image_to_tag" "helloworld:latest" --reference-policy=local

diff --git a/pkg/apis/config/features.go b/pkg/apis/config/features.go
@@ -54,6 +54,8 @@ func defaultFeaturesConfig() *Features {
 		ContainerSpecAddCapabilities: Disabled,
 		PodSpecTolerations:           Disabled,
 		PodSpecVolumesEmptyDir:       Disabled,
+		PodSpecPersistentVolumeClaim: Disabled,
+		PodSpecPersistentVolumeWrite: Disabled,
 		PodSpecInitContainers:        Disabled,
 		TagHeaderBasedRouting:        Disabled,
 		AutoDetectHTTP2:              Disabled,
@@ -79,6 +81,8 @@ func NewFeaturesConfigFromMap(data map[string]string) (*Features, error) {
 		asFlag("kubernetes.podspec-tolerations", &nc.PodSpecTolerations),
 		asFlag("kubernetes.podspec-volumes-emptydir", &nc.PodSpecVolumesEmptyDir),
 		asFlag("kubernetes.podspec-init-containers", &nc.PodSpecInitContainers),
+		asFlag("kubernetes.podspec-persistent-volume-claim", &nc.PodSpecPersistentVolumeClaim),
+		asFlag("kubernetes.podspec-persistent-volume-write", &nc.PodSpecPersistentVolumeWrite),
 		asFlag("tag-header-based-routing", &nc.TagHeaderBasedRouting),
 		asFlag("autodetect-http2", &nc.AutoDetectHTTP2)); err != nil {
 		return nil, err
@@ -107,6 +111,8 @@ type Features struct {
 	PodSpecTolerations           Flag
 	PodSpecVolumesEmptyDir       Flag
 	PodSpecInitContainers        Flag
+	PodSpecPersistentVolumeClaim Flag
+	PodSpecPersistentVolumeWrite Flag
 	TagHeaderBasedRouting        Flag
 	AutoDetectHTTP2              Flag
 }

diff --git a/pkg/apis/config/features_test.go b/pkg/apis/config/features_test.go
@@ -354,6 +354,42 @@ func TestFeaturesConfiguration(t *testing.T) {
 		data: map[string]string{
 			"kubernetes.podspec-volumes-emptydir": "Enabled",
 		},
+	}, {
+		name:    "kubernetes.podspec-persistent-volume-claim Disabled",
+		wantErr: false,
+		wantFeatures: defaultWith(&Features{
+			PodSpecPersistentVolumeClaim: Disabled,
+		}),
+		data: map[string]string{
+			"kubernetes.podspec-persistent-volume-claim": "Disabled",
+		},
+	}, {
+		name:    "kubernetes.podspec-persistent-volume-claim Enabled",
+		wantErr: false,
+		wantFeatures: defaultWith(&Features{
+			PodSpecPersistentVolumeClaim: Enabled,
+		}),
+		data: map[string]string{
+			"kubernetes.podspec-persistent-volume-claim": "Enabled",
+		},
+	}, {
+		name:    "kubernetes.podspec-persistent-volume-write Disabled",
+		wantErr: false,
+		wantFeatures: defaultWith(&Features{
+			PodSpecPersistentVolumeWrite: Disabled,
+		}),
+		data: map[string]string{
+			"kubernetes.podspec-persistent-volume-write": "Disabled",
+		},
+	}, {
+		name:    "kubernetes.podspec-persistent-volume-claim Enabled",
+		wantErr: false,
+		wantFeatures: defaultWith(&Features{
+			PodSpecPersistentVolumeWrite: Enabled,
+		}),
+		data: map[string]string{
+			"kubernetes.podspec-persistent-volume-write": "Enabled",
+		},
 	}, {
 		name:    "kubernetes.podspec-init-containers Disabled",
 		wantErr: false,

diff --git a/pkg/apis/serving/fieldmask.go b/pkg/apis/serving/fieldmask.go
@@ -45,6 +45,10 @@ func VolumeMask(ctx context.Context, in *corev1.Volume) *corev1.Volume {
 		out.EmptyDir = in.EmptyDir
 	}
 
+	if cfg.Features.PodSpecPersistentVolumeClaim != config.Disabled {
+		out.PersistentVolumeClaim = in.PersistentVolumeClaim
+	}
+
 	return out
 }
 
@@ -67,6 +71,10 @@ func VolumeSourceMask(ctx context.Context, in *corev1.VolumeSource) *corev1.Volu
 		out.EmptyDir = in.EmptyDir
 	}
 
+	if cfg.Features.PodSpecPersistentVolumeClaim != config.Disabled {
+		out.PersistentVolumeClaim = in.PersistentVolumeClaim
+	}
+
 	// Too many disallowed fields to list
 
 	return out

diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go
@@ -85,12 +85,7 @@ var (
 func ValidateVolumes(ctx context.Context, vs []corev1.Volume, mountedVolumes sets.String) (map[string]corev1.Volume, *apis.FieldError) {
 	volumes := make(map[string]corev1.Volume, len(vs))
 	var errs *apis.FieldError
-	features := config.FromContextOrDefaults(ctx).Features
 	for i, volume := range vs {
-		if volume.EmptyDir != nil && features.PodSpecVolumesEmptyDir != config.Enabled {
-			errs = errs.Also((&apis.FieldError{Message: fmt.Sprintf("EmptyDir volume support is off, "+
-				"but found EmptyDir volume %s", volume.Name)}).ViaIndex(i))
-		}
 		if _, ok := volumes[volume.Name]; ok {
 			errs = errs.Also((&apis.FieldError{
 				Message: fmt.Sprintf("duplicate volume name %q", volume.Name),
@@ -109,14 +104,36 @@ func ValidateVolumes(ctx context.Context, vs []corev1.Volume, mountedVolumes set
 	return volumes, errs
 }
 
+func validatePersistentVolumeClaims(volume corev1.VolumeSource, features *config.Features) *apis.FieldError {
+	var errs *apis.FieldError
+	if volume.PersistentVolumeClaim == nil {
+		return nil
+	}
+	if features.PodSpecPersistentVolumeClaim != config.Enabled {
+		errs = errs.Also(&apis.FieldError{Message: fmt.Sprintf("Persistent volume claim support is disabled, "+
+			"but found persistent volume claim %s", volume.PersistentVolumeClaim.ClaimName)})
+	}
+	isWriteEnabled := features.PodSpecPersistentVolumeWrite == config.Enabled
+	if !volume.PersistentVolumeClaim.ReadOnly && !isWriteEnabled {
+		errs = errs.Also(&apis.FieldError{Message: fmt.Sprintf("Persistent volume write support is disabled, "+
+			"but found persistent volume claim %s that is not read-only", volume.PersistentVolumeClaim.ClaimName)})
+	}
+	return errs
+}
+
 func validateVolume(ctx context.Context, volume corev1.Volume) *apis.FieldError {
-	errs := apis.CheckDisallowedFields(volume, *VolumeMask(ctx, &volume))
+	features := config.FromContextOrDefaults(ctx).Features
+	errs := validatePersistentVolumeClaims(volume.VolumeSource, features)
+	if volume.EmptyDir != nil && features.PodSpecVolumesEmptyDir != config.Enabled {
+		errs = errs.Also(&apis.FieldError{Message: fmt.Sprintf("EmptyDir volume support is disabled, "+
+			"but found EmptyDir volume %s", volume.Name)})
+	}
+	errs = errs.Also(apis.CheckDisallowedFields(volume, *VolumeMask(ctx, &volume)))
 	if volume.Name == "" {
 		errs = apis.ErrMissingField("name")
 	} else if len(validation.IsDNS1123Label(volume.Name)) != 0 {
 		errs = apis.ErrInvalidValue(volume.Name, "name")
 	}
-
 	vs := volume.VolumeSource
 	errs = errs.Also(apis.CheckDisallowedFields(vs, *VolumeSourceMask(ctx, &vs)))
 	var specified []string
@@ -142,12 +159,20 @@ func validateVolume(ctx context.Context, volume corev1.Volume) *apis.FieldError
 		specified = append(specified, "emptyDir")
 		errs = errs.Also(validateEmptyDirFields(vs.EmptyDir).ViaField("emptyDir"))
 	}
+
+	if vs.PersistentVolumeClaim != nil {
+		specified = append(specified, "persistentVolumeClaim")
+	}
+
 	if len(specified) == 0 {
 		fieldPaths := []string{"secret", "configMap", "projected"}
 		cfg := config.FromContextOrDefaults(ctx)
 		if cfg.Features.PodSpecVolumesEmptyDir == config.Enabled {
 			fieldPaths = append(fieldPaths, "emptyDir")
 		}
+		if cfg.Features.PodSpecPersistentVolumeClaim == config.Enabled {
+			fieldPaths = append(fieldPaths, "persistentVolumeClaim")
+		}
 		errs = errs.Also(apis.ErrMissingOneOf(fieldPaths...))
 	} else if len(specified) > 1 {
 		errs = errs.Also(apis.ErrMultipleOneOf(specified...))
@@ -597,8 +622,21 @@ func validateVolumeMounts(mounts []corev1.VolumeMount, volumes map[string]corev1
 		}
 		seenMountPath.Insert(filepath.Clean(vm.MountPath))
 
-		if volumes[vm.Name].EmptyDir == nil && !vm.ReadOnly {
-			errs = errs.Also(apis.ErrMissingField("readOnly").ViaIndex(i))
+		shouldCheckReadOnlyVolume := volumes[vm.Name].EmptyDir == nil && volumes[vm.Name].PersistentVolumeClaim == nil
+		if shouldCheckReadOnlyVolume && !vm.ReadOnly {
+			errs = errs.Also((&apis.FieldError{
+				Message: "volume mount should be readOnly for this type of volume",
+				Paths:   []string{"readOnly"},
+			}).ViaIndex(i))
+		}
+
+		if volumes[vm.Name].PersistentVolumeClaim != nil {
+			if volumes[vm.Name].PersistentVolumeClaim.ReadOnly && !vm.ReadOnly {
+				errs = errs.Also((&apis.FieldError{
+					Message: "volume is readOnly but volume mount is not",
+					Paths:   []string{"readOnly"},
+				}).ViaIndex(i))
+			}
 		}
 	}
 	return errs