Skip to content

Commit

Permalink
Moves funcionality from gitrepo to gitjob controller
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
0xavi0 committed May 31, 2024
1 parent 9b82db3 commit 84d478d
Show file tree
Hide file tree
Showing 10 changed files with 303 additions and 155 deletions.
13 changes: 13 additions & 0 deletions charts/fleet/templates/rbac_gitjob.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,19 @@ rules:
- 'events'
verbs:
- '*'
- apiGroups:
- ""
resources:
- serviceaccounts
verbs:
- "create"
- apiGroups:
- rbac.authorization.k8s.io
resources:
- roles
- rolebindings
verbs:
- "*"

---
apiVersion: rbac.authorization.k8s.io/v1
Expand Down
24 changes: 0 additions & 24 deletions integrationtests/controller/gitrepo/gitrepo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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 {
Expand Down
133 changes: 132 additions & 1 deletion integrationtests/gitjob/controller/controller_test.go
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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() {
Expand All @@ -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{
{
Expand Down Expand Up @@ -226,6 +251,112 @@ var _ = Describe("GitJob controller", func() {
}).Should(BeTrue())
})
})

When("creating a gitRepo that references a non existing helm secrets", 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())
})

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 HelmSecretNameForPaths 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 {
Expand Down
21 changes: 14 additions & 7 deletions integrationtests/gitjob/controller/suite_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package controller

import (
"bytes"
"context"
"path/filepath"
"testing"
Expand All @@ -17,21 +18,23 @@ 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 (
timeout = 30 * time.Second
)

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) {
Expand Down Expand Up @@ -59,14 +62,18 @@ 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())

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()
Expand Down
10 changes: 10 additions & 0 deletions internal/cmd/controller/errorutil/errorutil.go
Original file line number Diff line number Diff line change
@@ -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
}
72 changes: 72 additions & 0 deletions internal/cmd/controller/gitops/reconciler/gitjob_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -141,6 +152,67 @@ 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 {
// 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 {
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 {
Expand Down
Loading

0 comments on commit 84d478d

Please sign in to comment.