[v18] Reduce allocations in app server watcher

api/types/app.go

@@ -59,6 +59,8 @@ type Application interface {
 	SetURI(string)
 	// GetPublicAddr returns the app public address.
 	GetPublicAddr() string
+	// SetPublicAddr sets the app public address.
+	SetPublicAddr(s string)
 	// GetInsecureSkipVerify returns the app insecure setting.
 	GetInsecureSkipVerify() bool
 	// GetRewrite returns the app rewrite configuration.
@@ -249,6 +251,11 @@ func (a *AppV3) GetPublicAddr() string {
 	return a.Spec.PublicAddr
 }
 
+// SetPublicAddr sets the app public address.
+func (a *AppV3) SetPublicAddr(addr string) {
+	a.Spec.PublicAddr = addr
+}
+
 // GetInsecureSkipVerify returns the app insecure setting.
 func (a *AppV3) GetInsecureSkipVerify() bool {
 	return a.Spec.InsecureSkipVerify

lib/srv/app/watcher.go

@@ -88,11 +88,20 @@ func (s *Server) startResourceWatcher(ctx context.Context) (*services.GenericWat
 		for {
 			select {
 			case apps := <-watcher.ResourcesC:
-				appsWithAddr := make(types.Apps, 0, len(apps))
 				for _, app := range apps {
-					appsWithAddr = append(appsWithAddr, s.guessPublicAddr(app))
+					if app.GetPublicAddr() == "" {
+						pubAddr, err := FindPublicAddr(s.c.AccessPoint, app.GetPublicAddr(), app.GetName())
+						if err == nil {
+							app.SetPublicAddr(pubAddr)
+						} else {
+							s.log.ErrorContext(s.closeContext, "Unable to find public address for app, leaving empty",
+								"app_name", app.GetName(),
+								"error", err,
+							)
+						}
+					}
 				}
-				s.monitoredApps.setResources(appsWithAddr)
+				s.monitoredApps.setResources(apps)
 				select {
 				case s.reconcileCh <- struct{}{}:
 				case <-ctx.Done():
@@ -107,24 +116,6 @@ func (s *Server) startResourceWatcher(ctx context.Context) (*services.GenericWat
 	return watcher, nil
 }
 
-// guessPublicAddr will guess PublicAddr for given application if it is missing, based on proxy information and app name.
-func (s *Server) guessPublicAddr(app types.Application) types.Application {
-	if app.GetPublicAddr() != "" {
-		return app
-	}
-	appCopy := app.Copy()
-	pubAddr, err := FindPublicAddr(s.c.AccessPoint, app.GetPublicAddr(), app.GetName())
-	if err == nil {
-		appCopy.Spec.PublicAddr = pubAddr
-	} else {
-		s.log.ErrorContext(s.closeContext, "Unable to find public address for app, leaving empty",
-			"app_name", app.GetName(),
-			"error", err,
-		)
-	}
-	return appCopy
-}
-
 // FindPublicAddrClient is a client used for finding public addresses.
 type FindPublicAddrClient interface {
 	// GetProxies returns a list of proxy servers registered in the cluster

lib/srv/app/watcher_test.go

@@ -19,13 +19,13 @@
 package app
 
 import (
-	"context"
+	"cmp"
 	"maps"
-	"sort"
+	"slices"
 	"testing"
 	"time"
 
-	"github.com/google/go-cmp/cmp"
+	gocmp "github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/stretchr/testify/require"
 
@@ -36,7 +36,7 @@ import (
 // TestWatcher verifies that app agent properly detects and applies
 // changes to application resources.
 func TestWatcher(t *testing.T) {
-	ctx := context.Background()
+	ctx := t.Context()
 
 	// Make a static configuration app.
 	app0, err := makeStaticApp("app0", nil)
@@ -60,11 +60,27 @@ func TestWatcher(t *testing.T) {
 		},
 	})
 
+	// Create a single Proxy with a PublicAddr to exercise that
+	// apps without a PublicAddr automatically get one specified
+	// by the watcher.
+	require.NoError(t, s.authServer.AuthServer.UpsertProxy(t.Context(), &types.ServerV2{
+		Kind:    types.KindProxy,
+		Version: types.V2,
+		Metadata: types.Metadata{
+			Name: "FakeProxy",
+		},
+		Spec: types.ServerSpecV2{
+			PublicAddrs: []string{"test.example.com"},
+		},
+	}))
+
 	// Only app0 should be registered initially.
 	select {
 	case a := <-reconcileCh:
-		sort.Sort(a)
-		require.Empty(t, cmp.Diff(types.Apps{app0}, a,
+		slices.SortFunc(a, func(a, b types.Application) int {
+			return cmp.Compare(a.GetName(), b.GetName())
+		})
+		require.Empty(t, gocmp.Diff(types.Apps{app0}, a,
 			cmpopts.IgnoreFields(types.Metadata{}, "Revision"),
 		))
 	case <-time.After(time.Second):
@@ -77,11 +93,18 @@ func TestWatcher(t *testing.T) {
 	err = s.authServer.AuthServer.CreateApp(ctx, app1)
 	require.NoError(t, err)
 
+	// Set the PublicAddr _after_ creating the app. The watched apps will
+	// automatically have the address set if empty. In order for the comparisons
+	// below to pass this needs to be set on app1.
+	app1.SetPublicAddr("app1.test.example.com")
+
 	// It should be registered.
 	select {
 	case a := <-reconcileCh:
-		sort.Sort(a)
-		require.Empty(t, cmp.Diff(types.Apps{app0, app1}, a,
+		slices.SortFunc(a, func(a, b types.Application) int {
+			return cmp.Compare(a.GetName(), b.GetName())
+		})
+		require.Empty(t, gocmp.Diff(types.Apps{app0, app1}, a,
 			cmpopts.IgnoreFields(types.Metadata{}, "Revision"),
 		))
 	case <-time.After(time.Second):
@@ -97,8 +120,10 @@ func TestWatcher(t *testing.T) {
 	// It should not be registered, old app0 should remain.
 	select {
 	case a := <-reconcileCh:
-		sort.Sort(a)
-		require.Empty(t, cmp.Diff(types.Apps{app0, app1}, a,
+		slices.SortFunc(a, func(a, b types.Application) int {
+			return cmp.Compare(a.GetName(), b.GetName())
+		})
+		require.Empty(t, gocmp.Diff(types.Apps{app0, app1}, a,
 			cmpopts.IgnoreFields(types.Metadata{}, "Revision"),
 		))
 	case <-time.After(time.Second):
@@ -108,14 +133,21 @@ func TestWatcher(t *testing.T) {
 	// Create app with label group=b.
 	app2, err := makeDynamicApp("app2", map[string]string{"group": "b"})
 	require.NoError(t, err)
+
+	// Set the PublicAddr _before_ creating the app. The watcher should
+	// not modify apps with an already specified PublicAddr.
+	app2.SetPublicAddr("app2.some.other.addr.example.com")
+
 	err = s.authServer.AuthServer.CreateApp(ctx, app2)
 	require.NoError(t, err)
 
 	// It shouldn't be registered.
 	select {
 	case a := <-reconcileCh:
-		sort.Sort(a)
-		require.Empty(t, cmp.Diff(types.Apps{app0, app1}, a,
+		slices.SortFunc(a, func(a, b types.Application) int {
+			return cmp.Compare(a.GetName(), b.GetName())
+		})
+		require.Empty(t, gocmp.Diff(types.Apps{app0, app1}, a,
 			cmpopts.IgnoreFields(types.Metadata{}, "Revision"),
 		))
 	case <-time.After(time.Second):
@@ -130,8 +162,10 @@ func TestWatcher(t *testing.T) {
 	// Both should be registered now.
 	select {
 	case a := <-reconcileCh:
-		sort.Sort(a)
-		require.Empty(t, cmp.Diff(types.Apps{app0, app1, app2}, a,
+		slices.SortFunc(a, func(a, b types.Application) int {
+			return cmp.Compare(a.GetName(), b.GetName())
+		})
+		require.Empty(t, gocmp.Diff(types.Apps{app0, app1, app2}, a,
 			cmpopts.IgnoreFields(types.Metadata{}, "Revision"),
 		))
 	case <-time.After(time.Second):
@@ -146,8 +180,10 @@ func TestWatcher(t *testing.T) {
 	// app2 should get updated.
 	select {
 	case a := <-reconcileCh:
-		sort.Sort(a)
-		require.Empty(t, cmp.Diff(types.Apps{app0, app1, app2}, a,
+		slices.SortFunc(a, func(a, b types.Application) int {
+			return cmp.Compare(a.GetName(), b.GetName())
+		})
+		require.Empty(t, gocmp.Diff(types.Apps{app0, app1, app2}, a,
 			cmpopts.IgnoreFields(types.Metadata{}, "Revision"),
 		))
 	case <-time.After(time.Second):
@@ -162,8 +198,10 @@ func TestWatcher(t *testing.T) {
 	// Only app0 and app2 should remain registered.
 	select {
 	case a := <-reconcileCh:
-		sort.Sort(a)
-		require.Empty(t, cmp.Diff(types.Apps{app0, app2}, a,
+		slices.SortFunc(a, func(a, b types.Application) int {
+			return cmp.Compare(a.GetName(), b.GetName())
+		})
+		require.Empty(t, gocmp.Diff(types.Apps{app0, app2}, a,
 			cmpopts.IgnoreFields(types.Metadata{}, "Revision"),
 		))
 	case <-time.After(time.Second):
@@ -177,7 +215,7 @@ func TestWatcher(t *testing.T) {
 	// Only static app should remain.
 	select {
 	case a := <-reconcileCh:
-		require.Empty(t, cmp.Diff(types.Apps{app0}, a,
+		require.Empty(t, gocmp.Diff(types.Apps{app0}, a,
 			cmpopts.IgnoreFields(types.Metadata{}, "Revision"),
 		))
 	case <-time.After(time.Second):