hardening and optimisations around when to run checks

deploy/crds/example/monitor.yaml

@@ -4,4 +4,4 @@ metadata:
   name: example-imagemonitor
 spec:
   # Add fields here
-  excludePattern: "^todo$"
+  excludePattern: ""

pkg/cluster/deployments_deployment_configs.go

@@ -27,8 +27,6 @@ func (ol *ObjectsLabeler)LabelAllDeploymentsAndDeploymentConfigs(ctx context.Con
 	var listOpts = &client.ListOptions{Namespace:ns}
 	var matchRegex *regexp.Regexp
 
-
-
 	if err := ol.client.List(ctx,listOpts,dcList); err != nil{
 		return errors.Wrap(err, "failed to list deployment configs in namespace " + ns)
 	}
@@ -42,11 +40,12 @@ func (ol *ObjectsLabeler)LabelAllDeploymentsAndDeploymentConfigs(ctx context.Con
 			return errors.Wrap(err, "failed to compile pattern to exclude " + excludePattern)
 		}
 
-
 		for _, dc := range dcList.Items{
-			if matchRegex.MatchString(dc.Name){
-				fmt.Println("skipping ", dc.Name, " as matched by excludePattern" + excludePattern)
-				continue
+			if excludePattern != "" {
+				if matchRegex.MatchString(dc.Name) {
+					fmt.Println("skipping ", dc.Name, " as matched by excludePattern"+excludePattern)
+					continue
+				}
 			}
 			// dont care about over writing as these will be our namespaced labels
 			for k, v :=  range labels{

pkg/cluster/images.go

@@ -41,7 +41,6 @@ func (is *ImageService) FindImagesFromImageChangeParams(defaultNS string, params
 	}
 	log.V(10).Info("selector ", "s ", strings.Join(selectors, ","))
 	// find all the pods that match the dc labels
-	fmt.Println("label selector for pods ",strings.Join(selectors, ","))
 	pods, err := is.client.CoreV1().Pods(defaultNS).List(v1.ListOptions{LabelSelector: strings.Join(selectors, ",")})
 	if err != nil {
 		log.Error(err, "failed to list pods with labels "+strings.Join(selectors, ","))
@@ -123,7 +122,6 @@ func (is *ImageService) FindImagesFromLabels(ns string, deploymentLabels map[str
 		selectors = append(selectors, fmt.Sprintf("%s=%s", k, v))
 	}
 	log.V(10).Info("selector ", "s ", strings.Join(selectors, ","))
-	fmt.Println("label selector for pods ",strings.Join(selectors, ","))
 	pods, err := is.client.CoreV1().Pods(ns).List(v1.ListOptions{LabelSelector: strings.Join(selectors, ",")})
 	if err != nil {
 		log.Error(err, "failed to list pods with labels "+strings.Join(selectors, ","))

pkg/controller/deploymentconfigs/controller.go

@@ -69,6 +69,7 @@ clusterImageService := cluster.NewImageService(k8sClient, isClient)
 			registryImageService: riService,
 			dcClient:dcClient,
 		},
+		imageService:clusterImageService,
 	}
 }
 
@@ -93,6 +94,7 @@ type ReconcileDeploymentConfig struct {
 	dcClient            v12.AppsV1Interface
 	isClient            v13.ImageV1Interface
 	podService          *cluster.Pods
+	imageService        *cluster.ImageService
 	// turn into interfaces
 	reportService *Reports
 }
@@ -102,26 +104,33 @@ func (r *ReconcileDeploymentConfig) Reconcile(request reconcile.Request) (reconc
 	// we can label the deployment with last run time and we will see it again immediately but will then reque it based on the next check time
 	dc, err := r.dcClient.DeploymentConfigs(request.Namespace).Get(request.Name, v14.GetOptions{})
 	if err != nil{
-		log.Info("failed to get deployment config " + request.Namespace + "  ")
+		log.Error(err,"failed to get deployment config " + request.Namespace + "  " + request.Name )
+		return reconcile.Result{}, err
+	}
+	if _, ok := dc.Labels[domain.HeimdallMonitored]; !ok {
 		return reconcile.Result{}, nil
 	}
-	// TODO expand should check to check the image in the dc against the ones in the labels, note we might be able to use cluster.FindImagesFromImageChangeParams and
-	// cluster.FindImagesFromLabels
-	should,err := validation.ShouldCheck(dc)
+	images, err := r.reportService.GetImages(dc)
+	if err != nil{
+		log.Error(err, "failed to get images for deployment config when checking if should run check again")
+		return reconcile.Result{}, err
+	}
+	should,err := validation.ShouldCheck(dc, images)
 	if err != nil{
 		if validation.IsParseErr(err){
 			delete(dc.Annotations, domain.HeimdallLastChecked)
 			if _, err := r.dcClient.DeploymentConfigs(request.Namespace).Update(dc); err != nil{
 				// in this case we will requeue log the error and requeue to ensure we dont keep retrying the checks
 				log.Error(err, " failed to label deployment config " + request.Namespace + " " + request.Name)
-				return reconcile.Result{RequeueAfter: requeAfterFourHours}, nil
+				return reconcile.Result{}, nil
 			}
 			return reconcile.Result{}, nil
 		}
 	}
 	if ! should{
+		//return and we will see it again once it changes or 4 hrs passes
 		log.Info("critera for re checking " +dc.Name+ " not met")
-		return reconcile.Result{}, nil
+		return reconcile.Result{RequeueAfter: requeAfterFourHours}, nil
 	}
 
 	log.Info("deployment config " +dc.Name+" in namespace "+dc.Namespace+" is being monitored by heimdall")
@@ -131,11 +140,11 @@ func (r *ReconcileDeploymentConfig) Reconcile(request reconcile.Request) (reconc
 		log.Error(err,"failed to generate a report for images in dc " + request.Name + " in namespace " + request.Namespace)
 		return reconcile.Result{RequeueAfter: requeAfterFourHours}, nil
 	}
-
+	// after the report has been run we want to annotate our dc with information. If we fail here we may end up re running the report.
 	// reports can take some time so get a fresh dc copy
 	dc, err = r.dcClient.DeploymentConfigs(request.Namespace).Get(request.Name, v14.GetOptions{})
 	if err != nil{
-		log.Info("failed to get deployment config " + request.Namespace + "  ")
+		log.Error(err,"failed to get deployment config " + request.Namespace + "  " + request.Name )
 		return reconcile.Result{}, nil
 	}
 	// have to use annotation as labels have strict length and format
@@ -149,17 +158,15 @@ func (r *ReconcileDeploymentConfig) Reconcile(request reconcile.Request) (reconc
 		checked = append(checked,rep.ClusterImage.SHA256Path)
 		if err := r.podService.LabelPods(&rep); err != nil{
 			log.Error(err,"failed to label pod ")
-			return reconcile.Result{RequeueAfter: requeAfterFourHours}, nil
+			return reconcile.Result{}, nil
 		}
 	}
 	dc.Annotations[domain.HeimdallImagesChecked] = strings.Join(checked, ",")
 	if _, err := r.dcClient.DeploymentConfigs(request.Namespace).Update(dc); err != nil{
 		// in this case we will requeue log the error and requeue to ensure we dont keep retrying the checks
 		log.Error(err, " failed to label deployment config " + request.Namespace + " " + request.Name)
-		return reconcile.Result{RequeueAfter: requeAfterFourHours}, nil
+		return reconcile.Result{}, nil
 	}
-	// finally label the deployment with hiemdall.lastcheck time that needs to pass before we run the check again and requeue.
-	// also could label with last image seen the logic would then be has the image changed rerun else check the time.
-
+	// ensure we see this dc 4 hours from now or when it next changes
 	return reconcile.Result{RequeueAfter:requeAfterFourHours}, nil
 }

pkg/controller/deploymentconfigs/report.go

@@ -6,13 +6,13 @@ import (
 	"github.com/integr8ly/heimdall/pkg/domain"
 	"github.com/integr8ly/heimdall/pkg/registry"
 	v1 "github.com/openshift/api/apps/v1"
-	"github.com/pkg/errors"
 	v12 "github.com/openshift/client-go/apps/clientset/versioned/typed/apps/v1"
+	"github.com/pkg/errors"
 	v13 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"strings"
 )
 
-func getImageChangeParams(dc *v1.DeploymentConfig)[]*v1.DeploymentTriggerImageChangeParams  {
+func getImageChangeParams(dc *v1.DeploymentConfig) []*v1.DeploymentTriggerImageChangeParams {
 	var ret []*v1.DeploymentTriggerImageChangeParams
 	for _, tr := range dc.Spec.Triggers {
 		if tr.Type == v1.DeploymentTriggerOnImageChange && tr.ImageChangeParams != nil {
@@ -22,60 +22,45 @@ func getImageChangeParams(dc *v1.DeploymentConfig)[]*v1.DeploymentTriggerImageCh
 	return ret
 }
 
-
 type Reports struct {
-	clusterImageService *cluster.ImageService
+	clusterImageService  *cluster.ImageService
 	registryImageService *registry.ImageService
-	dcClient *v12.AppsV1Client
+	dcClient             *v12.AppsV1Client
 }
 
 func NewReport(clusterImageService *cluster.ImageService,
-registryImageService *registry.ImageService, dcClient *v12.AppsV1Client)*Reports  {
+	registryImageService *registry.ImageService, dcClient *v12.AppsV1Client) *Reports {
 	return &Reports{
 		clusterImageService:  clusterImageService,
 		registryImageService: registryImageService,
-		dcClient: dcClient,
+		dcClient:             dcClient,
 	}
 }
 
-func (r *Reports)Generate(ns, deploymentConfig string)([]domain.ReportResult,  error)  {
+func (r *Reports) Generate(ns, deploymentConfig string) ([]domain.ReportResult, error) {
 	var dcs []v1.DeploymentConfig
 
-	if deploymentConfig == "*"{
+	if deploymentConfig == "*" {
 		dcList, err := r.dcClient.DeploymentConfigs(ns).List(v13.ListOptions{})
-		if err != nil{
-			return nil, errors.Wrap(err, "failed to list deploymentconfigs in namespace " + ns)
+		if err != nil {
+			return nil, errors.Wrap(err, "failed to list deploymentconfigs in namespace "+ns)
 		}
 		dcs = dcList.Items
-	}else{
+	} else {
 		dc, err := r.dcClient.DeploymentConfigs(ns).Get(deploymentConfig, v13.GetOptions{})
-		if err != nil{
-			return nil, errors.Wrap(err, "failed to get deploymentconfigs in namespace " + ns + " with name " + deploymentConfig)
+		if err != nil {
+			return nil, errors.Wrap(err, "failed to get deploymentconfigs in namespace "+ns+" with name "+deploymentConfig)
 		}
 		dcs = append(dcs, *dc)
 	}
 
 	var reports []domain.ReportResult
-	var checked= map[string]domain.ReportResult{}
+	var checked = map[string]domain.ReportResult{}
 	for _, dc := range dcs {
-		log.Info("checking deployment with resource version " + dc.ResourceVersion)
-		var images []*domain.ClusterImage
-		log.Info("got deployment config ", "name", dc.Name)
-		icp := getImageChangeParams(&dc)
-		if len(icp) > 0 {
-			is, err := r.clusterImageService.FindImagesFromImageChangeParams(dc.Namespace, icp, dc.Spec.Template.Labels)
-			if err != nil {
-				return reports, errors.Wrap(err, "failed find images in deploymentconfig via its image triggers ")
-			}
-			images = append(images, is...)
-		} else {
-			is, err := r.clusterImageService.FindImagesFromLabels(dc.Namespace, dc.Spec.Template.Labels)
-			if err != nil {
-				return reports, errors.Wrap(err, "failed find images in deploymentconfig")
-			}
-			images = append(images, is...)
+		images, err := r.GetImages(&dc)
+		if err != nil {
+			return reports, errors.Wrap(err, "failed to get images for deployment config ")
 		}
-		log.Info("found images ", "no:", len(images))
 
 		for _, i := range images {
 			if !strings.Contains(i.FullPath, "redhat") {
@@ -96,7 +81,7 @@ func (r *Reports)Generate(ns, deploymentConfig string)([]domain.ReportResult,  e
 				fmt.Println("error creating report ", err)
 				fmt.Println("REPORTs ", len(reports))
 				log.Error(err, "a report failed ")
-			}else {
+			} else {
 				result.ClusterImage = i
 				result.Component = dc.Name
 				reports = append(reports, result)
@@ -107,3 +92,26 @@ func (r *Reports)Generate(ns, deploymentConfig string)([]domain.ReportResult,  e
 	}
 	return reports, nil
 }
+
+// get a list of the cluster images
+func (r *Reports) GetImages(dc *v1.DeploymentConfig) ([]*domain.ClusterImage, error) {
+	log.Info("checking deployment with resource version " + dc.ResourceVersion)
+	var images []*domain.ClusterImage
+	log.Info("got deployment config ", "name", dc.Name)
+	icp := getImageChangeParams(dc)
+	if len(icp) > 0 {
+		is, err := r.clusterImageService.FindImagesFromImageChangeParams(dc.Namespace, icp, dc.Spec.Template.Labels)
+		if err != nil {
+			return nil, errors.Wrap(err, "failed find images in deploymentconfig via its image triggers ")
+		}
+		images = append(images, is...)
+	} else {
+		is, err := r.clusterImageService.FindImagesFromLabels(dc.Namespace, dc.Spec.Template.Labels)
+		if err != nil {
+			return nil, errors.Wrap(err, "failed find images in deploymentconfig")
+		}
+		images = append(images, is...)
+	}
+	log.Info("found images ", "no:", len(images))
+	return images, nil
+}

pkg/controller/deployments/controller.go

@@ -18,6 +18,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 	"sigs.k8s.io/controller-runtime/pkg/source"
+	"strings"
 	"time"
 )
 
@@ -49,6 +50,7 @@ func newReconciler(mgr manager.Manager, k8sClient kubernetes.Interface,  riServi
 			deploymentClient: k8sClient.AppsV1(),
 		},
 		podService:       cluster.NewPods(mgr.GetClient()),
+		imageService:clusterImageService,
 	}
 }
 
@@ -63,43 +65,69 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
 }
 
 func (r *ReconcileDeployment) Reconcile(request reconcile.Request) (reconcile.Result, error) {
+	ctx := context.TODO()
 	d := &v12.Deployment{}
 	err := r.client.Get(context.TODO(), client.ObjectKey{Namespace:request.Namespace, Name:request.Name}, d)
 	if err != nil{
-		log.Info("failed to get deployment in namespace " + request.Namespace + " with name  " + d.Name)
+		log.Error(err,"failed to get deployment in namespace " + request.Namespace + " with name  " + d.Name)
+		return reconcile.Result{}, err
+	}
+	// ignore if not labeled
+	if _, ok := d.Labels[domain.HeimdallMonitored]; !ok {
 		return reconcile.Result{}, nil
 	}
-	should, err := validation.ShouldCheck(d)
+	images, err := r.reportService.GetImages(d)
+	if err != nil{
+		return reconcile.Result{}, err
+	}
+	should, err := validation.ShouldCheck(d, images)
 	if err != nil{
 		if validation.IsParseErr(err){
 			delete(d.Annotations, domain.HeimdallLastChecked)
 			if err := r.client.Update(context.TODO(),d); err != nil{
 				// in this case we will requeue log the error and requeue to ensure we dont keep retrying the checks
 				log.Error(err, " failed to label deployment " + request.Namespace + " " + request.Name)
-				return reconcile.Result{RequeueAfter: requeAfterFourHours}, nil
+				return reconcile.Result{}, err
 			}
-			return reconcile.Result{}, nil
+			return reconcile.Result{}, err
 		}
 	}
 	if ! should{
 		log.Info("critera for re checking " +d.Name+ " not met")
 		return reconcile.Result{}, nil
 	}
 
-
 	log.Info("deployment " +d.Name+" in namespace "+d.Namespace+" is being monitored by heimdall")
+
 	report, err :=r.reportService.Generate(request.Namespace, request.Name)
 	if err != nil{
 		log.Error(err,"failed to generate a report for images in dc " + request.Name + " in namespace " + request.Namespace)
 		return reconcile.Result{RequeueAfter: requeAfterFourHours}, nil
 	}
 	log.Info("generated reports for deployment ", "reports", len(report), "namespace", request.Namespace, "name", request.Name)
+	// make sure we are upto date
+	err = r.client.Get(context.TODO(), client.ObjectKey{Namespace:request.Namespace, Name:request.Name}, d)
+	if err != nil{
+		log.Info("failed to get deployment in namespace " + request.Namespace + " with name  " + d.Name)
+		return reconcile.Result{}, nil
+	}
+	if d.Annotations == nil{
+		d.Annotations = map[string]string{}
+	}
+	d.Annotations[domain.HeimdallLastChecked] = time.Now().Format(domain.TimeFormat)
+	checked := []string{}
 	for _,rep := range report{
+		checked = append(checked,rep.ClusterImage.SHA256Path)
 		if err := r.podService.LabelPods(&rep); err != nil{
 			log.Error(err,"failed to label pod ")
-			return reconcile.Result{RequeueAfter: requeAfterFourHours}, nil
+			return reconcile.Result{}, nil
 		}
 	}
+	d.Annotations[domain.HeimdallImagesChecked] = strings.Join(checked, ",")
+	if err := r.client.Update(ctx, d); err != nil{
+		log.Error(err,"failed to annotate deployment " + d.Namespace + " "+d.Name)
+		return reconcile.Result{}, nil
+	}
 	return reconcile.Result{RequeueAfter:requeAfterFourHours}, nil
 }
 
@@ -114,6 +142,7 @@ type ReconcileDeployment struct {
 	// turn into interfaces
 	reportService *Reports
 	podService          *cluster.Pods
+	imageService *cluster.ImageService
 }
 
 

pkg/controller/deployments/report.go

@@ -45,15 +45,14 @@ func (r *Reports)Generate(ns, name string)([]domain.ReportResult,  error)  {
 		}
 		deployments = append(deployments, *d)
 	}
-	var clusterImages []*domain.ClusterImage
+
 	for _, d := range deployments{
-		images,err := r.clusterImageService.FindImagesFromLabels(d.Namespace, d.Spec.Template.Labels)
+		images,err := r.GetImages(&d)
 		if err != nil{
 			// log
 			fmt.Println("error finding images", err)
 		}
-		clusterImages = append(clusterImages, images...)
-		for _, i := range clusterImages{
+		for _, i := range images{
 			if !strings.Contains(i.FullPath, "redhat") {
 				continue
 			}
@@ -76,3 +75,11 @@ func (r *Reports)Generate(ns, name string)([]domain.ReportResult,  error)  {
 	}
 	return reports, nil
 }
+
+func (r *Reports)GetImages(d *v12.Deployment)([]*domain.ClusterImage, error )  {
+	images,err := r.clusterImageService.FindImagesFromLabels(d.Namespace, d.Spec.Template.Labels)
+	if err != nil{
+		return nil, errors.Wrap(err, "failed to get images for deployment " + d.Name + " in namespace " + d.Namespace)
+	}
+	return images, nil
+}