Merge pull request #228 from numtide/fix/multiadmin-web-env-vars-and-multigateway-service

feat(cluster-handler): add multigateway global service and fix env vars

Author: fernando-villalba

pkg/cluster-handler/controller/multigrescluster/builders_global.go

@@ -255,9 +255,13 @@ func BuildMultiAdminWebDeployment(
 							Name:  "multiadmin-web",
 							Image: string(cluster.Spec.Images.MultiAdminWeb),
 							Env: []corev1.EnvVar{
+								{
+									Name:  "MULTIADMIN_API_URL",
+									Value: fmt.Sprintf("http://%s-multiadmin:18000", cluster.Name),
+								},
 								{
 									Name:  "POSTGRES_HOST",
-									Value: "multigateway",
+									Value: fmt.Sprintf("%s-multigateway", cluster.Name),
 								},
 								{
 									Name:  "POSTGRES_PORT",
@@ -349,3 +353,48 @@ func BuildMultiAdminWebService(
 
 	return svc, nil
 }
+
+// BuildMultiGatewayGlobalService constructs a cluster-level Service that selects
+// all multigateway pods across all cells. This provides a stable DNS name for
+// multiadmin-web to connect to PostgreSQL via any available multigateway,
+// regardless of which cells exist.
+func BuildMultiGatewayGlobalService(
+	cluster *multigresv1alpha1.MultigresCluster,
+	scheme *runtime.Scheme,
+) (*corev1.Service, error) {
+	labels := metadata.BuildStandardLabels(cluster.Name, metadata.ComponentMultiGateway)
+	metadata.AddClusterLabel(labels, cluster.Name)
+
+	// The selector intentionally omits the cell label so it matches
+	// multigateway pods from all cells in this cluster.
+	selector := map[string]string{
+		metadata.LabelAppComponent: metadata.ComponentMultiGateway,
+		metadata.LabelAppInstance:  cluster.Name,
+	}
+
+	svc := &corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      fmt.Sprintf("%s-multigateway", cluster.Name),
+			Namespace: cluster.Namespace,
+			Labels:    labels,
+		},
+		Spec: corev1.ServiceSpec{
+			Selector: selector,
+			Type:     corev1.ServiceTypeClusterIP,
+			Ports: []corev1.ServicePort{
+				{
+					Name:       "postgres",
+					Port:       15432,
+					TargetPort: intstr.FromString("postgres"),
+					Protocol:   corev1.ProtocolTCP,
+				},
+			},
+		},
+	}
+
+	if err := controllerutil.SetControllerReference(cluster, svc, scheme); err != nil {
+		return nil, err
+	}
+
+	return svc, nil
+}

pkg/cluster-handler/controller/multigrescluster/builders_global_test.go

@@ -1,6 +1,7 @@
 package multigrescluster
 
 import (
+	"fmt"
 	"testing"
 
 	multigresv1alpha1 "github.com/numtide/multigres-operator/api/v1alpha1"
@@ -209,6 +210,31 @@ func TestBuildMultiAdminWebDeployment(t *testing.T) {
 			}
 		}
 
+		// Verify env vars
+		envVars := got.Spec.Template.Spec.Containers[0].Env
+		wantEnv := map[string]string{
+			"MULTIADMIN_API_URL": fmt.Sprintf("http://%s-multiadmin:18000", cluster.Name),
+			"POSTGRES_HOST":      fmt.Sprintf("%s-multigateway", cluster.Name),
+			"POSTGRES_PORT":      "15432",
+			"POSTGRES_DATABASE":  "postgres",
+			"POSTGRES_USER":      "postgres",
+		}
+		for wantName, wantValue := range wantEnv {
+			found := false
+			for _, ev := range envVars {
+				if ev.Name == wantName {
+					found = true
+					if ev.Value != wantValue {
+						t.Errorf("Env %s = %q, want %q", wantName, ev.Value, wantValue)
+					}
+					break
+				}
+			}
+			if !found {
+				t.Errorf("Missing env var %s", wantName)
+			}
+		}
+
 		// Verify Selector does NOT contain mutable labels
 		selector := got.Spec.Selector.MatchLabels
 		if _, ok := selector["app.kubernetes.io/name"]; ok {
@@ -310,3 +336,64 @@ func TestBuildMultiAdminService(t *testing.T) {
 		}
 	})
 }
