fix: prevent status update loop

.github/PULL_REQUEST_TEMPLATE.md

@@ -9,7 +9,7 @@ Did you read the contributing and development guides?
  * http://moolen.github.io/harbor-sync/docs/development
 
 Do the tests pass locally?
- * run: <make docker-test> to be sure that all tests pass
+ * run: <make test> to be sure that all tests pass
 
 If this is a PR is a new feature please provide details and documentation about how to use it.
 

.github/workflows/e2e.yml

@@ -28,7 +28,13 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        harbor_version: [v2.2.0, v2.0.6]
+        include:
+        - harbor_version: v2.3.1
+          harbor_chart_version: v1.7.1
+        - harbor_version: v2.2.0
+          harbor_chart_version: v1.6.0
+        - harbor_version: v2.0.6
+          harbor_chart_version: v1.4.6
     steps:
       - name: Checkout secret-manger
         uses: actions/checkout@v2
@@ -37,13 +43,14 @@ jobs:
       - name: Create kind cluster
         uses: helm/kind-action@v1.0.0
         with:
-          version: v0.10.0
+          version: v0.11.1
           node_image: kindest/node:v1.20.2
           cluster_name: harbor-sync
       - name: Run e2e tests
         timeout-minutes: 30
         env:
           HARBOR_VERSION: ${{ matrix.harbor_version }}
+          HARBOR_CHART_VERSION: ${{ matrix.harbor_chart_version }}
           KIND_CLUSTER_NAME: harbor-sync
         run: |
           export PATH=$PATH:$(go env GOPATH)/bin

Makefile

@@ -74,12 +74,6 @@ vet:
 generate: bin/controller-gen
 	$(CONTROLLER_GEN) object:headerFile=./hack/boilerplate.go.txt paths="./..."
 
-# Run tests in container
-docker-test:
-	rm -rf bin
-	docker build -t test:latest -f Dockerfile.test .
-	docker run test:latest
-
 # Build the docker image
 docker-build:
 	docker build . -t ${IMG}

api/v1/types.go

