Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[KEP 5080]Ordered Namespace Deletion #5095

Merged
merged 3 commits into from
Feb 5, 2025

Conversation

cici37
Copy link
Contributor

@cici37 cici37 commented Jan 28, 2025

  • One-line PR description: Add the KEP for ordered namespace deletion.
  • Other comments:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 28, 2025
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 28, 2025
@cici37 cici37 mentioned this pull request Jan 25, 2025
4 tasks
@cici37
Copy link
Contributor Author

cici37 commented Jan 28, 2025

/cc @thockin @jpbetz @deads2k @aojea

@thockin thockin self-assigned this Jan 28, 2025
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this one up.

I am LGTM but for some smaller details.


Option 2: have the deletion priority bands defined instead of using numbers.

To begin with, the deletion order bands would be introduced:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer this approach - it's simpler to understand and less likely to cause people to want to do strict ordering of every resource. We can assign them numeric values with huge gaps between, so we could insert more if we ever need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could begin with something simpler and add more groups if needed later. I guess both could leave space for future possible deletion order insertion. :)


Each resource type will be assigned a `NamespaceDeletionOrder` value(<TOBESOLVED - @cici37>int value or string value). To define the DeletionOrder, the options are:

1. Add a field into APIResource (not mutable) so it could be observed easily(pkg/registry):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this option, but my feeling is not SUPER strong. Willing to be overruled if approvers have great reasons.

- Attempt to honor the DeletionOrderPriority for resource deletion.

- Monitor the deletion process for each `NamespaceDeletionOrder`. If the process hangs beyond a predefined timeout (e.g., 5 minutes),
it will detect the stall and trigger the deletion attempt for the next `NamespaceDeletionOrder` group.
Copy link

@brakthehack brakthehack Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this algorithm violate Goal #2? For example, if someone crafted a vulnerability that somehow prevents the pod from being deleted, could they force the system to delete the policies ahead of the pods thereby re-opening the original problem statement?

If you intend this to be used for security purposes, I suspect that enforcing the invariant in all cases would prevent it from abuse at the cost of user convenience.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @danwinship also said something similar, let's discuss.

If we treat this as an inviolable ordering, we do have a risk of introducing a dep cycle which requires human action to break. In particular "it worked fine in 1.32 and broke in 1.33". We can argue that this is "safer" but that's small consolation to the users whose stuff broke.

If we treat this as a strong but not inviolable ordering, we avoid cycles (at the cost of time) but some attacks remain possible. It's no worse than today, but we are trying to close the hole.

So which is worse?

Another course could be to do timeouts today, publish metrics on how often they fail, increase those timeouts over a few releases and then move it to inviolable. E.g. .1.33 is 5min; 1.34 is 10min; 1.35 is 30min; 1.36 is infinite. Anyone for whom this is a real issue will have time to scream and/or fix the broken pattern.

Another course would be to make it admin-configured, but I would be against that. Mentioning it for completeness.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we treat this as an inviolable ordering, we do have a risk of introducing a dep cycle which requires human action to break. In particular "it worked fine in 1.32 and broke in 1.33". We can argue that this is "safer" but that's small consolation to the users whose stuff broke.

What usage, concretely, do we think we're breaking? I think it's easier to weigh this with realistic examples.

If we treat this as a strong but not inviolable ordering, we avoid cycles (at the cost of time) but some attacks remain possible. It's no worse than today, but we are trying to close the hole.

Well, playing devil's advocate ... It's worse in that people may have a false sense of security that "we fixed the problem" ... except for a big ol asterisk that actually you can still intentionally guarantee a network policy bypass on deletion, which we have to hope people notice and respect?

It's not worse in that the accidental case is less likely, but we should be careful to not advertise it as a security feature then. And we are when we mention a "vulnerability" to address in the KEP motivation ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danwinship point about admin-applied NetPol (https://github.com/kubernetes/enhancements/pull/5095/files#r1936070816) is really important and could be the thing that swings the decision.

On one hand, the cycle-breaking model is not worse than today. On the other hand, that's faint praise. The spectre of dep-cycles is hypothetical. If we have enough information in the NS conditions that a human could see WHY it is "stuck", maybe that's good enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you already hang deletion on misbehaving finalizers?

