Check deletion_protection when type or scheme has changed #2942

yasinlachiny · 2022-12-22T17:49:12Z

Signed-off-by: yasin.lachiny yasin.lachiny@gmail.com

Issue

Fixes: 2940

Description

If we change the scheme of load balancer it will remove the load balancer and recreate it. but deletion_protection is enabled via an annotation in ingress.

I check deletion_protection and based on that decide whether to recreate ALB or not

Checklist

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the docs directory)
Manually tested
Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

linux-foundation-easycla · 2022-12-22T17:49:16Z

The committers listed above are authorized under a signed CLA.

✅ login: yasinlachiny (6fd3bdd6f83ce3b9543132a3df645ec0e42b5876)

k8s-ci-robot · 2022-12-22T17:49:21Z

Hi @yasinlachiny. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

codecov-commenter · 2022-12-23T08:55:23Z

Codecov Report

Patch coverage has no change and project coverage change: -0.35 ⚠️

Comparison is base (1392585) 54.40% compared to head (a86183d) 54.06%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2942      +/-   ##
==========================================
- Coverage   54.40%   54.06%   -0.35%     
==========================================
  Files         145      145              
  Lines        8429     8483      +54     
==========================================
  Hits         4586     4586              
- Misses       3512     3566      +54     
  Partials      331      331

