diff --git a/Godeps/Godeps.json b/Godeps/Godeps.json index 4c456253f..e57f51905 100644 --- a/Godeps/Godeps.json +++ b/Godeps/Godeps.json @@ -668,7 +668,7 @@ }, { "ImportPath": "k8s.io/api", - "Rev": "a0ea971d15ac" + "Rev": "34238374cd52" }, { "ImportPath": "k8s.io/apimachinery", diff --git a/go.mod b/go.mod index 567f95a6d..5d90a46b8 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index e7c22f051..a0cd7cf0e 100644 --- a/go.sum +++ b/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= diff --git a/pkg/admission/plugin/webhook/mutating/dispatcher.go b/pkg/admission/plugin/webhook/mutating/dispatcher.go index c1a961340..1fbe9d718 100644 --- a/pkg/admission/plugin/webhook/mutating/dispatcher.go +++ b/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)} diff --git a/pkg/admission/plugin/webhook/request/admissionreview.go b/pkg/admission/plugin/webhook/request/admissionreview.go index 5ea28d446..c60d0fb9e 100644 --- a/pkg/admission/plugin/webhook/request/admissionreview.go +++ b/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: diff --git a/pkg/admission/plugin/webhook/validating/dispatcher.go b/pkg/admission/plugin/webhook/validating/dispatcher.go index 50454b999..f065cdf50 100644 --- a/pkg/admission/plugin/webhook/validating/dispatcher.go +++ b/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 } diff --git a/pkg/endpoints/filters/warning.go b/pkg/endpoints/filters/warning.go index 323f634dc..55e85f0b7 100644 --- a/pkg/endpoints/filters/warning.go +++ b/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: - // * if this takes us over 4k truncate individual messages to 256 chars and regenerate headers - // * if we're already truncating truncate this message to 256 chars - // * if we're still over 4k omit this message + // truncate on a rune boundary, if needed + textRuneLength := utf8.RuneCountInString(text) + if r.truncating && textRuneLength > truncateItemRunes { + 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) + } } } diff --git a/pkg/endpoints/filters/warning_test.go b/pkg/endpoints/filters/warning_test.go index 2198506a9..24069283b 100644 --- a/pkg/endpoints/filters/warning_test.go +++ b/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) + } +}