From 9441fc783372b2170b347c7dd50f5f0088450021 Mon Sep 17 00:00:00 2001 From: Ian Davis Date: Mon, 28 Mar 2022 12:16:33 +0100 Subject: [PATCH 1/4] fix: report gateway http metrics only when response is successful --- core/corehttp/gateway_handler.go | 49 ++++++++++++++++++-- core/corehttp/gateway_handler_block.go | 11 +++-- core/corehttp/gateway_handler_unixfs_file.go | 12 ++--- 3 files changed, 58 insertions(+), 14 deletions(-) diff --git a/core/corehttp/gateway_handler.go b/core/corehttp/gateway_handler.go index eca2efff610..1827d750c30 100644 --- a/core/corehttp/gateway_handler.go +++ b/core/corehttp/gateway_handler.go @@ -33,8 +33,10 @@ const ( immutableCacheControl = "public, max-age=29030400, immutable" ) -var onlyAscii = regexp.MustCompile("[[:^ascii:]]") -var noModtime = time.Unix(0, 0) // disables Last-Modified header if passed as modtime +var ( + onlyAscii = regexp.MustCompile("[[:^ascii:]]") + noModtime = time.Unix(0, 0) // disables Last-Modified header if passed as modtime +) // HTML-based redirect for errors which can be recovered from, but we want // to provide hint to people that they should fix things on their end. @@ -93,6 +95,48 @@ func (sw *statusResponseWriter) WriteHeader(code int) { sw.ResponseWriter.WriteHeader(code) } +// ServeContent replies to the request using the content in the provided ReadSeeker +// and returns the status code written and any error encountered during a write. +// It wraps http.ServeContent which takes care of If-None-Match+Etag, +// Content-Length and range requests. +func ServeContent(w http.ResponseWriter, req *http.Request, name string, modtime time.Time, content io.ReadSeeker) (int, error) { + ew := &errRecordingResponseWriter{ResponseWriter: w} + http.ServeContent(ew, req, name, modtime, content) + return ew.code, ew.err +} + +// errRecordingResponseWriter wraps a ResponseWriter to record the status code and any write error. +type errRecordingResponseWriter struct { + http.ResponseWriter + code int + err error +} + +func (w *errRecordingResponseWriter) WriteHeader(code int) { + if w.code == 0 { + w.code = code + } + w.ResponseWriter.WriteHeader(code) +} + +func (w *errRecordingResponseWriter) Write(p []byte) (int, error) { + n, err := w.ResponseWriter.Write(p) + if err != nil && w.err == nil { + w.err = nil + } + return n, err +} + +// ReadFrom exposes errRecordingResponseWriter's underlying ResponseWriter to io.Copy +// to allow optimized methods to be taken advantage of. +func (w *errRecordingResponseWriter) ReadFrom(r io.Reader) (n int64, err error) { + n, err = io.Copy(w.ResponseWriter, r) + if err != nil && w.err == nil { + w.err = nil + } + return n, err +} + func newGatewaySummaryMetric(name string, help string) *prometheus.SummaryVec { summaryMetric := prometheus.NewSummaryVec( prometheus.SummaryOpts{ @@ -634,7 +678,6 @@ func addCacheControlHeaders(w http.ResponseWriter, r *http.Request, contentPath // TODO: set Cache-Control based on TTL of IPNS/DNSLink: https://github.com/ipfs/go-ipfs/issues/1818#issuecomment-1015849462 // TODO: set Last-Modified based on /ipns/ publishing timestamp? - } else { // immutable! CACHE ALL THE THINGS, FOREVER! wolololol w.Header().Set("Cache-Control", immutableCacheControl) diff --git a/core/corehttp/gateway_handler_block.go b/core/corehttp/gateway_handler_block.go index 13d7ebefd9f..b803e45ef16 100644 --- a/core/corehttp/gateway_handler_block.go +++ b/core/corehttp/gateway_handler_block.go @@ -33,10 +33,11 @@ func (i *gatewayHandler) serveRawBlock(w http.ResponseWriter, r *http.Request, b w.Header().Set("Content-Type", "application/vnd.ipld.raw") w.Header().Set("X-Content-Type-Options", "nosniff") // no funny business in the browsers :^) - // Done: http.ServeContent will take care of - // If-None-Match+Etag, Content-Length and range requests - http.ServeContent(w, r, name, modtime, content) + code, err := ServeContent(w, r, name, modtime, content) - // Update metrics - i.rawBlockGetMetric.WithLabelValues(contentPath.Namespace()).Observe(time.Since(begin).Seconds()) + // Was response successful? + if code/100 == 2 && err == nil { + // Update metrics + i.rawBlockGetMetric.WithLabelValues(contentPath.Namespace()).Observe(time.Since(begin).Seconds()) + } } diff --git a/core/corehttp/gateway_handler_unixfs_file.go b/core/corehttp/gateway_handler_unixfs_file.go index 9807969fee0..a4542aa4e5e 100644 --- a/core/corehttp/gateway_handler_unixfs_file.go +++ b/core/corehttp/gateway_handler_unixfs_file.go @@ -18,7 +18,6 @@ import ( // serveFile returns data behind a file along with HTTP headers based on // the file itself, its CID and the contentPath used for accessing it. func (i *gatewayHandler) serveFile(w http.ResponseWriter, r *http.Request, contentPath ipath.Path, fileCid cid.Cid, file files.File, begin time.Time) { - // Set Cache-Control and read optional Last-Modified time modtime := addCacheControlHeaders(w, r, contentPath, fileCid) @@ -78,10 +77,11 @@ func (i *gatewayHandler) serveFile(w http.ResponseWriter, r *http.Request, conte // special fixup around redirects w = &statusResponseWriter{w} - // Done: http.ServeContent will take care of - // If-None-Match+Etag, Content-Length and range requests - http.ServeContent(w, r, name, modtime, content) + code, err := ServeContent(w, r, name, modtime, content) - // Update metrics - i.unixfsFileGetMetric.WithLabelValues(contentPath.Namespace()).Observe(time.Since(begin).Seconds()) + // Was response successful? + if code/100 == 2 && err == nil { + // Update metrics + i.unixfsFileGetMetric.WithLabelValues(contentPath.Namespace()).Observe(time.Since(begin).Seconds()) + } } From a61fc04804d4d089cf795b035275646dcdf68388 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Wed, 6 Apr 2022 02:19:48 +0200 Subject: [PATCH 2/4] refactor: dataSent bool --- core/corehttp/gateway_handler.go | 12 +++++++++--- core/corehttp/gateway_handler_block.go | 7 ++++--- core/corehttp/gateway_handler_unixfs_file.go | 6 ++++-- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/core/corehttp/gateway_handler.go b/core/corehttp/gateway_handler.go index 5d71824f241..94473a54c2b 100644 --- a/core/corehttp/gateway_handler.go +++ b/core/corehttp/gateway_handler.go @@ -102,10 +102,16 @@ func (sw *statusResponseWriter) WriteHeader(code int) { // and returns the status code written and any error encountered during a write. // It wraps http.ServeContent which takes care of If-None-Match+Etag, // Content-Length and range requests. -func ServeContent(w http.ResponseWriter, req *http.Request, name string, modtime time.Time, content io.ReadSeeker) (int, error) { +func ServeContent(w http.ResponseWriter, req *http.Request, name string, modtime time.Time, content io.ReadSeeker) (int, bool, error) { ew := &errRecordingResponseWriter{ResponseWriter: w} http.ServeContent(ew, req, name, modtime, content) - return ew.code, ew.err + + // When we calculate some metrics we want a flag that lets us to ignore + // errors and 304 Not Modified, and only care when requested data + // was sent in full. + dataSent := ew.code/100 == 2 && ew.err == nil + + return ew.code, dataSent, ew.err } // errRecordingResponseWriter wraps a ResponseWriter to record the status code and any write error. @@ -445,7 +451,7 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request case "application/vnd.ipld.car": logger.Debugw("serving car stream", "path", contentPath) carVersion := formatParams["version"] - i.serveCar(w, r, resolvedPath, contentPath, carVersion, begin) + i.serveCar(w, r, resolvedPath, contentPath, carVersion, begin) return default: // catch-all for unsuported application/vnd.* err := fmt.Errorf("unsupported format %q", responseFormat) diff --git a/core/corehttp/gateway_handler_block.go b/core/corehttp/gateway_handler_block.go index 0bc35ab1140..afd553d3080 100644 --- a/core/corehttp/gateway_handler_block.go +++ b/core/corehttp/gateway_handler_block.go @@ -38,10 +38,11 @@ func (i *gatewayHandler) serveRawBlock(w http.ResponseWriter, r *http.Request, r w.Header().Set("Content-Type", "application/vnd.ipld.raw") w.Header().Set("X-Content-Type-Options", "nosniff") // no funny business in the browsers :^) - code, err := ServeContent(w, r, name, modtime, content) + // ServeContent will take care of + // If-None-Match+Etag, Content-Length and range requests + _, dataSent, _ := ServeContent(w, r, name, modtime, content) - // Was response successful? - if code/100 == 2 && err == nil { + if dataSent { // Update metrics i.rawBlockGetMetric.WithLabelValues(contentPath.Namespace()).Observe(time.Since(begin).Seconds()) } diff --git a/core/corehttp/gateway_handler_unixfs_file.go b/core/corehttp/gateway_handler_unixfs_file.go index bf0ce787542..2938c8f4883 100644 --- a/core/corehttp/gateway_handler_unixfs_file.go +++ b/core/corehttp/gateway_handler_unixfs_file.go @@ -82,10 +82,12 @@ func (i *gatewayHandler) serveFile(w http.ResponseWriter, r *http.Request, resol // special fixup around redirects w = &statusResponseWriter{w} - code, err := ServeContent(w, r, name, modtime, content) + // ServeContent will take care of + // If-None-Match+Etag, Content-Length and range requests + _, dataSent, _ := ServeContent(w, r, name, modtime, content) // Was response successful? - if code/100 == 2 && err == nil { + if dataSent { // Update metrics i.unixfsFileGetMetric.WithLabelValues(contentPath.Namespace()).Observe(time.Since(begin).Seconds()) } From f05dac5575ab014299bf8fdda567738056c25c33 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Wed, 6 Apr 2022 02:24:03 +0200 Subject: [PATCH 3/4] fix(ServeContent): store err --- core/corehttp/gateway_handler.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/corehttp/gateway_handler.go b/core/corehttp/gateway_handler.go index 94473a54c2b..58706b36568 100644 --- a/core/corehttp/gateway_handler.go +++ b/core/corehttp/gateway_handler.go @@ -131,7 +131,7 @@ func (w *errRecordingResponseWriter) WriteHeader(code int) { func (w *errRecordingResponseWriter) Write(p []byte) (int, error) { n, err := w.ResponseWriter.Write(p) if err != nil && w.err == nil { - w.err = nil + w.err = err } return n, err } @@ -141,7 +141,7 @@ func (w *errRecordingResponseWriter) Write(p []byte) (int, error) { func (w *errRecordingResponseWriter) ReadFrom(r io.Reader) (n int64, err error) { n, err = io.Copy(w.ResponseWriter, r) if err != nil && w.err == nil { - w.err = nil + w.err = err } return n, err } From afdbcd774b058f878905631138ebbc61fc64c44c Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Wed, 6 Apr 2022 18:27:57 +0200 Subject: [PATCH 4/4] fix(gw): 304 Not Modified as no-op This fix ensures we don't do any additional work when Etag match what user already has in their own cache. --- core/corehttp/gateway_handler.go | 3 ++- core/corehttp/gateway_handler_unixfs_dir.go | 7 ++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/core/corehttp/gateway_handler.go b/core/corehttp/gateway_handler.go index 58706b36568..b14b88739d2 100644 --- a/core/corehttp/gateway_handler.go +++ b/core/corehttp/gateway_handler.go @@ -410,7 +410,8 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request trace.SpanFromContext(r.Context()).SetAttributes(attribute.String("ResolvedPath", resolvedPath.String())) // Finish early if client already has matching Etag - if r.Header.Get("If-None-Match") == getEtag(r, resolvedPath.Cid()) { + ifNoneMatch := r.Header.Get("If-None-Match") + if ifNoneMatch == getEtag(r, resolvedPath.Cid()) || ifNoneMatch == getDirListingEtag(resolvedPath.Cid()) { w.WriteHeader(http.StatusNotModified) return } diff --git a/core/corehttp/gateway_handler_unixfs_dir.go b/core/corehttp/gateway_handler_unixfs_dir.go index e458e803076..15827713591 100644 --- a/core/corehttp/gateway_handler_unixfs_dir.go +++ b/core/corehttp/gateway_handler_unixfs_dir.go @@ -8,6 +8,7 @@ import ( "time" "github.com/dustin/go-humanize" + cid "github.com/ipfs/go-cid" files "github.com/ipfs/go-ipfs-files" "github.com/ipfs/go-ipfs/assets" "github.com/ipfs/go-ipfs/tracing" @@ -93,7 +94,7 @@ func (i *gatewayHandler) serveDirectory(w http.ResponseWriter, r *http.Request, // Generated dir index requires custom Etag (it may change between go-ipfs versions) if assets.BindataVersionHash != "" { - dirEtag := `"DirIndex-` + assets.BindataVersionHash + `_CID-` + resolvedPath.Cid().String() + `"` + dirEtag := getDirListingEtag(resolvedPath.Cid()) w.Header().Set("Etag", dirEtag) if r.Header.Get("If-None-Match") == dirEtag { w.WriteHeader(http.StatusNotModified) @@ -204,3 +205,7 @@ func (i *gatewayHandler) serveDirectory(w http.ResponseWriter, r *http.Request, // Update metrics i.unixfsGenDirGetMetric.WithLabelValues(contentPath.Namespace()).Observe(time.Since(begin).Seconds()) } + +func getDirListingEtag(dirCid cid.Cid) string { + return `"DirIndex-` + assets.BindataVersionHash + `_CID-` + dirCid.String() + `"` +}