add: Support the deletion protection of service and ingress #1269

kevin1689-cloud · 2023-04-28T00:59:03Z

Ⅰ. Describe what this PR does

This PR complete a feature: Support the deletion protection of service and ingress resources

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

The Cascading judgement of Service and Ingress is whether there are any endpoints exist for the Service or Ingress. The verify steps is as show below:

Service Deleteion Protection
1.Create a Service and label it with "policy.kruise.io/delete-protection=Always"
2.Delete the Service should be rejected
3.Patch the Service with label "policy.kruise.io/delete-protection=Cascading"
4.Create endpoints of the Service
5.Delete the Service should be rejected
6.Delete all endpoints of the Service
7.The Service can be delete successfully

Ingress Deleteion Protection
1.Create a Ingress and label it with "policy.kruise.io/delete-protection=Always"
2.Delete the Ingress should be rejected
3.Patch the Ingress with label "policy.kruise.io/delete-protection=Cascading"
4.Create Service which menotioned in spec.rules.http.paths.backend.service of Ingress spec
5.Create endpoints of the Service
6.Delete the Ingress should be rejected
7.Delete all endpoints of the Service
8.The Ingress can be delete successfully

Ⅳ. Special notes for reviews

None

kevin1689-cloud · 2023-05-01T12:35:46Z

Hi, I saw there are some CI ans E2E checks not successful. I checked my code and found there are indeed some bugs in my code. I'm working on to fix them, it's no need to review my code now. When I'm done, I will leave comment to require for code review. Thanks.

zmberg · 2023-05-08T07:32:08Z

@kevin1689-cloud tks very much, k8s community also have related PR, I have not yet figured out how to implement this.

kevin1689-cloud · 2023-05-18T06:00:42Z

@kevin1689-cloud tks very much, k8s community also have related PR, I have not yet figured out how to implement this.

Got it, I'm reading Conversation of the mentioned k8s community PR, the concept of "Lien" is interesting. I‘ve been busy at my company work recently, I thought I could back to working on this pr about 2-3 weeks later and I will fix the bug in my code, read the k8s community PR carefully and give my opinion on that.

zmberg · 2023-06-01T08:27:11Z

@kevin1689-cloud Should we discuss this at the community meeting next Thursday evening?

kevin1689-cloud · 2023-06-02T00:13:41Z

@kevin1689-cloud Should we discuss this at the community meeting next Thursday evening?

@zmberg Hi, thanks for the invite, how about we discuss this at the community meeting after next Thursday? I have some company project issue to due with in next week, and I'm afraid can't attend the community meeting in time. By the next community meeting, I will be ready.

kevin1689-cloud · 2023-07-09T08:42:08Z

@zmberg Hi, as we discussed, the main scenarios of service&ingress deletion protection is:

Avoid the IP address of service&ingress to be changed due to accidental deletion.

So this time we only support the Always delete protection. I noticed there are two E2E checks not successful, but seems they are not caused by this PR.

Please take a look. Thanks!

zmberg · 2023-07-10T02:26:32Z

config/webhook/manifests.yaml

@@ -474,6 +474,27 @@ webhooks:
    resources:
    - imagepulljobs
  sideEffects: None
+- admissionReviewVersions:


add vingress.kb.io and vservice.kb.io in file

zmberg · 2023-07-10T02:36:51Z

pkg/webhook/add_ingress.go