Oh no: https://stackoverflow.com/a/59667608

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you saying to special-case pods to check for delete OR a terminal state?

I'm saying that would be the only case. No priorities, no bands, no generic mechanism, no extensibility. When you delete a Namespace, instead of doing "delete all the resources in the namespace (in an undefined order)" it would do:

  1. Delete all pods in the namespace (in an undefined order).
  2. Wait for all the pods to be stopped or deleted.
  3. Delete all the other resources in the namespace (in an undefined order).

That's it. That's the KEP.

We don't need a fully-generic solution just to solve the Pods-vs-NetworkPolicies problem, and we can't be confident that the proposed generic solution is actually generic enough to solve any problems we don't currently know of.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find a gap to Dan's proposal 🥇

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cici37 This is a much more limited scope, what do you think? I am worried that we are solving JUST ONE specific problem, but Dan's right that we don't actuyally know if the more complex solution would solve anything else, because we don't know anything else's problem statements.

Your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love @danwinship's proposal. The more order we introduced, the more conflicts it would bring(especially with all the different circumstance of the policy usage and issues it may bring). I'll update the KEP accordingly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in spanish the idiom for this is is "muerto el perro se acabó la rabia" , that comes to say that if you remove the origin of the problem you don't have to worry about the effects (something like that)

Option 2: have the deletion priority bands defined instead of using numbers.

To begin with, the deletion order bands would be introduced:
- Workload Controllers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really matter if you delete workload controllers before workloads? I assume the intent is to stop them from recreating Pods, but once the Namespace has a DeletionTimestamp you can't create new resources in it, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably doesn't REALLY matter, but it seems reasonable to have a band before workloads, which is before default. That said, it could be YAGNI

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could begin with something simply and add order band later only if it's absolutely necessary? I am open with either way

```

After the deletion priority set, the namespace deletion process will honor the priority and delete the resources in sequence.
The resources with higher priority should always be deleted before the resources with lower priority.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"deleted and finalized"? or "fully deleted"?

Need to be clear that it's not enough for the Pods to just be marked for deletion; they need to have actually exited before you delete their NetworkPolicies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cici37 seems worth clarifying, since I think that the term "deleted" across the document refers to "absence of object" that means "the thing that was representing the object in the cluster no longer exist" aka "pod deleted and finalized"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just to add more details, the way we delete resource while namespace deletion is through dynamic client deletion and we would check the resource absence as well.

- Workload Controllers
- Workloads
- Default
- Policies
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are objects in "Policies" that can affect other objects in "Policies", then this ordering may not be enough.

eg, if you have a RoleBinding that controls modifications to NetworkPolicies. (I think that specific example could not actually lead to problems, but the general idea is there.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't have infinite bands, and I think we want to discourage trying to get into super fine-grained global order.

We could have polices1...N or have N integer numbers within each band, but I am not convinced it's a good way to spend the complexity budget, in the absence of real use-cases. If bands map to numbers in the implementation, we can certainly leave room for more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the absence of real use-cases

I'm totally willing to believe that this doesn't affect any real-world use cases, but the KEP currently seems to imply that we are worrying about cases we don't know about yet.

Either we only solve the problem of "Pods that are still running when objects that restrict them are deleted", or else we have to figure out specifically what other sorts of problems we are solving. We can't come up with a generic solution that will correctly protect against entirely unknown threats.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, couldn't this just be:

  1. Everything Else
  2. Policies

Do we have a concrete use case for any of the other bands?

If we think people will need ordering control later, I think you need something more like being able to specify "This type after that type" or at least rc-init style 00 ... 11 ... 23 .. 54 ordering.

Copy link
Member

@thockin thockin Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we allow to break cycles, then I think at least 3 are warranted:

  1. pods
  2. default
  3. policies

This way, a cycle-break on pods moves ahead to default, which might resolve the cycle. Moving into policies is the last resort.

If decide not to do cycle breaks, then 2 is probably OK, but I think 3 still gives additional warm-and-fuzzy that things which EXPLICITLY identify as policies come after things which are uknown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another question is regarding with use case for ValidatingAdmissionPolicy. Say the deletion order of VAP is in policies, but VAP is using other resources(e.g. configmap or CRD) for its parameterization which the deletion order is set to `default' and be deleted before, in this case, VAP will be invaluable after the param resource being deleted even through the VAP resource hasn't been deleted yet. Do we have security concern on this case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More than a security concern, what I've found is that an unordered deletion of the parameterization resource can lead the namespace deletion to hang indefinitely.

