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 <irbe@tailscale.com>

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
 }

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)
+		})
 	}
 }