Merge pull request #92667 from liggitt/admission-warnings

Admission webhook warnings

Kubernetes-commit: f7a13de36c4584464adc991c7a3d1f38f610232e

Godeps/Godeps.json

@@ -668,7 +668,7 @@
 		},
 		{
 			"ImportPath": "k8s.io/api",
-			"Rev": "a0ea971d15ac"
+			"Rev": "34238374cd52"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery",

go.mod

@@ -41,7 +41,7 @@ require (
 	gopkg.in/natefinch/lumberjack.v2 v2.0.0
 	gopkg.in/square/go-jose.v2 v2.2.2
 	gopkg.in/yaml.v2 v2.2.8
-	k8s.io/api v0.0.0-20200702090435-a0ea971d15ac
+	k8s.io/api v0.0.0-20200702090436-34238374cd52
 	k8s.io/apimachinery v0.0.0-20200702090251-3c2682fedbf2
 	k8s.io/client-go v0.0.0-20200702090712-ae79ca62dbf0
 	k8s.io/component-base v0.0.0-20200702091254-df61fd08516e
@@ -54,7 +54,7 @@ require (
 )
 
 replace (
-	k8s.io/api => k8s.io/api v0.0.0-20200702090435-a0ea971d15ac
+	k8s.io/api => k8s.io/api v0.0.0-20200702090436-34238374cd52
 	k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20200702090251-3c2682fedbf2
 	k8s.io/client-go => k8s.io/client-go v0.0.0-20200702090712-ae79ca62dbf0
 	k8s.io/component-base => k8s.io/component-base v0.0.0-20200702091254-df61fd08516e

go.sum

@@ -508,7 +508,7 @@ honnef.co/go/tools v0.0.0-20190106161140-3f1c8253044a/go.mod h1:rf3lG4BRIbNafJWh
 honnef.co/go/tools v0.0.0-20190418001031-e561f6794a2a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
 honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
 honnef.co/go/tools v0.0.1-2019.2.3/go.mod h1:a3bituU0lyd329TUQxRnasdCoJDkEUEAqEt0JzvZhAg=
-k8s.io/api v0.0.0-20200702090435-a0ea971d15ac/go.mod h1:PFJ2b4cud2iXU+DzN8Tf+HsJsN2KKcJUfbMmckp2bhI=
+k8s.io/api v0.0.0-20200702090436-34238374cd52/go.mod h1:PFJ2b4cud2iXU+DzN8Tf+HsJsN2KKcJUfbMmckp2bhI=
 k8s.io/apimachinery v0.0.0-20200702090251-3c2682fedbf2/go.mod h1:87+lSRzC+mGLZTTwIcPY67sorIVlqPhdNrvsToaUUns=
 k8s.io/client-go v0.0.0-20200702090712-ae79ca62dbf0/go.mod h1:FOr+6mK5IOJNdqcwn6f5OX41VN/TjEojjQBPz3HonwQ=
 k8s.io/component-base v0.0.0-20200702091254-df61fd08516e/go.mod h1:A8//EOJst1cT38h2/Xp3XNcXMevkmQZmnxWRCJIfgOQ=

pkg/admission/plugin/webhook/mutating/dispatcher.go

@@ -43,6 +43,7 @@ import (
 	webhookrequest "k8s.io/apiserver/pkg/admission/plugin/webhook/request"
 	auditinternal "k8s.io/apiserver/pkg/apis/audit"
 	webhookutil "k8s.io/apiserver/pkg/util/webhook"
+	"k8s.io/apiserver/pkg/warning"
 	utiltrace "k8s.io/utils/trace"
 )
 
@@ -267,6 +268,9 @@ func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *admiss
 			klog.Warningf("Failed to set admission audit annotation %s to %s for mutating webhook %s: %v", key, v, h.Name, err)
 		}
 	}
+	for _, w := range result.Warnings {
+		warning.AddWarning(ctx, "", w)
+	}
 
 	if !result.Allowed {
 		return false, &webhookutil.ErrWebhookRejection{Status: webhookerrors.ToStatusErr(h.Name, result.Result)}

pkg/admission/plugin/webhook/request/admissionreview.go

@@ -36,6 +36,7 @@ type AdmissionResponse struct {
 	Patch            []byte
 	PatchType        admissionv1.PatchType
 	Result           *metav1.Status
+	Warnings         []string
 }
 
 // VerifyAdmissionResponse checks the validity of the provided admission review object, and returns the
@@ -93,6 +94,7 @@ func VerifyAdmissionResponse(uid types.UID, mutating bool, review runtime.Object
 			Patch:            patch,
 			PatchType:        patchType,
 			Result:           r.Response.Result,
+			Warnings:         r.Response.Warnings,
 		}, nil
 
 	case *admissionv1beta1.AdmissionReview:
@@ -118,6 +120,7 @@ func VerifyAdmissionResponse(uid types.UID, mutating bool, review runtime.Object
 			Patch:            patch,
 			PatchType:        patchType,
 			Result:           r.Response.Result,
+			Warnings:         r.Response.Warnings,
 		}, nil
 
 	default:

pkg/admission/plugin/webhook/validating/dispatcher.go

@@ -33,6 +33,7 @@ import (
 	"k8s.io/apiserver/pkg/admission/plugin/webhook/generic"
 	webhookrequest "k8s.io/apiserver/pkg/admission/plugin/webhook/request"
 	webhookutil "k8s.io/apiserver/pkg/util/webhook"
+	"k8s.io/apiserver/pkg/warning"
 	"k8s.io/klog/v2"
 	utiltrace "k8s.io/utils/trace"
 )
@@ -227,6 +228,9 @@ func (d *validatingDispatcher) callHook(ctx context.Context, h *v1.ValidatingWeb
 			klog.Warningf("Failed to set admission audit annotation %s to %s for validating webhook %s: %v", key, v, h.Name, err)
 		}
 	}
+	for _, w := range result.Warnings {
+		warning.AddWarning(ctx, "", w)
+	}
 	if result.Allowed {
 		return nil
 	}

pkg/endpoints/filters/warning.go

@@ -17,10 +17,13 @@ limitations under the License.
 package filters
 
 import (
+	"fmt"
 	"net/http"
 	"sync"
+	"unicode/utf8"
 
 	"k8s.io/apimachinery/pkg/util/net"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apiserver/pkg/warning"
 )
 
@@ -33,10 +36,34 @@ func WithWarningRecorder(handler http.Handler) http.Handler {
 	})
 }
 
