From 3159a4aeb7926b16a02702f59d4b6abe6f9be4ce Mon Sep 17 00:00:00 2001 From: Jannik Hollenbach <13718901+J12934@users.noreply.github.com> Date: Mon, 22 Jun 2020 12:39:45 +0200 Subject: [PATCH 1/5] #21 Use OwnerReferences and labels to find jobs Names are not totally reliable to find the jobs anymore when we truncate them. --- .../controllers/execution/scan_controller.go | 123 ++++++++---------- 1 file changed, 54 insertions(+), 69 deletions(-) diff --git a/operator/controllers/execution/scan_controller.go b/operator/controllers/execution/scan_controller.go index bf517ea9..0304f00b 100644 --- a/operator/controllers/execution/scan_controller.go +++ b/operator/controllers/execution/scan_controller.go @@ -116,7 +116,6 @@ func (r *ScanReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { err = r.setHookStatus(&scan) case "ReadAndWriteHookProcessing": err = r.executeReadAndWriteHooks(&scan) - case "ReadAndWriteHookCompleted": err = r.startReadOnlyHooks(&scan) case "ReadOnlyHookProcessing": @@ -129,21 +128,6 @@ func (r *ScanReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { return ctrl.Result{}, nil } -func (r *ScanReconciler) getJob(name, namespace string) (*batch.Job, error) { - ctx := context.Background() - - var job batch.Job - err := r.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, &job) - if apierrors.IsNotFound(err) { - return nil, nil - } else if err != nil { - r.Log.Error(err, "unable to get job") - return nil, err - } - - return &job, nil -} - type jobCompletionType string const ( @@ -153,22 +137,51 @@ const ( unknown jobCompletionType = "Unknown" ) -func (r *ScanReconciler) checkIfJobIsCompleted(name, namespace string) (jobCompletionType, error) { - job, err := r.getJob(name, namespace) - if err != nil { - return unknown, err +func allJobsCompleted(jobs *batch.JobList) jobCompletionType { + hasCompleted := true + + for _, job := range jobs.Items { + if job.Status.Failed > 0 { + return failed + } else if job.Status.Succeeded == 0 { + hasCompleted = false + } } - if job == nil { - return unknown, errors.New("Both Job and error were nil. This isn't really expected") + + if hasCompleted { + return completed } + return incomplete +} + +func (r *ScanReconciler) getJobsForScan(scan *executionv1.Scan, labels client.MatchingLabels) (*batch.JobList, error) { + ctx := context.Background() - if job.Status.Succeeded != 0 { - return completed, nil + // check if k8s job for scan was already created + var jobs batch.JobList + if err := r.List( + ctx, + &jobs, + client.InNamespace(scan.Namespace), + client.MatchingField(ownerKey, scan.Name), + labels, + ); err != nil { + r.Log.Error(err, "Unable to list child jobs") + return nil, err } - if job.Status.Failed != 0 { - return failed, nil + + return &jobs, nil +} + +func (r *ScanReconciler) checkIfJobIsCompleted(scan *executionv1.Scan, labels client.MatchingLabels) (jobCompletionType, error) { + jobs, err := r.getJobsForScan(scan, labels) + if err != nil { + return unknown, err } - return unknown, nil + + r.Log.V(9).Info("Got related jobs", "count", len(jobs.Items)) + + return allJobsCompleted(jobs), nil } // Helper functions to check and remove string from a slice of strings. @@ -220,11 +233,11 @@ func (r *ScanReconciler) startScan(scan *executionv1.Scan) error { namespacedName := fmt.Sprintf("%s/%s", scan.Namespace, scan.Name) log := r.Log.WithValues("scan_init", namespacedName) - job, err := r.getJob(fmt.Sprintf("scan-%s", scan.Name), scan.Namespace) + jobs, err := r.getJobsForScan(scan, client.MatchingLabels{"experimental.securecodebox.io/job-type": "scanner"}) if err != nil { return err } - if job != nil { + if len(jobs.Items) > 0 { log.V(8).Info("Job already exists. Doesn't need to be created.") return nil } @@ -267,7 +280,7 @@ func (r *ScanReconciler) startScan(scan *executionv1.Scan) error { rules, ) - job, err = r.constructJobForScan(scan, &scanType) + job, err := r.constructJobForScan(scan, &scanType) if err != nil { log.Error(err, "unable to create job object ScanType") return err @@ -296,7 +309,7 @@ func (r *ScanReconciler) startScan(scan *executionv1.Scan) error { func (r *ScanReconciler) checkIfScanIsCompleted(scan *executionv1.Scan) error { ctx := context.Background() - status, err := r.checkIfJobIsCompleted(fmt.Sprintf("scan-%s", scan.Name), scan.Namespace) + status, err := r.checkIfJobIsCompleted(scan, client.MatchingLabels{"experimental.securecodebox.io/job-type": "scanner"}) if err != nil { return err } @@ -326,11 +339,11 @@ func (r *ScanReconciler) startParser(scan *executionv1.Scan) error { namespacedName := fmt.Sprintf("%s/%s", scan.Namespace, scan.Name) log := r.Log.WithValues("scan_parse", namespacedName) - job, err := r.getJob(fmt.Sprintf("parse-%s", scan.Name), scan.Namespace) + jobs, err := r.getJobsForScan(scan, client.MatchingLabels{"experimental.securecodebox.io/job-type": "parser"}) if err != nil { return err } - if job != nil { + if len(jobs.Items) > 0 { log.V(8).Info("Job already exists. Doesn't need to be created.") return nil } @@ -384,7 +397,7 @@ func (r *ScanReconciler) startParser(scan *executionv1.Scan) error { labels["experimental.securecodebox.io/job-type"] = "parser" automountServiceAccountToken := true var backOffLimit int32 = 3 - job = &batch.Job{ + job := &batch.Job{ ObjectMeta: metav1.ObjectMeta{ Annotations: make(map[string]string), Name: fmt.Sprintf("parse-%s", scan.Name), @@ -459,7 +472,7 @@ func (r *ScanReconciler) startParser(scan *executionv1.Scan) error { func (r *ScanReconciler) checkIfParsingIsCompleted(scan *executionv1.Scan) error { ctx := context.Background() - status, err := r.checkIfJobIsCompleted(fmt.Sprintf("parse-%s", scan.Name), scan.Namespace) + status, err := r.checkIfJobIsCompleted(scan, client.MatchingLabels{"experimental.securecodebox.io/job-type": "parser"}) if err != nil { return err } @@ -729,44 +742,13 @@ func (r *ScanReconciler) startReadOnlyHooks(scan *executionv1.Scan) error { return nil } -func allJobsCompleted(jobs *batch.JobList) jobCompletionType { - hasCompleted := true - - for _, job := range jobs.Items { - if job.Status.Failed > 0 { - return failed - } else if job.Status.Succeeded == 0 { - hasCompleted = false - } - } - - if hasCompleted { - return completed - } - return incomplete -} - func (r *ScanReconciler) checkIfReadOnlyHookIsCompleted(scan *executionv1.Scan) error { ctx := context.Background() - - // check if k8s job for scan was already created - var readOnlyHookJobs batch.JobList - if err := r.List( - ctx, - &readOnlyHookJobs, - client.InNamespace(scan.Namespace), - client.MatchingField(ownerKey, scan.Name), - client.MatchingLabels{ - "experimental.securecodebox.io/job-type": "read-only-hook", - }, - ); err != nil { - r.Log.Error(err, "Unable to list child jobs") + readOnlyHookCompletion, err := r.checkIfJobIsCompleted(scan, client.MatchingLabels{"experimental.securecodebox.io/job-type": "read-only-hook"}) + if err != nil { return err } - r.Log.V(9).Info("Got related jobs", "count", len(readOnlyHookJobs.Items)) - - readOnlyHookCompletion := allJobsCompleted(&readOnlyHookJobs) if readOnlyHookCompletion == completed { r.Log.V(7).Info("All ReadOnlyHooks have completed") scan.Status.State = "Done" @@ -1143,7 +1125,10 @@ func (r *ScanReconciler) executeReadAndWriteHooks(scan *executionv1.Scan) error }) return err case executionv1.InProgress: - jobStatus, err := r.checkIfJobIsCompleted(nonCompletedHook.JobName, scan.Namespace) + jobStatus, err := r.checkIfJobIsCompleted(scan, client.MatchingLabels{ + "experimental.securecodebox.io/job-type": "read-and-write-hook", + "experimental.securecodebox.io/hook-name": nonCompletedHook.HookName, + }) if err != nil { r.Log.Error(err, "Failed to check job status for ReadAndWrite Hook") return err From f384d043430e4b875429f517b0ad2a0a42fd1a81 Mon Sep 17 00:00:00 2001 From: Jannik Hollenbach <13718901+J12934@users.noreply.github.com> Date: Mon, 22 Jun 2020 13:27:53 +0200 Subject: [PATCH 2/5] #21 Use GeneratedName and trucate names over 58 chars This lets us add a `-` and 5 random chars to ensure unquire names. --- .../controllers/execution/scan_controller.go | 25 +++++++------ operator/utils/truncatedname.go | 11 ++++++ operator/utils/truncatedname_test.go | 35 +++++++++++++++++++ 3 files changed, 60 insertions(+), 11 deletions(-) create mode 100644 operator/utils/truncatedname.go create mode 100644 operator/utils/truncatedname_test.go diff --git a/operator/controllers/execution/scan_controller.go b/operator/controllers/execution/scan_controller.go index 0304f00b..e90c0a49 100644 --- a/operator/controllers/execution/scan_controller.go +++ b/operator/controllers/execution/scan_controller.go @@ -39,6 +39,7 @@ import ( "github.com/minio/minio-go/v6" executionv1 "github.com/secureCodeBox/secureCodeBox-v2-alpha/operator/apis/execution/v1" + util "github.com/secureCodeBox/secureCodeBox-v2-alpha/operator/utils" ) // ScanReconciler reconciles a Scan object @@ -399,10 +400,10 @@ func (r *ScanReconciler) startParser(scan *executionv1.Scan) error { var backOffLimit int32 = 3 job := &batch.Job{ ObjectMeta: metav1.ObjectMeta{ - Annotations: make(map[string]string), - Name: fmt.Sprintf("parse-%s", scan.Name), - Namespace: scan.Namespace, - Labels: labels, + Annotations: make(map[string]string), + GenerateName: util.TruncateName(fmt.Sprintf("parse-%s", scan.Name)), + Namespace: scan.Namespace, + Labels: labels, }, Spec: batch.JobSpec{ BackoffLimit: &backOffLimit, @@ -516,9 +517,9 @@ func (r *ScanReconciler) constructJobForScan(scan *executionv1.Scan, scanType *e labels["experimental.securecodebox.io/job-type"] = "scanner" job := &batch.Job{ ObjectMeta: metav1.ObjectMeta{ - Labels: labels, - Name: fmt.Sprintf("scan-%s", scan.Name), - Namespace: scan.Namespace, + Labels: labels, + GenerateName: util.TruncateName(fmt.Sprintf("scan-%s", scan.Name)), + Namespace: scan.Namespace, }, Spec: *scanType.Spec.JobTemplate.Spec.DeepCopy(), } @@ -1000,13 +1001,15 @@ func (r *ScanReconciler) createJobForHook(hook *executionv1.ScanCompletionHook, } else if hook.Spec.Type == executionv1.ReadOnly { labels["experimental.securecodebox.io/job-type"] = "read-only-hook" } + labels["experimental.securecodebox.io/hook-name"] = hook.Name + var backOffLimit int32 = 3 job := &batch.Job{ ObjectMeta: metav1.ObjectMeta{ - Annotations: make(map[string]string), - Name: fmt.Sprintf("%s-%s", hook.Name, scan.Name), - Namespace: scan.Namespace, - Labels: labels, + Annotations: make(map[string]string), + GenerateName: util.TruncateName(fmt.Sprintf("%s-%s", hook.Name, scan.Name)), + Namespace: scan.Namespace, + Labels: labels, }, Spec: batch.JobSpec{ BackoffLimit: &backOffLimit, diff --git a/operator/utils/truncatedname.go b/operator/utils/truncatedname.go new file mode 100644 index 00000000..21e215a9 --- /dev/null +++ b/operator/utils/truncatedname.go @@ -0,0 +1,11 @@ +package utils + +import "fmt" + +// TruncateName Ensures that the name used for a kubernetes object doesn't exceed the 63 char length limit. This actually cuts of anything after char 57, so that we can use the randomly generated suffix from k8s `generateName`. +func TruncateName(name string) string { + if len(name) >= 57 { + return fmt.Sprintf("%s-", name[0:57]) + } + return fmt.Sprintf("%s-", name) +} diff --git a/operator/utils/truncatedname_test.go b/operator/utils/truncatedname_test.go new file mode 100644 index 00000000..a0e68b70 --- /dev/null +++ b/operator/utils/truncatedname_test.go @@ -0,0 +1,35 @@ +package utils + +import ( + "fmt" + "testing" +) + +type testData struct { + in string + out string +} + +func TestAbc(t *testing.T) { + var tests = []testData{ + { + in: "abc", + out: "abc-", + }, + { + in: "scan-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + out: "scan-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-", + }, + { + in: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + out: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-", + }, + } + + for _, test := range tests { + actual := TruncateName(test.in) + if actual != test.out { + t.Error(fmt.Errorf("TruncateName(\"%s\") returned \"%s\", expected \"%s\"", test.in, actual, test.out)) + } + } +} From 8307fd9b82ffbd91339b36b2f094659177a1829a Mon Sep 17 00:00:00 2001 From: Jannik Hollenbach <13718901+J12934@users.noreply.github.com> Date: Mon, 22 Jun 2020 13:34:33 +0200 Subject: [PATCH 3/5] # 21 Autoformat helpers.js --- tests/integration/helpers.js | 47 ++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/tests/integration/helpers.js b/tests/integration/helpers.js index 74db064b..ea749029 100644 --- a/tests/integration/helpers.js +++ b/tests/integration/helpers.js @@ -8,8 +8,8 @@ const k8sBatchApi = kc.makeApiClient(k8s.BatchV1Api); const namespace = "integration-tests"; -const sleep = duration => - new Promise(resolve => setTimeout(resolve, duration * 1000)); +const sleep = (duration) => + new Promise((resolve) => setTimeout(resolve, duration * 1000)); async function deleteScan(name) { await k8sCRDApi.deleteNamespacedCustomObject( @@ -30,21 +30,18 @@ async function getScan(name) { "scans", name ); - return scan + return scan; } async function logJob(name) { try { - const { body: job } = await k8sBatchApi.readNamespacedJob( - name, - namespace, - ); - console.log(`Job: '${name}' Spec:`) - console.dir(job.spec) - console.log(`Job: '${name}' Status:`) - console.dir(job.status) + const { body: job } = await k8sBatchApi.readNamespacedJob(name, namespace); + console.log(`Job: '${name}' Spec:`); + console.dir(job.spec); + console.log(`Job: '${name}' Status:`); + console.dir(job.status); } catch (error) { - console.info(`Job: '${name} not found.'`) + console.info(`Job: '${name} not found.'`); } } @@ -61,12 +58,12 @@ async function scan(name, scanType, parameters = [], timeout = 180) { kind: "Scan", metadata: { // Use `generateName` instead of name to generate a random sufix and avoid name clashes - generateName: `${name}-` + generateName: `${name}-`, }, spec: { scanType, - parameters - } + parameters, + }, }; const { body } = await k8sCRDApi.createNamespacedCustomObject( @@ -82,25 +79,27 @@ async function scan(name, scanType, parameters = [], timeout = 180) { for (let i = 0; i < timeout; i++) { await sleep(1); const { status } = await getScan(actualName); - + if (status && status.state === "Done") { await deleteScan(actualName); return status.findings; } else if (status && status.state === "Errored") { await deleteScan(actualName); - throw new Error(`Scan failed with description "${status.errorDescription}"`) + throw new Error( + `Scan failed with description "${status.errorDescription}"` + ); } } - - console.error("Scan Timed out!") + + console.error("Scan Timed out!"); const scan = await getScan(actualName); - console.log("Last Scan State:") - console.dir(scan) - await logJob(`scan-${actualName}`) - await logJob(`parse-${actualName}`) + console.log("Last Scan State:"); + console.dir(scan); + await logJob(`scan-${actualName}`); + await logJob(`parse-${actualName}`); throw new Error("timed out while waiting for scan results"); } -module.exports.scan = scan; \ No newline at end of file +module.exports.scan = scan; From 5e77567ba8058350b94a55ccf3c1098763380e0d Mon Sep 17 00:00:00 2001 From: Jannik Hollenbach <13718901+J12934@users.noreply.github.com> Date: Mon, 22 Jun 2020 13:36:37 +0200 Subject: [PATCH 4/5] #21 Log all jobs in namespace Getting individual jobs is harder now, as the name has a random string at the end. Listing all jobs is more helpfull anyway, as it also lists jobs for hooks --- tests/integration/helpers.js | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tests/integration/helpers.js b/tests/integration/helpers.js index ea749029..28308a26 100644 --- a/tests/integration/helpers.js +++ b/tests/integration/helpers.js @@ -33,15 +33,18 @@ async function getScan(name) { return scan; } -async function logJob(name) { +async function logJobs() { try { - const { body: job } = await k8sBatchApi.readNamespacedJob(name, namespace); - console.log(`Job: '${name}' Spec:`); - console.dir(job.spec); - console.log(`Job: '${name}' Status:`); - console.dir(job.status); + const { body: jobs } = await k8sBatchApi.listNamespacedJob(namespace); + + for (const job of jobs.items) { + console.log(`Job: '${job.metadata.name}' Spec:`); + console.dir(job.spec); + console.log(`Job: '${job.metadata.name}' Status:`); + console.dir(job.status); + } } catch (error) { - console.info(`Job: '${name} not found.'`); + console.info(`Failed to list Jobs'`); } } @@ -96,8 +99,7 @@ async function scan(name, scanType, parameters = [], timeout = 180) { const scan = await getScan(actualName); console.log("Last Scan State:"); console.dir(scan); - await logJob(`scan-${actualName}`); - await logJob(`parse-${actualName}`); + await logJobs(); throw new Error("timed out while waiting for scan results"); } From b494d587558df22c47a12f61510c261f48301071 Mon Sep 17 00:00:00 2001 From: Jannik Hollenbach <13718901+J12934@users.noreply.github.com> Date: Mon, 22 Jun 2020 13:38:28 +0200 Subject: [PATCH 5/5] #21 Copy utils directory to operator image build --- operator/Dockerfile | 1 + 1 file changed, 1 insertion(+) diff --git a/operator/Dockerfile b/operator/Dockerfile index a765127f..42586593 100644 --- a/operator/Dockerfile +++ b/operator/Dockerfile @@ -13,6 +13,7 @@ RUN go mod download COPY main.go main.go COPY apis/ apis/ COPY controllers/ controllers/ +COPY utils/ utils/ # Build RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 GO111MODULE=on go build -a -o manager main.go