From db5f4164769aab90edf984204309b3411dac40e9 Mon Sep 17 00:00:00 2001
From: Hamid Ghaf <83242695+hghaf099@users.noreply.github.com>
Date: Thu, 18 May 2023 05:07:54 -0700
Subject: [PATCH] report intermediate error messages during request forwarding
 (#20643)

* report intermediate error messages during request forwarding

* CL
---
 builtin/logical/pki/crl_util.go    |  2 +-
 builtin/logical/pki/path_revoke.go |  2 +-
 changelog/20643.txt                |  3 +++
 sdk/logical/response_util.go       | 11 +++++++++++
 vault/request_handling.go          | 25 ++++++++++++++++++++++++-
 5 files changed, 40 insertions(+), 3 deletions(-)
 create mode 100644 changelog/20643.txt

diff --git a/builtin/logical/pki/crl_util.go b/builtin/logical/pki/crl_util.go
index e17b2a006ab8..894e427f1011 100644
--- a/builtin/logical/pki/crl_util.go
+++ b/builtin/logical/pki/crl_util.go
@@ -754,7 +754,7 @@ func (cb *crlBuilder) processRevocationQueue(sc *storageContext) error {
 				}
 
 				if err := sc.Storage.Put(sc.Context, confirmedEntry); err != nil {
-					return fmt.Errorf("error persisting cross-cluster revocation confirmation: %w\nThis may occur when the active node of the primary performance replication cluster is unavailable.", err)
+					return fmt.Errorf("error persisting cross-cluster revocation confirmation: %w", err)
 				}
 			} else {
 				// Since we're the active node of the primary cluster, go ahead
diff --git a/builtin/logical/pki/path_revoke.go b/builtin/logical/pki/path_revoke.go
index fac46bc813fb..7aa23bc72305 100644
--- a/builtin/logical/pki/path_revoke.go
+++ b/builtin/logical/pki/path_revoke.go
@@ -519,7 +519,7 @@ func (b *backend) maybeRevokeCrossCluster(sc *storageContext, config *crlConfig,
 	}
 
 	if err := sc.Storage.Put(sc.Context, reqEntry); err != nil {
-		return nil, fmt.Errorf("error persisting cross-cluster revocation request: %w\nThis may occur when the active node of the primary performance replication cluster is unavailable.", err)
+		return nil, fmt.Errorf("error persisting cross-cluster revocation request: %w", err)
 	}
 
 	resp := &logical.Response{
diff --git a/changelog/20643.txt b/changelog/20643.txt
new file mode 100644
index 000000000000..340ec5b547ff
--- /dev/null
+++ b/changelog/20643.txt
@@ -0,0 +1,3 @@
+```release-note:improvement
+core: report intermediate error messages during request forwarding
+```
diff --git a/sdk/logical/response_util.go b/sdk/logical/response_util.go
index 6708871f421c..aef0213ed9e8 100644
--- a/sdk/logical/response_util.go
+++ b/sdk/logical/response_util.go
@@ -76,10 +76,21 @@ func RespondErrorCommon(req *Request, resp *Response, err error) (int, error) {
 		var allErrors error
 		var codedErr *ReplicationCodedError
 		errwrap.Walk(err, func(inErr error) {
+			// The Walk function does not just traverse leaves, and execute the
+			// callback function on the entire error first. So, if the error is
+			// of type multierror.Error, we may want to skip storing the entire
+			// error first to avoid adding duplicate errors when walking down
+			// the leaf errors
+			if _, ok := inErr.(*multierror.Error); ok {
+				return
+			}
 			newErr, ok := inErr.(*ReplicationCodedError)
 			if ok {
 				codedErr = newErr
 			} else {
+				// if the error is of type fmt.wrapError which is typically
+				// made by calling fmt.Errorf("... %w", err), allErrors will
+				// contain duplicated error messages
 				allErrors = multierror.Append(allErrors, inErr)
 			}
 		})
diff --git a/vault/request_handling.go b/vault/request_handling.go
index b3612298727f..a415dca416a5 100644
--- a/vault/request_handling.go
+++ b/vault/request_handling.go
@@ -833,7 +833,30 @@ func (c *Core) doRouting(ctx context.Context, req *logical.Request) (*logical.Re
 	// If we're replicating and we get a read-only error from a backend, need to forward to primary
 	resp, err := c.router.Route(ctx, req)
 	if shouldForward(c, resp, err) {
-		return forward(ctx, c, req)
+		fwdResp, fwdErr := forward(ctx, c, req)
+		if fwdErr != nil && err != logical.ErrReadOnly {
+			// When handling the request locally, we got an error that
+			// contained ErrReadOnly, but had additional information.
+			// Since we've now forwarded this request and got _another_
+			// error, we should tell the user about both errors, so
+			// they know about both.
+			//
+			// When there is no error from forwarding, the request
+			// succeeded and so no additional context is necessary. When
+			// the initial error here was only ErrReadOnly, it's likely
+			// the plugin authors intended to forward this request
+			// remotely anyway.
+			repErr, ok := fwdErr.(*logical.ReplicationCodedError)
+			if ok {
+				fwdErr = &logical.ReplicationCodedError{
+					Msg:  fmt.Sprintf("errors from both primary and secondary; primary error was %s; secondary errors follow: %s", repErr.Error(), err.Error()),
+					Code: repErr.Code,
+				}
+			} else {
+				fwdErr = multierror.Append(fwdErr, err)
+			}
+		}
+		return fwdResp, fwdErr
 	}
 	return resp, err
 }