KEP-5040: Remove gitRepo volume driver. #5039

vinayakankugoyal · 2025-01-14T01:46:17Z

One-line PR description: Initial KEP to remove support for gitRepo volume driver.

Issue link: Remove gitRepo volume driver #5040

Other comments:

xing-yang · 2025-01-14T03:24:05Z

Can you open an enhancement issue? Are you planning to target Alpha in 1.33?

vinayakankugoyal · 2025-01-14T18:06:07Z

Can you open an enhancement issue? Are you planning to target Alpha in 1.33?

#5040. Yeah we are planning to target Alpha in 1.33

vinayakankugoyal · 2025-01-14T18:20:50Z

/ok-to-test

vinayakankugoyal · 2025-01-14T18:34:22Z

/retest

keps/sig-storage/5040-remove-gitrepo-driver/README.md

keps/sig-storage/5040-remove-gitrepo-driver/kep.yaml

keps/sig-storage/5040-remove-gitrepo-driver/README.md

thockin

I am LGTM on technicals - need th testing and release and PRR sections.

keps/sig-storage/5040-remove-gitrepo-driver/README.md

thockin · 2025-01-16T23:19:06Z

/lgtm
/approve

thockin · 2025-01-16T23:20:27Z

sig-storage officially owns this, but to save you effort, I will apply the leads label -- stop me if you disagree!

saad-ali · 2025-01-17T00:56:39Z

Thanks Tim and Vinayak.

We are supportive of this plan from the SIG Storage side of things.

/lgtm
/approve

vinayakankugoyal · 2025-01-17T01:00:17Z

/assign @jpbetz
For PRR.

sftim · 2025-01-17T14:18:28Z

keps/sig-storage/5040-remove-gitrepo-driver/README.md

