Fix 2.2.0 router deployment selector migration

internal/kube/controller/controller_test.go

@@ -1357,7 +1357,7 @@ func (f *factory) routerDeployment(name string, namespace string) *unstructured.
 	content := map[string]interface{}{
 		"spec": map[string]interface{}{
 			"selector": map[string]interface{}{
-				"matchLabels": f.routerSelectorWithGroup(name),
+				"matchLabels": f.routerSelector(false),
 			},
 			"template": map[string]interface{}{
 				"spec": map[string]interface{}{
@@ -1482,7 +1482,7 @@ func (r *ExpectedResources) deployment(name string, namespace string) *unstructu
 	content := map[string]interface{}{
 		"spec": map[string]interface{}{
 			"selector": map[string]interface{}{
-				"matchLabels": f.routerSelectorWithGroup(name),
+				"matchLabels": f.routerSelector(false),
 			},
 			"template": map[string]interface{}{
 				"spec": map[string]interface{}{

internal/kube/site/resources/skupper-router-deployment.yaml

@@ -44,7 +44,6 @@ spec:
   selector:
     matchLabels:
       skupper.io/component: router
-      skupper.io/group: {{ .Group }}
   template:
     metadata:
       annotations:

internal/kube/site/site.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 
 	internalnetwork "github.com/skupperproject/skupper/internal/network"
+	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -127,10 +128,53 @@ func (s *Site) CheckLeadListeners(listener *skupperv2alpha1.Listener) bool {
 }
 
 func (s *Site) StartRecovery(site *skupperv2alpha1.Site) error {
-	//TODO: check version and perform any necessary update tasks
+	if err := s.migrateRouterDeploymentSelectors(context.TODO(), site); err != nil {
+		return err
+	}
 	return s.reconcile(site, true)
 }
 
+// migrateRouterDeploymentSelectors deletes router Deployments whose
+// spec.selector includes skupper.io/group so the next reconcile recreates
+// them with the desired selector. Skupper 2.2.0 added skupper.io/group to
+// the selector in PR #2363; that change is reverted in 2.2.1+. See Issue #2440.
+func (s *Site) migrateRouterDeploymentSelectors(ctx context.Context, site *skupperv2alpha1.Site) error {
+	deployments := s.clients.GetKubeClient().AppsV1().Deployments(site.Namespace)
+	list, err := deployments.List(ctx, metav1.ListOptions{
+		LabelSelector: "skupper.io/component=router,skupper.io/type=site",
+	})
+	if err != nil {
+		return fmt.Errorf("router deployment selector migration: list failed: %w", err)
+	}
+	for i := range list.Items {
+		d := &list.Items[i]
+		if !needsSelectorMigration(d) {
+			continue
+		}
+		s.logger.Info("Recreating router Deployment to update immutable selector",
+			slog.String("namespace", site.Namespace),
+			slog.String("name", d.Name))
+		policy := metav1.DeletePropagationBackground
+		preconditions := metav1.Preconditions{UID: &d.UID, ResourceVersion: &d.ResourceVersion}
+		err = deployments.Delete(ctx, d.Name, metav1.DeleteOptions{
+			PropagationPolicy: &policy,
+			Preconditions:     &preconditions,
+		})
+		if err != nil && !errors.IsNotFound(err) {
+			return fmt.Errorf("router deployment selector migration: delete %s failed: %w", d.Name, err)
+		}
+	}
+	return nil
+}
+
+func needsSelectorMigration(d *appsv1.Deployment) bool {
+	if d.Spec.Selector == nil {
+		return false
+	}
+	_, hasGroup := d.Spec.Selector.MatchLabels["skupper.io/group"]
+	return hasGroup
+}
+
 func (s *Site) isEdge() bool {
 	return s.routerMode() == qdr.ModeEdge
 }

internal/kube/site/site_test.go

@@ -16,9 +16,12 @@ import (
 	"github.com/skupperproject/skupper/internal/version"
 	skupperv2alpha1 "github.com/skupperproject/skupper/pkg/apis/skupper/v2alpha1"
 	"gotest.tools/v3/assert"
+	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 )
 
 func TestSite_Recover(t *testing.T) {
@@ -90,6 +93,88 @@ func TestSite_Recover(t *testing.T) {
 	}
 }
 
+func TestSite_migrateRouterDeploymentSelectors(t *testing.T) {
+	legacySelector := &metav1.LabelSelector{MatchLabels: map[string]string{
+		"skupper.io/component": "router",
+	}}
+	v220Selector := &metav1.LabelSelector{MatchLabels: map[string]string{
+		"skupper.io/component": "router",
+		"skupper.io/group":     "skupper-router",
+	}}
+	routerLabels := map[string]string{
+		"skupper.io/component": "router",
+		"skupper.io/type":      "site",
+	}
+	makeDeployment := func(name string, selector *metav1.LabelSelector, labels map[string]string) *appsv1.Deployment {
+		return &appsv1.Deployment{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      name,
+				Namespace: "test",
+				UID:       types.UID("uid-" + name),
+				Labels:    labels,
+			},
+			Spec: appsv1.DeploymentSpec{Selector: selector},
+		}
+	}
+
+	tests := []struct {
+		name        string
+		objects     []runtime.Object
+		stillExists []string
+		gone        []string
+	}{
+		{
+			name:    "2.2.0 deployment is recreated",
+			objects: []runtime.Object{makeDeployment("skupper-router", v220Selector, routerLabels)},
+			gone:    []string{"skupper-router"},
+		},
+		{
+			name:        "legacy deployment is left alone",
+			objects:     []runtime.Object{makeDeployment("skupper-router", legacySelector, routerLabels)},
+			stillExists: []string{"skupper-router"},
+		},
+		{
+			name: "HA: only 2.2.0 deployments are deleted",
+			objects: []runtime.Object{
+				makeDeployment("skupper-router", v220Selector, routerLabels),
+				makeDeployment("skupper-router-2", legacySelector, routerLabels),
+			},
+			stillExists: []string{"skupper-router-2"},
+			gone:        []string{"skupper-router"},
+		},
+		{
+			name: "unrelated deployment is ignored",
+			objects: []runtime.Object{
+				makeDeployment("not-a-router", v220Selector, map[string]string{"app": "other"}),
+			},
+			stillExists: []string{"not-a-router"},
+		},
+		{
+			name: "no deployments yet is a no-op",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			s, err := newSiteMocks("test", tt.objects, nil, "", false)
+			assert.NilError(t, err)
+
+			err = s.migrateRouterDeploymentSelectors(context.Background(), s.site)
+			assert.NilError(t, err)
+
+			deployments := s.clients.GetKubeClient().AppsV1().Deployments("test")
+			for _, name := range tt.stillExists {
+				_, err := deployments.Get(context.Background(), name, metav1.GetOptions{})
+				assert.NilError(t, err, "expected %s to still exist", name)
+			}
+			for _, name := range tt.gone {
+				_, err := deployments.Get(context.Background(), name, metav1.GetOptions{})
+				assert.Assert(t, errors.IsNotFound(err), "expected %s to be deleted, got err=%v", name, err)
+			}
+		})
+	}
+}
+
 func TestSite_checkDefaultRouterAccess(t *testing.T) {
 	type args struct {
 		site *skupperv2alpha1.Site