From f698f0fd40a8e3a779b451e650d8893dc4207d73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Rold=C3=A1n=20Betancort?= Date: Wed, 6 Nov 2024 13:53:59 +0000 Subject: [PATCH 1/4] reproduce bug in HCLRevisionFromString when parsing zero logical clock While developing the test-case for `EmitImmediatelyStrategy` with CRDB, I detected something I wasn't expecting: when comparing the revision out of the datastore write, and what came out of Watch API, the revisions were different. We use `SHOW COMMIT TIMESTAMP` to retrieve a CRDB [transaction timestamp] (https://github.com/cockroachdb/cockroach/pull/80848). The value coming out of the `update` field in change streams had the same value, but the difference was the notation: one included the logical clock, the other didn't, but logical clocks were the same (zero): - revision generated out of the transaction in `readTransactionCommitRev` uses `NewForHLC(hlcNow)`, which takes a `Decimal`. This in turn calls `decimal.String()`, which returns a number stripped out of the decimal part if it's zero. - revision obtained out of the changefeeds uses `revisions.HLCRevisionFromString (details.Updated)`, and `details.Updated` always comes with the decimal part, even when it's zero Both timestamps were the same, but had a different string representation, and led to a different `HCLRevision` logical lock, hence `.Equal()` method failing. --- .../datastore/revisions/hlcrevision_test.go | 85 +++++++++++++++---- 1 file changed, 70 insertions(+), 15 deletions(-) diff --git a/internal/datastore/revisions/hlcrevision_test.go b/internal/datastore/revisions/hlcrevision_test.go index 07c6fa04bd..4c0e9217da 100644 --- 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,28 @@ 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) + withTimestamp.String() + 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 +116,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 +245,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", From 0c96d12faa666089f1800573c604cb842373b2c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Rold=C3=A1n=20Betancort?= Date: Wed, 6 Nov 2024 13:55:07 +0000 Subject: [PATCH 2/4] fix bug in HCLRevisionFromString when parsing zero logical clock --- .../proxy/schemacaching/watchingcache_test.go | 6 ++++-- .../datastore/revisions/commonrevision_test.go | 12 ++++++------ internal/datastore/revisions/hlcrevision.go | 15 ++++----------- internal/datastore/revisions/hlcrevision_test.go | 1 - 4 files changed, 14 insertions(+), 20 deletions(-) diff --git a/internal/datastore/proxy/schemacaching/watchingcache_test.go b/internal/datastore/proxy/schemacaching/watchingcache_test.go index 0e8ee2fc65..0e24f89fcf 100644 --- 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 index eccbe97717..a8607b59e5 100644 --- 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 index 20e0ae645e..06a9396f59 100644 --- 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 { @@ -65,6 +65,7 @@ func parseHLCRevisionString(revisionStr string) (datastore.Revision, error) { if err != nil { return datastore.NoRevision, spiceerrors.MustBugf("could not cast logicalclock to uint32: %v", err) } + return HLCRevision{timestamp, uintLogicalClock + logicalClockOffset}, nil } @@ -90,7 +91,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 +122,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 +131,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 index 4c0e9217da..735c5d7a47 100644 --- a/internal/datastore/revisions/hlcrevision_test.go +++ b/internal/datastore/revisions/hlcrevision_test.go @@ -80,7 +80,6 @@ func TestConstructForTimestamp(t *testing.T) { t.Run(strconv.Itoa(int(input)), func(t *testing.T) { rev := zeroHLC withTimestamp := rev.ConstructForTimestamp(input) - withTimestamp.String() require.Equal(t, output, withTimestamp.String()) require.Equal(t, input, withTimestamp.TimestampNanoSec()) }) From b787b02d62001e6dd4e9b0acc36feca4510a738c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Rold=C3=A1n=20Betancort?= Date: Wed, 6 Nov 2024 16:57:30 +0000 Subject: [PATCH 3/4] adds code and test to detect logical clock overflow we don't expect it to be possible to have CRDB generate 4B conflicts --- internal/datastore/revisions/hlcrevision.go | 6 +++++- internal/datastore/revisions/hlcrevision_test.go | 7 +++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/internal/datastore/revisions/hlcrevision.go b/internal/datastore/revisions/hlcrevision.go index 06a9396f59..62f1687c7b 100644 --- a/internal/datastore/revisions/hlcrevision.go +++ b/internal/datastore/revisions/hlcrevision.go @@ -56,11 +56,15 @@ 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) diff --git a/internal/datastore/revisions/hlcrevision_test.go b/internal/datastore/revisions/hlcrevision_test.go index 735c5d7a47..669022a1ad 100644 --- a/internal/datastore/revisions/hlcrevision_test.go +++ b/internal/datastore/revisions/hlcrevision_test.go @@ -289,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", From fa3c8138b955bacf7df528e9ed5b0839cddcf5d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Rold=C3=A1n=20Betancort?= Date: Wed, 6 Nov 2024 14:19:26 +0000 Subject: [PATCH 4/4] fix trivy ratelimit errors in CI --- .github/workflows/security.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/security.yaml b/.github/workflows/security.yaml index 82edb83552..e79343dd0b 100644 --- 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: