Improve error handling

To follow best practices for error and exception handling in Go and to prevent regressions,
err113 and gosec checks in golang-ci-linter are now enabled.
Errors now use API errors from k8s.io/apimachinery and predefined messages for status updates.

.golangci.yaml

@@ -7,5 +7,8 @@ linters:
     - ginkgolinter
     - gofmt
     - govet
+    - gosec
+    - errname
+    - err113
 run:
   timeout: 5m

api/v1beta1/conditions.go

@@ -56,6 +56,9 @@ const (
 
 // Common Messages used by API objects.
 const (
+	//NovaNetworkAttachmentsReadyInitMessage
+	NovaNetworkAttachmentsReadyErrorMessage = "NetworkAttachments error occurred not all pods have interfaces with ips as configured in NetworkAttachments: %s"
+
 	// NovaAPIReadyInitMessage
 	NovaAPIReadyInitMessage = "NovaAPI not started"
 
@@ -172,4 +175,7 @@ const (
 
 	//CellHostDiscoverErrorMessage
 	CellHostDiscoverErrorMessage = "CellHostDiscover error occurred %s"
+
+	//InputReadyErrorMessage
+	InputReadyErrorMessage = "Input data error occurred %s"
 )

controllers/common.go

@@ -95,7 +95,7 @@ const (
 
 	// fields to index to reconcile when change
 	passwordSecretField        = ".spec.secret"
-	caBundleSecretNameField    = ".spec.tls.caBundleSecretName"
+	caBundleSecretNameField    = ".spec.tls.caBundleSecretName" // #nosec G101
 	tlsAPIInternalField        = ".spec.tls.api.internal.secretName"
 	tlsAPIPublicField          = ".spec.tls.api.public.secretName"
 	tlsMetadataField           = ".spec.tls.secretName"
@@ -186,8 +186,6 @@ type conditionUpdater interface {
 	MarkTrue(t condition.Type, messageFormat string, messageArgs ...interface{})
 }
 
-// ensureSecret - ensures that the Secret object exists and the expected fields
-// are in the Secret. It returns a hash of the values of the expected fields.
 func ensureSecret(
 	ctx context.Context,
 	secretName types.NamespacedName,
@@ -225,7 +223,10 @@ func ensureSecret(
 	for _, field := range expectedFields {
 		val, ok := secret.Data[field]
 		if !ok {
-			err := fmt.Errorf("field '%s' not found in secret/%s", field, secretName.Name)
+			// Usage in the code
+			err := k8s_errors.NewBadRequest(fmt.Sprintf("field '%s' not found in secret/%s", field, secretName.Name))
+			// ensureSecret - ensures that the Secret object exists and the expected fields
+			// are in the Secret. It returns a hash of the values of the expected fields.
 			conditionUpdater.Set(condition.FalseCondition(
 				condition.InputReadyCondition,
 				condition.ErrorReason,
@@ -579,7 +580,7 @@ func getNovaClient(
 			return nil, err
 		}
 		if (ctrlResult != ctrl.Result{}) {
-			return nil, fmt.Errorf("the CABundleSecret %s not found", auth.GetCABundleSecretName())
+			return nil, fmt.Errorf("the CABundleSecret %s not found: %w", auth.GetCABundleSecretName(), err)
 		}
 
 		tlsConfig = &openstack.TLSConfig{
@@ -641,7 +642,7 @@ func ensureMemcached(
 				condition.RequestedReason,
 				condition.SeverityInfo,
 				condition.MemcachedReadyWaitingMessage))
-			return nil, fmt.Errorf("memcached %s not found", memcachedName)
+			return nil, fmt.Errorf("%w: memcached %s not found", err, memcachedName)
 		}
 		conditionUpdater.Set(condition.FalseCondition(
 			condition.MemcachedReadyCondition,
@@ -658,7 +659,7 @@ func ensureMemcached(
 			condition.RequestedReason,
 			condition.SeverityInfo,
 			condition.MemcachedReadyWaitingMessage))
-		return nil, fmt.Errorf("memcached %s is not ready", memcachedName)
+		return nil, fmt.Errorf("%w: memcached %s is not ready", ErrResourceIsNotReady, memcachedName)
 	}
 	conditionUpdater.MarkTrue(condition.MemcachedReadyCondition, condition.MemcachedReadyMessage)
 

controllers/errors.go

@@ -0,0 +1,12 @@
+package controllers
+
+import (
+	"errors"
+)
+
+var (
+	ErrResourceIsNotReady = errors.New("Resource is not ready")
+	ErrInvalidStatus      = errors.New("invalid status")
+	ErrCannotUpdateObject = errors.New("cannot update object")
+	ErrFieldNotFound      = errors.New("field not found in Secret")
+)

controllers/nova_controller.go

@@ -302,7 +302,7 @@ func (r *NovaReconciler) Reconcile(ctx context.Context, req ctrl.Request) (resul
 	case nova.DBCompleted:
 		instance.Status.Conditions.MarkTrue(novav1.NovaAPIDBReadyCondition, condition.DBReadyMessage)
 	default:
-		return ctrl.Result{}, fmt.Errorf("invalid DatabaseStatus from ensureAPIDB: %d", apiDBStatus)
+		return ctrl.Result{}, fmt.Errorf("%w from ensureAPIDB: %d", ErrInvalidStatus, apiDBStatus)
 	}
 
 	// We need to create a list of cellNames to iterate on and as the map
@@ -332,7 +332,7 @@ func (r *NovaReconciler) Reconcile(ctx context.Context, req ctrl.Request) (resul
 			creatingDBs = append(creatingDBs, cellName)
 		case nova.DBCompleted:
 		default:
-			return ctrl.Result{}, fmt.Errorf("invalid DatabaseStatus from ensureCellDB: %d for cell %s", status, cellName)
+			return ctrl.Result{}, fmt.Errorf("%w from ensureCellDB: %d for cell %s", ErrInvalidStatus, status, cellName)
 		}
 		cellDBs[cellName] = &nova.Database{Database: cellDB, Status: status}
 	}
@@ -380,7 +380,7 @@ func (r *NovaReconciler) Reconcile(ctx context.Context, req ctrl.Request) (resul
 		instance.Status.Conditions.MarkTrue(
 			novav1.NovaAPIMQReadyCondition, novav1.NovaAPIMQReadyMessage)
 	default:
-		return ctrl.Result{}, fmt.Errorf("invalid MessageBusStatus from  for the API MQ: %d", apiMQStatus)
+		return ctrl.Result{}, fmt.Errorf("%w from  for the API MQ: %d", ErrInvalidStatus, apiMQStatus)
 	}
 
 	cellMQs := map[string]*nova.MessageBus{}
@@ -408,7 +408,7 @@ func (r *NovaReconciler) Reconcile(ctx context.Context, req ctrl.Request) (resul
 			creatingMQs = append(creatingMQs, cellName)
 		case nova.MQCompleted:
 		default:
-			return ctrl.Result{}, fmt.Errorf("invalid MessageBusStatus from ensureMQ: %d for cell %s", status, cellName)
+			return ctrl.Result{}, fmt.Errorf("%w from ensureMQ: %d for cell %s", ErrInvalidStatus, status, cellName)
 		}
 		cellMQs[cellName] = &nova.MessageBus{TransportURL: cellTransportURL, Status: status}
 	}
@@ -1631,8 +1631,8 @@ func (r *NovaReconciler) ensureMQ(
 
 	url, ok := secret.Data[TransportURLSelector]
 	if !ok {
-		return "", nova.MQFailed, fmt.Errorf(
-			"the TransportURL secret %s does not have 'transport_url' field", transportURL.Status.SecretName)
+		return "", nova.MQFailed, k8s_errors.NewBadRequest(fmt.Sprintf(
+			"the TransportURL secret %s does not have 'transport_url' field", transportURL.Status.SecretName))
 	}
 
 	return string(url), nova.MQCompleted, nil
@@ -1673,7 +1673,7 @@ func (r *NovaReconciler) ensureMetadata(
 	// If it is not created by us, we don't touch it
 	if !k8s_errors.IsNotFound(err) && !OwnedBy(metadata, instance) {
 		err := fmt.Errorf(
-			"cannot update NovaMetadata/%s as the cell is not owning it", metadata.Name)
+			"%w NovaMetadata/%s as the cell is not owning it", ErrCannotUpdateObject, metadata.Name)
 		Log.Error(err,
 			"NovaMetadata is enabled, but there is a "+
 				"NovaMetadata CR not owned by us. We cannot update it. "+

controllers/novaapi_controller.go

@@ -588,13 +588,12 @@ func (r *NovaAPIReconciler) ensureDeployment(
 	if networkReady {
 		instance.Status.Conditions.MarkTrue(condition.NetworkAttachmentsReadyCondition, condition.NetworkAttachmentsReadyMessage)
 	} else {
-		err := fmt.Errorf("not all pods have interfaces with ips as configured in NetworkAttachments: %s", instance.Spec.NetworkAttachments)
 		instance.Status.Conditions.Set(condition.FalseCondition(
 			condition.NetworkAttachmentsReadyCondition,
 			condition.ErrorReason,
 			condition.SeverityWarning,
-			condition.NetworkAttachmentsReadyErrorMessage,
-			err.Error()))
+			novav1.NovaNetworkAttachmentsReadyErrorMessage,
+			instance.Spec.NetworkAttachments))
 
 		return ctrl.Result{}, err
 	}

controllers/novacell_controller.go

@@ -426,7 +426,7 @@ func (r *NovaCellReconciler) ensureNoVNCProxy(
 	// If it is not created by us, we don't touch it
 	if !k8s_errors.IsNotFound(err) && !OwnedBy(novncproxy, instance) {
 		err := fmt.Errorf(
-			"cannot update NovaNoVNCProxy/%s as the cell is not owning it", novncproxyName.Name)
+			"%w NovaNoVNCProxy/%s as the cell is not owning it", ErrCannotUpdateObject, novncproxyName.Name)
 		Log.Error(err,
 			"NovaNoVNCProxy is enabled in this cell, but there is a "+
 				"NovaNoVNCProxy CR not owned by the cell. We cannot update it. "+
@@ -548,7 +548,7 @@ func (r *NovaCellReconciler) ensureMetadata(
 	// If it is not created by us, we don't touch it
 	if !k8s_errors.IsNotFound(err) && !OwnedBy(metadata, instance) {
 		err := fmt.Errorf(
-			"cannot update NovaMetadata/%s as the cell is not owning it", metadata.Name)
+			"%w NovaMetadata/%s as the cell is not owning it", ErrCannotUpdateObject, metadata.Name)
 		Log.Error(err,
 			"NovaMetadata is enabled, but there is a "+
 				"NovaMetadata CR not owned by us. We cannot update it. "+
@@ -708,7 +708,7 @@ func (r *NovaCellReconciler) ensureNovaCompute(
 	// If it is not created by us, we don't touch it
 	if !k8s_errors.IsNotFound(err) && !OwnedBy(novacompute, instance) {
 		err := fmt.Errorf(
-			"cannot update NovaCompute/%s as the cell is not owning it", novacompute.Name)
+			"%w NovaCompute/%s as the cell is not owning it", ErrCannotUpdateObject, novacompute.Name)
 		Log.Error(err,
 			"NovaCompute is defined in the cell, but there is a "+
 				"NovaCompute CR not owned by us. We cannot update it. "+

controllers/novacompute_controller.go

@@ -403,13 +403,12 @@ func (r *NovaComputeReconciler) ensureDeployment(
 	if networkReady {
 		instance.Status.Conditions.MarkTrue(condition.NetworkAttachmentsReadyCondition, condition.NetworkAttachmentsReadyMessage)
 	} else {
-		err := fmt.Errorf("not all pods have interfaces with ips as configured in NetworkAttachments: %s", instance.Spec.NetworkAttachments)
 		instance.Status.Conditions.Set(condition.FalseCondition(
 			condition.NetworkAttachmentsReadyCondition,
 			condition.ErrorReason,
 			condition.SeverityWarning,
-			condition.NetworkAttachmentsReadyErrorMessage,
-			err.Error()))
+			novav1.NovaNetworkAttachmentsReadyErrorMessage,
+			instance.Spec.NetworkAttachments))
 
 		return ctrl.Result{}, err
 	}

controllers/novaconductor_controller.go

@@ -560,13 +560,12 @@ func (r *NovaConductorReconciler) ensureDeployment(
 	if networkReady {
 		instance.Status.Conditions.MarkTrue(condition.NetworkAttachmentsReadyCondition, condition.NetworkAttachmentsReadyMessage)
 	} else {
-		err := fmt.Errorf("not all pods have interfaces with ips as configured in NetworkAttachments: %s", instance.Spec.NetworkAttachments)
 		instance.Status.Conditions.Set(condition.FalseCondition(
 			condition.NetworkAttachmentsReadyCondition,
 			condition.ErrorReason,
 			condition.SeverityWarning,
-			condition.NetworkAttachmentsReadyErrorMessage,
-			err.Error()))
+			novav1.NovaNetworkAttachmentsReadyErrorMessage,
+			instance.Spec.NetworkAttachments))
 
 		return ctrl.Result{}, err
 	}

controllers/novametadata_controller.go

@@ -598,13 +598,12 @@ func (r *NovaMetadataReconciler) ensureDeployment(
 	if networkReady {
 		instance.Status.Conditions.MarkTrue(condition.NetworkAttachmentsReadyCondition, condition.NetworkAttachmentsReadyMessage)
 	} else {
-		err := fmt.Errorf("not all pods have interfaces with ips as configured in NetworkAttachments: %s", instance.Spec.NetworkAttachments)
 		instance.Status.Conditions.Set(condition.FalseCondition(
 			condition.NetworkAttachmentsReadyCondition,
 			condition.ErrorReason,
 			condition.SeverityWarning,
-			condition.NetworkAttachmentsReadyErrorMessage,
-			err.Error()))
+			novav1.NovaNetworkAttachmentsReadyErrorMessage,
+			instance.Spec.NetworkAttachments))
 
 		return ctrl.Result{}, err
 	}

controllers/novanovncproxy_controller.go

@@ -546,13 +546,12 @@ func (r *NovaNoVNCProxyReconciler) ensureDeployment(
 	if networkReady {
 		instance.Status.Conditions.MarkTrue(condition.NetworkAttachmentsReadyCondition, condition.NetworkAttachmentsReadyMessage)
 	} else {
-		err := fmt.Errorf("not all pods have interfaces with ips as configured in NetworkAttachments: %s", instance.Spec.NetworkAttachments)
 		instance.Status.Conditions.Set(condition.FalseCondition(
 			condition.NetworkAttachmentsReadyCondition,
 			condition.ErrorReason,
 			condition.SeverityWarning,
-			condition.NetworkAttachmentsReadyErrorMessage,
-			err.Error()))
+			novav1.NovaNetworkAttachmentsReadyErrorMessage,
+			instance.Spec.NetworkAttachments))
 
 		return ctrl.Result{}, err
 	}

controllers/novascheduler_controller.go

@@ -613,13 +613,12 @@ func (r *NovaSchedulerReconciler) ensureDeployment(
 	if networkReady {
 		instance.Status.Conditions.MarkTrue(condition.NetworkAttachmentsReadyCondition, condition.NetworkAttachmentsReadyMessage)
 	} else {
-		err := fmt.Errorf("not all pods have interfaces with ips as configured in NetworkAttachments: %s", instance.Spec.NetworkAttachments)
 		instance.Status.Conditions.Set(condition.FalseCondition(
 			condition.NetworkAttachmentsReadyCondition,
 			condition.ErrorReason,
 			condition.SeverityWarning,
-			condition.NetworkAttachmentsReadyErrorMessage,
-			err.Error()))
+			novav1.NovaNetworkAttachmentsReadyErrorMessage,
+			instance.Spec.NetworkAttachments))
 
 		return ctrl.Result{}, err
 	}

test/functional/novacell_controller_test.go

@@ -696,7 +696,7 @@ var _ = Describe("NovaCell controller", func() {
 				corev1.ConditionFalse,
 				condition.ErrorReason,
 				fmt.Sprintf(
-					"NovaNoVNCProxy error occurred cannot update "+
+					"NovaNoVNCProxy error occurred cannot update object "+
 						"NovaNoVNCProxy/%s as the cell is not owning it",
 					cell2.NoVNCProxyName.Name,
 				),
@@ -833,7 +833,7 @@ var _ = Describe("NovaCell controller", func() {
 				corev1.ConditionFalse,
 				condition.ErrorReason,
 				fmt.Sprintf(
-					"NovaMetadata error occurred cannot update "+
+					"NovaMetadata error occurred cannot update object "+
 						"NovaMetadata/%s as the cell is not owning it",
 					cell2.MetadataName.Name,
 				),

test/functional/suite_test.go

@@ -244,7 +244,7 @@ var _ = BeforeSuite(func() {
 	dialer := &net.Dialer{Timeout: time.Duration(10) * time.Second}
 	addrPort := fmt.Sprintf("%s:%d", webhookInstallOptions.LocalServingHost, webhookInstallOptions.LocalServingPort)
 	Eventually(func() error {
-		conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true})
+		conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true}) // #nosec G402
 		if err != nil {
 			return err
 		}