Skip to content

feat: cleanup DNS on FQDN change #166

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

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

feat: cleanup DNS on FQDN change #166

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
57 changes: 50 additions & 7 deletions internal/controller/tunnelbinding_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,13 @@

// Custom data for ease of (re)use

ctx context.Context
log logr.Logger
binding *networkingv1alpha1.TunnelBinding
configmap *corev1.ConfigMap
fallbackTarget string
cfAPI *cf.API
ctx context.Context
log logr.Logger
binding *networkingv1alpha1.TunnelBinding
configmap *corev1.ConfigMap
fallbackTarget string
cfAPI *cf.API
removedHostnames []string
Copy link
Owner

Choose a reason for hiding this comment

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

Would this have hostnames from all tunnel bindings? I don't particularly like that tbh, where modifying one can result in a cleanup from another binding.

Could we do something per tunnelBinding utilizing status to keep track of previous, and delete them if the spec is changed to a new one?

Copy link
Contributor Author

@gifff gifff May 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

No, it only holds removed hostnames for a particular tunnelBinding.

But the way the code is put together, the removedHostnames are shared across tunnelBinding reconciliation. This is fine as long as the MaxConcurrentReconciles is not set (defaults to 1) which it is now, and would get problematic if the concurrency is set larger than 1.

}

// labelsForBinding returns the labels for selecting the Bindings served by a Tunnel.
Expand All @@ -75,6 +76,7 @@
func (r *TunnelBindingReconciler) initStruct(ctx context.Context, tunnelBinding *networkingv1alpha1.TunnelBinding) error {
r.ctx = ctx
r.binding = tunnelBinding
r.removedHostnames = []string{}

var err error
var namespacedName apitypes.NamespacedName
Expand Down Expand Up @@ -195,12 +197,24 @@
if err := r.creationLogic(); err != nil {
return ctrl.Result{}, err
}

if err := r.cleanupDNSLogic(); err != nil {
return ctrl.Result{}, err
}

return ctrl.Result{}, nil
}

func (r *TunnelBindingReconciler) setStatus() error {
status := make([]networkingv1alpha1.ServiceInfo, 0, len(r.binding.Subjects))
var hostnames string
shouldRemoveHostnames := map[string]bool{}

for _, svc := range r.binding.Status.Services {
// Mark hostname for cleanup
shouldRemoveHostnames[svc.Hostname] = true
}

for _, sub := range r.binding.Subjects {
hostname, target, err := r.getConfigForSubject(sub)
if err != nil {
Expand All @@ -210,6 +224,15 @@
}
status = append(status, networkingv1alpha1.ServiceInfo{Hostname: hostname, Target: target})
hostnames += hostname + ","

// Keep / Insert / Update hostname
shouldRemoveHostnames[hostname] = false
}

for hostname, shouldRemove := range shouldRemoveHostnames {
if shouldRemove {
r.removedHostnames = append(r.removedHostnames, hostname)
}
}

r.binding.Status.Services = status
Expand Down Expand Up @@ -256,8 +279,28 @@
return nil
}

func (r *TunnelBindingReconciler) creationLogic() error {
func (r *TunnelBindingReconciler) cleanupDNSLogic() error {
if r.binding.TunnelRef.DisableDNSUpdates || len(r.removedHostnames) < 1 {
return nil
}

errors := false
var err error
for _, hostname := range r.removedHostnames {
if err = r.deleteDNSLogic(hostname); err != nil {
errors = true
}
}

if errors {
r.Recorder.Event(r.binding, corev1.EventTypeWarning, "FailedDNSCleanupPartial", "Some DNS entries failed to cleanup")
return err
}

return nil
}

func (r *TunnelBindingReconciler) creationLogic() error {

Check failure on line 303 in internal/controller/tunnelbinding_controller.go

View workflow job for this annotation

GitHub Actions / lint 

cognitive-complexity: function (*TunnelBindingReconciler).creationLogic has cognitive complexity 13 (> max enabled 7) (revive)
// Add labels for TunnelBinding
if r.binding.Labels == nil {
r.binding.Labels = make(map[string]string)
Expand Down
Loading