Merge pull request #2122 from authzed/hcl-zero-fix

fix HCLRevision mismatch on zero logical clock
authzed · Nov 6, 2024 · 554777d · 554777d
2 parents 20f450c + fa3c813
commit 554777d
Show file tree

Hide file tree

Showing 5 changed files with 98 additions and 35 deletions.
diff --git a/.github/workflows/security.yaml b/.github/workflows/security.yaml
@@ -67,6 +67,9 @@ jobs:
           format: "table"
           exit-code: "1"
           severity: "CRITICAL,HIGH,MEDIUM"
+        env:
+          TRIVY_DB_REPOSITORY: "public.ecr.aws/aquasecurity/trivy-db"
+          TRIVY_JAVA_DB_REPOSITORY: "public.ecr.aws/aquasecurity/trivy-java-db"
       - uses: "goreleaser/goreleaser-action@v6"
         id: "goreleaser"
         with:

diff --git a/internal/datastore/proxy/schemacaching/watchingcache_test.go b/internal/datastore/proxy/schemacaching/watchingcache_test.go
@@ -251,11 +251,13 @@ func TestWatchingCacheFallbackToStandardCache(t *testing.T) {
 	require.NoError(t, wcache.startSync(context.Background()))
 
 	// Ensure the namespace is not found, but is cached in the fallback caching layer.
-	_, _, err = wcache.SnapshotReader(rev("1")).ReadNamespaceByName(context.Background(), "somenamespace")
+	r := rev("1")
+	_, _, err = wcache.SnapshotReader(r).ReadNamespaceByName(context.Background(), "somenamespace")
 	require.ErrorAs(t, err, &datastore.ErrNamespaceNotFound{})
 	require.False(t, wcache.namespaceCache.inFallbackMode)
 
-	entry, ok := c.Get("n:somenamespace@1")
+	expectedKey := cache.StringKey("n:somenamespace@" + r.String())
+	entry, ok := c.Get(expectedKey)
 	require.True(t, ok)
 	require.NotNil(t, entry.notFound)
 

diff --git a/internal/datastore/revisions/commonrevision_test.go b/internal/datastore/revisions/commonrevision_test.go
@@ -151,7 +151,7 @@ func TestRevisionComparison(t *testing.T) {
 
 func TestRevisionBidirectionalParsing(t *testing.T) {
 	tcs := []string{
-		"1", "2", "42", "192747564535", "1.0000000004", "1.0000000002", "1.0000000042", "-1235",
+		"1.0000000000", "2.0000000000", "42.0000000000", "192747564535.0000000000", "1.0000000004", "1.0000000002", "1.0000000042", "-1235.0000000000",
 	}
 
 	for _, tc := range tcs {
@@ -223,11 +223,11 @@ func TestTransactionIDRevisionParsing(t *testing.T) {
 
 func TestHLCRevisionParsing(t *testing.T) {
 	tcs := map[string]bool{
-		"1":                              false,
-		"2":                              false,
-		"42":                             false,
-		"1257894000000000000":            false,
-		"-1":                             false,
+		"1.0000000000":                   false,
+		"2.0000000000":                   false,
+		"42.0000000000":                  false,
+		"1257894000000000000.0000000000": false,
+		"-1.0000000000":                  false,
 		"1.0000000004":                   false,
 		"9223372036854775807.0000000004": false,
 	}

diff --git a/internal/datastore/revisions/hlcrevision.go b/internal/datastore/revisions/hlcrevision.go
@@ -39,7 +39,7 @@ func parseHLCRevisionString(revisionStr string) (datastore.Revision, error) {
 		if err != nil {
 			return datastore.NoRevision, fmt.Errorf("invalid revision string: %q", revisionStr)
 		}
-		return HLCRevision{timestamp, 0}, nil
+		return HLCRevision{timestamp, logicalClockOffset}, nil
 	}
 
 	if len(pieces) != 2 {
@@ -56,15 +56,20 @@ func parseHLCRevisionString(revisionStr string) (datastore.Revision, error) {
 	}
 
 	paddedLogicalClockStr := pieces[1] + strings.Repeat("0", logicalClockLength-len(pieces[1]))
-	logicalclock, err := strconv.ParseUint(paddedLogicalClockStr, 10, 32)
+	logicalclock, err := strconv.ParseUint(paddedLogicalClockStr, 10, 64)
 	if err != nil {
 		return datastore.NoRevision, fmt.Errorf("invalid revision string: %q", revisionStr)
 	}
 
+	if logicalclock > math.MaxUint32 {
+		return datastore.NoRevision, spiceerrors.MustBugf("received logical lock that exceeds MaxUint32 (%d > %d): revision %q", logicalclock, math.MaxUint32, revisionStr)
+	}
+
 	uintLogicalClock, err := safecast.ToUint32(logicalclock)
 	if err != nil {
 		return datastore.NoRevision, spiceerrors.MustBugf("could not cast logicalclock to uint32: %v", err)
 	}
+
 	return HLCRevision{timestamp, uintLogicalClock + logicalClockOffset}, nil
 }
 
@@ -90,7 +95,7 @@ func NewForHLC(decimal decimal.Decimal) (HLCRevision, error) {
 
 // NewHLCForTime creates a new revision for the given time.
 func NewHLCForTime(time time.Time) HLCRevision {
-	return HLCRevision{time.UnixNano(), 0}
+	return HLCRevision{time.UnixNano(), logicalClockOffset}
 }
 
 func (hlc HLCRevision) Equal(rhs datastore.Revision) bool {
@@ -121,10 +126,6 @@ func (hlc HLCRevision) LessThan(rhs datastore.Revision) bool {
 }
 
 func (hlc HLCRevision) String() string {
-	if hlc.logicalclock == 0 {
-		return strconv.FormatInt(hlc.time, 10)
-	}
-
 	logicalClockString := strconv.FormatInt(int64(hlc.logicalclock)-int64(logicalClockOffset), 10)
 	return strconv.FormatInt(hlc.time, 10) + "." + strings.Repeat("0", logicalClockLength-len(logicalClockString)) + logicalClockString
 }
@@ -134,15 +135,11 @@ func (hlc HLCRevision) TimestampNanoSec() int64 {
 }
 
 func (hlc HLCRevision) InexactFloat64() float64 {
-	if hlc.logicalclock == 0 {
-		return float64(hlc.time)
-	}
-
 	return float64(hlc.time) + float64(hlc.logicalclock-logicalClockOffset)/math.Pow10(logicalClockLength)
 }
 
 func (hlc HLCRevision) ConstructForTimestamp(timestamp int64) WithTimestampRevision {
-	return HLCRevision{timestamp, 0}
+	return HLCRevision{timestamp, logicalClockOffset}
 }
 
 func (hlc HLCRevision) AsDecimal() (decimal.Decimal, error) {

diff --git a/internal/datastore/revisions/hlcrevision_test.go b/internal/datastore/revisions/hlcrevision_test.go
@@ -1,34 +1,43 @@
 package revisions
 
 import (
+	"strconv"
 	"testing"
 	"time"
 
+	"github.com/authzed/spicedb/pkg/datastore"
+
 	"github.com/shopspring/decimal"
 	"github.com/stretchr/testify/require"
 )
 
 func TestNewForHLC(t *testing.T) {
-	tcs := []string{
-		"1",
-		"2",
-		"42",
-		"1257894000000000000",
-		"-1",
-		"1.0000000023",
-		"1703283409994227985.0000000004",
-		"1703283409994227985.0000000040",
-		"1703283409994227985.0010000000",
+	tcs := map[string]string{
+		"1":                              "1.0000000000",
+		"2":                              "2.0000000000",
+		"42":                             "42.0000000000",
+		"1257894000000000000":            "1257894000000000000.0000000000",
+		"-1":                             "-1.0000000000",
+		"1.0000000023":                   "1.0000000023",
+		"1703283409994227985.0000000004": "1703283409994227985.0000000004",
+		"1703283409994227985.0000000040": "1703283409994227985.0000000040",
+		"1703283409994227985.0010000000": "1703283409994227985.0010000000",
+		"1730898575294981085.0000000000": "1730898575294981085.0000000000",
 	}
 
-	for _, tc := range tcs {
-		t.Run(tc, func(t *testing.T) {
-			d, err := decimal.NewFromString(tc)
+	for inputTimestamp, expectedTimestamp := range tcs {
+		t.Run(inputTimestamp, func(t *testing.T) {
+			d, err := decimal.NewFromString(inputTimestamp)
 			require.NoError(t, err)
 
 			rev, err := NewForHLC(d)
 			require.NoError(t, err)
-			require.Equal(t, tc, rev.String())
+			revFromString, err := HLCRevisionFromString(inputTimestamp)
+			require.NoError(t, err)
+			require.True(t, rev.Equal(revFromString), "expected equal, got %v and %v", rev, revFromString)
+
+			require.Equal(t, expectedTimestamp, rev.String())
+			require.Equal(t, expectedTimestamp, revFromString.String())
 		})
 	}
 }
@@ -56,6 +65,27 @@ func TestTimestampNanoSec(t *testing.T) {
 	}
 }
 
+func TestConstructForTimestamp(t *testing.T) {
+	tcs := map[int64]string{
+		1:                   "1.0000000000",
+		2:                   "2.0000000000",
+		42:                  "42.0000000000",
+		1257894000000000000: "1257894000000000000.0000000000",
+		-1:                  "-1.0000000000",
+		9223372036854775807: "9223372036854775807.0000000000",
+		1703283409994227985: "1703283409994227985.0000000000",
+	}
+
+	for input, output := range tcs {
+		t.Run(strconv.Itoa(int(input)), func(t *testing.T) {
+			rev := zeroHLC
+			withTimestamp := rev.ConstructForTimestamp(input)
+			require.Equal(t, output, withTimestamp.String())
+			require.Equal(t, input, withTimestamp.TimestampNanoSec())
+		})
+	}
+}
+
 func TestInexactFloat64(t *testing.T) {
 	tcs := map[string]float64{
 		"1":                              1,
@@ -85,10 +115,18 @@ func TestInexactFloat64(t *testing.T) {
 
 func TestNewHLCForTime(t *testing.T) {
 	time := time.Now()
-	rev := NewForTime(time)
+	rev := NewHLCForTime(time)
 	require.Equal(t, time.UnixNano(), rev.TimestampNanoSec())
 }
 
+func TestNoRevision(t *testing.T) {
+	rev, err := HLCRevisionFromString("0")
+	require.NoError(t, err)
+	require.False(t, rev.Equal(datastore.NoRevision))
+	require.True(t, rev.GreaterThan(datastore.NoRevision))
+	require.False(t, rev.LessThan(datastore.NoRevision))
+}
+
 func TestHLCKeyEquals(t *testing.T) {
 	tcs := []struct {
 		left    string
@@ -206,6 +244,22 @@ func TestHLCKeyLessThanFunc(t *testing.T) {
 	}
 }
 
+func TestHLCFromStringError(t *testing.T) {
+	tcs := map[string]string{
+		"1a":    "invalid revision string",
+		"1.0.0": "invalid revision string",
+		"1a.0":  "invalid revision string",
+		"1.0a":  "invalid revision string",
+	}
+
+	for tc, expectedErr := range tcs {
+		t.Run(tc, func(t *testing.T) {
+			_, err := HLCRevisionFromString(tc)
+			require.ErrorContains(t, err, expectedErr)
+		})
+	}
+}
+
 func TestHLCToFromDecimal(t *testing.T) {
 	tcs := []string{
 		"1",
@@ -235,6 +289,13 @@ func TestHLCToFromDecimal(t *testing.T) {
 	}
 }
 
+func TestFailsIfLogicalClockExceedsMaxUin32(t *testing.T) {
+	expectedError := "received logical lock that exceeds MaxUint32 (9999999999 > 4294967295): revision \"0.9999999999\""
+	require.PanicsWithValue(t, expectedError, func() {
+		_, _ = HLCRevisionFromString("0.9999999999")
+	})
+}
+
 func BenchmarkHLCParsing(b *testing.B) {
 	tcs := []string{
 		"1",