@@ -0,0 +1,25 @@
+/*
+Copyright 2021 The Kruise Authors.


zmberg · 2023-07-10T02:37:24Z

pkg/webhook/ingress/validating/ingress_handler.go

+	"context"
+	"net/http"
+
+	"github.com/openkruise/kruise/pkg/webhook/util/deletionprotection"


gofmt imports sort

zmberg · 2023-07-10T02:41:56Z

pkg/webhook/service/validating/service_handler.go

+	"context"
+	"net/http"
+
+	"github.com/openkruise/kruise/pkg/webhook/util/deletionprotection"


gofmt imports sort.

kevin1689-cloud · 2023-07-10T04:27:44Z

@zmberg Done. Please take a look.

zmberg · 2023-07-10T07:39:33Z

/lgtm

veophi · 2023-08-07T09:51:23Z

pkg/webhook/add_ingress.go

+)
+
+func init() {
+	addHandlers(validating.HandlerMap)


you'd better use addHandlersWithGate

Do not add the handler if corresponding feature-gate is not enabled.

@veophi Done, did the same change as add_service.go.

sonatype-lift · 2023-08-08T01:39:40Z

pkg/webhook/add_ingress.go

+
+func init() {
+	addHandlersWithGate(validating.HandlerMap, func() (enabled bool) {
+		if !utilfeature.DefaultFeatureGate.Enabled(features.ResourcesDeletionProtection) {


S1008: should use 'return utilfeature.DefaultFeatureGate.Enabled(features.ResourcesDeletionProtection)' instead of 'if !utilfeature.DefaultFeatureGate.Enabled(features.ResourcesDeletionProtection) { return false }; return true'

ℹ️ 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.}

sonatype-lift · 2023-08-08T01:39:42Z

pkg/webhook/add_service.go

+
+func init() {
+	addHandlersWithGate(validating.HandlerMap, func() (enabled bool) {
+		if !utilfeature.DefaultFeatureGate.Enabled(features.ResourcesDeletionProtection) {


S1008: should use 'return utilfeature.DefaultFeatureGate.Enabled(features.ResourcesDeletionProtection)' instead of 'if !utilfeature.DefaultFeatureGate.Enabled(features.ResourcesDeletionProtection) { return false }; return true'

ℹ️ 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.}

Signed-off-by: kevin1689 <kevinyang1689@163.com>

codecov-commenter · 2023-08-17T02:26:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (master@9ccd897). Click here to learn what that means.
Report is 47 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #1269   +/-   ##
=========================================
  Coverage          ?   50.26%           
=========================================
  Files             ?      157           
  Lines             ?    23450           
  Branches          ?        0           
=========================================
  Hits              ?    11787           
  Misses            ?    10460           
  Partials          ?     1203

Flag	Coverage Δ
unittests	`50.26% <ø> (?)`

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zmberg · 2024-03-07T01:56:25Z

/lgtm

zmberg · 2024-03-07T01:56:40Z

/approve

kruise-bot · 2024-03-07T01:56:46Z

[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

…se#1269)

kruise-bot requested review from FillZpp and furykerry April 28, 2023 00:59

kruise-bot added the size/L size/L: 100-499 label Apr 28, 2023

kevin1689-cloud force-pushed the service_ingress_deletion_protection branch 2 times, most recently from 32d5ece to 66ce4af Compare April 28, 2023 11:09

kruise-bot added size/XL size/XL: 500-999 and removed size/L size/L: 100-499 labels Apr 28, 2023

kevin1689-cloud force-pushed the service_ingress_deletion_protection branch from 66ce4af to 0b8567f Compare April 28, 2023 16:42

kevin1689-cloud force-pushed the service_ingress_deletion_protection branch 4 times, most recently from 95c0be5 to 43c376f Compare May 27, 2023 13:08

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

kevin1689-cloud force-pushed the service_ingress_deletion_protection branch from 43c376f to dc63b5a Compare July 9, 2023 04:21

kruise-bot added size/L size/L: 100-499 and removed size/XL size/XL: 500-999 labels Jul 9, 2023

kevin1689-cloud force-pushed the service_ingress_deletion_protection branch from dc63b5a to 78db319 Compare July 9, 2023 04:32

zmberg reviewed Jul 10, 2023

View reviewed changes

pkg/webhook/add_ingress.go Outdated

@@ -0,0 +1,25 @@

/*

Copyright 2021 The Kruise Authors.

Copy link

Member

zmberg Jul 10, 2023

Choose a reason for hiding this comment

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

2023

zmberg reviewed Jul 10, 2023

View reviewed changes

kevin1689-cloud force-pushed the service_ingress_deletion_protection branch from 78db319 to ab3a2f7 Compare July 10, 2023 04:26

veophi reviewed Aug 7, 2023

View reviewed changes

kevin1689-cloud force-pushed the service_ingress_deletion_protection branch from ab3a2f7 to 880aaf9 Compare August 8, 2023 01:26

kruise-bot added size/XL size/XL: 500-999 and removed lgtm size/L size/L: 100-499 labels Aug 8, 2023

sonatype-lift bot reviewed Aug 8, 2023

View reviewed changes

add: Support the deletion protection of service and ingress

aa32751

Signed-off-by: kevin1689 <kevinyang1689@163.com>

kevin1689-cloud force-pushed the service_ingress_deletion_protection branch from 880aaf9 to aa32751 Compare August 8, 2023 02:01

kruise-bot added size/L size/L: 100-499 and removed size/XL size/XL: 500-999 labels Aug 8, 2023

kevin1689-cloud requested a review from veophi August 8, 2023 03:07

kruise-bot added the needs-rebase label Aug 16, 2023

Merge branch 'master' into service_ingress_deletion_protection

b6f1a2e

kruise-bot removed the needs-rebase label Aug 17, 2023

zmberg closed this Jan 5, 2024

Roadmap automation moved this from In progress to Done Jan 5, 2024

zmberg reopened this Jan 5, 2024

Roadmap automation moved this from Done to In progress Jan 5, 2024

kruise-bot added the lgtm label Mar 7, 2024

kruise-bot added the approved label Mar 7, 2024

kruise-bot merged commit 04254fb into openkruise:master Mar 7, 2024
46 of 47 checks passed

Roadmap automation moved this from In progress to Done Mar 7, 2024

zmberg modified the milestones: v1.5, 1.6 Mar 7, 2024

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

add: Support the deletion protection of service and ingress (openkrui…

a468b00

…se#1269)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add: Support the deletion protection of service and ingress #1269

add: Support the deletion protection of service and ingress #1269

kevin1689-cloud commented Apr 28, 2023

kevin1689-cloud commented May 1, 2023

zmberg commented May 8, 2023

kevin1689-cloud commented May 18, 2023 •

edited

Loading

zmberg commented Jun 1, 2023

kevin1689-cloud commented Jun 2, 2023 •

edited

Loading

kevin1689-cloud commented Jul 9, 2023

zmberg Jul 10, 2023

zmberg Jul 10, 2023

zmberg Jul 10, 2023

zmberg Jul 10, 2023

kevin1689-cloud commented Jul 10, 2023

zmberg commented Jul 10, 2023

veophi Aug 7, 2023

veophi Aug 7, 2023

kevin1689-cloud Aug 8, 2023 •

edited

Loading

sonatype-lift bot Aug 8, 2023

sonatype-lift bot Aug 8, 2023

codecov-commenter commented Aug 17, 2023 •

edited by codecov bot

Loading

zmberg commented Mar 7, 2024

zmberg commented Mar 7, 2024

kruise-bot commented Mar 7, 2024

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: Support the deletion protection of service and ingress #1269

add: Support the deletion protection of service and ingress #1269

Conversation

kevin1689-cloud commented Apr 28, 2023

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

kevin1689-cloud commented May 1, 2023

zmberg commented May 8, 2023

kevin1689-cloud commented May 18, 2023 • edited Loading

zmberg commented Jun 1, 2023

kevin1689-cloud commented Jun 2, 2023 • edited Loading

kevin1689-cloud commented Jul 9, 2023

zmberg Jul 10, 2023

Choose a reason for hiding this comment

zmberg Jul 10, 2023

Choose a reason for hiding this comment

zmberg Jul 10, 2023

Choose a reason for hiding this comment

zmberg Jul 10, 2023

Choose a reason for hiding this comment

kevin1689-cloud commented Jul 10, 2023

zmberg commented Jul 10, 2023

veophi Aug 7, 2023

Choose a reason for hiding this comment

veophi Aug 7, 2023

Choose a reason for hiding this comment

kevin1689-cloud Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

sonatype-lift bot Aug 8, 2023

Choose a reason for hiding this comment

sonatype-lift bot Aug 8, 2023

Choose a reason for hiding this comment

codecov-commenter commented Aug 17, 2023 • edited by codecov bot Loading

Codecov Report

zmberg commented Mar 7, 2024

zmberg commented Mar 7, 2024

kruise-bot commented Mar 7, 2024

kevin1689-cloud commented May 18, 2023 •

edited

Loading

kevin1689-cloud commented Jun 2, 2023 •

edited

Loading

kevin1689-cloud Aug 8, 2023 •

edited

Loading

codecov-commenter commented Aug 17, 2023 •

edited by codecov bot

Loading