Repository: knative/pkg (go)

Issue:
Warnings are silently dropped by k8s due to newlines.
/kind bug

## Expected Behavior
When creating a warning like this:

```
warnField := apis.ErrGeneric(fmt.Sprintf("failed policy: %s", failingPolicy), "image").ViaFieldIndex(field, i)
warnField.Details = warnDetails
errs = errs.Also(warnField).At(apis.WarningLevel)
```
I would expect this to work. However, this turns into something like this:
```
{
	"kind": "AdmissionReview",
	"apiVersion": "admission.k8s.io/v1",
	"response": {
		"uid": "f3ebec5c-6302-4efb-9ef8-49215353a566",
		"allowed": true,
		"warnings": ["failed policy: image-policy-keyless: spec.containers[0].image\nregistry.local:5000/sigstore/demo@sha256:a420f0e3a1f8c20c6be3caaca3d6601139806b7dc5d7e4256a912675cfd319c6 signature keyless validation failed for authority authority-0 for registry.local:5000/sigstore/demo@sha256:a420f0e3a1f8c20c6be3caaca3d6601139806b7dc5d7e4256a912675cfd319c6: Get \"https://registry.local:5000/v2/\": dial tcp: lookup registry.local on 10.12.0.10:53: no such host; Get \"http://registry.local:5000/v2/\": dial tcp: lookup registry.local on 10.12.0.10:53: no such host"]
	}
}
```
Note the `\n` that we use to construct the error message by joining on `\n`

I'd argue this is not very well spelled out here what's allowed and not allowed:
https://github.com/kubernetes/api/blob/master/admission/v1/types.go#L144

But, digging deeper, the the problem is that k8s silently ignores this:
https://github.com/kubernetes/apiserver/blob/b10345148a460f9ed3dc576871a547953d021adf/pkg/endpoints/filters/warning_test.go#L83-L85

I would expect warnings to get created by our pkg to get wired through to `kubectl` properly.

## Actual Behavior
The warning message gets dropped on the floor, and nothing is passed to `kubectl`.

## Steps to Reproduce the Problem

1.  Create the warning like above that includes `details`, or anything that causes a `join` to happen.
2. 
3.

## Additional Info
Thanks @mattmoor for second set of eyes for digging into this :) 

Pull request description:
split warnings. Fix issue: 2581
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

<!-- Thanks for sending a pull request! -->

# Changes
🐛 Fix #2581 

If warnings contain newlines, split on them, and create a new warning for each of them.
<!--
In addition, categorize the changes you're making using the "/kind" Prow command, example:

/kind <kind>

Supported kinds are: api-change, bug, cleanup, deprecation, removal, documentation, enhancement, performance

-->
/kind <kind>

<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

<!-- Please include the 'why' behind your changes if no issue exists -->

**Release Note**

<!--
:page_facing_up: If this change has user-visible impact, write a release note in the block
below. Include the string "action required" if additional action is required of
users switching to the new release, for example in case of a breaking change.

Write as if you are speaking to users, not other Knative contributors. If this
change has no user-visible impact, no release-note is needed.
-->
```release-note
* Fixes a bug (#2581) where (most if not all) warnings were silently ignored by k8s apiserver due to having newlines on them.
```

**Docs**

<!--
:book: If this change has user-visible impact, link to an issue or PR in
https://github.com/knative/docs.

See here for guidance on getting permanent links to files: https://help.github.com/en/articles/getting-permanent-links-to-files

Please use the following format for linking documentation:
- [knative/docs]: <issue or pr link>
- [Feature Track]: <link>
- [Usage]: <link>
- [Other doc]: <link>
-->
```docs

```


Patch:
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(


Test patch:
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)
+		}
+	}
+}