The namespace hangs if the ValidatingAdmissionPolicy is targeting resources with complex termination flows (such as a PVC and perhaps a LoadBalancer Service) and the associated ValidatingAdmissionPolicyBinding has parameterNotFoundAction set to Deny.
A reproducible example can be found here: kubernetes/kubernetes#129883

Should we add this UC to the KEP as well?

Note:
Perhaps my comment called for a new thread, please let me know if that's the case

When a lack of progress detected(maybe caused by the dependency cycle described above), the options would be:
- Fall back to the previous behavior.
- Pros: The deletion would not be blocked; no breaking changes;
- Cons: Security concern remains unaddressed or could be bypassed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the normal NetworkPolicy case, the user would be bypassing their own security, so... whatever.

But in the "cluster-admin enforcing egress NetworkPolicy on users" case (or any other case involving an admin-created resource vs a user-created resource), this completely breaks the feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this concern related with custom resource which user might create their own policy/workload? We could have the same deletion order applied to CRD and user could configure which deletion group their CR should belong to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But in the "cluster-admin enforcing egress NetworkPolicy on users" case (or any other case involving an admin-created resource vs a user-created resource), this completely breaks the feature.

@danwinship can you elaborate I'm missing the point, if the pod is deleted before the network policy it means that network policies will be applied for the whole live of the pod

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aojea I'm saying, if we obey finalizers on Pods, then the plan is basically broken, because the user can always just add a finalizer to subvert the deletion ordering.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. We were looking at this as an outside attacker hitting the window between NetPol being deleted and Pod being deleted, but Dan points out that the pod owner could use this to hit that window for otherwise disallowed egress!

e.g. admin put a NetPol which says no egress to internet. The pod gets compromised (or inside-job). They add an unresolvable finalizer. When the NS is deleted, the NetPol will eventually go away and the compromised pod can exfiltrate their corporate secrets.

Ouch. So is that worse than having unresolvable deadlocks?

Perhaps we go back to an earlier comment - we have a timeout for a few releases but then convert to hard-stop?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xref to https://github.com/kubernetes/enhancements/pull/5095/files#r1935990201 wherein the discussion of cycle-breaking is going on.

Copy link
Member

@aojea aojea Feb 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

he pod gets compromised (or inside-job). They add an unresolvable finalizer.

that looks like an rbac problem not a deletion namespace order problem, I don't think we need to build a totally bullet proof system, just put the primitives in place in order to do so ... why a cluster admin will allow people to add finalizer to pods, that means that there has to be some controller removing those ... I think the point is right, but the solution is in another layer

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that looks like an rbac problem not a deletion namespace order problem, I don't think we need to build a totally bullet proof system, just put the primitives in place in order to do so ... why a cluster admin will allow people to add finalizer to pods, that means that there has to be some controller removing those ... I think the point is right, but the solution is in another layer

I think if we don't, we have to be careful about communicating then.
If you say "this vulnerability was fixed by this KEP", I expect it to be fixed, not partially mitigated with a pretty gaping hole in it.

A cluster admin might reasonably expect that networkpolicies + some sort of other admission controls (prohibit privileged / host net?) would protect them from bad behavior of a controller able to create pods, and a controller creating pods would pretty reasonably want permission to add a finalizer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This thread has a better discussion for that problem though: #5095 (comment)

When a lack of progress detected(maybe caused by the dependency cycle described above), the options would be:
- Fall back to the previous behavior.
- Pros: The deletion would not be blocked; no breaking changes;
- Cons: Security concern remains unaddressed or could be bypassed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of "Pod outlives policy that restricts the Pod", we don't actually need the Pod to be deleted before we delete the policy, we only need the Pod to be stopped...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In current namespace deletion process, the resources deletion process would not differentiate deleting the resource and stop the resource..

