From 04864e9396847621751df8b2b74e5df04101c9e3 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Thu, 14 Nov 2024 12:07:04 -0500 Subject: [PATCH] Fix inadvertent deletion of aggregated ServiceImports on agent restart If the broker co-exists on a managed cluster, on LH agent restart, the aggregated ServiceImports on the broker are inadvertently deleted during reconciliation. Reconciliation should only process local aggregated ServiceImports and should ignore ServiceImports in the broker namespace. The latter are distinguished by the presence of the "multicluster.kubernetes.io/service-name" annotation. The reconciliation unit test was adjusted to cover this case. Fixes https://github.com/submariner-io/submariner/issues/3188 Signed-off-by: Tom Pantelis --- pkg/agent/controller/controller_suite_test.go | 13 ++++++++----- pkg/agent/controller/reconciliation_test.go | 13 +++++++------ pkg/agent/controller/service_import.go | 4 ++-- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/pkg/agent/controller/controller_suite_test.go b/pkg/agent/controller/controller_suite_test.go index 93bdbe65a..624e69eec 100644 --- a/pkg/agent/controller/controller_suite_test.go +++ b/pkg/agent/controller/controller_suite_test.go @@ -312,8 +312,8 @@ func newTestDiver() *testDriver { t.brokerEndpointSliceClient = t.syncerConfig.BrokerClient.Resource(*test.GetGroupVersionResourceFor(t.syncerConfig.RestMapper, &discovery.EndpointSlice{})).Namespace(test.RemoteNamespace) - t.cluster1.init(t.syncerConfig) - t.cluster2.init(t.syncerConfig) + t.cluster1.init(t.syncerConfig, nil) + t.cluster2.init(t.syncerConfig, nil) return t } @@ -327,15 +327,18 @@ func (t *testDriver) afterEach() { close(t.stopCh) } -func (c *cluster) init(syncerConfig *broker.SyncerConfig) { +func (c *cluster) init(syncerConfig *broker.SyncerConfig, dynClient *dynamicfake.FakeDynamicClient) { for k, v := range c.service.Labels { c.serviceEndpointSlices[0].Labels[k] = v } c.serviceIP = c.service.Spec.ClusterIP - c.localDynClient = dynamicfake.NewSimpleDynamicClient(syncerConfig.Scheme) - fake.AddBasicReactors(&c.localDynClient.Fake) + c.localDynClient = dynClient + if c.localDynClient == nil { + c.localDynClient = dynamicfake.NewSimpleDynamicClient(syncerConfig.Scheme) + fake.AddBasicReactors(&c.localDynClient.Fake) + } c.localServiceImportReactor = fake.NewFailingReactorForResource(&c.localDynClient.Fake, "serviceimports") diff --git a/pkg/agent/controller/reconciliation_test.go b/pkg/agent/controller/reconciliation_test.go index 5d30ae109..af58d5ec3 100644 --- a/pkg/agent/controller/reconciliation_test.go +++ b/pkg/agent/controller/reconciliation_test.go @@ -105,6 +105,11 @@ var _ = Describe("Reconciliation", func() { t.afterEach() t = newTestDiver() + brokerDynClient := t.syncerConfig.BrokerClient.(*fake.FakeDynamicClient) + + // Use the broker client for cluster1 to simulate the broker being on the same cluster. + t.cluster1.init(t.syncerConfig, brokerDynClient) + test.CreateResource(t.cluster1.localServiceImportClient.Namespace(test.LocalNamespace), localServiceImport) test.CreateResource(t.cluster1.localEndpointSliceClient, localEndpointSlice) test.CreateResource(t.cluster1.localServiceExportClient, serviceExport) @@ -117,12 +122,6 @@ var _ = Describe("Reconciliation", func() { t.cluster1.start(t, *t.syncerConfig) t.cluster2.start(t, *t.syncerConfig) - t.awaitNonHeadlessServiceExported(&t.cluster1) - - testutil.EnsureNoActionsForResource(&t.cluster1.localDynClient.Fake, "serviceimports", "delete") - testutil.EnsureNoActionsForResource(&t.cluster1.localDynClient.Fake, "endpointslices", "delete") - - brokerDynClient := t.syncerConfig.BrokerClient.(*fake.FakeDynamicClient) testutil.EnsureNoActionsForResource(&brokerDynClient.Fake, "endpointslices", "delete") // For migration cleanup, it may attempt to delete a local legacy ServiceImport from the broker so ignore it. @@ -137,6 +136,8 @@ var _ = Describe("Reconciliation", func() { return false }).Should(BeFalse()) + + t.awaitNonHeadlessServiceExported(&t.cluster1) }) }) diff --git a/pkg/agent/controller/service_import.go b/pkg/agent/controller/service_import.go index 2f1f11768..d31b499a9 100644 --- a/pkg/agent/controller/service_import.go +++ b/pkg/agent/controller/service_import.go @@ -197,8 +197,8 @@ func (c *ServiceImportController) reconcileLocalAggregatedServiceImports() { for i := range siList.Items { si := c.converter.toServiceImport(&siList.Items[i]) - if serviceImportSourceName(si) != "" { - // This is not an aggregated ServiceImport. + if serviceImportSourceName(si) != "" || si.Annotations[mcsv1a1.LabelServiceName] != "" { + // This is not a local aggregated ServiceImport. continue }