Fix a bug in zoneToRegion which didn't work if multiple zones where…

… passed separated by `__` (#2731)

* Fix the bug in `zoneToRegion`

There was a bug in `zoneToRegion` function because of which the PV resources
were not being created with correct `nodeAffinity` set. `zoneToRegion` tried
to get regions from  passed zone just by removing `-[onechar]` from the zone.
For example if we passed the zone `us-west1-b` this function would return
`us-west1` by removing `-b`. This would work properly in the cases where just
one zone is given to the function.

But if more than one zones are given to the function (separated by `__`), this
function didn't work properly. Because for the input `us-west1-a__us-west2-a`
it returned `us-west1-a__us-west2` (using existing logic), which is incorrect.
As we can see the `-a` was not reomved from first zone. This commit doesn't
introduce the separator `__`, it was already part of code and zones were being
passed to this funciton separated by `__`.

This commit fixes above problem by making sure that we split the zones and then
remove `-char` from the zone and add the regions together again using `__` sep.

* Add a const for regions and zone separator

* Correct function name

pkg/blockstorage/blockstorage_test.go

@@ -28,6 +28,7 @@ import (
 	ktags "github.com/kanisterio/kanister/pkg/blockstorage/tags"
 	envconfig "github.com/kanisterio/kanister/pkg/config"
 	"github.com/kanisterio/kanister/pkg/field"
+	"github.com/kanisterio/kanister/pkg/kube/volume"
 	"github.com/kanisterio/kanister/pkg/log"
 )
 
@@ -306,7 +307,7 @@ func (s *BlockStorageProviderSuite) getConfig(c *C, region string) map[string]st
 }
 
 func (b *BlockStorageProviderSuite) isRegional(az string) bool {
-	return strings.Contains(az, "__")
+	return strings.Contains(az, volume.RegionZoneSeparator)
 }
 
 func (b *BlockStorageProviderSuite) TestFilterSnasphotWithTags(c *C) {

pkg/blockstorage/gcepd/gcepd.go

@@ -36,6 +36,7 @@ import (
 	"github.com/kanisterio/kanister/pkg/blockstorage/zone"
 	"github.com/kanisterio/kanister/pkg/field"
 	"github.com/kanisterio/kanister/pkg/kube"
+	"github.com/kanisterio/kanister/pkg/kube/volume"
 	"github.com/kanisterio/kanister/pkg/log"
 	"github.com/kanisterio/kanister/pkg/poll"
 )
@@ -409,7 +410,7 @@ func (s *GpdStorage) VolumeCreateFromSnapshot(ctx context.Context, snapshot bloc
 	if err != nil {
 		return nil, err
 	}
-	volZone := strings.Join(zones, "__")
+	volZone := strings.Join(zones, volume.RegionZoneSeparator)
 	// Validates new Zones
 	region, err = getRegionFromZones(volZone)
 	if err != nil {
@@ -627,7 +628,7 @@ func (s *GpdStorage) dynamicRegionToZoneMap(ctx context.Context) (map[string][]s
 }
 
 func isMultiZone(az string) bool {
-	return strings.Contains(az, "__")
+	return strings.Contains(az, volume.RegionZoneSeparator)
 }
 
 // getRegionFromZones function is used from the link below
@@ -666,5 +667,5 @@ func (s *GpdStorage) getSelfLinks(ctx context.Context, zones []string) ([]string
 }
 
 func splitZones(az string) []string {
-	return strings.Split(az, "__")
+	return strings.Split(az, volume.RegionZoneSeparator)
 }

pkg/kube/volume/volume.go

@@ -43,6 +43,8 @@ const (
 	// NoPVCNameSpecified is used by the caller to indicate that the PVC name
 	// should be auto-generated
 	NoPVCNameSpecified = ""
+
+	RegionZoneSeparator = "__"
 )
 
 // CreatePVC creates a PersistentVolumeClaim and returns its name
@@ -336,11 +338,29 @@ func labelSelector(labels map[string]string) string {
 	return strings.Join(ls, ",")
 }
 
-// zoneToRegion removes -latter or just last latter from provided zone.
+// zoneToRegion figures out region from a zone and to do that it
+// just removes `-[onchar]` from the end of zone.
 func zoneToRegion(zone string) string {
+	// zone can have multiple zone separate by `__` that's why first call
+	// zonesToRegions to get region for every zone and then return back
+	// by appending every region with `__` separator
+	return strings.Join(zonesToRegions(zone), RegionZoneSeparator)
+}
+
+func zonesToRegions(zone string) []string {
+	reg := map[string]struct{}{}
 	// TODO: gocritic rule below suggests to use regexp.MustCompile but it
 	// panics if regex cannot be compiled. We should add proper test before
 	// enabling this below so that no change to this regex results in a panic
 	r, _ := regexp.Compile("-?[a-z]$") //nolint:gocritic
-	return r.ReplaceAllString(zone, "")
+	for _, z := range strings.Split(zone, RegionZoneSeparator) {
+		zone = r.ReplaceAllString(z, "")
+		reg[zone] = struct{}{}
+	}
+
+	var regions []string
+	for k := range reg {
+		regions = append(regions, k)
+	}
+	return regions
 }

pkg/kube/volume/volume_test.go

@@ -201,3 +201,63 @@ func (s *TestVolSuite) fakeUnstructuredSnasphotWSize(vsName, namespace, size str
 	}
 	return &unstructured.Unstructured{Object: Object}
 }
+
+func (s *TestVolSuite) TestZoneToRegion(c *C) {
+	for _, tc := range []struct {
+		zone           string
+		expectedRegion []string
+	}{
+		{
+			zone:           "us-west1-b",
+			expectedRegion: []string{"us-west1"},
+		},
+		{
+			zone:           "us-west1-a",
+			expectedRegion: []string{"us-west1"},
+		},
+		{
+			zone:           "us-west2-c",
+			expectedRegion: []string{"us-west2"},
+		},
+		{
+			zone:           "us-west1-a__us-west2-b",
+			expectedRegion: []string{"us-west1", "us-west2"},
+		},
+		{
+			zone:           "us-west1-a__us-west2-b__us-west2-c",
+			expectedRegion: []string{"us-west1", "us-west2"},
+		},
+		{
+			zone:           "us-west1-a__us-west1-b__us-west2-b__us-west2-c",
+			expectedRegion: []string{"us-west1", "us-west2"},
+		},
+		{
+			zone:           "us-west1-a__us-west1-b__us-west2-b__us-west2-c__us-west2-d",
+			expectedRegion: []string{"us-west1", "us-west2"},
+		},
+	} {
+		reg := zonesToRegions(tc.zone)
+		c.Assert(slicesEqual(reg, tc.expectedRegion), Equals, true)
+	}
+}
+
+// slicesEqual compares two unordered slices and returns true if
+// both of them have same elements
+func slicesEqual(one, two []string) bool {
+	if len(one) != len(two) {
+		return false
+	}
+
+	for _, o := range one {
+		var found bool
+		for _, t := range two {
+			if o == t {
+				found = true
+			}
+		}
+		if !found {
+			return false
+		}
+	}
+	return true
+}