Merge pull request #1358 from YaoZengzeng/se

prevent deletion of addresses that are already occupied by other objects

Author: kmesh-bot

pkg/controller/workload/cache/service_cache.go

@@ -72,6 +72,18 @@ func (s *serviceCache) AddOrUpdateService(svc *workloadapi.Service) {
 func (s *serviceCache) DeleteService(resourceName string) {
 	s.mutex.Lock()
 	defer s.mutex.Unlock()
+
+	svc, ok := s.servicesByResourceName[resourceName]
+	if !ok {
+		return
+	}
+
+	for _, addr := range svc.GetAddresses() {
+		addrStr, _ := netip.AddrFromSlice(addr.GetAddress())
+		networkAddress := composeNetworkAddress(addr.GetNetwork(), addrStr)
+		s.deleteAddr(networkAddress, svc)
+	}
+
 	delete(s.servicesByResourceName, resourceName)
 }
 
@@ -91,3 +103,13 @@ func (s *serviceCache) GetService(resourceName string) *workloadapi.Service {
 	defer s.mutex.RUnlock()
 	return s.servicesByResourceName[resourceName]
 }
+
+func (s *serviceCache) deleteAddr(addr NetworkAddress, svc *workloadapi.Service) {
+	if service, ok := s.servicesByAddr[addr]; ok {
+		if service.GetNamespace() == svc.GetNamespace() && service.GetName() == svc.GetName() {
+			// NOTE: If the associated service is updated, we can no longer delete it.
+			// Ref: https://github.com/kmesh-net/kmesh/issues/1352
+			delete(s.servicesByAddr, addr)
+		}
+	}
+}

pkg/controller/workload/cache/service_cache_test.go

@@ -0,0 +1,79 @@
+/*
+ * Copyright The Kmesh Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package cache
+
+import (
+	"net/netip"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"kmesh.net/kmesh/api/v2/workloadapi"
+	"kmesh.net/kmesh/pkg/controller/workload/common"
+)
+
+func TestAddOrUpdateService(t *testing.T) {
+	cache := NewServiceCache()
+
+	svc1 := common.CreateFakeService("svc1", "10.240.10.1", "", nil)
+
+	cache.AddOrUpdateService(svc1)
+
+	name := svc1.ResourceName()
+
+	assert.Equal(t, svc1, cache.GetService(name))
+	assert.Equal(t, svc1, cache.GetServiceByAddr(NetworkAddress{Address: netip.MustParseAddr("10.240.10.1")}))
+}
+
+func TestDeleteService(t *testing.T) {
+	t.Run("normal delete", func(t *testing.T) {
+		cache := NewServiceCache()
+
+		svc1 := common.CreateFakeService("svc1", "10.240.10.1", "", nil)
+
+		cache.AddOrUpdateService(svc1)
+
+		name := svc1.ResourceName()
+		assert.Equal(t, svc1, cache.GetService(name))
+
+		cache.DeleteService(name)
+		assert.Equal(t, (*workloadapi.Service)(nil), cache.GetService(name))
+		assert.Equal(t, (*workloadapi.Service)(nil), cache.GetServiceByAddr(NetworkAddress{Address: netip.MustParseAddr("10.240.10.1")}))
+	})
+
+	t.Run("address override", func(t *testing.T) {
+		cache := NewServiceCache()
+
+		svc1 := common.CreateFakeService("svc1", "10.240.10.1", "", nil)
+		cache.AddOrUpdateService(svc1)
+
+		// Both svc1 and svc2 point to address "10.240.10.1"
+		svc2 := common.CreateFakeService("svc2", "10.240.10.1", "", nil)
+		cache.AddOrUpdateService(svc2)
+
+		name1 := svc1.ResourceName()
+		name2 := svc2.ResourceName()
+
+		assert.Equal(t, svc1, cache.GetService(name1))
+		assert.Equal(t, svc2, cache.GetService(name2))
+
+		// Delete svc1
+		cache.DeleteService(name1)
+		// Address "10.240.10.1" has been overwritten by svc2, so it will not be deleted
+		assert.Equal(t, svc2, cache.GetServiceByAddr(NetworkAddress{Address: netip.MustParseAddr("10.240.10.1")}))
+	})
+}

pkg/controller/workload/cache/workload_cache.go

@@ -98,7 +98,7 @@ func (w *cache) DeleteWorkload(uid string) {
 		for _, ip := range workload.Addresses {
 			addr, _ := netip.AddrFromSlice(ip)
 			networkAddress := composeNetworkAddress(workload.Network, addr)
-			delete(w.byAddr, networkAddress)
+			w.deleteAddr(networkAddress, uid)
 		}
 
 		delete(w.byUid, uid)
@@ -115,3 +115,13 @@ func (w *cache) List() []*workloadapi.Workload {
 
 	return out
 }
+
+func (w *cache) deleteAddr(addr NetworkAddress, uid string) {
+	if workload, ok := w.byAddr[addr]; ok {
+		if workload.Uid == uid {
+			// NOTE: If the uid is updated, it means that a new object occupies this address and we can no longer delete it.
+			// Ref: https://github.com/kmesh-net/kmesh/issues/1352
+			delete(w.byAddr, addr)
+		}
+	}
+}

pkg/controller/workload/cache/workload_cache_test.go

@@ -134,4 +134,32 @@ func TestDeleteWorkload(t *testing.T) {
 		assert.Equal(t, (*workloadapi.Workload)(nil), w.byAddr[NetworkAddress{Network: "ut-net", Address: addr1}])
 		assert.Equal(t, (*workloadapi.Workload)(nil), w.byAddr[NetworkAddress{Network: "ut-net", Address: addr2}])
 	})
+
+	t.Run("address override", func(t *testing.T) {
+		w := NewWorkloadCache()
+		Addresses := []string{
+			"192.168.224.22",
+			"1.2.3.4",
+		}
+		workload := common.CreateFakeWorkload("192.168.224.22", "", common.WithWorkloadBasicInfo("ut-workload", "123456", "ut-net"), common.WithAddresses(Addresses))
+		w.AddOrUpdateWorkload(workload)
+
+		// Both workload2 and workload point to address "192.168.224.22"
+		workload2 := common.CreateFakeWorkload("192.168.224.22", "", common.WithWorkloadBasicInfo("ut-workload-2", "abcdefg", "ut-net"))
+		w.AddOrUpdateWorkload(workload2)
+
+		assert.Equal(t, workload, w.byUid["123456"])
+		assert.Equal(t, workload2, w.byUid["abcdefg"])
+
+		w.DeleteWorkload("123456")
+		assert.Equal(t, (*workloadapi.Workload)(nil), w.byUid["123456"])
+
+		addr0 := netip.MustParseAddr(Addresses[0])
+		addr1 := netip.MustParseAddr(Addresses[1])
+
+		// Address "192.168.224.22" has been overwritten by workload2, so it will not be deleted
+		assert.Equal(t, workload2, w.byAddr[NetworkAddress{Network: "ut-net", Address: addr0}])
+		// Address "1.2.3.4" still points to workload, so it will be deleted.
+		assert.Equal(t, (*workloadapi.Workload)(nil), w.byAddr[NetworkAddress{Network: "ut-net", Address: addr1}])
+	})
 }