Cache busting #284

Jefftree · 2022-03-10T19:51:38Z

Support Cache Busting in kube-openapi.

Also fix a bug where lazy marshaling wasn't performed on OpenAPI V3.

Jefftree · 2022-03-10T20:37:55Z

/cc @apelisse

Jefftree · 2022-03-14T19:48:13Z

/assign @apelisse
/cc @alexzielenski

k8s-ci-robot · 2022-03-14T19:48:15Z

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

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

In response to this:

/assign @apelisse
/cc @alexzielenski

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.

alexzielenski · 2022-03-16T00:36:33Z

pkg/handler3/handler.go

+		if err != nil {
+			return nil, err
+		}
+		discovery.Paths[k] = "openapi/v3/" + k + "?hash=" + unquoteETag(etag)


While consuming this API in a test I found it a bit unwieldy. I expected discovery.Paths to be a groupVersion -> ETAG map, rather than a groupVersion -> path map. I ended up using url.Parse to extract the hash query param from the path since the client-go discovery client for OpenAPI V3 asks for the hash by itself.

Also it is worth pointing out that in the future more query params may be added to the OpenAPI endpoints, so it may become inconvenient to deal with these paths with an assumed URL structure.

We may also in the future end up wanting to extend the discovery map to include additional metadata. Should the type of discovery.Paths[k] be a map[string]interface{} rather than simply a string? So that for example the response might be:

{ "paths": { "batch/v1": { "hash": <ETAG> } } }