+
+func TestBuildMultiGatewayGlobalService(t *testing.T) {
+	scheme := runtime.NewScheme()
+	_ = multigresv1alpha1.AddToScheme(scheme)
+	_ = corev1.AddToScheme(scheme)
+
+	cluster := &multigresv1alpha1.MultigresCluster{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-cluster",
+			Namespace: "default",
+			UID:       "cluster-uid",
+		},
+	}
+
+	t.Run("Success", func(t *testing.T) {
+		got, err := BuildMultiGatewayGlobalService(cluster, scheme)
+		if err != nil {
+			t.Fatalf("BuildMultiGatewayGlobalService() error = %v", err)
+		}
+
+		if got.Name != "my-cluster-multigateway" {
+			t.Errorf("Name = %v, want %v", got.Name, "my-cluster-multigateway")
+		}
+
+		// Verify selector targets all multigateways for this cluster (no cell label)
+		selector := got.Spec.Selector
+		if selector["app.kubernetes.io/component"] != "multigateway" {
+			t.Errorf("Selector component = %v, want multigateway", selector["app.kubernetes.io/component"])
+		}
+		if selector["app.kubernetes.io/instance"] != "my-cluster" {
+			t.Errorf("Selector instance = %v, want my-cluster", selector["app.kubernetes.io/instance"])
+		}
+		if _, hasCell := selector["multigres.com/cell"]; hasCell {
+			t.Error("Selector must NOT contain cell label to match all cells")
+		}
+
+		// Verify port
+		if len(got.Spec.Ports) != 1 {
+			t.Fatalf("Ports count = %d, want 1", len(got.Spec.Ports))
+		}
+		if got.Spec.Ports[0].Port != 15432 {
+			t.Errorf("Port = %d, want 15432", got.Spec.Ports[0].Port)
+		}
+		if got.Spec.Ports[0].Name != "postgres" {
+			t.Errorf("Port name = %v, want postgres", got.Spec.Ports[0].Name)
+		}
+
+		// Verify OwnerReference
+		if len(got.OwnerReferences) != 1 {
+			t.Errorf("OwnerReferences count = %v, want 1", len(got.OwnerReferences))
+		}
+	})
+
+	t.Run("ControllerRefError", func(t *testing.T) {
+		emptyScheme := runtime.NewScheme()
+		_, err := BuildMultiGatewayGlobalService(cluster, emptyScheme)
+		if err == nil {
+			t.Error("Expected error due to missing scheme types, got nil")
+		}
+	})
+}

pkg/cluster-handler/controller/multigrescluster/integration_test.go

