Merge pull request #56 from relu/api-refactoring

API refactoring

api/go.mod

@@ -3,7 +3,7 @@ module github.com/fluxcd/image-reflector-controller/api
 go 1.15
 
 require (
-	github.com/fluxcd/pkg/apis/meta v0.3.0
+	github.com/fluxcd/pkg/apis/meta v0.4.0
 	k8s.io/api v0.19.3
 	k8s.io/apimachinery v0.19.3
 	sigs.k8s.io/controller-runtime v0.6.3

api/go.sum

@@ -61,8 +61,8 @@ github.com/evanphx/json-patch v4.2.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLi
 github.com/evanphx/json-patch v4.5.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk=
 github.com/evanphx/json-patch v4.9.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk=
 github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4=
-github.com/fluxcd/pkg/apis/meta v0.3.0 h1:o2YkfGgf0j8sKeZs8cBmmmMKLA7kEoS1qYViOial1Ds=
-github.com/fluxcd/pkg/apis/meta v0.3.0/go.mod h1:wOzQQx8CdtUQCGaLzqGu4QgnNxYkI6/wvdvlovxWhF0=
+github.com/fluxcd/pkg/apis/meta v0.4.0 h1:JChqB9GGgorW9HWKxirTVV0rzrcLyzBaVjinmqZ0iHA=
+github.com/fluxcd/pkg/apis/meta v0.4.0/go.mod h1:wOzQQx8CdtUQCGaLzqGu4QgnNxYkI6/wvdvlovxWhF0=
 github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo=
 github.com/fsnotify/fsnotify v1.4.9 h1:hsms1Qyu0jgnwNXIxa+/V/PDsU6CfLf6CNO8H7IWoS4=
 github.com/fsnotify/fsnotify v1.4.9/go.mod h1:znqG4EE+3YCdAaPaxE2ZRY/06pZUdp0tY4IgpuI1SZQ=

api/v1alpha1/imagerepository_types.go

@@ -17,6 +17,8 @@ limitations under the License.
 package v1alpha1
 
 import (
+	"time"
+
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
@@ -31,10 +33,15 @@ type ImageRepositorySpec struct {
 	// Image is the name of the image repository
 	// +required
 	Image string `json:"image,omitempty"`
-	// ScanInterval is the (minimum) length of time to wait between
+	// Interval is the length of time to wait between
 	// scans of the image repository.
+	// +required
+	Interval metav1.Duration `json:"interval,omitempty"`
+
+	// Timeout for image scanning.
+	// Defaults to 'Interval' duration.
 	// +optional
-	ScanInterval *metav1.Duration `json:"scanInterval,omitempty"`
+	Timeout *metav1.Duration `json:"timeout,omitempty"`
 
 	// SecretRef can be given the name of a secret containing
 	// credentials to use for the image registry. The secret should be
@@ -49,7 +56,8 @@ type ImageRepositorySpec struct {
 }
 
 type ScanResult struct {
-	TagCount int `json:"tagCount"`
+	TagCount int          `json:"tagCount"`
+	ScanTime *metav1.Time `json:"scanTime,omitempty"`
 }
 
 // ImageRepositoryStatus defines the observed state of ImageRepository
@@ -75,28 +83,26 @@ type ImageRepositoryStatus struct {
 }
 
 // SetImageRepositoryReadiness sets the ready condition with the given status, reason and message.
-func SetImageRepositoryReadiness(ir ImageRepository, status metav1.ConditionStatus, reason, message string) ImageRepository {
-	ir.Status.Conditions = []metav1.Condition{
-		{
-			Type:               meta.ReadyCondition,
-			Status:             status,
-			LastTransitionTime: metav1.Now(),
-			Reason:             reason,
-			Message:            message,
-		},
-	}
+func SetImageRepositoryReadiness(ir *ImageRepository, status metav1.ConditionStatus, reason, message string) {
 	ir.Status.ObservedGeneration = ir.ObjectMeta.Generation
-	return ir
+	meta.SetResourceCondition(ir, meta.ReadyCondition, status, reason, message)
 }
 
-func GetLastTransitionTime(ir ImageRepository) *metav1.Time {
-	for _, condition := range ir.Status.Conditions {
-		if condition.Type == meta.ReadyCondition {
-			return &condition.LastTransitionTime
-		}
-	}
+// GetStatusConditions returns a pointer to the Status.Conditions slice
+func (in *ImageRepository) GetStatusConditions() *[]metav1.Condition {
+	return &in.Status.Conditions
+}
 
-	return nil
+// GetTimeout returns the timeout with default.
+func (in ImageRepository) GetTimeout() time.Duration {
+	duration := in.Spec.Interval.Duration
+	if in.Spec.Timeout != nil {
+		duration = in.Spec.Timeout.Duration
+	}
+	if duration < time.Second {
+		return time.Second
+	}
+	return duration
 }
 
 // +kubebuilder:object:root=true

config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml

@@ -47,9 +47,9 @@ spec:
               image:
                 description: Image is the name of the image repository
                 type: string
-              scanInterval:
-                description: ScanInterval is the (minimum) length of time to wait
-                  between scans of the image repository.
+              interval:
+                description: Interval is the length of time to wait between scans
+                  of the image repository.
                 type: string
               secretRef:
                 description: SecretRef can be given the name of a secret containing
@@ -66,6 +66,9 @@ spec:
                   image scans. It does not apply to already started scans. Defaults
                   to false.
                 type: boolean
+              timeout:
+                description: Timeout for image scanning. Defaults to 'Interval' duration.
+                type: string
             type: object
           status:
             description: ImageRepositoryStatus defines the observed state of ImageRepository
@@ -151,6 +154,9 @@ spec:
               lastScanResult:
                 description: LastScanResult contains the number of fetched tags.
                 properties:
+                  scanTime:
+                    format: date-time
+                    type: string
                   tagCount:
                     type: integer
                 required:

controllers/imagerepository_controller.go

@@ -43,11 +43,6 @@ import (
 	imagev1alpha1 "github.com/fluxcd/image-reflector-controller/api/v1alpha1"
 )
 
-const (
-	scanTimeout         = 10 * time.Second
-	defaultScanInterval = 10 * time.Minute
-)
-
 type DatabaseWriter interface {
 	SetTags(repo string, tags []string)
 }
@@ -70,6 +65,7 @@ type ImageRepositoryReconciler struct {
 
 func (r *ImageRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	ctx := context.Background()
+	reconcileStart := time.Now()
 
 	// NB: In general, if an error is returned then controller-runtime
 	// will requeue the request with back-off. In the following this
@@ -78,21 +74,20 @@ func (r *ImageRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, er
 
 	var imageRepo imagev1alpha1.ImageRepository
 	if err := r.Get(ctx, req.NamespacedName, &imageRepo); err != nil {
-		// _Might_ get requeued
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
 	log := r.Log.WithValues("controller", strings.ToLower(imagev1alpha1.ImageRepositoryKind), "request", req.NamespacedName)
 
 	if imageRepo.Spec.Suspend {
 		msg := "ImageRepository is suspended, skipping reconciliation"
-		status := imagev1alpha1.SetImageRepositoryReadiness(
-			imageRepo,
+		imagev1alpha1.SetImageRepositoryReadiness(
+			&imageRepo,
 			metav1.ConditionFalse,
 			meta.SuspendedReason,
 			msg,
 		)
-		if err := r.Status().Update(ctx, &status); err != nil {
+		if err := r.Status().Update(ctx, &imageRepo); err != nil {
 			log.Error(err, "unable to update status")
 			return ctrl.Result{Requeue: true}, err
 		}
@@ -102,47 +97,51 @@ func (r *ImageRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, er
 
 	ref, err := name.ParseReference(imageRepo.Spec.Image)
 	if err != nil {
-		status := imagev1alpha1.SetImageRepositoryReadiness(
-			imageRepo,
+		imagev1alpha1.SetImageRepositoryReadiness(
+			&imageRepo,
 			metav1.ConditionFalse,
 			imagev1alpha1.ImageURLInvalidReason,
 			err.Error(),
 		)
-		if err := r.Status().Update(ctx, &status); err != nil {
+		if err := r.Status().Update(ctx, &imageRepo); err != nil {
 			return ctrl.Result{Requeue: true}, err
 		}
 		log.Error(err, "Unable to parse image name", "imageName", imageRepo.Spec.Image)
 		return ctrl.Result{Requeue: true}, err
 	}
 
-	imageRepo.Status.CanonicalImageName = ref.Context().String()
+	// Set CanonicalImageName based on the parsed reference
+	if c := ref.Context().String(); imageRepo.Status.CanonicalImageName != c {
+		imageRepo.Status.CanonicalImageName = c
+		if err = r.Status().Update(ctx, &imageRepo); err != nil {
+			return ctrl.Result{Requeue: true}, err
+		}
+	}
 
-	now := time.Now()
-	ok, when := r.shouldScan(imageRepo, now)
+	// Throttle scans based on spec Interval
+	ok, when := r.shouldScan(imageRepo, reconcileStart)
 	if ok {
-		ctx, cancel := context.WithTimeout(ctx, scanTimeout)
-		defer cancel()
-
-		reconciledRepo, reconcileErr := r.scan(ctx, imageRepo, ref)
-		if err = r.Status().Update(ctx, &reconciledRepo); err != nil {
+		reconcileErr := r.scan(ctx, &imageRepo, ref)
+		if err = r.Status().Update(ctx, &imageRepo); err != nil {
 			return ctrl.Result{Requeue: true}, err
 		}
-
 		if reconcileErr != nil {
 			return ctrl.Result{Requeue: true}, reconcileErr
-		} else {
-			log.Info(fmt.Sprintf("reconciliation finished in %s, next run in %s",
-				time.Now().Sub(now).String(),
-				when),
-			)
 		}
 	}
 
+	log.Info(fmt.Sprintf("reconciliation finished in %s, next run in %s",
+		time.Now().Sub(reconcileStart).String(),
+		when.String(),
+	))
+
 	return ctrl.Result{RequeueAfter: when}, nil
 }
 
-func (r *ImageRepositoryReconciler) scan(ctx context.Context, imageRepo imagev1alpha1.ImageRepository, ref name.Reference) (imagev1alpha1.ImageRepository, error) {
-	canonicalName := ref.Context().String()
+func (r *ImageRepositoryReconciler) scan(ctx context.Context, imageRepo *imagev1alpha1.ImageRepository, ref name.Reference) error {
+	timeout := imageRepo.GetTimeout()
+	ctx, cancel := context.WithTimeout(ctx, timeout)
+	defer cancel()
 
 	var options []remote.Option
 	if imageRepo.Spec.SecretRef != nil {
@@ -151,39 +150,45 @@ func (r *ImageRepositoryReconciler) scan(ctx context.Context, imageRepo imagev1a
 			Namespace: imageRepo.GetNamespace(),
 			Name:      imageRepo.Spec.SecretRef.Name,
 		}, &secret); err != nil {
-			return imagev1alpha1.SetImageRepositoryReadiness(
+			imagev1alpha1.SetImageRepositoryReadiness(
 				imageRepo,
 				metav1.ConditionFalse,
 				meta.ReconciliationFailedReason,
 				err.Error(),
-			), err
+			)
+			return err
 		}
 		auth, err := authFromSecret(secret, ref.Context().RegistryStr())
 		if err != nil {
-			return imagev1alpha1.SetImageRepositoryReadiness(
+			imagev1alpha1.SetImageRepositoryReadiness(
 				imageRepo,
 				metav1.ConditionFalse,
 				meta.ReconciliationFailedReason,
 				err.Error(),
-			), err
+			)
+			return err
 		}
 		options = append(options, remote.WithAuth(auth))
 	}
 
 	tags, err := remote.ListWithContext(ctx, ref.Context(), options...)
 	if err != nil {
-		return imagev1alpha1.SetImageRepositoryReadiness(
+		imagev1alpha1.SetImageRepositoryReadiness(
 			imageRepo,
 			metav1.ConditionFalse,
 			meta.ReconciliationFailedReason,
 			err.Error(),
-		), err
+		)
+		return err
 	}
 
+	canonicalName := ref.Context().String()
 	// TODO: add context and error handling to database ops
 	r.Database.SetTags(canonicalName, tags)
 
+	scanTime := metav1.Now()
 	imageRepo.Status.LastScanResult.TagCount = len(tags)
+	imageRepo.Status.LastScanResult.ScanTime = &scanTime
 
 	// if the reconcile request annotation was set, consider it
 	// handled (NB it doesn't matter here if it was changed since last
@@ -192,26 +197,25 @@ func (r *ImageRepositoryReconciler) scan(ctx context.Context, imageRepo imagev1a
 		imageRepo.Status.SetLastHandledReconcileRequest(token)
 	}
 
-	return imagev1alpha1.SetImageRepositoryReadiness(
+	imagev1alpha1.SetImageRepositoryReadiness(
 		imageRepo,
 		metav1.ConditionTrue,
 		meta.ReconciliationSucceededReason,
 		fmt.Sprintf("successful scan, found %v tags", len(tags)),
-	), nil
+	)
+
+	return nil
 }
 
 // shouldScan takes an image repo and the time now, and says whether
 // the repository should be scanned now, and how long to wait for the
 // next scan.
 func (r *ImageRepositoryReconciler) shouldScan(repo imagev1alpha1.ImageRepository, now time.Time) (bool, time.Duration) {
-	scanInterval := defaultScanInterval
-	if repo.Spec.ScanInterval != nil {
-		scanInterval = repo.Spec.ScanInterval.Duration
-	}
+	scanInterval := repo.Spec.Interval.Duration
 
 	// never scanned; do it now
-	lastTransitionTime := imagev1alpha1.GetLastTransitionTime(repo)
-	if lastTransitionTime == nil {
+	lastScanTime := repo.Status.LastScanResult.ScanTime
+	if lastScanTime == nil {
 		return true, scanInterval
 	}
 
@@ -235,7 +239,7 @@ func (r *ImageRepositoryReconciler) shouldScan(repo imagev1alpha1.ImageRepositor
 		return true, scanInterval
 	}
 
-	when := scanInterval - now.Sub(lastTransitionTime.Time)
+	when := scanInterval - now.Sub(lastScanTime.Time)
 	if when < time.Second {
 		return true, scanInterval
 	}