+var (
+	truncateAtTotalRunes = 4 * 1024
+	truncateItemRunes    = 256
+)
+
+type recordedWarning struct {
+	agent string
+	text  string
+}
+
 type recorder struct {
-	lock     sync.Mutex
+	// lock guards calls to AddWarning from multiple threads
+	lock sync.Mutex
+
+	// recorded tracks whether AddWarning was already called with a given text
 	recorded map[string]bool
-	writer   http.ResponseWriter
+
+	// ordered tracks warnings added so they can be replayed and truncated if needed
+	ordered []recordedWarning
+
+	// written tracks how many runes of text have been added as warning headers
+	written int
+
+	// truncating tracks if we have already exceeded truncateAtTotalRunes and are now truncating warning messages as we add them
+	truncating bool
+
+	// writer is the response writer to add warning headers to
+	writer http.ResponseWriter
 }
 
 func (r *recorder) AddWarning(agent, text string) {
@@ -47,6 +74,11 @@ func (r *recorder) AddWarning(agent, text string) {
 	r.lock.Lock()
 	defer r.lock.Unlock()
 
+	// if we've already exceeded our limit and are already truncating, return early
+	if r.written >= truncateAtTotalRunes && r.truncating {
+		return
+	}
+
 	// init if needed
 	if r.recorded == nil {
 		r.recorded = map[string]bool{}
@@ -57,13 +89,45 @@ func (r *recorder) AddWarning(agent, text string) {
 		return
 	}
 	r.recorded[text] = true
+	r.ordered = append(r.ordered, recordedWarning{agent: agent, text: text})
 
-	// TODO(liggitt): track total message characters written:
+	// truncate on a rune boundary, if needed
-	// * if this takes us over 4k truncate individual messages to 256 chars and regenerate headers
+	textRuneLength := utf8.RuneCountInString(text)
-	// * if we're already truncating truncate this message to 256 chars
+	if r.truncating && textRuneLength > truncateItemRunes {
-	// * if we're still over 4k omit this message
+		text = string([]rune(text)[:truncateItemRunes])
+		textRuneLength = truncateItemRunes
+	}
 
-	if header, err := net.NewWarningHeader(299, agent, text); err == nil {
+	// compute the header
+	header, err := net.NewWarningHeader(299, agent, text)
+	if err != nil {
+		return
+	}
+
+	// if this fits within our limit, or we're already truncating, write and return
+	if r.written+textRuneLength <= truncateAtTotalRunes || r.truncating {
+		r.written += textRuneLength
 		r.writer.Header().Add("Warning", header)
+		return
+	}
+
+	// otherwise, enable truncation, reset, and replay the existing items as truncated warnings
+	r.truncating = true
+	r.written = 0
+	r.writer.Header().Del("Warning")
+	utilruntime.HandleError(fmt.Errorf("exceeded max warning header size, truncating"))
+	for _, w := range r.ordered {
+		agent := w.agent
+		text := w.text
+
+		textRuneLength := utf8.RuneCountInString(text)
+		if textRuneLength > truncateItemRunes {
+			text = string([]rune(text)[:truncateItemRunes])
+			textRuneLength = truncateItemRunes
+		}
+		if header, err := net.NewWarningHeader(299, agent, text); err == nil {
+			r.written += textRuneLength
+			r.writer.Header().Add("Warning", header)
+		}
 	}
 }

pkg/endpoints/filters/warning_test.go

@@ -98,3 +98,35 @@ func Test_recorder_AddWarning(t *testing.T) {
 		})
 	}
 }
