Support for skipping subtree revisions to increase read performance and reduce disk usage

integration/admin/admin_integration_test.go

@@ -220,8 +220,9 @@ func TestAdminServer_UpdateTree(t *testing.T) {
 		want.TreeId = tree.TreeId
 		want.CreateTime = tree.CreateTime
 		want.UpdateTime = tree.UpdateTime
+		want.StorageSettings = tree.StorageSettings
 		if !proto.Equal(tree, want) {
-			diff := cmp.Diff(tree, want)
+			diff := cmp.Diff(tree, want, cmp.Comparer(proto.Equal))
 			t.Errorf("%v: post-UpdateTree diff:\n%v", test.desc, diff)
 		}
 	}
@@ -357,7 +358,7 @@ func TestAdminServer_DeleteTree(t *testing.T) {
 		want.Deleted = true
 		want.DeleteTime = deletedTree.DeleteTime
 		if got := deletedTree; !proto.Equal(got, want) {
-			diff := cmp.Diff(got, want)
+			diff := cmp.Diff(got, want, cmp.Comparer(proto.Equal))
 			t.Errorf("%v: post-DeleteTree() diff (-got +want):\n%v", test.desc, diff)
 		}
 
@@ -366,7 +367,7 @@ func TestAdminServer_DeleteTree(t *testing.T) {
 			t.Fatalf("%v: GetTree() returned err = %v", test.desc, err)
 		}
 		if got, want := storedTree, deletedTree; !proto.Equal(got, want) {
-			diff := cmp.Diff(got, want)
+			diff := cmp.Diff(got, want, cmp.Comparer(proto.Equal))
 			t.Errorf("%v: post-GetTree() diff (-got +want):\n%v", test.desc, diff)
 		}
 	}
@@ -447,7 +448,7 @@ func TestAdminServer_UndeleteTree(t *testing.T) {
 			continue
 		}
 		if got, want := undeletedTree, createdTree; !proto.Equal(got, want) {
-			diff := cmp.Diff(got, want)
+			diff := cmp.Diff(got, want, cmp.Comparer(proto.Equal))
 			t.Errorf("%v: post-UndeleteTree() diff (-got +want):\n%v", test.desc, diff)
 		}
 
@@ -456,7 +457,7 @@ func TestAdminServer_UndeleteTree(t *testing.T) {
 			t.Fatalf("%v: GetTree() returned err = %v", test.desc, err)
 		}
 		if got, want := storedTree, createdTree; !proto.Equal(got, want) {
-			diff := cmp.Diff(got, want)
+			diff := cmp.Diff(got, want, cmp.Comparer(proto.Equal))
 			t.Errorf("%v: post-GetTree() diff (-got +want):\n%v", test.desc, diff)
 		}
 	}

storage/mysql/admin_storage.go

@@ -15,17 +15,21 @@
 package mysql
 
 import (
+	"bytes"
 	"context"
 	"database/sql"
+	"encoding/gob"
 	"fmt"
 	"sync"
 	"time"
 
 	"github.com/google/trillian"
 	"github.com/google/trillian/storage"
+	"github.com/google/trillian/storage/mysql/mysqlpb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/types/known/anypb"
 	"google.golang.org/protobuf/types/known/timestamppb"
 	"k8s.io/klog/v2"
 )
