add support for downwardAPI in projected volumes

[KauzClay]

Fixes #4190

Proposed Changes

• Adds complete support for projectedVolumes
• Old implementation did not support downwardAPI

Release Note

Adds support for downwardAPI sources in projected volumes on Knative Services

[codecov]

Codecov Report

Patch coverage: 80.85% and project coverage change: -0.01 ⚠️

Comparison is base (0c8f091) 86.22% compared to head (ebf8182) 86.21%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13896      +/-   ##
==========================================
- Coverage   86.22%   86.21%   -0.01%     
==========================================
  Files         199      199              
  Lines       14706    14749      +43     
==========================================
+ Hits        12680    12716      +36     
- Misses       1727     1731       +4     
- Partials      299      302       +3     

Impacted Files	Coverage Δ
pkg/apis/serving/fieldmask.go	94.54% <75.00%> (-0.38%)	⬇️
pkg/apis/serving/k8s_validation.go	94.33% <86.95%> (-0.25%)	⬇️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[evankanderson]

I think we want to guard the pod metadata like nodeName behind the same flag that's used for environment variables.

[evankanderson]

@dprotaso for a call on this; I think this was an oversight and we already allow many of these fields through.

Do we want to limit the pod metadata fields?

[dprotaso]

yeah let's limit the fields to just metadata.*

I agree with my past self here - #4190 (comment)

[evankanderson]

I think we established that downwardAPI currently only exposes metadata.*.

Now I'm wondering if we should be defensive here...

[dprotaso]

This fixes #4190

[KauzClay]

@evankanderson

I think we want to guard the pod metadata like nodeName behind the same flag that's used for environment variables.

If we do make this limited via the PodSpecFieldRef flag, do you imagine the flag would block of all use of DownwardAPI? (this is what my commit wip: gate downwardAPI support on PodSpecFieldRef feature flag does)

Or just entries that specify FieldRef would get blocked, while entries that use ResourceFieldRef would be allowed?

[evankanderson]

@evankanderson

I think we want to guard the pod metadata like nodeName behind the same flag that's used for environment variables.

If we do make this limited via the PodSpecFieldRef flag, do you imagine the flag would block of all use of DownwardAPI? (this is what my commit wip: gate downwardAPI support on PodSpecFieldRef feature flag does)

Or just entries that specify FieldRef would get blocked, while entries that use ResourceFieldRef would be allowed?

Looking at https://kubernetes.io/docs/concepts/workloads/pods/downward-api/#available-fields and #4190, I'm less concerned about exposing all the fields without a flag. My particular fieldRef fields of concern were:

• spec.nodeName - this leaks server-ful information that might not make sense in a "virtual node" or "PodVM" type environment where.
• status.hostIP - the same as nodeName, in a slightly different format.
• status.podIP - while not quite as critical, leaking the podIP encourages non-serverless patterns like attempting to address individual instances (e.g. to run consensus protocols)

It looks like those fields only get exposed via environment variables, so I'd be okay without needing a flag-guard. I'd like to hear from @dprotaso as well.

The other fields mentioned in #4190 are:

• metadata.name - this exposes the Pod name, which could theoretically be a problem for non-pod-based runtimes, but I think this could be faked out as a unique name if Pods aren't exposed in the Knative API.
• metadata.uid - again, this theoretically ties to the Pod lifecycle, but could probably be emulated easily.
• metadata.{labels,annotations} - these seem generally useful assuming that they basically correspond the the labels/annotations on the Revision.

[KauzClay]

@evankanderson okay, thanks for explaining more, I think I'm understanding now.

For now, I removed my wip commit to wholesale enable/disable DownwardAPI via a flag.

As far as faking the metadata.name/uid, if that is decided, would that be a separate PR?

[evankanderson]

As far as faking the metadata.name/uid, if that is decided, would that be a separate PR?

That was more of a note for compatibility between this (OSS) implementation and other implementations which may not be Kubernetes-based.

/approve

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, KauzClay

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:

• OWNERS [evankanderson]
• pkg/apis/OWNERS [evankanderson]

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

[dprotaso]

Thanks @KauzClay

metadata.name - this exposes the Pod name, which could theoretically be a problem for non-pod-based runtimes, but I think this could be faked out as a unique name if Pods aren't exposed in the Knative API.

This in theory should be the same as invoking hostname in a container

metadata.uid - again, this theoretically ties to the Pod lifecycle, but could probably be emulated easily.

In my view this is benign

metadata.{labels,annotations} - these seem generally useful assuming that they basically correspond the the labels/annotations on the Revision.

+1

We could gate the other properties that aren't supported through volumes but it's not worth being so defensive and we can do that latter if the upstream changes.

/lgtm