S3 config should not stomp local secret config

Gopkg.toml

@@ -51,7 +51,7 @@ required = [
 
 [[constraint]]
   name = "github.com/aws/aws-sdk-go"
-  version = "1.15.52"
+  version = "1.15.90"
 
 [[override]]
   name = "github.com/docker/distribution"

pkg/apis/imageregistry/v1alpha1/types.go

@@ -10,7 +10,9 @@ import (
 )
 
 const (
-	OperatorStatusTypeRemoved = "Removed"
+	OperatorStatusTypeRemoved             = "Removed"
+	ImageRegistryPrivateConfiguration     = "image-registry-private-configuration"
+	ImageRegistryPrivateConfigurationUser = ImageRegistryPrivateConfiguration + "-user"
 )
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
@@ -116,6 +118,19 @@ type ImageRegistryConfigStorageStatus struct {
 	State ImageRegistryConfigStorageState `json:"state"`
 }
 
+const (
+	// StorageExists denotes whether or not the registry storage medium exists
+	StorageExists = "StorageExists"
+
+	// StorageTagged denotes whether or not the registry storage medium
+	// that we created was tagged correctly
+	StorageTagged = "StorageTagged"
+
+	// StorageEncrypted denotes whether or not the registry storage medium
+	// that we created has encryption enabled
+	StorageEncrypted = "StorageEncrypted"
+)
+
 type ImageRegistryStatus struct {
 	operatorsv1alpha1api.OperatorStatus `json:",inline"`
 

pkg/clusterconfig/clusterconfig.go

@@ -5,10 +5,14 @@ import (
 	"strings"
 
 	installer "github.com/openshift/installer/pkg/types"
+
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/yaml"
+
 	coreset "k8s.io/client-go/kubernetes/typed/core/v1"
 
+	regopapi "github.com/openshift/cluster-image-registry-operator/pkg/apis/imageregistry/v1alpha1"
 	regopclient "github.com/openshift/cluster-image-registry-operator/pkg/client"
 )
 
@@ -25,6 +29,7 @@ const (
 	installerConfigNamespace = "kube-system"
 	installerConfigName      = "cluster-config-v1"
 	installerAWSCredsName    = "aws-creds"
+	operatorNamespace        = "openshift-image-registry"
 )
 
 type StorageType string
@@ -102,21 +107,44 @@ func GetAWSConfig() (*Config, error) {
 	if err != nil {
 		return nil, err
 	}
+
 	cfg.Storage.Type = StorageTypeS3
 	if installConfig.Platform.AWS != nil {
 		cfg.Storage.S3.Region = installConfig.Platform.AWS.Region
 	}
-	sec, err := client.Secrets(installerConfigNamespace).Get(installerAWSCredsName, metav1.GetOptions{})
-	if err != nil {
-		return nil, fmt.Errorf("unable to read aws-creds secret: %v", err)
-	}
 
-	cfg.Storage.Type = StorageTypeS3
-	if v, ok := sec.Data["aws_access_key_id"]; ok {
-		cfg.Storage.S3.AccessKey = string(v)
-	}
-	if v, ok := sec.Data["aws_secret_access_key"]; ok {
-		cfg.Storage.S3.SecretKey = string(v)
+	// Look for a user defined secret to get the AWS credentials from first
+	sec, err := client.Secrets(operatorNamespace).Get(regopapi.ImageRegistryPrivateConfigurationUser, metav1.GetOptions{})
+	if err != nil && errors.IsNotFound(err) {
+		// If no user defined secret is found, use the system one
+		sec, err = client.Secrets(installerConfigNamespace).Get(installerAWSCredsName, metav1.GetOptions{})
+		if err != nil {
+			return nil, fmt.Errorf("unable to get secret %q: %v", fmt.Sprintf("%s/%s", installerConfigNamespace, installerAWSCredsName), err)
+		}
+		if v, ok := sec.Data["aws_access_key_id"]; ok {
+			cfg.Storage.S3.AccessKey = string(v)
+		} else {
+			return nil, fmt.Errorf("Secret %q does not contain required key \"aws_access_key_id\"", fmt.Sprintf("%s/%s", installerConfigNamespace, installerAWSCredsName))
+		}
+		if v, ok := sec.Data["aws_secret_access_key"]; ok {
+			cfg.Storage.S3.SecretKey = string(v)
+		} else {
+			return nil, fmt.Errorf("Secret %q does not contain required key \"aws_secret_access_key\"", fmt.Sprintf("%s/%s", installerConfigNamespace, installerAWSCredsName))
+		}
+	} else if err != nil {
+		return nil, err
+	} else {
+		if v, ok := sec.Data["REGISTRY_STORAGE_S3_ACCESSKEY"]; ok {
+			cfg.Storage.S3.AccessKey = string(v)
+		} else {
+			return nil, fmt.Errorf("Secret %q does not contain required key \"REGISTRY_STORAGE_S3_ACCESSKEY\"", fmt.Sprintf("%s/%s", operatorNamespace, regopapi.ImageRegistryPrivateConfigurationUser))
+		}
+		if v, ok := sec.Data["REGISTRY_STORAGE_S3_SECRETKEY"]; ok {
+			cfg.Storage.S3.SecretKey = string(v)
+		} else {
+			return nil, fmt.Errorf("Secret %q does not contain required key \"REGISTRY_STORAGE_S3_SECRETKEY\"", fmt.Sprintf("%s/%s", operatorNamespace, regopapi.ImageRegistryPrivateConfigurationUser))
+
+		}
 	}
 
 	return cfg, nil

pkg/operator/bootstrap.go

@@ -35,7 +35,10 @@ func resourceName(namespace string) string {
 }
 
 func (c *Controller) Bootstrap() error {
-
+	client, err := regopset.NewForConfig(c.kubeconfig)
+	if err != nil {
+		return err
+	}
 	crList, err := c.listers.ImageRegistry.List(labels.Everything())
 	if err != nil {
 		if !errors.IsNotFound(err) {
@@ -122,22 +125,18 @@ func (c *Controller) Bootstrap() error {
 	}
 
 	driver, err := storage.NewDriver(cr.Name, c.params.Deployment.Namespace, &cr.Spec.Storage)
-	if err != nil {
-		if err != storage.ErrStorageNotConfigured {
-			return err
-		}
-	} else {
-		err = driver.CompleteConfiguration(&cr.Status)
-		if err != nil {
-			return err
-		}
+	if err != nil && err != storage.ErrStorageNotConfigured {
+		return err
 	}
 
-	client, err := regopset.NewForConfig(c.kubeconfig)
+	err = driver.CompleteConfiguration(cr)
+	_, cerr := client.ImageRegistries().Create(cr)
+	if cerr != nil {
+		return cerr
+	}
 	if err != nil {
 		return err
 	}
 
-	_, err = client.ImageRegistries().Create(cr)
-	return err
+	return nil
 }

pkg/operator/finalizer.go

@@ -16,6 +16,7 @@ import (
 
 	regopapi "github.com/openshift/cluster-image-registry-operator/pkg/apis/imageregistry/v1alpha1"
 	regopset "github.com/openshift/cluster-image-registry-operator/pkg/generated/clientset/versioned/typed/imageregistry/v1alpha1"
+	"github.com/openshift/cluster-image-registry-operator/pkg/storage"
 
 	"github.com/openshift/cluster-image-registry-operator/pkg/parameters"
 )
@@ -49,7 +50,21 @@ func (c *Controller) finalizeResources(o *regopapi.ImageRegistry) error {
 
 	glog.Infof("finalizing %s", objectInfo(o))
 
-	err := c.RemoveResources(o)
+	driver, err := storage.NewDriver(o.Name, o.Namespace, &o.Spec.Storage)
+	if err != nil {
+		return err
+	}
+
+	err = driver.RemoveStorage(o)
+	if err != nil {
+		errOp := c.clusterStatus.Update(osapi.OperatorFailing, osapi.ConditionTrue, "unable to remove storage")
+		if errOp != nil {
+			glog.Errorf("unable to update cluster status to %s=%s: %s", osapi.OperatorFailing, osapi.ConditionTrue, errOp)
+		}
+		return fmt.Errorf("unable to finalize resource: %s", err)
+	}
+
+	err = c.RemoveResources(o)
 	if err != nil {
 		errOp := c.clusterStatus.Update(osapi.OperatorFailing, osapi.ConditionTrue, "unable to remove registry")
 		if errOp != nil {

pkg/resource/generator.go

@@ -2,9 +2,10 @@ package resource
 
 import (
 	"fmt"
-
 	"github.com/golang/glog"
 
+	routeset "github.com/openshift/client-go/route/clientset/versioned/typed/route/v1"
+
 	"k8s.io/apimachinery/pkg/api/errors"
 	metaapi "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
@@ -15,11 +16,14 @@ import (
 	"k8s.io/client-go/util/retry"
 
 	configset "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
-	routeset "github.com/openshift/client-go/route/clientset/versioned/typed/route/v1"
-
 	regopapi "github.com/openshift/cluster-image-registry-operator/pkg/apis/imageregistry/v1alpha1"
 	"github.com/openshift/cluster-image-registry-operator/pkg/client"
+	"github.com/openshift/cluster-image-registry-operator/pkg/clusterconfig"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 	"github.com/openshift/cluster-image-registry-operator/pkg/parameters"
+	"github.com/openshift/cluster-image-registry-operator/pkg/storage"
+	"github.com/openshift/cluster-image-registry-operator/pkg/storage/util"
 )
 
 func NewGenerator(kubeconfig *rest.Config, listers *client.Listers, params *parameters.Globals) *Generator {
@@ -86,8 +90,95 @@ func (g *Generator) list(cr *regopapi.ImageRegistry) ([]Mutator, error) {
 	return mutators, nil
 }
 
+// syncStorage checks:
+// 1.)  to make sure that an existing storage medium still exists and we can access it
+// 2.)  to see if the storage medium name changed and we need to:
+//      a.) check to make sure that we can access the storage or
+//      b.) see if we need to try to create the new storage
+func (g *Generator) syncStorage(cr *regopapi.ImageRegistry, modified *bool) error {
+	var runCreate bool
+	*modified = true
+	// Create a driver with the current configuration
+	driver, err := storage.NewDriver(cr.Name, cr.Namespace, &cr.Spec.Storage)
+	if err != nil {
+		return err
+	}
+
+	// If the storage medium name is blank, we need to create it
+	name, err := driver.GetStorageName(cr)
+	if err != nil {
+		return err
+	}
+	if len(name) == 0 {
+		runCreate = true
+	} else if driver.StorageChanged(cr) {
+		exists, err := driver.StorageExists(cr)
+		if err != nil {
+			return err
+		}
+		if !exists {
+			runCreate = true
+		}
+	} else {
+		exists, err := driver.StorageExists(cr)
+		if err != nil {
+			return err
+		}
+		if !exists {
+			runCreate = true
+		}
+	}
+
+	if runCreate {
+		if err := driver.CreateStorage(cr); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+// syncSecrets checks to see if we have updated storage credentials from:
+// 1.) user provided credentials in the image-registry-private-configuration-user secret
+// and updates the image-registry-private-configuration secret which provides
+// those credentials to the registry pod
+func (g *Generator) syncSecrets(cr *regopapi.ImageRegistry, modified *bool) error {
+	client, err := clusterconfig.GetCoreClient()
+	if err != nil {
+		return err
+	}
+
+	// Get the existing image-registry-private-configuration secret
+	sec, err := client.Secrets(g.params.Deployment.Namespace).Get(regopapi.ImageRegistryPrivateConfiguration, metav1.GetOptions{})
+	if err != nil && !errors.IsNotFound(err) {
+		return fmt.Errorf("unable to get secret %q: %v", fmt.Sprintf("%s/%s", g.params.Deployment.Namespace, regopapi.ImageRegistryPrivateConfiguration), err)
+	}
+
+	// Create a driver with the current configuration
+	driver, err := storage.NewDriver(cr.Name, cr.Namespace, &cr.Spec.Storage)
+	if err != nil {
+		return err
+	}
+
+	data, err := driver.SyncSecrets(sec)
+	if err != nil {
+		return err
+	}
+	if data != nil {
+		glog.Infof("Updating secret %q with updated credentials.", fmt.Sprintf("%s/%s", g.params.Deployment.Namespace, regopapi.ImageRegistryPrivateConfiguration))
+
+		// Update the image-registry-private-configuration secret
+		_, err := util.CreateOrUpdateSecret(regopapi.ImageRegistryPrivateConfiguration, g.params.Deployment.Namespace, data)
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
 func (g *Generator) removeObsoleteRoutes(cr *regopapi.ImageRegistry, modified *bool) error {
 	routeClient, err := routeset.NewForConfig(g.kubeconfig)
+
 	if err != nil {
 		return err
 	}
@@ -168,6 +259,17 @@ func (g *Generator) Apply(cr *regopapi.ImageRegistry, modified *bool) error {
 		return fmt.Errorf("unable to remove obsolete routes: %s", err)
 	}
 
+	// Make sure that we always sync secrets before we sync storage
+	err = g.syncSecrets(cr, modified)
+	if err != nil {
+		return fmt.Errorf("unable to sync secrets: %s", err)
+	}
+
+	err = g.syncStorage(cr, modified)
+	if err != nil {
+		return fmt.Errorf("unable to sync storage configuration: %s", err)
+	}
+
 	return nil
 }