From 670ba49b251022690f293a3492464232a0d4cc84 Mon Sep 17 00:00:00 2001 From: Kevin Dorosh Date: Thu, 29 Sep 2022 10:39:43 -0400 Subject: [PATCH 1/2] Add env var to escape new default behavior --- pkg/api/shared/status.go | 14 ++++- .../v1/clients/kube/resource_client_test.go | 21 ++++++- pkg/api/v2/reporter/reporter.go | 14 ++++- pkg/api/v2/reporter/reporter_test.go | 60 +++++++++++++++++++ 4 files changed, 105 insertions(+), 4 deletions(-) diff --git a/pkg/api/shared/status.go b/pkg/api/shared/status.go index 5f6bd1ff4..efe88c148 100644 --- a/pkg/api/shared/status.go +++ b/pkg/api/shared/status.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "fmt" + "os" "github.com/golang/protobuf/jsonpb" "github.com/solo-io/go-utils/contextutils" @@ -20,6 +21,15 @@ const ( MaxStatusBytes = 2048 ) +var ( + // only public for unit tests! + DisableMaxStatusSize = false +) + +func init() { + DisableMaxStatusSize = os.Getenv("DISABLE_MAX_STATUS_SIZE") == "true" +} + // ApplyStatus is used by clients that don't support patch updates to resource statuses (e.g. consul, files, in-memory) func ApplyStatus(rc clients.ResourceClient, statusClient resources.StatusClient, inputResource resources.InputResource, opts clients.ApplyStatusOpts) (resources.Resource, error) { name := inputResource.GetMetadata().GetName() @@ -78,9 +88,9 @@ func GetJsonPatchData(ctx context.Context, inputResource resources.InputResource patch := fmt.Sprintf(`[{"op": "replace", "path": "/status/statuses/%s", "value": %s}]`, ns, string(bytes)) // only replace our status so other reporters are not affected (e.g. blue-green of gloo) data := []byte(patch) - if len(data) > MaxStatusBytes { + if !DisableMaxStatusSize && len(data) > MaxStatusBytes { if contextutils.GetLogLevel() == zapcore.DebugLevel { - contextutils.LoggerFrom(ctx).Warnf("status patch is too large, will not apply: %s", data) + contextutils.LoggerFrom(ctx).Debugf("status patch is too large, will not apply: %s", data) } else { contextutils.LoggerFrom(ctx).Warnw("status patch is too large, will not apply", zap.Int("status_bytes", len(data))) } diff --git a/pkg/api/v1/clients/kube/resource_client_test.go b/pkg/api/v1/clients/kube/resource_client_test.go index c0b0f8eca..6d189377b 100644 --- a/pkg/api/v1/clients/kube/resource_client_test.go +++ b/pkg/api/v1/clients/kube/resource_client_test.go @@ -617,7 +617,6 @@ var _ = Describe("Test Kube ResourceClient", func() { }) It("skips status updates if too large", func() { - var sb strings.Builder for i := 0; i < shared.MaxStatusBytes+1; i++ { sb.WriteString("a") @@ -634,6 +633,26 @@ var _ = Describe("Test Kube ResourceClient", func() { Expect(err).To(MatchError(ContainSubstring("patch is too large"))) Expect(res).To(BeNil()) }) + + It("honors env var override on status truncation", func() { + + shared.DisableMaxStatusSize = true + + var sb strings.Builder + for i := 0; i < shared.MaxStatusBytes+1; i++ { + sb.WriteString("a") + } + tooLargeReason := sb.String() + + statusClient.SetStatus(resourceToUpdate, &core.Status{ + State: 2, + Reason: tooLargeReason, + ReportedBy: "me", + }) + + _, err := rc.ApplyStatus(statusClient, resourceToUpdate, clients.ApplyStatusOpts{}) + Expect(err).NotTo(MatchError(ContainSubstring("patch is too large"))) + }) }) Describe("listing resources", func() { diff --git a/pkg/api/v2/reporter/reporter.go b/pkg/api/v2/reporter/reporter.go index 67025835f..6b6eb7fca 100644 --- a/pkg/api/v2/reporter/reporter.go +++ b/pkg/api/v2/reporter/reporter.go @@ -2,6 +2,7 @@ package reporter import ( "context" + "os" "sort" "strings" @@ -22,6 +23,15 @@ const ( MaxStatusKeys = 100 ) +var ( + // only public for unit tests! + DisableTruncateStatus = false +) + +func init() { + DisableTruncateStatus = os.Getenv("DISABLE_TRUNCATE_STATUS") == "true" +} + type Report struct { Warnings []string Errors error @@ -268,7 +278,9 @@ func (r *reporter) WriteReports(ctx context.Context, resourceErrs ResourceReport return errors.Errorf("reporter: was passed resource of kind %v but no client to support it", kind) } status := r.StatusFromReport(report, subresourceStatuses) - status = trimStatus(status) + if !DisableTruncateStatus { + status = trimStatus(status) + } resourceStatus := r.statusClient.GetStatus(resource) if status.Equal(resourceStatus) { diff --git a/pkg/api/v2/reporter/reporter_test.go b/pkg/api/v2/reporter/reporter_test.go index 739e999b0..03d8fdce4 100644 --- a/pkg/api/v2/reporter/reporter_test.go +++ b/pkg/api/v2/reporter/reporter_test.go @@ -188,6 +188,66 @@ var _ = Describe("Reporter", func() { })) }) + It("honors override for truncate statuses", func() { + rep.DisableTruncateStatus = true + + r1, err := mockResourceClient.Write(v1.NewMockResource("", "mocky"), clients.WriteOpts{}) + Expect(err).NotTo(HaveOccurred()) + + var sb strings.Builder + for i := 0; i < rep.MaxStatusBytes+1; i++ { + sb.WriteString("a") + } + + // an error larger than our max (1kb) that should NOT be truncated + veryLargeError := sb.String() + + childSubresourceStatuses := map[string]*core.Status{} + for i := 0; i < rep.MaxStatusKeys+1; i++ { // we have numerous keys, and do not expect to trim to num(parentkeys)/2 (i.e. rep.MaxStatusKeys/2) + var sb strings.Builder + for j := 0; j <= i; j++ { + sb.WriteString("a") + } + childSubresourceStatuses[fmt.Sprintf("child-subresource-%s", sb.String())] = &core.Status{ + State: core.Status_Warning, + Reason: veryLargeError, + ReportedBy: "test", + SubresourceStatuses: nil, // intentionally nil; only test recursive call once + } + } + + subresourceStatuses := map[string]*core.Status{} + for i := 0; i < rep.MaxStatusKeys+1; i++ { // we have numerous keys, and do not expect to trim to 100 keys (rep.MaxStatusKeys) + var sb strings.Builder + for j := 0; j <= i; j++ { + sb.WriteString("a") + } + subresourceStatuses[fmt.Sprintf("parent-subresource-%s", sb.String())] = &core.Status{ + State: core.Status_Warning, + Reason: veryLargeError, + ReportedBy: "test", + SubresourceStatuses: childSubresourceStatuses, + } + } + + resourceErrs := rep.ResourceReports{ + r1.(*v1.MockResource): rep.Report{Errors: fmt.Errorf(veryLargeError)}, + } + err = reporter.WriteReports(context.TODO(), resourceErrs, subresourceStatuses) + Expect(err).NotTo(HaveOccurred()) + + r1, err = mockResourceClient.Read(r1.GetMetadata().Namespace, r1.GetMetadata().Name, clients.ReadOpts{}) + Expect(err).NotTo(HaveOccurred()) + + status := statusClient.GetStatus(r1.(*v1.MockResource)) + Expect(status).To(Equal(&core.Status{ + State: 2, + Reason: veryLargeError, + ReportedBy: "test", + SubresourceStatuses: subresourceStatuses, + })) + }) + It("handles conflict", func() { r1, err := mockResourceClient.Write(v1.NewMockResource("", "mocky"), clients.WriteOpts{}) Expect(err).NotTo(HaveOccurred()) From b176d0d7af58b2b08a1b806e5d33c757ff699c71 Mon Sep 17 00:00:00 2001 From: Kevin Dorosh Date: Thu, 29 Sep 2022 10:48:59 -0400 Subject: [PATCH 2/2] Add changelog --- changelog/v0.30.7/escape-hatches.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/v0.30.7/escape-hatches.yaml diff --git a/changelog/v0.30.7/escape-hatches.yaml b/changelog/v0.30.7/escape-hatches.yaml new file mode 100644 index 000000000..a5ab2e3fd --- /dev/null +++ b/changelog/v0.30.7/escape-hatches.yaml @@ -0,0 +1,5 @@ +changelog: + - type: FIX + resolvesIssue: false + description: Add escape hatches via env var for any users depending on large statuses + issueLink: https://github.com/solo-io/solo-kit/issues/523