@@ -102,6 +102,9 @@ type WebhookUpdatePayload struct {
 
 // HarborSyncStatus defines the observed state of HarborSync
 type HarborSyncStatus struct {
+	// +optional
+	LastReconciliation metav1.Time `json:"lastReconciliation,omitempty"`
+
 	// +optional
 	ProjectList []ProjectStatus `json:"projectStatus,omitempty"`
 

cmd/controller.go

@@ -56,6 +56,7 @@ func init() {
 	flags.Duration("harbor-poll-interval", time.Minute*5, "poll interval to update harbor projects & robot accounts")
 	flags.Duration("force-sync-interval", time.Minute*10, "set this to force reconciliation after a certain time")
 	flags.Duration("rotation-interval", time.Minute*60, "set this to rotate the credentials after the specified time")
+	flags.Duration("requeue-interval", time.Minute*5, "set this to prevent reconciling for a specified time")
 	flags.Bool("leader-elect", true, "enable leader election")
 	flags.String("namespace", "kube-system", "namespace in which harbor-sync runs (used for leader-election)")
 	viper.BindPFlags(flags)
@@ -70,6 +71,7 @@ func init() {
 	viper.BindEnv("harbor-poll-interval", "HARBOR_POLL_INTERVAL")
 	viper.BindEnv("force-sync-interval", "FORCE_SYNC_INTERVAL")
 	viper.BindEnv("rotation-interval", "ROTATION_INTERVAL")
+	viper.BindEnv("requeue-interval", "REQUEUE_INTERVAL")
 	rootCmd.AddCommand(controllerCmd)
 }
 
@@ -86,6 +88,7 @@ var controllerCmd = &cobra.Command{
 			"namespace":             viper.GetDuration("namespace"),
 			"force-sync-interval":   viper.GetDuration("force-sync-interval"),
 			"rotation-interval":     viper.GetDuration("rotation-interval"),
+			"requeue-interval":      viper.GetDuration("requeue-interval"),
 			"harbor-poll-interval":  viper.GetDuration("harbor-poll-interval"),
 			"skip-tls-verification": viper.GetDuration("skip-tls-verification"),
 			"harbor-api-debug":      viper.GetDuration("harbor-api-debug"),
@@ -156,6 +159,7 @@ var controllerCmd = &cobra.Command{
 			CredCache:        crdStore,
 			RotationInterval: viper.GetDuration("rotation-interval"),
 			Client:           mgr.GetClient(),
+			RequeueInterval:  viper.GetDuration("requeue-interval"),
 			Harbor:           harborRepo,
 		}).SetupWithManager(mgr, syncCfgChanges); err != nil {
 			log.Error(err, "unable to create controller")

config/crd/bases/crd.harborsync.io_harborsyncs.yaml

@@ -127,6 +127,9 @@ spec:
                 - type
                 type: object
               type: array
+            lastReconciliation:
+              format: date-time
+              type: string
             projectStatus:
               items:
                 properties:

pkg/controllers/controller.go

@@ -29,6 +29,7 @@ import (
 	log "github.com/sirupsen/logrus"
 	v1 "k8s.io/api/core/v1"
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/event"
@@ -44,6 +45,7 @@ import (
 type HarborSyncConfigReconciler struct {
 	client.Client
 	RotationInterval time.Duration
+	RequeueInterval  time.Duration
 	CredCache        reconciler.CredentialStore
 	Harbor           harbor.API
 }
@@ -66,8 +68,22 @@ func (r *HarborSyncConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req
 			return ctrl.Result{}, nil
 		}
 		log.Error(err, "unable to fetch sync config")
-		return ctrl.Result{Requeue: true, RequeueAfter: time.Second * 15}, err
+		return ctrl.Result{RequeueAfter: time.Second * 15}, err
 	}
+
+	defer func() {
+		err := r.Status().Update(context.Background(), &syncConfig)
+		if err != nil {
+			log.Errorf("unable to update status: %s", err.Error())
+		}
+	}()
+
+	// return early if cr has been updated recently
+	if !shouldReconcile(syncConfig) {
+		log.Infof("skipping reconciliation")
+		return ctrl.Result{RequeueAfter: r.RequeueInterval}, nil
+	}
+
 	// mappingFunc calls the Kubernetes-specific mapping functions
 	mappingFunc := func(
 		mapping crdv1.ProjectMapping,
@@ -83,12 +99,8 @@ func (r *HarborSyncConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req
 		err = f(r, mapping, syncConfig, project, *credential, baseURL)
 		if err != nil {
 			c := NewSyncCondition(crdv1.HarborSyncReady, v1.ConditionFalse, "Mapping failed", err.Error())
-			SetSyncCondition(&syncConfig.Status, *c)
-			err = r.Status().Update(context.Background(), &syncConfig)
-			if err != nil {
-				log.Errorf("unable to update status mapping func: %s", err.Error())
-			}
 			log.Error(err, "mapping failed")
+			SetSyncCondition(&syncConfig.Status, *c)
 			return
 		}
 	}
@@ -97,18 +109,11 @@ func (r *HarborSyncConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req
 		log.Error(err)
 		c := NewSyncCondition(crdv1.HarborSyncReady, v1.ConditionFalse, "Error Reconciling", err.Error())
 		SetSyncCondition(&syncConfig.Status, *c)
-		err = r.Status().Update(context.Background(), &syncConfig)
-		if err != nil {
-			log.Errorf("unable to update status after reconcile error: %s", err.Error())
-		}
-		return ctrl.Result{Requeue: true, RequeueAfter: time.Second * 30}, nil
+		return ctrl.Result{RequeueAfter: time.Second * 30}, nil
 	}
 	c := NewSyncCondition(crdv1.HarborSyncReady, v1.ConditionTrue, "Successfully reconciled", "Successfully reconciled")
+	syncConfig.Status.LastReconciliation = metav1.Now()
 	SetSyncCondition(&syncConfig.Status, *c)
-	err = r.Status().Update(context.Background(), &syncConfig)
-	if err != nil {
-		log.Errorf("unable to update status after reconcile: %s", err.Error())
-	}
 	log.Info("successfully reconciled")
 	return ctrl.Result{}, nil
 }
@@ -153,6 +158,9 @@ func Reconcile(
 		selector.ProjectName,
 	).Set(float64(len(matches)))
 
+	// reset projectList
+	cfg.Status.ProjectList = []crdv1.ProjectStatus{}
+
 	// reconcile robot accounts
 	for _, project := range matches {
 		credential, changed, err := reconciler.ReconcileRobotAccounts(
@@ -259,3 +267,11 @@ func runWebhook(
 	}
 	return fmt.Errorf("webhook errors: %#v", errs)
 }
+
+func shouldReconcile(syncConfig crdv1.HarborSync) bool {
+	cmp := syncConfig.Status.LastReconciliation.Time.Add(time.Minute)
+	if cmp.After(time.Now()) {
+		return false
+	}
+	return true
+}

pkg/controllers/controller_test.go

@@ -33,6 +33,7 @@ import (
 	. "github.com/onsi/gomega"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 )
@@ -51,6 +52,7 @@ var _ = Describe("Controller", func() {
 		hscr = &HarborSyncConfigReconciler{
 			k8sClient,
 			time.Hour * 24,
+			time.Second * 15,
 			credStore,
 			fakeHarbor,
 		}
@@ -350,3 +352,27 @@ var _ = Describe("Controller", func() {
 		})
 	})
 })
+
+var _ = Describe("Reconciler", func() {
+	It("should not reconcile when recently changed", func() {
+		Expect(shouldReconcile(crdv1.HarborSync{
+			Status: crdv1.HarborSyncStatus{
+				LastReconciliation: metav1.Now(),
+			},
+		})).To(BeFalse())
+	})
+
+	It("should reconcile when not recently changed", func() {
+		Expect(shouldReconcile(crdv1.HarborSync{
+			Status: crdv1.HarborSyncStatus{
+				LastReconciliation: metav1.NewTime(time.Now().Add(-time.Hour)),
+			},
+		})).To(BeTrue())
+	})
+
+	It("should reconcile when status is empty", func() {
+		Expect(shouldReconcile(crdv1.HarborSync{
+			Status: crdv1.HarborSyncStatus{},
+		})).To(BeTrue())
+	})
+})

pkg/controllers/suite_test.go

@@ -28,7 +28,6 @@ import (
 	"k8s.io/client-go/rest"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
-	"sigs.k8s.io/controller-runtime/pkg/envtest/printer"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 	// +kubebuilder:scaffold:imports
@@ -41,9 +40,7 @@ var testEnv *envtest.Environment
 func TestAPIs(t *testing.T) {
 	RegisterFailHandler(Fail)
 
-	RunSpecsWithDefaultAndCustomReporters(t,
-		"Controller Suite",
-		[]Reporter{printer.NewlineReporter{}})
+	RunSpecs(t, "Controller Suite")
 }
 
 var _ = BeforeSuite(func(done Done) {

pkg/reconciler/mapping_test.go

@@ -22,7 +22,7 @@ import (
 	crdv1 "github.com/moolen/harbor-sync/api/v1"
 	"github.com/moolen/harbor-sync/pkg/harbor"
 	"github.com/moolen/harbor-sync/pkg/test"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -48,7 +48,7 @@ var _ = Describe("Mapping", func() {
 			test.DeleteHarborSyncConfig(k8sClient, "my-match-cfg")
 		})
 
-		It("should create secrets in namespace", func(done Done) {
+		It("should create secrets in namespace", func() {
 			var err error
 			mapping := crdv1.ProjectMapping{
 				Type:      crdv1.MatchMappingType,
@@ -85,8 +85,6 @@ var _ = Describe("Mapping", func() {
 			Expect(string(teamASecret.Data[v1.DockerConfigJsonKey])).To(Equal(`{"auths":{"my-registry-url":{"username":"robot$sync-bot","password":"my-token","auth":"cm9ib3Qkc3luYy1ib3Q6bXktdG9rZW4="}}}`))
 			Expect(string(teamBSecret.Data[v1.DockerConfigJsonKey])).To(Equal(`{"auths":{"my-registry-url":{"username":"robot$sync-bot","password":"my-token","auth":"cm9ib3Qkc3luYy1ib3Q6bXktdG9rZW4="}}}`))
 			Expect(SystemSecret.Data).To(BeNil())
-
-			close(done)
 		})
 	})
 
@@ -104,7 +102,7 @@ var _ = Describe("Mapping", func() {
 			test.DeleteHarborSyncConfig(k8sClient, "my-translate-cfg")
 		})
 
-		It("should create secrets in namespace", func(done Done) {
+		It("should create secrets in namespace", func() {
 			var err error
 			mapping := crdv1.ProjectMapping{
 				Type:      crdv1.MatchMappingType,
@@ -137,8 +135,6 @@ var _ = Describe("Mapping", func() {
 
 			Expect(string(teamASecret.Data[v1.DockerConfigJsonKey])).To(Equal(`{"auths":{"my-registry-url":{"username":"robot$sync-bot","password":"my-token","auth":"cm9ib3Qkc3luYy1ib3Q6bXktdG9rZW4="}}}`))
 			Expect(teamBSecret.Data).To(BeNil())
-
-			close(done)
 		})
 	})
 

pkg/reconciler/robot_test.go

@@ -70,7 +70,7 @@ var _ = Describe("Controller", func() {
 
 	Describe("Robot", func() {
 
-		It("should reconcile robot accounts", func(done Done) {
+		It("should reconcile robot accounts", func() {
 			cfg := test.EnsureHarborSyncConfigWithParams(k8sClient, "my-cfg", "my-project", &mapping, nil)
 			harborClient.GetRobotAccountsFunc = nil
 			credentials, changed, err := ReconcileRobotAccounts(
@@ -89,10 +89,9 @@ var _ = Describe("Controller", func() {
 			Expect(err).ToNot(HaveOccurred())
 			Expect(cacheCreds.Name).To(Equal(createdAccount.Name))
 			Expect(cacheCreds.Token).To(Equal(createdAccount.Token))
-			close(done)
 		})
 
-		It("should use robot credentials from store", func(done Done) {
+		It("should use robot credentials from store", func() {
 			cfg := test.EnsureHarborSyncConfigWithParams(k8sClient, "my-cfg", "my-project", &mapping, nil)
 			harborClient.CreateRobotAccountFunc = nil
 			credStore.Set("foo", crdv1.RobotAccountCredential{Name: "robot$sync-bot", Token: "bar"})
@@ -112,10 +111,9 @@ var _ = Describe("Controller", func() {
 			Expect(err).ToNot(HaveOccurred())
 			Expect(cacheCreds.Name).To(Equal("robot$sync-bot"))
 			Expect(cacheCreds.Token).To(Equal("bar"))
-			close(done)
 		})
 
-		It("should delete robot account when credentials are missing", func(done Done) {
+		It("should delete robot account when credentials are missing", func() {
 			cfg := test.EnsureHarborSyncConfigWithParams(k8sClient, "my-cfg", "my-project", &mapping, nil)
 			var deleteCalled bool
 			harborClient.DeleteRobotAccountFunc = func(project harbor.Project, robotID int) error {
@@ -139,10 +137,9 @@ var _ = Describe("Controller", func() {
 			Expect(err).ToNot(HaveOccurred())
 			Expect(cacheCreds.Name).To(Equal(createdAccount.Name))
 			Expect(cacheCreds.Token).To(Equal(createdAccount.Token))
-			close(done)
 		})
 
-		It("should re-create disabled account", func(done Done) {
+		It("should re-create disabled account", func() {
 			cfg := test.EnsureHarborSyncConfigWithParams(k8sClient, "my-cfg", "my-project", &mapping, nil)
 			harborClient.GetRobotAccountsFunc = func(project harbor.Project) ([]harbor.Robot, error) {
 				return []harbor.Robot{
@@ -177,10 +174,9 @@ var _ = Describe("Controller", func() {
 			Expect(err).ToNot(HaveOccurred())
 			Expect(cacheCreds.Name).To(Equal(createdAccount.Name))
 			Expect(cacheCreds.Token).To(Equal(createdAccount.Token))
-			close(done)
 		})
 
-		It("should re-create expiring account", func(done Done) {
+		It("should re-create expiring account", func() {
 			cfg := test.EnsureHarborSyncConfigWithParams(k8sClient, "my-cfg", "my-project", &mapping, nil)
 			harborClient.GetRobotAccountsFunc = func(project harbor.Project) ([]harbor.Robot, error) {
 				return []harbor.Robot{
@@ -214,10 +210,9 @@ var _ = Describe("Controller", func() {
 			Expect(err).ToNot(HaveOccurred())
 			Expect(cacheCreds.Name).To(Equal(createdAccount.Name))
 			Expect(cacheCreds.Token).To(Equal(createdAccount.Token))
-			close(done)
 		})
 
-		It("should rotate account", func(done Done) {
+		It("should rotate account", func() {
 			cfg := test.EnsureHarborSyncConfigWithParams(k8sClient, "my-cfg", "my-project", &mapping, nil)
 			harborClient.GetRobotAccountsFunc = func(project harbor.Project) ([]harbor.Robot, error) {
 				return []harbor.Robot{
@@ -251,7 +246,6 @@ var _ = Describe("Controller", func() {
 			Expect(err).ToNot(HaveOccurred())
 			Expect(cacheCreds.Name).To(Equal(createdAccount.Name))
 			Expect(cacheCreds.Token).To(Equal(createdAccount.Token))
-			close(done)
 		})
 	})
 })