-
Notifications
You must be signed in to change notification settings - Fork 38.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding a limit on the size of request body the apiserver will decode for write operations #73805
Adding a limit on the size of request body the apiserver will decode for write operations #73805
Conversation
80e49d2
to
262609c
Compare
3eecdaf
to
0a1bf00
Compare
67a0765
to
af17697
Compare
/unassign |
|
||
// Tests that the apiserver limits the resource size in write operations. | ||
func TestMaxResourceSize(t *testing.T) { | ||
s, clientSet, closeFn := setup(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest this instead to limit to 1MB to avoid needing 200MB in memory to run this test:
stopCh := make(chan struct{})
defer close(stopCh)
clientSet, _ := framework.StartTestServer(t, stopCh, framework.TestServerSetup{
ModifyServerRunOptions: func(opts *options.ServerRunOptions) {
opts.GenericServerRunOptions.MaxRequestBodyBytes = 1024*1024
},
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Done.
one test comment, agree on matching json patch copy size |
af17697
to
a7da55a
Compare
@liggitt comments addressed. PTAL. Thank you. |
a7da55a
to
ee787c8
Compare
/retest |
2 similar comments
/retest |
/retest |
/test all |
I think the new potential http status code is worth a release note |
once it has a release note and the 10->100 test fixup is done, lgtm |
ratio between json and protobuf.
ee787c8
to
27166e4
Compare
Both done. PTAL. Thanks. |
/lgtm |
updated release note to clarify the limit only applies to resource requests, not proxy subresources like |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: caesarxuchao, liggitt 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:
Approvers can indicate their approval by writing |
The first two commits are #73713.
/assign
/kind bug-fix
/sig api-machinery
/release-note-none
This doesn't require a release note because request that fails the new limit has always been invalid. This patch just make it fail early in the apiserver REST handler, instead of after hitting etcd.