From ee3135cd261e070e88bf86b66f8efd29740763dd Mon Sep 17 00:00:00 2001 From: Xavi Garcia Date: Fri, 31 May 2024 09:38:56 +0200 Subject: [PATCH] Moves funcionality from gitrepo to gitjob controller Moves the following from `gitrepo` controller to `gitjob` controller: * AuthorizeAndAssignDefaults * RBAC resources creation * Helm secrets check * NewTargetsConfigMap The `gitrepo` controller still purges `bundles` and `bundledeployments` on `gitrepo` deletion and handles the status coming from `bundles` and `bundledeployments` Some functionality that was shared between both controllers has been moved to a common package to avoid code repetition. Signed-off-by: Xavi Garcia --- charts/fleet/templates/rbac_gitjob.yaml | 20 +++ .../controller/gitrepo/gitrepo_test.go | 24 ---- .../gitjob/controller/controller_test.go | 133 +++++++++++++++++- .../gitjob/controller/suite_test.go | 21 ++- .../cmd/controller/errorutil/errorutil.go | 10 ++ .../gitops/reconciler/gitjob_controller.go | 71 ++++++++++ internal/cmd/controller/gitrepo/status.go | 52 +++++++ .../reconciler/cluster_controller.go | 3 +- .../reconciler/clustergroup_controller.go | 3 +- .../reconciler/gitrepo_controller.go | 127 +---------------- 10 files changed, 309 insertions(+), 155 deletions(-) create mode 100644 internal/cmd/controller/errorutil/errorutil.go diff --git a/charts/fleet/templates/rbac_gitjob.yaml b/charts/fleet/templates/rbac_gitjob.yaml index 39be2423b5..1e26cf856d 100644 --- a/charts/fleet/templates/rbac_gitjob.yaml +++ b/charts/fleet/templates/rbac_gitjob.yaml @@ -43,6 +43,26 @@ rules: - 'events' verbs: - '*' + - apiGroups: + - "" + resources: + - serviceaccounts + verbs: + - "create" + - apiGroups: + - rbac.authorization.k8s.io + resources: + - roles + verbs: + - escalate + - create + - bind + - apiGroups: + - rbac.authorization.k8s.io + resources: + - rolebindings + verbs: + - create --- apiVersion: rbac.authorization.k8s.io/v1 diff --git a/integrationtests/controller/gitrepo/gitrepo_test.go b/integrationtests/controller/gitrepo/gitrepo_test.go index 3e4d6c1da2..361be4a850 100644 --- a/integrationtests/controller/gitrepo/gitrepo_test.go +++ b/integrationtests/controller/gitrepo/gitrepo_test.go @@ -12,7 +12,6 @@ import ( "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" corev1 "k8s.io/api/core/v1" - rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ) @@ -58,29 +57,6 @@ var _ = Describe("GitRepo", func() { Expect(err).NotTo(HaveOccurred()) }) - It("creates RBAC resources", func() { - Expect(gitrepo.Spec.PollingInterval).To(BeNil()) - - Eventually(func() bool { - ns := types.NamespacedName{ - Name: fmt.Sprintf("git-%s", gitrepoName), - Namespace: namespace, - } - - if err := k8sClient.Get(ctx, ns, &corev1.ServiceAccount{}); err != nil { - return false - } - if err := k8sClient.Get(ctx, ns, &rbacv1.Role{}); err != nil { - return false - } - if err := k8sClient.Get(ctx, ns, &rbacv1.RoleBinding{}); err != nil { - return false - } - - return true - }).Should(BeTrue()) - }) - It("updates the gitrepo status", func() { org := gitrepo.ResourceVersion Eventually(func() bool { diff --git a/integrationtests/gitjob/controller/controller_test.go b/integrationtests/gitjob/controller/controller_test.go index 4a2b976044..ccb88ddf02 100644 --- a/integrationtests/gitjob/controller/controller_test.go +++ b/integrationtests/gitjob/controller/controller_test.go @@ -1,13 +1,19 @@ package controller import ( + "strings" + "time" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "sigs.k8s.io/controller-runtime/pkg/client" v1alpha1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" "github.com/rancher/wrangler/v2/pkg/name" batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -43,6 +49,24 @@ var _ = Describe("GitJob controller", func() { Expect(job.Spec.Template.Spec.Containers).To(HaveLen(1)) Expect(job.Spec.Template.Spec.Containers[0].Args).To(ContainElements("fleet", "apply")) + + // it should create RBAC resources for that gitRepo + Eventually(func() bool { + saName := name.SafeConcatName("git", gitRepo.Name) + ns := types.NamespacedName{Name: saName, Namespace: gitRepo.Namespace} + + if err := k8sClient.Get(ctx, ns, &corev1.ServiceAccount{}); err != nil { + return false + } + if err := k8sClient.Get(ctx, ns, &rbacv1.Role{}); err != nil { + return false + } + if err := k8sClient.Get(ctx, ns, &rbacv1.RoleBinding{}); err != nil { + return false + } + + return true + }).Should(BeTrue()) }) When("a job completes successfully", func() { @@ -54,7 +78,8 @@ var _ = Describe("GitJob controller", func() { // simulate job was successful Eventually(func() error { err := k8sClient.Get(ctx, types.NamespacedName{Name: jobName, Namespace: gitRepoNamespace}, &job) - Expect(err).ToNot(HaveOccurred()) + // We could be checking this when the job is still not created + Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred()) job.Status.Succeeded = 1 job.Status.Conditions = []batchv1.JobCondition{ { @@ -226,6 +251,112 @@ var _ = Describe("GitJob controller", func() { }).Should(BeTrue()) }) }) + + When("creating a gitRepo that references a nonexistent helm secret", func() { + var ( + gitRepo v1alpha1.GitRepo + gitRepoName string + helmSecretNameForPaths string + helmSecretName string + ) + + JustBeforeEach(func() { + gitRepoName = "test-no-for-paths-secret" + gitRepo = createGitRepo(gitRepoName) + gitRepo.Spec.HelmSecretNameForPaths = helmSecretNameForPaths + gitRepo.Spec.HelmSecretName = helmSecretName + // Create should return an error + err := k8sClient.Create(ctx, &gitRepo) + Expect(err).ToNot(HaveOccurred()) + Expect(simulateGitPollerUpdatingCommitInStatus(gitRepo, commit)).ToNot(HaveOccurred()) + }) + + AfterEach(func() { + err := k8sClient.Delete(ctx, &gitRepo) + Expect(err).ToNot(HaveOccurred()) + // reset the logs buffer so we don't read logs from previous tests + logsBuffer.Reset() + }) + + Context("helmSecretNameForPaths secret does not exist", func() { + BeforeEach(func() { + helmSecretNameForPaths = "secret-does-not-exist" + helmSecretName = "" + }) + It("logs an error about HelmSecretNameForPaths not being found", func() { + Eventually(func() bool { + strLogs := logsBuffer.String() + return strings.Contains(strLogs, `failed to look up HelmSecretNameForPaths, error: Secret \"secret-does-not-exist\" not found`) + }).Should(BeTrue()) + }) + + It("doesn't create RBAC resources", func() { + Consistently(func() bool { + saName := name.SafeConcatName("git", gitRepo.Name) + ns := types.NamespacedName{Name: saName, Namespace: gitRepo.Namespace} + + if err := k8sClient.Get(ctx, ns, &corev1.ServiceAccount{}); !errors.IsNotFound(err) { + return false + } + if err := k8sClient.Get(ctx, ns, &rbacv1.Role{}); !errors.IsNotFound(err) { + return false + } + if err := k8sClient.Get(ctx, ns, &rbacv1.RoleBinding{}); !errors.IsNotFound(err) { + return false + } + return true + }, time.Second*5, time.Second*1).Should(BeTrue()) + }) + + It("doesn't create the job", func() { + Consistently(func() bool { + jobName := name.SafeConcatName(gitRepoName, name.Hex(repo+commit, 5)) + newJob := &batchv1.Job{} + err := k8sClient.Get(ctx, types.NamespacedName{Name: jobName, Namespace: gitRepo.Namespace}, newJob) + return errors.IsNotFound(err) + }, time.Second*5, time.Second*1).Should(BeTrue()) + }) + }) + Context("helmSecretName secret does not exist", func() { + BeforeEach(func() { + helmSecretNameForPaths = "" + helmSecretName = "secret-does-not-exist" + }) + It("logs an error about HelmSecretName not being found", func() { + Eventually(func() bool { + strLogs := logsBuffer.String() + return strings.Contains(strLogs, `failed to look up helmSecretName, error: Secret \"secret-does-not-exist\" not found`) + }).Should(BeTrue()) + }) + + It("doesn't create RBAC resources", func() { + Consistently(func() bool { + saName := name.SafeConcatName("git", gitRepo.Name) + ns := types.NamespacedName{Name: saName, Namespace: gitRepo.Namespace} + + if err := k8sClient.Get(ctx, ns, &corev1.ServiceAccount{}); !errors.IsNotFound(err) { + return false + } + if err := k8sClient.Get(ctx, ns, &rbacv1.Role{}); !errors.IsNotFound(err) { + return false + } + if err := k8sClient.Get(ctx, ns, &rbacv1.RoleBinding{}); !errors.IsNotFound(err) { + return false + } + return true + }, time.Second*5, time.Second*1).Should(BeTrue()) + }) + + It("doesn't create the job", func() { + Consistently(func() bool { + jobName := name.SafeConcatName(gitRepoName, name.Hex(repo+commit, 5)) + newJob := &batchv1.Job{} + err := k8sClient.Get(ctx, types.NamespacedName{Name: jobName, Namespace: gitRepo.Namespace}, newJob) + return errors.IsNotFound(err) + }, time.Second*5, time.Second*1).Should(BeTrue()) + }) + }) + }) }) func simulateIncreaseForceSyncGeneration(gitRepo v1alpha1.GitRepo) error { diff --git a/integrationtests/gitjob/controller/suite_test.go b/integrationtests/gitjob/controller/suite_test.go index a335e236b6..f1b655dcb2 100644 --- a/integrationtests/gitjob/controller/suite_test.go +++ b/integrationtests/gitjob/controller/suite_test.go @@ -1,6 +1,7 @@ package controller import ( + "bytes" "context" "path/filepath" "testing" @@ -17,9 +18,10 @@ import ( "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" - ctrl "sigs.k8s.io/controller-runtime" + ctrlruntime "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" ) const ( @@ -27,11 +29,12 @@ const ( ) var ( - cfg *rest.Config - testEnv *envtest.Environment - ctx context.Context - cancel context.CancelFunc - k8sClient client.Client + cfg *rest.Config + testEnv *envtest.Environment + ctx context.Context + cancel context.CancelFunc + k8sClient client.Client + logsBuffer bytes.Buffer ) func TestGitJobController(t *testing.T) { @@ -59,7 +62,7 @@ var _ = BeforeSuite(func() { Expect(err).NotTo(HaveOccurred()) Expect(k8sClient).NotTo(BeNil()) - mgr, err := ctrl.NewManager(cfg, ctrl.Options{ + mgr, err := ctrlruntime.NewManager(cfg, ctrlruntime.Options{ Scheme: scheme.Scheme, }) Expect(err).ToNot(HaveOccurred()) @@ -67,6 +70,10 @@ var _ = BeforeSuite(func() { ctlr := gomock.NewController(GinkgoT()) gitPollerMock := mocks.NewMockGitPoller(ctlr) + // redirect logs to a buffer that we can read in the tests + GinkgoWriter.TeeTo(&logsBuffer) + ctrlruntime.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + // do nothing if gitPoller is called. gitPoller calls are tested in unit tests gitPollerMock.EXPECT().AddOrModifyGitRepoPollJob(gomock.Any(), gomock.Any()).AnyTimes() gitPollerMock.EXPECT().CleanUpGitRepoPollJobs(gomock.Any()).AnyTimes() diff --git a/internal/cmd/controller/errorutil/errorutil.go b/internal/cmd/controller/errorutil/errorutil.go new file mode 100644 index 0000000000..862e2f87f9 --- /dev/null +++ b/internal/cmd/controller/errorutil/errorutil.go @@ -0,0 +1,10 @@ +package errorutil + +import apierrors "k8s.io/apimachinery/pkg/api/errors" + +func IgnoreConflict(err error) error { + if apierrors.IsConflict(err) { + return nil + } + return err +} diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go index d34da01ea7..bfc89ef314 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go +++ b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go @@ -104,6 +104,17 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr } if errors.IsNotFound(err) && gitRepo.Status.Commit != "" { + if err := r.validateExternalSecretExist(ctx, &gitRepo); err != nil { + nsname := types.NamespacedName{Namespace: gitRepo.Namespace, Name: gitRepo.Name} + return ctrl.Result{}, grutil.UpdateErrorStatus(ctx, r.Client, nsname, gitRepo.Status, err) + } + logger.V(1).Info("Creating Git job resources") + if err := r.createJobRBAC(ctx, &gitRepo); err != nil { + return ctrl.Result{}, err + } + if err := r.createTargetsConfigMap(ctx, &gitRepo); err != nil { + return ctrl.Result{}, err + } if err := r.createJob(ctx, &gitRepo); err != nil { return ctrl.Result{}, fmt.Errorf("error creating git job: %v", err) } @@ -141,6 +152,66 @@ func generationOrCommitChangedPredicate() predicate.Predicate { } } +func (r *GitJobReconciler) createJobRBAC(ctx context.Context, gitrepo *v1alpha1.GitRepo) error { + // No update needed, values are the same. So we ignore AlreadyExists. + saName := name.SafeConcatName("git", gitrepo.Name) + sa := grutil.NewServiceAccount(gitrepo.Namespace, saName) + if err := controllerutil.SetControllerReference(gitrepo, sa, r.Scheme); err != nil { + return err + } + if err := r.Create(ctx, sa); err != nil && !errors.IsAlreadyExists(err) { + return err + } + + role := grutil.NewRole(gitrepo.Namespace, saName) + if err := controllerutil.SetControllerReference(gitrepo, role, r.Scheme); err != nil { + return err + } + if err := r.Create(ctx, role); err != nil && !errors.IsAlreadyExists(err) { + return err + } + + rb := grutil.NewRoleBinding(gitrepo.Namespace, saName) + if err := controllerutil.SetControllerReference(gitrepo, rb, r.Scheme); err != nil { + return err + } + if err := r.Create(ctx, rb); err != nil && !errors.IsAlreadyExists(err) { + return err + } + + return nil +} + +func (r *GitJobReconciler) createTargetsConfigMap(ctx context.Context, gitrepo *v1alpha1.GitRepo) error { + configMap, err := grutil.NewTargetsConfigMap(gitrepo) + if err != nil { + return err + } + if err := controllerutil.SetControllerReference(gitrepo, configMap, r.Scheme); err != nil { + return err + } + data := configMap.BinaryData + _, err = controllerutil.CreateOrUpdate(ctx, r.Client, configMap, func() error { + configMap.BinaryData = data + return nil + }) + + return err +} + +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 { + return fmt.Errorf("failed to look up HelmSecretNameForPaths, error: %v", err) + } + } else if gitrepo.Spec.HelmSecretName != "" { + if err := r.Get(ctx, types.NamespacedName{Namespace: gitrepo.Namespace, Name: gitrepo.Spec.HelmSecretName}, &corev1.Secret{}); err != nil { + return fmt.Errorf("failed to look up helmSecretName, error: %v", err) + } + } + return nil +} + func (r *GitJobReconciler) createJob(ctx context.Context, gitRepo *v1alpha1.GitRepo) error { job, err := r.newJob(ctx, gitRepo) if err != nil { diff --git a/internal/cmd/controller/gitrepo/status.go b/internal/cmd/controller/gitrepo/status.go index 0ebe66586e..2732e61b8f 100644 --- a/internal/cmd/controller/gitrepo/status.go +++ b/internal/cmd/controller/gitrepo/status.go @@ -2,11 +2,20 @@ package gitrepo import ( "context" + "fmt" "sort" + "time" "github.com/rancher/fleet/internal/cmd/controller/summary" + "github.com/rancher/fleet/internal/metrics" fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" + "github.com/rancher/wrangler/v2/pkg/condition" + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" + fleetutil "github.com/rancher/fleet/internal/cmd/controller/errorutil" + errutil "k8s.io/apimachinery/pkg/util/errors" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -97,3 +106,46 @@ func UpdateDisplayState(gitrepo *fleet.GitRepo) error { return nil } + +// SetCondition sets the condition and updates the timestamp, if the condition changed +func SetCondition(status *fleet.GitRepoStatus, err error) { + cond := condition.Cond(fleet.GitRepoAcceptedCondition) + origStatus := status.DeepCopy() + cond.SetError(status, "", fleetutil.IgnoreConflict(err)) + if !equality.Semantic.DeepEqual(origStatus, status) { + cond.LastUpdated(status, time.Now().UTC().Format(time.RFC3339)) + } +} + +// UpdateErrorStatus sets the condition in the status and tries to update the resource +func UpdateErrorStatus(ctx context.Context, c client.Client, req types.NamespacedName, status fleet.GitRepoStatus, orgErr error) error { + SetCondition(&status, orgErr) + if statusErr := UpdateStatus(ctx, c, req, status); statusErr != nil { + merr := []error{orgErr, fmt.Errorf("failed to update the status: %w", statusErr)} + return errutil.NewAggregate(merr) + } + return orgErr +} + +// UpdateStatus updates the status for the GitRepo resource. It retries on +// conflict. If the status was updated successfully, it also collects (as in +// updates) metrics for the resource GitRepo resource. +func UpdateStatus(ctx context.Context, c client.Client, req types.NamespacedName, status fleet.GitRepoStatus) error { + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + t := &fleet.GitRepo{} + err := c.Get(ctx, req, t) + if err != nil { + return err + } + t.Status = status + + err = c.Status().Update(ctx, t) + if err != nil { + return err + } + + metrics.GitRepoCollector.Collect(ctx, t) + + return nil + }) +} diff --git a/internal/cmd/controller/reconciler/cluster_controller.go b/internal/cmd/controller/reconciler/cluster_controller.go index 34a849a425..dbb0ce1cd0 100644 --- a/internal/cmd/controller/reconciler/cluster_controller.go +++ b/internal/cmd/controller/reconciler/cluster_controller.go @@ -15,6 +15,7 @@ import ( "github.com/rancher/fleet/pkg/durations" "github.com/rancher/fleet/pkg/sharding" + fleetutil "github.com/rancher/fleet/internal/cmd/controller/errorutil" "github.com/rancher/wrangler/v2/pkg/condition" corev1 "k8s.io/api/core/v1" @@ -202,7 +203,7 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct func (r *ClusterReconciler) setCondition(status *fleet.ClusterStatus, err error) { cond := condition.Cond(fleet.ClusterConditionProcessed) origStatus := status.DeepCopy() - cond.SetError(status, "", ignoreConflict(err)) + cond.SetError(status, "", fleetutil.IgnoreConflict(err)) if !equality.Semantic.DeepEqual(origStatus, status) { cond.LastUpdated(status, time.Now().UTC().Format(time.RFC3339)) } diff --git a/internal/cmd/controller/reconciler/clustergroup_controller.go b/internal/cmd/controller/reconciler/clustergroup_controller.go index fc9e0ea430..93f81d691b 100644 --- a/internal/cmd/controller/reconciler/clustergroup_controller.go +++ b/internal/cmd/controller/reconciler/clustergroup_controller.go @@ -10,6 +10,7 @@ import ( "strings" "time" + fleetutil "github.com/rancher/fleet/internal/cmd/controller/errorutil" "github.com/rancher/fleet/internal/cmd/controller/summary" "github.com/rancher/fleet/internal/metrics" fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" @@ -129,7 +130,7 @@ func (r *ClusterGroupReconciler) Reconcile(ctx context.Context, req ctrl.Request func (r *ClusterGroupReconciler) setCondition(status *fleet.ClusterGroupStatus, err error) { cond := condition.Cond(fleet.ClusterGroupConditionProcessed) origStatus := status.DeepCopy() - cond.SetError(status, "", ignoreConflict(err)) + cond.SetError(status, "", fleetutil.IgnoreConflict(err)) if !equality.Semantic.DeepEqual(origStatus, status) { cond.LastUpdated(status, time.Now().UTC().Format(time.RFC3339)) } diff --git a/internal/cmd/controller/reconciler/gitrepo_controller.go b/internal/cmd/controller/reconciler/gitrepo_controller.go index f7136fe868..7276a079ee 100644 --- a/internal/cmd/controller/reconciler/gitrepo_controller.go +++ b/internal/cmd/controller/reconciler/gitrepo_controller.go @@ -8,7 +8,6 @@ import ( "reflect" "slices" "strings" - "time" grutil "github.com/rancher/fleet/internal/cmd/controller/gitrepo" "github.com/rancher/fleet/internal/cmd/controller/imagescan" @@ -17,17 +16,12 @@ import ( "github.com/rancher/fleet/pkg/sharding" "github.com/reugn/go-quartz/quartz" - "github.com/rancher/wrangler/v2/pkg/condition" "github.com/rancher/wrangler/v2/pkg/genericcondition" - "github.com/rancher/wrangler/v2/pkg/name" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/equality" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - errutil "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" @@ -133,24 +127,24 @@ func (r *GitRepoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct oldStatus := gitrepo.Status.DeepCopy() gitrepo, err := grutil.AuthorizeAndAssignDefaults(ctx, r.Client, gitrepo) if err != nil { - return ctrl.Result{}, r.updateErrorStatus(ctx, req.NamespacedName, *oldStatus, err) + return ctrl.Result{}, grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, *oldStatus, err) } // Refresh the status err = grutil.SetStatusFromBundleDeployments(ctx, r.Client, gitrepo) if err != nil { - return ctrl.Result{}, r.updateErrorStatus(ctx, req.NamespacedName, gitrepo.Status, err) + return ctrl.Result{}, grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err) } err = grutil.SetStatusFromBundles(ctx, r.Client, gitrepo) if err != nil { - return ctrl.Result{}, r.updateErrorStatus(ctx, req.NamespacedName, gitrepo.Status, err) + return ctrl.Result{}, grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err) } // Ideally, this should be done in the git job reconciler, but setting the status from bundle deployments // updates the display state too. if err = grutil.UpdateDisplayState(gitrepo); err != nil { - return ctrl.Result{}, r.updateErrorStatus(ctx, req.NamespacedName, gitrepo.Status, err) + return ctrl.Result{}, grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err) } grutil.SetStatusFromResourceKey(ctx, r.Client, gitrepo) @@ -159,120 +153,18 @@ func (r *GitRepoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct gitrepo.Status.Summary.Ready, gitrepo.Status.Summary.DesiredReady) - r.setCondition(&gitrepo.Status, nil) + grutil.SetCondition(&gitrepo.Status, nil) - err = r.updateStatus(ctx, req.NamespacedName, gitrepo.Status) + err = grutil.UpdateStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status) if err != nil { logger.V(1).Error(err, "Reconcile failed final update to git repo status", "status", gitrepo.Status) return ctrl.Result{}, err } - // Validate external secrets exist - if gitrepo.Spec.HelmSecretNameForPaths != "" { - if err := r.Get(ctx, types.NamespacedName{Namespace: gitrepo.Namespace, Name: gitrepo.Spec.HelmSecretNameForPaths}, &corev1.Secret{}); err != nil { - err = fmt.Errorf("failed to look up HelmSecretNameForPaths, error: %v", err) - return ctrl.Result{}, r.updateErrorStatus(ctx, req.NamespacedName, gitrepo.Status, err) - - } - } else if gitrepo.Spec.HelmSecretName != "" { - if err := r.Get(ctx, types.NamespacedName{Namespace: gitrepo.Namespace, Name: gitrepo.Spec.HelmSecretName}, &corev1.Secret{}); err != nil { - err = fmt.Errorf("failed to look up helmSecretName, error: %v", err) - return ctrl.Result{}, r.updateErrorStatus(ctx, req.NamespacedName, gitrepo.Status, err) - } - } - - // Start creating/updating the job - logger.V(1).Info("Creating Git job resources") - - configMap, err := grutil.NewTargetsConfigMap(gitrepo) - if err != nil { - return ctrl.Result{}, err - } - if err := controllerutil.SetControllerReference(gitrepo, configMap, r.Scheme); err != nil { - return ctrl.Result{}, err - } - data := configMap.BinaryData - _, err = controllerutil.CreateOrUpdate(ctx, r.Client, configMap, func() error { - configMap.BinaryData = data - return nil - }) - if err != nil { - return ctrl.Result{}, err - } - - // No update needed, values are the same. So we ignore AlreadyExists. - saName := name.SafeConcatName("git", gitrepo.Name) - sa := grutil.NewServiceAccount(gitrepo.Namespace, saName) - if err := controllerutil.SetControllerReference(gitrepo, sa, r.Scheme); err != nil { - return ctrl.Result{}, err - } - if err := r.Create(ctx, sa); err != nil && !apierrors.IsAlreadyExists(err) { - return ctrl.Result{}, err - } - - role := grutil.NewRole(gitrepo.Namespace, saName) - if err := controllerutil.SetControllerReference(gitrepo, role, r.Scheme); err != nil { - return ctrl.Result{}, err - } - if err := r.Create(ctx, role); err != nil && !apierrors.IsAlreadyExists(err) { - return ctrl.Result{}, err - } - - rb := grutil.NewRoleBinding(gitrepo.Namespace, saName) - if err := controllerutil.SetControllerReference(gitrepo, rb, r.Scheme); err != nil { - return ctrl.Result{}, err - } - if err := r.Create(ctx, rb); err != nil && !apierrors.IsAlreadyExists(err) { - return ctrl.Result{}, err - } - return ctrl.Result{}, nil } -// setCondition sets the condition and updates the timestamp, if the condition changed -func (r *GitRepoReconciler) setCondition(status *fleet.GitRepoStatus, err error) { - cond := condition.Cond(fleet.GitRepoAcceptedCondition) - origStatus := status.DeepCopy() - cond.SetError(status, "", ignoreConflict(err)) - if !equality.Semantic.DeepEqual(origStatus, status) { - cond.LastUpdated(status, time.Now().UTC().Format(time.RFC3339)) - } -} - -// updateErrorStatus sets the condition in the status and tries to update the resource -func (r *GitRepoReconciler) updateErrorStatus(ctx context.Context, req types.NamespacedName, status fleet.GitRepoStatus, orgErr error) error { - r.setCondition(&status, orgErr) - if statusErr := r.updateStatus(ctx, req, status); statusErr != nil { - merr := []error{orgErr, fmt.Errorf("failed to update the status: %w", statusErr)} - return errutil.NewAggregate(merr) - } - return orgErr -} - -// updateStatus updates the status for the GitRepo resource. It retries on -// conflict. If the status was updated successfully, it also collects (as in -// updates) metrics for the resource GitRepo resource. -func (r *GitRepoReconciler) updateStatus(ctx context.Context, req types.NamespacedName, status fleet.GitRepoStatus) error { - return retry.RetryOnConflict(retry.DefaultRetry, func() error { - t := &fleet.GitRepo{} - err := r.Get(ctx, req, t) - if err != nil { - return err - } - t.Status = status - - err = r.Status().Update(ctx, t) - if err != nil { - return err - } - - metrics.GitRepoCollector.Collect(ctx, t) - - return nil - }) -} - // SetupWithManager sets up the controller with the Manager. func (r *GitRepoReconciler) SetupWithManager(mgr ctrl.Manager) error { // Note: Maybe use mgr.GetFieldIndexer().IndexField for better performance? @@ -472,10 +364,3 @@ func acceptedLastUpdate(conds []genericcondition.GenericCondition) string { return "" } - -func ignoreConflict(err error) error { - if apierrors.IsConflict(err) { - return nil - } - return err -}