Add DeletionProtection mechanism reject Namespace deletion when PVCs are included under NS #1228

kevin1689-cloud · 2023-03-19T13:30:58Z

Ⅰ. Describe what this PR does

This PR complete a feature: The DeletionProtection mechanism reject Namespace deletion when PVCs are included under NameSpace

Ⅱ. Does this pull request fix one issue?

fixes #1223

Ⅲ. Describe how to verify it

1.Create a namespace and label it with "policy.kruise.io/delete-protection=Cascading"
2.Create a pvc in that namespace
3.Try to delete the namespce, the namespace can't be deleted and show an error which tell user the reason is there are existing pvc in the namespce

Ⅳ. Special notes for reviews

None

kruise-bot · 2023-03-19T13:31:14Z

Welcome @kevin1689-cloud! It looks like this is your first PR to openkruise/kruise 🎉

codecov-commenter · 2023-03-19T13:42:24Z

Codecov Report

Merging #1228 (5e1004d) into master (8d59840) will decrease coverage by 0.04%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #1228      +/-   ##
==========================================
- Coverage   48.50%   48.46%   -0.04%     
==========================================
  Files         149      149              
  Lines       20606    20606              
==========================================
- Hits         9995     9987       -8     
- Misses       9517     9524       +7     
- Partials     1094     1095       +1

