fix: classify WAL restore errors with precise gRPC statuses

Map each restorer sentinel to the gRPC status that best reflects whether
the operator should retry:

  - ErrWALNotFound      -> NotFound        (terminal)
  - ErrInvalidWalName   -> InvalidArgument (terminal)
  - ErrConnectivity     -> Unavailable     (retry)
  - ErrGeneric          -> Unavailable     (retry)
  - anything else       -> Internal        (terminal)

Previously every non-NotFound failure was returned verbatim, leaving the
operator unable to tell transient blips apart from terminal conditions
like a malformed WAL name.

Signed-off-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>

Taskfile.yml

@@ -46,7 +46,7 @@ tasks:
       - wordlist-ordered
     env:
       # renovate: datasource=git-refs depName=spellcheck lookupName=https://github.com/cloudnative-pg/daggerverse currentValue=main
-      DAGGER_SPELLCHECK_SHA: 21755df1a27febff19b28dbe6ae57116f6088bb0
+      DAGGER_SPELLCHECK_SHA: 537e0ddae55ad0344af8f4454de7f27baeee48fa
     cmds:
       - >
         GITHUB_REF= dagger -s call -m github.com/cloudnative-pg/daggerverse/spellcheck@${DAGGER_SPELLCHECK_SHA}
@@ -60,7 +60,7 @@ tasks:
     desc: Check for conventional commits
     env:
       # renovate: datasource=git-refs depName=commitlint lookupName=https://github.com/cloudnative-pg/daggerverse currentValue=main
-      DAGGER_COMMITLINT_SHA: 21755df1a27febff19b28dbe6ae57116f6088bb0
+      DAGGER_COMMITLINT_SHA: 537e0ddae55ad0344af8f4454de7f27baeee48fa
     cmds:
       - >
         GITHUB_REF= dagger -s call -m github.com/cloudnative-pg/daggerverse/commitlint@${DAGGER_COMMITLINT_SHA}
@@ -74,7 +74,7 @@ tasks:
       - wordlist-ordered
     env:
       # renovate: datasource=git-refs depName=uncommitted lookupName=https://github.com/cloudnative-pg/daggerverse currentValue=main
-      DAGGER_UNCOMMITTED_SHA: 21755df1a27febff19b28dbe6ae57116f6088bb0
+      DAGGER_UNCOMMITTED_SHA: 537e0ddae55ad0344af8f4454de7f27baeee48fa
     cmds:
       - GITHUB_REF= dagger -s call -m github.com/cloudnative-pg/daggerverse/uncommitted@${DAGGER_UNCOMMITTED_SHA} check-uncommitted --source . stdout
     sources:
@@ -86,7 +86,7 @@ tasks:
       - controller-gen
     env:
       # renovate: datasource=git-refs depName=crd-gen-refs lookupName=https://github.com/cloudnative-pg/daggerverse currentValue=main
-      DAGGER_CRDGENREF_SHA: 21755df1a27febff19b28dbe6ae57116f6088bb0
+      DAGGER_CRDGENREF_SHA: 537e0ddae55ad0344af8f4454de7f27baeee48fa
       # renovate: datasource=go depName=github.com/elastic/crd-ref-docs
       CRDREFDOCS_VERSION: v0.3.0
     cmds:
@@ -381,7 +381,7 @@ tasks:
     run: once
     env:
       # renovate: datasource=git-refs depName=controller-gen lookupName=https://github.com/cloudnative-pg/daggerverse currentValue=main
-      DAGGER_CONTROLLER_GEN_SHA: 21755df1a27febff19b28dbe6ae57116f6088bb0
+      DAGGER_CONTROLLER_GEN_SHA: 537e0ddae55ad0344af8f4454de7f27baeee48fa
     cmds:
       - >
         GITHUB_REF= dagger -s call -m github.com/cloudnative-pg/daggerverse/controller-gen@${DAGGER_CONTROLLER_GEN_SHA}

config/crd/bases/barmancloud.cnpg.io_objectstores.yaml

@@ -176,25 +176,6 @@ spec:
                         format: int32
                         minimum: 1
                         type: integer
