From 108b3dd3c3f2a60429808cb138781749c9afbe83 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 | 35 ++++++++++ .../controller/service_endpoint_slices.go | 66 ++++++++++++------- 4 files changed, 79 insertions(+), 28 deletions(-) diff --git a/go.mod b/go.mod index 4cd32dee..16168ceb 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,7 @@ require ( github.com/onsi/gomega v1.34.2 github.com/pkg/errors v0.9.1 github.com/prometheus/client_golang v1.20.4 - github.com/submariner-io/admiral v0.19.0-rc1 + github.com/submariner-io/admiral v0.19.0-rc1.0.20241007115040-cd5bf4d54261 github.com/submariner-io/shipyard v0.19.0-rc1 k8s.io/api v0.31.1 k8s.io/apimachinery v0.31.1 diff --git a/go.sum b/go.sum index 4bbf467c..682aebef 100644 --- a/go.sum +++ b/go.sum @@ -389,8 +389,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.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= -github.com/submariner-io/admiral v0.19.0-rc1 h1:UBYQmbnsaICgs6Qyid5egN8uc7L4Ql0DTBx3sb4vihA= -github.com/submariner-io/admiral v0.19.0-rc1/go.mod h1:2bmvHxnJ0piOMriKolIJ4/o5negeZ7ge/NIEeARXNS8= +github.com/submariner-io/admiral v0.19.0-rc1.0.20241007115040-cd5bf4d54261 h1:b39I5740FgONLFymnRR36wtvjqXnwzC85A/Bwaw3dgw= +github.com/submariner-io/admiral v0.19.0-rc1.0.20241007115040-cd5bf4d54261/go.mod h1:2bmvHxnJ0piOMriKolIJ4/o5negeZ7ge/NIEeARXNS8= github.com/submariner-io/shipyard v0.19.0-rc1 h1:KaQnYlGLZjUFAepgpHW0pmM9J74sjrOl5A1hsdLtWFQ= github.com/submariner-io/shipyard v0.19.0-rc1/go.mod h1:HrnoErg/rM3r+QjUePY0x/SZi1xaeyjuQ+JTEc1bkMM= 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 a16bb460..6cf47660 100644 --- a/pkg/agent/controller/clusterip_service_test.go +++ b/pkg/agent/controller/clusterip_service_test.go @@ -198,6 +198,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("the session affinity is configured for an exported service", func() { BeforeEach(func() { t.cluster1.service.Spec.SessionAffinity = corev1.ServiceAffinityClientIP diff --git a/pkg/agent/controller/service_endpoint_slices.go b/pkg/agent/controller/service_endpoint_slices.go index 236e380b..e7e977f3 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" @@ -176,29 +178,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 @@ -346,12 +326,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