jsonDataLimit#4055 #4233

cr22rc · 2020-10-06T21:13:00Z

Signed-off-by: rickr cr22rc@users.noreply.github.com

Fixes #

see #4055

Proposed Changes

Provide configmap with jsonData max bytes enforce in validating webhook.

By default jsonData max bytes is 4096 bytes.

knative-test-reporter-robot · 2020-10-06T21:34:15Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-eventing-upgrade-tests	2020-10-06 21:34:13.445 +0000 UTC	1/3

Automatically retrying due to test flakiness...
/test pull-knative-eventing-upgrade-tests

cmd/webhook/main.go

lionelvillard · 2020-10-07T13:36:07Z

pkg/apis/sources/config/pingconfig/config/ping_template_types.go

+// replicate it here.
+
+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+type PingTemplateSpec struct {


why is this needed?

It generates the deepcopy for store. Model pattern as channel store

This is quite confusing. For channel it's different because the config and the spec are the same. That's not the case for PingSource. I would remove this and just use PingDefaults. WDYT?

I'll investigate that. Currently, preoccupied with something else will get back ASAP. Thanks

lionelvillard · 2020-10-07T13:42:08Z

pkg/apis/sources/config/pingconfig/config/doc.go

+
+// Package config holds the typed objects that define the schemas for
+// ConfigMap objects that pertain to our API objects.
+package config


can we move all files in this package into apis/source/config?

yeah +1 on relocating all of those

What if we have future sources with a config? The thought behind doing this was to allow pkg/apis/sources/config/somenewsource/... ?

flattening does not prevent adding defaults for other sources, does it?

Probably not. And I'll go ahead and do this change. I'm on the record :) that keeping them separate while maybe not necessary would be cleaner IMO

pkg/apis/sources/v1beta1/ping_validation.go

pkg/apis/sources/v1alpha2/ping_validation.go

zhongduo · 2020-10-07T13:47:29Z

config/core/configmaps/default-pingsource.yaml

+    knative.dev/example-checksum: "6eaeecba"
+data:
+  default-ping-config: |
+    ################################


Example part should be put under _example: |, see example here:

eventing/config/core/configmaps/observability.yaml

Line 27 in 5deb379

_example: |

I think the confusion was I copied the example comment block. I'm now following what was done with default-broker.yaml, default-broker-channel.yaml, and default-channel.yaml

zhongduo · 2020-10-07T13:49:24Z

pkg/apis/sources/config/pingconfig/config/ping_defaults.go

