fix: prevent status update loop (#97)

Author: moolen

.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"`
 

api/v1/zz_generated.deepcopy.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")

cmd/controller.go

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

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

@@ -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.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/controller_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) {