From a72b823453590ae752a37156567380d59080ea5f Mon Sep 17 00:00:00 2001 From: s3rj1k Date: Sat, 27 Sep 2025 19:45:48 +0200 Subject: [PATCH 1/2] Add EllipsizeName utility to handle Kubernetes object name limits --- api/v1beta1/zz_generated.deepcopy.go | 5 +- cmd/main.go | 14 +- controllers/handlers_helm.go | 11 +- controllers/profile_utils.go | 39 ++++- lib/clusterops/clusterreports.go | 13 +- lib/clusterops/clustersummary.go | 11 +- lib/utils/names.go | 116 ++++++++++++ lib/utils/names_test.go | 252 +++++++++++++++++++++++++++ test/fv/fv_suite_test.go | 4 + test/fv/utils_test.go | 4 +- 10 files changed, 444 insertions(+), 25 deletions(-) create mode 100644 lib/utils/names.go create mode 100644 lib/utils/names_test.go diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 7b45c814..110e4df8 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -21,12 +21,11 @@ limitations under the License. package v1beta1 import ( + apiv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" corev1 "k8s.io/api/core/v1" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1" runtime "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" - - apiv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. diff --git a/cmd/main.go b/cmd/main.go index 1eef7bc4..fcfb92ac 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -56,17 +56,17 @@ import ( metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" "sigs.k8s.io/controller-runtime/pkg/webhook" - libsveltosv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" - "github.com/projectsveltos/libsveltos/lib/crd" - "github.com/projectsveltos/libsveltos/lib/deployer" - logs "github.com/projectsveltos/libsveltos/lib/logsettings" - libsveltosset "github.com/projectsveltos/libsveltos/lib/set" - configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1" "github.com/projectsveltos/addon-controller/api/v1beta1/index" "github.com/projectsveltos/addon-controller/controllers" "github.com/projectsveltos/addon-controller/controllers/dependencymanager" "github.com/projectsveltos/addon-controller/internal/telemetry" + "github.com/projectsveltos/addon-controller/lib/utils" + libsveltosv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" + "github.com/projectsveltos/libsveltos/lib/crd" + "github.com/projectsveltos/libsveltos/lib/deployer" + logs "github.com/projectsveltos/libsveltos/lib/logsettings" + libsveltosset "github.com/projectsveltos/libsveltos/lib/set" //+kubebuilder:scaffold:imports ) @@ -183,6 +183,8 @@ func main() { controllers.SetDriftDetectionRegistry(registry) controllers.SetAgentInMgmtCluster(agentInMgmtCluster) + utils.GetNameManager().SetClient(mgr.GetClient()) + // Start dependency manager dependencymanager.InitializeManagerInstance(ctx, mgr.GetClient(), autoDeployDependencies, ctrl.Log.WithName("dependency_manager")) diff --git a/controllers/handlers_helm.go b/controllers/handlers_helm.go index fc98ae5c..28408d70 100644 --- a/controllers/handlers_helm.go +++ b/controllers/handlers_helm.go @@ -76,6 +76,7 @@ import ( "github.com/projectsveltos/addon-controller/controllers/chartmanager" "github.com/projectsveltos/addon-controller/controllers/clustercache" "github.com/projectsveltos/addon-controller/lib/clusterops" + "github.com/projectsveltos/addon-controller/lib/utils" libsveltosv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" "github.com/projectsveltos/libsveltos/lib/clusterproxy" "github.com/projectsveltos/libsveltos/lib/deployer" @@ -92,7 +93,7 @@ var ( ) const ( - writeFilePermission = 0644 + writeFilePermission = 0o644 lockTimeout = 30 notInstalledMessage = "Not installed yet and action is uninstall" defaultMaxHistory = 2 @@ -2432,8 +2433,14 @@ func updateClusterReportWithHelmReports(ctx context.Context, c client.Client, return err } - clusterReportName := clusterops.GetClusterReportName(profileOwnerRef.Kind, profileOwnerRef.Name, + fullName := clusterops.GetClusterReportName(profileOwnerRef.Kind, profileOwnerRef.Name, clusterSummary.Spec.ClusterName, clusterSummary.Spec.ClusterType) + clusterReportName, err := utils.GetNameManager().AllocateName(ctx, clusterSummary.Spec.ClusterNamespace, + fullName, &configv1beta1.ClusterReport{}, + ) + if err != nil { + return nil + } err = retry.RetryOnConflict(retry.DefaultRetry, func() error { clusterReport := &configv1beta1.ClusterReport{} diff --git a/controllers/profile_utils.go b/controllers/profile_utils.go index f0c0874a..1a8ce501 100644 --- a/controllers/profile_utils.go +++ b/controllers/profile_utils.go @@ -41,6 +41,7 @@ import ( configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1" "github.com/projectsveltos/addon-controller/controllers/dependencymanager" "github.com/projectsveltos/addon-controller/lib/clusterops" + "github.com/projectsveltos/addon-controller/lib/utils" "github.com/projectsveltos/addon-controller/pkg/scope" libsveltosv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" "github.com/projectsveltos/libsveltos/lib/clusterproxy" @@ -545,9 +546,16 @@ func updateClusterSummary(ctx context.Context, c client.Client, profileScope *sc clusterSummary.Annotations = profileScope.Profile.GetAnnotations() clusterSummary.Spec.ClusterProfileSpec = *profileScope.GetSpec() clusterSummary.Spec.ClusterType = clusterproxy.GetClusterType(cluster) + + fullName := clusterops.GetClusterSummaryName(profileScope.GetKind(), profileScope.Name(), + cluster.Name, cluster.APIVersion == libsveltosv1beta1.GroupVersion.String()) addClusterSummaryLabels(clusterSummary, profileScope, cluster) // Copy annotation. Paused annotation might be set on ClusterProfile. clusterSummary.Annotations = profileScope.Profile.GetAnnotations() + if clusterSummary.Annotations == nil { + clusterSummary.Annotations = make(map[string]string) + } + clusterSummary.Annotations[clusterops.FullNameAnnotation] = fullName return c.Update(ctx, clusterSummary) } @@ -583,8 +591,12 @@ func addClusterSummaryLabels(clusterSummary *configv1beta1.ClusterSummary, profi func createClusterSummary(ctx context.Context, c client.Client, profileScope *scope.ProfileScope, cluster *corev1.ObjectReference) error { - clusterSummaryName := clusterops.GetClusterSummaryName(profileScope.GetKind(), profileScope.Name(), + fullName := clusterops.GetClusterSummaryName(profileScope.GetKind(), profileScope.Name(), cluster.Name, cluster.APIVersion == libsveltosv1beta1.GroupVersion.String()) + clusterSummaryName, err := utils.GetNameManager().AllocateName(ctx, cluster.Namespace, fullName, &configv1beta1.ClusterSummary{}) + if err != nil { + return err + } clusterSummary := &configv1beta1.ClusterSummary{ ObjectMeta: metav1.ObjectMeta{ @@ -612,6 +624,10 @@ func createClusterSummary(ctx context.Context, c client.Client, profileScope *sc addClusterSummaryLabels(clusterSummary, profileScope, cluster) // Copy annotation. Paused annotation might be set on ClusterProfile. clusterSummary.Annotations = profileScope.Profile.GetAnnotations() + if clusterSummary.Annotations == nil { + clusterSummary.Annotations = make(map[string]string) + } + clusterSummary.Annotations[clusterops.FullNameAnnotation] = fullName return c.Create(ctx, clusterSummary) } @@ -903,6 +919,13 @@ func createClusterReport(ctx context.Context, c client.Client, profile client.Ob clusterType := clusterproxy.GetClusterType(cluster) + fullName := clusterops.GetClusterReportName(profile.GetObjectKind().GroupVersionKind().Kind, profile.GetName(), + cluster.Name, clusterType) + clusterReportName, err := utils.GetNameManager().AllocateName(ctx, cluster.Namespace, fullName, &configv1beta1.ClusterReport{}) + if err != nil { + return err + } + lbls := map[string]string{ configv1beta1.ClusterNameLabel: cluster.Name, configv1beta1.ClusterTypeLabel: string(clusterproxy.GetClusterType(cluster)), @@ -913,12 +936,16 @@ func createClusterReport(ctx context.Context, c client.Client, profile client.Ob lbls[clusterops.ProfileLabelName] = profile.GetName() } + annotations := map[string]string{ + clusterops.FullNameAnnotation: fullName, + } + clusterReport := &configv1beta1.ClusterReport{ ObjectMeta: metav1.ObjectMeta{ - Namespace: cluster.Namespace, - Name: clusterops.GetClusterReportName(profile.GetObjectKind().GroupVersionKind().Kind, profile.GetName(), - cluster.Name, clusterType), - Labels: lbls, + Namespace: cluster.Namespace, + Name: clusterReportName, + Labels: lbls, + Annotations: annotations, OwnerReferences: []metav1.OwnerReference{ { Kind: profile.GetObjectKind().GroupVersionKind().Kind, @@ -934,7 +961,7 @@ func createClusterReport(ctx context.Context, c client.Client, profile client.Ob }, } - err := c.Create(ctx, clusterReport) + err = c.Create(ctx, clusterReport) if err != nil { if apierrors.IsAlreadyExists(err) { return nil diff --git a/lib/clusterops/clusterreports.go b/lib/clusterops/clusterreports.go index 6ea37c5e..7dc76971 100644 --- a/lib/clusterops/clusterreports.go +++ b/lib/clusterops/clusterreports.go @@ -26,6 +26,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1" + "github.com/projectsveltos/addon-controller/lib/utils" libsveltosv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" ) @@ -45,9 +46,13 @@ func UpdateClusterReportWithResourceReports(ctx context.Context, c client.Client return nil } - clusterReportName := GetClusterReportName(profileRef.Kind, profileRef.Name, clusterName, clusterType) + fullName := GetClusterReportName(profileRef.Kind, profileRef.Name, clusterName, clusterType) + clusterReportName, err := utils.GetNameManager().AllocateName(ctx, clusterNamespace, fullName, &configv1beta1.ClusterReport{}) + if err != nil { + return err + } - err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { clusterReport := &configv1beta1.ClusterReport{} err := c.Get(ctx, types.NamespacedName{Namespace: clusterNamespace, Name: clusterReportName}, clusterReport) @@ -69,13 +74,13 @@ func UpdateClusterReportWithResourceReports(ctx context.Context, c client.Client } func GetClusterReportName(profileKind, profileName, clusterName string, clusterType libsveltosv1beta1.ClusterType) string { - // TODO: shorten this value prefix := "" // For backward compatibility (before addition of Profile) leave this empty for ClusterProfiles if profileKind == configv1beta1.ProfileKind { prefix = "p--" } - return prefix + profileName + nameSeparator + strings.ToLower(string(clusterType)) + + name := prefix + profileName + nameSeparator + strings.ToLower(string(clusterType)) + nameSeparator + clusterName + return name } // ConvertResourceReportsToObjectReference converts a slice of ResourceReports to diff --git a/lib/clusterops/clustersummary.go b/lib/clusterops/clustersummary.go index 29666d13..42928352 100644 --- a/lib/clusterops/clustersummary.go +++ b/lib/clusterops/clustersummary.go @@ -40,6 +40,9 @@ const ( // ProfileLabelName is added to all ClusterSummary instances created // by a Profile instance ProfileLabelName = "projectsveltos.io/profile-name" + + // FullNameAnnotation stores the full name before ellipsization + FullNameAnnotation = "projectsveltos.io/full-name" ) // GetClusterSummary returns the ClusterSummary instance created by a specific @@ -88,12 +91,16 @@ func GetClusterSummaryName(profileKind, profileName, clusterName string, isSvelt clusterType = libsveltosv1beta1.ClusterTypeSveltos } prefix := GetPrefix(clusterType) + + var name string if profileKind == configv1beta1.ClusterProfileKind { // For backward compatibility (code before addition of Profiles) do not change this - return fmt.Sprintf("%s-%s-%s", profileName, prefix, clusterName) + name = fmt.Sprintf("%s-%s-%s", profileName, prefix, clusterName) + } else { + name = fmt.Sprintf("p--%s-%s-%s", profileName, prefix, clusterName) } - return fmt.Sprintf("p--%s-%s-%s", profileName, prefix, clusterName) + return name } func GetPrefix(clusterType libsveltosv1beta1.ClusterType) string { diff --git a/lib/utils/names.go b/lib/utils/names.go new file mode 100644 index 00000000..3aa59490 --- /dev/null +++ b/lib/utils/names.go @@ -0,0 +1,116 @@ +package utils + +import ( + "context" + "crypto/rand" + "encoding/binary" + "fmt" + "hash/fnv" + "sync" + + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + maxNameLength = 63 + suffixLength = 10 // dash + prefix letter + 8 hex chars + maxBaseLength = maxNameLength - suffixLength +) + +var ( + nameManager = &NameManager{ + mu: sync.Mutex{}, + } +) + +// NameManager handles ClusterSummary name allocation +type NameManager struct { + client client.Client + mu sync.Mutex +} + +func GetNameManager() *NameManager { + return nameManager +} + +func (nm *NameManager) SetClient(c client.Client) { + nm.mu.Lock() + defer nm.mu.Unlock() + nm.client = c +} + +// AllocateName allocates a name with collision handling +func (nm *NameManager) AllocateName(ctx context.Context, namespace, fullName string, obj client.Object) (string, error) { + nm.mu.Lock() + defer nm.mu.Unlock() + + if len(fullName) <= maxNameLength { + return fullName, nil + } + + var ( + ellipsized string + err error + ) + + ellipsized, err = EllipsizeName(fullName) + if err != nil { + return "", err + } + + if nm.client != nil && nm.exists(ctx, namespace, ellipsized, obj) { + ellipsized, err = EllipsizeNameWithRand(ellipsized) + if err != nil { + return "", err + } + } + + return ellipsized, nil +} + +func (nm *NameManager) exists(ctx context.Context, namespace, name string, obj client.Object) bool { + err := nm.client.Get(ctx, types.NamespacedName{ + Namespace: namespace, + Name: name, + }, obj) + return err == nil +} + +func truncateForSuffix(name string) string { + runes := []rune(name) + if len(runes) > maxBaseLength { + return string(runes[:maxBaseLength]) + } + return name +} + +// EllipsizeNameWithRand creates a name with random suffix for collision resolution +func EllipsizeNameWithRand(base string) (string, error) { + var randomBytes [4]byte + if _, err := rand.Read(randomBytes[:]); err != nil { + return "", fmt.Errorf("failed to generate random suffix: %w", err) + } + + randomValue := binary.BigEndian.Uint32(randomBytes[:]) + + return fmt.Sprintf("%s-r%08x", + truncateForSuffix(base), + randomValue, + ), nil +} + +// EllipsizeName ensures a Kubernetes object name is <= 63 characters +func EllipsizeName(name string) (string, error) { + h := fnv.New32a() + + _, err := h.Write([]byte(name)) + if err != nil { + return "", fmt.Errorf("failed to write hash: %w", err) + } + + return fmt.Sprintf("%s-h%08x", + truncateForSuffix(name), + h.Sum32(), + ), nil +} diff --git a/lib/utils/names_test.go b/lib/utils/names_test.go new file mode 100644 index 00000000..073306b3 --- /dev/null +++ b/lib/utils/names_test.go @@ -0,0 +1,252 @@ +package utils + +import ( + "context" + "regexp" + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1" +) + +func setupScheme() (*runtime.Scheme, error) { + s := runtime.NewScheme() + if err := configv1beta1.AddToScheme(s); err != nil { + return nil, err + } + if err := clientgoscheme.AddToScheme(s); err != nil { + return nil, err + } + return s, nil +} + +func testEllipsizeFunc(t *testing.T, tests []struct { + name string + input string +}, fn func(string) (string, error), suffixPattern *regexp.Regexp, +) { + + t.Helper() + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := fn(tt.input) + if err != nil { + t.Errorf("error not expected: %v", err) + } + + if len(result) > maxNameLength { + t.Errorf("result length = %d, want <= %d", len(result), maxNameLength) + } + + if !suffixPattern.MatchString(result) { + t.Errorf("result %q should match pattern %s", result, suffixPattern.String()) + } + + if result == tt.input { + t.Errorf("result should differ from input") + } + + if len(tt.input)+suffixLength > maxNameLength { + if len(result) != maxNameLength { + t.Errorf("result length = %d, want exactly %d for long input", len(result), maxNameLength) + } + } + }) + } +} + +func TestEllipsizeName(t *testing.T) { + tests := []struct { + name string + input string + }{ + { + name: "short name gets hash", + input: "short", + }, + { + name: "medium name gets hash", + input: "my-profile-capi-my-cluster", + }, + { + name: "at max length gets truncated and hash", + input: strings.Repeat("a", maxNameLength), + }, + { + name: "over max length gets ellipsized", + input: strings.Repeat("a", maxNameLength+1), + }, + { + name: "very long name gets ellipsized", + input: strings.Repeat("a", maxNameLength*2), + }, + { + name: "real cluster summary name", + input: "very-long-profile-name-that-exceeds-limits-capi-very-long-cluster-name-that-also-exceeds", + }, + { + name: "real cluster report name", + input: "p--very-long-profile-name-that-exceeds-limits--capi--very-long-cluster-name-that-also-exceeds", + }, + } + + hashSuffixPattern := regexp.MustCompile(`-h[0-9a-f]{8}$`) + testEllipsizeFunc(t, tests, EllipsizeName, hashSuffixPattern) +} + +func TestEllipsizeNameWithRand(t *testing.T) { + tests := []struct { + name string + input string + }{ + { + name: "short name gets random suffix", + input: "short", + }, + { + name: "medium name gets random suffix", + input: "my-profile-capi-my-cluster", + }, + { + name: "at max base length", + input: strings.Repeat("a", maxBaseLength), + }, + { + name: "over max base length gets truncated", + input: strings.Repeat("a", maxBaseLength+1), + }, + { + name: "very long name gets truncated", + input: strings.Repeat("a", maxNameLength*2), + }, + { + name: "real cluster summary name", + input: "very-long-profile-name-that-exceeds-limits-capi-very-long-cluster-name-that-also-exceeds", + }, + { + name: "ellipsized name with hash suffix", + input: "very-long-profile-name-that-exceeds-limits-capi-very-l-h52674346", + }, + } + + randomSuffixPattern := regexp.MustCompile(`-r[0-9a-f]{8}$`) + testEllipsizeFunc(t, tests, EllipsizeNameWithRand, randomSuffixPattern) +} + +func TestNameManager(t *testing.T) { //nolint:funlen // nested tests + t.Run("with nil client", func(t *testing.T) { + nm := &NameManager{} + + tests := []struct { + name string + fullName string + }{ + { + name: "short name", + fullName: "short-name", + }, + { + name: "long name gets ellipsized", + fullName: "very-long-profile-name-that-exceeds-limits-capi-very-long-cluster-name", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := nm.AllocateName(context.Background(), "default", tt.fullName, nil) + if err != nil { + t.Errorf("error not expected: %v", err) + } + + if len(result) > maxNameLength { + t.Errorf("AllocateName() result length = %d, want <= %d", len(result), maxNameLength) + } + }) + } + }) + + t.Run("with client - no collision (no objects)", func(t *testing.T) { + scheme, err := setupScheme() + if err != nil { + t.Fatalf("setupScheme() failed: %v", err) + } + + c := fake.NewClientBuilder().WithScheme(scheme).Build() + + nm := &NameManager{} + nm.SetClient(c) + + fullName := "my-profile-capi-my-cluster" + result, err := nm.AllocateName(context.Background(), "default", fullName, &configv1beta1.ClusterSummary{}) + if err != nil { + t.Errorf("error not expected: %v", err) + } + + if len(result) > maxNameLength { + t.Errorf("AllocateName() result length = %d, want <= %d", len(result), maxNameLength) + } + + if result != fullName { + t.Errorf("AllocateName() result = %q, want %q (no collision, name should stay same)", result, fullName) + } + }) + + t.Run("with client - long name with collision", func(t *testing.T) { + scheme, err := setupScheme() + if err != nil { + t.Fatalf("setupScheme() failed: %v", err) + } + + namespace := "default" + fullName := "very-long-profile-name-that-exceeds-limits-capi-very-long-cluster-name-that-also-exceeds" + ellipsizedName, err := EllipsizeName(fullName) + if err != nil { + t.Errorf("error not expected: %v", err) + } + + existingClusterSummary := &configv1beta1.ClusterSummary{ + ObjectMeta: metav1.ObjectMeta{ + Name: ellipsizedName, + Namespace: namespace, + }, + } + + initObjects := []client.Object{existingClusterSummary} + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(initObjects...). + Build() + + nm := &NameManager{} + nm.SetClient(c) + + result, err := nm.AllocateName(context.Background(), namespace, fullName, &configv1beta1.ClusterSummary{}) + if err != nil { + t.Errorf("error not expected: %v", err) + } + + if len(result) > maxNameLength { + t.Errorf("AllocateName() result length = %d, want <= %d", len(result), maxNameLength) + } + + if len(result) != maxNameLength { + t.Errorf("AllocateName() result length = %d, want exactly %d for long name with collision", len(result), maxNameLength) + } + + if result == ellipsizedName { + t.Errorf("AllocateName() result = %q, want different name due to collision", result) + } + + randomSuffixPattern := regexp.MustCompile(`-r[0-9a-f]{8}$`) + if !randomSuffixPattern.MatchString(result) { + t.Errorf("AllocateName() result %q should end with -r<8hexchars> due to collision", result) + } + }) +} diff --git a/test/fv/fv_suite_test.go b/test/fv/fv_suite_test.go index 9fe74dea..21744284 100644 --- a/test/fv/fv_suite_test.go +++ b/test/fv/fv_suite_test.go @@ -41,6 +41,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1" + "github.com/projectsveltos/addon-controller/lib/utils" libsveltosv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" ) @@ -101,6 +102,9 @@ var _ = BeforeSuite(func() { k8sClient, err = client.New(restConfig, client.Options{Scheme: scheme}) Expect(err).NotTo(HaveOccurred()) + // Initialize NameManager with k8s client for tests + utils.GetNameManager().SetClient(k8sClient) + if isCAPIInstalled(context.TODO(), k8sClient) { verifyCAPICluster() } else { diff --git a/test/fv/utils_test.go b/test/fv/utils_test.go index a1d38681..0fad91c5 100644 --- a/test/fv/utils_test.go +++ b/test/fv/utils_test.go @@ -194,7 +194,7 @@ func deleteClusterProfile(clusterProfile *configv1beta1.ClusterProfile) { currentClusterSummary := &configv1beta1.ClusterSummary{} err := k8sClient.Get(context.TODO(), types.NamespacedName{Namespace: clusterSummaryNamespace, Name: clusterSummaryName}, currentClusterSummary) - if err == nil || !apierrors.IsNotFound(err) { + if err == nil { return false } } @@ -237,7 +237,7 @@ func deleteProfile(profile *configv1beta1.Profile) { currentClusterSummary := &configv1beta1.ClusterSummary{} err := k8sClient.Get(context.TODO(), types.NamespacedName{Namespace: clusterSummaryNamespace, Name: clusterSummaryName}, currentClusterSummary) - if err == nil || !apierrors.IsNotFound(err) { + if err == nil { return false } } From eaf63f0e30e1cd1a684a61a4ac441110568ef58e Mon Sep 17 00:00:00 2001 From: s3rj1k Date: Thu, 9 Oct 2025 16:01:36 +0200 Subject: [PATCH 2/2] Add cache layer for AllocateName --- cmd/main.go | 52 ++++-- controllers/clustersummary_controller.go | 3 + controllers/profile_utils.go | 3 + lib/utils/names.go | 78 +++++++- lib/utils/names_test.go | 218 ++++++++++++++++++++++- 5 files changed, 331 insertions(+), 23 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index fcfb92ac..d6e88c7a 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -61,6 +61,7 @@ import ( "github.com/projectsveltos/addon-controller/controllers" "github.com/projectsveltos/addon-controller/controllers/dependencymanager" "github.com/projectsveltos/addon-controller/internal/telemetry" + "github.com/projectsveltos/addon-controller/lib/clusterops" "github.com/projectsveltos/addon-controller/lib/utils" libsveltosv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" "github.com/projectsveltos/libsveltos/lib/crd" @@ -124,22 +125,7 @@ func main() { pflag.Parse() reportMode = controllers.ReportMode(tmpReportMode) - disableFor := []client.Object{} - byObject := map[client.Object]cache.ByObject{} - if disableCaching { - // Note: Only Secrets with type addons.projectsveltos.io/cluster-profile are cached - // The default client of the manager won't use the cache for secrets at all. - disableFor = []client.Object{ - &corev1.Secret{}, - &corev1.ConfigMap{}, - } - - fieldSelector := fields.OneTermEqualSelector("type", string(libsveltosv1beta1.ClusterProfileSecretType)) - - byObject[&corev1.Secret{}] = cache.ByObject{ - Field: fieldSelector, - } - } + disableFor, byObject := getCacheConfig() ctrl.SetLogger(klog.Background()) ctrlOptions := ctrl.Options{ @@ -185,6 +171,8 @@ func main() { utils.GetNameManager().SetClient(mgr.GetClient()) + rebuildNameCache(ctx) + // Start dependency manager dependencymanager.InitializeManagerInstance(ctx, mgr.GetClient(), autoDeployDependencies, ctrl.Log.WithName("dependency_manager")) @@ -216,6 +204,38 @@ func main() { } } +func getCacheConfig() (disableFor []client.Object, byObject map[client.Object]cache.ByObject) { + disableFor = []client.Object{} + byObject = map[client.Object]cache.ByObject{} + if disableCaching { + // Note: Only Secrets with type addons.projectsveltos.io/cluster-profile are cached + // The default client of the manager won't use the cache for secrets at all. + disableFor = []client.Object{ + &corev1.Secret{}, + &corev1.ConfigMap{}, + } + + fieldSelector := fields.OneTermEqualSelector("type", string(libsveltosv1beta1.ClusterProfileSecretType)) + + byObject[&corev1.Secret{}] = cache.ByObject{ + Field: fieldSelector, + } + } + return +} + +func rebuildNameCache(ctx context.Context) { + // Rebuild name cache from existing ClusterSummary and ClusterReport objects + setupLog.Info("Rebuilding name cache from existing ClusterSummary objects") + if err := utils.GetNameManager().RebuildCache(ctx, &configv1beta1.ClusterSummaryList{}, clusterops.FullNameAnnotation); err != nil { + setupLog.Error(err, "failed to rebuild cache for ClusterSummary") + } + setupLog.Info("Rebuilding name cache from existing ClusterReport objects") + if err := utils.GetNameManager().RebuildCache(ctx, &configv1beta1.ClusterReportList{}, clusterops.FullNameAnnotation); err != nil { + setupLog.Error(err, "failed to rebuild cache for ClusterReport") + } +} + func initFlags(fs *pflag.FlagSet) { fs.IntVar(&tmpReportMode, "report-mode", defaulReportMode, "Indicates how ReportSummaries need to be collected") diff --git a/controllers/clustersummary_controller.go b/controllers/clustersummary_controller.go index 7d9467c6..dc9e8935 100644 --- a/controllers/clustersummary_controller.go +++ b/controllers/clustersummary_controller.go @@ -49,6 +49,7 @@ import ( configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1" "github.com/projectsveltos/addon-controller/controllers/chartmanager" "github.com/projectsveltos/addon-controller/lib/clusterops" + "github.com/projectsveltos/addon-controller/lib/utils" "github.com/projectsveltos/addon-controller/pkg/scope" libsveltosv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" "github.com/projectsveltos/libsveltos/lib/clusterproxy" @@ -1599,6 +1600,8 @@ func (r *ClusterSummaryReconciler) removeFinalizer(clusterSummaryScope *scope.Cl configv1beta1.ClusterSummaryFinalizer); !finalizersUpdated { return fmt.Errorf("failed to remove finalizer") } + utils.GetNameManager().RemoveName(clusterSummaryScope.ClusterSummary.Namespace, + clusterSummaryScope.ClusterSummary.Name) } return nil diff --git a/controllers/profile_utils.go b/controllers/profile_utils.go index 1a8ce501..70840023 100644 --- a/controllers/profile_utils.go +++ b/controllers/profile_utils.go @@ -838,6 +838,7 @@ func cleanClusterSummaries(ctx context.Context, c client.Client, profileScope *s cs.Namespace, cs.Name)) return err } + utils.GetNameManager().RemoveName(cs.Namespace, cs.Name) } } if err := updateClusterSummarySyncMode(ctx, c, cs, profileScope.GetSpec().SyncMode); err != nil { @@ -1001,6 +1002,8 @@ func cleanClusterReports(ctx context.Context, c client.Client, profileScope *sco if !apierrors.IsNotFound(err) { return err } + } else { + utils.GetNameManager().RemoveName(cr.Namespace, cr.Name) } } diff --git a/lib/utils/names.go b/lib/utils/names.go index 3aa59490..78e1ba36 100644 --- a/lib/utils/names.go +++ b/lib/utils/names.go @@ -8,6 +8,7 @@ import ( "hash/fnv" "sync" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -20,13 +21,15 @@ const ( var ( nameManager = &NameManager{ - mu: sync.Mutex{}, + cache: make(map[string]struct{}), + mu: sync.Mutex{}, } ) // NameManager handles ClusterSummary name allocation type NameManager struct { client client.Client + cache map[string]struct{} // Set of allocated object names (namespace/name) mu sync.Mutex } @@ -40,6 +43,63 @@ func (nm *NameManager) SetClient(c client.Client) { nm.client = c } +func getCacheKey(namespace, name string) string { + return namespace + "/" + name +} + +// RebuildCache rebuilds cache from existing objects on startup +func (nm *NameManager) RebuildCache(ctx context.Context, listObj client.ObjectList, fullNameAnnotation string) error { + nm.mu.Lock() + defer nm.mu.Unlock() + + if nm.client == nil { + return fmt.Errorf("client not set") + } + + if err := nm.client.List(ctx, listObj); err != nil { + return fmt.Errorf("failed to list objects: %w", err) + } + + // Extract items from the list using reflection + items, err := extractItems(listObj) + if err != nil { + return err + } + + for _, obj := range items { + cacheKey := getCacheKey(obj.GetNamespace(), obj.GetName()) + nm.cache[cacheKey] = struct{}{} + } + + return nil +} + +// extractItems extracts items from an ObjectList +func extractItems(list client.ObjectList) ([]client.Object, error) { + items, err := meta.ExtractList(list) + if err != nil { + return nil, fmt.Errorf("failed to extract items: %w", err) + } + + objects := make([]client.Object, 0, len(items)) + for _, item := range items { + if obj, ok := item.(client.Object); ok { + objects = append(objects, obj) + } + } + + return objects, nil +} + +// RemoveName removes name from cache to keep cache in sync +func (nm *NameManager) RemoveName(namespace, name string) { + nm.mu.Lock() + defer nm.mu.Unlock() + + cacheKey := getCacheKey(namespace, name) + delete(nm.cache, cacheKey) +} + // AllocateName allocates a name with collision handling func (nm *NameManager) AllocateName(ctx context.Context, namespace, fullName string, obj client.Object) (string, error) { nm.mu.Lock() @@ -59,13 +119,27 @@ func (nm *NameManager) AllocateName(ctx context.Context, namespace, fullName str return "", err } - if nm.client != nil && nm.exists(ctx, namespace, ellipsized, obj) { + cacheKey := getCacheKey(namespace, ellipsized) + if _, exists := nm.cache[cacheKey]; exists { + // Conflict in cache, use random suffix ellipsized, err = EllipsizeNameWithRand(ellipsized) if err != nil { return "", err } + } else if nm.client != nil { + if nm.exists(ctx, namespace, ellipsized, obj) { + // Object exists in API, update cache and resolve conflict + nm.cache[cacheKey] = struct{}{} + ellipsized, err = EllipsizeNameWithRand(ellipsized) + if err != nil { + return "", err + } + } } + cacheKey = getCacheKey(namespace, ellipsized) + nm.cache[cacheKey] = struct{}{} + return ellipsized, nil } diff --git a/lib/utils/names_test.go b/lib/utils/names_test.go index 073306b3..3503a509 100644 --- a/lib/utils/names_test.go +++ b/lib/utils/names_test.go @@ -15,6 +15,12 @@ import ( configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1" ) +const ( + testNamespace = "default" + testLongProfileName = "very-long-profile-name-that-exceeds-limits-capi-very-long-cluster-name" + fullNameAnnotation = "projectsveltos.io/full-name" +) + func setupScheme() (*runtime.Scheme, error) { s := runtime.NewScheme() if err := configv1beta1.AddToScheme(s); err != nil { @@ -142,7 +148,9 @@ func TestEllipsizeNameWithRand(t *testing.T) { func TestNameManager(t *testing.T) { //nolint:funlen // nested tests t.Run("with nil client", func(t *testing.T) { - nm := &NameManager{} + nm := &NameManager{ + cache: make(map[string]struct{}), + } tests := []struct { name string @@ -180,11 +188,13 @@ func TestNameManager(t *testing.T) { //nolint:funlen // nested tests c := fake.NewClientBuilder().WithScheme(scheme).Build() - nm := &NameManager{} + nm := &NameManager{ + cache: make(map[string]struct{}), + } nm.SetClient(c) fullName := "my-profile-capi-my-cluster" - result, err := nm.AllocateName(context.Background(), "default", fullName, &configv1beta1.ClusterSummary{}) + result, err := nm.AllocateName(context.Background(), testNamespace, fullName, &configv1beta1.ClusterSummary{}) if err != nil { t.Errorf("error not expected: %v", err) } @@ -204,7 +214,7 @@ func TestNameManager(t *testing.T) { //nolint:funlen // nested tests t.Fatalf("setupScheme() failed: %v", err) } - namespace := "default" + namespace := testNamespace fullName := "very-long-profile-name-that-exceeds-limits-capi-very-long-cluster-name-that-also-exceeds" ellipsizedName, err := EllipsizeName(fullName) if err != nil { @@ -224,7 +234,9 @@ func TestNameManager(t *testing.T) { //nolint:funlen // nested tests WithObjects(initObjects...). Build() - nm := &NameManager{} + nm := &NameManager{ + cache: make(map[string]struct{}), + } nm.SetClient(c) result, err := nm.AllocateName(context.Background(), namespace, fullName, &configv1beta1.ClusterSummary{}) @@ -250,3 +262,199 @@ func TestNameManager(t *testing.T) { //nolint:funlen // nested tests } }) } + +func TestCacheFunctionality(t *testing.T) { + t.Run("cache stores and retrieves names", testCacheStoresAndRetrievesNames) + t.Run("RebuildCache populates cache from existing objects", testRebuildCachePopulatesCache) + t.Run("cache handles different namespaces", testCacheHandlesDifferentNamespaces) +} + +func testCacheStoresAndRetrievesNames(t *testing.T) { + t.Helper() + nm := &NameManager{ + cache: make(map[string]struct{}), + } + + namespace := testNamespace + fullName := testLongProfileName + ellipsizedName, err := EllipsizeName(fullName) + if err != nil { + t.Fatalf("EllipsizeName() failed: %v", err) + } + + // First allocation should compute and cache + result1, err := nm.AllocateName(context.Background(), namespace, fullName, &configv1beta1.ClusterSummary{}) + if err != nil { + t.Errorf("AllocateName() error = %v", err) + } + + if result1 != ellipsizedName { + t.Errorf("AllocateName() first call = %q, want %q", result1, ellipsizedName) + } + + // Second allocation with same name should detect conflict and allocate different name + result2, err := nm.AllocateName(context.Background(), namespace, fullName, &configv1beta1.ClusterSummary{}) + if err != nil { + t.Errorf("AllocateName() error = %v", err) + } + + if result2 == result1 { + t.Errorf("AllocateName() second call should differ due to conflict, got same: %q", result2) + } + + // Verify cache contains both entries + cacheKey1 := getCacheKey(namespace, result1) + if _, ok := nm.cache[cacheKey1]; !ok { + t.Errorf("cache should contain key %q", cacheKey1) + } + + cacheKey2 := getCacheKey(namespace, result2) + if _, ok := nm.cache[cacheKey2]; !ok { + t.Errorf("cache should contain key %q", cacheKey2) + } +} + +func testRebuildCachePopulatesCache(t *testing.T) { + t.Helper() + scheme, err := setupScheme() + if err != nil { + t.Fatalf("setupScheme() failed: %v", err) + } + + namespace := testNamespace + fullName1 := "very-long-profile-name-that-exceeds-limits-capi-cluster-one-that-also-exceeds" + fullName2 := "another-very-long-profile-name-that-exceeds-limits-capi-cluster-two-that-also-exceeds" + + ellipsizedName1, err := EllipsizeName(fullName1) + if err != nil { + t.Fatalf("EllipsizeName() failed: %v", err) + } + + ellipsizedName2, err := EllipsizeName(fullName2) + if err != nil { + t.Fatalf("EllipsizeName() failed: %v", err) + } + + cs1, cs2 := createTestClusterSummaries(namespace, fullName1, fullName2, ellipsizedName1, ellipsizedName2) + + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(cs1, cs2). + Build() + + nm := &NameManager{ + cache: make(map[string]struct{}), + } + nm.SetClient(c) + + // Rebuild cache + err = nm.RebuildCache(context.Background(), &configv1beta1.ClusterSummaryList{}, fullNameAnnotation) + if err != nil { + t.Errorf("RebuildCache() error = %v", err) + } + + verifyCacheEntries(t, nm, namespace, ellipsizedName1, ellipsizedName2) + + // AllocateName should detect conflict and generate new name with random suffix + result, err := nm.AllocateName(context.Background(), namespace, fullName1, &configv1beta1.ClusterSummary{}) + if err != nil { + t.Errorf("AllocateName() error = %v", err) + } + + if result == ellipsizedName1 { + t.Errorf("AllocateName() result = %q, should differ from cached name due to conflict", result) + } + + // Verify result has random suffix format: name-r12345678 + if len(result) < suffixLength { + t.Errorf("AllocateName() result too short: %q", result) + } + suffix := result[len(result)-suffixLength:] + if suffix[0] != '-' || suffix[1] != 'r' { + t.Errorf("AllocateName() result = %q should have random suffix format '-rXXXXXXXX', got suffix: %q", result, suffix) + } +} + +func createTestClusterSummaries(namespace, fullName1, fullName2, ellipsizedName1, ellipsizedName2 string) (cs1, cs2 *configv1beta1.ClusterSummary) { + cs1 = &configv1beta1.ClusterSummary{ + ObjectMeta: metav1.ObjectMeta{ + Name: ellipsizedName1, + Namespace: namespace, + Annotations: map[string]string{ + fullNameAnnotation: fullName1, + }, + }, + } + + cs2 = &configv1beta1.ClusterSummary{ + ObjectMeta: metav1.ObjectMeta{ + Name: ellipsizedName2, + Namespace: namespace, + Annotations: map[string]string{ + fullNameAnnotation: fullName2, + }, + }, + } + + return +} + +func verifyCacheEntries(t *testing.T, nm *NameManager, namespace, ellipsizedName1, ellipsizedName2 string) { + t.Helper() + cacheKey1 := getCacheKey(namespace, ellipsizedName1) + if _, ok := nm.cache[cacheKey1]; !ok { + t.Errorf("cache should contain key %q", cacheKey1) + } + + cacheKey2 := getCacheKey(namespace, ellipsizedName2) + if _, ok := nm.cache[cacheKey2]; !ok { + t.Errorf("cache should contain key %q", cacheKey2) + } +} + +func testCacheHandlesDifferentNamespaces(t *testing.T) { + t.Helper() + nm := &NameManager{ + cache: make(map[string]struct{}), + } + + fullName := testLongProfileName + ellipsizedName1, err := EllipsizeName(fullName) + if err != nil { + t.Fatalf("EllipsizeName() failed: %v", err) + } + + ellipsizedName2, err := EllipsizeNameWithRand(ellipsizedName1) + if err != nil { + t.Fatalf("EllipsizeNameWithRand() failed: %v", err) + } + + // Manually populate cache with different namespaces + nm.cache[getCacheKey("namespace1", ellipsizedName1)] = struct{}{} + nm.cache[getCacheKey("namespace2", ellipsizedName2)] = struct{}{} + + // Allocate from namespace1 - should detect conflict and use different name + result1, err := nm.AllocateName(context.Background(), "namespace1", fullName, &configv1beta1.ClusterSummary{}) + if err != nil { + t.Errorf("AllocateName() error = %v", err) + } + + if result1 == ellipsizedName1 { + t.Errorf("AllocateName() namespace1 should differ from cached name due to conflict, got %q", result1) + } + + // Allocate from namespace2 - should detect conflict and use different name + result2, err := nm.AllocateName(context.Background(), "namespace2", fullName, &configv1beta1.ClusterSummary{}) + if err != nil { + t.Errorf("AllocateName() error = %v", err) + } + + if result2 == ellipsizedName2 { + t.Errorf("AllocateName() namespace2 should differ from cached name due to conflict, got %q", result2) + } + + // Results should be different from each other + if result1 == result2 { + t.Errorf("AllocateName() results should differ across namespaces") + } +}