+}
+
+// NewPingDefaultsConfigFromConfigMap creates a PingDefaults from the supplied configMap
+func NewPingDefaultsConfigFromConfigMap(config *corev1.ConfigMap) (*PingDefaults, error) {


Do you need to handle nil?

no present on a nil map comes back as false.

matzew · 2020-10-08T11:50:04Z

config/core/configmaps/default-pingsource.yaml

+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: default-ping-webhook


I'd rather pick a different name. Almost all CMs in evneting start with config- - except for default-ch-webhook, unfortunately

@matzew Ok I followed that. Given the importance of this conformance can you open a issue for this?

I've reconsidered. I think the confusion was I copied the example comment block. I'm now following what was done with default-broker.yaml, default-broker-channel.yaml, and default-channel.yaml

codecov · 2020-11-21T20:19:13Z

Codecov Report

Merging #4233 (9244b69) into master (d8468ca) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4233      +/-   ##
==========================================
+ Coverage   81.35%   81.39%   +0.04%     
==========================================
  Files         292      292              
  Lines        8322     8341      +19     
==========================================
+ Hits         6770     6789      +19     
  Misses       1144     1144              
  Partials      408      408

Impacted Files	Coverage Δ
pkg/apis/sources/v1alpha2/ping_validation.go	`100.00% <100.00%> (ø)`
pkg/apis/sources/v1beta1/ping_validation.go	`100.00% <100.00%> (ø)`
pkg/apis/sources/v1beta2/ping_validation.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8468ca...9244b69. Read the comment docs.

lgtm-com · 2020-11-21T20:36:08Z

This pull request introduces 1 alert when merging aa94a97 into c3e71f5 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

knative-metrics-robot · 2020-11-21T20:53:37Z

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/sources/v1beta2/ping_validation.go	100.0%	89.2%	-10.8

cr22rc · 2020-12-02T17:08:35Z

I think I addressed all.
I guess if I can't get it merged .. I'll just close it.

zhongduo · 2020-12-03T01:10:09Z

@cr22rc is there a use case or existing problem that need this? Generally I think it worth bring it up in the eventing meeting (we happened to discuss this week that we should revisit long living PRs in the meeting). Or a discussion in Slack can also be useful.

zhongduo · 2020-12-03T01:13:36Z

pkg/apis/sources/config/ping_defaults.go

+	if d.DataMaxSize < 1 {
+		d.DataMaxSize = 4096
+	}


If we reserve the default to be 0, which means unlimited, we can achieve backward compatibility.

I don't think that's good admins can install this not know that user could dump large amount of data into their etcd .. Better for user to get a good message that it's limited and admin can adjust

zhongduo · 2020-12-03T01:19:09Z

Another concern is that I am not sure if a global configmap is a good option for this. IIUC, this can be per source based right? Did you consider adding it as a ping source attribute? This way each one can have a different setting.

cr22rc · 2020-12-07T18:09:43Z

We don't want user to create unlimited sized messages. Just ading that to the pingsource does not stop them. In reality the data is not much used since it just static. 4096 probably far exceeds what most user will every put in.

antoineco · 2021-02-12T16:26:00Z

config/core/configmaps/default-pingsource.yaml

+    knative.dev/example-checksum: "6eaeecba"
+data:
+  ping-config: |
+   # dataMaxSize: 4096 # Max number of bytes allowed to be sent for message excluding any base64 decoding.


I see an example checksum, but no _example key in the configmap.

Good point. knative.dev/example-checksum: "6eaeecba" Is not needed here.

antoineco · 2021-02-12T16:28:42Z

pkg/apis/sources/v1alpha2/ping_validation_test.go

+			if test.source.Spec.JsonData == "TOBIG" {
+				var b strings.Builder
+				b.Grow(5000)
+				b.WriteString("\"")
+				for i := 0; i < 4998; i++ {
+					b.WriteString("a")
+				}
+				b.WriteString("\"")
+				test.source.Spec.JsonData = b.String()
+			}


Maybe just declare a variable with that buffer initialized before the tests?

antoineco · 2021-02-12T16:29:01Z

pkg/apis/sources/v1alpha2/ping_validation_test.go

+		source: PingSource{
+			Spec: PingSourceSpec{
+				Schedule: "*/2 * * * *",
+				JsonData: "TOBIG",


"Too" takes 2 "O".

lionelvillard · 2021-02-12T19:21:56Z

Set a limit on a type and I'll create 1000 objects instead of 100.

That's one possible trade-off, with the advantage that it can be directly be implemented by using k8s quotas.

Another trade-off is to limit the CR size, and AFAIK that's not something k8s supports right now, am I right?

antoineco · 2021-02-12T19:35:34Z

It doesn't indeed, but I still don't understand, why focusing on just the PingSource? It seems to me like a careful Kubernetes admin would limit this globally using either

an admission controller (link)
some kind of policy agent

This issue can apply to literally any Kubernetes object: you could create 1.5 MB of annotations, and a ConfigMap would be helpless in preventing that. Even if it wasn't, I doubt anyone would want to go ahead and create one ConfigMap per Kubernetes type (good luck).

Just my 2c, but to me it feels like the wrong place and the wrong way to address such a broad concern and I'm surprised to see this in Knative.

lionelvillard · 2021-02-12T20:25:48Z

To be clear: this PR is not about limiting the size of CRs, it is about fairness. It's about making sure a tenant does not allocate all available resources. For PingSource, there is a direct link between CR size and fairness, thus this PR.

As for the implementation, I agree it could be generalized but for now it's really only applicable to PingSource.

(and please keep discussing)

Default is unbounded. Signed-off-by: rickr <cr22rc@users.noreply.github.com>

lionelvillard · 2021-02-15T16:34:36Z

/approve

/hold

to address comments.

knative-prow-robot · 2021-02-15T16:34:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cr22rc, lionelvillard

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [lionelvillard]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cr22rc · 2021-02-15T17:01:03Z

Changed so the default for data length is not checked.

lionelvillard · 2021-02-16T14:20:10Z

/unhold

lionelvillard · 2021-02-16T15:47:37Z

/lgtm

lberk · 2021-02-17T15:26:53Z

this change seems to be breaking downstream for us. Particularly having an empty data.ping-config and the eventing-webhook fails to start with

{"level":"fatal","ts":"2021-02-17T07:53:19.466Z","logger":"eventing-webhook.ping-config-store","caller":"configmap/store.go:149","msg":"Error initializing pingdefaults config \"config-ping-webhook\": \"ConfigMap is missing (or empty) key: \\\"ping-config\\\" : map[ping-config:]\"","knative.dev/pod":"eventing-webhook-7c5499d784-tvpgd","stacktrace":"knative.dev/pkg/configmap.(*UntypedStore).OnConfigChanged\n\t/go/src/knative.dev/eventing/vendor/knative.dev/pkg/configmap/store.go:149\nknative.dev/pkg/configmap.(*ManualWatcher).OnChange\n\t/go/src/knative.dev/eventing/vendor/knative.dev/pkg/configmap/manual_watcher.go:72\nknative.dev/pkg/configmap/informer.(*InformedWatcher).addConfigMapEvent\n\t/go/src/knative.dev/eventing/vendor/knative.dev/pkg/configmap/informer/informed_watcher.go:190\nk8s.io/client-go/tools/cache.ResourceEventHandlerFuncs.OnAdd\n\t/go/src/knative.dev/eventing/vendor/k8s.io/client-go/tools/cache/controller.go:227\nk8s.io/client-go/tools/cache.(*processorListener).run.func1\n\t/go/src/knative.dev/eventing/vendor/k8s.io/client-go/tools/cache/shared_informer.go:777\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1\n\t/go/src/knative.dev/eventing/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:155\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil\n\t/go/src/knative.dev/eventing/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:156\nk8s.io/apimachinery/pkg/util/wait.JitterUntil\n\t/go/src/knative.dev/eventing/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:133\nk8s.io/apimachinery/pkg/util/wait.Until\n\t/go/src/knative.dev/eventing/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:90\nk8s.io/client-go/tools/cache.(*processorListener).run\n\t/go/src/knative.dev/eventing/vendor/k8s.io/client-go/tools/cache/shared_informer.go:771\nk8s.io/apimachinery/pkg/util/wait.(*Group).Start.func1\n\t/go/src/knative.dev/eventing/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:73"}

Why is the intermediate field ping-config necessary for a single key/value pair? Could we please follow the example of other configmaps and have a _example entry instead of a blank entry?

matzew · 2021-02-17T16:03:21Z

@cr22rc Do you think this is a problem for migrations?

matzew · 2021-02-17T16:55:27Z

I'd vote to revert this, before we cut the release - since it does have issues

This reverts commit 84b99db.

cr22rc · 2021-02-17T17:14:08Z

I think the fix is just to set limit
#4893

cr22rc · 2021-02-17T17:15:45Z

I'm still not sure how the scenario is that this fails. I doesn't fail locally nor did it on the e2e tests.

antoineco · 2021-02-17T17:23:13Z

@matzew I'd vote for this as well. Besides, not all PR comments were addressed, including some important ones regarding the ConfigMap itself:

tayarani · 2021-02-17T21:12:29Z

I would vote we revert this as well. We're getting the same errors as posted by @lberk

lberk · 2021-02-17T21:46:40Z

I've done my best to address some of the comments and fix this breakage in #4894

That being said, if there are still issues including #4894 I have no problem reverting the entire set of patches before the release.

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 6, 2020

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 6, 2020

knative-prow-robot requested review from chaodaiG and tommyreddad October 6, 2020 21:13

knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Oct 6, 2020

cr22rc force-pushed the jsonDataLimit-4055 branch from b067889 to fc69dc1 Compare October 6, 2020 21:22

lionelvillard reviewed Oct 7, 2020

View reviewed changes

cmd/webhook/main.go Outdated Show resolved Hide resolved

lionelvillard reviewed Oct 7, 2020

View reviewed changes

pkg/apis/sources/v1beta1/ping_validation.go Outdated Show resolved Hide resolved

lionelvillard reviewed Oct 7, 2020

View reviewed changes

pkg/apis/sources/v1alpha2/ping_validation.go Outdated Show resolved Hide resolved

zhongduo reviewed Oct 7, 2020

View reviewed changes

matzew reviewed Oct 8, 2020

View reviewed changes

cr22rc force-pushed the jsonDataLimit-4055 branch from fc69dc1 to b83317d Compare October 8, 2020 17:58

cr22rc force-pushed the jsonDataLimit-4055 branch from b83317d to aa94a97 Compare November 21, 2020 20:12

cr22rc force-pushed the jsonDataLimit-4055 branch from aa94a97 to faea835 Compare November 21, 2020 20:46

zhongduo reviewed Dec 3, 2020

View reviewed changes

zhongduo mentioned this pull request Jan 5, 2021

Add zhongduo (Jimmy Lin) as an approver #4691

Merged

cr22rc force-pushed the jsonDataLimit-4055 branch from faea835 to 3bba87b Compare February 12, 2021 15:51

antoineco reviewed Feb 12, 2021

View reviewed changes

knative-prow-robot assigned antoineco Feb 12, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2021

jsonDataLimit#4055

9244b69

Default is unbounded. Signed-off-by: rickr <cr22rc@users.noreply.github.com>

cr22rc force-pushed the jsonDataLimit-4055 branch from 3bba87b to 9244b69 Compare February 15, 2021 11:47

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 15, 2021

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 15, 2021

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 16, 2021

knative-prow-robot assigned lionelvillard Feb 16, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2021

knative-prow-robot merged commit 84b99db into knative:master Feb 16, 2021

matzew added a commit to matzew/eventing that referenced this pull request Feb 17, 2021

Revert "jsonDataLimit#4055 (knative#4233)"

d001f6b

This reverts commit 84b99db.

jsonDataLimit#4055 #4233

jsonDataLimit#4055 #4233

Conversation

cr22rc commented Oct 6, 2020

Proposed Changes

knative-test-reporter-robot commented Oct 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cr22rc Oct 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cr22rc Oct 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 21, 2020 • edited

Codecov Report

lgtm-com bot commented Nov 21, 2020

knative-metrics-robot commented Nov 21, 2020

cr22rc commented Dec 2, 2020

zhongduo commented Dec 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongduo commented Dec 3, 2020 • edited

cr22rc commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lionelvillard commented Feb 12, 2021

antoineco commented Feb 12, 2021 • edited

lionelvillard commented Feb 12, 2021

lionelvillard commented Feb 15, 2021

knative-prow-robot commented Feb 15, 2021

cr22rc commented Feb 15, 2021 • edited

lionelvillard commented Feb 16, 2021

lionelvillard commented Feb 16, 2021

lberk commented Feb 17, 2021

matzew commented Feb 17, 2021

matzew commented Feb 17, 2021

cr22rc commented Feb 17, 2021

cr22rc commented Feb 17, 2021

antoineco commented Feb 17, 2021 • edited

tayarani commented Feb 17, 2021

lberk commented Feb 17, 2021

cr22rc Oct 19, 2020 •

edited

cr22rc Oct 8, 2020 •

edited

codecov bot commented Nov 21, 2020 •

edited

zhongduo commented Dec 3, 2020 •

edited

antoineco commented Feb 12, 2021 •

edited

cr22rc commented Feb 15, 2021 •

edited

antoineco commented Feb 17, 2021 •

edited