raftlog: don't embed raftpb.Entry in Entry

pkg/kv/kvserver/debug_print.go

@@ -348,7 +348,7 @@ func tryRaftLogEntry(kv storage.MVCCKeyValue) (string, error) {
 	defer e.Release()
 
 	if len(e.Data) == 0 {
-		return fmt.Sprintf("%s: EMPTY\n", &e.Entry), nil
+		return fmt.Sprintf("%s: EMPTY\n", (*raftpb.Entry)(&e.RaftEntry)), nil
 	}
 	e.Data = nil
 	cmd := e.Cmd
@@ -366,7 +366,7 @@ func tryRaftLogEntry(kv storage.MVCCKeyValue) (string, error) {
 	}
 	cmd.WriteBatch = nil
 
-	return fmt.Sprintf("%s (ID %s) by %s\n%s\nwrite batch:\n%s", &e.Entry, e.ID, leaseStr, &cmd, wbStr), nil
+	return fmt.Sprintf("%s (ID %s) by %s\n%s\nwrite batch:\n%s", (*raftpb.Entry)(&e.RaftEntry), e.ID, leaseStr, &cmd, wbStr), nil
 }
 
 func tryTxn(kv storage.MVCCKeyValue) (string, error) {

pkg/kv/kvserver/raftlog/BUILD.bazel

@@ -30,6 +30,7 @@ go_library(
 go_test(
     name = "raftlog_test",
     srcs = [
+        "command_test.go",
         "encoding_test.go",
         "entry_bench_test.go",
         "entry_test.go",

pkg/kv/kvserver/raftlog/command_test.go

@@ -0,0 +1,28 @@
+// Copyright 2023 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package raftlog
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestReplicatedCmdString checks that ReplicatedCmd does not implement
+// fmt.Stringer. It is fine to implement this in the future, however previously
+// at one point it "accidentally" implemented fmt.Stringer via embedding, which
+// could hide information.
+func TestReplicatedCmdString(t *testing.T) {
+	var cmd interface{} = (*ReplicatedCmd)(nil)
+	_, ok := cmd.(fmt.Stringer)
+	require.False(t, ok)
+}

pkg/kv/kvserver/raftlog/encoding_test.go

@@ -62,7 +62,7 @@ func BenchmarkRaftAdmissionMetaOverhead(b *testing.B) {
 
 			encodingBuf := make([]byte, RaftCommandPrefixLen+raftAdmissionMeta.Size()+len(marshaledRaftCmd))
 			raftEnt := Entry{
-				Entry: raftpb.Entry{
+				RaftEntry: RaftEntry{
 					Term:  1,
 					Index: 1,
 					Type:  raftpb.EntryNormal,

pkg/kv/kvserver/raftlog/entry.go

@@ -67,11 +67,17 @@ func DecomposeRaftEncodingStandardOrSideloaded(data []byte) (kvserverbase.CmdIDK
 	return kvserverbase.CmdIDKey(data[1 : 1+RaftCommandIDLen]), data[1+RaftCommandIDLen:]
 }
 
+// RaftEntry is structurally identical to raftpb.Entry but does not implement any
+// of its methods (and in particular fmt.Stringer). This avoids leaking the String
+// method into structs that embed an Entry, where it would produce poor results
+// since it wouldn't reflect any additional fields.
+type RaftEntry raftpb.Entry
+
 // Entry contains data related to a raft log entry. This is the raftpb.Entry
 // itself but also all encapsulated data relevant for command application and
 // admission control.
 type Entry struct {
-	raftpb.Entry
+	RaftEntry
 	ID                kvserverbase.CmdIDKey // may be empty for zero Entry
 	Cmd               kvserverpb.RaftCommand
 	ConfChangeV1      *raftpb.ConfChange            // only set for config change
@@ -92,7 +98,7 @@ var entryPool = sync.Pool{
 // NewEntry populates an Entry from the provided raftpb.Entry.
 func NewEntry(ent raftpb.Entry) (*Entry, error) {
 	e := entryPool.Get().(*Entry)
-	*e = Entry{Entry: ent}
+	*e = Entry{RaftEntry: RaftEntry(ent)}
 	if err := e.load(); err != nil {
 		return nil, err
 	}
@@ -109,7 +115,7 @@ func NewEntryFromRawValue(b []byte) (*Entry, error) {
 
 	e := entryPool.Get().(*Entry)
 
-	if err := storage.MakeValue(meta).GetProto(&e.Entry); err != nil {
+	if err := storage.MakeValue(meta).GetProto((*raftpb.Entry)(&e.RaftEntry)); err != nil {
 		return nil, errors.Wrap(err, "unmarshalling raft Entry")
 	}
 
@@ -137,7 +143,7 @@ func raftEntryFromRawValue(b []byte) (raftpb.Entry, error) {
 }
 
 func (e *Entry) load() error {
-	typ, err := EncodingOf(e.Entry)
+	typ, err := EncodingOf(raftpb.Entry(e.RaftEntry))
 	if err != nil {
 		return err
 	}
@@ -154,10 +160,10 @@ func (e *Entry) load() error {
 	}
 	switch typ {
 	case EntryEncodingStandardWithAC, EntryEncodingSideloadedWithAC:
-		e.ID, raftCmdBytes = DecomposeRaftEncodingStandardOrSideloaded(e.Entry.Data)
+		e.ID, raftCmdBytes = DecomposeRaftEncodingStandardOrSideloaded(e.RaftEntry.Data)
 		e.ApplyAdmissionControl = true
 	case EntryEncodingStandardWithoutAC, EntryEncodingSideloadedWithoutAC:
-		e.ID, raftCmdBytes = DecomposeRaftEncodingStandardOrSideloaded(e.Entry.Data)
+		e.ID, raftCmdBytes = DecomposeRaftEncodingStandardOrSideloaded(e.RaftEntry.Data)
 	case EntryEncodingEmpty:
 		// Nothing to load, the empty raftpb.Entry is represented by a trivial
 		// Entry.
@@ -174,7 +180,7 @@ func (e *Entry) load() error {
 
 	if ccTarget != nil {
 		// Conf change - more unmarshaling to do.
-		if err := protoutil.Unmarshal(e.Entry.Data, ccTarget); err != nil {
+		if err := protoutil.Unmarshal(e.RaftEntry.Data, ccTarget); err != nil {
 			return errors.Wrap(err, "unmarshalling ConfChange")
 		}
 		e.ConfChangeContext = &kvserverpb.ConfChangeContext{}
@@ -207,7 +213,7 @@ func (e *Entry) ConfChange() raftpb.ConfChangeI {
 // log, i.e. it is the inverse to NewEntryFromRawBytes.
 func (e *Entry) ToRawBytes() ([]byte, error) {
 	var value roachpb.Value
-	if err := value.SetProto(&e.Entry); err != nil {
+	if err := value.SetProto((*raftpb.Entry)(&e.RaftEntry)); err != nil {
 		return nil, err
 	}
 

pkg/kv/kvserver/replica_application_state_machine_test.go

@@ -93,7 +93,7 @@ func TestReplicaStateMachineChangeReplicas(t *testing.T) {
 
 		// Stage a command with the ChangeReplicas trigger.
 		ent := &raftlog.Entry{
-			Entry: raftpb.Entry{
+			RaftEntry: raftlog.RaftEntry{
 				Index: r.mu.state.RaftAppliedIndex + 1,
 				Type:  raftpb.EntryConfChange,
 			},
@@ -197,7 +197,7 @@ func TestReplicaStateMachineRaftLogTruncationStronglyCoupled(t *testing.T) {
 		// Stage a command that truncates one raft log entry which we pretend has a
 		// byte size of 1.
 		ent := &raftlog.Entry{
-			Entry: raftpb.Entry{
+			RaftEntry: raftlog.RaftEntry{
 				Index: raftAppliedIndex + 1,
 				Type:  raftpb.EntryNormal,
 			},
@@ -316,7 +316,7 @@ func TestReplicaStateMachineRaftLogTruncationLooselyCoupled(t *testing.T) {
 			// Stage a command that truncates one raft log entry which we pretend has a
 			// byte size of 1.
 			ent := &raftlog.Entry{
-				Entry: raftpb.Entry{
+				RaftEntry: raftlog.RaftEntry{
 					Index: raftAppliedIndex + 1,
 					Type:  raftpb.EntryNormal,
 				},
@@ -447,7 +447,7 @@ func TestReplicaStateMachineEphemeralAppBatchRejection(t *testing.T) {
 	for _, s := range []string{"earlier", "later"} {
 		req, repr := descWriteRepr(s)
 		ent := &raftlog.Entry{
-			Entry: raftpb.Entry{
+			RaftEntry: raftlog.RaftEntry{
 				Index: raftAppliedIndex + 1,
 				Type:  raftpb.EntryNormal,
 			},