From 1a1a778438d9c33eda67bf855a6a17161a5b6b07 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Mon, 7 Oct 2024 08:07:58 -0400 Subject: [PATCH] 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 https://github.com/submariner-io/lighthouse/issues/1646 Signed-off-by: Tom Pantelis --- go.mod | 2 +- go.sum | 4 +- .../controller/clusterip_service_test.go | 37 +++++++++++ .../controller/service_endpoint_slices.go | 66 ++++++++++++------- 4 files changed, 81 insertions(+), 28 deletions(-) diff --git a/go.mod b/go.mod index 25f8a30c..deb7bae3 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 1cea9822..46ef3532 100644 --- a/go.sum +++ b/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= diff --git a/pkg/agent/controller/clusterip_service_test.go b/pkg/agent/controller/clusterip_service_test.go index 1fc9932a..207c7730 100644 --- a/pkg/agent/controller/clusterip_service_test.go +++ b/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() diff --git a/pkg/agent/controller/service_endpoint_slices.go b/pkg/agent/controller/service_endpoint_slices.go index 9215b7d1..c1fdd8fc 100644 --- a/pkg/agent/controller/service_endpoint_slices.go +++ b/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