Use identifying labels for lookup of EPS on service EPS update

...instead of all labels otherwise, on update to the service labels,
the existing EPS is not found resulting in a new one created.

Related to submariner-io#1646

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>

go.mod

@@ -8,7 +8,7 @@ require (
 	github.com/onsi/gomega v1.31.1
 	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.18.0
-	github.com/submariner-io/admiral v0.17.2
+	github.com/submariner-io/admiral v0.17.3-0.20241007115831-4ce43e9a3d31
 	github.com/submariner-io/shipyard v0.17.2
 	k8s.io/api v0.29.6
 	k8s.io/apimachinery v0.29.6

go.sum

@@ -388,8 +388,8 @@ github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO
 github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4=
 github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
 github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
-github.com/submariner-io/admiral v0.17.2 h1:GjhVcJXC+fZ0igkSRihJKRypNQ3joI91pZIcOpiQmGU=
-github.com/submariner-io/admiral v0.17.2/go.mod h1:WZbjUAVf+tlQnLuvTIzHiF5gNkrD52rIhvuSxm2P2wM=
+github.com/submariner-io/admiral v0.17.3-0.20241007115831-4ce43e9a3d31 h1:XJjBfwDO39G8NNBhjo2UIyTs52q6vW8TDILafoMt07Y=
+github.com/submariner-io/admiral v0.17.3-0.20241007115831-4ce43e9a3d31/go.mod h1:WZbjUAVf+tlQnLuvTIzHiF5gNkrD52rIhvuSxm2P2wM=
 github.com/submariner-io/shipyard v0.17.2 h1:+ev89enbv98uP6BgrIRyVoyXYqOD/+9o49ELjtPugio=
 github.com/submariner-io/shipyard v0.17.2/go.mod h1:Mrp0LPXBXYpbjMwhqq89G86Xgjz+U4vZM9Qg+F1ZBQw=
 github.com/tidwall/pretty v1.0.0/go.mod h1:XNkn88O1ChpSDQmQeStsy+sBenx6DDtFZJxhVysOjyk=

pkg/agent/controller/clusterip_service_test.go

@@ -19,10 +19,12 @@ limitations under the License.
 package controller_test
 
 import (
+	"context"
 	"fmt"
 	"strconv"
 
 	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
 	"github.com/submariner-io/admiral/pkg/fake"
 	"github.com/submariner-io/admiral/pkg/resource"
 	"github.com/submariner-io/admiral/pkg/syncer/test"
@@ -193,6 +195,41 @@ func testClusterIPServiceInOneCluster() {
 		})
 	})
 
