From da7191979f8bd3731ae3abb086cf3eed5e2ac12a Mon Sep 17 00:00:00 2001
From: Ville Aikas <11279988+vaikas@users.noreply.github.com>
Date: Tue, 23 Aug 2022 12:52:18 -0700
Subject: [PATCH] split warnings. Fix issue: 2581 (#2582)

* split warnings. Fix issue: 2581

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* Add tests for splits. Simplify.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
---
 webhook/admission.go                  |  13 +++
 webhook/admission_integration_test.go | 110 ++++++++++++++++++++++++++
 2 files changed, 123 insertions(+)

diff --git a/webhook/admission.go b/webhook/admission.go
index ea5518260..857987ad3 100644
--- a/webhook/admission.go
+++ b/webhook/admission.go
@@ -21,6 +21,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"strings"
 	"time"
 
 	"go.uber.org/zap"
@@ -124,6 +125,18 @@ func admissionHandler(rootLogger *zap.SugaredLogger, stats StatsReporter, c Admi
 		if !reviewResponse.Allowed || reviewResponse.PatchType != nil || response.Response == nil {
 			response.Response = reviewResponse
 		}
+
+		// If warnings contain newlines, which they will do by default if
+		// using Knative apis.FieldError, split them based on newlines
+		// and create a new warning. This is because any control characters
+		// in the warnings will cause the warning to be dropped silently.
+		if reviewResponse.Warnings != nil {
+			cleanedWarnings := make([]string, 0, len(reviewResponse.Warnings))
+			for _, w := range reviewResponse.Warnings {
+				cleanedWarnings = append(cleanedWarnings, strings.Split(w, "\n")...)
+			}
+			reviewResponse.Warnings = cleanedWarnings
+		}
 		response.Response.UID = review.Request.UID
 
 		logger = logger.With(
diff --git a/webhook/admission_integration_test.go b/webhook/admission_integration_test.go
index 66a73c483..8e143c918 100644
--- a/webhook/admission_integration_test.go
+++ b/webhook/admission_integration_test.go
@@ -437,3 +437,113 @@ func TestAdmissionInvalidResponseForResource(t *testing.T) {
 	// Stats should be reported for requests that have admission disallowed
 	metricstest.CheckStatsReported(t, requestCountName, requestLatenciesName)
 }
+
+func TestAdmissionWarningResponseForResource(t *testing.T) {
+	// Test that our single warning below (with newlines) should be turned into
+	// these three warnings
+	expectedWarnings := []string{"everything is not fine.", "like really", "for sure"}
+	ac := &fixedAdmissionController{
+		path:     "/warnmeplease",
+		response: &admissionv1.AdmissionResponse{Warnings: []string{"everything is not fine.\nlike really\nfor sure"}},
+	}
+	wh, serverURL, ctx, cancel, err := testSetup(t, ac)
+	if err != nil {
+		t.Fatal("testSetup() =", err)
+	}
+
+	eg, _ := errgroup.WithContext(ctx)
+	eg.Go(func() error { return wh.Run(ctx.Done()) })
+	wh.InformersHaveSynced()
+	defer func() {
+		cancel()
+		if err := eg.Wait(); err != nil {
+			t.Error("Unable to run controller:", err)
+		}
+	}()
+
+	pollErr := waitForServerAvailable(t, serverURL, testTimeout)
+	if pollErr != nil {
+		t.Fatal("waitForServerAvailable() =", err)
+	}
+	tlsClient, err := createSecureTLSClient(t, kubeclient.Get(ctx), &wh.Options)
+	if err != nil {
+		t.Fatal("createSecureTLSClient() =", err)
+	}
+
+	resource := createResource(testResourceName)
+
+	marshaled, err := json.Marshal(resource)
+	if err != nil {
+		t.Fatal("Failed to marshal resource:", err)
+	}
+
+	admissionreq := &admissionv1.AdmissionRequest{
+		Operation: admissionv1.Create,
+		Kind: metav1.GroupVersionKind{
+			Group:   "pkg.knative.dev",
+			Version: "v1alpha1",
+			Kind:    "Resource",
+		},
+		UserInfo: authenticationv1.UserInfo{
+			Username: user1,
+		},
+	}
+
+	admissionreq.Resource.Group = "pkg.knative.dev"
+	admissionreq.Object.Raw = marshaled
+
+	rev := &admissionv1.AdmissionReview{
+		Request: admissionreq,
+	}
+	reqBuf := new(bytes.Buffer)
+	err = json.NewEncoder(reqBuf).Encode(&rev)
+	if err != nil {
+		t.Fatal("Failed to marshal admission review:", err)
+	}
+
+	u, err := url.Parse("https://" + serverURL)
+	if err != nil {
+		t.Fatal("bad url", err)
+	}
+
+	u.Path = path.Join(u.Path, ac.Path())
+
+	req, err := http.NewRequest("GET", u.String(), reqBuf)
+	if err != nil {
+		t.Fatal("http.NewRequest() =", err)
+	}
+
+	req.Header.Add("Content-Type", "application/json")
+
+	response, err := tlsClient.Do(req)
+	if err != nil {
+		t.Fatal("Failed to receive response", err)
+	}
+
+	if got, want := response.StatusCode, http.StatusOK; got != want {
+		t.Errorf("Response status code = %v, wanted %v", got, want)
+	}
+
+	defer response.Body.Close()
+	respBody, err := ioutil.ReadAll(response.Body)
+	if err != nil {
+		t.Fatal("Failed to read response body", err)
+	}
+
+	reviewResponse := admissionv1.AdmissionReview{}
+
+	err = json.NewDecoder(bytes.NewReader(respBody)).Decode(&reviewResponse)
+	if err != nil {
+		t.Fatal("Failed to decode response:", err)
+	}
+
+	warnings := reviewResponse.Response.Warnings
+	if len(warnings) != 3 {
+		t.Errorf("Received unexpected warnings, wanted 3 got: %s", reviewResponse.Response.Warnings)
+	}
+	for i, w := range warnings {
+		if expectedWarnings[i] != w {
+			t.Errorf("Unexpected warning want %s got %s", expectedWarnings[i], w)
+		}
+	}
+}