Use common generateName for storage

- Use a commont generateName function to generate storage name - Wrap the swift storage creation in a loop that checks for an existing container
openshift · Aug 12, 2019 · 34a535d · 34a535d
1 parent 4a57a26
commit 34a535d
Show file tree

Hide file tree

Showing 7 changed files with 250 additions and 58 deletions.
diff --git a/pkg/storage/azure/azure.go b/pkg/storage/azure/azure.go
@@ -18,7 +18,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/util/rand"
-	"k8s.io/apimachinery/pkg/util/uuid"
 	"k8s.io/client-go/rest"
 	"k8s.io/klog"
 
@@ -463,12 +462,14 @@ func (d *driver) CreateStorage(cr *imageregistryv1.Config) error {
 				util.UpdateCondition(cr, imageregistryv1.StorageExists, operatorapiv1.ConditionFalse, storageExistsReasonAzureError, fmt.Sprintf("Unable to get account primary key: %s", err))
 				return err
 			}
-
-			for i := 0; i < 5000; i++ {
+			const numRetries = 5000
+			for i := 0; i < numRetries; i++ {
 				// If the bucket name is blank, let's generate one
 				if len(d.Config.Container) == 0 {
 					// Container name must be between 3 and 63 characters long
-					d.Config.Container = fmt.Sprintf("%s-%s-%s", infra.Status.InfrastructureName, imageregistryv1.ImageRegistryName, strings.Replace(string(uuid.NewUUID()), "-", "", -1))[0:62]
+					if d.Config.Container, err = util.GenerateStorageName(d.Listers, ""); err != nil {
+						return err
+					}
 				}
 
 				err = d.createStorageContainer(d.Config.AccountName, key, d.Config.Container)

diff --git a/pkg/storage/gcs/gcs.go b/pkg/storage/gcs/gcs.go
@@ -5,11 +5,9 @@ import (
 	"fmt"
 	"reflect"
 	"strconv"
-	"strings"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/util/uuid"
 	"k8s.io/client-go/rest"
 
 	gstorage "cloud.google.com/go/storage"
@@ -204,11 +202,6 @@ func (d *driver) CreateStorage(cr *imageregistryv1.Config) error {
 		return err
 	}
 
-	infra, err := util.GetInfrastructure(d.Listers)
-	if err != nil {
-		return err
-	}
-
 	// If a bucket name is supplied, and it already exists and we can access it
 	// just update the config
 	var bucket *gstorage.BucketHandle
@@ -239,10 +232,13 @@ func (d *driver) CreateStorage(cr *imageregistryv1.Config) error {
 		util.UpdateCondition(cr, imageregistryv1.StorageExists, operatorapi.ConditionTrue, "GCS Bucket Exists", "User supplied GCS bucket exists and is accessible")
 
 	} else {
-		for i := 0; i < 5000; i++ {
+		const numRetries = 5000
+		for i := 0; i < numRetries; i++ {
 			// If the bucket name is blank, let's generate one
 			if len(d.Config.Bucket) == 0 {
-				d.Config.Bucket = fmt.Sprintf("%s-%s-%s-%s", infra.Status.InfrastructureName, imageregistryv1.ImageRegistryName, d.Config.Region, strings.Replace(string(uuid.NewUUID()), "-", "", -1))[0:62]
+				if d.Config.Bucket, err = util.GenerateStorageName(d.Listers, d.Config.Region); err != nil {
+					return err
+				}
 			}
 			bucketAttrs := gstorage.BucketAttrs{Location: d.Config.Region}
 			bucket = gclient.Bucket(d.Config.Bucket)

diff --git a/pkg/storage/s3/s3.go b/pkg/storage/s3/s3.go
@@ -6,7 +6,6 @@ import (
 	"net/http"
 	"net/url"
 	"reflect"
-	"strings"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
@@ -19,7 +18,6 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/util/uuid"
 	"k8s.io/client-go/rest"
 
 	configapiv1 "github.com/openshift/api/config/v1"
@@ -358,10 +356,13 @@ func (d *driver) CreateStorage(cr *imageregistryv1.Config) error {
 	} else {
 		generatedName := false
 		// Retry up to 5000 times if we get a naming conflict
-		for i := 0; i < 5000; i++ {
+		const numRetries = 5000
+		for i := 0; i < numRetries; i++ {
 			// If the bucket name is blank, let's generate one
 			if len(d.Config.Bucket) == 0 {
-				d.Config.Bucket = fmt.Sprintf("%s-%s-%s-%s", infra.Status.InfrastructureName, imageregistryv1.ImageRegistryName, d.Config.Region, strings.Replace(string(uuid.NewUUID()), "-", "", -1))[0:62]
+				if d.Config.Bucket, err = util.GenerateStorageName(d.Listers, d.Config.Region); err != nil {
+					return err
+				}
 				generatedName = true
 			}
 
@@ -441,6 +442,7 @@ func (d *driver) CreateStorage(cr *imageregistryv1.Config) error {
 		_, err := svc.PutBucketTaggingWithContext(d.Context, &s3.PutBucketTaggingInput{
 			Bucket: aws.String(d.Config.Bucket),
 			Tagging: &s3.Tagging{
+
 				TagSet: []*s3.Tag{
 					{
 						Key:   aws.String("kubernetes.io/cluster/" + infra.Status.InfrastructureName),

diff --git a/pkg/storage/swift/swift.go b/pkg/storage/swift/swift.go
@@ -2,7 +2,6 @@ package swift
 
 import (
 	"fmt"
-	"math/rand"
 	"reflect"
 	"strings"
 
@@ -302,16 +301,30 @@ func (d *driver) ensureAuthURLHasAPIVersion() error {
 	return nil
 }
 
+func (d *driver) containerExists(client *gophercloud.ServiceClient, containerName string) error {
+	if len(containerName) == 0 {
+		return nil
+	}
+	_, err := containers.Get(client, containerName, containers.GetOpts{}).Extract()
+
+	return err
+
+}
+
 func (d *driver) StorageExists(cr *imageregistryv1.Config) (bool, error) {
 	client, err := d.getSwiftClient(cr)
 	if err != nil {
 		util.UpdateCondition(cr, imageregistryv1.StorageExists, operatorapi.ConditionUnknown, "Could not connect to registry storage", err.Error())
 		return false, err
 	}
 
-	_, err = containers.Get(client, cr.Spec.Storage.Swift.Container, containers.GetOpts{}).Extract()
+	err = d.containerExists(client, cr.Spec.Storage.Swift.Container)
 	if err != nil {
-		util.UpdateCondition(cr, imageregistryv1.StorageExists, operatorapi.ConditionFalse, "Storage does not exist", err.Error())
+		if serr, ok := err.(*gophercloud.ErrResourceNotFound); ok {
+			util.UpdateCondition(cr, imageregistryv1.StorageExists, operatorapi.ConditionFalse, "Storage does not exist", serr.Error())
+			return false, nil
+		}
+		util.UpdateCondition(cr, imageregistryv1.StorageExists, operatorapi.ConditionUnknown, "Unknown error occurred", err.Error())
 		return false, err
 	}
 
@@ -328,14 +341,6 @@ func (d *driver) StorageChanged(cr *imageregistryv1.Config) bool {
 	return false
 }
 
-func generateContainerName(clusterID string) string {
-	bytes := make([]byte, 8)
-	for i := 0; i < 8; i++ {
-		bytes[i] = byte(65 + rand.Intn(25)) // A=65 and Z=65+25
-	}
-	return clusterID + "-image-registry-" + string(bytes)
-}
-
 func (d *driver) CreateStorage(cr *imageregistryv1.Config) error {
 	client, err := d.getSwiftClient(cr)
 	if err != nil {
@@ -348,33 +353,62 @@ func (d *driver) CreateStorage(cr *imageregistryv1.Config) error {
 		return fmt.Errorf("failed to get cluster infrastructure info: %v", err)
 	}
 
-	// Generate new container name if it wasn't provided.
-	// The name has cluster ID as a prefix plus "image-registry" suffix, which is complemented
-	// by 8 capital latin letters
-	// Example of a generated name: user-298xw-image-registry-FHEIBGDD
-	if cr.Spec.Storage.Swift.Container == "" {
-		cr.Spec.Storage.Swift.Container = generateContainerName(infra.Status.InfrastructureName)
-	}
+	generatedName := false
+	const numRetries = 5000
+	for i := 0; i < numRetries; i++ {
+		if len(cr.Spec.Storage.Swift.Container) == 0 {
+			if cr.Spec.Storage.Swift.Container, err = util.GenerateStorageName(d.Listers, ""); err != nil {
+				return err
+			}
+			generatedName = true
+		}
 
-	createOps := containers.CreateOpts{
-		Metadata: map[string]string{
-			"Openshiftclusterid": infra.Status.InfrastructureName,
-			"Name":               cr.Spec.Storage.Swift.Container,
-		},
-	}
+		err = d.containerExists(client, cr.Spec.Storage.Swift.Container)
+		if err != nil {
+			// If the error is not ErrResourceNotFound
+			// return the error
+			if _, ok := err.(*gophercloud.ErrResourceNotFound); !ok {
+				util.UpdateCondition(cr, imageregistryv1.StorageExists, operatorapi.ConditionFalse, "Unable to check if container exists", fmt.Sprintf("Error occurred checking if container exists: %v", err))
+				return err
+			}
+			// If the error is ErrResourceNotFound
+			// fall through to the container creation
+		}
+		// If we were supplied a container name and it exists
+		// we can skip the create
+		if !generatedName && err == nil {
+			util.UpdateCondition(cr, imageregistryv1.StorageExists, operatorapi.ConditionTrue, "Container exists", "User supplied container already exists")
+			break
+		}
+		// If we generated a container name and it exists
+		// let's try again
+		if generatedName && err == nil {
+			cr.Spec.Storage.Swift.Container = ""
+			continue
+		}
 
-	_, err = containers.Create(client, cr.Spec.Storage.Swift.Container, createOps).Extract()
-	if err != nil {
-		util.UpdateCondition(cr, imageregistryv1.StorageExists, operatorapi.ConditionFalse, "Creation Failed", err.Error())
-		cr.Status.StorageManaged = false
-		return err
-	}
+		createOps := containers.CreateOpts{
+			Metadata: map[string]string{
+				"Openshiftclusterid": infra.Status.InfrastructureName,
+				"Name":               cr.Spec.Storage.Swift.Container,
+			},
+		}
+
+		_, err = containers.Create(client, cr.Spec.Storage.Swift.Container, createOps).Extract()
+		if err != nil {
+			util.UpdateCondition(cr, imageregistryv1.StorageExists, operatorapi.ConditionFalse, "Creation Failed", err.Error())
+			cr.Status.StorageManaged = false
+			return err
+		}
 
-	util.UpdateCondition(cr, imageregistryv1.StorageExists, operatorapi.ConditionTrue, "Swift Container Created", "")
+		util.UpdateCondition(cr, imageregistryv1.StorageExists, operatorapi.ConditionTrue, "Swift Container Created", "")
 
-	cr.Status.StorageManaged = true
-	cr.Status.Storage.Swift = d.Config.DeepCopy()
-	cr.Spec.Storage.Swift = d.Config.DeepCopy()
+		cr.Status.StorageManaged = true
+		cr.Status.Storage.Swift = d.Config.DeepCopy()
+		cr.Spec.Storage.Swift = d.Config.DeepCopy()
+
+		break
+	}
 
 	return nil
 }

diff --git a/pkg/storage/swift/swift_test.go b/pkg/storage/swift/swift_test.go
@@ -212,15 +212,27 @@ func TestSwiftCreateStorageNativeSecret(t *testing.T) {
 	defer th.TeardownHTTP()
 	handleAuthentication(t, "container")
 
+	numRequests := 0
+
 	th.Mux.HandleFunc("/"+container, func(w http.ResponseWriter, r *http.Request) {
-		th.TestMethod(t, r, "PUT")
+		// The first request should be a head request
+		// to check if container with name exists
+		if numRequests == 0 {
+			th.TestMethod(t, r, "HEAD")
+			numRequests++
+		} else {
+			// Second request should be the actual create
+			th.TestMethod(t, r, "PUT")
+		}
+
 		th.TestHeader(t, r, "Accept", "application/json")
 
 		w.Header().Set("Content-Length", "0")
 		w.Header().Set("Content-Type", "text/html; charset=UTF-8")
 		w.Header().Set("Date", "Wed, 17 Aug 2016 19:25:43 GMT")
 		w.Header().Set("X-Trans-Id", "tx554ed59667a64c61866f1-0058b4ba37")
 		w.WriteHeader(http.StatusNoContent)
+
 	})
 
 	d, installConfig := mockConfig(false, th.Endpoint()+"v3", MockUPISecretNamespaceLister{})
@@ -353,8 +365,16 @@ func TestSwiftCreateStorageCloudConfig(t *testing.T) {
 		cloudSecretKey: fakeCloudsYAMLData,
 	}
 
+	numRequests := 0
+
 	th.Mux.HandleFunc("/"+container, func(w http.ResponseWriter, r *http.Request) {
-		th.TestMethod(t, r, "PUT")
+		if numRequests == 0 {
+			th.TestMethod(t, r, "HEAD")
+			numRequests++
+		} else {
+			th.TestMethod(t, r, "PUT")
+		}
+
 		th.TestHeader(t, r, "Accept", "application/json")
 
 		w.Header().Set("Content-Length", "0")
@@ -561,8 +581,15 @@ func TestSwiftEndpointTypeObjectStore(t *testing.T) {
 	defer th.TeardownHTTP()
 	handleAuthentication(t, "object-store")
 
+	numRequests := 0
+
 	th.Mux.HandleFunc("/"+container, func(w http.ResponseWriter, r *http.Request) {
-		th.TestMethod(t, r, "PUT")
+		if numRequests == 0 {
+			th.TestMethod(t, r, "HEAD")
+			numRequests++
+		} else {
+			th.TestMethod(t, r, "PUT")
+		}
 		th.TestHeader(t, r, "Accept", "application/json")
 
 		w.Header().Set("Content-Length", "0")
@@ -581,7 +608,3 @@ func TestSwiftEndpointTypeObjectStore(t *testing.T) {
 	th.AssertEquals(t, operatorapi.ConditionTrue, installConfig.Status.Conditions[0].Status)
 	th.AssertEquals(t, container, installConfig.Status.Storage.Swift.Container)
 }
-
-func TestSwiftGenerateContainerName(t *testing.T) {
-	th.AssertEquals(t, false, generateContainerName("image_registry_") == generateContainerName("image_registry_"))
-}
diff --git a/pkg/storage/util/util.go b/pkg/storage/util/util.go
@@ -2,6 +2,7 @@ package util
 
 import (
 	"fmt"
+	"math/rand"
 	"strings"
 
 	corev1 "k8s.io/api/core/v1"
@@ -104,3 +105,52 @@ func GetValueFromSecret(sec *corev1.Secret, key string) (string, error) {
 	}
 	return "", fmt.Errorf("secret %q does not contain required key %q", fmt.Sprintf("%s/%s", sec.Namespace, sec.Name), key)
 }
+
+// GenerateStorageName generates a unique name for the storage
+// medium that the registry will use
+func GenerateStorageName(listers *regopclient.Listers, additionalInfo ...string) (string, error) {
+
+	// Get the infrastructure name
+	infra, err := GetInfrastructure(listers)
+	if err != nil {
+		return "", err
+	}
+
+	// A slice to store the parts of our name
+	var parts []string
+
+	// Put the infrastructure name first
+	parts = append(parts, infra.Status.InfrastructureName)
+
+	// Image Registry Name second
+	parts = append(parts, imageregistryv1.ImageRegistryName)
+
+	// Additional information provided to the function third
+	for _, i := range additionalInfo {
+		if len(i) != 0 {
+			parts = append(parts, i)
+		}
+	}
+
+	// Join the slice together with dashes
+	name := strings.Join(parts, "-")
+
+	// Check the length and pad or truncate as needed
+	switch {
+	case len(name) < 62:
+		padding := 62 - len(name) - 1
+		bytes := make([]byte, padding)
+		for i := 0; i < padding; i++ {
+			bytes[i] = byte(97 + rand.Intn(25)) // a=97 and z=97+25
+		}
+		name = fmt.Sprintf("%s-%s", name, string(bytes))
+	case len(name) > 62:
+		name = name[0:62]
+		if strings.HasSuffix(name, "-") {
+			name = name[0:61] + string(byte(97+rand.Intn(25)))
+		}
+	}
+
+	return name, nil
+
+}