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.

Sign up for GitHub

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

Jump to bottom

Do not delete bastion floating ip if set in spec #2257

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Do not delete bastion floating ip if set in spec #2257

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions controllers/openstackcluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,18 @@ func (r *OpenStackClusterReconciler) deleteBastion(ctx context.Context, scope *s
return err
}

var statusFloatingIP *string
var specFloatingIP *string
if openStackCluster.Status.Bastion != nil && openStackCluster.Status.Bastion.FloatingIP != "" {
// Floating IP set in status
Copy link
Contributor

@EmilienM EmilienM Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Please remove that comment, I think it's too obvious.

statusFloatingIP = &openStackCluster.Status.Bastion.FloatingIP
}
if openStackCluster.Spec.Bastion.FloatingIP != nil {
// Floating IP from the spec
Copy link
Contributor

@EmilienM EmilienM Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Please remove that comment, I think it's too obvious.

specFloatingIP = openStackCluster.Spec.Bastion.FloatingIP
}

if statusFloatingIP != nil && (specFloatingIP == nil || *statusFloatingIP != *specFloatingIP) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here please add a comment on what opinionated decision we make.

We only remove the bastion's floating IP if it exists and if it's the not same value defined both in the spec and in status. This decision was made so if a user specifies a pre-created floating IP that is intended to be used for the bastion, the floating IP won't get removed once the bastion is destroyed.

Or something like that.

if err = networkingService.DeleteFloatingIP(openStackCluster, openStackCluster.Status.Bastion.FloatingIP); err != nil {
handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to delete floating IP: %w", err), false)
return fmt.Errorf("failed to delete floating IP: %w", err)
Expand All @@ -270,6 +281,10 @@ func (r *OpenStackClusterReconciler) deleteBastion(ctx context.Context, scope *s

for _, address := range addresses {
if address.Type == corev1.NodeExternalIP {
// If a floating IP is set for the bastion spec, skip deleting it
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, put more context in this comment.

if specFloatingIP != nil && address.Address == *specFloatingIP {
continue
}
// Floating IP may not have properly saved in bastion status (thus not deleted above), delete any remaining floating IP
if err = networkingService.DeleteFloatingIP(openStackCluster, address.Address); err != nil {
handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to delete floating IP: %w", err), false)
Expand Down