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

fix: don't delete attached load balancers #492

Closed
wants to merge 2 commits into from

Conversation

rawkode
Copy link

@rawkode rawkode commented Jul 26, 2022

When a Load Balancer is attached via the load-balancer-id annotation,
thee CCM should not be responsible for deleting it as it was likely
provisioned by another means.

Signed-off-by: David Flanagan david@rawkode.dev

Closes #454

Copy link
Collaborator

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

Hey @rawkode, thanks for your contribution!

I'm not sure I follow the rationale for your change though, more context in my comment.

If #454 is to be implemented, shouldn't we add a new annotation to support preserving LBs?

cloud-controller-manager/do/loadbalancers.go Outdated Show resolved Hide resolved
When a load balancer is protected via the load-balancer-protect annotation,
the load balancer will not be deleted when the service is removed.

Signed-off-by: David Flanagan <david@rawkode.dev>
Copy link
Collaborator

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

Thanks, this looks really good.

May I ask you to extend our annotation documentation at https://github.com/digitalocean/digitalocean-cloud-controller-manager/blob/master/docs/controllers/services/annotations.md as well?

@rawkode
Copy link
Author

rawkode commented Aug 2, 2022

Yeah. I'll get that updated

@rawkode
Copy link
Author

rawkode commented Aug 8, 2022

@timoreimann Should be good to go

Copy link
Collaborator

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

lgtm

@timoreimann
Copy link
Collaborator

I'll have another look soon, thanks.

@timoreimann
Copy link
Collaborator

Apologies for having dropped the ball on this one. By now, I think there are out-of-band alternatives to generally protect resources from deletion, e.g., kyverno. I'm going to close out this PR as I think the involved complexity is better managed by such tools.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Protect load balancer from being deleted
3 participants