Revert "Merge pull request #47353 from apelisse/http-cache"

[liggitt]

Some issues were discovered with the caching merged in #47353:

• uses a disk-based cache that is not safe between processes (does not use atomic fs operations)
• writes get/list responses to disk that should not be cached (like kubectl get secrets)
• is vulnerable to partially written cache responses being used as responses to future requests
• breaks uses of the client transport that make use of websockets
• defaults to enabling the cache for any client builder using RecommendedConfigOverrideFlags or DefaultClientConfig which affects more components than just kubectl

This reverts commit fc89743, reversing changes made to 29ab38e.

[liggitt]

cc @apelisse @mbohlool @pwittrock from original PR

[apelisse]

Thanks @liggitt for working on that, and sorry for breaking your use-case. All good points,

Can you clarify what the difference is between:

uses a disk-based cache that is not safe between processes (does not use atomic fs operations)

and

is vulnerable to partially written cache responses being used as responses to future requests

Or maybe the question is, would a fix for the first point also fix that second point? If not, can you clarify what you meant for the latter.

[liggitt]

uses a disk-based cache that is not safe between processes (does not use atomic fs operations)

this means that two successful writes by separate processes would stomp on each other (process 1 starts writing, process 2 starts writing, they both write to the file in an interleaved way)

is vulnerable to partially written cache responses being used as responses to future requests

this means that if a single process fails halfway through writing the file, the half-written file is left in place. a future invocation of the cache will scan existing files to determine what is in cache, and the half-written file gets put in the index of available cached responses.

would a fix for the first point also fix that second point?

possibly, depending on the fix... if the disk cache changed to always write to a tmp file and only on success do a move to a file that is used to serve future cached responses, I think that would likely address both issues.

[apelisse]

if the disk cache changed to always write to a tmp file and only on success do a move to a file that is used to serve future cached responses, I think that would likely address both issues.

I think we're on the same page then, that's exactly what I had in mind. Thanks

[smarterclayton]

/lgtm

Given that this impacts concurrent kubectl clients in unpredictable ways, and also caches secret data on disk.

[liggitt]

rebased to fix godep conflict

[liggitt]

fixed conflict in staging godep

[smarterclayton]

/retest

[thockin]

/lgtm

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, smarterclayton, thockin

Associated issue: 47353

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

Needs approval from an approver in each of these OWNERS Files:

• OWNERS [smarterclayton,thockin]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[smarterclayton]

/retest Neither look related

[liggitt]

unit flaked on #50022
/retest

[fejta-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

[k8s-github-robot]

/test all [submit-queue is verifying that this PR is safe to merge]

[k8s-ci-robot]

@liggitt: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-unit	4ee72eb	link	/test pull-kubernetes-unit

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 50254, 50174, 50179)