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

x/pkgsite: remove HTML replacements #44210

Closed
jba opened this issue Feb 10, 2021 · 9 comments
Closed

x/pkgsite: remove HTML replacements #44210

jba opened this issue Feb 10, 2021 · 9 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. pkgsite
Milestone
pkgsite/unplanned

Comments

@jba
Copy link
Contributor

jba commented Feb 10, 2021

The pkg.go.dev frontend replaces placeholders in HTML in several circumstances:

  • To insert latest-version information into cached pages, so that it is fresh.
  • To display a redirect banner on the first page served after the redirect, and only that page. We don’t want to cache the banner.
  • To display the highest tagged major version of a module, if it is not the current version.
  • To display whether a module is deprecated or retracted (work in progress).

These replacements have three problems:

  1. They are unsafe, since they perform blind string replacement on HTML without any checks or sanitization. All served HTML should be generated via the safehtml package.
  2. The latest-version code performs extra database lookups on every request, duplicating work unnecessarily.
  3. They obscure control flow, because the replacements happen in a middleware layer far from the page-generating code.

Suggested Fixes

For the redirect banner, we can just avoid caching it. To generalize, we can have our caching middleware respect the outgoing Cache-Control header. https://golang.org/cl/290889 is a proof of concept. We also have to avoid serving from the cache when the cookie is set.

To ensure that latest-version information is fresh, we can adjust the cache TTLs (the time that a page remains cached). Any non-zero TTL can produce a stale page, but perfection here is misleading. It already takes a few minutes from the time someone pushes a new latest version to the proxy, to the time that the frontend can display that version.

Frontend fetch speeds up the process, making a module available in under 30 seconds in most cases. But it is only triggered on a 404, which isn’t cached. So visiting the same URL that was just fetched should result in a fresh page. (Visiting the unversioned URL after fetching the versioned one would involve the cache, which isn't ideal. At present I have no good solution for that.)

Experiments show that about two-thirds of cache hits occur in the first five minutes, so changing the TTL to 5 minutes should not affect the cache hit ratio signicantly.
@jba jba added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. pkgsite labels Feb 10, 2021
@jba jba added this to the pkgsite/unplanned milestone Feb 10, 2021
@jba jba self-assigned this Feb 10, 2021
@gopherbot
Copy link

Change https://golang.org/cl/290969 mentions this issue: internal/frontend: reduce cache TTLs to 10 minutes

@jba
Copy link
Contributor Author

jba commented Feb 10, 2021

We should also explore doing cache invalidation from the worker.

@jba jba mentioned this issue Feb 11, 2021
Closed
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 11, 2021
@jba
internal/frontend: reduce cache TTLs to 10 minutes
d223212 
Based on recent request patterns, most cache hits (94%) happen within
10 minutes. Lower all cache TTLs to 10 minutes to validate that the
cache hit ratio doesn't change significantly.

For golang/go#44210

Change-Id: Ia59b7059a04215e4fe4424a4a868bc561d490b86
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/290969
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/292070 mentions this issue: internal/frontend: separate 404 redirect tests

@gopherbot
Copy link

Change https://golang.org/cl/292429 mentions this issue: internal/cookie: fix error return bug

@gopherbot
Copy link

Change https://golang.org/cl/292449 mentions this issue: internal/frontend: test redirect banner

@gopherbot
Copy link

Change https://golang.org/cl/292489 mentions this issue: internal/frontend: handle redirect in main serving path

@gopherbot
Copy link

Change https://golang.org/cl/292492 mentions this issue: internal/middleware: remove latest-minor-class substitution

@gopherbot
Copy link

Change https://golang.org/cl/292491 mentions this issue: internal/middleware: remove latest-minor-version substitution

gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 16, 2021
@jba
internal/middleware: remove latest-minor-version substitution
0b4f745 
It was unused.

For golang/go#44210

Change-Id: I17250d27ab62217d8358bce29c2ea82353881e81
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/292491
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 16, 2021
@jba
internal/middleware: remove latest-minor-class substitution
9fad5fb 
Remove the $$ substitution for the latest-minor CSS class.
Instead, determine the class on the normal serving path.

This is possible because the worker invalidates the cache when
processing a new latest version, so pages will not be stale
even though they are cached.

For golang/go#44210

Change-Id: Ide18e8257369810cd98f0b7d4fb41b75af961b83
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/292492
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 16, 2021
@jba
internal/frontend: separate 404 redirect tests
460d23d 
Make a separate test for cases where not finding
a module results in a redirection.

Also, get the cookie value more directly,
instead of relying on cookie.Extract.

For golang/go#44210

Change-Id: Ibf4ae07802e60ff6c70c3ec8835a653cb84b2368
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/292070
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 17, 2021
@jba
internal/cookie: fix error return bug
72b59e4 
For golang/go#44210

Change-Id: I3e6ec4bf592f0129004837630bec139036a22099
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/292429
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 17, 2021
@jba
internal/frontend: test redirect banner
365e265 
Verify that a redirect banner is added when appropriate,
and is not cached.

For golang/go#44210

Change-Id: Ib9b8cd8c67502724aeb0dbc79b1d5ce6784a67a3
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/292449
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 17, 2021
@jba
internal/frontend: handle redirect in main serving path
7abdc20 
Handle the logic of the redirect banner in the ordinary serving path,
instead of using a middleware layer.

To make this work, we have to bypass the cache when the redirect
cookie is set, so we don't cache the page with the banner.

For golang/go#44210

Change-Id: I145f058373980ff9e5d3617529e849b25161b390
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/292489
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/293009 mentions this issue: internal/frontend: do latest-major-version logic in main serving path

@golang golang locked and limited conversation to collaborators Feb 19, 2022
@rsc rsc unassigned jba Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. pkgsite
Projects
None yet
Milestone
pkgsite/unplanned
Development

No branches or pull requests
2 participants
@gopherbot @jba