Repository: rancher/fleet
Pull request #3553 (2025-04-14 13:48:27)

Removes the random string separator calculation.
For being too complex for an issue that will possibly never occur and because the implementation fails when the strings in the bundle description have a suffix that matches the beginning of the random string generated.

It tries now all the possible characters, and if all of them are found in the bundle definitions, it returns an error.

I'll also add that to the documentation.



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 &&
diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go
index 12241e8d..f1bbfb1f 100644
--- a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go
+++ b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go
@@ -725,6 +725,7 @@ func (r *GitJobReconciler) newJobSpec(ctx context.Context, gitrepo *v1alpha1.Git
 	}
 
 	drivenScanSeparator := ""
+	var err error
 	if len(gitrepo.Spec.Bundles) > 0 {
 		paths = []string{}
 		// use driven scan instead
@@ -735,7 +736,10 @@ func (r *GitJobReconciler) newJobSpec(ctx context.Context, gitrepo *v1alpha1.Git
 		// directories for the classic fleet scan, but since we need to
 		// pass 2 strings, we will separate them with
 		// the calculated separator.
-		drivenScanSeparator = getDrivenScanSeparator(*gitrepo)
+		drivenScanSeparator, err = getDrivenScanSeparator(*gitrepo)
+		if err != nil {
+			return nil, err
+		}
 		for _, b := range gitrepo.Spec.Bundles {
 			path := b.Base
 			if b.Options != "" {
@@ -1601,50 +1605,30 @@ func gitSSHCommandEnvVar(strictChecks bool) corev1.EnvVar {
 // definitions in the given GitRepo.
 // Since we cannot disregard the possibility that a user might have an
 // unavoidable need to use the character ":" (or another character typically not
-// used in directory or file paths), we need to find a separator that works for any
-// possible combination.
-// The function will first search for simple characters from those in
-// bundleOptionsSeparatorChars, and if none of them can be used, it will create
-// separators consisting of random strings with incremental length until it finds one
-// that meets our requirements.
-func getDrivenScanSeparator(gitrepo v1alpha1.GitRepo) string {
+// used in directory or file paths), we need to find possible alternatives.
+// The function will search for simple characters from those in
+// bundleOptionsSeparatorChars, and if none of them can be used, it will return an error.
+func getDrivenScanSeparator(gitrepo v1alpha1.GitRepo) (string, error) {
 	for _, sep := range bundleOptionsSeparatorChars {
-		if !separatorInBundleDefinitions(gitrepo, string(sep)) {
+		if !separatorInBundleDefinitions(gitrepo, sep) {
 			// We can safely use this separator
-			return string(sep)
+			return string(sep), nil
 		}
 	}
-	// we need to create a string separator because all the bytes in bundleOptionsSeparatorChars are
-	// used in the bundle definitions paths.
-	// Get a random combination of increasing length until it is not found in the bundles definitions.
-	length := 2
-	sep := getRandomStringSeparator(length)
-	for separatorInBundleDefinitions(gitrepo, sep) {
-		length++
-		sep = getRandomStringSeparator(length)
-	}
 
-	return sep
+	return "", fmt.Errorf("bundle base and/or options paths contain all possible characters from %q, please update those paths to remedy this", bundleOptionsSeparatorChars)
 }
 
-func separatorInBundleDefinitions(gitrepo v1alpha1.GitRepo, sep string) bool {
+func separatorInBundleDefinitions(gitrepo v1alpha1.GitRepo, sep rune) bool {
 	for _, b := range gitrepo.Spec.Bundles {
-		if strings.Contains(b.Options, sep) {
+		if strings.ContainsRune(b.Options, sep) {
 			return true
 		}
 
-		if strings.Contains(b.Base, sep) {
+		if strings.ContainsRune(b.Base, sep) {
 			return true
 		}
 	}
 
 	return false
 }
-
-func getRandomStringSeparator(length int) string {
-	b := make([]byte, length)
-	for i := range b {
-		b[i] = bundleOptionsSeparatorChars[rand.IntN(len(bundleOptionsSeparatorChars))]
-	}
-	return string(b)
-}


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)
+			}
+
+		})
+	}
+}
diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_test.go b/internal/cmd/controller/gitops/reconciler/gitjob_test.go
index 1253933c..40c5b2a2 100644
--- a/internal/cmd/controller/gitops/reconciler/gitjob_test.go
+++ b/internal/cmd/controller/gitops/reconciler/gitjob_test.go
@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"os"
 	"slices"
-	"strings"
 	"testing"
 	"time"
 
@@ -2314,10 +2313,9 @@ func TestGitClonerSSH(t *testing.T) {
 
 func TestDrivenScanSeparator(t *testing.T) {
 	tests := map[string]struct {
-		bundles                    []fleetv1.BundlePath
-		expectedRandomResult       bool
-		expectedResult             string
-		expectedResultRandomLength int
+		bundles        []fleetv1.BundlePath
+		expectError    bool
+		expectedResult string
 	}{
 		"Bundle definitions have no separator character": {
 			bundles: []fleetv1.BundlePath{
@@ -2330,8 +2328,8 @@ func TestDrivenScanSeparator(t *testing.T) {
 					Options: "options2.yaml",
 				},
 			},
-			expectedRandomResult: false,
-			expectedResult:       ":",
+			expectError:    false,
+			expectedResult: ":",
 		},
 		"Bundle definitions have : separator character": {
 			bundles: []fleetv1.BundlePath{
@@ -2344,8 +2342,8 @@ func TestDrivenScanSeparator(t *testing.T) {
 					Options: "options2.yaml",
 				},
 			},
-			expectedRandomResult: false,
-			expectedResult:       ",",
+			expectError:    false,
+			expectedResult: ",",
 		},
 		"Bundle definitions have : and , separator characters": {
 			bundles: []fleetv1.BundlePath{
@@ -2358,8 +2356,8 @@ func TestDrivenScanSeparator(t *testing.T) {
 					Options: "options2.yaml",
 				},
 			},
-			expectedRandomResult: false,
-			expectedResult:       "|",
+			expectError:    false,
+			expectedResult: "|",
 		},
 		"Bundle definitions have : , and | separator characters": {
 			bundles: []fleetv1.BundlePath{
@@ -2372,8 +2370,8 @@ func TestDrivenScanSeparator(t *testing.T) {
 					Options: "options2|.yaml",
 				},
 			},
-			expectedRandomResult: false,
-			expectedResult:       "?",
+			expectError:    false,
+			expectedResult: "?",
 		},
 		"Bundle definitions have : ,  | and ? separator characters": {
 			bundles: []fleetv1.BundlePath{
@@ -2386,8 +2384,8 @@ func TestDrivenScanSeparator(t *testing.T) {
 					Options: "options2|.yaml",
 				},
 			},
-			expectedRandomResult: false,
-			expectedResult:       "<",
+			expectError:    false,
+			expectedResult: "<",
 		},
 		"Bundle definitions have : ,  |  ? and < separator characters": {
 			bundles: []fleetv1.BundlePath{
@@ -2400,10 +2398,10 @@ func TestDrivenScanSeparator(t *testing.T) {
 					Options: "options2|.yaml",
 				},
 			},
-			expectedRandomResult: false,
-			expectedResult:       ">",
+			expectError:    false,
+			expectedResult: ">",
 		},
-		"Bundle definitions all separator characters": {
+		"Bundle definitions have all separator characters": {
 			bundles: []fleetv1.BundlePath{
 				{
 					Base:    "test?one:two",
@@ -2414,9 +2412,8 @@ func TestDrivenScanSeparator(t *testing.T) {
 					Options: "options2|.yaml",
 				},
 			},
-			expectedRandomResult:       true,
-			expectedResult:             "",
-			expectedResultRandomLength: 2,
+			expectError:    true,
+			expectedResult: "",
 		},
 	}
 	for name, test := range tests {
@@ -2426,61 +2423,21 @@ func TestDrivenScanSeparator(t *testing.T) {
 					Bundles: test.bundles,
 				},
 			}
-			separator := getDrivenScanSeparator(*gitrepo)
-			if !test.expectedRandomResult {
+			separator, err := getDrivenScanSeparator(*gitrepo)
+			if !test.expectError {
+				if err != nil {
+					t.Errorf("unexpected error: %v", err)
+				}
 				if test.expectedResult != separator {
 					t.Errorf("expecting separator to be: %q, got: %q", test.expectedResult, separator)
 				}
 			} else {
-				// Verify that the string has the desired length and
-				// is composed only of the specified characters.
-				if len(separator) != test.expectedResultRandomLength {
-					t.Errorf("expecting separator length to be: %d, got: %d", test.expectedResultRandomLength, len(separator))
-				}
-				for _, char := range separator {
-					if !strings.ContainsRune(bundleOptionsSeparatorChars, char) {
-						t.Errorf("expecting separator to only contain characters from the set: %q, got: %q", bundleOptionsSeparatorChars, separator)
-					}
-				}
-				for _, b := range gitrepo.Spec.Bundles {
-					// use the separator and verify we get the expected items
-					concatStr := b.Base + separator + b.Options
-					items := strings.Split(concatStr, separator)
-					if len(items) != 2 {
-						t.Errorf("expecting to have exactly 2 items, got: %d", len(items))
-					}
-					if items[0] != b.Base {
-						t.Errorf("expecting first item to be: %q, got: %q", b.Base, items[0])
-					}
-					if items[1] != b.Options {
-						t.Errorf("expecting second item to be: %q, got: %q", b.Options, items[1])
-					}
-				}
-				// Take it a step further and use the random separator in the bundle definitions.
-				// This will verify that the function returns a longer separator.
-				gitrepo = &fleetv1.GitRepo{
-					Spec: fleetv1.GitRepoSpec{
-						Bundles: []fleetv1.BundlePath{
-							{
-								Base:    fmt.Sprintf("test?one:%stwo", separator),
-								Options: "options.yaml",
-							},
-							{
-								Base:    "test,one<>",
-								Options: "options2|.yaml",
-							},
-						},
-					},
+				expectedErrorMessage := fmt.Sprintf("bundle base and/or options paths contain all possible characters from %q, please update those paths to remedy this", bundleOptionsSeparatorChars)
+				if err == nil {
+					t.Errorf("expecting error, got none")
 				}
-				// Finally, verify that the new separator is also valid.
-				newSeparator := getDrivenScanSeparator(*gitrepo)
-				if len(newSeparator) < len(separator) {
-					t.Errorf("expecting separator length to be greater or equal to %d, got: %d", len(separator), len(newSeparator))
-				}
-				for _, char := range newSeparator {
-					if !strings.ContainsRune(bundleOptionsSeparatorChars, char) {
-						t.Errorf("expecting separator to only contain characters from the set: %q, got: %q", bundleOptionsSeparatorChars, separator)
-					}
+				if err.Error() != expectedErrorMessage {
+					t.Errorf("expecting error %q, got %q", expectedErrorMessage, err.Error())
 				}
 			}
 		})