@@ -47,8 +51,8 @@ const (
 			Description,
 			CreateTimeMillis,
 			UpdateTimeMillis,
-			PrivateKey,
-			PublicKey,
+			PrivateKey, -- Unused
+			PublicKey, -- Used to store StorageSettings
 			MaxRootDurationMillis,
 			Deleted,
 			DeleteTimeMillis
@@ -62,7 +66,7 @@ const (
 )
 
 // NewAdminStorage returns a MySQL storage.AdminStorage implementation backed by DB.
-func NewAdminStorage(db *sql.DB) storage.AdminStorage {
+func NewAdminStorage(db *sql.DB) *mysqlAdminStorage {
 	return &mysqlAdminStorage{db}
 }
 
@@ -223,6 +227,39 @@ func (t *adminTX) CreateTree(ctx context.Context, tree *trillian.Tree) (*trillia
 	}
 	rootDuration := newTree.MaxRootDuration.AsDuration()
 
+	// When creating a new tree we automatically add StorageSettings to allow us to
+	// determine that this tree can support newer storage features. When reading
+	// trees that do not have this StorageSettings populated, it must be assumed that
+	// the tree was created with the oldest settings.
+	// The gist of this code is super simple: create a new StorageSettings with the most
+	// modern defaults if the created tree does not have one, and then create a struct that
+	// represents this to store in the DB. Unfortunately because this involves anypb, struct
+	// copies, marshalling, and proper error handling this turns into a scary amount of code.
+	if tree.StorageSettings != nil {
+		newTree.StorageSettings = proto.Clone(tree.StorageSettings).(*anypb.Any)
+	} else {
+		o := &mysqlpb.StorageOptions{
+			SubtreeRevisions: false, // Default behaviour for new trees is to skip writing subtree revisions.
+		}
+		a, err := anypb.New(o)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create new StorageOptions: %v", err)
+		}
+		newTree.StorageSettings = a
+	}
+	o := &mysqlpb.StorageOptions{}
+	if err := anypb.UnmarshalTo(newTree.StorageSettings, o, proto.UnmarshalOptions{}); err != nil {
+		return nil, fmt.Errorf("failed to unmarshal StorageOptions: %v", err)
+	}
+	ss := storageSettings{
+		Revisioned: o.SubtreeRevisions,
+	}
+	buff := &bytes.Buffer{}
+	enc := gob.NewEncoder(buff)
+	if err := enc.Encode(ss); err != nil {
+		return nil, fmt.Errorf("failed to encode storageSettings: %v", err)
+	}
+
 	insertTreeStmt, err := t.tx.PrepareContext(
 		ctx,
 		`INSERT INTO Trees(
@@ -236,8 +273,8 @@ func (t *adminTX) CreateTree(ctx context.Context, tree *trillian.Tree) (*trillia
 			Description,
 			CreateTimeMillis,
 			UpdateTimeMillis,
-			PrivateKey,
-			PublicKey,
+			PrivateKey, -- Unused
+			PublicKey, -- Used to store StorageSettings
 			MaxRootDurationMillis)
 		VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`)
 	if err != nil {
@@ -261,8 +298,8 @@ func (t *adminTX) CreateTree(ctx context.Context, tree *trillian.Tree) (*trillia
 		newTree.Description,
 		nowMillis,
 		nowMillis,
-		[]byte{}, // Unused, filling in for backward compatibility.
-		[]byte{}, // Unused, filling in for backward compatibility.
+		[]byte{},     // PrivateKey: Unused, filling in for backward compatibility.
+		buff.Bytes(), // Using the otherwise unused PublicKey for storing StorageSettings.
 		rootDuration/time.Millisecond,
 	)
 	if err != nil {
@@ -356,7 +393,10 @@ func (t *adminTX) UpdateTree(ctx context.Context, treeID int64, updateFunc func(
 		tree.Description,
 		nowMillis,
 		rootDuration/time.Millisecond,
-		[]byte{}, // Unused, filling in for backward compatibility.
+		[]byte{}, // PrivateKey: Unused, filling in for backward compatibility.
+		// PublicKey should not be updated with any storageSettings here without
+		// a lot of thought put into it. At the moment storageSettings are inferred
+		// when reading the tree, even if no value is stored in the database.
 		tree.TreeId); err != nil {
 		return nil, err
 	}
@@ -419,8 +459,21 @@ func validateDeleted(ctx context.Context, tx *sql.Tx, treeID int64, wantDeleted
 }
 
 func validateStorageSettings(tree *trillian.Tree) error {
-	if tree.StorageSettings != nil {
-		return fmt.Errorf("storage_settings not supported, but got %v", tree.StorageSettings)
+	if tree.StorageSettings.MessageIs(&mysqlpb.StorageOptions{}) {
+		return nil
 	}
-	return nil
+	if tree.StorageSettings == nil {
+		// No storage settings is OK, we'll just use the defaults for new trees
+		return nil
+	}
+	return fmt.Errorf("storage_settings must be nil or mysqlpb.StorageOptions, but got %v", tree.StorageSettings)
+}
+
+// storageSettings allows us to persist storage settings to the DB.
+// It is a tempting trap to use protos for this, but the way they encode
+// makes it impossible to tell the difference between no value ever written
+// and a value that was written with the default values for each field.
+// Using an explicit struct and gob encoding allows us to tell the difference.
+type storageSettings struct {
+	Revisioned bool
 }

storage/mysql/admin_storage_test.go

@@ -15,13 +15,16 @@
 package mysql
 
 import (
+	"bytes"
 	"context"
 	"database/sql"
+	"encoding/gob"
 	"fmt"
 	"testing"
 
 	"github.com/google/trillian"
 	"github.com/google/trillian/storage"
+	"github.com/google/trillian/storage/mysql/mysqlpb"
 	"github.com/google/trillian/storage/testonly"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/types/known/anypb"
@@ -128,12 +131,16 @@ func TestAdminTX_TreeWithNulls(t *testing.T) {
 	}
 }
 
-func TestAdminTX_StorageSettingsNotSupported(t *testing.T) {
+func TestAdminTX_StorageSettings(t *testing.T) {
 	cleanTestDB(DB)
 	s := NewAdminStorage(DB)
 	ctx := context.Background()
 
-	settings, err := anypb.New(&trillian.Tree{})
+	badSettings, err := anypb.New(&trillian.Tree{})
+	if err != nil {
+		t.Fatalf("Error marshaling proto: %v", err)
+	}
+	goodSettings, err := anypb.New(&mysqlpb.StorageOptions{})
 	if err != nil {
 		t.Fatalf("Error marshaling proto: %v", err)
 	}
@@ -142,16 +149,38 @@ func TestAdminTX_StorageSettingsNotSupported(t *testing.T) {
 		desc string
 		// fn attempts to either create or update a tree with a non-nil, valid Any proto
 		// on Tree.StorageSettings. It's expected to return an error.
-		fn func(storage.AdminStorage) error
+		fn      func(storage.AdminStorage) error
+		wantErr bool
 	}{
 		{
-			desc: "CreateTree",
+			desc: "CreateTree Bad Settings",
+			fn: func(s storage.AdminStorage) error {
+				tree := proto.Clone(testonly.LogTree).(*trillian.Tree)
+				tree.StorageSettings = badSettings
+				_, err := storage.CreateTree(ctx, s, tree)
+				return err
+			},
+			wantErr: true,
+		},
+		{
+			desc: "CreateTree nil Settings",
 			fn: func(s storage.AdminStorage) error {
 				tree := proto.Clone(testonly.LogTree).(*trillian.Tree)
-				tree.StorageSettings = settings
+				tree.StorageSettings = nil
 				_, err := storage.CreateTree(ctx, s, tree)
 				return err
 			},
+			wantErr: false,
+		},
+		{
+			desc: "CreateTree StorageOptions Settings",
+			fn: func(s storage.AdminStorage) error {
+				tree := proto.Clone(testonly.LogTree).(*trillian.Tree)
+				tree.StorageSettings = goodSettings
+				_, err := storage.CreateTree(ctx, s, tree)
+				return err
+			},
+			wantErr: false,
 		},
 		{
 			desc: "UpdateTree",
@@ -160,14 +189,93 @@ func TestAdminTX_StorageSettingsNotSupported(t *testing.T) {
 				if err != nil {
 					t.Fatalf("CreateTree() failed with err = %v", err)
 				}
-				_, err = storage.UpdateTree(ctx, s, tree.TreeId, func(tree *trillian.Tree) { tree.StorageSettings = settings })
+				_, err = storage.UpdateTree(ctx, s, tree.TreeId, func(tree *trillian.Tree) { tree.StorageSettings = badSettings })
 				return err
 			},
+			wantErr: true,
 		},
 	}
 	for _, test := range tests {
-		if err := test.fn(s); err == nil {
-			t.Errorf("%v: err = nil, want non-nil", test.desc)
+		if err := test.fn(s); (err != nil) != test.wantErr {
+			t.Errorf("err: %v, wantErr = %v", err, test.wantErr)
+		}
+	}
+}
+
+// Test reading variants of trees that could have been created by old versions
+// of Trillian to check we infer the correct storage options.
+func TestAdminTX_GetTreeLegacies(t *testing.T) {
+	cleanTestDB(DB)
+	s := NewAdminStorage(DB)
+	ctx := context.Background()
+
+	serializedStorageSettings := func(revisioned bool) []byte {
+		ss := storageSettings{
+			Revisioned: revisioned,
+		}
+		buff := &bytes.Buffer{}
+		enc := gob.NewEncoder(buff)
+		if err := enc.Encode(ss); err != nil {
+			t.Fatalf("failed to encode storageSettings: %v", err)
+		}
+		return buff.Bytes()
+	}
+	tests := []struct {
+		desc           string
+		key            []byte
+		wantRevisioned bool
+	}{
+		{
+			desc:           "No data",
+			key:            []byte{},
+			wantRevisioned: true,
+		},
+		{
+			desc:           "Public key",
+			key:            []byte("trustmethatthisisapublickey"),
+			wantRevisioned: true,
+		},
+		{
+			desc:           "StorageOptions revisioned",
+			key:            serializedStorageSettings(true),
+			wantRevisioned: true,
+		},
+		{
+			desc:           "StorageOptions revisionless",
+			key:            serializedStorageSettings(false),
+			wantRevisioned: false,
+		},
+	}
+	for _, tC := range tests {
+		// Create a tree with default settings, and then reach into the DB to override
+		// whatever was written into the persisted settings to align with the test case.
+		tree, err := storage.CreateTree(ctx, s, testonly.LogTree)
+		if err != nil {
+			t.Fatal(err)
+		}
+		// We are reaching really into the internals here, but it's the only way to set up
+		// archival state. Going through the Create/Update methods will change the storage
+		// options.
+		tx, err := s.db.BeginTx(ctx, nil /* opts */)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if _, err := tx.Exec("UPDATE Trees SET PublicKey = ? WHERE TreeId = ?", tC.key, tree.TreeId); err != nil {
+			t.Fatal(err)
+		}
+		if err := tx.Commit(); err != nil {
+			t.Fatal(err)
+		}
+		readTree, err := storage.GetTree(ctx, s, tree.TreeId)
+		if err != nil {
+			t.Fatal(err)
+		}
+		o := &mysqlpb.StorageOptions{}
+		if err := anypb.UnmarshalTo(readTree.StorageSettings, o, proto.UnmarshalOptions{}); err != nil {
+			t.Fatal(err)
+		}
+		if got, want := o.SubtreeRevisions, tC.wantRevisioned; got != want {
+			t.Errorf("%s SubtreeRevisions: got %t, wanted %t", tC.desc, got, want)
 		}
 	}
 }

storage/mysql/mysqlpb/gen.go

@@ -0,0 +1,18 @@
+// Copyright 2023 Google LLC. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package mysqlpb contains protobuf definitions used by the mysql implementation.
+package mysqlpb
+
+//go:generate protoc -I=. --go_out=paths=source_relative:. options.proto