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.

Sign up for GitHub

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

Jump to bottom

Kubectl to use http caching to cache openapi responses from the server #50404

Merged
merged 5 commits into from
Sep 1, 2017
Merged

Kubectl to use http caching to cache openapi responses from the server #50404

merged 5 commits into from
Sep 1, 2017

Conversation

apelisse
Copy link
Member

@apelisse apelisse commented Aug 9, 2017 •
edited
Loading

What this PR does / why we need it:

This PR is trying to address the problems raised in #50254

  • 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

All of these points are addressed by this pull-request:

  1. It now uses atomic fs operations
  2. Doesn't cache by default, only if requested by the client (and it's only done by openapi client)
  3. Fixed because of atomic fs operations
  4. Found the reason for the bug: Cache wrapper couldn't be unwrapped. I implemented the WrappedRoundTripper interface.
  5. Since 2. is fixed, I think that should be fine

@smarterclayton @liggitt

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #50254

Special notes for your reviewer:

Release note:

Allows kubectl to use http caching mechanism for the OpenAPI schema. The cache directory can be configured through `--cache-dir` command line flag to kubectl. If set to empty string, caching will be disabled.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 9, 2017
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Aug 9, 2017
@apelisse
Copy link
Member Author

apelisse commented Aug 9, 2017

Looks like I forgot to commit the new diskv files ...

@apelisse apelisse force-pushed the http-cache branch 3 times, most recently from 621849b to 932f58a Compare August 15, 2017 15:32
@kubernetes kubernetes deleted a comment from k8s-ci-robot Aug 15, 2017
@apelisse apelisse force-pushed the http-cache branch 3 times, most recently from e53f6cb to 06df1b2 Compare August 16, 2017 17:44
@apelisse apelisse changed the title WIP: Http cache v2 Http cache v2 Aug 16, 2017
@apelisse
Copy link
Member Author

PTAL @liggitt

@apelisse
Copy link
Member Author

/retest

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2017
@apelisse apelisse added this to the v1.8 milestone Aug 17, 2017
pwittrock
pwittrock reviewed Aug 17, 2017
View reviewed changes
staging/src/k8s.io/client-go/transport/round_trippers_test.go Outdated
if err != nil {
t.Fatal(err)
}
if rt.Request.Header.Get("If-None-Match") != "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets write a test that can check what was actually stored to verify nothing was written to the cache.

@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 17, 2017
@pwittrock
Copy link
Member

One note about additional testing, otherwise lgtm.

@pwittrock pwittrock changed the title Http cache v2 Client go to use http caching to cache openapi responses from the server Aug 17, 2017
@pwittrock
Copy link
Member

pwittrock commented Aug 17, 2017 •
edited
Loading

@liggitt My understanding is that this is consistent with how the caching libraries expect things.

I think in the ideal solution, the server would define the caching behavior in the response header to tell all clients whether or not to cache the result (e.g. browsers, other language clients, etc would know what to do).

The vendored client cache library is also supposed to allow the caching behavior to be controlled by the client if it is specified in the request headers, so the client could send a no-store control header and the client wouldn't cache it even without the plumbing logic in the PR.

So the code is (now) partially redundant, but more explicit in what it is doing vs depending on implementation details of the vendored code.

@pwittrock
Copy link
Member

/retest

2 similar comments
@apelisse
Copy link
Member Author

/retest

@apelisse
Copy link
Member Author

/retest

@apelisse
Copy link
Member Author

OK Looks like we have consensus on that, can we have approval and lgtm? @liggitt @smarterclayton

@liggitt
Copy link
Member

liggitt commented Aug 29, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2017
@apelisse apelisse mentioned this pull request Aug 29, 2017
Merged
@apelisse
Copy link
Member Author

@smarterclayton I'd really like if you could approve that, but I'll look for another approver.

@bgrant0607
Copy link
Member

What is the flag name? It should be in the release note. Also, the convention is to separate words with hyphens.

What K8s components will use this? The release note should specify that, also, since it's probably not obvious to users what will be affected by this change.

@apelisse apelisse changed the title Client go to use http caching to cache openapi responses from the server Kubectl to use http caching to cache openapi responses from the server Aug 30, 2017
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2017
@apelisse
Copy link
Member Author

@bgrant0607 Updated the release note, renamed the flag, PTAL

@bgrant0607
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, bgrant0607, liggitt

Associated issue: 50254

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

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

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

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2017
@cblecker
Copy link
Member

/test all

@mengqiy
Copy link
Member

mengqiy commented Aug 31, 2017

/retest

@fejta-bot
Copy link

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

Review the full test history for this PR.

1 similar comment
@fejta-bot
Copy link

/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
Copy link

Automatic merge from submit-queue (batch tested with PRs 51480, 49616, 50123, 50846, 50404)

@k8s-github-robot k8s-github-robot merged commit 0f2c2bd into kubernetes:master Sep 1, 2017
hh pushed a commit to ii/kubernetes that referenced this pull request Sep 3, 2017
Merge pull request kubernetes#51546 from apelisse/remove-duplicate-fa…
0f2a72f 
…ke-openapi

Automatic merge from submit-queue (batch tested with PRs 51682, 51546, 51369, 50924, 51827)

Remove duplicate fake and unused openapi

**What this PR does / why we need it**:
Follow-up on PR kubernetes#50404

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #

**Special notes for your reviewer**:

**Release note**:
```release-note
NONE
```
@shiywang shiywang mentioned this pull request Sep 7, 2017
Closed
@billiford billiford mentioned this pull request Apr 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pwittrock pwittrock pwittrock left review comments

@mengqiy mengqiy mengqiy left review comments

@liggitt liggitt liggitt left review comments

Assignees

@liggitt liggitt

@smarterclayton smarterclayton

@mengqiy mengqiy

@caesarxuchao caesarxuchao

@bgrant0607 bgrant0607

@pwittrock pwittrock

@wojtek-t wojtek-t

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.