diff --git a/api/v1alpha1/imagerepository_types.go b/api/v1alpha1/imagerepository_types.go index 01d29d2c..7095e676 100644 --- a/api/v1alpha1/imagerepository_types.go +++ b/api/v1alpha1/imagerepository_types.go @@ -17,8 +17,9 @@ limitations under the License. package v1alpha1 import ( + "time" + corev1 "k8s.io/api/core/v1" - apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/fluxcd/pkg/apis/meta" @@ -32,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 @@ -50,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 @@ -76,19 +83,9 @@ 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 { +func SetImageRepositoryReadiness(ir *ImageRepository, status metav1.ConditionStatus, reason, message string) { ir.Status.ObservedGeneration = ir.ObjectMeta.Generation - meta.SetResourceCondition(&ir, meta.ReadyCondition, status, reason, message) - return ir -} - -// GetLastTransitionTime returns the LastTransitionTime attribute of the ready conditon -func GetLastTransitionTime(ir ImageRepository) *metav1.Time { - if rc := apimeta.FindStatusCondition(ir.Status.Conditions, meta.ReadyCondition); rc != nil { - return &rc.LastTransitionTime - } - - return nil + meta.SetResourceCondition(ir, meta.ReadyCondition, status, reason, message) } // GetStatusConditions returns a pointer to the Status.Conditions slice @@ -96,6 +93,18 @@ func (in *ImageRepository) GetStatusConditions() *[]metav1.Condition { return &in.Status.Conditions } +// 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 // +kubebuilder:subresource:status // +kubebuilder:printcolumn:name="Last scan",type=string,JSONPath=`.status.lastScanTime` diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 9b2f4fe4..26a00276 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -199,8 +199,9 @@ func (in *ImageRepositoryList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ImageRepositorySpec) DeepCopyInto(out *ImageRepositorySpec) { *out = *in - if in.ScanInterval != nil { - in, out := &in.ScanInterval, &out.ScanInterval + out.Interval = in.Interval + if in.Timeout != nil { + in, out := &in.Timeout, &out.Timeout *out = new(v1.Duration) **out = **in } @@ -231,7 +232,7 @@ func (in *ImageRepositoryStatus) DeepCopyInto(out *ImageRepositoryStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } - out.LastScanResult = in.LastScanResult + in.LastScanResult.DeepCopyInto(&out.LastScanResult) out.ReconcileRequestStatus = in.ReconcileRequestStatus } @@ -248,6 +249,10 @@ func (in *ImageRepositoryStatus) DeepCopy() *ImageRepositoryStatus { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ScanResult) DeepCopyInto(out *ScanResult) { *out = *in + if in.ScanTime != nil { + in, out := &in.ScanTime, &out.ScanTime + *out = (*in).DeepCopy() + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ScanResult. diff --git a/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml b/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml index 15a4cb9b..dd76f01c 100644 --- a/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml +++ b/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: diff --git a/controllers/imagerepository_controller.go b/controllers/imagerepository_controller.go index d938aad9..a8c4e20f 100644 --- a/controllers/imagerepository_controller.go +++ b/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,7 +74,6 @@ 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) } @@ -86,13 +81,13 @@ func (r *ImageRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, er 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,52 @@ 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 + } + } + // Throttle scans based on spec Interval now := time.Now() ok, when := r.shouldScan(imageRepo, now) 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 +151,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,30 +198,29 @@ 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 } - // Is the controller seeing this because the reconcileAt + // Is the controller seeing this because the reconcileAt return nil // annotation was tweaked? Despite the name of the annotation, all // that matters is that it's different. if syncAt, ok := meta.ReconcileAnnotationValue(repo.GetAnnotations()); ok { @@ -235,7 +240,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 } diff --git a/controllers/policy_test.go b/controllers/policy_test.go index e4eb0863..85828c3c 100644 --- a/controllers/policy_test.go +++ b/controllers/policy_test.go @@ -19,11 +19,11 @@ package controllers import ( "context" "net/http/httptest" - "time" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" imagev1alpha1 "github.com/fluxcd/image-reflector-controller/api/v1alpha1" @@ -51,7 +51,8 @@ var _ = Describe("ImagePolicy controller", func() { repo := imagev1alpha1.ImageRepository{ Spec: imagev1alpha1.ImageRepositorySpec{ - Image: imgRepo, + Interval: metav1.Duration{Duration: reconciliationInterval}, + Image: imgRepo, }, } imageObjectName := types.NamespacedName{ @@ -61,7 +62,7 @@ var _ = Describe("ImagePolicy controller", func() { repo.Name = imageObjectName.Name repo.Namespace = imageObjectName.Namespace - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), contextTimeout) defer cancel() r := imageRepoReconciler @@ -94,7 +95,7 @@ var _ = Describe("ImagePolicy controller", func() { pol.Namespace = polName.Namespace pol.Name = polName.Name - ctx, cancel = context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancel = context.WithTimeout(context.Background(), contextTimeout) defer cancel() Expect(r.Create(ctx, &pol)).To(Succeed()) diff --git a/controllers/scan_test.go b/controllers/scan_test.go index f4e58e55..ed1a35da 100644 --- a/controllers/scan_test.go +++ b/controllers/scan_test.go @@ -20,13 +20,13 @@ import ( "context" "fmt" "net/http/httptest" - "time" "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/v1/remote" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" imagev1alpha1 "github.com/fluxcd/image-reflector-controller/api/v1alpha1" @@ -55,7 +55,8 @@ var _ = Describe("ImageRepository controller", func() { // 2. probably going to want to have several test cases repo = imagev1alpha1.ImageRepository{ Spec: imagev1alpha1.ImageRepositorySpec{ - Image: "alpine", + Interval: metav1.Duration{Duration: reconciliationInterval}, + Image: "alpine", }, } imageRepoName := types.NamespacedName{ @@ -66,7 +67,7 @@ var _ = Describe("ImageRepository controller", func() { repo.Name = imageRepoName.Name repo.Namespace = imageRepoName.Namespace - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), contextTimeout) defer cancel() r := imageRepoReconciler @@ -89,7 +90,8 @@ var _ = Describe("ImageRepository controller", func() { repo = imagev1alpha1.ImageRepository{ Spec: imagev1alpha1.ImageRepositorySpec{ - Image: imgRepo, + Interval: metav1.Duration{Duration: reconciliationInterval}, + Image: imgRepo, }, } objectName := types.NamespacedName{ @@ -100,7 +102,7 @@ var _ = Describe("ImageRepository controller", func() { repo.Name = objectName.Name repo.Namespace = objectName.Namespace - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), contextTimeout) defer cancel() r := imageRepoReconciler @@ -119,8 +121,9 @@ var _ = Describe("ImageRepository controller", func() { It("does not process the image", func() { repo = imagev1alpha1.ImageRepository{ Spec: imagev1alpha1.ImageRepositorySpec{ - Image: "alpine", - Suspend: true, + Interval: metav1.Duration{Duration: reconciliationInterval}, + Image: "alpine", + Suspend: true, }, } imageRepoName := types.NamespacedName{ @@ -131,7 +134,7 @@ var _ = Describe("ImageRepository controller", func() { repo.Name = imageRepoName.Name repo.Namespace = imageRepoName.Namespace - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), contextTimeout) defer cancel() r := imageRepoReconciler @@ -159,7 +162,8 @@ var _ = Describe("ImageRepository controller", func() { repo = imagev1alpha1.ImageRepository{ Spec: imagev1alpha1.ImageRepositorySpec{ - Image: imgRepo, + Interval: metav1.Duration{Duration: reconciliationInterval}, + Image: imgRepo, }, } objectName := types.NamespacedName{ @@ -170,7 +174,7 @@ var _ = Describe("ImageRepository controller", func() { repo.Name = objectName.Name repo.Namespace = objectName.Namespace - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), contextTimeout) defer cancel() r := imageRepoReconciler @@ -181,13 +185,11 @@ var _ = Describe("ImageRepository controller", func() { var repoAfter imagev1alpha1.ImageRepository Eventually(func() bool { err := r.Get(context.Background(), objectName, &repoAfter) - return err == nil && repoAfter.Status.CanonicalImageName != "" + return err == nil && repoAfter.Status.CanonicalImageName != "" && repoAfter.Status.LastScanResult.ScanTime != nil }, timeout, interval).Should(BeTrue()) - lastScan := imagev1alpha1.GetLastTransitionTime(repoAfter) - Expect(lastScan).ToNot(BeNil()) - requestToken := "this can be anything, so long as it's a change" + lastScanTime := repoAfter.Status.LastScanResult.ScanTime repoAfter.Annotations = map[string]string{ meta.ReconcileAtAnnotation: requestToken, @@ -195,7 +197,7 @@ var _ = Describe("ImageRepository controller", func() { Expect(r.Update(ctx, &repoAfter)).To(Succeed()) Eventually(func() bool { err := r.Get(context.Background(), objectName, &repoAfter) - return err == nil && imagev1alpha1.GetLastTransitionTime(repoAfter).After(lastScan.Time) + return err == nil && repoAfter.Status.LastScanResult.ScanTime.After(lastScanTime.Time) }, timeout, interval).Should(BeTrue()) Expect(repoAfter.Status.LastHandledReconcileAt).To(Equal(requestToken)) }) @@ -249,7 +251,8 @@ var _ = Describe("ImageRepository controller", func() { repo = imagev1alpha1.ImageRepository{ Spec: imagev1alpha1.ImageRepositorySpec{ - Image: imgRepo, + Interval: metav1.Duration{Duration: reconciliationInterval}, + Image: imgRepo, SecretRef: &corev1.LocalObjectReference{ Name: "docker", }, @@ -263,7 +266,7 @@ var _ = Describe("ImageRepository controller", func() { repo.Name = objectName.Name repo.Namespace = objectName.Namespace - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), contextTimeout) defer cancel() r := imageRepoReconciler diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 4094bac0..57af2687 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -38,9 +38,10 @@ import ( // for Eventually const ( - timeout = time.Second * 30 - interval = time.Second * 1 - // indexInterval = time.Second * 1 + timeout = time.Second * 30 + contextTimeout = time.Second * 5 + interval = time.Second * 1 + reconciliationInterval = time.Second * 5 ) // These tests use Ginkgo (BDD-style Go testing framework). Refer to