-                      restoreAdditionalCommandArgs:
-                        description: |-
-                          Additional arguments that can be appended to the 'barman-cloud-restore'
-                          command-line invocation. These arguments provide flexibility to customize
-                          the data restore process further, according to specific requirements or
-                          configurations.
-
-                          Example:
-                          In a scenario where specialized restore options are required, such as setting
-                          a specific read timeout or defining custom behavior, users can use this field
-                          to specify additional command arguments.
-
-                          Note:
-                          It's essential to ensure that the provided arguments are valid and supported
-                          by the 'barman-cloud-restore' command, to avoid potential errors or unintended
-                          behavior during execution.
-                        items:
-                          type: string
-                        type: array
                     type: object
                   destinationPath:
                     description: |-

containers/sidecar-requirements.in

@@ -1,3 +1,3 @@
-barman[azure,cloud,google,snappy,zstandard,lz4]==3.19.1
+barman[azure,cloud,google,snappy,zstandard,lz4]==3.18.0
 setuptools==82.0.1
 zipp>=3.19.1 # not directly required, pinned by Snyk to avoid a vulnerability

containers/sidecar-requirements.txt

@@ -18,9 +18,9 @@ azure-storage-blob==12.29.0 \
     --hash=sha256:2824ddd7ebc9056034ebc76b17971a38e9aa5835abb0d565b9700493f2a6c657 \
     --hash=sha256:ccf8a1bcd5e49df83ab85aab793b579e5ba2eeea2ad8900b2f62ca3a37dc391f
     # via barman
-barman==3.19.1 \
-    --hash=sha256:0a6a9e1babf97687732d8b2a3eb79ea95d55246a5257b9433865cb6e755221c0 \
-    --hash=sha256:2f71c4a1f1ba53f694cbdf838bb9906d8ba02b97d1fd3041196e8999bec7a1ee
+barman==3.18.0 \
+    --hash=sha256:8e752ac93d2f3a61e86b8374185209cae477a638ece7e6f540070f36d28d6997 \
+    --hash=sha256:ff90c44dafa4107b7574142771cdc2611c4cf1af818d93d3e67440a0c81164b9
     # via -r sidecar-requirements.in
 boto3==1.43.14 \
     --hash=sha256:574335744656cfed0b362a0a0467aaf2eb2bf15526edcd02d31d3c661f4b09e4 \
@@ -788,4 +788,6 @@ zstandard==0.25.0 \
 setuptools==82.0.1 \
     --hash=sha256:7d872682c5d01cfde07da7bccc7b65469d3dca203318515ada1de5eda35efbf9 \
     --hash=sha256:a59e362652f08dcd477c78bb6e7bd9d80a7995bc73ce773050228a348ce2e5bb
-    # via -r sidecar-requirements.in
+    # via
+    #   -r sidecar-requirements.in
+    #   barman

internal/cnpgi/common/errors.go

