From bcbeee7c09473b29d1c78123fc4b3cecd4109739 Mon Sep 17 00:00:00 2001
From: "W. Trevor King" <wking@tremily.us>
Date: Thu, 7 Dec 2023 15:27:23 -0800
Subject: [PATCH] pkg/cvo/sync_worker: Verification-failure details for
 unforced updates too

We've including verification-failure details for forced updates since
5ad3c1434f (pkg/cvo/updatepayload: Event when forcing through a
sig-verification failure, 2022-04-07, 2022, #763), but had not been
including them in logs or other output in the "we aren't forcing, so
this blocks the update's acceptance" case.  This commit adds the
detail to the Event, so it's available, but keeps only the high-level
message in the RetrievePayload status output (which feeds the
ReleaseAccepted condition in ClusterVersion), because while the
low-level are useful for debugging, they're pretty chatty for
condition consumers that are more interested in just knowing basically
why the update request isn't being accepted.

The newline-to-// replacement is because apparently Event messages
truncate at the first newline.  I have not tracked down docs or source
to back that up, but confirmed it in pre-merge testing [1].

[1]: https://github.com/openshift/cluster-version-operator/pull/1003#issuecomment-1847671077
---
 pkg/cvo/sync_worker.go | 40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go
index 6c0f6bbe3c..5f68e379d0 100644
--- a/pkg/cvo/sync_worker.go
+++ b/pkg/cvo/sync_worker.go
@@ -288,8 +288,9 @@ func (w *SyncWorker) syncPayload(ctx context.Context, work *SyncWork,
 		})
 		info, err := w.retriever.RetrievePayload(ctx, work.Desired)
 		if err != nil {
-			msg := fmt.Sprintf("Retrieving payload failed version=%q image=%q failure=%v", desired.Version, desired.Image, err)
+			msg := fmt.Sprintf("Retrieving payload failed version=%q image=%q failure=%s", desired.Version, desired.Image, strings.ReplaceAll(unwrappedErrorAggregate(err), "\n", " // "))
 			w.eventRecorder.Eventf(cvoObjectRef, corev1.EventTypeWarning, "RetrievePayloadFailed", msg)
+			msg = fmt.Sprintf("Retrieving payload failed version=%q image=%q failure=%s", desired.Version, desired.Image, err)
 			reporter.ReportPayload(LoadPayloadStatus{
 				Failure:            err,
 				Step:               "RetrievePayload",
@@ -302,21 +303,7 @@ func (w *SyncWorker) syncPayload(ctx context.Context, work *SyncWork,
 		}
 		acceptedRisksMsg := ""
 		if info.VerificationError != nil {
-			for err := info.VerificationError; err != nil; err = errors.Unwrap(err) {
-				details := err.Error()
-
-				// library-go/pkg/verify wraps the details, but does not include them
-				// in the top-level error string.  If we have an error like that,
-				// include the details here.
-
-				if acceptedRisksMsg == "" {
-					acceptedRisksMsg = details
-					continue
-				}
-				if !strings.Contains(acceptedRisksMsg, details) {
-					acceptedRisksMsg = fmt.Sprintf("%s\n%s", acceptedRisksMsg, details)
-				}
-			}
+			acceptedRisksMsg = unwrappedErrorAggregate(info.VerificationError)
 			w.eventRecorder.Eventf(cvoObjectRef, corev1.EventTypeWarning, "RetrievePayload", acceptedRisksMsg)
 		}
 
@@ -1300,3 +1287,24 @@ func runThrottledStatusNotifier(ctx context.Context, interval time.Duration, buc
 		}
 	}, 1*time.Second)
 }
+
+// unwrappedErrorAggregate recursively calls Unwrap on the input error
+// and generates a string discussing all the results it considers
+// This is helpful for getting details out of errors where one of the
+// higher-level wrappers decides to not include some of the
+// lower-level details, for use-cases where you want to dig back down
+// and get those lower-level details.
+func unwrappedErrorAggregate(err error) string {
+	msg := ""
+	for ; err != nil; err = errors.Unwrap(err) {
+		details := err.Error()
+		if msg == "" {
+			msg = details
+			continue
+		}
+		if !strings.Contains(msg, details) {
+			msg = fmt.Sprintf("%s\n%s", msg, details)
+		}
+	}
+	return msg
+}