From 60e4f023d4274b86bf2bb73e9aadb7f9ffe16fb9 Mon Sep 17 00:00:00 2001
From: Anders Eknert <anders@eknert.com>
Date: Sat, 10 Apr 2021 13:48:15 +0200
Subject: [PATCH] Do not rely on ETag in 304 server response headers (#3362)

Though the HTTP standard says it should be there, a _lot_ of the HTTP server implementations out there ignores sending the provided ETag back in the 304 response. No wonder given how it would just be echoing what that client just provided.

This fixed bundle caching for at least:

* Nginx
* Azure
* Rego playground (nginx based)

..and probably many more.

Fixes #3361

Signed-off-by: Anders Eknert <anders@eknert.com>
---
 download/download.go      |  6 +++-
 download/download_test.go | 59 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/download/download.go b/download/download.go
index 30d2c30dee..6e5860cd95 100644
--- a/download/download.go
+++ b/download/download.go
@@ -189,7 +189,11 @@ func (d *Downloader) download(ctx context.Context, m metrics.Metrics) (*bundle.B
 		return nil, "", nil
 
 	case http.StatusNotModified:
-		return nil, resp.Header.Get("ETag"), nil
+		etag := resp.Header.Get("ETag")
+		if etag == "" {
+			etag = d.etag
+		}
+		return nil, etag, nil
 	case http.StatusNotFound:
 		return nil, "", fmt.Errorf("server replied with not found")
 	case http.StatusUnauthorized:
diff --git a/download/download_test.go b/download/download_test.go
index 1ca84a58bf..cd2ad06719 100644
--- a/download/download_test.go
+++ b/download/download_test.go
@@ -153,6 +153,48 @@ func TestFailureUnexpected(t *testing.T) {
 	}
 }
 
+func TestEtagInResponse(t *testing.T) {
+	ctx := context.Background()
+	fixture := newTestFixture(t)
+	fixture.server.etagInResponse = true
+	fixture.d = New(Config{}, fixture.client, "/bundles/test/bundle1").WithCallback(fixture.oneShot)
+	defer fixture.server.stop()
+
+	if fixture.d.etag != "" {
+		t.Fatalf("Expected empty downloader ETag but got %v", fixture.d.etag)
+	}
+
+	fixture.server.expEtag = "some etag value"
+
+	err := fixture.d.oneShot(ctx)
+	if err != nil {
+		t.Fatal("Unexpected:", err)
+	} else if len(fixture.updates) != 1 {
+		t.Fatal("expected update")
+	} else if fixture.d.etag != fixture.server.expEtag {
+		t.Fatalf("Expected downloader ETag %v but got %v", fixture.server.expEtag, fixture.d.etag)
+	}
+
+	if fixture.updates[0].Bundle == nil {
+		// 200 response on first request, bundle should be present
+		t.Errorf("Expected bundle in response")
+	}
+
+	err = fixture.d.oneShot(ctx)
+	if err != nil {
+		t.Fatal("Unexpected:", err)
+	} else if len(fixture.updates) != 2 {
+		t.Fatal("expected two updates")
+	} else if fixture.d.etag != fixture.server.expEtag {
+		t.Fatalf("Expected downloader ETag %v but got %v", fixture.server.expEtag, fixture.d.etag)
+	}
+
+	if fixture.updates[1].Bundle != nil {
+		// 304 response on second request, bundle should _not_ be present
+		t.Errorf("Expected no bundle in response")
+	}
+}
+
 type testFixture struct {
 	d                         *Downloader
 	client                    rest.Client
@@ -230,12 +272,13 @@ func (t *testFixture) oneShot(ctx context.Context, u Update) {
 }
 
 type testServer struct {
-	t       *testing.T
-	expCode int
-	expEtag string
-	expAuth string
-	bundles map[string]bundle.Bundle
-	server  *httptest.Server
+	t              *testing.T
+	expCode        int
+	expEtag        string
+	expAuth        string
+	bundles        map[string]bundle.Bundle
+	server         *httptest.Server
+	etagInResponse bool
 }
 
 func (t *testServer) handle(w http.ResponseWriter, r *http.Request) {
@@ -262,7 +305,9 @@ func (t *testServer) handle(w http.ResponseWriter, r *http.Request) {
 	if t.expEtag != "" {
 		etag := r.Header.Get("If-None-Match")
 		if etag == t.expEtag {
-			w.Header().Add("Etag", t.expEtag)
+			if t.etagInResponse {
+				w.Header().Add("Etag", t.expEtag)
+			}
 			w.WriteHeader(304)
 			return
 		}