@@ -20,38 +20,10 @@ SPDX-License-Identifier: Apache-2.0
 package common
 
 import (
-	"errors"
-
-	barmanRestorer "github.com/cloudnative-pg/barman-cloud/pkg/restorer"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
 
-// classifyWALRestoreError maps an error returned by the WAL
-// restorer to a gRPC-coded error so the caller can tell terminal
-// failures apart from transient ones via the status code.
-func classifyWALRestoreError(walName string, walErr error) error {
-	switch {
-	case errors.Is(walErr, barmanRestorer.ErrWALNotFound):
-		return newWALNotFoundError(walName)
-	case errors.Is(walErr, barmanRestorer.ErrInvalidWalName):
-		// A malformed WAL name will never become valid on retry.
-		return newInvalidWALNameError(walName, walErr)
-	case errors.Is(walErr, barmanRestorer.ErrConnectivity),
-		errors.Is(walErr, barmanRestorer.ErrGeneric):
-		// barman-cloud exit codes 2 (connectivity) and 4
-		// (generic) both surface conditions that are retryable
-		// in practice — barman uses the "generic" bucket for
-		// some connection-class failures too, not just exit 2.
-		return newUnavailableError(walName, walErr)
-	default:
-		// Unrecognized exit codes and unexpected failures (e.g.
-		// the barman-cloud command could not be executed). No
-		// positive signal that retry would help.
-		return newInternalWALRestoreError(walName, walErr)
-	}
-}
-
 // ErrEndOfWALStreamReached is returned when end of WAL is detected in the cloud archive.
 var ErrEndOfWALStreamReached = status.Errorf(codes.OutOfRange, "end of WAL reached")
 
@@ -63,18 +35,19 @@ var ErrEndOfWALStreamReached = status.Errorf(codes.OutOfRange, "end of WAL reach
 var ErrMissingPermissions = status.Errorf(codes.FailedPrecondition,
 	"no permission to download the backup credentials, retrying")
 
-// newWALNotFoundError reports that the requested WAL file is not
-// present in the object store. Emits codes.NotFound: this is a
-// terminal condition (the file won't appear on retry).
+// newWALNotFoundError returns an error indicating that the
+// requested WAL file is not present in the object store.
+// It carries a gRPC NotFound status so the operator can
+// treat it as a terminal condition.
 func newWALNotFoundError(walName string) error {
 	return status.Errorf(codes.NotFound, "wal %q not found", walName)
 }
 
-// newUnavailableError reports that downloading the WAL file failed
-// for a reason expected to be transient (a barman-cloud connectivity
-// blip, or a generic exit code that in practice covers retryable
-// conditions too). Emits codes.Unavailable: per gRPC conventions,
-// the canonical signal for "retry may succeed".
+// newUnavailableError returns an error indicating that
+// downloading the given WAL file failed for a transient
+// reason (e.g. a connectivity blip). It carries a gRPC
+// Unavailable status so the operator will retry the
+// request.
 func newUnavailableError(walName string, err error) error {
 	return status.Errorf(
 		codes.Unavailable,
@@ -84,9 +57,10 @@ func newUnavailableError(walName string, err error) error {
 	)
 }
 
-// newInvalidWALNameError reports that the requested WAL name is
-// not a valid name. Emits codes.InvalidArgument: this is a
-// terminal condition (the same name won't become valid on retry).
+// newInvalidWALNameError returns an error indicating that
+// the requested WAL name is not valid. It carries a gRPC
+// InvalidArgument status so the operator treats it as a
+// terminal condition.
 func newInvalidWALNameError(walName string, err error) error {
 	return status.Errorf(
 		codes.InvalidArgument,
@@ -96,10 +70,10 @@ func newInvalidWALNameError(walName string, err error) error {
 	)
 }
 
-// newInternalWALRestoreError reports that downloading the WAL
-// file failed for an unclassified reason. Emits codes.Internal:
-// we have no positive signal that retry would help, so by gRPC
-// convention this is treated as terminal.
+// newInternalWALRestoreError returns an error indicating
+// that downloading the given WAL file failed for an
+// unclassified reason. It carries a gRPC Internal status
+// so the operator treats it as a terminal condition.
 func newInternalWALRestoreError(walName string, err error) error {
 	return status.Errorf(
 		codes.Internal,

internal/cnpgi/common/errors_test.go

@@ -1,86 +0,0 @@
-/*
-Copyright © contributors to CloudNativePG, established as
-CloudNativePG a Series of LF Projects, LLC.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-
-SPDX-License-Identifier: Apache-2.0
-*/
-
-package common
-
-import (
-	"errors"
-	"fmt"
-
-	barmanRestorer "github.com/cloudnative-pg/barman-cloud/pkg/restorer"
-	. "github.com/onsi/ginkgo/v2"
-	. "github.com/onsi/gomega"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
-)
-
-var _ = Describe("classifyWALRestoreError", func() {
-	const walName = "000000010000000000000001"
-
-	DescribeTable(
-		"maps barman restorer sentinels to gRPC status codes",
-		func(walErr error, expectedCode codes.Code) {
-			got := classifyWALRestoreError(walName, walErr)
-			Expect(got).To(HaveOccurred())
-
-			st, ok := status.FromError(got)
-			Expect(ok).To(BeTrue(), "returned error must carry a gRPC status")
-			Expect(st.Code()).To(Equal(expectedCode))
-			Expect(st.Message()).To(ContainSubstring(walName))
-		},
-		Entry("ErrWALNotFound -> NotFound",
-			fmt.Errorf("object storage or file not found: %w", barmanRestorer.ErrWALNotFound),
-			codes.NotFound),
-		Entry("ErrInvalidWalName -> InvalidArgument",
-			fmt.Errorf("invalid name for a WAL file: %w", barmanRestorer.ErrInvalidWalName),
-			codes.InvalidArgument),
-		Entry("ErrConnectivity -> Unavailable",
-			fmt.Errorf("connectivity failure, retrying: %w", barmanRestorer.ErrConnectivity),
-			codes.Unavailable),
-		Entry("ErrGeneric -> Unavailable (barman uses exit 4 for some retryable cases too)",
-			fmt.Errorf("generic error: %w", barmanRestorer.ErrGeneric),
-			codes.Unavailable),
-		Entry("unknown error -> Internal",
-			errors.New("something we did not classify"),
-			codes.Internal),
-	)
-
-	It("matches the sentinel even through several wrapping layers", func() {
-		// The plugin wraps barman errors via fmt.Errorf("...: %w", ...);
-		// classification must keep working if more wraps appear above.
-		inner := fmt.Errorf("connectivity failure, retrying: %w", barmanRestorer.ErrConnectivity)
-		wrapped := fmt.Errorf("while restoring WAL %s: %w", walName, inner)
-
-		got := classifyWALRestoreError(walName, wrapped)
-		st, ok := status.FromError(got)
-		Expect(ok).To(BeTrue())
-		Expect(st.Code()).To(Equal(codes.Unavailable))
-	})
-
-	It("treats ErrWALNotFound as terminal even when the error chain mentions other sentinels in its message", func() {
-		// Defensive: if the underlying error stringifies to something
-		// resembling another sentinel's message, the switch must still
-		// match by identity (errors.Is), not by substring.
-		walErr := fmt.Errorf("not found, looks like a connectivity failure: %w", barmanRestorer.ErrWALNotFound)
-
-		got := classifyWALRestoreError(walName, walErr)
-		st, _ := status.FromError(got)
-		Expect(st.Code()).To(Equal(codes.NotFound))
-	})
-})

internal/cnpgi/common/suite_test.go

@@ -1,32 +0,0 @@
-/*
-Copyright © contributors to CloudNativePG, established as
-CloudNativePG a Series of LF Projects, LLC.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-
-SPDX-License-Identifier: Apache-2.0
-*/
-
-package common
-
-import (
-	"testing"
-
-	. "github.com/onsi/ginkgo/v2"
-	. "github.com/onsi/gomega"
-)
-
-func TestCommon(t *testing.T) {
-	RegisterFailHandler(Fail)
-	RunSpecs(t, "Common Suite")
-}

internal/cnpgi/common/wal.go

@@ -365,7 +365,28 @@ func (w WALServiceImplementation) restoreFromBarmanObjectStore(
 	// is the one that PostgreSQL has requested to restore.
 	// The failure has already been logged in walRestorer.RestoreList method
 	if walStatus[0].Err != nil {
-		return classifyWALRestoreError(walStatus[0].WalName, walStatus[0].Err)
+		walName := walStatus[0].WalName
+		walErr := walStatus[0].Err
+		switch {
+		case errors.Is(walErr, barmanRestorer.ErrWALNotFound):
+			// A missing WAL is a terminal condition: surface it as a
+			// gRPC NotFound so the operator stops retrying.
+			return newWALNotFoundError(walName)
+		case errors.Is(walErr, barmanRestorer.ErrInvalidWalName):
+			// A malformed WAL name will never succeed on retry.
+			return newInvalidWALNameError(walName, walErr)
+		case errors.Is(walErr, barmanRestorer.ErrConnectivity),
+			errors.Is(walErr, barmanRestorer.ErrGeneric):
+			// Connectivity and generic barman-cloud failures are
+			// considered transient: surface them as gRPC Unavailable
+			// so the operator retries the download.
+			return newUnavailableError(walName, walErr)
+		default:
+			// Unrecognized exit codes and unexpected failures (e.g. the
+			// barman-cloud command could not be executed) are surfaced
+			// as gRPC Internal so the operator treats them as terminal.
+			return newInternalWALRestoreError(walName, walErr)
+		}
 	}
 
 	// We skip this step if streaming connection is not available

manifest.yaml

@@ -175,25 +175,6 @@ spec:
                         format: int32
                         minimum: 1
                         type: integer
-                      restoreAdditionalCommandArgs:
-                        description: |-
-                          Additional arguments that can be appended to the 'barman-cloud-restore'
-                          command-line invocation. These arguments provide flexibility to customize
-                          the data restore process further, according to specific requirements or
-                          configurations.
-
-                          Example:
-                          In a scenario where specialized restore options are required, such as setting
-                          a specific read timeout or defining custom behavior, users can use this field
-                          to specify additional command arguments.
-
-                          Note:
-                          It's essential to ensure that the provided arguments are valid and supported
-                          by the 'barman-cloud-restore' command, to avoid potential errors or unintended
-                          behavior during execution.
-                        items:
-                          type: string
-                        type: array
                     type: object
                   destinationPath:
                     description: |-