I don't disagree with that, but it should be a URL rather than the hash so it's built automatically (clients shouldn't even have to know what to do with the hash, it should just contain the URL they need to download as a relative link).

While consuming this API in a test I found it a bit unwieldy. I expected discovery.Paths to be a groupVersion -> ETAG map, rather than a groupVersion -> path map. I ended up using url.Parse to extract the hash query param from the path since the client-go discovery client for OpenAPI V3 asks for the hash by itself.

Interesting, why didn't you just want to download the relative path given?

We may also in the future end up wanting to extend the discovery map to include additional metadata.

Good idea, I'll change the structure.

I ended up using url.Parse to extract the hash query param from the path since the client-go discovery client

Thanks for catching this inconsistency. Question for both of you: Would it make more sense for the client-go interface to directly consume the URL (eg: OpenAPISchema(URL string)) instead of the current OpenAPISchema(gvPath, hash string). I agree we shouldn't assume an URL structure and going the route of parsing the URL may add more complexity in the future

I don't disagree with that, but it should be a URL rather than the hash so it's built automatically (clients shouldn't even have to know what to do with the hash, it should just contain the URL they need to download as a relative link).

In that case a URL could be included as one of the keys in the map I proposed

Changed the Discovery schema format to

{ "paths": { "apis/batch/v1": { "url": "/openapi/v3/apis/batch/v1?hash=XXX" } } }

Yeah, I don't disagree that having an object rather than a direct mapping gives us more flexibility in the future, but we shouldn't need the hash per-se.

pkg/handler3/handler.go

apelisse · 2022-03-16T02:51:49Z

pkg/handler3/handler.go

+		if err != nil {
+			return nil, err
+		}
+		discovery.Paths[k] = "openapi/v3/" + k + "?hash=" + unquoteETag(etag)


I don't disagree with that, but it should be a URL rather than the hash so it's built automatically (clients shouldn't even have to know what to do with the hash, it should just contain the URL they need to download as a relative link).

alexzielenski

Looks clean. Very minor nits

pkg/handler3/handler.go

pkg/handler3/handler_test.go

pkg/handler3/handler.go

apelisse · 2022-03-16T21:13:19Z

pkg/handler3/handler.go

+			if hash := r.URL.Query().Get("hash"); hash != "" && hash != etag {
+				u := constructURL(group, etag)
+				http.Redirect(w, r, u, 301)
+				return
+			}
+			applyCacheBusting(w, r, etag)


Piling up on Alex's comment, a lot of the conditions here are very similar to the conditions in applyCacheBusting. Try to remove the applyCacheBusting method below and move the code here, I think you'll be able to do a few other simplifications.

Since I don't code enough these days, I'll do it for fun. Not addressing the Vary comment.

Suggested change

if hash := r.URL.Query().Get("hash"); hash != "" && hash != etag {

u := constructURL(group, etag)

http.Redirect(w, r, u, 301)

return

}

applyCacheBusting(w, r, etag)

if hash := r.URL.Query().Get("hash"); hash != "" {

// The Vary header is required because the Accept header can

// change the contents returned. This prevents clients from caching

// protobuf as JSON and vice versa.

// Do you need Vary in the 301?!

w.Header().Set("Vary", "*")

if hash != etag {

u := constructURL(group, etag)

http.Redirect(w, r, u, 301)

return

}

// Only set these headers when a hash is given.

w.Header().Set("Cache-Control", "public, immutable")

// Set the Expires directive to the maximum value of one year from the request,

// effectively indicating that the cache never expires.

w.Header().Set("Expires", time.Now().AddDate(1, 0, 0).Format(time.RFC1123))

}

So we're not redirecting if no hash is given?

pkg/handler3/handler.go

apelisse · 2022-03-22T03:29:36Z

/lgtm
/approve

k8s-ci-robot · 2022-03-22T03:29:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, Jefftree

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~~ [apelisse]

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

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 10, 2022

k8s-ci-robot requested review from roycaihw and sttts March 10, 2022 19:51

Jefftree force-pushed the cache-busting branch from 6cad230 to 6b70b0f Compare March 10, 2022 20:35

k8s-ci-robot requested a review from apelisse March 10, 2022 20:37

Jefftree force-pushed the cache-busting branch from 6b70b0f to 8dda4bb Compare March 14, 2022 19:45

Jefftree changed the title ~~[WIP] Cache busting~~ Cache busting Mar 14, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 14, 2022

Jefftree force-pushed the cache-busting branch from 8dda4bb to 518886a Compare March 14, 2022 19:47

k8s-ci-robot assigned apelisse Mar 14, 2022

alexzielenski reviewed Mar 16, 2022

View reviewed changes

apelisse reviewed Mar 16, 2022

View reviewed changes

Jefftree force-pushed the cache-busting branch 5 times, most recently from ba6b055 to 768107d Compare March 16, 2022 18:45

alexzielenski reviewed Mar 16, 2022

View reviewed changes

pkg/handler3/handler.go Outdated Show resolved Hide resolved

pkg/handler3/handler.go Show resolved Hide resolved

Jefftree force-pushed the cache-busting branch from 768107d to c5dedbf Compare March 16, 2022 20:26

apelisse reviewed Mar 16, 2022

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 21, 2022

Jefftree force-pushed the cache-busting branch from 4c3fa0f to 8598807 Compare March 22, 2022 01:31

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 22, 2022

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 22, 2022

apelisse reviewed Mar 22, 2022

View reviewed changes

pkg/handler3/handler.go Show resolved Hide resolved

pkg/handler3/handler.go Show resolved Hide resolved

pkg/handler3/handler.go Outdated Show resolved Hide resolved

Jefftree force-pushed the cache-busting branch from 8598807 to cc7ba84 Compare March 22, 2022 03:24

Cache busting

b0d7548

Jefftree force-pushed the cache-busting branch from cc7ba84 to b0d7548 Compare March 22, 2022 03:26

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 22, 2022

k8s-ci-robot merged commit 89ac9db into kubernetes:master Mar 22, 2022

This was referenced Mar 23, 2022

Update kube-openapi kubernetes/kubernetes#108895

Merged

REQUEST: New membership for alexzielenski kubernetes/org#3343

Closed

Jefftree deleted the cache-busting branch March 21, 2023 15:39

-			if hash := r.URL.Query().Get("hash"); hash != "" && hash != etag {
-				u := constructURL(group, etag)
-				http.Redirect(w, r, u, 301)
-				return
-			}
-			applyCacheBusting(w, r, etag)
+			if hash := r.URL.Query().Get("hash"); hash != "" {
+				// The Vary header is required because the Accept header can
+				// change the contents returned. This prevents clients from caching
+				// protobuf as JSON and vice versa.
+				// Do you need Vary in the 301?!
+				w.Header().Set("Vary", "*")
+				if hash != etag {
+					u := constructURL(group, etag)
+					http.Redirect(w, r, u, 301)
+					return
+				}
+				// Only set these headers when a hash is given.
+				w.Header().Set("Cache-Control", "public, immutable")
+				// Set the Expires directive to the maximum value of one year from the request,
+				// effectively indicating that the cache never expires.
+				w.Header().Set("Expires", time.Now().AddDate(1, 0, 0).Format(time.RFC1123))
+			}

Uh oh!

Cache busting #284

Cache busting #284

Uh oh!

Conversation

Jefftree commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jefftree commented Mar 10, 2022

Uh oh!

Jefftree commented Mar 14, 2022

Uh oh!

k8s-ci-robot commented Mar 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefftree Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexzielenski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apelisse commented Mar 22, 2022

Uh oh!

k8s-ci-robot commented Mar 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Jefftree commented Mar 10, 2022 •

edited

Loading

Jefftree Mar 16, 2022 •

edited

Loading