Remove services.CompareResources (#65151)

All remaining reconcilers relying on the default
ReconcilerConfig.CompareResources function have been updated to
provide one explicitly. All direct uses have been replaced with
equivalent functionality via the appropriate IsEqual function.

The golangci-lint depguard rules have been updated to
remove the current exclusions.

.golangci.yml

@@ -109,9 +109,6 @@ linters:
             - '!**/lib/auth/test/**'
             - '!**/lib/services/suite/**'
             - '!**/e/lib/accesslist/equal.go'
-            - '!**/e/lib/auth/saml.go'
-            - '!**lib/services/compare.go'
-            - '!**/lib/services/local/access_list.go'
           deny:
             - pkg: github.com/google/go-cmp/cmp
               desc: '"github.com/google/go-cmp/cmp" should only be used in tests'

api/types/accesslist/accesslist.go

@@ -709,9 +709,7 @@ func WithIgnoreEphemeralFields() EqualAccessListsOption {
 // EqualAccessLists compares two access lists for semantic equality.
 //
 // By default, this function performs a standard equality check. Use WithIgnoreEphemeralFields()
-// to ignore ephemeral fields that are managed by reconcilers or the backend. This function
-// mimics the behavior of services.CompareResources for AccessList types when used with
-// WithIgnoreEphemeralFields().
+// to ignore ephemeral fields that are managed by reconcilers or the backend.
 //
 // By default, this function clones the input access lists before comparison to avoid
 // modifying the originals. Use WithSkipClone() to skip cloning if the inputs can be

api/types/matchers_accessgraph.go

@@ -54,3 +54,8 @@ func (a *AccessGraphAWSSync) CheckAndSetDefaults() error {
 
 	return nil
 }
+
+// IsEqual determines if two resources are equivalent to one another.
+func (a *AccessGraphAWSSync) IsEqual(other *AccessGraphAWSSync) bool {
+	return deriveTeleportEqualAccessGraphAWSSync(a, other)
+}

lib/kube/proxy/watcher.go

@@ -43,11 +43,14 @@ func (s *TLSServer) startReconciler(ctx context.Context) (err error) {
 	s.reconciler, err = services.NewReconciler(services.ReconcilerConfig[types.KubeCluster]{
 		Matcher:             s.matcher,
 		GetCurrentResources: s.getResources,
-		GetNewResources:     s.monitoredKubeClusters.get,
-		OnCreate:            s.onCreate,
-		OnUpdate:            s.onUpdate,
-		OnDelete:            s.onDelete,
-		Logger:              s.log.With("kind", types.KindKubernetesCluster),
+		CompareResources: func(kc1, kc2 types.KubeCluster) int {
+			return services.EqualFromBool(kc1.IsEqual(kc2))
+		},
+		GetNewResources: s.monitoredKubeClusters.get,
+		OnCreate:        s.onCreate,
+		OnUpdate:        s.onUpdate,
+		OnDelete:        s.onDelete,
+		Logger:          s.log.With("kind", types.KindKubernetesCluster),
 	})
 	if err != nil {
 		return trace.Wrap(err)

lib/services/compare.go

@@ -1,71 +0,0 @@
-/*
- * Teleport
- * Copyright (C) 2023  Gravitational, Inc.
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU Affero General Public License as published by
- * the Free Software Foundation, either version 3 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU Affero General Public License for more details.
- *
- * You should have received a copy of the GNU Affero General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-package services
-
-import (
-	"strings"
-
-	"github.com/google/go-cmp/cmp"
-	"github.com/google/go-cmp/cmp/cmpopts"
-
-	headerv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/header/v1"
-	"github.com/gravitational/teleport/api/types"
-	"github.com/gravitational/teleport/api/types/accesslist"
-	"github.com/gravitational/teleport/api/types/compare"
-	"github.com/gravitational/teleport/api/types/header"
-)
-
-// CompareResources compares two resources by all significant fields.
-func CompareResources[T any](resA, resB T) int {
-	var equal bool
-	if hasEqual, ok := any(resA).(compare.IsEqual[T]); ok {
-		equal = hasEqual.IsEqual(resB)
-	} else {
-		equal = cmp.Equal(resA, resB,
-			ignoreProtoXXXFields(),
-			cmpopts.IgnoreFields(types.Metadata{}, "Revision"),
-			cmpopts.IgnoreFields(types.DatabaseV3{}, "Status"),
-			cmpopts.IgnoreFields(types.UserSpecV2{}, "Status"),
-			cmpopts.IgnoreFields(accesslist.AccessList{}, "Status"),
-			cmpopts.IgnoreFields(header.Metadata{}, "Revision"),
-			cmpopts.IgnoreUnexported(headerv1.Metadata{}),
-
-			// Managed by IneligibleStatusReconciler, ignored by all others.
-			cmpopts.IgnoreFields(accesslist.AccessListMemberSpec{}, "IneligibleStatus"),
-			cmpopts.IgnoreFields(accesslist.Owner{}, "IneligibleStatus"),
-
-			cmpopts.EquateEmpty(),
-		)
-	}
-	if equal {
-		return Equal
-	}
-	return Different
-}
-
-// ignoreProtoXXXFields is a cmp.Option that ignores XXX_* fields from proto
-// messages.
-func ignoreProtoXXXFields() cmp.Option {
-	return cmp.FilterPath(func(path cmp.Path) bool {
-		if field, ok := path.Last().(cmp.StructField); ok {
-			return strings.HasPrefix(field.Name(), "XXX_")
-		}
-		return false
-	}, cmp.Ignore())
-}

lib/services/compare_test.go

@@ -1,76 +0,0 @@
-/*
- * Teleport
- * Copyright (C) 2024  Gravitational, Inc.
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU Affero General Public License as published by
- * the Free Software Foundation, either version 3 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU Affero General Public License for more details.
- *
- * You should have received a copy of the GNU Affero General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-package services
-
-import (
-	"testing"
-
-	"github.com/stretchr/testify/require"
-
-	"github.com/gravitational/teleport/api/types/accesslist"
-)
-
-func TestCompareResources(t *testing.T) {
-	compareTestCase(t, "cmp equal", compareResource{true}, compareResource{true}, Equal)
-	compareTestCase(t, "cmp not equal", compareResource{true}, compareResource{false}, Different)
-
-	// These results should be forced since we're going through a custom compare function.
-	compareTestCase(t, "IsEqual equal", &compareResourceWithEqual{true}, &compareResourceWithEqual{false}, Equal)
-	compareTestCase(t, "IsEqual not equal", &compareResourceWithEqual{false}, &compareResourceWithEqual{false}, Different)
-
-	// These results compare AccessListMemberSpec, which should ignore the IneligibleStatus field.
-	newAccessListMemberSpec := func(ineligibleStatus, accessList string) accesslist.AccessListMemberSpec {
-		return accesslist.AccessListMemberSpec{
-			AccessList:       accessList,
-			IneligibleStatus: ineligibleStatus,
-		}
-	}
-	compareTestCase(t, "cmp equal with equal IneligibleStatus", newAccessListMemberSpec("status1", "accessList1"), newAccessListMemberSpec("status1", "accessList1"), Equal)
-	compareTestCase(t, "cmp equal with different IneligibleStatus", newAccessListMemberSpec("status1", "accessList1"), newAccessListMemberSpec("status2", "accessList1"), Equal)
-	compareTestCase(t, "cmp not equal", newAccessListMemberSpec("status1", "accessList1"), newAccessListMemberSpec("status1", "accessList2"), Different)
-
-	// These results compare the IneligibleStatus field in accesslist.Owner, which should be ignored.
-	newAccessListOwner := func(ineligibleStatus, name string) accesslist.Owner {
-		return accesslist.Owner{
-			Name:             name,
-			IneligibleStatus: ineligibleStatus,
-		}
-	}
-	compareTestCase(t, "cmp equal with equal IneligibleStatus", newAccessListOwner("status1", "alice"), newAccessListOwner("status1", "alice"), Equal)
-	compareTestCase(t, "cmp equal with different IneligibleStatus", newAccessListOwner("status1", "alice"), newAccessListOwner("status2", "alice"), Equal)
-	compareTestCase(t, "cmp different when name differs", newAccessListOwner("status1", "alice"), newAccessListOwner("status1", "bob"), Different)
-}
-
-func compareTestCase[T any](t *testing.T, name string, resA, resB T, expected int) {
-	t.Run(name, func(t *testing.T) {
-		require.Equal(t, expected, CompareResources(resA, resB))
-	})
-}
-
-type compareResource struct {
-	Field bool
-}
-
-type compareResourceWithEqual struct {
-	ForceCompare bool
-}
-
-func (r *compareResourceWithEqual) IsEqual(_ *compareResourceWithEqual) bool {
-	return r.ForceCompare
-}

lib/services/reconciler.go

@@ -97,7 +97,7 @@ func (c *GenericReconcilerConfig[K, T]) CheckAndSetDefaults() error {
 		return trace.BadParameter("missing reconciler OnDelete")
 	}
 	if c.CompareResources == nil {
-		c.CompareResources = CompareResources[T]
+		return trace.BadParameter("missing reconciler CompareResources")
 	}
 	if c.Logger == nil {
 		c.Logger = slog.With(teleport.ComponentKey, "reconciler")

lib/services/reconciler_test.go

@@ -23,6 +23,8 @@ import (
 	"maps"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/gravitational/trace"
 	"github.com/stretchr/testify/require"
 
@@ -251,7 +253,13 @@ func TestReconciler(t *testing.T) {
 						return t.Metadata.Name
 					})
 				},
-				CompareResources: test.comparator,
+				CompareResources: func(tr1, tr2 testResource) int {
+					if test.comparator != nil {
+						return test.comparator(tr1, tr2)
+					}
+
+					return EqualFromBool(cmp.Equal(tr1, tr2, cmpopts.IgnoreUnexported(headerv1.Metadata{})))
+				},
 				OnCreate: func(ctx context.Context, tr testResource) error {
 					onCreateCalls = append(onCreateCalls, tr)
 					return nil
@@ -320,6 +328,9 @@ func TestGenericReconciler(t *testing.T) {
 			}}
 			return MatchResourceLabels(selectors, tr.GetMetadata().Labels)
 		},
+		CompareResources: func(tr1, tr2 testResource) int {
+			return EqualFromBool(cmp.Equal(tr1, tr2, cmpopts.IgnoreUnexported(headerv1.Metadata{})))
+		},
 		GetCurrentResources: func() map[resourceID]testResource {
 			return registeredResources
 		},

lib/srv/app/watcher.go

@@ -38,11 +38,14 @@ func (s *Server) startReconciler(ctx context.Context) error {
 	reconciler, err := services.NewReconciler(services.ReconcilerConfig[types.Application]{
 		Matcher:             s.matcher,
 		GetCurrentResources: s.getResources,
-		GetNewResources:     s.monitoredApps.get,
-		OnCreate:            s.onCreate,
-		OnUpdate:            s.onUpdate,
-		OnDelete:            s.onDelete,
-		Logger:              s.log.With("kind", types.KindApp),
+		CompareResources: func(a1, a2 types.Application) int {
+			return services.EqualFromBool(a1.IsEqual(a2))
+		},
+		GetNewResources: s.monitoredApps.get,
+		OnCreate:        s.onCreate,
+		OnUpdate:        s.onUpdate,
+		OnDelete:        s.onDelete,
+		Logger:          s.log.With("kind", types.KindApp),
 	})
 	if err != nil {
 		return trace.Wrap(err)

lib/srv/db/watcher.go

@@ -41,11 +41,14 @@ func (s *Server) startReconciler(ctx context.Context) error {
 	reconciler, err := services.NewReconciler(services.ReconcilerConfig[types.Database]{
 		Matcher:             s.matcher,
 		GetCurrentResources: s.getResources,
-		GetNewResources:     s.monitoredDatabases.getLocked,
-		OnCreate:            s.onCreate,
-		OnUpdate:            s.onUpdate,
-		OnDelete:            s.onDelete,
-		Logger:              s.log.With("kind", types.KindDatabase),
+		CompareResources: func(d1, d2 types.Database) int {
+			return services.EqualFromBool(d1.IsEqual(d2))
+		},
+		GetNewResources: s.monitoredDatabases.getLocked,
+		OnCreate:        s.onCreate,
+		OnUpdate:        s.onUpdate,
+		OnDelete:        s.onDelete,
+		Logger:          s.log.With("kind", types.KindDatabase),
 	})
 	if err != nil {
 		return trace.Wrap(err)

lib/srv/desktop/discovery.go

@@ -124,6 +124,9 @@ func (s *WindowsService) startDesktopDiscovery() error {
 			_, ok := d.GetLabel(types.DiscoveryLabelWindowsOS)
 			return ok
 		},
+		CompareResources: func(wd1, wd2 types.WindowsDesktop) int {
+			return services.EqualFromBool(wd1.IsEqual(wd2))
+		},
 		GetCurrentResources: func() map[string]types.WindowsDesktop { return s.currentDesktops(s.closeCtx) },
 		GetNewResources:     s.getDesktopsFromLDAP,
 		OnCreate:            s.upsertDesktop,
@@ -445,6 +448,9 @@ func (s *WindowsService) startDynamicReconciler(ctx context.Context) error {
 			Matcher: func(desktop types.WindowsDesktop) bool {
 				return services.MatchResourceLabels(s.cfg.ResourceMatchers, desktop.GetAllLabels())
 			},
+			CompareResources: func(wd1, wd2 types.WindowsDesktop) int {
+				return services.EqualFromBool(wd1.IsEqual(wd2))
+			},
 			GetCurrentResources: func() map[string]types.WindowsDesktop { return currentResources },
 			GetNewResources:     func() map[string]types.WindowsDesktop { return newResources },
 			OnCreate:            s.upsertDesktop,

lib/srv/discovery/access_graph_aws.go

@@ -803,9 +803,9 @@ func (s *Server) startCloudtrailPoller(ctx context.Context, reloadCh <-chan stru
 			}
 			return matchersMap
 		},
-		// Compare allows custom comparators without having to implement IsEqual.
-		// Defaults to `CompareResources[T]` if not specified.
-		CompareResources: services.CompareResources[*types.AccessGraphAWSSync],
+		CompareResources: func(aga1, aga2 *types.AccessGraphAWSSync) int {
+			return services.EqualFromBool(aga1.IsEqual(aga2))
+		},
 		OnCreate: func(_ context.Context, disc *types.AccessGraphAWSSync) error {
 			spawnMatcher(ctx, disc)
 			return nil

lib/srv/discovery/database_watcher.go

@@ -51,6 +51,9 @@ func (s *Server) startDatabaseWatchers() error {
 		services.ReconcilerConfig[types.Database]{
 			Matcher:             func(database types.Database) bool { return true },
 			GetCurrentResources: s.getCurrentDatabases,
+			CompareResources: func(d1, d2 types.Database) int {
+				return services.EqualFromBool(d1.IsEqual(d2))
+			},
 			GetNewResources: func() map[string]types.Database {
 				mu.RLock()
 				defer mu.RUnlock()

lib/srv/discovery/discovery_test.go

@@ -1453,7 +1453,7 @@ func TestDiscoveryKubeServices(t *testing.T) {
 				a1 := types.Apps(existingApps)
 				a2 := types.Apps(tt.expectedAppsToExistInAuth)
 				for k := range a1 {
-					require.Equal(t, services.Equal, services.CompareResources(a1[k], a2[k]))
+					require.True(t, a1[k].IsEqual(a2[k]))
 				}
 			})
 		})
@@ -1874,9 +1874,7 @@ func TestDiscoveryInCloudKube(t *testing.T) {
 				c1 := types.KubeClusters(tc.expectedClustersToExistInAuth).ToMap()
 				c2 := types.KubeClusters(kubeClusters).ToMap()
 				for k := range c1 {
-					if services.CompareResources(c1[k], c2[k]) != services.Equal {
-						require.Equal(t, c1[k], c2[k], "expected no differences")
-					}
+					require.True(t, c1[k].IsEqual(c2[k]), "expected no differences")
 				}
 			case <-time.After(10 * time.Second):
 				require.FailNow(t, "Didn't receive reconcile event after 10s")

lib/srv/discovery/kube_integration_watcher_test.go

@@ -48,7 +48,6 @@ import (
 	"github.com/gravitational/teleport/lib/authz"
 	"github.com/gravitational/teleport/lib/cloud/mocks"
 	"github.com/gravitational/teleport/lib/integrations/awsoidc"
-	"github.com/gravitational/teleport/lib/services"
 	"github.com/gravitational/teleport/lib/srv/discovery/common"
 	"github.com/gravitational/teleport/lib/srv/discovery/fetchers"
 	"github.com/gravitational/teleport/lib/utils/log/logtest"
@@ -449,7 +448,7 @@ func TestDiscoveryKubeIntegrationEKS(t *testing.T) {
 				k1 := types.KubeServers(kubeServers).ToMap()
 				k2 := types.KubeServers(tc.expectedServersToExistInAuth).ToMap()
 				for k := range k1 {
-					require.Equal(t, services.Equal, services.CompareResources(k1[k], k2[k]), "kube server in auth server does not match expected")
+					require.True(t, k1[k].IsEqual(k2[k]), "kube server in auth server does not match expected")
 				}
 			})
 		})

lib/srv/discovery/kube_services_watcher.go

@@ -55,6 +55,9 @@ func (s *Server) startKubeAppsWatchers() error {
 
 				return utils.FromSlice(filterResources(apps, types.OriginDiscoveryKubernetes, s.DiscoveryGroup), types.Application.GetName)
 			},
+			CompareResources: func(a1, a2 types.Application) int {
+				return services.EqualFromBool(a1.IsEqual(a2))
+			},
 			GetNewResources: func() map[string]types.Application {
 				mu.Lock()
 				defer mu.Unlock()

lib/srv/discovery/kube_watcher.go

@@ -62,8 +62,8 @@ func (s *Server) startKubeWatchers() error {
 				return utils.FromSlice(kubeResources, types.KubeCluster.GetName)
 			},
 			CompareResources: func(kc1, kc2 types.KubeCluster) int {
-				if res := services.CompareResources(kc1, kc2); res != services.Equal {
-					return res
+				if !kc1.IsEqual(kc2) {
+					return services.Different
 				}
 				// Additionally compare Status field using its IsEqual method.
 				// This is needed because CompareResources ignores Status field of KubeCluster and for most