In this case, the `NamespaceDeletionOrder` will be associated with the resources naturally and the resource which does not have an opinion will default to a value.

- Pros:
- Having it configurable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think of this as can also be a Con, since exposing the capacity to establish random deletion priorities between different clusters will make the namespace deletion different per cluster

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think of this as can also be a Con, since exposing the capacity to establish random deletion priorities between different clusters will make the namespace deletion different per cluster

This shouldn't generally break your workload though?
Also, isn't this already arguably try by just adding controllers with finalizers that may not be in all clusters?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think anyone wants it to be user-configurable. The question is whether the type defines it's own classification, or whether we define that external to the type. I think it is intrinsic to the type, but am not going to die on this hill today. I feel like types SHOULD BE tightly sealed plugins, and any time we store information about those types outside of the type itself, it's a smell.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think anyone wants it to be user-configurable. The question is whether the type defines it's own classification, or whether we define that external to the type. I think it is intrinsic to the type, but am not going to die on this hill today. I feel like types SHOULD BE tightly sealed plugins, and any time we store information about those types outside of the type itself, it's a smell.

When you say type, only core types, not CRDs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now core, but when CRDs get this, they would be the same. CRDs which implement policies could denote that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's at least one bit of user config? :-)

Point taken though, we should probably at least spell out how we're leaving room to add that later without resolving details such as "what is the CRD opt-in field" yet?

