Cache TTL fixes

Rather than returning 404 responses for GET requests on cached data
that's older than our TTL, we want to ensure it gets refreshed by the
lookup service. It's only for our exists (HEAD) requests that we should
treat expired cache as non-existent.

internal/middleware/identify_instance.go

@@ -73,7 +73,7 @@ func IdentifyInstanceByIP(logger *zap.Logger, db *sqlx.DB) gin.HandlerFunc {
 		}
 
 		if instanceIPAddress != nil {
-			// We found the row, set the instnace ID into the gin context.
+			// We found the row, set the instance ID into the gin context.
 			c.Set(ContextKeyInstanceID, instanceIPAddress.InstanceID)
 		}
 	}

pkg/api/v1/router.go

@@ -8,11 +8,13 @@ import (
 	"reflect"
 	"strings"
 	"text/template"
+	"time"
 
 	"github.com/gin-gonic/gin"
 	"github.com/go-playground/validator/v10"
 	"github.com/google/uuid"
 	"github.com/jmoiron/sqlx"
+	"github.com/spf13/viper"
 	"go.uber.org/zap"
 
 	"go.hollow.sh/toolbox/ginjwt"
@@ -120,11 +122,25 @@ func (r *Router) getMetadata(c *gin.Context) (*models.InstanceMetadatum, error)
 		return nil, errNotFound
 	}
 
+	metadataNotFound := false
+
 	// We got an instance ID from the middleware, either because we could match
 	// the request IP to an ID, or the request itself provided the instance ID.
 	metadata, err := models.FindInstanceMetadatum(c.Request.Context(), r.DB, instanceID)
-
 	if err != nil && errors.Is(err, sql.ErrNoRows) {
+		metadataNotFound = true
+	}
+
+	// Treat metadata that's older than our TTL as non-existent, to ensure we
+	// retrieve it using the lookup service
+	cacheTTL := viper.GetDuration("cache_ttl")
+	if err == nil && metadata != nil && cacheTTL != 0 && metadata.UpdatedAt.Add(cacheTTL).Before(time.Now()) {
+		r.Logger.Sugar().Info("TTL exceeded for instance ", metadata.ID, " not returning cached metadata")
+
+		metadataNotFound = true
+	}
+
+	if metadataNotFound {
 		// We couldn't find an instance_metadata row for this instance ID. Try
 		// to fetch it from the upstream lookup service (if enabled and configured)
 		middleware.MetricMetadataCacheMiss.Inc()
@@ -168,11 +184,25 @@ func (r *Router) getUserdata(c *gin.Context) (*models.InstanceUserdatum, error)
 		return nil, errNotFound
 	}
 
+	userdataNotFound := false
+
 	// We got an instance ID from the middleware, either because we could match
 	// the request IP to an ID, or the request itself provided the instance ID.
 	userdata, err := models.FindInstanceUserdatum(c.Request.Context(), r.DB, instanceID)
