Merge pull request #819 from shiftstack/os_fix_swift_fallback

OCPBUGS-4090: swift: Retry connecting to OpenStack

Author: openshift-merge-robot

pkg/storage/storage.go

@@ -203,7 +203,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) {