Add support for Kubernetes “image” volume type #15878
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15878 +/- ##
==========================================
+ Coverage 80.91% 80.93% +0.01%
==========================================
Files 210 210
Lines 16731 16769 +38
==========================================
+ Hits 13538 13572 +34
- Misses 2842 2843 +1
- Partials 351 354 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
891eb23 to
10844df
Compare
| @@ -77,6 +77,7 @@ const ( | |||
| FeaturePodSpecShareProcessNamespace = "kubernetes.podspec-shareprocessnamespace" | |||
| FeaturePodSpecTolerations = "kubernetes.podspec-tolerations" | |||
| FeaturePodSpecTopologySpreadConstraints = "kubernetes.podspec-topologyspreadconstraints" | |||
| FeaturePodSpecVolumesImage = "kubernetes.podspec-volumes-image" | |||
There was a problem hiding this comment.
@Fedosin a little nit, there's should be at least a enable/disable feature flag test added into features_test.go.
There was a problem hiding this comment.
Thanks! I added the test there.
There was a problem hiding this comment.
Missing entry and and short explanation in default ConfigMap: https://github.com/knative/serving/blob/main/config/core/configmaps/features.yaml
Otherwise looks good, thanks a lot @Fedosin!
|
|
||
| switch iv.PullPolicy { | ||
| case corev1.PullIfNotPresent, corev1.PullAlways, corev1.PullNever, "": | ||
| // ok |
There was a problem hiding this comment.
I think we should be consistent with what K8s has wrt policy validation e.g. "" case, 🤔
There was a problem hiding this comment.
Hey! I think the function you provided is for internal Kubernetes validation. From the user's perspective, pullPolicy is an optional parameter, and an empty string ("") is a valid value.
Here's how I tested it:
➭ cat <<'EOF' | kubectl apply -f -
apiVersion: v1
kind: Pod
metadata:
name: image-volume
spec:
containers:
- name: shell
image: debian
command: ["sleep", "infinity"]
volumeMounts:
- name: volume
mountPath: /volume
volumes:
- name: volume
image:
reference: quay.io/crio/artifact:v2
pullPolicy: ""
EOF
pod/image-volume created
➭ kubectl get pod image-volume -o jsonpath='{.spec.volumes[?(@.name=="volume")].image.pullPolicy}'
IfNotPresentI took the code from here and changed pullPolicy to "".
From the field description it also appears that the
value can be "", because it "defaults to Always if :latest tag is specified, or IfNotPresent otherwise."
713f6be to
5a556bf
Compare
PTAL, the above config map should be updated. |
5a556bf to
853e8c3
Compare
Kubernetes v1.31 introduces image volumes that allow mounting OCI images/artefacts directly into Pods. This patch extends Knative Serving to recognise and validate this new type. Highlights • Feature gate – New config-map key kubernetes.podspec-volumes-image and flag PodSpecVolumesImage (disabled by default). • API / field-mask updates – VolumeSourceMask now preserves .image when the gate is enabled. – Added ImageVolumeSourceMask helper for shallow-copying allowed fields. • Validation – validateVolume checks feature-gate state, treats image as a mutually-exclusive source and performs per-field checks via validateImageVolumeSource. • Schema-tweak overrides – Whitelisted volumes.image behind the new feature gate so generated CRD/OpenAPI stays in sync. • Tests – Added helper to enable the feature in tests. – Expanded TestVolumeValidation with positive/negative cases for the image volume. All unit tests (go test ./...) pass.
Generated by ./hack/update-codegen.sh
853e8c3 to
885c72d
Compare
|
/lgtm Deferring to Dave for approval, PTAL. |
|
So we usually have feature flags to gate end users from using PodSpec features that may affect scalability etc. Hence why we've guarded persistent volumes before since at a certain scale it's not clear how many readers/writers say an EFS volume can support. I think for image volumes it doesn't suffer from this problem since it's just pulling down an OCI image. Thus I'm inclined to just support this feature without needing a feature flag. |
|
Regarding scalability there might be some concerns not sure to what extent this is battle tested: WDYT? |
|
@skonto Yeah, bugs and other issues are the reason why the "image" volume type is not enabled by default in Kubernetes. But I believe they will fix the issues and make it stable in the near future. |
I think this applies to containers images which users can already abuse - so I'm not concerned about volumes. |
|
If it's alpha I'm ok putting this behind a flag. @Fedosin can we create a follow up issue to drop the flag when the feature GAs and we catch up to that release of K8s? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, Fedosin 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:
Approvers can indicate their approval by writing |
Fixes #15488
Proposed Changes
Kubernetes v1.31 introduced image volumes that allow mounting OCI images directly into Pods.
This patch extends Knative Serving to recognize and validate this new type.
Highlights
• Feature gate
– New config-map key kubernetes.podspec-volumes-image and flag PodSpecVolumesImage (disabled by default).
• API / field-mask updates
– VolumeSourceMask now preserves .image when the gate is enabled.
– Added ImageVolumeSourceMask helper for shallow-copying allowed fields.
• Validation
– validateVolume checks feature-gate state, treats image as a mutually-exclusive source and performs per-field checks via validateImageVolumeSource.
• Schema-tweak overrides
– Whitelisted volumes.image behind the new feature gate so generated CRD/OpenAPI stays in sync.
• Tests
– Added helper to enable the feature in tests.
– Expanded TestVolumeValidation with positive/negative cases for the image volume.
Release Note