The committers listed above are authorized under a signed CLA.

• ✅ login: elijah-rou / name: Elijah Roussos (c1238e6, a25fa18)
• ✅ login: dprotaso / name: Dave Protasowski (fcc25f9, a8c80ef, 0d2f30c)

Welcome @elijah-rou! It looks like this is your first PR to knative/serving 🎉

Hi @elijah-rou. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/ok-to-test

/lgtm
/approve

thanks for the change!

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, elijah-rou

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

The pull request process is described here

please take a look at the unit test failure

In pkg/reconciler/route/resources/service_test.go pls add the corresponding flag.

As in #15669 pls add the right validation for this new flag. Also pls you will need to create some docs in knative/docs repo.

/hold

Will fix everything today :) docs PR will probably only come tomorrow

As in #15669 pls add the right validation for this new flag. Also pls you will need to create some docs in knative/docs repo.

Mostly done. Just one comment with this. What validation needs to be done? I don't believe there are specific restrictions using mountPropagation beyond specifying an appropriate mode (None, HostToContainer, Bidirectional), validation which should be taken care of by K8s

What validation needs to be done?

Fail when user uses the feature but the flag is disabled, for example for the hostPath we have:

	if volume.HostPath != nil && features.PodSpecVolumesHostPath != config.Enabled {
		errs = errs.Also(&apis.FieldError{Message: fmt.Sprintf("HostPath volume support is disabled, "+
			"but found HostPath volume %s", volume.Name)})
	}

There is more to it in the link.

I don't believe there are specific restrictions using mountPropagation beyond specifying an appropriate mode (None, HostToContainer, Bidirectional), validation which should be taken care of by K8s

There are actually see here, especially if we want to fail fast:

	if *mountPropagation == core.MountPropagationBidirectional && !privileged {
		allErrs = append(allErrs, field.Forbidden(fldPath, "Bidirectional mount propagation is available only to privileged containers"))
	}

...

if we add support for some other  flags there are more restrictions:

	case core.RecursiveReadOnlyEnabled, core.RecursiveReadOnlyIfPossible:
....
		if mount.MountPropagation != nil && *mount.MountPropagation != core.MountPropagationNone {
			allErrs = append(allErrs, field.Forbidden(fldPath, "may only be specified when mountPropagation is None or not specified"))
		}

I don't believe there are specific restrictions using mountPropagation beyond specifying an appropriate mode (None, HostToContainer, Bidirectional), validation which should be taken care of by K8s

There are actually see here, especially if we want to fail fast:

	if *mountPropagation == core.MountPropagationBidirectional && !privileged {
		allErrs = append(allErrs, field.Forbidden(fldPath, "Bidirectional mount propagation is available only to privileged containers"))
	}

...

if we add support for some other  flags there are more restrictions:

	case core.RecursiveReadOnlyEnabled, core.RecursiveReadOnlyIfPossible:
....
		if mount.MountPropagation != nil && *mount.MountPropagation != core.MountPropagationNone {
			allErrs = append(allErrs, field.Forbidden(fldPath, "may only be specified when mountPropagation is None or not specified"))
		}

Sorry, didn't realise about the privileged field. Will do it.

What validation needs to be done?

Fail when user uses the feature but the flag is disabled, for example for the hostPath we have:

	if volume.HostPath != nil && features.PodSpecVolumesHostPath != config.Enabled {
		errs = errs.Also(&apis.FieldError{Message: fmt.Sprintf("HostPath volume support is disabled, "+
			"but found HostPath volume %s", volume.Name)})
	}

There is more to it in the link.

Sure I'll get this done as well.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.91%. Comparing base (ae7b265) to head (fcc25f9).
Report is 2 commits behind head on main.

@@            Coverage Diff             @@
##             main   #15758      +/-   ##
==========================================
- Coverage   80.94%   80.91%   -0.03%     
==========================================
  Files         210      210              
  Lines       16721    16739      +18     
==========================================
+ Hits        13535    13545      +10     
- Misses       2837     2842       +5     
- Partials      349      352       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

closing and re-opening so we pick up changes in our linter config

There's a compile error

/override "codecov"

it's fixed in main

@dprotaso: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

• codecov

Only the following failed contexts/checkruns were expected:

• EasyCLA
• License Compliance
• build-tests_serving_main
• codecov/project
• istio-latest-no-mesh-tls_serving_main
• istio-latest-no-mesh_serving_main
• style / suggester / github_actions
• style / suggester / shell
• tide
• unit-tests_serving_main
• upgrade-tests_serving_main

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

/override "codecov/project"

@dprotaso: Overrode contexts on behalf of dprotaso: codecov/project

I tweak the flag name to kubernetes.podspec-volumes-mount-propagation

/hold cancel
/lgtm