diff --git a/integrationtests/controller/bundle/label_migration_test.go b/integrationtests/controller/bundle/label_migration_test.go new file mode 100644 index 0000000000..71d5665aad --- /dev/null +++ b/integrationtests/controller/bundle/label_migration_test.go @@ -0,0 +1,94 @@ +package bundle + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/rancher/fleet/integrationtests/utils" + "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +var _ = Describe("Bundle label migration", func() { + BeforeEach(func() { + var err error + namespace, err = utils.NewNamespaceName() + Expect(err).ToNot(HaveOccurred()) + Expect(k8sClient.Create(ctx, &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: namespace}, + })).ToNot(HaveOccurred()) + + _, err = utils.CreateCluster(ctx, k8sClient, "cluster", namespace, nil, namespace) + Expect(err).NotTo(HaveOccurred()) + + DeferCleanup(func() { + Expect(k8sClient.Delete(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}})).ToNot(HaveOccurred()) + }) + }) + + createBundle := func(name string, labels map[string]string) { + bundle := &v1alpha1.Bundle{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: labels, + }, + Spec: v1alpha1.BundleSpec{ + BundleDeploymentOptions: v1alpha1.BundleDeploymentOptions{ + DefaultNamespace: "default", + }, + Targets: []v1alpha1.BundleTarget{ + { + BundleDeploymentOptions: v1alpha1.BundleDeploymentOptions{ + TargetNamespace: "targetNs", + }, + Name: "cluster", + ClusterName: "cluster", + }, + }, + }, + } + Expect(k8sClient.Create(ctx, bundle)).ToNot(HaveOccurred()) + } + + DescribeTable("should remove deprecated label after migration", + func(bundleName string, initialLabels map[string]string) { + const deprecatedLabel = "fleet.cattle.io/created-by-display-name" + + createBundle(bundleName, initialLabels) + DeferCleanup(func() { + Expect(k8sClient.Delete(ctx, &v1alpha1.Bundle{ + ObjectMeta: metav1.ObjectMeta{Name: bundleName, Namespace: namespace}, + })).ToNot(HaveOccurred()) + }) + + Eventually(func(g Gomega) { + bundle := &v1alpha1.Bundle{} + g.Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: namespace, Name: bundleName}, bundle)).To(Succeed()) + g.Expect(bundle.Status.ObservedGeneration).To(BeNumerically(">", 0)) + }).Should(Succeed()) + + bundle := &v1alpha1.Bundle{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: namespace, Name: bundleName}, bundle)).To(Succeed()) + + Expect(bundle.Labels).ToNot(HaveKey(deprecatedLabel)) + Expect(bundle.Labels).To(HaveKey(v1alpha1.CreatedByUserIDLabel)) + }, + Entry("with label present initially", + "bundle-with-label", + map[string]string{ + "fleet.cattle.io/created-by-display-name": "admin", + v1alpha1.CreatedByUserIDLabel: "user-12345", + }, + ), + Entry("without label present initially", + "bundle-without-label", + map[string]string{ + v1alpha1.CreatedByUserIDLabel: "user-12345", + }, + ), + ) +}) diff --git a/integrationtests/controller/bundle/suite_test.go b/integrationtests/controller/bundle/suite_test.go index 5d901cd24b..01d51206a7 100644 --- a/integrationtests/controller/bundle/suite_test.go +++ b/integrationtests/controller/bundle/suite_test.go @@ -1,6 +1,7 @@ package bundle import ( + "bytes" "context" "testing" "time" @@ -19,16 +20,19 @@ import ( "k8s.io/client-go/rest" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" + "sigs.k8s.io/controller-runtime/pkg/log/zap" ) var ( - cancel context.CancelFunc - cfg *rest.Config - ctx context.Context - k8sClient client.Client - testenv *envtest.Environment + cancel context.CancelFunc + cfg *rest.Config + ctx context.Context + k8sClient client.Client + testenv *envtest.Environment + logsBuffer bytes.Buffer namespace string ) @@ -49,6 +53,10 @@ var _ = BeforeSuite(func() { cfg, err = utils.StartTestEnv(testenv) Expect(err).NotTo(HaveOccurred()) + // Set up log capture + GinkgoWriter.TeeTo(&logsBuffer) + ctrl.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + k8sClient, err = utils.NewClient(cfg) Expect(err).NotTo(HaveOccurred()) diff --git a/integrationtests/controller/bundle/userid_logging_test.go b/integrationtests/controller/bundle/userid_logging_test.go new file mode 100644 index 0000000000..cafb745df4 --- /dev/null +++ b/integrationtests/controller/bundle/userid_logging_test.go @@ -0,0 +1,124 @@ +package bundle + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/rancher/fleet/integrationtests/utils" + "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var _ = Describe("Bundle UserID logging", func() { + var ( + bundle *v1alpha1.Bundle + cluster *v1alpha1.Cluster + namespace string + ) + + createBundle := func(name, clusterName string, labels map[string]string) { + bundle = &v1alpha1.Bundle{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: labels, + }, + Spec: v1alpha1.BundleSpec{ + BundleDeploymentOptions: v1alpha1.BundleDeploymentOptions{ + DefaultNamespace: "default", + }, + Targets: []v1alpha1.BundleTarget{ + { + Name: clusterName, + ClusterName: clusterName, + }, + }, + Resources: []v1alpha1.BundleResource{ + { + Name: "test-configmap.yaml", + Content: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test-cm\ndata:\n key: value\n", + }, + }, + }, + } + Expect(k8sClient.Create(ctx, bundle)).ToNot(HaveOccurred()) + } + + BeforeEach(func() { + var err error + namespace, err = utils.NewNamespaceName() + Expect(err).ToNot(HaveOccurred()) + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}} + Expect(k8sClient.Create(ctx, ns)).ToNot(HaveOccurred()) + + logsBuffer.Reset() + + DeferCleanup(func() { + Expect(k8sClient.Delete(ctx, bundle)).ToNot(HaveOccurred()) + Expect(k8sClient.Delete(ctx, cluster)).ToNot(HaveOccurred()) + Expect(k8sClient.Delete(ctx, ns)).ToNot(HaveOccurred()) + }) + }) + + waitForReconciliation := func() { + Eventually(func() int64 { + err := k8sClient.Get(ctx, client.ObjectKey{Namespace: bundle.Namespace, Name: bundle.Name}, bundle) + if err != nil { + return 0 + } + return bundle.Status.ObservedGeneration + }).Should(BeNumerically(">", 0)) + + Eventually(func() string { + return logsBuffer.String() + }).Should(ContainSubstring(bundle.Name)) + } + + When("Bundle has user ID label", func() { + const userID = "user-12345" + + BeforeEach(func() { + var err error + cluster, err = utils.CreateCluster(ctx, k8sClient, "test-cluster", namespace, nil, namespace) + Expect(err).NotTo(HaveOccurred()) + + createBundle("test-bundle-with-userid", "test-cluster", map[string]string{ + v1alpha1.CreatedByUserIDLabel: userID, + }) + }) + + It("includes userID in log output", func() { + waitForReconciliation() + + logs := logsBuffer.String() + Expect(logs).To(Or( + ContainSubstring(`"userID":"`+userID+`"`), + ContainSubstring(`"userID": "`+userID+`"`), + )) + + Expect(logs).To(ContainSubstring("bundle")) + Expect(logs).To(ContainSubstring(bundle.Name)) + }) + }) + + When("Bundle does not have user ID label", func() { + BeforeEach(func() { + var err error + cluster, err = utils.CreateCluster(ctx, k8sClient, "test-cluster-2", namespace, nil, namespace) + Expect(err).NotTo(HaveOccurred()) + + createBundle("test-bundle-without-userid", "test-cluster-2", nil) + }) + + It("does not include userID in log output", func() { + waitForReconciliation() + + logs := logsBuffer.String() + bundleLogs := utils.ExtractResourceLogs(logs, bundle.Name) + Expect(bundleLogs).NotTo(ContainSubstring(`"userID"`)) + }) + }) +}) diff --git a/integrationtests/gitjob/controller/label_migration_test.go b/integrationtests/gitjob/controller/label_migration_test.go new file mode 100644 index 0000000000..0f45921655 --- /dev/null +++ b/integrationtests/gitjob/controller/label_migration_test.go @@ -0,0 +1,82 @@ +package controller + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/rancher/fleet/integrationtests/utils" + "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +var _ = Describe("GitRepo label migration", func() { + var namespace string + + BeforeEach(func() { + var err error + namespace, err = utils.NewNamespaceName() + Expect(err).ToNot(HaveOccurred()) + Expect(k8sClient.Create(ctx, &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: namespace}, + })).ToNot(HaveOccurred()) + + DeferCleanup(func() { + Expect(k8sClient.Delete(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}})).ToNot(HaveOccurred()) + }) + }) + + createGitRepo := func(name string, labels map[string]string) { + gitrepo := &v1alpha1.GitRepo{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: labels, + }, + Spec: v1alpha1.GitRepoSpec{ + Repo: "https://github.com/rancher/fleet-test-data/not-found", + }, + } + Expect(k8sClient.Create(ctx, gitrepo)).ToNot(HaveOccurred()) + } + + DescribeTable("should remove deprecated label after migration", + func(gitRepoName string, initialLabels map[string]string) { + const deprecatedLabel = "fleet.cattle.io/created-by-display-name" + + createGitRepo(gitRepoName, initialLabels) + DeferCleanup(func() { + Expect(k8sClient.Delete(ctx, &v1alpha1.GitRepo{ + ObjectMeta: metav1.ObjectMeta{Name: gitRepoName, Namespace: namespace}, + })).ToNot(HaveOccurred()) + }) + + Eventually(func(g Gomega) { + gitrepo := &v1alpha1.GitRepo{} + g.Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: namespace, Name: gitRepoName}, gitrepo)).To(Succeed()) + g.Expect(gitrepo.Status.ObservedGeneration).To(BeNumerically(">", 0)) + }).Should(Succeed()) + + gitrepo := &v1alpha1.GitRepo{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: namespace, Name: gitRepoName}, gitrepo)).To(Succeed()) + + Expect(gitrepo.Labels).ToNot(HaveKey(deprecatedLabel)) + Expect(gitrepo.Labels).To(HaveKey(v1alpha1.CreatedByUserIDLabel)) + }, + Entry("with label present initially", + "gitrepo-with-label", + map[string]string{ + "fleet.cattle.io/created-by-display-name": "admin", + v1alpha1.CreatedByUserIDLabel: "user-12345", + }, + ), + Entry("without label present initially", + "gitrepo-without-label", + map[string]string{ + v1alpha1.CreatedByUserIDLabel: "user-12345", + }, + ), + ) +}) diff --git a/integrationtests/gitjob/controller/userid_logging_test.go b/integrationtests/gitjob/controller/userid_logging_test.go new file mode 100644 index 0000000000..6618158318 --- /dev/null +++ b/integrationtests/gitjob/controller/userid_logging_test.go @@ -0,0 +1,86 @@ +package controller + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/rancher/fleet/integrationtests/utils" + fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +var _ = Describe("GitRepo UserID logging", func() { + var ( + gitrepo *fleet.GitRepo + namespace string + ) + + createGitRepo := func(name string, labels map[string]string) { + gitrepo = &fleet.GitRepo{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: labels, + }, + Spec: fleet.GitRepoSpec{ + Repo: "https://github.com/rancher/fleet-test-data/single-path", + }, + } + Expect(k8sClient.Create(ctx, gitrepo)).ToNot(HaveOccurred()) + } + + BeforeEach(func() { + var err error + namespace, err = utils.NewNamespaceName() + Expect(err).ToNot(HaveOccurred()) + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}} + Expect(k8sClient.Create(ctx, ns)).ToNot(HaveOccurred()) + + logsBuffer.Reset() + + DeferCleanup(func() { + Expect(k8sClient.Delete(ctx, gitrepo)).ToNot(HaveOccurred()) + Expect(k8sClient.Delete(ctx, ns)).ToNot(HaveOccurred()) + }) + }) + + When("GitRepo has user ID label", func() { + const userID = "user-12345" + + BeforeEach(func() { + createGitRepo("test-gitrepo-with-userid", map[string]string{ + fleet.CreatedByUserIDLabel: userID, + }) + }) + + It("includes userID in log output", func() { + Eventually(func() string { + return logsBuffer.String() + }, timeout).Should(Or( + ContainSubstring(`"userID":"`+userID+`"`), + ContainSubstring(`"userID": "`+userID+`"`), + )) + + logs := logsBuffer.String() + Expect(logs).To(ContainSubstring("gitjob")) + Expect(logs).To(ContainSubstring(gitrepo.Name)) + }) + }) + + When("GitRepo does not have user ID label", func() { + BeforeEach(func() { + createGitRepo("test-gitrepo-without-userid", nil) + }) + + It("does not include userID in log output", func() { + Eventually(func() string { + return logsBuffer.String() + }, timeout).Should(ContainSubstring(gitrepo.Name)) + + logs := logsBuffer.String() + gitrepoLogs := utils.ExtractResourceLogs(logs, gitrepo.Name) + Expect(gitrepoLogs).NotTo(ContainSubstring(`"userID"`)) + }) + }) +}) diff --git a/integrationtests/utils/helpers.go b/integrationtests/utils/helpers.go index 9f791b6ef6..d6657cb091 100644 --- a/integrationtests/utils/helpers.go +++ b/integrationtests/utils/helpers.go @@ -2,6 +2,7 @@ package utils import ( "context" + "strings" "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" @@ -51,3 +52,14 @@ func CreateCluster(ctx context.Context, k8sClient client.Client, name, controlle err = k8sClient.Status().Update(ctx, cluster) return cluster, err } + +// ExtractResourceLogs extracts log lines related to a specific resource name +func ExtractResourceLogs(allLogs, resourceName string) string { + var resourceLogs []string + for _, line := range strings.Split(allLogs, "\n") { + if strings.Contains(line, resourceName) { + resourceLogs = append(resourceLogs, line) + } + } + return strings.Join(resourceLogs, "\n") +} diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go index 25e406b3b8..da856de34f 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go +++ b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go @@ -199,7 +199,18 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{RequeueAfter: durations.DefaultRequeueAfter}, nil } + // Migration: Remove the obsolete created-by-display-name label if it exists + if err := r.removeDisplayNameLabel(ctx, req.NamespacedName); err != nil { + logger.V(1).Error(err, "Failed to remove display name label") + return ctrl.Result{}, err + } + logger = logger.WithValues("generation", gitrepo.Generation, "commit", gitrepo.Status.Commit).WithValues("conditions", gitrepo.Status.Conditions) + + if userID := gitrepo.Labels[v1alpha1.CreatedByUserIDLabel]; userID != "" { + logger = logger.WithValues("userID", userID) + } + ctx = log.IntoContext(ctx, logger) logger.V(1).Info("Reconciling GitRepo") @@ -404,6 +415,28 @@ func (r *GitJobReconciler) addGitRepoFinalizer(ctx context.Context, nsName types return nil } +// removeDisplayNameLabel removes the obsolete created-by-display-name label from the gitrepo if it exists. +func (r *GitJobReconciler) removeDisplayNameLabel(ctx context.Context, nsName types.NamespacedName) error { + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + gitrepo := &v1alpha1.GitRepo{} + if err := r.Get(ctx, nsName, gitrepo); err != nil { + return err + } + + if gitrepo.Labels == nil { + return nil + } + + const deprecatedLabel = "fleet.cattle.io/created-by-display-name" + if _, exists := gitrepo.Labels[deprecatedLabel]; !exists { + return nil + } + + delete(gitrepo.Labels, deprecatedLabel) + return r.Update(ctx, gitrepo) + }) +} + func (r *GitJobReconciler) validateExternalSecretExist(ctx context.Context, gitrepo *v1alpha1.GitRepo) error { if gitrepo.Spec.HelmSecretNameForPaths != "" { if err := r.Get(ctx, types.NamespacedName{Namespace: gitrepo.Namespace, Name: gitrepo.Spec.HelmSecretNameForPaths}, &corev1.Secret{}); err != nil { diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_test.go b/internal/cmd/controller/gitops/reconciler/gitjob_test.go index 4dcadad8a4..c30766728c 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_test.go +++ b/internal/cmd/controller/gitops/reconciler/gitjob_test.go @@ -522,7 +522,7 @@ func TestReconcile_Error_WhenGetGitJobErrors(t *testing.T) { mockClient := mocks.NewMockClient(mockCtrl) mockClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(nil) - mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(1).DoAndReturn( + mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(2).DoAndReturn( func(ctx context.Context, req types.NamespacedName, gitrepo *fleetv1.GitRepo, opts ...interface{}) error { gitrepo.Name = gitRepo.Name gitrepo.Namespace = gitRepo.Namespace @@ -590,7 +590,7 @@ func TestReconcile_Error_WhenSecretDoesNotExist(t *testing.T) { mockClient := mocks.NewMockClient(mockCtrl) mockClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(nil) - mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), &gitRepoPointerMatcher{}, gomock.Any()).Times(2).DoAndReturn( + mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), &gitRepoPointerMatcher{}, gomock.Any()).Times(3).DoAndReturn( func(ctx context.Context, req types.NamespacedName, gitrepo *fleetv1.GitRepo, opts ...interface{}) error { gitrepo.Name = gitRepo.Name gitrepo.Namespace = gitRepo.Namespace diff --git a/internal/cmd/controller/reconciler/bundle_controller.go b/internal/cmd/controller/reconciler/bundle_controller.go index 068c933060..0dd3ce1f5f 100644 --- a/internal/cmd/controller/reconciler/bundle_controller.go +++ b/internal/cmd/controller/reconciler/bundle_controller.go @@ -200,6 +200,10 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr ) } + if userID := bundle.Labels[fleet.CreatedByUserIDLabel]; userID != "" { + logger = logger.WithValues("userID", userID) + } + if !bundle.DeletionTimestamp.IsZero() { return r.handleDelete(ctx, logger, req, bundle) } @@ -210,6 +214,11 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr bundleOrig := bundle.DeepCopy() + // Migration: Remove the obsolete created-by-display-name label if it exists + if err := r.removeDisplayNameLabel(ctx, bundle); err != nil { + return ctrl.Result{}, err + } + logger.V(1).Info( "Reconciling bundle, checking targets, calculating changes, building objects", "generation", @@ -460,6 +469,24 @@ func (r *BundleReconciler) ensureFinalizer(ctx context.Context, bundle *fleet.Bu return r.Update(ctx, bundle) } +// removeDisplayNameLabel removes the obsolete created-by-display-name label from the bundle if it exists. +func (r *BundleReconciler) removeDisplayNameLabel(ctx context.Context, bundle *fleet.Bundle) error { + if bundle.Labels == nil { + return nil + } + + const deprecatedLabel = "fleet.cattle.io/created-by-display-name" + if _, exists := bundle.Labels[deprecatedLabel]; !exists { + return nil + } + + delete(bundle.Labels, deprecatedLabel) + if err := r.Update(ctx, bundle); err != nil { + return err + } + return nil +} + func (r *BundleReconciler) createBundleDeployment( ctx context.Context, logger logr.Logger, diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/gitrepo_types.go b/pkg/apis/fleet.cattle.io/v1alpha1/gitrepo_types.go index 4b707847c8..6f6ee2910e 100644 --- a/pkg/apis/fleet.cattle.io/v1alpha1/gitrepo_types.go +++ b/pkg/apis/fleet.cattle.io/v1alpha1/gitrepo_types.go @@ -13,6 +13,7 @@ var ( RepoLabel = "fleet.cattle.io/repo-name" BundleLabel = "fleet.cattle.io/bundle-name" BundleNamespaceLabel = "fleet.cattle.io/bundle-namespace" + CreatedByUserIDLabel = "fleet.cattle.io/created-by-user-id" ) const (