-
 	if err != nil && errors.Is(err, sql.ErrNoRows) {
+		userdataNotFound = true
+	}
+
+	// Treat userdata that's older than our TTL as non-existent, to ensure we
+	// retrieve it using the lookup service
+	cacheTTL := viper.GetDuration("cache_ttl")
+	if err == nil && userdata != nil && cacheTTL != 0 && userdata.UpdatedAt.Add(cacheTTL).Before(time.Now()) {
+		r.Logger.Sugar().Info("TTL exceeded for instance ", userdata.ID, " not returning cached userdata")
+
+		userdataNotFound = true
+	}
+
+	if userdataNotFound {
 		// We couldn't find an instance_metadata row for this instance ID. Try
 		// to fetch it from the upstream lookup service (if enabled and configured)
 		if r.LookupEnabled && r.LookupClient != nil {

pkg/api/v1/router_instance_metadata.go

@@ -70,15 +70,6 @@ func (r *Router) instanceMetadataGet(c *gin.Context) {
 		return
 	}
 
-	// Treat metadata that's older then its TTL as non-existent
-	cacheTTL := viper.GetDuration("cache_ttl")
-	if metadata != nil && cacheTTL != 0 && metadata.UpdatedAt.Add(cacheTTL).Before(time.Now()) {
-		r.Logger.Sugar().Info("Metadata for ", metadata.ID, " is older than our TTL, ignoring")
-		notFoundResponse(c)
-
-		return
-	}
-
 	if metadata != nil {
 		augmentedMetadata, err := addTemplateFields(metadata.Metadata, r.TemplateFields)
 		if err != nil {
@@ -149,10 +140,10 @@ func (r *Router) instanceMetadataExistsInternal(c *gin.Context) {
 		return
 	}
 
-	// Treat metadata that's older then its TTL as non-existent
+	// Treat metadata that's older than our TTL as non-existent
 	cacheTTL := viper.GetDuration("cache_ttl")
 	if metadata != nil && cacheTTL != 0 && metadata.UpdatedAt.Add(cacheTTL).Before(time.Now()) {
-		r.Logger.Sugar().Info("Metadata for ", metadata.ID, " is older than our TTL, ignoring (exists check)")
+		r.Logger.Sugar().Info("TTL exceeded for instance ", metadata.ID, " not returning cached metadata (internal exists check)")
 		c.Status(http.StatusNotFound)
 
 		return
@@ -183,15 +174,6 @@ func (r *Router) instanceUserdataGet(c *gin.Context) {
 		return
 	}
 
-	// Treat userdata that's older then its TTL as non-existent
-	cacheTTL := viper.GetDuration("cache_ttl")
-	if userdata != nil && cacheTTL != 0 && userdata.UpdatedAt.Add(cacheTTL).Before(time.Now()) {
-		r.Logger.Sugar().Info("Userdata for ", userdata.ID, " is older than our TTL, ignoring")
-		notFoundResponse(c)
-
-		return
-	}
-
 	if userdata != nil {
 		c.String(http.StatusOK, string(userdata.Userdata.Bytes))
 	} else {
@@ -246,10 +228,10 @@ func (r *Router) instanceUserdataExistsInternal(c *gin.Context) {
 		return
 	}
 
-	// Treat userdata that's older then its TTL as non-existent
+	// Treat userdata that's older than our TTL as non-existent
 	cacheTTL := viper.GetDuration("cache_ttl")
 	if userdata != nil && cacheTTL != 0 && userdata.UpdatedAt.Add(cacheTTL).Before(time.Now()) {
-		r.Logger.Sugar().Info("Userdata for ", userdata.ID, " is older than our TTL, ignoring (exists check)")
+		r.Logger.Sugar().Info("TTL exceeded for instance ", userdata.ID, " not returning cached userdata (internal exists check)")
 		c.Status(http.StatusNotFound)
 
 		return

pkg/api/v1/router_instance_metadata_lookup_test.go

@@ -6,9 +6,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
-	"time"
 
-	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
 
 	"go.hollow.sh/metadataservice/internal/lookup"
@@ -72,40 +70,4 @@ func TestGetMetadataLookupByIP(t *testing.T) {
 			assert.Equal(t, testcase.expectedStatus, w.Code)
 		})
 	}
-
-	// Verify cache TTL settings are honored by setting the TTL to -1 second so we can avoid having to sleep
-	viper.Set("cache_ttl", -time.Second)
-
-	for _, testcase := range testCases {
-		t.Run(testcase.testName, func(t *testing.T) {
-			lookupClient.setResponse(testcase.instanceIP, testcase.lookupResponse)
-			w := httptest.NewRecorder()
-
-			req, _ := http.NewRequestWithContext(context.TODO(), http.MethodGet, v1api.GetMetadataPath(), nil)
-			req.RemoteAddr = net.JoinHostPort(testcase.instanceIP, "")
-			router.ServeHTTP(w, req)
-
-			if testcase.expectedStatus != http.StatusInternalServerError {
-				testcase.expectedStatus = http.StatusNotFound
-			}
-
-			assert.Equal(t, testcase.expectedStatus, w.Code)
-		})
-	}
-
-	// Setting cache_ttl to zero should disable the TTL behavior
-	viper.Set("cache_ttl", 0)
-
-	for _, testcase := range testCases {
-		t.Run(testcase.testName, func(t *testing.T) {
-			lookupClient.setResponse(testcase.instanceIP, testcase.lookupResponse)
-			w := httptest.NewRecorder()
-
-			req, _ := http.NewRequestWithContext(context.TODO(), http.MethodGet, v1api.GetMetadataPath(), nil)
-			req.RemoteAddr = net.JoinHostPort(testcase.instanceIP, "")
-			router.ServeHTTP(w, req)
-
-			assert.Equal(t, testcase.expectedStatus, w.Code)
-		})
-	}
 }

pkg/api/v1/router_instance_metadata_test.go

@@ -11,7 +11,9 @@ import (
 	"regexp"
 	"testing"
 	"text/template"
+	"time"
 
+	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
 
 	"go.hollow.sh/metadataservice/internal/dbtools"
@@ -696,6 +698,29 @@ func TestGetMetadataInternal(t *testing.T) {
 		})
 	}
 
+	// Verify cache TTL settings are honored by setting the TTL to -1 second so we can avoid having to sleep
+	viper.Set("cache_ttl", -time.Second)
+
+	// Now our HEAD requests should behave as if cached metadata no longer exists
+	for _, testcase := range testCases {
+		t.Run(testcase.testName, func(t *testing.T) {
+			w := httptest.NewRecorder()
+
+			req, _ := http.NewRequestWithContext(context.TODO(), http.MethodHead, v1api.GetInternalMetadataByIDPath(testcase.instanceID), nil)
+			router.ServeHTTP(w, req)
+
+			expectedStatus := testcase.expectedStatus
+			if expectedStatus == http.StatusOK {
+				// Flip the expected 200 responses to 404s
+				expectedStatus = http.StatusNotFound
+			}
+			assert.Equal(t, expectedStatus, w.Code)
+		})
+	}
+
+	// Disable cache for remaining tests
+	viper.Set("cache_ttl", 0)
+
 	// GET request tests
 	for _, testcase := range testCases {
 		t.Run(testcase.testName, func(t *testing.T) {

pkg/api/v1/router_instance_userdata_lookup_test.go

@@ -6,9 +6,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
-	"time"
 
-	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
 
 	"go.hollow.sh/metadataservice/internal/lookup"
@@ -72,40 +70,4 @@ func TestGetUserdataLookupByIP(t *testing.T) {
 			assert.Equal(t, testcase.expectedStatus, w.Code)
 		})
 	}
-
-	// Verify cache TTL settings are honored by setting the TTL to -1 second so we can avoid having to sleep
-	viper.Set("cache_ttl", -time.Second)
-
-	for _, testcase := range testCases {
-		t.Run(testcase.testName, func(t *testing.T) {
-			lookupClient.setResponse(testcase.instanceIP, testcase.lookupResponse)
-			w := httptest.NewRecorder()
-
-			req, _ := http.NewRequestWithContext(context.TODO(), http.MethodGet, v1api.GetUserdataPath(), nil)
-			req.RemoteAddr = net.JoinHostPort(testcase.instanceIP, "")
-			router.ServeHTTP(w, req)
-
-			if testcase.expectedStatus == http.StatusOK {
-				testcase.expectedStatus = http.StatusNotFound
-			}
-
-			assert.Equal(t, testcase.expectedStatus, w.Code)
-		})
-	}
-
-	// Setting cache_ttl to zero should disable the TTL behavior
-	viper.Set("cache_ttl", 0)
-
-	for _, testcase := range testCases {
-		t.Run(testcase.testName, func(t *testing.T) {
-			lookupClient.setResponse(testcase.instanceIP, testcase.lookupResponse)
-			w := httptest.NewRecorder()
-
-			req, _ := http.NewRequestWithContext(context.TODO(), http.MethodGet, v1api.GetUserdataPath(), nil)
-			req.RemoteAddr = net.JoinHostPort(testcase.instanceIP, "")
-			router.ServeHTTP(w, req)
-
-			assert.Equal(t, testcase.expectedStatus, w.Code)
-		})
-	}
 }

pkg/api/v1/router_instance_userdata_test.go

@@ -10,7 +10,9 @@ import (
 	"net/http/httptest"
 	"regexp"
 	"testing"
+	"time"
 
+	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
 
 	"go.hollow.sh/metadataservice/internal/dbtools"
@@ -476,6 +478,29 @@ func TestGetUserdataInternal(t *testing.T) {
 		})
 	}
 
+	// Verify cache TTL settings are honored by setting the TTL to -1 second so we can avoid having to sleep
+	viper.Set("cache_ttl", -time.Second)
+
+	// Now our HEAD requests should behave as if cached userdata no longer exists
+	for _, testcase := range testCases {
+		t.Run(testcase.testName, func(t *testing.T) {
+			w := httptest.NewRecorder()
+
+			req, _ := http.NewRequestWithContext(context.TODO(), http.MethodHead, v1api.GetInternalUserdataByIDPath(testcase.instanceID), nil)
+			router.ServeHTTP(w, req)
+
+			expectedStatus := testcase.expectedStatus
+			if expectedStatus == http.StatusOK {
+				// Flip the expected 200 responses to 404s
+				expectedStatus = http.StatusNotFound
+			}
+			assert.Equal(t, expectedStatus, w.Code)
+		})
+	}
+
+	// Disable cache for remaining tests
+	viper.Set("cache_ttl", 0)
+
 	// GET request tests
 	for _, testcase := range testCases {
 		t.Run(testcase.testName, func(t *testing.T) {