@@ -360,7 +360,8 @@ func TestMultigresCluster_HappyPath(t *testing.T) {
 										},
 										Resources: resolver.DefaultResourcesAdminWeb(),
 										Env: []corev1.EnvVar{
-											{Name: "POSTGRES_HOST", Value: "multigateway"},
+											{Name: "MULTIADMIN_API_URL", Value: "http://" + clusterName + "-multiadmin:18000"},
+											{Name: "POSTGRES_HOST", Value: clusterName + "-multigateway"},
 											{Name: "POSTGRES_PORT", Value: "15432"},
 											{Name: "POSTGRES_DATABASE", Value: "postgres"},
 											{Name: "POSTGRES_USER", Value: "postgres"},
@@ -643,7 +644,8 @@ func TestMultigresCluster_HappyPath(t *testing.T) {
 										},
 										Resources: resolver.DefaultResourcesAdminWeb(),
 										Env: []corev1.EnvVar{
-											{Name: "POSTGRES_HOST", Value: "multigateway"},
+											{Name: "MULTIADMIN_API_URL", Value: "http://minimal-cluster-multiadmin:18000"},
+											{Name: "POSTGRES_HOST", Value: "minimal-cluster-multigateway"},
 											{Name: "POSTGRES_PORT", Value: "15432"},
 											{Name: "POSTGRES_DATABASE", Value: "postgres"},
 											{Name: "POSTGRES_USER", Value: "postgres"},
@@ -923,7 +925,8 @@ func TestMultigresCluster_HappyPath(t *testing.T) {
 										},
 										Resources: resolver.DefaultResourcesAdminWeb(),
 										Env: []corev1.EnvVar{
-											{Name: "POSTGRES_HOST", Value: "multigateway"},
+											{Name: "MULTIADMIN_API_URL", Value: "http://lazy-cluster-multiadmin:18000"},
+											{Name: "POSTGRES_HOST", Value: "lazy-cluster-multigateway"},
 											{Name: "POSTGRES_PORT", Value: "15432"},
 											{Name: "POSTGRES_DATABASE", Value: "postgres"},
 											{Name: "POSTGRES_USER", Value: "postgres"},

pkg/cluster-handler/controller/multigrescluster/multigrescluster_controller.go

@@ -3,6 +3,7 @@ package multigrescluster
 import (
 	"context"
 	"fmt"
+	"slices"
 	"time"
 
 	appsv1 "k8s.io/api/apps/v1"
@@ -23,8 +24,11 @@ import (
 	multigresv1alpha1 "github.com/numtide/multigres-operator/api/v1alpha1"
 	"github.com/numtide/multigres-operator/pkg/monitoring"
 	"github.com/numtide/multigres-operator/pkg/resolver"
+	"github.com/numtide/multigres-operator/pkg/util/metadata"
 )
 
+const finalizerName = "multigres.com/cluster-cleanup"
+
 // MultigresClusterReconciler reconciles a MultigresCluster object.
 type MultigresClusterReconciler struct {
 	client.Client
@@ -40,6 +44,7 @@ type MultigresClusterReconciler struct {
 // +kubebuilder:rbac:groups=multigres.com,resources=multigresclusters/finalizers,verbs=update
 // +kubebuilder:rbac:groups=multigres.com,resources=coretemplates;celltemplates;shardtemplates,verbs=get;list;watch
 // +kubebuilder:rbac:groups=multigres.com,resources=cells;tablegroups;toposervers,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=multigres.com,resources=shards,verbs=get;list;watch
 // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;patch;delete
 func (r *MultigresClusterReconciler) Reconcile(
 	ctx context.Context,
@@ -107,9 +112,19 @@ func (r *MultigresClusterReconciler) Reconcile(
 		}
 	}
 
-	// If being deleted, let Kubernetes GC handle cleanup
+	// Add finalizer on first reconcile to guarantee ordered deletion.
+	// We do not return early here because GenerationChangedPredicate would
+	// filter out the metadata-only update, preventing child resource creation.
+	if !slices.Contains(cluster.Finalizers, finalizerName) {
+		cluster.Finalizers = append(cluster.Finalizers, finalizerName)
+		if err := r.Update(ctx, cluster); err != nil {
+			l.Error(err, "Failed to add finalizer")
+			return ctrl.Result{}, err
+		}
+	}
+
 	if !cluster.DeletionTimestamp.IsZero() {
-		return ctrl.Result{}, nil
+		return r.handleDeletion(ctx, cluster)
 	}
 
 	{
@@ -199,6 +214,75 @@ func (r *MultigresClusterReconciler) Reconcile(
 	return ctrl.Result{}, nil
 }
 
+// handleDeletion orchestrates phased deletion of the MultigresCluster.
+// It deletes Cells and TableGroups first so their data-handler finalizers
+// can run against the still-live topo servers. Once all Cells and Shards
+// are fully removed, it removes our finalizer, allowing Kubernetes GC
+// to clean up the remaining resources (topo servers, deployments).
+func (r *MultigresClusterReconciler) handleDeletion(
+	ctx context.Context,
+	cluster *multigresv1alpha1.MultigresCluster,
+) (ctrl.Result, error) {
+	l := log.FromContext(ctx)
+	clusterLabels := client.MatchingLabels{metadata.LabelMultigresCluster: cluster.Name}
+	ns := client.InNamespace(cluster.Namespace)
+
+	// Delete all Cells owned by this cluster.
+	cells := &multigresv1alpha1.CellList{}
+	if err := r.List(ctx, cells, ns, clusterLabels); err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to list cells: %w", err)
+	}
+	for i := range cells.Items {
+		if cells.Items[i].DeletionTimestamp.IsZero() {
+			if err := r.Delete(ctx, &cells.Items[i]); err != nil && !errors.IsNotFound(err) {
+				return ctrl.Result{}, fmt.Errorf("failed to delete cell %q: %w", cells.Items[i].Name, err)
+			}
+			l.Info("Initiated cell deletion", "cell", cells.Items[i].Name)
+		}
+	}
+
+	// Delete all TableGroups owned by this cluster (cascades to Shards).
+	tableGroups := &multigresv1alpha1.TableGroupList{}
+	if err := r.List(ctx, tableGroups, ns, clusterLabels); err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to list tablegroups: %w", err)
+	}
+	for i := range tableGroups.Items {
+		if tableGroups.Items[i].DeletionTimestamp.IsZero() {
+			if err := r.Delete(ctx, &tableGroups.Items[i]); err != nil && !errors.IsNotFound(err) {
+				return ctrl.Result{}, fmt.Errorf("failed to delete tablegroup %q: %w", tableGroups.Items[i].Name, err)
+			}
+			l.Info("Initiated tablegroup deletion", "tablegroup", tableGroups.Items[i].Name)
+		}
+	}
+
+	// Check if any Cells or Shards still exist (waiting for data-handler finalizer processing).
+	shards := &multigresv1alpha1.ShardList{}
+	if err := r.List(ctx, shards, ns, clusterLabels); err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to list shards: %w", err)
+	}
+
+	remaining := len(cells.Items) + len(shards.Items)
+	if remaining > 0 {
+		l.Info("Waiting for data-handler finalizers",
+			"remainingCells", len(cells.Items),
+			"remainingShards", len(shards.Items),
+		)
+		return ctrl.Result{RequeueAfter: 2 * time.Second}, nil
+	}
+
+	// All Cells and Shards are gone — safe to remove our finalizer.
+	cluster.Finalizers = slices.DeleteFunc(cluster.Finalizers, func(s string) bool {
+		return s == finalizerName
+	})
+	if err := r.Update(ctx, cluster); err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to remove finalizer: %w", err)
+	}
+
+	l.Info("Cluster cleanup complete, finalizer removed")
+	r.Recorder.Event(cluster, "Normal", "CleanupComplete", "All data-handler resources cleaned up")
+	return ctrl.Result{}, nil
+}
+
 // SetupWithManager sets up the controller with the Manager.
 func (r *MultigresClusterReconciler) SetupWithManager(
 	mgr ctrl.Manager,

pkg/cluster-handler/controller/multigrescluster/reconcile_global.go

@@ -14,9 +14,10 @@ import (
 
 // Builder function variables to allow mocking in tests
 var (
-	buildMultiAdminService       = BuildMultiAdminService
-	buildMultiAdminWebDeployment = BuildMultiAdminWebDeployment
-	buildMultiAdminWebService    = BuildMultiAdminWebService
+	buildMultiAdminService         = BuildMultiAdminService
+	buildMultiAdminWebDeployment   = BuildMultiAdminWebDeployment
+	buildMultiAdminWebService      = BuildMultiAdminWebService
+	buildMultiGatewayGlobalService = BuildMultiGatewayGlobalService
 )
 
 func (r *MultigresClusterReconciler) reconcileGlobalComponents(
@@ -231,5 +232,22 @@ func (r *MultigresClusterReconciler) reconcileMultiAdminWeb(
 		return fmt.Errorf("failed to apply multiadmin-web service: %w", err)
 	}
 
+	// 3. Reconcile global multigateway Service
+	desiredGwSvc, err := buildMultiGatewayGlobalService(cluster, r.Scheme)
+	if err != nil {
+		return fmt.Errorf("failed to build global multigateway service: %w", err)
+	}
+
+	desiredGwSvc.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("Service"))
+	if err := r.Patch(
+		ctx,
+		desiredGwSvc,
+		client.Apply,
+		client.ForceOwnership,
+		client.FieldOwner("multigres-operator"),
+	); err != nil {
+		return fmt.Errorf("failed to apply global multigateway service: %w", err)
+	}
+
 	return nil
 }