Impacted Files	Coverage Δ
webhooks/networking/ingress_validator.go	`39.16% <0.00%> (-23.77%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

johngmyers · 2022-12-25T01:19:47Z

I'm a little confused by the logic here. If the scheme of an existing ALB cannot be changed, then changing the scheme in the specification requires removing and recreating the load balancer. Having deletion protection enabled on the old load balancer does not remove this need to replace the LB in order for it to be within spec.

It looks like this PR causes LBC to silently leave the LB out of spec. This does not seem to be desirable behavior.

If the current behavior is not desirable, then an admission webhook should probably prevent modifications to the spec that cannot be implemented with deletion protection when the LB has deletion protection enabled. So that the webhook doesn't have to query the AWS API for the status of the LB, the reconciler should probably store the LB's deletion protection status in the ingresses using it. Since the load balancer status doesn't have a useful field for this, it'd probably have to be an annotation on the Ingress.

yasinlachiny · 2022-12-26T21:41:49Z

Hi @johngmyers
Thank you for reviewing my code.

I changed the logic and used webhook so if a user changes scheme from internal to internet-facing now it shows:

for: "ing.yml": admission webhook "vingress.elbv2.k8s.aws" denied the request: cannot change the scheme or type of ingress test/ingress-test with deletion protection enabled

I used alb and internal as default values.
If the logic is good I'll add a test for the newer function.

johngmyers · 2022-12-28T21:44:41Z

webhooks/networking/ingress_validator.go

@@ -19,6 +19,9 @@ import (

 const (
 	apiPathValidateNetworkingIngress = "/validate-networking-v1-ingress"
+	lbAttrsDeletionProtectionEnabled = "deletion_protection.enabled"
+	schemDefault                     = "internal"


defaultScheme

johngmyers · 2022-12-28T22:03:35Z

webhooks/networking/ingress_validator.go

@@ -19,6 +19,9 @@ import (

 const (
 	apiPathValidateNetworkingIngress = "/validate-networking-v1-ingress"
+	lbAttrsDeletionProtectionEnabled = "deletion_protection.enabled"
+	schemDefault                     = "internal"
+	defaultIngressClass              = "alb"


Per the documentation, the default ingress class is the one with the ingressclass.kubernetes.io/is-default-class annotation set to true. It's not necessarily the one named alb.

I can't find the code that uses that annotation, though.

johngmyers · 2022-12-28T22:05:18Z

webhooks/networking/ingress_validator.go

+	}
+
+	// Check if the scheme or type of the load balancer changed in the new Ingress object
+	if rawSchemaold != rawSchema || ingClass != oldIngClass {


Incorrectly ignores any scheme set in the IngressClassParams.

johngmyers · 2022-12-28T22:07:45Z

webhooks/networking/ingress_validator.go

+	_, err := v.annotationParser.ParseStringMapAnnotation(annotations.IngressSuffixLoadBalancerAttributes, &lbAttributes, ing.Annotations)
+	if err != nil {
+		return "", err
+	}


Incorectly ignores any deletion protection set through the loadBalancerAttributes of the IngressClassParams.

johngmyers · 2022-12-28T22:38:58Z

I do like the approach, though.

A good question would be if this could reuse the controller's code for determining if the old and new ingresses would have deletion protection and/or the scheme changed. Even better would be to determine if the controller would have to reprovision the LB, to make the check cover issues other than scheme.

k8s-ci-robot · 2023-02-20T20:06:48Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yasinlachiny
Once this PR has been reviewed and has the lgtm label, please assign m00nf1sh for approval. For more information see the Kubernetes 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:

OWNERS

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

yasinlachiny · 2023-02-20T20:37:14Z

Hi @johngmyers

Even better would be to determine if the controller would have to reprovision the LB, to make the check cover issues other than sche

We can certainly use an existing function to determine whether we should delete ing. The root cause of this problem is isSDKLoadBalancerRequiresReplacement so this problem occurs when we change scheme or type so by just checking them we will be fine. But for checking this function we need to create the model in ingress_validator and we are able to do this but it's so unclean. We need many fields for building the model and most of them are internal in their packages. Either making them external or using some getter function sounds unclean to me.
Making attributes external needs changing a lot and getter functions are not really the best way.

I did half of the way to creating the model in ingress_validator. If you think it's better I can complete it.
The downsides are:

creating a model twice. one in the validator and one in the actual code.
the getter functions are ok?
I mean something like

func (r *groupReconciler) GetModelBuilder() ingress.ModelBuilder {
	return r.modelBuilder
}

func (r *groupReconciler) GetGroupLoader() ingress.GroupLoader {
	return r.groupLoader
}

Signed-off-by: yasin.lachiny <yasin.lachiny@gmail.com>

yasinlachiny · 2023-03-05T20:50:07Z

Hi @johngmyers @kishorj @M00nF1sh
Could you please share your opinion about the new code if you have time?

k8s-triage-robot · 2023-06-03T21:48:39Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2023-07-03T22:13:32Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

johngmyers · 2023-07-07T02:55:38Z

/remove-lifecycle rotten

k8s-triage-robot · 2024-01-20T10:08:58Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-02-19T10:20:16Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2024-03-20T10:59:01Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2024-03-20T10:59:06Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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/test-infra repository.

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Dec 22, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 22, 2022

k8s-ci-robot requested review from kishorj and M00nF1sh December 22, 2022 17:49

yasinlachiny force-pushed the delete branch from 3b4d741 to 6fd3bdd Compare December 22, 2022 17:51

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 22, 2022

yasinlachiny force-pushed the delete branch from 6fd3bdd to 8a4f5ca Compare December 23, 2022 08:46

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 23, 2022

johngmyers suggested changes Dec 28, 2022

View reviewed changes

yasinlachiny mentioned this pull request Jan 17, 2023

fix: check default ingclass when ingclass is nill #2963

Merged

6 tasks

johngmyers mentioned this pull request Jan 24, 2023

Add johngmyers as reviewer #2999

Merged

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 20, 2023

yasinlachiny force-pushed the delete branch from 90ea5bf to c6a39fe Compare February 20, 2023 20:14

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 20, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2023

yasinlachiny added 2 commits March 5, 2023 16:11

Check deletion_protection when type or scheme has changed

751b3bc

Signed-off-by: yasin.lachiny <yasin.lachiny@gmail.com>

Enable deletion protection for scheme and type with admission webhook

a86183d

Signed-off-by: yasin.lachiny <yasin.lachiny@gmail.com>

yasinlachiny force-pushed the delete branch from c6a39fe to a86183d Compare March 5, 2023 15:14

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 5, 2023

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 3, 2023

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 3, 2023

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jul 7, 2023

johngmyers mentioned this pull request Aug 23, 2023

Add johngmyers as approver #3356

Merged

12 tasks

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 20, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 19, 2024

k8s-ci-robot closed this Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check deletion_protection when type or scheme has changed #2942

Check deletion_protection when type or scheme has changed #2942

yasinlachiny commented Dec 22, 2022 •

edited by kishorj

Loading

linux-foundation-easycla bot commented Dec 22, 2022 •

edited

Loading

k8s-ci-robot commented Dec 22, 2022

codecov-commenter commented Dec 23, 2022 •

edited

Loading

johngmyers commented Dec 25, 2022

yasinlachiny commented Dec 26, 2022 •

edited

Loading

johngmyers Dec 28, 2022

johngmyers Dec 28, 2022

johngmyers Dec 28, 2022

johngmyers Dec 28, 2022

johngmyers commented Dec 28, 2022

k8s-ci-robot commented Feb 20, 2023

yasinlachiny commented Feb 20, 2023

yasinlachiny commented Mar 5, 2023

k8s-triage-robot commented Jun 3, 2023

k8s-triage-robot commented Jul 3, 2023

johngmyers commented Jul 7, 2023

k8s-triage-robot commented Jan 20, 2024

k8s-triage-robot commented Feb 19, 2024

k8s-triage-robot commented Mar 20, 2024

k8s-ci-robot commented Mar 20, 2024

Check deletion_protection when type or scheme has changed #2942

Check deletion_protection when type or scheme has changed #2942

Conversation

yasinlachiny commented Dec 22, 2022 • edited by kishorj Loading

Issue

Description

Checklist

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

linux-foundation-easycla bot commented Dec 22, 2022 • edited Loading

k8s-ci-robot commented Dec 22, 2022

codecov-commenter commented Dec 23, 2022 • edited Loading

Codecov Report

johngmyers commented Dec 25, 2022

yasinlachiny commented Dec 26, 2022 • edited Loading

johngmyers Dec 28, 2022

Choose a reason for hiding this comment

johngmyers Dec 28, 2022

Choose a reason for hiding this comment

johngmyers Dec 28, 2022

Choose a reason for hiding this comment

johngmyers Dec 28, 2022

Choose a reason for hiding this comment

johngmyers commented Dec 28, 2022

k8s-ci-robot commented Feb 20, 2023

yasinlachiny commented Feb 20, 2023

yasinlachiny commented Mar 5, 2023

k8s-triage-robot commented Jun 3, 2023

k8s-triage-robot commented Jul 3, 2023

johngmyers commented Jul 7, 2023

k8s-triage-robot commented Jan 20, 2024

k8s-triage-robot commented Feb 19, 2024

k8s-triage-robot commented Mar 20, 2024

k8s-ci-robot commented Mar 20, 2024

yasinlachiny commented Dec 22, 2022 •

edited by kishorj

Loading

linux-foundation-easycla bot commented Dec 22, 2022 •

edited

Loading

codecov-commenter commented Dec 23, 2022 •

edited

Loading

yasinlachiny commented Dec 26, 2022 •

edited

Loading