Flag	Coverage Δ
unittests	`48.46% <ø> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

… are included under namespace (openkruise#1228) Signed-off-by: kevin1689 <kevinyang1689@163.com>

zmberg · 2023-03-23T05:07:28Z

@kevin1689-cloud good, can you can some E2E in code?

kevin1689-cloud · 2023-03-23T05:27:04Z

@kevin1689-cloud good, can you can some E2E in code?

Sure, l will working on that.

zmberg · 2023-03-23T06:30:47Z

@furykerry Is only PVC bound to PV worth protecting?

zmberg · 2023-03-24T03:42:40Z

@kevin1689-cloud I think PVC which unbound to pv or deleting are not worth protecting.

kevin1689-cloud · 2023-03-24T05:16:33Z

@kevin1689-cloud I think PVC which unbound to pv or deleting are not worth protecting.

@zmberg Got it, as the Deletion Protection mechanism is aim to enhance applications' availability, I agree with the PVC which not bound to PV is not worth protecting. I will modify my code and E2E to achieve that.

I checked kubernetes code, PVC have 3 status: Pending, Bound, Lost. So the logic is when PVC's status is in "Bound", protect the NameSpace to be delete, and when PVC is in other status, the NameSpace can be delete, is that correct?

zmberg · 2023-03-24T05:41:42Z

@kevin1689-cloud I think PVC which unbound to pv or deleting are not worth protecting.

@zmberg Got it, as the Deletion Protection mechanism is aim to enhance applications' availability, I agree with the PVC which not bound to PV is not worth protecting. I will modify my code and E2E to achieve that.

I checked kubernetes code, PVC have 3 status: Pending, Bound, Lost. So the logic is when PVC's status is in "Bound", protect the NameSpace to be delete, and when PVC is in other status, the NameSpace can be delete, is that correct?

Yes.

kevin1689-cloud · 2023-03-24T05:45:54Z

@kevin1689-cloud I think PVC which unbound to pv or deleting are not worth protecting.

@zmberg Got it, as the Deletion Protection mechanism is aim to enhance applications' availability, I agree with the PVC which not bound to PV is not worth protecting. I will modify my code and E2E to achieve that.
I checked kubernetes code, PVC have 3 status: Pending, Bound, Lost. So the logic is when PVC's status is in "Bound", protect the NameSpace to be delete, and when PVC is in other status, the NameSpace can be delete, is that correct?

Yes.

Ok.

sonatype-lift · 2023-03-26T14:28:16Z

test/e2e/policy/deletionprotection.go

+					StorageClassName: &storageClassName,
+				},
+			}
+			pvc, err = c.CoreV1().PersistentVolumeClaims(ns.Name).Create(context.TODO(), pvc, metav1.CreateOptions{})


SA4006: this value of pvc is never used

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-03-26T14:28:18Z

test/e2e/policy/deletionprotection.go

+					},
+				},
+			}
+			pv, err = framework.CreatePV(c, pv)


SA4006: this value of pv is never used

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-03-26T14:28:20Z

test/e2e/policy/deletionprotection.go

+			ginkgo.By("Delete the PV bounded to PVC")
+			err = c.CoreV1().PersistentVolumes().Delete(context.TODO(), pvName, metav1.DeleteOptions{})
+			_, err = c.CoreV1().PersistentVolumes().Patch(context.TODO(), pvName, types.StrategicMergePatchType,
+				[]byte(fmt.Sprintf(`{"metadata":{"finalizers":null}}`)), metav1.PatchOptions{})


S1039: unnecessary use of fmt.Sprintf

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

… are included under namespace (openkruise#1228) Signed-off-by: kevin1689 <kevinyang1689@163.com>

kevin1689-cloud · 2023-03-27T02:49:39Z

@zmberg Done. Now namespace will be protected only when PVCs in "Bound" status are included under the namespce. The relevant E2E tests have been added. Please take a look.

zmberg · 2023-03-31T03:31:12Z

pkg/webhook/util/deletionprotection/deletion_protection.go

+		var boundCount int
+		for i := range pvcs.Items {
+			pvc := &pvcs.Items[i]
+			if pvc.Status.Phase == v1.ClaimBound {


if pvc.deletionTimestamp == nil && pvc.Status.Phase == v1.ClaimBound {

}

zmberg · 2023-03-31T03:33:28Z

test/e2e/policy/deletionprotection.go

@@ -19,6 +19,7 @@ package policy
 import (
 	"context"
 	"fmt"
+	"k8s.io/apimachinery/pkg/api/resource"


go imports the 'import' section.

… are included under namespace (openkruise#1228) Signed-off-by: kevin1689 <kevinyang1689@163.com>

… in Bound status are included under NS (openkruise#1228) Signed-off-by: kevin1689 <kevinyang1689@163.com>

kevin1689-cloud · 2023-04-01T03:43:51Z

@zmberg Done. I have made a misoperation when push the code and submitted serveral recently merged issue's code. Now I have fixed this misoperation. Please check and if there are any problem, please tell me. Thanks!

zmberg · 2023-04-06T09:36:33Z

/lgtm

furykerry · 2023-04-06T09:45:01Z

pkg/webhook/util/deletionprotection/deletion_protection.go

@@ -71,6 +71,21 @@ func ValidateNamespaceDeletion(c client.Client, namespace *v1.Namespace) error {
 		if activeCount > 0 {
 			return fmt.Errorf("forbidden by ResourcesProtectionDeletion for %s=%s and active pods %d>0", policyv1alpha1.DeletionProtectionKey, val, activeCount)
 		}
+
+		pvcs := v1.PersistentVolumeClaimList{}
+		if err := c.List(context.TODO(), &pvcs, client.InNamespace(namespace.Name)); err != nil {


plz add DisableDeepCopy option here

and add DisableDeepCopy option in list pods function again.

@furykerry @zmberg Done. Now the list pvc and list pods function is as shown below:

pvcs := v1.PersistentVolumeClaimList{} if err := c.List(context.TODO(), &pvcs, client.InNamespace(namespace.Name), utilclient.DisableDeepCopy);

pods := v1.PodList{} if err := c.List(context.TODO(), &pods, client.InNamespace(namespace.Name), utilclient.DisableDeepCopy);

Please take a look.

… in Bound status are included under NS (openkruise#1228) Signed-off-by: kevin1689 <kevinyang1689@163.com>

furykerry

/lgtm

zmberg · 2023-04-07T06:39:01Z

/approve

kruise-bot · 2023-04-07T06:39:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zmberg

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~~ [zmberg]

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

… in Bound status are included under NS (openkruise#1228) (openkruise#1228) Signed-off-by: kevin1689 <kevinyang1689@163.com>

… in Bound status are included under NS (openkruise#1228) (openkruise#1228)

kruise-bot requested review from FillZpp and zmberg March 19, 2023 13:31

kruise-bot added the size/S size/S 10-29 label Mar 19, 2023

kevin1689-cloud changed the title ~~pvc_deletion_protection~~ Add DeletionProtection mechanism reject Namespace deletion when PVCs are included under NS Mar 19, 2023

kevin1689-cloud force-pushed the pvc_deletion_protection branch from 2cb9866 to 33b35c9 Compare March 20, 2023 00:54

kevin1689-cloud added a commit to kevin1689-cloud/kruise that referenced this pull request Mar 20, 2023

add: DeletionProtection mechanism reject namespace deletion when PVCs…

08d7100

… are included under namespace (openkruise#1228) Signed-off-by: kevin1689 <kevinyang1689@163.com>

kevin1689-cloud force-pushed the pvc_deletion_protection branch from 33b35c9 to 08d7100 Compare March 20, 2023 00:58

kruise-bot added size/M size/M: 30-99 and removed size/S size/S 10-29 labels Mar 26, 2023

sonatype-lift bot reviewed Mar 26, 2023

View reviewed changes

kevin1689-cloud added a commit to kevin1689-cloud/kruise that referenced this pull request Mar 27, 2023

add: DeletionProtection mechanism reject namespace deletion when PVCs…

3ff577b

… are included under namespace (openkruise#1228) Signed-off-by: kevin1689 <kevinyang1689@163.com>

kevin1689-cloud force-pushed the pvc_deletion_protection branch from 5fc170f to 3ff577b Compare March 27, 2023 01:40

zmberg reviewed Mar 31, 2023

View reviewed changes

kevin1689-cloud added a commit to kevin1689-cloud/kruise that referenced this pull request Mar 31, 2023

add: DeletionProtection mechanism reject namespace deletion when PVCs…

738b036

… are included under namespace (openkruise#1228) Signed-off-by: kevin1689 <kevinyang1689@163.com>

kevin1689-cloud force-pushed the pvc_deletion_protection branch from 1f18afb to 6fa1bab Compare March 31, 2023 10:22

kruise-bot added size/L size/L: 100-499 and removed size/M size/M: 30-99 labels Mar 31, 2023

kevin1689-cloud force-pushed the pvc_deletion_protection branch from 6fa1bab to 738b036 Compare March 31, 2023 10:41

kruise-bot added size/L size/L: 100-499 and removed size/M size/M: 30-99 labels Apr 1, 2023

kevin1689-cloud added a commit to kevin1689-cloud/kruise that referenced this pull request Apr 1, 2023

add: DeletionProtection mechanism reject Namespace deletion when PVCs…

5e1004d

… in Bound status are included under NS (openkruise#1228) Signed-off-by: kevin1689 <kevinyang1689@163.com>

kevin1689-cloud force-pushed the pvc_deletion_protection branch from 335a828 to 5e1004d Compare April 1, 2023 02:29

kruise-bot added size/M size/M: 30-99 and removed size/L size/L: 100-499 labels Apr 1, 2023

kevin1689-cloud added a commit to kevin1689-cloud/kruise that referenced this pull request Apr 1, 2023

add: DeletionProtection mechanism reject Namespace deletion when PVCs…

ba0c168

… in Bound status are included under NS (openkruise#1228) Signed-off-by: kevin1689 <kevinyang1689@163.com>

kevin1689-cloud force-pushed the pvc_deletion_protection branch from 5e1004d to ba0c168 Compare April 1, 2023 02:46

kevin1689-cloud requested a review from zmberg April 6, 2023 06:10

kruise-bot assigned zmberg Apr 6, 2023

kruise-bot added the lgtm label Apr 6, 2023

furykerry reviewed Apr 6, 2023

View reviewed changes

add: DeletionProtection mechanism reject Namespace deletion when PVCs…

b41e773

… in Bound status are included under NS (openkruise#1228) Signed-off-by: kevin1689 <kevinyang1689@163.com>

kevin1689-cloud force-pushed the pvc_deletion_protection branch from ba0c168 to b41e773 Compare April 6, 2023 12:22

kruise-bot removed the lgtm label Apr 6, 2023

furykerry reviewed Apr 7, 2023

View reviewed changes

kruise-bot assigned furykerry Apr 7, 2023

kruise-bot added the lgtm label Apr 7, 2023

kruise-bot added the approved label Apr 7, 2023

kruise-bot merged commit 2418fae into openkruise:master Apr 7, 2023
6 of 23 checks passed

zmberg added this to the v1.5 milestone Jun 8, 2023

zmberg added this to In progress in Roadmap via automation Jun 8, 2023

zmberg moved this from In progress to Done in Roadmap Jun 8, 2023

ppbits pushed a commit to ppbits/kruise that referenced this pull request Apr 4, 2024

add: DeletionProtection mechanism reject Namespace deletion when PVCs…

4efb604

… in Bound status are included under NS (openkruise#1228) (openkruise#1228)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DeletionProtection mechanism reject Namespace deletion when PVCs are included under NS #1228

Add DeletionProtection mechanism reject Namespace deletion when PVCs are included under NS #1228

kevin1689-cloud commented Mar 19, 2023

kruise-bot commented Mar 19, 2023

codecov-commenter commented Mar 19, 2023 •

edited

zmberg commented Mar 23, 2023

kevin1689-cloud commented Mar 23, 2023

zmberg commented Mar 23, 2023

zmberg commented Mar 24, 2023

kevin1689-cloud commented Mar 24, 2023

zmberg commented Mar 24, 2023

kevin1689-cloud commented Mar 24, 2023

sonatype-lift bot Mar 26, 2023

sonatype-lift bot Mar 26, 2023

sonatype-lift bot Mar 26, 2023

kevin1689-cloud commented Mar 27, 2023

zmberg Mar 31, 2023

zmberg Mar 31, 2023

kevin1689-cloud commented Apr 1, 2023 •

edited

zmberg commented Apr 6, 2023

furykerry Apr 6, 2023

zmberg Apr 6, 2023

kevin1689-cloud Apr 6, 2023

furykerry left a comment

zmberg commented Apr 7, 2023

kruise-bot commented Apr 7, 2023

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file

Add DeletionProtection mechanism reject Namespace deletion when PVCs are included under NS #1228

Add DeletionProtection mechanism reject Namespace deletion when PVCs are included under NS #1228

Conversation

kevin1689-cloud commented Mar 19, 2023

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

kruise-bot commented Mar 19, 2023

codecov-commenter commented Mar 19, 2023 • edited

Codecov Report

zmberg commented Mar 23, 2023

kevin1689-cloud commented Mar 23, 2023

zmberg commented Mar 23, 2023

zmberg commented Mar 24, 2023

kevin1689-cloud commented Mar 24, 2023

zmberg commented Mar 24, 2023

kevin1689-cloud commented Mar 24, 2023

sonatype-lift bot Mar 26, 2023

Choose a reason for hiding this comment

sonatype-lift bot Mar 26, 2023

Choose a reason for hiding this comment

sonatype-lift bot Mar 26, 2023

Choose a reason for hiding this comment

kevin1689-cloud commented Mar 27, 2023

zmberg Mar 31, 2023

Choose a reason for hiding this comment

zmberg Mar 31, 2023

Choose a reason for hiding this comment

kevin1689-cloud commented Apr 1, 2023 • edited

zmberg commented Apr 6, 2023

furykerry Apr 6, 2023

Choose a reason for hiding this comment

zmberg Apr 6, 2023

Choose a reason for hiding this comment

kevin1689-cloud Apr 6, 2023

Choose a reason for hiding this comment

furykerry left a comment

Choose a reason for hiding this comment

zmberg commented Apr 7, 2023

kruise-bot commented Apr 7, 2023

codecov-commenter commented Mar 19, 2023 •

edited

kevin1689-cloud commented Apr 1, 2023 •

edited