From c56e94af2d023872b2cd99479e2c2a98c99c695f Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Fri, 4 Aug 2023 07:55:59 -0700 Subject: [PATCH] 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 --- ipn/ipnlocal/cert.go | 6 ++--- ipn/ipnlocal/local.go | 6 ++--- ipn/ipnlocal/profiles.go | 18 +++++++++------ ipn/store.go | 16 +++++++++++++- ipn/store_test.go | 48 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 80 insertions(+), 14 deletions(-) create mode 100644 ipn/store_test.go diff --git a/ipn/ipnlocal/cert.go b/ipn/ipnlocal/cert.go index 16ef17eb8..9da760bb3 100644 --- a/ipn/ipnlocal/cert.go +++ b/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 diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index b420cdf80..03b088998 100644 --- a/ipn/ipnlocal/local.go +++ b/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 { diff --git a/ipn/ipnlocal/profiles.go b/ipn/ipnlocal/profiles.go index a38bccab0..26e0517cd 100644 --- a/ipn/ipnlocal/profiles.go +++ b/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 diff --git a/ipn/store.go b/ipn/store.go index ee5a238a7..3bef012ba 100644 --- a/ipn/store.go +++ b/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)) } diff --git a/ipn/store_test.go b/ipn/store_test.go new file mode 100644 index 000000000..fcc082d8a --- /dev/null +++ b/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) + } +}