swift: Retry connecting to OpenStack

The function IsSwiftEnabled probes a connection to Swift to determine
whether CIRO should fall back to using a persistent volume claim as a
storage backend. This function runs on the OpenStack platform to provide
an alternative solution for when Swift is not available.

Before this patch, any failure to communicate with Swift would make CIRO
fall back to using a PVC. However, a failure to probing Swift could come
from a temporary failure to reach the OpenStack API.

With this patch, when an issue with the connection to the OpenStack
identity service is not allowing CIRO to probe Swift, the probing
reports an error instead of directly falling back to Swift.

As a consequence, after this patch, temporary failures in early steps of
the connection to Swift do not make CIRO fall back to using persistent
volume claims when object storage might be available in OpenStack.

Author: pierreprinetti

pkg/storage/storage.go

@@ -202,7 +202,11 @@ func GetPlatformStorage(listers *regopclient.StorageListers) (imageregistryv1.Im
 		cfg.IBMCOS = &imageregistryv1.ImageRegistryConfigStorageIBMCOS{}
 		replicas = 2
 	case configapiv1.OpenStackPlatformType:
-		if swift.IsSwiftEnabled(listers) {
+		swiftEnabled, err := swift.IsSwiftEnabled(listers)
+		if err != nil {
+			return cfg, 0, err
+		}
+		if swiftEnabled {
 			cfg.Swift = &imageregistryv1.ImageRegistryConfigStorageSwift{}
 			replicas = 2
 			break

pkg/storage/swift/swift.go

@@ -3,6 +3,7 @@ package swift
 import (
 	"crypto/tls"
 	"crypto/x509"
+	"errors"
 	"fmt"
 	"net/http"
 	"reflect"
@@ -19,7 +20,7 @@ import (
 	yamlv2 "gopkg.in/yaml.v2"
 
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
+	apimachineryerrors "k8s.io/apimachinery/pkg/api/errors"
 	k8sutilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/klog/v2"
 
@@ -63,19 +64,26 @@ func replaceEmpty(a string, b string) string {
 }
 
 // IsSwiftEnabled checks if Swift service is available for OpenStack platform
-func IsSwiftEnabled(listers *regopclient.StorageListers) bool {
+func IsSwiftEnabled(listers *regopclient.StorageListers) (bool, error) {
 	driver := NewDriver(&imageregistryv1.ImageRegistryConfigStorageSwift{}, listers)
 	conn, err := driver.getSwiftClient()
 	if err != nil {
-		klog.Errorf("swift storage inaccessible: %v", err)
-		return false
+		if errors.As(err, &ErrContainerEndpointNotFound{}) {
+			klog.Errorf("error connecting to Swift: %v", err)
+			return false, nil
+		}
+		klog.Errorf("error connecting to OpenStack: %v", err)
+		return false, err
 	}
+
 	// Try to list containers to make sure the user has required permissions to do that
-	if _, err = containers.List(conn, containers.ListOpts{}).AllPages(); err != nil {
+	if err := containers.List(conn, containers.ListOpts{}).EachPage(func(_ pagination.Page) (bool, error) {
+		return false, nil
+	}); err != nil {
 		klog.Errorf("error listing swift containers: %v", err)
-		return false
+		return false, nil
 	}
-	return true
+	return true, nil
 }
 
 // GetConfig reads credentials
@@ -84,7 +92,7 @@ func GetConfig(listers *regopclient.StorageListers) (*Swift, error) {
 
 	// Look for a user defined secret to get the Swift credentials
 	sec, err := listers.Secrets.Get(defaults.ImageRegistryPrivateConfigurationUser)
-	if err != nil && errors.IsNotFound(err) {
+	if err != nil && apimachineryerrors.IsNotFound(err) {
 		// If no user defined credentials were provided, then try to find them in the secret,
 		// created by cloud-credential-operator.
 		sec, err = listers.Secrets.Get(defaults.CloudCredentialsName)
@@ -103,7 +111,7 @@ func GetConfig(listers *regopclient.StorageListers) (*Swift, error) {
 			var cloudName string
 			cloudInfra, err := util.GetInfrastructure(listers)
 			if err != nil {
-				if !errors.IsNotFound(err) {
+				if !apimachineryerrors.IsNotFound(err) {
 					return nil, fmt.Errorf("failed to get cluster infrastructure info: %v", err)
 				}
 			}
@@ -173,7 +181,7 @@ func GetConfig(listers *regopclient.StorageListers) (*Swift, error) {
 // system trust bundle should be used instead.
 func (d *driver) CABundle() (string, bool, error) {
 	cm, err := d.Listers.OpenShiftConfig.Get("cloud-provider-config")
-	if errors.IsNotFound(err) {
+	if apimachineryerrors.IsNotFound(err) {
 		return "", true, nil
 	}
 	if err != nil {
@@ -186,6 +194,15 @@ func (d *driver) CABundle() (string, bool, error) {
 	return caBundle, false, nil
 }
 
+type ErrContainerEndpointNotFound struct {
+	wrapped error
+}
+
+func (err ErrContainerEndpointNotFound) Unwrap() error { return err.wrapped }
+func (err ErrContainerEndpointNotFound) Error() string {
+	return fmt.Sprintf("container endpoint not found in the OpenStack catalog: %v", err.wrapped)
+}
+
 // getSwiftClient returns a client that allows to interact with the OpenStack Swift service
 func (d *driver) getSwiftClient() (*gophercloud.ServiceClient, error) {
 	cfg, err := GetConfig(d.Listers)
@@ -215,18 +232,18 @@ func (d *driver) getSwiftClient() (*gophercloud.ServiceClient, error) {
 
 	provider, err := openstack.NewClient(opts.IdentityEndpoint)
 	if err != nil {
-		return nil, fmt.Errorf("Create new provider client failed: %v", err)
+		return nil, fmt.Errorf("failed to create a new OpenStack provider client: %w", err)
 	}
 
 	cert, _, err := d.CABundle()
 	if err != nil {
-		return nil, fmt.Errorf("Failed to get cloud provider CA certificate: %v", err)
+		return nil, fmt.Errorf("failed to get cloud provider CA certificate: %w", err)
 	}
 
 	if cert != "" {
 		certPool, err := x509.SystemCertPool()
 		if err != nil {
-			return nil, fmt.Errorf("Create system cert pool failed: %v", err)
+			return nil, fmt.Errorf("failed to read the system cert pool: %w", err)
 		}
 		certPool.AppendCertsFromPEM([]byte(cert))
 		client := http.Client{
@@ -241,24 +258,42 @@ func (d *driver) getSwiftClient() (*gophercloud.ServiceClient, error) {
 
 	err = openstack.Authenticate(provider, *opts)
 	if err != nil {
-		return nil, fmt.Errorf("Failed to authenticate provider client: %v", err)
+		return nil, fmt.Errorf("failed to authenticate against OpenStack: %w", err)
 	}
 
 	endpointOpts := gophercloud.EndpointOpts{
 		Region: regionName,
 		Name:   "swift",
 	}
 
-	var client *gophercloud.ServiceClient
-	client, err = openstack.NewContainerV1(provider, endpointOpts)
-	if _, ok := err.(*gophercloud.ErrEndpointNotFound); ok {
-		endpointOpts.Type = "object-store"
-		client, err = openstack.NewContainerV1(provider, endpointOpts)
-		if err != nil {
-			return nil, err
+	client, err := openstack.NewContainerV1(provider, endpointOpts)
+	if err != nil {
+		if _, ok := err.(*gophercloud.ErrEndpointNotFound); ok {
+			// In gophercloud the default endpoint type for
+			// containers is "container". However, some OpenStack
+			// clouds are deployed with a single endpoint type for
+			// all Swift entities - "object-store".
+			//
+			// If a "container" endpoint is not found, then try
+			// "object-store".
+			endpointOpts.Type = "object-store"
+
+			var errOnAlternativeEndpoint error
+			client, errOnAlternativeEndpoint = openstack.NewContainerV1(provider, endpointOpts)
+			if errOnAlternativeEndpoint != nil {
+				if _, ok := errOnAlternativeEndpoint.(*gophercloud.ErrEndpointNotFound); ok {
+					// If none of the endpoints is found, then
+					// return the error we got on the default one
+					// so that we limit confusion on the error
+					// trace.
+					return nil, ErrContainerEndpointNotFound{err}
+				} else {
+					return nil, fmt.Errorf("failed to get the object storage alternative endpoint: %w", errOnAlternativeEndpoint)
+				}
+			}
+			return client, nil
 		}
-	} else if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to get the object storage endpoint: %w", err)
 	}
 
 	return client, nil

pkg/storage/swift/swift_test.go

@@ -2,6 +2,7 @@ package swift
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"reflect"
@@ -910,7 +911,9 @@ func TestSwiftIsAvailable(t *testing.T) {
 		Infrastructures: fakeInfrastructureLister(cloudName),
 		OpenShiftConfig: MockConfigMapNamespaceLister{},
 	}
-	th.AssertEquals(t, true, IsSwiftEnabled(listers))
+	isSwiftEnabled, err := IsSwiftEnabled(listers)
+	th.AssertNoErr(t, err)
+	th.AssertEquals(t, true, isSwiftEnabled)
 }
 
 func TestSwiftIsNotAvailable(t *testing.T) {
@@ -953,16 +956,21 @@ func TestSwiftIsNotAvailable(t *testing.T) {
 
 	_, err := d.getSwiftClient()
 	// if Swift endpoint is not registered, getSwiftClient should return *ErrEndpointNotFound
-	_, ok := err.(*gophercloud.ErrEndpointNotFound)
-	th.AssertEquals(t, true, ok)
+	gophercloudNotFound := new(gophercloud.ErrEndpointNotFound)
+	th.AssertEquals(t, true, errors.As(err, &gophercloudNotFound))
+
+	// ...which should be reported as the sentinel error type ErrContainerEndpointNotFound
+	th.AssertEquals(t, true, errors.As(err, &ErrContainerEndpointNotFound{}))
 
 	// IsSwiftEnabled should return false in this case
 	listers := &regopclient.StorageListers{
 		Secrets:         MockIPISecretNamespaceLister{},
 		Infrastructures: fakeInfrastructureLister(cloudName),
 		OpenShiftConfig: MockConfigMapNamespaceLister{},
 	}
-	th.AssertEquals(t, false, IsSwiftEnabled(listers))
+	isSwiftEnabled, err := IsSwiftEnabled(listers)
+	th.AssertNoErr(t, err)
+	th.AssertEquals(t, false, isSwiftEnabled)
 }
 
 func TestNoPermissionsKeystone(t *testing.T) {
@@ -1012,7 +1020,9 @@ func TestNoPermissionsKeystone(t *testing.T) {
 		Infrastructures: fakeInfrastructureLister(cloudName),
 		OpenShiftConfig: MockConfigMapNamespaceLister{},
 	}
-	th.AssertEquals(t, false, IsSwiftEnabled(listers))
+	isSwiftEnabled, err := IsSwiftEnabled(listers)
+	th.AssertNoErr(t, err)
+	th.AssertEquals(t, false, isSwiftEnabled)
 }
 
 func TestNoPermissionsSwauth(t *testing.T) {
@@ -1062,7 +1072,9 @@ func TestNoPermissionsSwauth(t *testing.T) {
 		Infrastructures: fakeInfrastructureLister(cloudName),
 		OpenShiftConfig: MockConfigMapNamespaceLister{},
 	}
-	th.AssertEquals(t, false, IsSwiftEnabled(listers))
+	isSwiftEnabled, err := IsSwiftEnabled(listers)
+	th.AssertNoErr(t, err)
+	th.AssertEquals(t, false, isSwiftEnabled)
 }
 
 func TestConfigStatusUpdate(t *testing.T) {