When ServiceEntry are with the identical host name, try to `autoAlloc…

…ateIPs` same IP (istio#40165) (istio#636) (istio#640)

* Add integration tests for DNS AUTO ALLOCATION

Signed-off-by: Yann Liu <yannliu@redhat.com>

* Reuse the same ip for services with the identical hostname

Signed-off-by: Yann Liu <yannliu@redhat.com>

* Revert "Add integration tests for DNS AUTO ALLOCATION"

This reverts commit 61c49a5.

Signed-off-by: Yann Liu <yannliu@redhat.com>

* Update tests

Signed-off-by: Yann Liu <yannliu@redhat.com>

* Reuse set ip logic

Signed-off-by: Yann Liu <yannliu@redhat.com>

* Add a release note

Signed-off-by: Yann Liu <yannliu@redhat.com>

Signed-off-by: Yann Liu <yannliu@redhat.com>

Signed-off-by: Yann Liu <yannliu@redhat.com>
Co-authored-by: Yang Liu <yann.l.80s@gmail.com>

Signed-off-by: Yann Liu <yannliu@redhat.com>
Co-authored-by: Yang Liu <yann.l.80s@gmail.com>

pilot/pkg/serviceregistry/serviceentry/servicediscovery.go

@@ -48,6 +48,11 @@ type instancesKey struct {
 	namespace string
 }
 
+type octetPair struct {
+	thirdOctet  int
+	fourthOctet int
+}
+
 func makeInstanceKey(i *model.ServiceInstance) instancesKey {
 	return instancesKey{i.Service.Hostname, i.Service.Attributes.Namespace}
 }
@@ -919,30 +924,46 @@ func autoAllocateIPs(services []*model.Service) []*model.Service {
 	// So we bump X to 511, so that the resulting IP is 240.240.2.1
 	maxIPs := 255 * 255 // are we going to exceed this limit by processing 64K services?
 	x := 0
+	hostnameAllocatedOctets := make(map[string]octetPair)
+
 	for _, svc := range services {
 		// we can allocate IPs only if
 		// 1. the service has resolution set to static/dns. We cannot allocate
 		//   for NONE because we will not know the original DST IP that the application requested.
 		// 2. the address is not set (0.0.0.0)
 		// 3. the hostname is not a wildcard
-		if svc.DefaultAddress == constants.UnspecifiedIP && !svc.Hostname.IsWildCarded() &&
-			svc.Resolution != model.Passthrough {
-			x++
-			if x%255 == 0 {
+		if svc.DefaultAddress == constants.UnspecifiedIP && !svc.Hostname.IsWildCarded() && svc.Resolution != model.Passthrough {
+			hostname := svc.Hostname.String()
+			if allocatedOctets, ok := hostnameAllocatedOctets[hostname]; ok {
+				log.Debugf("Reuse IP for domain %s", hostname)
+				setAutoAllocatedIPs(svc, allocatedOctets)
+			} else {
 				x++
+				if x%255 == 0 {
+					x++
+				}
+				if x >= maxIPs {
+					log.Errorf("out of IPs to allocate for service entries")
+					return services
+				}
+				pair := octetPair{
+					thirdOctet:  x / 255,
+					fourthOctet: x % 255,
+				}
+				hostnameAllocatedOctets[hostname] = pair
+				setAutoAllocatedIPs(svc, pair)
 			}
-			if x >= maxIPs {
-				log.Errorf("out of IPs to allocate for service entries")
-				return services
-			}
-			thirdOctet := x / 255
-			fourthOctet := x % 255
-			svc.AutoAllocatedAddress = fmt.Sprintf("240.240.%d.%d", thirdOctet, fourthOctet)
 		}
 	}
 	return services
 }
 
+func setAutoAllocatedIPs(svc *model.Service, octets octetPair) {
+	a := octets.thirdOctet
+	b := octets.fourthOctet
+	svc.AutoAllocatedAddress = fmt.Sprintf("240.240.%d.%d", a, b)
+}
+
 func makeConfigKey(svc *model.Service) model.ConfigKey {
 	return model.ConfigKey{
 		Kind:      gvk.ServiceEntry,

pilot/pkg/serviceregistry/serviceentry/servicediscovery_test.go

@@ -1400,7 +1400,7 @@ func Test_autoAllocateIP_values(t *testing.T) {
 	inServices := make([]*model.Service, 512)
 	for i := 0; i < 512; i++ {
 		temp := model.Service{
-			Hostname:       "foo.com",
+			Hostname:       host.Name(fmt.Sprintf("foo%d.com", i)),
 			Resolution:     model.ClientSideLB,
 			DefaultAddress: constants.UnspecifiedIP,
 		}
@@ -1429,15 +1429,15 @@ func Test_autoAllocateIP_values(t *testing.T) {
 		t.Errorf("expected last IP address to be %s, got %s", expectedLastIP, gotServices[len(gotServices)-1].AutoAllocatedAddress)
 	}
 
-	gotIPMap := make(map[string]bool)
+	gotIPMap := make(map[string]string)
 	for _, svc := range gotServices {
 		if svc.AutoAllocatedAddress == "" || doNotWant[svc.AutoAllocatedAddress] {
 			t.Errorf("unexpected value for auto allocated IP address %s", svc.AutoAllocatedAddress)
 		}
-		if gotIPMap[svc.AutoAllocatedAddress] {
-			t.Errorf("multiple allocations of same IP address to different services: %s", svc.AutoAllocatedAddress)
+		if v, ok := gotIPMap[svc.AutoAllocatedAddress]; ok && v != svc.Hostname.String() {
+			t.Errorf("multiple allocations of same IP address to different services with different hostname: %s", svc.AutoAllocatedAddress)
 		}
-		gotIPMap[svc.AutoAllocatedAddress] = true
+		gotIPMap[svc.AutoAllocatedAddress] = svc.Hostname.String()
 	}
 }
 

releasenotes/notes/serviceentry-ip-auto-allocation.yaml

@@ -0,0 +1,8 @@
+apiVersion: release-notes/v2
+kind: bug-fix
+area: installation
+issue:
+- 40166
+releaseNotes:
+- |
+  **Fixed** an issue where adding a `ServiceEntry` could affect an existing `ServiceEntry` with the same hostname.