ipn: avoid useless no-op WriteState calls

Rather than make each ipn.StateStore implementation guard against
useless writes (a write of the same value that's already in the
store), do writes via a new wrapper that has a fast path for the
unchanged case.

This then fixes profileManager's flood of useless writes to AWS SSM,
etc.

Updates #8785

Signed-off-by: Brad Fitzpatrick <[email protected]>

ipn/ipnlocal/cert.go

@@ -339,11 +339,11 @@ func (s certStateStore) Read(domain string, now time.Time) (*TLSCertKeyPair, err
 }
 
 func (s certStateStore) WriteCert(domain string, cert []byte) error {
-	return s.WriteState(ipn.StateKey(domain+".crt"), cert)
+	return ipn.WriteState(s.StateStore, ipn.StateKey(domain+".crt"), cert)
 }
 
 func (s certStateStore) WriteKey(domain string, key []byte) error {
-	return s.WriteState(ipn.StateKey(domain+".key"), key)
+	return ipn.WriteState(s.StateStore, ipn.StateKey(domain+".key"), key)
 }
 
 func (s certStateStore) ACMEKey() ([]byte, error) {
@@ -351,7 +351,7 @@ func (s certStateStore) ACMEKey() ([]byte, error) {
 }
 
 func (s certStateStore) WriteACMEKey(key []byte) error {
-	return s.WriteState(ipn.StateKey(acmePEMName), key)
+	return ipn.WriteState(s.StateStore, ipn.StateKey(acmePEMName), key)
 }
 
 // TLSCertKeyPair is a TLS public and private key, and whether they were obtained

ipn/ipnlocal/local.go

@@ -2209,7 +2209,7 @@ func (b *LocalBackend) initMachineKeyLocked() (err error) {
 	}
 
 	keyText, _ = b.machinePrivKey.MarshalText()
-	if err := b.store.WriteState(ipn.MachineKeyStateKey, keyText); err != nil {
+	if err := ipn.WriteState(b.store, ipn.MachineKeyStateKey, keyText); err != nil {
 		b.logf("error writing machine key to store: %v", err)
 		return err
 	}
@@ -2224,7 +2224,7 @@ func (b *LocalBackend) initMachineKeyLocked() (err error) {
 //
 // b.mu must be held.
 func (b *LocalBackend) clearMachineKeyLocked() error {
-	if err := b.store.WriteState(ipn.MachineKeyStateKey, nil); err != nil {
+	if err := ipn.WriteState(b.store, ipn.MachineKeyStateKey, nil); err != nil {
 		return err
 	}
 	b.machinePrivKey = key.MachinePrivate{}
@@ -4830,7 +4830,7 @@ func (b *LocalBackend) SetDevStateStore(key, value string) error {
 	if b.store == nil {
 		return errors.New("no state store")
 	}
-	err := b.store.WriteState(ipn.StateKey(key), []byte(value))
+	err := ipn.WriteState(b.store, ipn.StateKey(key), []byte(value))
 	b.logf("SetDevStateStore(%q, %q) = %v", key, value, err)
 
 	if err != nil {

ipn/ipnlocal/profiles.go

@@ -51,6 +51,10 @@ func (pm *profileManager) dlogf(format string, args ...any) {
 	pm.logf(format, args...)
 }
 
+func (pm *profileManager) WriteState(id ipn.StateKey, val []byte) error {
+	return ipn.WriteState(pm.store, id, val)
+}
+
 // CurrentUserID returns the current user ID. It is only non-empty on
 // Windows where we have a multi-user system.
 func (pm *profileManager) CurrentUserID() ipn.WindowsUserID {
@@ -182,9 +186,9 @@ func (pm *profileManager) setUnattendedModeAsConfigured() error {
 	}
 
 	if pm.prefs.ForceDaemon() {
-		return pm.store.WriteState(ipn.ServerModeStartKey, []byte(pm.currentProfile.Key))
+		return pm.WriteState(ipn.ServerModeStartKey, []byte(pm.currentProfile.Key))
 	} else {
-		return pm.store.WriteState(ipn.ServerModeStartKey, nil)
+		return pm.WriteState(ipn.ServerModeStartKey, nil)
 	}
 }
 
@@ -288,7 +292,7 @@ func (pm *profileManager) writePrefsToStore(key ipn.StateKey, prefs ipn.PrefsVie
 	if key == "" {
 		return nil
 	}
-	if err := pm.store.WriteState(key, prefs.ToBytes()); err != nil {
+	if err := pm.WriteState(key, prefs.ToBytes()); err != nil {
 		pm.logf("WriteState(%q): %v", key, err)
 		return err
 	}
@@ -336,7 +340,7 @@ func (pm *profileManager) SwitchProfile(id ipn.ProfileID) error {
 
 func (pm *profileManager) setAsUserSelectedProfileLocked() error {
 	k := ipn.CurrentProfileKey(string(pm.currentUserID))
-	return pm.store.WriteState(k, []byte(pm.currentProfile.Key))
+	return pm.WriteState(k, []byte(pm.currentProfile.Key))
 }
 
 func (pm *profileManager) loadSavedPrefs(key ipn.StateKey) (ipn.PrefsView, error) {
@@ -394,7 +398,7 @@ func (pm *profileManager) DeleteProfile(id ipn.ProfileID) error {
 	if kp.ID == pm.currentProfile.ID {
 		pm.NewProfile()
 	}
-	if err := pm.store.WriteState(kp.Key, nil); err != nil {
+	if err := pm.WriteState(kp.Key, nil); err != nil {
 		return err
 	}
 	delete(pm.knownProfiles, id)
@@ -407,7 +411,7 @@ func (pm *profileManager) DeleteAllProfiles() error {
 	metricDeleteAllProfile.Add(1)
 
 	for _, kp := range pm.knownProfiles {
-		if err := pm.store.WriteState(kp.Key, nil); err != nil {
+		if err := pm.WriteState(kp.Key, nil); err != nil {
 			// Write to remove references to profiles we've already deleted, but
 			// return the original error.
 			pm.writeKnownProfiles()
@@ -424,7 +428,7 @@ func (pm *profileManager) writeKnownProfiles() error {
 	if err != nil {
 		return err
 	}
-	return pm.store.WriteState(ipn.KnownProfilesStateKey, b)
+	return pm.WriteState(ipn.KnownProfilesStateKey, b)
 }
 
 // NewProfile creates and switches to a new unnamed profile. The new profile is

ipn/store.go

@@ -4,6 +4,7 @@
 package ipn
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -71,9 +72,22 @@ type StateStore interface {
 	// ErrStateNotExist) if the ID doesn't have associated state.
 	ReadState(id StateKey) ([]byte, error)
 	// WriteState saves bs as the state associated with ID.
+	//
+	// Callers should generally use the ipn.WriteState wrapper func
+	// instead, which only writes if the value is different from what's
+	// already in the store.
 	WriteState(id StateKey, bs []byte) error
 }
 
+// WriteState is a wrapper around store.WriteState that only writes if
+// the value is different from what's already in the store.
+func WriteState(store StateStore, id StateKey, v []byte) error {
+	if was, err := store.ReadState(id); err == nil && bytes.Equal(was, v) {
+		return nil
+	}
+	return store.WriteState(id, v)
+}
+
 // StateStoreDialerSetter is an optional interface that StateStores
 // can implement to allow the caller to set a custom dialer.
 type StateStoreDialerSetter interface {
@@ -91,5 +105,5 @@ func ReadStoreInt(store StateStore, id StateKey) (int64, error) {
 
 // PutStoreInt puts an integer into a StateStore.
 func PutStoreInt(store StateStore, id StateKey, val int64) error {
-	return store.WriteState(id, fmt.Appendf(nil, "%d", val))
+	return WriteState(store, id, fmt.Appendf(nil, "%d", val))
 }

ipn/store_test.go

@@ -0,0 +1,48 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+package ipn
+
+import (
+	"bytes"
+	"sync"
+	"testing"
+
+	"tailscale.com/util/mak"
+)
+
+type memStore struct {
+	mu     sync.Mutex
+	writes int
+	m      map[StateKey][]byte
+}
+
+func (s *memStore) ReadState(k StateKey) ([]byte, error) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	return bytes.Clone(s.m[k]), nil
+}
+
+func (s *memStore) WriteState(k StateKey, v []byte) error {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	mak.Set(&s.m, k, bytes.Clone(v))
+	s.writes++
+	return nil
+}
+
+func TestWriteState(t *testing.T) {
+	var ss StateStore = new(memStore)
+	WriteState(ss, "foo", []byte("bar"))
+	WriteState(ss, "foo", []byte("bar"))
+	got, err := ss.ReadState("foo")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if want := []byte("bar"); !bytes.Equal(got, want) {
+		t.Errorf("got %q; want %q", got, want)
+	}
+	if got, want := ss.(*memStore).writes, 1; got != want {
+		t.Errorf("got %d writes; want %d", got, want)
+	}
+}