Add support for emptyDir volume type #11669

skonto · 2021-07-13T15:55:10Z

Part of work in #11664

Proposed Changes

Adds the required feature flag kubernetes.podspec-volumes-emptydir, updates the related CRD and apis.
Updates related unit tests
Validates EmptyDir contents. A bit stricter with the negative values compared to what K8s does here.

Tested manually, test cases and results here.

/cc @dprotaso @julz @markusthoemmes

codecov · 2021-07-13T16:00:48Z

Codecov Report

Merging #11669 (6392811) into main (0b09cdd) will decrease coverage by 0.04%.
The diff coverage is 77.41%.

@@            Coverage Diff             @@
##             main   #11669      +/-   ##
==========================================
- Coverage   87.82%   87.78%   -0.05%     
==========================================
  Files         190      190              
  Lines        9249     9275      +26     
==========================================
+ Hits         8123     8142      +19     
- Misses        876      879       +3     
- Partials      250      254       +4

Impacted Files	Coverage Δ
pkg/apis/serving/k8s_validation.go	`96.12% <69.56%> (-1.84%)`	⬇️
pkg/apis/config/features.go	`94.73% <100.00%> (+0.29%)`	⬆️
pkg/apis/serving/fieldmask.go	`95.01% <100.00%> (+0.10%)`	⬆️
pkg/reconciler/configuration/configuration.go	`84.61% <0.00%> (-2.31%)`	⬇️
pkg/reconciler/revision/background.go	`89.58% <0.00%> (-2.09%)`	⬇️
pkg/autoscaler/statforwarder/processor.go	`94.44% <0.00%> (+5.55%)`	⬆️
pkg/autoscaler/statforwarder/forwarder.go	`96.29% <0.00%> (+5.55%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b09cdd...6392811. Read the comment docs.

markusthoemmes · 2021-07-13T16:03:53Z

hack/schemapatch-config.yaml

@@ -9,6 +9,7 @@ k8s.io/api/core/v1.VolumeSource:
    - Secret
    - ConfigMap
    - Projected
+    - EmptyDir


We should add a preserveUnknownFields: true # for feature flagged fields here in accordance with the other types. So far, all of the fields that require a feature flag are not actually in the schema but allowed this way.

Ok yes let me try that, want to check what it means from a validation perspective.

markusthoemmes

a few drive-by comments. Looks good overall.

/assign @dprotaso

markusthoemmes · 2021-07-13T16:04:08Z

pkg/apis/config/features.go

@@ -72,7 +73,8 @@ func NewFeaturesConfigFromMap(data map[string]string) (*Features, error) {
 		asFlag("kubernetes.containerspec-addcapabilities", &nc.ContainerSpecAddCapabilities),
 		asFlag("kubernetes.podspec-tolerations", &nc.PodSpecTolerations),
 		asFlag("tag-header-based-routing", &nc.TagHeaderBasedRouting),
-		asFlag("autodetect-http2", &nc.AutoDetectHTTP2)); err != nil {
+		asFlag("autodetect-http2", &nc.AutoDetectHTTP2),
+		asFlag("kubernetes.podspec-volumes-emptydir", &nc.PodSpecVolumesEmptyDir)); err != nil {


nit: group with the others?

markusthoemmes · 2021-07-13T16:04:14Z

pkg/apis/config/features.go

@@ -97,6 +99,7 @@ type Features struct {
 	PodSpecTolerations           Flag
 	TagHeaderBasedRouting        Flag
 	AutoDetectHTTP2              Flag
+	PodSpecVolumesEmptyDir       Flag


same nit as above.

markusthoemmes · 2021-07-13T16:05:23Z

pkg/apis/serving/k8s_validation.go

+		if volume.EmptyDir != nil {
+			if features.PodSpecVolumesEmptyDir != config.Enabled {


Suggested change

if volume.EmptyDir != nil {

if features.PodSpecVolumesEmptyDir != config.Enabled {

if volume.EmptyDir != nil && features.PodSpecVolumesEmptyDir != config.Enabled {

markusthoemmes · 2021-07-13T16:06:47Z

pkg/apis/serving/fieldmask.go

@@ -57,6 +58,7 @@ func VolumeSourceMask(in *corev1.VolumeSource) *corev1.VolumeSource {
 	out.Secret = in.Secret
 	out.ConfigMap = in.ConfigMap
 	out.Projected = in.Projected
+	out.EmptyDir = in.EmptyDir


Any reason we don't check the feature flag here like we do for example for the SecurtityContext types?

A few lines above before a volumeValidation call is done, we check the features flags and emit an error.
Creating more errors from that point on seemed verbose to me. Otherwise we could create two errors (volumeMask and volumeSourceMask) and avoid the top one or have them all anyway.

julz · 2021-07-13T15:58:11Z

config/core/configmaps/features.yaml

@@ -131,3 +131,8 @@ data:
    # 1. Enabled: http2 connection will be attempted via upgrade.
    # 2. Disabled: http2 connection will only be attempted when port name is set to "h2c".
    autodetect-http2: "disabled"
+
+    # Controls whether volume supporty for EmptyDir is enabled or not.


Suggested change

# Controls whether volume supporty for EmptyDir is enabled or not.

# Controls whether volume support for EmptyDir is enabled or not.

julz · 2021-07-13T16:06:52Z

pkg/apis/serving/k8s_validation.go

 	for i, volume := range vs {
+		// skip validating empty dir volumes if the feature is not enabled


Im not sure I understand this comment; we skip validating if the feature (that is, allowing this type of volume) is enabled. (If the feature is disabled then we DO validate that the volume is not there). Is the comment supposed to be further down maybe, above the validateEmptyDirFields line?

Yeah this needs to be removed initially I had a different logic here.

julz · 2021-07-13T16:07:42Z

pkg/apis/serving/k8s_validation.go

 	if len(specified) == 0 {
-		errs = errs.Also(apis.ErrMissingOneOf("secret", "configMap", "projected"))
+		errs = errs.Also(apis.ErrMissingOneOf("secret", "configMap", "projected", "emptyDir"))


presumably "emptydir" should only be in this list if the flag is enabled?

dprotaso · 2021-07-14T13:20:32Z

A question for everyone (@julz, @skonto, @markusthoemmes) - what do people thing about allowing emptyDir without then need for a feature flag?

markusthoemmes · 2021-07-14T13:23:07Z

Personally, I'm game, but what are the ramifications of that? Do we need to change specs?

julz · 2021-07-14T13:41:05Z

interesting! I'm pretty ok with it, on balance, from a security perspective: it's technically more surface area, but not dramatically (although as it happens there was a recent upstream cve that not supporting arbitrary volumes meant we weren't exposed to).

Thinking out loud it does seems possible though to imagine a more opinionated administrator wanting to turn this off it in order to prevent patterns that might be difficult to support (like downloading models on startup), and if we add it without a feature flag then as @markusthoemmes says arguably we should probably add it to the spec, and it may not be a feature all implementations could support so.. I'd probably vote for keeping it flagged, though not very strongly and I'm super ok going another way if y'all disagree.

(I do wish we could have a more generic way to exclude fields so we could get out of the PodSpec validation business; maybe the openapi stuff eventually?..)

dprotaso · 2021-07-14T13:48:08Z

Do we need to change specs?

So the specs are more about conformance - and technically the OSS implementation can be a superset of that.

ie. We surface replica counts in the revision status though it's not in the spec at this moment. When we bring that to the KTC it could be veto'd by a member. Similarly this is the same case with multiple containers.

dprotaso · 2021-07-14T13:50:04Z

My motivation for emptyDir not being behind a feature flag is because I think it'll help support multi-container use cases - ie. IPC via a socket or a named pipe

dprotaso · 2021-07-14T13:52:00Z

I'm ok being conservative and having the flag to introduce this feature. ie. I'm not sure there are resourceLimits on emptyDir volumes etc.

But long term I think it's beneficial to open it up

dprotaso · 2021-07-14T13:53:39Z

oh interesting you can have limits and requests on ephemeral storage

https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#setting-requests-and-limits-for-local-ephemeral-storage

julz · 2021-07-14T13:54:39Z

oh interesting you can have limits and requests on ephemeral storage

Yah I actually double-checked that before writing the sentence about being ok with it from a security perspective 😇

skonto · 2021-07-15T13:05:40Z

I would vote to keep it flagged but open it up later for sure (if we see no issues). Btw if an emptyDir should not be allowed in an app it could be caught at the deployment level eg. yaml file when people submit code or by setting a podSecurityPolicy that does not allow this volume type (same approach to avoid hostPath mounts in arbitrary pods). However, the flag is an easier way to do so in any case (but not that flexible as it is an on/off approach).

dprotaso · 2021-07-15T18:32:14Z

Sounds good

skonto · 2021-07-16T14:14:54Z

Ready for another review round.

dprotaso · 2021-07-18T01:20:40Z

/lgtm
/approve

knative-prow-robot · 2021-07-18T01:20:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, skonto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~config/OWNERS~~ [dprotaso]
~~hack/OWNERS~~ [dprotaso]
~~pkg/apis/OWNERS~~ [dprotaso]
~~pkg/reconciler/route/OWNERS~~ [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mattmoor · 2021-07-22T15:40:44Z

Awesome to see this. I was just asking in slack about this because I want it for something I'm playing with!

🎉

* support emptyDir volume * fixes * reorder feature

* Add support for emptyDir volume type (knative#11669) * support emptyDir volume * fixes * reorder feature * Add support for emptydir rw access and e2e testing (knative#11705) * add support for emptydir rw access, e2e * fix header * fix quantity * set to alpha feature * toggle feature * fixes * fix fmt * fix nit Co-authored-by: Stavros Kontopoulos <[email protected]>

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 13, 2021

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/API API objects and controllers area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Jul 13, 2021

markusthoemmes reviewed Jul 13, 2021

View reviewed changes

knative-prow-robot assigned dprotaso Jul 13, 2021

julz reviewed Jul 13, 2021

View reviewed changes

skonto added 3 commits July 16, 2021 14:32

support emptyDir volume

38ff3c6

fixes

edc52ed

reorder feature

6392811

skonto force-pushed the add_emptydir_support branch from 943b8c9 to 6392811 Compare July 16, 2021 11:33

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2021

knative-prow-robot merged commit 317cde6 into knative:main Jul 18, 2021

markusthoemmes pushed a commit to markusthoemmes/knative-serving that referenced this pull request Aug 20, 2021

Add support for emptyDir volume type (knative#11669)

0f35245

* support emptyDir volume * fixes * reorder feature

markusthoemmes pushed a commit to markusthoemmes/knative-serving that referenced this pull request Aug 23, 2021

Add support for emptyDir volume type (knative#11669)

3e31166

* support emptyDir volume * fixes * reorder feature

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for emptyDir volume type #11669

Add support for emptyDir volume type #11669

skonto commented Jul 13, 2021 •

edited

Loading

codecov bot commented Jul 13, 2021 •

edited

Loading

markusthoemmes Jul 13, 2021

skonto Jul 15, 2021 •

edited

Loading

markusthoemmes left a comment

markusthoemmes Jul 13, 2021

markusthoemmes Jul 13, 2021

markusthoemmes Jul 13, 2021

markusthoemmes Jul 13, 2021

skonto Jul 15, 2021 •

edited

Loading

julz Jul 13, 2021

julz Jul 13, 2021

skonto Jul 15, 2021

julz Jul 13, 2021

skonto Jul 15, 2021

dprotaso commented Jul 14, 2021

markusthoemmes commented Jul 14, 2021

julz commented Jul 14, 2021

dprotaso commented Jul 14, 2021 •

edited

Loading

dprotaso commented Jul 14, 2021

dprotaso commented Jul 14, 2021

dprotaso commented Jul 14, 2021

julz commented Jul 14, 2021

skonto commented Jul 15, 2021 •

edited

Loading

dprotaso commented Jul 15, 2021

skonto commented Jul 16, 2021

dprotaso commented Jul 18, 2021

knative-prow-robot commented Jul 18, 2021

mattmoor commented Jul 22, 2021

		if volume.EmptyDir != nil {
		if features.PodSpecVolumesEmptyDir != config.Enabled {

	if volume.EmptyDir != nil {
	if features.PodSpecVolumesEmptyDir != config.Enabled {
	if volume.EmptyDir != nil && features.PodSpecVolumesEmptyDir != config.Enabled {

	# Controls whether volume supporty for EmptyDir is enabled or not.
	# Controls whether volume support for EmptyDir is enabled or not.

		for i, volume := range vs {
		// skip validating empty dir volumes if the feature is not enabled

Add support for emptyDir volume type #11669

Add support for emptyDir volume type #11669

Conversation

skonto commented Jul 13, 2021 • edited Loading

Proposed Changes

codecov bot commented Jul 13, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

skonto Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

markusthoemmes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprotaso commented Jul 14, 2021

markusthoemmes commented Jul 14, 2021

julz commented Jul 14, 2021

dprotaso commented Jul 14, 2021 • edited Loading

dprotaso commented Jul 14, 2021

dprotaso commented Jul 14, 2021

dprotaso commented Jul 14, 2021

julz commented Jul 14, 2021

skonto commented Jul 15, 2021 • edited Loading

dprotaso commented Jul 15, 2021

skonto commented Jul 16, 2021

dprotaso commented Jul 18, 2021

knative-prow-robot commented Jul 18, 2021

mattmoor commented Jul 22, 2021

skonto commented Jul 13, 2021 •

edited

Loading

codecov bot commented Jul 13, 2021 •

edited

Loading

skonto Jul 15, 2021 •

edited

Loading

skonto Jul 15, 2021 •

edited

Loading

dprotaso commented Jul 14, 2021 •

edited

Loading

skonto commented Jul 15, 2021 •

edited

Loading