fix: Remove job deletion for jobs with TTLSecondsAfterFinished set

[tom1299]

Description

Currently jobs are directly deleted after they are completed by the trivy-operator without honouring the jobs ttlSecondsAfterFinished and thus also the helm configuration parameter operator.scanJobTTL.
The change introduced in this PR will only delete the job if it has no ttlSecondsAfterFinished.

Related issues

• Close Vulnerability ScanJob is immediately deleted #2362

Checklist

• I've read the guidelines for contributing to this repository.
• I've added tests that prove my fix is effective or that my feature works.
• I've updated the documentation with the relevant information (if needed).
• I've added usage information (if the PR introduces new options)
• I've included a "before" and "after" example to the description (if the PR is a user interface change).

[simar7]

I read through the docs but it wasn't clear to me why we should check for both !=nil and !=0. Could you explain that?

[tom1299]

Actually now that you mentioned it, it seems to me that the !=0 is superfluous / not really necessary. I would remove this from the if statement.

[simar7]

Could you update your PR?

[tom1299]

@simar7 I'm sorry but somehow I totally forgot to do the update. I made the change and squashed the commits so that the commit history looks better.

[simar7]

@afdesk can you take another look?

[afdesk]

looks good!

@tom1299 thanks for your contribution

[afdesk]

@simar7 will merge it, right?

[simar7]

@tom1299 is it possible to write a test for this case? While it is a trivial change, it is an important one. Maybe we can use this test setup?

trivy-operator/tests/envtest/controller_test.go

Lines 200 to 226 in 2ed26a2

	DescribeTable("On ttl reconcile loop",
	func(report client.Object, reportFile string, cm client.Object, cmFile string) {
	Expect(loadResource(report, path.Join(testdataResourceDir, reportFile))).Should(Succeed())
	if report.GetNamespace() != kubeSystemNamespace {
	report.SetNamespace(WorkloadNamespace)
	}
	if cm != nil {
	Expect(loadResource(cm, path.Join(testdataResourceDir, cmFile))).Should(Succeed())
	if cm.GetNamespace() != kubeSystemNamespace {
	cm.SetNamespace(WorkloadNamespace)
	}
	Expect(k8sClient.Create(ctx, cm)).Should(Succeed())
	}
	Expect(k8sClient.Create(ctx, report)).Should(Succeed())
	caLookupKey := client.ObjectKeyFromObject(report)
	createdVulnerabilityReport := &v1alpha1.VulnerabilityReport{}
	time.Sleep(2 * time.Second)
	// We'll need to retry getting this newly created Job, given that creation may not immediately happen.
	Eventually(func() error {
	return k8sClient.Get(ctx, caLookupKey, createdVulnerabilityReport)
	}, timeout, interval).ShouldNot(Succeed())
	},
	Entry("Should delete vulnerability report", &v1alpha1.VulnerabilityReport{}, "vulnerability-ttl.yaml", nil, ""),
	Entry("Should delete config audit report", &v1alpha1.ConfigAuditReport{}, "config-audit-ttl-historical.yaml", nil, ""),
	Entry("Should delete config audit report", &v1alpha1.ConfigAuditReport{}, "config-audit-ttl.yaml", &corev1.ConfigMap{}, "policy.yaml"),
	)

WDYT?

[tom1299]

@simar7 Adding a test case like above is a very good idea. I will have a deeper look into the env tests and try to add a test case for the ttl jobs.

[tom1299]

@simar7 So I had a look at the env tests. And the main issue for adding a test case to the env tests is that the ScanJobController can not be easily added to the env test setup here. Since the ScanJobController contains the method to test, it will never be executed.
I also think that even if it would be possible to add the controller, the JobHasAnyCondition Predicate would prevent the Controller from processing the job since the Jobs created in the env tests do not seem to have a status.
Here is the predicate code:

var JobHasAnyCondition = predicate.NewPredicateFuncs(func(obj client.Object) bool {
	if job, ok := obj.(*batchv1.Job); ok {
		return len(job.Status.Conditions) > 0
	}
	return false
})

And it is registered with ScanJobController here:

func (r *ScanJobController) SetupWithManager(mgr ctrl.Manager) error {
	var predicates []predicate.Predicate
	if !r.ConfigData.VulnerabilityScanJobsInSameNamespace() {
		predicates = append(predicates, InNamespace(r.Config.Namespace))
	}
	predicates = append(predicates, ManagedByTrivyOperator, IsVulnerabilityReportScan, JobHasAnyCondition)
	return ctrl.NewControllerManagedBy(mgr).
		For(&batchv1.Job{}, builder.WithPredicates(predicates...)).
		Complete(r.reconcileJobs())
}

By the way, there seems to be a redundant implementation in the ScanJobController itself here:

if len(job.Status.Conditions) == 0 {
	log.V(1).Info("Ignoring Job without conditions")
	return ctrl.Result{}, nil
}

Which means that the when the Predicate would be removed, the ScanJobController would still not process the job. For me this might also be an issue that needs to be fixed.

So I think it would be rather hard to test the changes in the env tests. Or maybe you could give me another hint how to add a test ? Otherwise I can suggest looking into the integration tests. Maybe we could add a test case here ?

[tom1299]

As an alternative I added a integration test here 932e1a8. the tests validates that if ttl is set for scan jobs, they are not deleted immediately. What do you think about that ? Would that be a way to to the testing ?