+
+func TestTruncation(t *testing.T) {
+	originalTotalRunes, originalItemRunes := truncateAtTotalRunes, truncateItemRunes
+	truncateAtTotalRunes, truncateItemRunes = 25, 5
+	defer func() {
+		truncateAtTotalRunes, truncateItemRunes = originalTotalRunes, originalItemRunes
+	}()
+
+	responseRecorder := httptest.NewRecorder()
+	warningRecorder := &recorder{writer: responseRecorder}
+
+	// add items longer than the individual length
+	warningRecorder.AddWarning("", "aaaaaaaaaa")  // long item
+	warningRecorder.AddWarning("-", "aaaaaaaaaa") // duplicate item
+	warningRecorder.AddWarning("", "bb")          // short item
+	warningRecorder.AddWarning("", "ccc")         // short item
+	warningRecorder.AddWarning("", "Iñtërnâtiô")  // long item
+	// check they are preserved
+	if e, a := []string{`299 - "aaaaaaaaaa"`, `299 - "bb"`, `299 - "ccc"`, `299 - "Iñtërnâtiô"`}, responseRecorder.Header()["Warning"]; !reflect.DeepEqual(e, a) {
+		t.Errorf("expected\n%#v\ngot\n%#v", e, a)
+	}
+	// add an item that exceeds the length and triggers truncation, reducing existing items to 15 runes
+	warningRecorder.AddWarning("", "e")          // triggering item, 16 total
+	warningRecorder.AddWarning("", "ffffffffff") // long item to get truncated, 21 total
+	warningRecorder.AddWarning("", "ffffffffff") // duplicate item
+	warningRecorder.AddWarning("", "gggggggggg") // item to get truncated, 26 total
+	warningRecorder.AddWarning("", "h")          // item to get ignored since we're over our limit
+	// check that existing items are truncated, and order preserved
+	if e, a := []string{`299 - "aaaaa"`, `299 - "bb"`, `299 - "ccc"`, `299 - "Iñtër"`, `299 - "e"`, `299 - "fffff"`, `299 - "ggggg"`}, responseRecorder.Header()["Warning"]; !reflect.DeepEqual(e, a) {
+		t.Errorf("expected\n%#v\ngot\n%#v", e, a)
+	}
+}