From f34e08e186ac00c6e86266080c47630f7e5a81d2 Mon Sep 17 00:00:00 2001 From: Irbe Krumina Date: Thu, 20 Mar 2025 14:40:36 +0000 Subject: [PATCH] ipn: ensure that conffile is source of truth for advertised services. (#15361) If conffile is used to configure tailscaled, always update currently advertised services from conffile, even if they are empty in the conffile, to ensure that it is possible to transition to a state where no services are advertised. Updates tailscale/corp#24795 Signed-off-by: Irbe Krumina --- ipn/conf.go | 8 +- ipn/ipnlocal/local_test.go | 145 +++++++++++++++++++++++++++++++------ 2 files changed, 130 insertions(+), 23 deletions(-) diff --git a/ipn/conf.go b/ipn/conf.go index addeea79e..2c9fb2fd1 100644 --- a/ipn/conf.go +++ b/ipn/conf.go @@ -145,9 +145,15 @@ func (c *ConfigVAlpha) ToPrefs() (MaskedPrefs, error) { mp.AppConnector = *c.AppConnector mp.AppConnectorSet = true } + // Configfile should be the source of truth for whether this node + // advertises any services. We need to ensure that each reload updates + // currently advertised services as else the transition from 'some + // services are advertised' to 'advertised services are empty/unset in + // conffile' would have no effect (especially given that an empty + // service slice would be omitted from the JSON config). + mp.AdvertiseServicesSet = true if c.AdvertiseServices != nil { mp.AdvertiseServices = c.AdvertiseServices - mp.AdvertiseServicesSet = true } return mp, nil } diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index aa9137275..bdccdb53d 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -4745,32 +4745,133 @@ func TestLoginNotifications(t *testing.T) { // TestConfigFileReload tests that the LocalBackend reloads its configuration // when the configuration file changes. func TestConfigFileReload(t *testing.T) { - cfg1 := `{"Hostname": "foo", "Version": "alpha0"}` - f := filepath.Join(t.TempDir(), "cfg") - must.Do(os.WriteFile(f, []byte(cfg1), 0600)) - sys := new(tsd.System) - sys.InitialConfig = must.Get(conffile.Load(f)) - lb := newTestLocalBackendWithSys(t, sys) - must.Do(lb.Start(ipn.Options{})) - - lb.mu.Lock() - hn := lb.hostinfo.Hostname - lb.mu.Unlock() - if hn != "foo" { - t.Fatalf("got %q; want %q", hn, "foo") + type testCase struct { + name string + initial *conffile.Config + updated *conffile.Config + checkFn func(*testing.T, *LocalBackend) } - cfg2 := `{"Hostname": "bar", "Version": "alpha0"}` - must.Do(os.WriteFile(f, []byte(cfg2), 0600)) - if !must.Get(lb.ReloadConfig()) { - t.Fatal("reload failed") + tests := []testCase{ + { + name: "hostname_change", + initial: &conffile.Config{ + Parsed: ipn.ConfigVAlpha{ + Version: "alpha0", + Hostname: ptr.To("initial-host"), + }, + }, + updated: &conffile.Config{ + Parsed: ipn.ConfigVAlpha{ + Version: "alpha0", + Hostname: ptr.To("updated-host"), + }, + }, + checkFn: func(t *testing.T, b *LocalBackend) { + if got := b.Prefs().Hostname(); got != "updated-host" { + t.Errorf("hostname = %q; want updated-host", got) + } + }, + }, + { + name: "start_advertising_services", + initial: &conffile.Config{ + Parsed: ipn.ConfigVAlpha{ + Version: "alpha0", + }, + }, + updated: &conffile.Config{ + Parsed: ipn.ConfigVAlpha{ + Version: "alpha0", + AdvertiseServices: []string{"svc:abc", "svc:def"}, + }, + }, + checkFn: func(t *testing.T, b *LocalBackend) { + if got := b.Prefs().AdvertiseServices().AsSlice(); !reflect.DeepEqual(got, []string{"svc:abc", "svc:def"}) { + t.Errorf("AdvertiseServices = %v; want [svc:abc, svc:def]", got) + } + }, + }, + { + name: "change_advertised_services", + initial: &conffile.Config{ + Parsed: ipn.ConfigVAlpha{ + Version: "alpha0", + AdvertiseServices: []string{"svc:abc", "svc:def"}, + }, + }, + updated: &conffile.Config{ + Parsed: ipn.ConfigVAlpha{ + Version: "alpha0", + AdvertiseServices: []string{"svc:abc", "svc:ghi"}, + }, + }, + checkFn: func(t *testing.T, b *LocalBackend) { + if got := b.Prefs().AdvertiseServices().AsSlice(); !reflect.DeepEqual(got, []string{"svc:abc", "svc:ghi"}) { + t.Errorf("AdvertiseServices = %v; want [svc:abc, svc:ghi]", got) + } + }, + }, + { + name: "unset_advertised_services", + initial: &conffile.Config{ + Parsed: ipn.ConfigVAlpha{ + Version: "alpha0", + AdvertiseServices: []string{"svc:abc"}, + }, + }, + updated: &conffile.Config{ + Parsed: ipn.ConfigVAlpha{ + Version: "alpha0", + }, + }, + checkFn: func(t *testing.T, b *LocalBackend) { + if b.Prefs().AdvertiseServices().Len() != 0 { + t.Errorf("got %d AdvertiseServices wants none", b.Prefs().AdvertiseServices().Len()) + } + }, + }, } - lb.mu.Lock() - hn = lb.hostinfo.Hostname - lb.mu.Unlock() - if hn != "bar" { - t.Fatalf("got %q; want %q", hn, "bar") + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "tailscale.conf") + + // Write initial config + initialJSON, err := json.Marshal(tc.initial.Parsed) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, initialJSON, 0644); err != nil { + t.Fatal(err) + } + + // Create backend with initial config + tc.initial.Path = path + tc.initial.Raw = initialJSON + sys := &tsd.System{ + InitialConfig: tc.initial, + } + b := newTestLocalBackendWithSys(t, sys) + + // Update config file + updatedJSON, err := json.Marshal(tc.updated.Parsed) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, updatedJSON, 0644); err != nil { + t.Fatal(err) + } + + // Trigger reload + if ok, err := b.ReloadConfig(); !ok || err != nil { + t.Fatalf("ReloadConfig() = %v, %v; want true, nil", ok, err) + } + + // Check outcome + tc.checkFn(t, b) + }) } }