KEP-5040: Remove gitRepo volume driver.

[vinayakankugoyal]

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

• Issue link: Remove gitRepo volume driver #5040

• Other comments:

[xing-yang]

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

[vinayakankugoyal]

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]

/ok-to-test

[vinayakankugoyal]

/retest

[thockin]

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

[BenTheElder]

[took a first pass as PRR Shadow]

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

[BenTheElder]

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.

[vinayakankugoyal]

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

[thockin]

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

[vinayakankugoyal]

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

[sftim]

@thockin I think we should, and we should try to make that allow for a future, more rich way to manage add ons.

I know maybe WAGNI (we ain't gonna need it), but at least some demand is there.

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

No, partly because of kubernetes/website#42590 (the detailed list of volume types doesn't really belong in that page) and partly because I wouldn't put it there.
We could put the VAP in the removal announcement and make it evergreen; after we do that, other pages can hyperlink there. Hyperlinking to more detail is the right approach.

[thockin]

@sftim:

We could put the VAP in the removal announcement and make it evergreen;

Fotr the less clueful, what does that mean concretely?

[sftim]

[evergreen]

See https://kubernetes.io/docs/contribute/new-content/blogs-case-studies/#guidelines-and-expectations and kubernetes/website#31934

Also https://kubernetes.io/docs/contribute/new-content/blogs-case-studies/#technical-considerations-for-submitting-a-blog-post for what front matter you set.

@thockin I noted your encyclopedic knowledge on many things Kubernetes and inferred you'd know this detail. Sorry about that.

[sftim]

I'd want to block off-by-default on having this in place. But not a merge of this PR.

[BenTheElder]

Yeah, I think we should plan to publish a VAP somewhere, and that should exist by off-by-default, but for alpha it can be a detail we're working out.

[vinayakankugoyal]

Added that we plan to publish the VAPs to a public repository both in the VAP section and in the graduation criteria section.

[toVersus]

I don’t think this is something to discuss here, but I was a bit curious.

Kubernetes's CEL functionality is enhanced with each version, and some features and functions become available only in certain Kubernetes versions or later. If VAPs were to be published in a public repository, it would likely be necessary to explicitly state which Kubernetes version it supports.

This wouldn’t be an issue if newer CEL features weren’t used in VAPs, but for cases like the feature proposed in kubernetes/kubernetes#129939, it would become relevant.

[thockin]

Yeah, this came up in another context last week - we may need CEL-expressions to come with minimum version requirements.

[thockin]

Whose approvals do we need to secure to get this merged? PRR obviously, and @saad-ali has already approved - anyone else?

/approve

[thockin]

Should kubelet issue an event that it used an insecure volume?

[vinayakankugoyal]

Is there precedent for doing that?

[thockin]

I don't think so, but that doesn't make a bad idea out of hand :)

[vinayakankugoyal]

Didn't mean to suggest that it was a bad idea, actually sounds like a good idea, I'm just not sure if its possible because I don't know enough about how those events are generated.

[thockin]

I'm not saying it's a great idea either :) - it's an idea, please consider it and apply your context and wisdom.

[BenTheElder]

This is looking good to me as a PRR shadow, making some remaining minor non-blocking suggestions. Thanks!

[k8s-ci-robot]

@vinayakankugoyal: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-enhancements-test

/test pull-enhancements-verify

Use /test all to run all jobs.

In response to this:

/test pull-kubernetes-cos-cgroupv2-containerd-node-e2e-features
/test pull-kubernetes-cos-cgroupv1-containerd-node-e2e-features
/test pull-crio-cgroupv1-node-e2e-features

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[toVersus]

I don’t think this is something to discuss here, but I was a bit curious.

Kubernetes's CEL functionality is enhanced with each version, and some features and functions become available only in certain Kubernetes versions or later. If VAPs were to be published in a public repository, it would likely be necessary to explicitly state which Kubernetes version it supports.

This wouldn’t be an issue if newer CEL features weren’t used in VAPs, but for cases like the feature proposed in kubernetes/kubernetes#129939, it would become relevant.

[jpbetz]

Thanks @BenTheElder for the 1st pass PRR review. I agree this looks good for alpha.

/approve
For PRR

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, saad-ali, thockin, vinayakankugoyal

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:

• keps/prod-readiness/OWNERS [jpbetz]
• 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

[vinayakankugoyal]

@saad-ali @thockin and @xing-yang - could one of you also LGTM so that this can merge.

[thockin]

/lgtm