From 0de3ce0569f944688c684aba021ac2d5bb010167 Mon Sep 17 00:00:00 2001
From: Hidde Beydals <hidde@hhh.computer>
Date: Thu, 13 Feb 2025 11:43:03 +0800
Subject: [PATCH] fix: improve debug information `http` step

This adds the HTTP response code to the error message in case of
failures, and provides a `trace` level log message with the HTTP
response. Combined, this should make it easier to debug a variety
of error scenarios a user may run into when trying to write a `http`
directive.

Signed-off-by: Hidde Beydals <hidde@hhh.computer>
---
 internal/directives/http_requester.go      | 29 ++++++++++++++++++----
 internal/directives/http_requester_test.go |  4 +--
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/internal/directives/http_requester.go b/internal/directives/http_requester.go
index 1d9e5df2d..59bd173c5 100644
--- a/internal/directives/http_requester.go
+++ b/internal/directives/http_requester.go
@@ -5,7 +5,6 @@ import (
 	"context"
 	"crypto/tls"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -17,6 +16,7 @@ import (
 	"github.com/xeipuuv/gojsonschema"
 
 	kargoapi "github.com/akuity/kargo/api/v1alpha1"
+	"github.com/akuity/kargo/internal/logging"
 )
 
 const (
@@ -68,7 +68,7 @@ func (h *httpRequester) validate(cfg Config) error {
 }
 
 func (h *httpRequester) runPromotionStep(
-	_ context.Context,
+	ctx context.Context,
 	_ *PromotionStepContext,
 	cfg HTTPConfig,
 ) (PromotionStepResult, error) {
@@ -88,7 +88,7 @@ func (h *httpRequester) runPromotionStep(
 			fmt.Errorf("error sending HTTP request: %w", err)
 	}
 	defer resp.Body.Close()
-	env, err := h.buildExprEnv(resp)
+	env, err := h.buildExprEnv(ctx, resp)
 	if err != nil {
 		return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored},
 			fmt.Errorf("error building expression context from HTTP response: %w", err)
@@ -116,7 +116,10 @@ func (h *httpRequester) runPromotionStep(
 		}, nil
 	case failure:
 		return PromotionStepResult{Status: kargoapi.PromotionPhaseFailed},
-			&terminalError{err: errors.New("HTTP response met failure criteria")}
+			&terminalError{err: fmt.Errorf(
+				"HTTP (%d) response met failure criteria",
+				resp.StatusCode,
+			)}
 	default:
 		return PromotionStepResult{Status: kargoapi.PromotionPhaseRunning}, nil
 	}
@@ -165,7 +168,10 @@ func (h *httpRequester) getClient(cfg HTTPConfig) (*http.Client, error) {
 	}, nil
 }
 
-func (h *httpRequester) buildExprEnv(resp *http.Response) (map[string]any, error) {
+func (h *httpRequester) buildExprEnv(
+	ctx context.Context,
+	resp *http.Response,
+) (map[string]any, error) {
 	const maxBytes = 2 << 20
 
 	// Early check of Content-Length if available
@@ -194,6 +200,19 @@ func (h *httpRequester) buildExprEnv(resp *http.Response) (map[string]any, error
 			return nil, fmt.Errorf("response body exceeds maximum size of %d bytes", maxBytes)
 		}
 	}
+
+	// TODO(hidde): It has proven to be difficult to figure out why a HTTP step
+	// fails or is not working as expected. To remediate this, we log the
+	// response body and headers at trace level. This is a temporary solution
+	// until we have a better way to present this information to the user, e.g.
+	// as part of the step output or error message.
+	logging.LoggerFromContext(ctx).Trace(
+		"HTTP request response",
+		"status", resp.StatusCode,
+		"header", resp.Header,
+		"body", string(bodyBytes),
+	)
+
 	env := map[string]any{
 		"response": map[string]any{
 			// TODO(krancour): Casting as an int64 is a short-term fix here because
diff --git a/internal/directives/http_requester_test.go b/internal/directives/http_requester_test.go
index 68547fa9f..73590757e 100644
--- a/internal/directives/http_requester_test.go
+++ b/internal/directives/http_requester_test.go
@@ -333,7 +333,7 @@ func Test_httpRequester_runPromotionStep(t *testing.T) {
 				FailureExpression: "response.status == 404",
 			},
 			assertions: func(t *testing.T, res PromotionStepResult, err error) {
-				require.ErrorContains(t, err, "HTTP response met failure criteria")
+				require.ErrorContains(t, err, "HTTP (404) response met failure criteria")
 				require.True(t, isTerminal(err))
 				require.Equal(t, kargoapi.PromotionPhaseFailed, res.Status)
 			},
@@ -505,7 +505,7 @@ func Test_httpRequester_buildExprEnv(t *testing.T) {
 	h := &httpRequester{}
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			env, err := h.buildExprEnv(testCase.resp)
+			env, err := h.buildExprEnv(context.Background(), testCase.resp)
 			testCase.assertions(t, env, err)
 		})
 	}