From ab32f522f39b730f3d1b616401fa3966c0c09d3d 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 | 4 +- go.sum | 8 +-- .../controller/clusterip_service_test.go | 35 ++++++++++ .../controller/service_endpoint_slices.go | 66 ++++++++++++------- 4 files changed, 82 insertions(+), 31 deletions(-) diff --git a/go.mod b/go.mod index 2e106d83..de8b9be6 100644 --- a/go.mod +++ b/go.mod @@ -7,8 +7,8 @@ require ( github.com/onsi/ginkgo/v2 v2.20.2 github.com/onsi/gomega v1.34.2 github.com/pkg/errors v0.9.1 - github.com/prometheus/client_golang v1.20.2 - github.com/submariner-io/admiral v0.19.0-m3 + github.com/prometheus/client_golang v1.20.4 + github.com/submariner-io/admiral v0.19.0-m3.0.20241007113622-538061402b91 github.com/submariner-io/shipyard v0.19.0-m3 k8s.io/api v0.31.0 k8s.io/apimachinery v0.31.0 diff --git a/go.sum b/go.sum index dcbbf152..42e2e4f6 100644 --- a/go.sum +++ b/go.sum @@ -328,8 +328,8 @@ github.com/pquerna/cachecontrol v0.0.0-20171018203845-0dec1b30a021/go.mod h1:prY github.com/prometheus/client_golang v0.9.1/go.mod h1:7SWBe2y4D6OKWSNQJUaRYU/AaXPKyh/dDVn+NZz0KFw= github.com/prometheus/client_golang v0.9.3/go.mod h1:/TN21ttK/J9q6uSwhBd54HahCDft0ttaMvbicHlPoso= github.com/prometheus/client_golang v1.0.0/go.mod h1:db9x61etRT2tGnBNRi70OPL5FsnadC4Ky3P0J6CfImo= -github.com/prometheus/client_golang v1.20.2 h1:5ctymQzZlyOON1666svgwn3s6IKWgfbjsejTMiXIyjg= -github.com/prometheus/client_golang v1.20.2/go.mod h1:PIEt8X02hGcP8JWbeHyeZ53Y/jReSnHgO035n//V5WE= +github.com/prometheus/client_golang v1.20.4 h1:Tgh3Yr67PaOv/uTqloMsCEdeuFTatm5zIq5+qNN23vI= +github.com/prometheus/client_golang v1.20.4/go.mod h1:PIEt8X02hGcP8JWbeHyeZ53Y/jReSnHgO035n//V5WE= github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo= github.com/prometheus/client_model v0.0.0-20190129233127-fd36f4220a90/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA= github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA= @@ -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-m3 h1:LTkYxCvB8S1210P2FZtCb6dzjaPpIgBrRQxZkH/snDo= -github.com/submariner-io/admiral v0.19.0-m3/go.mod h1:xRpP1rDOblEdPHr0qrC+plcTNfShYJAOH2fexqOmI1A= +github.com/submariner-io/admiral v0.19.0-m3.0.20241007113622-538061402b91 h1:DEYBbJPyE9xrokFDNoBe5heFPxsBiM4fi+xEnynUkg0= +github.com/submariner-io/admiral v0.19.0-m3.0.20241007113622-538061402b91/go.mod h1:Mm9PhtbIvai0Tl+8yzrItzoTJoWpvYKT2t6cIXLNvsU= github.com/submariner-io/shipyard v0.19.0-m3 h1:NliwAktRPF4OsLj1TDgpaOJD/bmmZW/FH9+mJmWgxbk= github.com/submariner-io/shipyard v0.19.0-m3/go.mod h1:BY1ceSnPz1/hN5F9uljcSzy5n5qgAOENsIvZpJ+XPOU= 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