+	When("the labels for an exported service are updated", func() {
+		JustBeforeEach(func() {
+			t.cluster1.createService()
+			t.cluster1.createServiceExport()
+			t.awaitNonHeadlessServiceExported(&t.cluster1)
+		})
+
+		It("should update the existing EndpointSlice labels", func() {
+			existingEPS := findEndpointSlices(t.cluster1.localEndpointSliceClient, t.cluster1.service.Namespace,
+				t.cluster1.service.Name, t.cluster1.clusterID)[0]
+
+			By("Updating service labels")
+
+			newLabelName := "new-label"
+			newLabelValue := "new-value"
+
+			t.cluster1.service.Labels[newLabelName] = newLabelValue
+			t.cluster1.serviceEndpointSlices[0].Labels[newLabelName] = newLabelValue
+
+			t.cluster1.updateServiceEndpointSlices()
+
+			Eventually(func() map[string]string {
+				eps, err := t.cluster1.localEndpointSliceClient.Get(context.TODO(), existingEPS.Name, metav1.GetOptions{})
+				Expect(err).To(Succeed())
+
+				return eps.GetLabels()
+			}).Should(HaveKeyWithValue(newLabelName, newLabelValue))
+
+			newSlices := findEndpointSlices(t.cluster1.localEndpointSliceClient, t.cluster1.service.Namespace,
+				t.cluster1.service.Name, t.cluster1.clusterID)
+			Expect(newSlices).To(HaveLen(1))
+			Expect(newSlices[0].Name).To(Equal(existingEPS.Name))
+		})
+	})
+
 	When("two Services with the same name in different namespaces are exported", func() {
 		It("should correctly export both services", func() {
 			t.cluster1.createService()

pkg/agent/controller/service_endpoint_slices.go

@@ -28,7 +28,9 @@ import (
 	"github.com/pkg/errors"
 	"github.com/submariner-io/admiral/pkg/federate"
 	"github.com/submariner-io/admiral/pkg/log"
+	"github.com/submariner-io/admiral/pkg/resource"
 	"github.com/submariner-io/admiral/pkg/syncer"
+	"github.com/submariner-io/admiral/pkg/util"
 	"github.com/submariner-io/lighthouse/pkg/constants"
 	discovery "k8s.io/api/discovery/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -175,29 +177,7 @@ func (c *ServiceEndpointSliceController) onServiceEndpointSlice(obj runtime.Obje
 		return nil, false
 	}
 
-	if op == syncer.Delete {
-		list, err := c.localClient.List(context.TODO(), metav1.ListOptions{
-			LabelSelector: k8slabels.SelectorFromSet(returnEPS.Labels).String(),
-		})
-		if err != nil {
-			logger.Error(err, "Error listing EndpointSlice resources for delete")
-			return nil, true
-		}
-
-		if len(list.Items) == 0 {
-			logger.V(log.DEBUG).Infof("Existing EndpointSlice not found with labels: %#v", returnEPS.Labels)
-			return nil, false
-		}
-
-		returnEPS.Name = list.Items[0].GetName()
-	}
-
-	name := returnEPS.Name
-	if name == "" {
-		name = returnEPS.GenerateName
-	}
-
-	logger.V(logLevel).Infof("Returning EndpointSlice %s/%s: %s", serviceEPS.Namespace, name,
+	logger.V(logLevel).Infof("Returning EndpointSlice %s/%s: %s", serviceEPS.Namespace, returnEPS.GenerateName,
 		endpointSliceStringer{returnEPS})
 
 	return returnEPS, false
@@ -345,12 +325,48 @@ func (c *ServiceEndpointSliceController) isHeadless() bool {
 }
 
 func (c *ServiceEndpointSliceController) Distribute(ctx context.Context, obj runtime.Object) error {
-	return c.federator.Distribute(ctx, obj) //nolint:wrapcheck // No need to wrap here
+	toDistribute := resource.MustToUnstructured(obj)
+	labels := toDistribute.GetLabels()
+
+	identifyingLabels := map[string]string{}
+	if c.isHeadless() {
+		identifyingLabels[constants.LabelSourceName] = labels[constants.LabelSourceName]
+	} else {
+		identifyingLabels[mcsv1a1.LabelServiceName] = labels[mcsv1a1.LabelServiceName]
+		identifyingLabels[constants.LabelSourceNamespace] = labels[constants.LabelSourceNamespace]
+		identifyingLabels[constants.MCSLabelSourceCluster] = labels[constants.MCSLabelSourceCluster]
+	}
+
+	_, _, err := util.CreateOrUpdateWithOptions[*unstructured.Unstructured](ctx, util.CreateOrUpdateOptions[*unstructured.Unstructured]{
+		Client: resource.ForDynamic(c.localClient),
+		Obj:    toDistribute,
+		MutateOnUpdate: func(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) {
+			return util.CopyImmutableMetadata(obj, toDistribute), nil
+		},
+		IdentifyingLabels: identifyingLabels,
+	})
+
+	return err
 }
 
 func (c *ServiceEndpointSliceController) Delete(ctx context.Context, obj runtime.Object) error {
 	if c.isHeadless() {
-		return c.federator.Delete(ctx, obj) //nolint:wrapcheck // No need to wrap here
+		list, err := c.localClient.List(ctx, metav1.ListOptions{
+			LabelSelector: k8slabels.Set(map[string]string{
+				constants.LabelSourceName: resource.MustToMeta(obj).GetLabels()[constants.LabelSourceName],
+			}).String(),
+		})
+		if err != nil {
+			return errors.Wrap(err, "error listing EndpointSlice resources for delete")
+		}
+
+		if len(list.Items) == 0 {
+			logger.V(log.DEBUG).Infof("Existing EndpointSlice not found for service EPS %q",
+				resource.MustToMeta(obj).GetLabels()[constants.LabelSourceName])
+			return nil
+		}
+
+		return c.localClient.Delete(ctx, list.Items[0].GetName(), metav1.DeleteOptions{}) //nolint:wrapcheck // No need to wrap here
 	}
 
 	// For a non-headless service, we never delete the single exported EPS - we update its endpoint condition based on