Skip to content
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

fix kubelet status http calls with truncation #82669

Merged
merged 1 commit into from Sep 13, 2019

Conversation

@rphillips
Copy link
Member

commented Sep 12, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
#76518 introduced a change to limit the read on various calls including the kubelet http status request using utilio.ReadAtMost. By design, ReadAtMost will return an error if there
is a truncation, but the calling code needs to know to handle it.

Which issue(s) this PR fixes:
#82671

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Single static pod files and pod files from http endpoints cannot be larger than 10 MB. HTTP probe payloads are now truncated to 10KB.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

/cc @sttts @smarterclayton @deads2k @derekwaynecarr @sjenning

@rphillips

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

@rphillips

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

@k8s-ci-robot k8s-ci-robot requested review from mattjmcnaughton and dims Sep 12, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

@rphillips: GitHub didn't allow me to request PR reviews from the following users: haiyanmeng.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @haiyanmeng @mattjmcnaughton @dims @tallclair

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot requested a review from tallclair Sep 12, 2019
@@ -94,7 +94,7 @@ func (s *sourceURL) extractFromURL() error {
}
defer resp.Body.Close()
data, err := utilio.ReadAtMost(resp.Body, maxConfigLength)
if err != nil {
if err != nil && err != utilio.ErrLimitReached {

This comment has been minimized.

Copy link
@Random-Liu

Random-Liu Sep 12, 2019

Member

I think it is fine to return error directly for this one.

@@ -112,7 +112,7 @@ func DoHTTPProbe(url *url.URL, headers http.Header, client GetHTTPInterface) (pr
}
defer res.Body.Close()
b, err := utilio.ReadAtMost(res.Body, maxRespBodyLength)
if err != nil {
if err != nil && err != utilio.ErrLimitReached {

This comment has been minimized.

Copy link
@Random-Liu

Random-Liu Sep 12, 2019

Member

This one makes sense to me, but can we at list log the error instead of directly ignoring it?

@eparis eparis referenced this pull request Sep 12, 2019
22 of 22 tasks complete
@rphillips rphillips force-pushed the rphillips:fixes/76518 branch from 7d2d0a1 to ee5c61d Sep 12, 2019
@derekwaynecarr

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

thanks for making the updates.

/lgtm
/approve

@Random-Liu

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

/approve
/lgtm

@liggitt

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

if we are cherry-picking this, we should consider doing #82514 as well :)

this is fixing a regression already in the 1.16 branch. #82514 seems reasonable for a 1.16.1 pick

@liggitt

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

also we are missing tests :)

agree that a test is important here

@tallclair

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

/assign
/cc @haiyanmeng

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

@tallclair: GitHub didn't allow me to request PR reviews from the following users: haiyanmeng.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/assign
/cc @haiyanmeng

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@lachie83

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@lachie83

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

@rphillips please proceed with raising the cherry-pick into release-1.16 branch and link it to the tracking issue. The cherry-pick process is described here.

@dchen1107

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

Thanks for reporting the issue and providing the fix. Can I ask a test here?

@Random-Liu

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

BTW, actually we may also want to include a 1.16 release note for #82669 (comment).

Basically, now you can't have a single static pod file, or pods from http endpoint that is larger than 10MB. For most users 10MB is big enough, but we should at least have a release note about this just in case.

@rphillips Can you include this in the release note of your cherry-pick? Thanks!

@rphillips rphillips force-pushed the rphillips:fixes/76518 branch from ee5c61d to 5db481b Sep 13, 2019
@k8s-ci-robot k8s-ci-robot added size/M and removed lgtm size/XS labels Sep 13, 2019
@rphillips

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2019

@dchen1107 added a couple unit tests
@Random-Liu kind ping to re-review

PR#76518 introduced a change to limit the the read on various calls with
utilio.ReadAtMost.  By design, ReadAtMost will return an error if there
is a truncation, but the calling code needs to know to handle it.
@rphillips rphillips force-pushed the rphillips:fixes/76518 branch from 5db481b to 1065add Sep 13, 2019
@liggitt

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

/retest

@liggitt

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

unit test lgtm

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 13, 2019
@k8s-ci-robot k8s-ci-robot merged commit 40695b0 into kubernetes:master Sep 13, 2019
25 checks passed
25 checks passed
cla/linuxfoundation rphillips authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-alpha-features Skipped.
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot modified the milestones: v1.16, v1.17 Sep 13, 2019
k8s-ci-robot added a commit that referenced this pull request Sep 13, 2019
…pstream-release-1.16

Automated cherry pick of #82669: fix kubelet status http calls with truncation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.