Comment on lines 275 to 282
2. Maintain a hard-coded map of DeletionOrder per resources for the resources which have a deletion order preference. Any resources which have no preference opinion would be in the Default category.
```
var resourceDeletionOrderPriorities = map[ResourceType]DeletionOrderPriority{
"/pods": DeletionOrdeWorkload,
"apps.k8s.io/deployments": DeletionOrdeWorkloadController.
"networking.k8s.io/networkpolicies”: DeletionOrdePolicy,
……
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have this model in the apiserver aggregator https://github.com/kubernetes/kubernetes/blob/2e64c722492c9bbb54b177258e7b659717f7e3da/cmd/kube-apiserver/app/aggregator.go

/ The proper way to resolve this letting the aggregator know the desired group and version-within-group order of the underlying servers
// is to refactor the genericapiserver.DelegationTarget to include a list of priorities based on which APIs were installed.
// This requires the APIGroupInfo struct to evolve and include the concept of priorities and to avoid mistakes, the core storage map there needs to be updated.
// That ripples out every bit as far as you'd expect, so for 1.7 we'll include the list here instead of being built up during storage.
var apiVersionPriorities = merge(controlplaneapiserver.DefaultGenericAPIServicePriorities(), map[schema.GroupVersion]controlplaneapiserver.APIServicePriority{
	{Group: "", Version: "v1"}: {Group: 18000, Version: 1},
	// to my knowledge, nothing below here collides
	{Group: "apps", Version: "v1"}:                    {Group: 17800, Version: 15},
	{Group: "autoscaling", Version: "v1"}:             {Group: 17500, Version: 15},
	{Group: "autoscaling", Version: "v2"}:             {Group: 17500, Version: 30},
	{Group: "autoscaling", Version: "v2beta1"}:        {Group: 17500, Version: 9},
	{Group: "autoscaling", Version: "v2beta2"}:        {Group: 17500, Version: 1},
	{Group: "batch", Version: "v1"}:                   {Group: 17400, Version: 15},
	{Group: "batch", Version: "v1beta1"}:              {Group: 17400, Version: 9},
	{Group: "batch", Version: "v2alpha1"}:             {Group: 17400, Version: 9},
	{Group: "networking.k8s.io", Version: "v1"}:       {Group: 17200, Version: 15},
	{Group: "networking.k8s.io", Version: "v1beta1"}:  {Group: 17200, Version: 9},
	{Group: "networking.k8s.io", Version: "v1alpha1"}: {Group: 17200, Version: 1},
	{Group: "policy", Version: "v1"}:                  {Group: 17100, Version: 15},
	{Group: "policy", Version: "v1beta1"}:             {Group: 17100, Version: 9},
	{Group: "storage.k8s.io", Version: "v1"}:          {Group: 16800, Version: 15},
	{Group: "storage.k8s.io", Version: "v1beta1"}:     {Group: 16800, Version: 9},
	{Group: "storage.k8s.io", Version: "v1alpha1"}:    {Group: 16800, Version: 1},
	{Group: "scheduling.k8s.io", Version: "v1"}:       {Group: 16600, Version: 15},
	{Group: "node.k8s.io", Version: "v1"}:             {Group: 16300, Version: 15},
	{Group: "node.k8s.io", Version: "v1alpha1"}:       {Group: 16300, Version: 1},
	{Group: "node.k8s.io", Version: "v1beta1"}:        {Group: 16300, Version: 9},
	{Group: "resource.k8s.io", Version: "v1beta1"}:    {Group: 16200, Version: 9},
	{Group: "resource.k8s.io", Version: "v1alpha3"}:   {Group: 16200, Version: 1},
	// Append a new group to the end of the list if unsure.
	// You can use min(existing group)-100 as the initial value for a group.
	// Version can be set to 9 (to have space around) for a new group.
})

I think this is simple and easier to maintain in the long term, as the motivation for this is NetworkPolicies and Pods, do we envision to have more use cases to establish priorities on namespace deletion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is simple and easier to maintain in the long term, as the motivation for this is NetworkPolicies and Pods, do we envision to have more use cases to establish priorities on namespace deletion?

I think this is a key outstanding question, which is ongoing in a thread above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is discussed in above comment thread but wanna point out the main benefit we could get from configuring the deletion order through APIResource is the consistency between native type and CRD.

@aojea
Copy link
Member

aojea commented Feb 4, 2025

lol, we have a loop on the comments links, so you get redirected to one thread from another thread that brings you back to the original thread 🤣

@danwinship
Copy link
Contributor

shh! it's a trap for AI scrapers!

@cici37
Copy link
Contributor Author

cici37 commented Feb 5, 2025

Summary on current discussions:

  1. Thanks for @danwinship 's proposal, I have updated the KEP to reflect his comment
 - Delete all pods in the namespace (in an undefined order).
 - Wait for all the pods to be stopped or deleted.
 - Delete all the other resources in the namespace (in an undefined order).
  1. I have added a section for security concern regarding with policy with parameter resources.(section, related issue). However, due to the changes in 1, this will be out of the scope of current KEP.
  2. In this case, we would like to have the deletion process described in 1 being strict enforced with a feature gate guarded.

Thank you so much for the input!

@felipe88alves
Copy link

felipe88alves commented Feb 5, 2025

Apologies if this has already been discussed, but isn't the idea of deleting the ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding resources last simple enough to add here?
Seems like this would solve problem the parameter resource problem.

 - Delete all pods in the namespace (in an undefined order).
 - Wait for all the pods to be stopped or deleted.
 - Delete all the other resources in the namespace except ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding (in an undefined order).
 - Wait for all the deleted resources to be stopped or deleted.
 - Delete all the ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding resources.

@cici37
Copy link
Contributor Author

cici37 commented Feb 5, 2025

Apologies if this has already been discussed, but isn't the idea of deleting the ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding resources last simple enough to add here?

It will not address the issues of:

  1. Security concern regarding with VAP being invalid after parameter resources being deleted
  2. VAP using parameter resource with .spec.paramRef.parameterNotFoundAction: Deny. VAP could even use pod for parameter resource(Even through we would not expect this much in real use case but there is no technical blocker to prevent user from doing so). It would cause blocker for the followup deletion.
  3. VAP is cluster scoped and is not limited on guard resources in current namespace.
  4. The similar concerns exist also with Webhooks using parameter resources.

The current proposal may not be perfect for all scenarios but at least would not make things worse and would address the security concern of workload in a reasonable way.

Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2025
@cici37
Copy link
Contributor Author

cici37 commented Feb 5, 2025

/assign @jpbetz
for PRR and KEP approval. Thank you!

@jpbetz
Copy link
Contributor

jpbetz commented Feb 5, 2025

/approve
For PRR

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cici37, jpbetz, thockin

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2025
@k8s-ci-robot k8s-ci-robot merged commit 373ed87 into kubernetes:master Feb 5, 2025
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.33 milestone Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants