Repository: rancher/fleet
Pull request #3551 (2025-04-14 08:30:40)

Trigger cluster imports only from relevant config updates
Fleet's agent management logic re-imports clusters based on configuration updates, which includes API server URL and CA.

However, those config fields are only used by clusters which do not source API server URL or CA from secrets.
Clusters which do use secrets instead must not be affected by changes to API server URL and CA in config.

This patch provides unit tests validating this new behaviour.

Refers to #3061


Patch:
diff --git a/internal/cmd/controller/agentmanagement/controllers/cluster/import.go b/internal/cmd/controller/agentmanagement/controllers/cluster/import.go
index 35939761..476f1fd6 100644
--- a/internal/cmd/controller/agentmanagement/controllers/cluster/import.go
+++ b/internal/cmd/controller/agentmanagement/controllers/cluster/import.go
@@ -5,6 +5,7 @@ import (
 	"context"
 	"crypto/sha256"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"os"
@@ -29,6 +30,7 @@ import (
 	"github.com/rancher/wrangler/v3/pkg/randomtoken"
 	"github.com/rancher/wrangler/v3/pkg/yaml"
 
+	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
@@ -81,7 +83,7 @@ func RegisterImport(
 // fleet-controller config map. This is important for changes to apiServerURL
 // and apiServerCA, as they are needed e.g. to update the fleet-agent-bootstrap
 // secret.
-func (i *importHandler) onConfig(config *config.Config) error {
+func (i *importHandler) onConfig(cfg *config.Config) error {
 	clusters, err := i.clusters.List("", metav1.ListOptions{})
 	if err != nil {
 		return err
@@ -91,15 +93,22 @@ func (i *importHandler) onConfig(config *config.Config) error {
 		return nil
 	}
 
+	if cfg == nil {
+		return errors.New("config is nil: this should never happen")
+	}
+
 	for _, cluster := range clusters.Items {
 		if cluster.Spec.KubeConfigSecret == "" {
 			continue
 		}
 
-		hasConfigChanged := config.APIServerURL != cluster.Status.APIServerURL ||
-			hashStatusField(config.APIServerCA) != cluster.Status.APIServerCAHash ||
-			config.AgentTLSMode != cluster.Status.AgentTLSMode ||
-			hasGarbageCollectionIntervalChanged(config, cluster)
+		hasConfigChanged, err := i.hasAPIServerConfigChanged(cfg, cluster)
+		if err != nil {
+			return fmt.Errorf("cluster %s/%s: could not check for config changes: %v", cluster.Namespace, cluster.Name, err)
+		}
+
+		hasConfigChanged = hasConfigChanged || cfg.AgentTLSMode != cluster.Status.AgentTLSMode ||
+			hasGarbageCollectionIntervalChanged(cfg, cluster)
 
 		if hasConfigChanged {
 			logrus.Infof("API server config changed, trigger cluster import for cluster %s/%s", cluster.Namespace, cluster.Name)
@@ -114,6 +123,37 @@ func (i *importHandler) onConfig(config *config.Config) error {
 	return nil
 }
 
+// hasAPIServerConfigChanged checks for changes in API server URL or CA configuration, comparing the current state of
+// the cluster with cfg. However, if the cluster references a secret through its `KubeConfigSecret` field, then API
+// server URL and CA are understood to be sourced from there, hence config changes for those fields will be skipped.
+// Returns a boolean indicating whether URL or CA config has changed, and any error that may have occurred (such as the
+// referenced secret not being found).
+func (i *importHandler) hasAPIServerConfigChanged(cfg *config.Config, cluster fleet.Cluster) (bool, error) {
+	kubeConfigSecretNamespace := getKubeConfigSecretNS(cluster)
+
+	var secretAPIServerCA, secretAPIServerURL []byte
+	var secret *corev1.Secret
+
+	if cluster.Spec.KubeConfigSecret != "" {
+		var err error
+		secret, err = i.secrets.Get(kubeConfigSecretNamespace, cluster.Spec.KubeConfigSecret)
+		if err != nil {
+			return false, err
+		}
+
+		secretAPIServerURL = secret.Data[config.APIServerURLKey]
+		secretAPIServerCA = secret.Data[config.APIServerCAKey]
+	}
+
+	// if the API server URL is non-empty in the secret, then it is sourced from there; config changes for that field
+	// are irrelevant.
+	// The same applies to the CA.
+	hasURLChanged := len(secretAPIServerURL) == 0 && cfg.APIServerURL != cluster.Status.APIServerURL
+	hasCAChanged := len(secretAPIServerCA) == 0 && hashStatusField(cfg.APIServerCA) != cluster.Status.APIServerCAHash
+
+	return hasURLChanged || hasCAChanged, nil
+}
+
 func hashStatusField(field any) string {
 	hasher := sha256.New224()
 	b, err := json.Marshal(field)
@@ -223,10 +263,8 @@ func (i *importHandler) importCluster(cluster *fleet.Cluster, status fleet.Clust
 		return status, nil
 	}
 
-	kubeConfigSecretNamespace := cluster.Namespace
-	if cluster.Spec.KubeConfigSecretNamespace != "" {
-		kubeConfigSecretNamespace = cluster.Spec.KubeConfigSecretNamespace
-	}
+	kubeConfigSecretNamespace := getKubeConfigSecretNS(*cluster)
+
 	logrus.Debugf("Cluster import for '%s/%s'. Getting kubeconfig from secret in namespace %s", cluster.Namespace, cluster.Name, kubeConfigSecretNamespace)
 
 	secret, err := i.secrets.Get(kubeConfigSecretNamespace, cluster.Spec.KubeConfigSecret)
@@ -443,6 +481,14 @@ func (i *importHandler) restConfigFromKubeConfig(data []byte, agentTLSMode strin
 	return clientcmd.NewDefaultClientConfig(raw, &clientcmd.ConfigOverrides{}).ClientConfig()
 }
 
+func getKubeConfigSecretNS(cluster fleet.Cluster) string {
+	if cluster.Spec.KubeConfigSecretNamespace == "" {
+		return cluster.Namespace
+	}
+
+	return cluster.Spec.KubeConfigSecretNamespace
+}
+
 func hasGarbageCollectionIntervalChanged(config *config.Config, cluster fleet.Cluster) bool {
 	return (config.GarbageCollectionInterval.Duration != 0 && cluster.Status.GarbageCollectionInterval == nil) ||
 		(cluster.Status.GarbageCollectionInterval != nil &&


Test patch:
diff --git a/internal/cmd/controller/agentmanagement/controllers/cluster/import_test.go b/internal/cmd/controller/agentmanagement/controllers/cluster/import_test.go
new file mode 100644
index 00000000..f119757d
--- /dev/null
+++ b/internal/cmd/controller/agentmanagement/controllers/cluster/import_test.go
@@ -0,0 +1,323 @@
+package cluster
+
+import (
+	"testing"
+	"time"
+
+	"github.com/rancher/wrangler/v3/pkg/generic/fake"
+	"go.uber.org/mock/gomock"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/rancher/fleet/internal/config"
+	fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
+)
+
+func TestOnConfig(t *testing.T) {
+	cases := map[string]struct {
+		cfg              config.Config
+		handlerWithMocks func() importHandler
+	}{
+		"no clusters, no import": {
+			cfg: config.Config{
+				APIServerCA:  []byte("foo"),
+				APIServerURL: "https://hello.world",
+				AgentTLSMode: "system-store",
+			},
+			handlerWithMocks: func() importHandler {
+				ctrl := gomock.NewController(t)
+
+				secretsCache := fake.NewMockCacheInterface[*corev1.Secret](ctrl)
+
+				clustersController := fake.NewMockControllerInterface[*fleet.Cluster, *fleet.ClusterList](ctrl)
+				clustersController.EXPECT().List("", metav1.ListOptions{}).Return(&fleet.ClusterList{}, nil)
+
+				return importHandler{
+					clusters: clustersController,
+					secrets:  secretsCache,
+				}
+			},
+		},
+		"no URL or CA in secret, do not trigger import when URL changes": {
+			cfg: config.Config{
+				APIServerCA:               []byte("foo"),
+				APIServerURL:              "https://hello.new.world",
+				AgentTLSMode:              "system-store",
+				GarbageCollectionInterval: metav1.Duration{Duration: 10 * time.Minute},
+			},
+			handlerWithMocks: func() importHandler {
+				ctrl := gomock.NewController(t)
+
+				secretsCache := fake.NewMockCacheInterface[*corev1.Secret](ctrl)
+				secretsCache.EXPECT().Get(gomock.Any(), "my-kubeconfig-secret").Return(&corev1.Secret{}, nil)
+
+				clustersController := fake.NewMockControllerInterface[*fleet.Cluster, *fleet.ClusterList](ctrl)
+				clustersController.EXPECT().List("", metav1.ListOptions{}).
+					Return(&fleet.ClusterList{
+						Items: []fleet.Cluster{
+							{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:      "cluster",
+									Namespace: "fleet-default",
+								},
+								Spec: fleet.ClusterSpec{
+									KubeConfigSecret: "my-kubeconfig-secret",
+								},
+								Status: fleet.ClusterStatus{
+									APIServerURL:              "https://hello.world",
+									APIServerCAHash:           hashStatusField("foo"),
+									AgentTLSMode:              "system-store",
+									GarbageCollectionInterval: &metav1.Duration{Duration: 10 * time.Minute},
+								},
+							},
+						},
+					}, nil)
+
+				clustersController.EXPECT().UpdateStatus(gomock.Any()) // import triggered
+
+				return importHandler{
+					clusters: clustersController,
+					secrets:  secretsCache,
+				}
+			},
+		},
+		"no URL or CA in secret, do not trigger import when CA changes": {
+			cfg: config.Config{
+				APIServerCA:               []byte("new-foo"),
+				APIServerURL:              "https://hello.world",
+				AgentTLSMode:              "system-store",
+				GarbageCollectionInterval: metav1.Duration{Duration: 10 * time.Minute},
+			},
+			handlerWithMocks: func() importHandler {
+				ctrl := gomock.NewController(t)
+
+				secretsCache := fake.NewMockCacheInterface[*corev1.Secret](ctrl)
+				secretsCache.EXPECT().Get(gomock.Any(), "my-kubeconfig-secret").Return(&corev1.Secret{}, nil)
+
+				clustersController := fake.NewMockControllerInterface[*fleet.Cluster, *fleet.ClusterList](ctrl)
+				clustersController.EXPECT().List("", metav1.ListOptions{}).
+					Return(&fleet.ClusterList{
+						Items: []fleet.Cluster{
+							{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:      "cluster",
+									Namespace: "fleet-default",
+								},
+								Spec: fleet.ClusterSpec{
+									KubeConfigSecret: "my-kubeconfig-secret",
+								},
+								Status: fleet.ClusterStatus{
+									APIServerURL:              "https://hello.world",
+									APIServerCAHash:           hashStatusField("foo"),
+									AgentTLSMode:              "system-store",
+									GarbageCollectionInterval: &metav1.Duration{Duration: 10 * time.Minute},
+								},
+							},
+						},
+					}, nil)
+
+				clustersController.EXPECT().UpdateStatus(gomock.Any()) // import triggered
+
+				return importHandler{
+					clusters: clustersController,
+					secrets:  secretsCache,
+				}
+			},
+		},
+		"no URL or CA in secret, trigger import when agent TLS mode changes": {
+			cfg: config.Config{
+				APIServerCA:               []byte("foo"),
+				APIServerURL:              "https://hello.world",
+				AgentTLSMode:              "strict",
+				GarbageCollectionInterval: metav1.Duration{Duration: 10 * time.Minute},
+			},
+			handlerWithMocks: func() importHandler {
+				ctrl := gomock.NewController(t)
+
+				secretsCache := fake.NewMockCacheInterface[*corev1.Secret](ctrl)
+				secretsCache.EXPECT().Get(gomock.Any(), "my-kubeconfig-secret").Return(&corev1.Secret{}, nil)
+
+				clustersController := fake.NewMockControllerInterface[*fleet.Cluster, *fleet.ClusterList](ctrl)
+				clustersController.EXPECT().List("", metav1.ListOptions{}).
+					Return(&fleet.ClusterList{
+						Items: []fleet.Cluster{
+							{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:      "cluster",
+									Namespace: "fleet-default",
+								},
+								Spec: fleet.ClusterSpec{
+									KubeConfigSecret: "my-kubeconfig-secret",
+								},
+								Status: fleet.ClusterStatus{
+									APIServerURL:              "https://hello.world",
+									APIServerCAHash:           hashStatusField("foo"),
+									AgentTLSMode:              "system-store",
+									GarbageCollectionInterval: &metav1.Duration{Duration: 10 * time.Minute},
+								},
+							},
+						},
+					}, nil)
+
+				clustersController.EXPECT().UpdateStatus(gomock.Any()) // import triggered
+
+				return importHandler{
+					clusters: clustersController,
+					secrets:  secretsCache,
+				}
+			},
+		},
+		"no URL or CA in secret, trigger import when agent garbage collection interval changes": {
+			cfg: config.Config{
+				APIServerCA:               []byte("foo"),
+				APIServerURL:              "https://hello.world",
+				AgentTLSMode:              "system-store",
+				GarbageCollectionInterval: metav1.Duration{Duration: 5 * time.Minute},
+			},
+			handlerWithMocks: func() importHandler {
+				ctrl := gomock.NewController(t)
+
+				secretsCache := fake.NewMockCacheInterface[*corev1.Secret](ctrl)
+				secretsCache.EXPECT().Get(gomock.Any(), "my-kubeconfig-secret").Return(&corev1.Secret{}, nil)
+
+				clustersController := fake.NewMockControllerInterface[*fleet.Cluster, *fleet.ClusterList](ctrl)
+				clustersController.EXPECT().List("", metav1.ListOptions{}).
+					Return(&fleet.ClusterList{
+						Items: []fleet.Cluster{
+							{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:      "cluster",
+									Namespace: "fleet-default",
+								},
+								Spec: fleet.ClusterSpec{
+									KubeConfigSecret: "my-kubeconfig-secret",
+								},
+								Status: fleet.ClusterStatus{
+									APIServerURL:              "https://hello.world",
+									APIServerCAHash:           hashStatusField("foo"),
+									AgentTLSMode:              "system-store",
+									GarbageCollectionInterval: &metav1.Duration{Duration: 10 * time.Minute},
+								},
+							},
+						},
+					}, nil)
+
+				clustersController.EXPECT().UpdateStatus(gomock.Any()) // import triggered
+
+				return importHandler{
+					clusters: clustersController,
+					secrets:  secretsCache,
+				}
+			},
+		},
+		"URL and CA in secret, do not trigger import when only URL or CA changes": {
+			cfg: config.Config{
+				APIServerCA:               []byte("new-foo"),
+				APIServerURL:              "https://hello.new.world",
+				AgentTLSMode:              "system-store",
+				GarbageCollectionInterval: metav1.Duration{Duration: 10 * time.Minute},
+			},
+			handlerWithMocks: func() importHandler {
+				ctrl := gomock.NewController(t)
+
+				clustersController := fake.NewMockControllerInterface[*fleet.Cluster, *fleet.ClusterList](ctrl)
+				clustersController.EXPECT().List("", metav1.ListOptions{}).
+					Return(&fleet.ClusterList{
+						Items: []fleet.Cluster{
+							{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:      "cluster",
+									Namespace: "fleet-default",
+								},
+								Spec: fleet.ClusterSpec{
+									KubeConfigSecret: "my-kubeconfig-secret",
+								},
+								Status: fleet.ClusterStatus{
+									APIServerURL:              "https://hello.secret.world",
+									APIServerCAHash:           hashStatusField("secret-foo"),
+									AgentTLSMode:              "system-store",
+									GarbageCollectionInterval: &metav1.Duration{Duration: 10 * time.Minute},
+								},
+							},
+						},
+					}, nil)
+
+				secretsCache := fake.NewMockCacheInterface[*corev1.Secret](ctrl)
+				secretsCache.EXPECT().Get(gomock.Any(), "my-kubeconfig-secret").Return(&corev1.Secret{
+					Data: map[string][]byte{
+						"apiServerURL": []byte("https://hello.secret.world"),
+						"apiServerCA":  []byte(hashStatusField("secret-foo")),
+					},
+				}, nil)
+
+				// No UpdateStatus expected
+
+				return importHandler{
+					clusters: clustersController,
+					secrets:  secretsCache,
+				}
+			},
+		},
+		"URL and CA in secret, trigger import when agent TLS mode changes": {
+			cfg: config.Config{
+				APIServerCA:               []byte("foo"),
+				APIServerURL:              "https://hello.world",
+				AgentTLSMode:              "strict",
+				GarbageCollectionInterval: metav1.Duration{Duration: 10 * time.Minute},
+			},
+			handlerWithMocks: func() importHandler {
+				ctrl := gomock.NewController(t)
+
+				clustersController := fake.NewMockControllerInterface[*fleet.Cluster, *fleet.ClusterList](ctrl)
+				clustersController.EXPECT().List("", metav1.ListOptions{}).
+					Return(&fleet.ClusterList{
+						Items: []fleet.Cluster{
+							{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:      "cluster",
+									Namespace: "fleet-default",
+								},
+								Spec: fleet.ClusterSpec{
+									KubeConfigSecret: "my-kubeconfig-secret",
+								},
+								Status: fleet.ClusterStatus{
+									APIServerURL:              "https://hello.secret.world",
+									APIServerCAHash:           hashStatusField("secret-foo"),
+									AgentTLSMode:              "system-store",
+									GarbageCollectionInterval: &metav1.Duration{Duration: 10 * time.Minute},
+								},
+							},
+						},
+					}, nil)
+
+				secretsCache := fake.NewMockCacheInterface[*corev1.Secret](ctrl)
+				secretsCache.EXPECT().Get(gomock.Any(), "my-kubeconfig-secret").Return(&corev1.Secret{
+					Data: map[string][]byte{
+						"apiServerURL": []byte("https://hello.secret.world"),
+						"apiServerCA":  []byte(hashStatusField("secret-foo")),
+					},
+				}, nil)
+
+				clustersController.EXPECT().UpdateStatus(gomock.Any()) // import triggered
+
+				return importHandler{
+					clusters: clustersController,
+					secrets:  secretsCache,
+				}
+			},
+		},
+	}
+
+	for name, c := range cases {
+		t.Run(name, func(t *testing.T) {
+			ih := c.handlerWithMocks()
+
+			err := ih.onConfig(&c.cfg)
+			if err != nil {
+				t.Errorf("unexpected error: expected nil, got %v", err)
+			}
+
+		})
+	}
+}