+We propose removing support for the in-tree gitRepo volume driver.
+
+We acknowledge that this is highly unusual, but there are mitigating circumstances:
+* gitRepo volume types can be exploited to gain remote code execution as root on the nodes as shown in [CVE-2024-10220](https://nvd.nist.gov/vuln/detail/cve-2024-10220)
+* gitRepo has perfectly workable alternatives:
+  * Using [git-sync](https://github.com/kubernetes/git-sync)
+  * Using an initContainer. See https://kubernetes.io/docs/concepts/storage/volumes/#gitrepo for more details
+* gitRepo has been deprecated for a long time and is unmaintained
+* gitRepo has low usage (based on limited data)
+* there exists precedent for removing volume drivers which were unmaintained like [glusterfs](https://kubernetes.io/docs/concepts/storage/volumes/#glusterfs)
+
+Given this, we think kubernetes should EOL this feature with urgency.


💭 Who's “we“? KEPs are usually written in the persona of the Kubernetes project, or maybe of a lead SIG.

We here and in other locations is meant to represent the author(s). Would you like me to say the following instead?

"Give this, Kubernetes thinks it should EOL this feature with urgency."

keps/sig-storage/5040-remove-gitrepo-driver/README.md

sftim · 2025-01-17T14:22:15Z

keps/sig-storage/5040-remove-gitrepo-driver/README.md

+pod with an appropriate error message guiding users to either use an alternative
+or setting the feature-gate `GitRepoVolumeDriver` to `true`.
+
+### Timeline:


Suggested change

### Timeline:

### Timeline

This is the delivery plan, not a design detail.

This is the delivery plan, not a design detail.

I am not seeing a concrete suggestion here. Do you prefer this section be dropped or moved out of Design Details?

keps/sig-storage/5040-remove-gitrepo-driver/README.md

keps/sig-storage/5040-remove-gitrepo-driver/kep.yaml

sftim · 2025-01-17T14:24:26Z

keps/sig-storage/5040-remove-gitrepo-driver/kep.yaml

+replaces:
+
+# The target maturity stage in the current dev cycle for this KEP.
+stage: alpha


Check that this is right. You might mean “deprecated”.

I wasn't sure what values this can take.

But based on your feedback about using “deprecated” in the graduation criteria maybe it makes sense to change this to "removed with opt-out"?

We had a discussion on adding Deprecated but that PR didn't merge, let me go look for it.

Turns out it was waiting for me. Oops. #4898

Since we are jumping to off-by-default, I think the Stage you want is "disabled"

Gotcha, Ill wait for #4898 to be merged before updating this.

AFAICT nothing is validating this, so don't block on it :)

😭

"map[/home/prow/go/src/github.com/kubernetes/enhancements/keps/sig-storage/5040-remove-gitrepo-driver/kep.yaml:[getting PRR approver for diabled stage: invalid stage: diabled, should be one of [alpha beta stable]]]"

Opened #5049 to fix validation.

keps/sig-storage/5040-remove-gitrepo-driver/README.md

sftim · 2025-01-17T14:25:57Z

@saad-ali I support this change, but the KEP document is not not ready to merge as implementable. It needs work.

k8s-ci-robot · 2025-01-17T18:23:11Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2025-01-17T18:23:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: saad-ali, thockin, vinayakankugoyal
Once this PR has been reviewed and has the lgtm label, please ask for approval from jpbetz. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

keps/prod-readiness/OWNERS
~~keps/sig-storage/OWNERS~~ [saad-ali]

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

BenTheElder

[took a first pass as PRR Shadow]

Independently: personal +1 for this effort overall, thank you.

BenTheElder · 2025-01-17T21:33:16Z

keps/sig-storage/5040-remove-gitrepo-driver/README.md

+
+This removal is similar to removal of support for
+[glusterfs](https://kubernetes.io/docs/concepts/storage/volumes/#glusterfs)
+which was deprecated in v1.25 and removed in v1.26.


Was there a KEP for this? Did we offer any migration like CSI migration?

I don't think there was a KEP. I searched for the "glusterfs" under /keps/ and got only one hit but that KEP had nothing to do with removal.

keps/sig-storage/5040-remove-gitrepo-driver/README.md

BenTheElder · 2025-01-17T21:38:49Z

keps/sig-storage/5040-remove-gitrepo-driver/README.md

+**For non-optional features moving to GA, the graduation criteria must include
+[conformance tests].**
+
+[conformance tests]: https://git.k8s.io/community/contributors/devel/sig-architecture/conformance-tests.md


Noting: We do not have a conformance test for this volume type: https://github.com/kubernetes/kubernetes/blob/master/test/conformance/testdata/conformance.yaml

node_e2e tests are not conformance tests, and I think for this "feature" it's reasonable to not have one.

Hmm.. I see that the following test in node_e2e saying NodeConformance.

https://github.com/kubernetes/kubernetes/blob/b66142831a98278a21105101454edcce3ee38c42/test/e2e_node/apparmor_test.go#L50

BenTheElder · 2025-01-17T21:42:42Z

keps/sig-storage/5040-remove-gitrepo-driver/README.md

+<!--
+What signals should users be paying attention to when the feature is young
+that might indicate a serious problem?
+-->


Presumably the user could monitor for pods being rejected?
(If there isn't a suitable signal, there's a section for that below)

We aren't removing the gitRepo volume from the API so pods won't be rejected by kube-apiserver, kubelet will fail to start them. Maybe the recommendation should be around Pods that are failing to start?

keps/sig-storage/5040-remove-gitrepo-driver/README.md

thockin · 2025-01-18T04:32:55Z

keps/sig-storage/5040-remove-gitrepo-driver/README.md

+using the following Validating Admission Policy (VAP).
+
+```yaml
+apiVersion: admissionregistration.k8s.io/v1


The VAP I have locally is a little different, perhaps @jpbetz or @cici37 can clarify which is more idiomatic or preferred? I think you started from an earlier draft of mine, because I had forgotten ReplicationController, too.

apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingAdmissionPolicy metadata: name: "deny-git-repo-volumes" spec: failurePolicy: Fail matchConstraints: resourceRules: - apiGroups: [""] apiVersions: ["v1"] operations: ["CREATE"] resources: ["pods"] - apiGroups: [""] apiVersions: ["v1"] operations: ["CREATE", "UPDATE"] resources: ["podtemplates", "replicationcontrollers"] - apiGroups: ["apps"] apiVersions: ["v1"] operations: ["CREATE", "UPDATE"] resources: ["daemonsets", "deployments", "replicasets", "statefulsets"] - apiGroups: ["batch"] apiVersions: ["v1"] operations: ["CREATE", "UPDATE"] resources: ["jobs", "cronjobs"] validations: - expression: | (object.kind == "Pod" && object.spec.?volumes.orValue([]).all(v, !has(v.gitRepo))) || (object.kind == "PodTemplate" && object.template.spec.?volumes.orValue([]).all(v, !has(v.gitRepo))) || (["DaemonSet","Deployment","ReplicaSet",'StatefulSet','Job','ReplicationController'].filter(kind, object.kind == kind) != [] && object.spec.template.spec.?volumes.orValue([]).all(v, !has(v.gitRepo))) || (object.kind == "CronJob" && object.spec.jobTemplate.spec.template.spec.?volumes.orValue([]).all(v, !has(v.gitRepo))) message: "gitRepo volumes are forbidden" --- apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingAdmissionPolicyBinding metadata: name: "deny-git-repo-volumes" spec: policyName: "deny-git-repo-volumes" validationActions: [Deny] # empty matchResources means match all

Your uses multiple expressions, which I am assuming is a logical AND? Mine uses postive polarity, but that results in OR.

Mine includes ReplicationController which yours misses.

The VAP I have locally is a little different, perhaps @jpbetz or @cici37 can clarify which is more idiomatic or preferred? I think you started from an earlier draft of mine, because I had forgotten ReplicationController, too.

I used this as a starting point. I looks like they forgot ReplicationController as well 😅

Your uses multiple expressions, which I am assuming is a logical AND? Mine uses postive polarity, but that results in OR.

I tend to always use multiple expression other wise things become really hard to read and yes these are ANDed. According to the documentation

"If an expression evaluates to false, the validation check is enforced according to the spec.failurePolicy field."

Also verified this with some tests.

thockin · 2025-01-18T04:33:42Z

keps/sig-storage/5040-remove-gitrepo-driver/README.md

+
+### Validating Admission Policy
+
+Users can prevent workloads with gitRepo volumes from being admitted 


Should we discuss creating a git repo to hold VAPs like this?

Should we just put the VAP in the gitRepo volume documentation here?

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 14, 2025

k8s-ci-robot requested review from saad-ali and xing-yang January 14, 2025 01:46

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 14, 2025

vinayakankugoyal marked this pull request as draft January 14, 2025 01:46

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 14, 2025

vinayakankugoyal mentioned this pull request Jan 14, 2025

Remove gitRepo volume driver #5040

Open

4 tasks

vinayakankugoyal force-pushed the gitRepo branch from a7dca39 to faf4ffe Compare January 14, 2025 18:09

vinayakankugoyal changed the title ~~KEP-NNNN: remove gitRepo volume driver.~~ KEP-5040: Remove gitRepo volume driver. Jan 14, 2025

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jan 14, 2025

vinayakankugoyal force-pushed the gitRepo branch from faf4ffe to b39ccb0 Compare January 14, 2025 18:26

thockin self-assigned this Jan 14, 2025

thockin reviewed Jan 14, 2025

View reviewed changes

vinayakankugoyal force-pushed the gitRepo branch from b39ccb0 to 430203c Compare January 15, 2025 01:44

thockin reviewed Jan 15, 2025

View reviewed changes

keps/sig-storage/5040-remove-gitrepo-driver/README.md Show resolved Hide resolved

thockin reviewed Jan 15, 2025

View reviewed changes

keps/sig-storage/5040-remove-gitrepo-driver/README.md Show resolved Hide resolved

thockin reviewed Jan 15, 2025

View reviewed changes

keps/sig-storage/5040-remove-gitrepo-driver/kep.yaml Outdated Show resolved Hide resolved

vinayakankugoyal force-pushed the gitRepo branch from 430203c to 07fa827 Compare January 16, 2025 00:18

vinayakankugoyal marked this pull request as ready for review January 16, 2025 00:32

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 16, 2025

vinayakankugoyal force-pushed the gitRepo branch from 07fa827 to a4b48b0 Compare January 16, 2025 00:36

thockin reviewed Jan 16, 2025

View reviewed changes

keps/sig-storage/5040-remove-gitrepo-driver/README.md Show resolved Hide resolved

thockin reviewed Jan 16, 2025

View reviewed changes

keps/sig-storage/5040-remove-gitrepo-driver/README.md Show resolved Hide resolved

keps/sig-storage/5040-remove-gitrepo-driver/README.md Show resolved Hide resolved

vinayakankugoyal force-pushed the gitRepo branch from a4b48b0 to 34acb88 Compare January 16, 2025 18:10

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 16, 2025

vinayakankugoyal force-pushed the gitRepo branch 2 times, most recently from dbd3cfa to fbbcb5d Compare January 16, 2025 19:17

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 16, 2025

thockin added the lead-opted-in Denotes that an issue has been opted in to a release label Jan 16, 2025

thockin added this to the v1.33 milestone Jan 16, 2025

k8s-ci-robot assigned saad-ali Jan 17, 2025

k8s-ci-robot assigned jpbetz Jan 17, 2025

sftim reviewed Jan 17, 2025

View reviewed changes

vinayakankugoyal force-pushed the gitRepo branch from fbbcb5d to c7efe3c Compare January 17, 2025 18:23

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2025

vinayakankugoyal force-pushed the gitRepo branch 3 times, most recently from 42ef30b to bb72740 Compare January 17, 2025 21:35

BenTheElder reviewed Jan 17, 2025

View reviewed changes

vinayakankugoyal force-pushed the gitRepo branch from bb72740 to 1f51a71 Compare January 17, 2025 23:28

thockin reviewed Jan 18, 2025

View reviewed changes

vinayakankugoyal force-pushed the gitRepo branch 4 times, most recently from 5cef8d7 to 8cb44f9 Compare January 18, 2025 06:18

KEP-5040: remove gitRepo volume driver.

8169ca0

vinayakankugoyal force-pushed the gitRepo branch from 8cb44f9 to 8169ca0 Compare January 18, 2025 16:58


		### Validating Admission Policy

		Users can prevent workloads with gitRepo volumes from being admitted

KEP-5040: Remove gitRepo volume driver. #5039

Are you sure you want to change the base?

KEP-5040: Remove gitRepo volume driver. #5039

Conversation

vinayakankugoyal commented Jan 14, 2025 • edited Loading

xing-yang commented Jan 14, 2025

vinayakankugoyal commented Jan 14, 2025

vinayakankugoyal commented Jan 14, 2025

vinayakankugoyal commented Jan 14, 2025

thockin left a comment

Choose a reason for hiding this comment

thockin commented Jan 16, 2025

thockin commented Jan 16, 2025

saad-ali commented Jan 17, 2025

vinayakankugoyal commented Jan 17, 2025

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

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

Choose a reason for hiding this comment

sftim commented Jan 17, 2025

k8s-ci-robot commented Jan 17, 2025

k8s-ci-robot commented Jan 17, 2025

BenTheElder 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

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

vinayakankugoyal commented Jan 14, 2025 •

edited

Loading