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

Add caching headers to all responses #101

Merged
merged 2 commits into from
Sep 11, 2019
Merged

Add caching headers to all responses #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e0ef98f
Add caching headers to all responses
ruflin Sep 11, 2019
a857ab3
add pull request id
ruflin Sep 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Added

* Add validation check that Kibana min/max are valid semver versions. [#99](https://github.com/elastic/integrations-registry/pull/99)
* Adding Cache-Control max-age headers to all http responses set to 1h. [#101](https://github.com/elastic/integrations-registry/pull/101)

### Changed

Expand Down
1 change: 1 addition & 0 deletions categories.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ type Category struct {
// categoriesHandler is a dynamic handler as it will also allow filtering in the future.
func categoriesHandler() func(w http.ResponseWriter, r *http.Request) {
return func(w http.ResponseWriter, r *http.Request) {
cacheHeaders(w)

packagePaths, err := util.GetPackagePaths(packagesBasePath)
if err != nil {
Expand Down
6 changes: 6 additions & 0 deletions handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ func notFound(w http.ResponseWriter, err error) {

func catchAll(publicPath string) func(w http.ResponseWriter, r *http.Request) {
return func(w http.ResponseWriter, r *http.Request) {
cacheHeaders(w)

path := r.RequestURI

Expand Down Expand Up @@ -86,3 +87,8 @@ func sendHeader(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
}
}

func cacheHeaders(w http.ResponseWriter) {
w.Header().Add("Cache-Control", "max-age="+cacheTime)
w.Header().Add("Cache-Control", "public")
}
Copy link

Choose a reason for hiding this comment

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

These are a good start and Good Enough to merge, IMO.

However, I think adding a time-based (Last-Modified) or content-based (ETag) header would be a good improvement because it allows the client to reuse a response which is older than cacheTime but which hasn't changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Last-Modified will be a bit tricky as the data is generated and it is not clear to me what we would set it to. I was thinking about the ETag quite a bit and was not sure if it will be beneficial if we have a very long cache time. We could use the file sha1 as the etag for example.

Copy link

Choose a reason for hiding this comment

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

I think there are reasonable answers to those questions, but let's avoid them for now and focus on the max-age value.

I don't think the registry should use the same caching policy for all responses.

/package seems well-suited for a "far future" expiration date. e.g. a year or more. The assets associated with a [email protected] should never change. AFAIK, the registry won't allow force push/republish, so any change should be a new version. That means the expiration should be able to be well into the future without any issues.

/search & /categories are dynamic and should have the expirations set to whatever we feel is Fast Enough and/or doesn't overwhelm the registry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM and agree on the different caching times. I expect we will have in the future a CDN in front so the total number of request on the service itself should become pretty low.

2 changes: 2 additions & 0 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"net/http"
"os"
"os/signal"
"strconv"
"syscall"

ucfgYAML "github.com/elastic/go-ucfg/yaml"
Expand All @@ -22,6 +23,7 @@ var (
packagesBasePath string
address string
configPath = "config.yml"
cacheTime = strconv.Itoa(60 * 60) // 1 hour
)

func init() {
Expand Down
1 change: 1 addition & 0 deletions main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,4 +79,5 @@ func runEndpoint(t *testing.T, endpoint, path, file string, handler func(w http.
}

assert.Equal(t, string(data), recorder.Body.String())
assert.Equal(t, recorder.Header()["Cache-Control"], []string{"max-age=" + cacheTime, "public"})
}
1 change: 1 addition & 0 deletions search.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

func searchHandler() func(w http.ResponseWriter, r *http.Request) {
return func(w http.ResponseWriter, r *http.Request) {
cacheHeaders(w)

query := r.URL.Query()

Expand Down