feat: Enhance walkers to create some kind of report #1289

bxf12315 · 2024-04-18T09:03:39Z

No description provided.

dejanb

Just a few remarks on the first look. I still didn't try to run it

common/walker/Cargo.toml

deploy/compose/compose-walkers.yaml

common/walker/src/report.rs

trust-git-bot · 2024-04-19T03:53:34Z

🚀 Deployed Preview: https://trustification-spog-pr-1289-preview.surge.sh ✨

bxf12315 · 2024-04-19T09:33:27Z

retest it

dejanb

I couldn't run the compose successfully, the walkers panic with

invalid character 's' looking for beginning of value

Did you see this as well? If not, can you provide some basic instructions on how to test it?

common/walker/Cargo.toml

deploy/compose/compose.yaml

bxf12315 · 2024-04-29T13:57:30Z

I couldn't run the compose successfully, the walkers panic with
invalid character 's' looking for beginning of value
Did you see this as well? If not, can you provide some basic instructions on how to test it?

I can run TRUST_IMAGE=trust TRUST_VERSION=latest docker-compose -f compose.yaml -f compose-guac.yaml -f compose-trustification.yaml -f compose-collectors.yaml -f compose-walkers.yaml up --force-recreate.

dejanb

I ran the walker manually for now ingesting dataset1

REPORT_TYPE=SBOM REPORT_PATH=/tmp/share/reports/sbom RUST_LOG=info cargo run -p trust bombastic walker --sink http://localhost:8082 --devmode --source file:./data/ds1/sbom

Here are some comments

common/walker/src/report.rs

deploy/compose/.env

dejanb

Great work. I think things are going in the right direction. Here are some more comments for minor improvements.

common/walker/src/report.rs

deploy/k8s/8.log

deploy/k8s/charts/trustification/templates/helpers/_extra.tpl

deploy/k8s/charts/trustification/values.yaml

dejanb

This looks good to me. Just a few more minor comments

common/walker/src/report.rs

ctron

I just took a look at the Rust part. I don't feel having enough understanding of the deployment part.

common/walker/src/report.rs

ctron · 2024-05-16T14:35:12Z

common/walker/templates/report.html

+    <meta charset="UTF-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1.0">
+    <title>Walker Report</title>
+    <link rel="stylesheet" href="https://unpkg.com/@patternfly/patternfly/patternfly.css">


Sorry for overlooking this before. However, relying on CDNs like this, for a product, will cause some issues.

Assuming this is run in an air-gapped environment, this won't work. I am also not sure if this is compliant with privacy policies like GDPR.

A possible solution would be to provide an env-var (via clap) to customize this base URL. We could then add this as a static asset and configure it's location from the Helm charts.

I don't see any fix for this. I see that there as an additional ConfigMap replacing this file, injecting this value via Helm. This seems overly complex to me and duplicates the while file.

I think having a way to replace the file is good. However, as we already have a template engine in place, and to replace elements in this file, we should simply re-use that same functionality for replacing the value as well. This would allow us to get rid of all the extra code below.

My method is too complicated. I will change it.

To me it looks like there's still the link unpkg.

deploy/k8s/charts/trustification/templates/services/report/020-Configmap.yaml

deploy/k8s/charts/trustification/templates/services/report/030-PersistentVolumeClaim.yaml

deploy/k8s/charts/trustification/values.schema.json

bombastic/walker/src/lib.rs

ctron · 2024-05-21T07:28:37Z

common/walker/src/report.rs

+        log::info!("This report contains no error messages and does not require the generation of an error report");
+        return Ok(());
+    }
+    let template_file_path = env::var_os("TEMPLATE_FILE");


Is there a reason this isn't handled as a clap argument? Including adding the env attribute.

This is still using env::var_os somewhere inside the code. Why can't this be handled in the place we have the other options, using clap, using the long and env option?

ctron · 2024-05-21T07:29:01Z

bombastic/walker/src/lib.rs

+
+    /// Path of the HTML output file
+    #[arg(long, default_value = "/tmp/share/report")]
+    pub report_path: Option<String>,


Is there a reason the env attribute is missing?

That sill seems to be unresolved.

#1289 (comment)
so I do not add it in env.

I am not sure how this aligns with that comment.

There's a default value, having the type of the value Option<T> doesn't seem to make sense

It can be overridden by --report-path (which also seems to be used), for other args we allow overriding by env. I don't see any reason why we shouldn't do this in this case too.

The same seems true for the vexination walker.

ok , I am not sure my understander is correct, I updated the PR, please check agian.

You can keep the default value. Add the env marker and make it String.

common/walker/src/report.rs

ctron · 2024-05-21T07:31:37Z

common/walker/templates/report.html

+    <meta charset="UTF-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1.0">
+    <title>Walker Report</title>
+    <link rel="stylesheet" href="https://unpkg.com/@patternfly/patternfly/patternfly.css">


I don't see any fix for this. I see that there as an additional ConfigMap replacing this file, injecting this value via Helm. This seems overly complex to me and duplicates the while file.

I think having a way to replace the file is good. However, as we already have a template engine in place, and to replace elements in this file, we should simply re-use that same functionality for replacing the value as well. This would allow us to get rid of all the extra code below.

deploy/compose/.env

deploy/k8s/charts/trustification/templates/services/report/030-PersistentVolumeClaim.yaml

ctron · 2024-05-21T07:37:42Z

deploy/k8s/charts/trustification/templates/services/report/060-Deployment.yaml

+{{- $mod := dict "root" . "name" "report-server" "component" "report" "module" .Values.modules.report -}}
+
+
+{{/*---*/}}


Why is this needed?

ctron · 2024-05-21T07:38:44Z

deploy/k8s/charts/trustification/values.yaml

+  report:
+    enabled: true
+    ingress: {}
+    infrastructure: {}


The infrastructure got removed. Please remove this as well.

ctron · 2024-05-21T07:39:35Z

deploy/k8s/charts/trustification/values.yaml

+    ingress: {}
+    infrastructure: {}
+    image: {}
+    patternflyUrl: "https://unpkg.com/@patternfly/patternfly/patternfly.css"


As said before, this isn't a viable option for many reasons. I think we should provide a suitable option by default and save us some trouble during productization.

Can we define several CSS options by adding a new configuration file?

IIRC we decided on moving away from this approach and add some static CSS?

ctron · 2024-05-21T07:40:05Z

vexination/walker/src/lib.rs

+    pub report_enable: bool,
+
+    /// Path of the HTML output file
+    #[arg(long, default_value = "/tmp/share/reports")]


Please see the other comment (bombastic) on this option.

I don't see this issue being resolved.

bxf12315 · 2024-05-28T11:39:47Z

@ctron @dejanb There is a new issue, patternfly.css and patternfly.min.css's sizes are both more than 1m. but configmap's size is 1m. so I can not input CSS file into configmap. So I customized some CSS styles.[1] please check it.
The PR also deploys on [2].

[1] https://github.com/trustification/trustification/pull/1289/files#diff-c94245949b61109bee8690aaba8bddf73b52617107b05fe30621394692139ef7R7
[2] https://report-xiabai-test.apps.scale.scale.trustification.cloud/

deploy/k8s/charts/trustification/values.yaml

ctron · 2024-05-28T13:06:13Z

bombastic/walker/src/lib.rs

+
+    /// Path of the HTML output file
+    #[arg(long, default_value = "/tmp/share/report")]
+    pub report_path: Option<String>,


That sill seems to be unresolved.

bombastic/walker/src/lib.rs

ctron · 2024-05-28T13:07:40Z

bombastic/walker/src/lib.rs

+    pub report_enable: bool,
+
+    /// Define report output path
+    #[arg(long, default_value = "/tmp/share/report")]


I don't think it makes sense to have this Option<T> with a default value.

common/walker/src/report.rs

deploy/k8s/charts/trustification/templates/services/report/010-Configmap-ReportTemplate.yaml

ctron · 2024-05-28T13:11:55Z

deploy/k8s/charts/trustification/templates/services/report/060-Deployment.yaml

+          command: [ "/usr/sbin/nginx" ]
+          args: [ "-g", "daemon off;" ]
+          ports:
+            - containerPort: 8018


Why don't we use the port 8080 in this case?

I still don't see port 8080 here, but there's no explanation why this is required.

ctron · 2024-05-28T13:13:06Z

vexination/walker/src/lib.rs

+    pub report_enable: bool,
+
+    /// Path of the HTML output file
+    #[arg(long, default_value = "/tmp/share/reports")]


I don't see this issue being resolved.

ctron · 2024-05-31T06:28:01Z

deploy/k8s/charts/trustification/values.yaml

+    ingress: {}
+    infrastructure: {}
+    image: {}
+    patternflyUrl: "https://unpkg.com/@patternfly/patternfly/patternfly.css"


IIRC we decided on moving away from this approach and add some static CSS?

ctron · 2024-05-31T06:29:04Z

bombastic/walker/src/lib.rs

+    pub report_enable: bool,
+
+    /// Path of the HTML output file
+    #[arg(long, default_value = "/tmp/share/report")]


Suggested change

#[arg(long, default_value = "/tmp/share/report")]

#[arg(long, env, default_value = "/tmp/share/report")]

ctron · 2024-05-31T06:29:19Z

bombastic/walker/src/lib.rs

+
+    /// Path of the HTML output file
+    #[arg(long, default_value = "/tmp/share/report")]
+    pub report_path: Option<String>,


Suggested change

pub report_path: Option<String>,

pub report_path: String,

ctron · 2024-05-31T06:29:33Z

bombastic/walker/src/lib.rs

@@ -162,7 +170,9 @@ impl Run {
                        scanner.run(interval.into()).await?;
                    } else {
                        let (report, result) = scanner.run_once().await.split()?;
-                        handle_report(report).await?;
+                        if self.report_enable {
+                            handle_report(report, self.report_path, "Sbom".to_string()).await?;


Suggested change

handle_report(report, self.report_path, "Sbom".to_string()).await?;

handle_report(report, self.report_path, "SBOM".to_string()).await?;

ctron · 2024-05-31T06:30:47Z

common/walker/src/report.rs

+        log::info!("This report contains no error messages and does not require the generation of an error report");
+        return Ok(());
+    }
+    let template_file_path = env::var_os("TEMPLATE_FILE");


This is still using env::var_os somewhere inside the code. Why can't this be handled in the place we have the other options, using clap, using the long and env option?

ctron · 2024-05-31T06:31:27Z

common/walker/src/report.rs

+    let mut tera = Tera::default();
+    if let Some(file) = template_file_path {
+        let template_file_path = file.to_str().unwrap_or(DEFAULT_TEMPLATE_FILIE).to_string();
+        let _ = tera.add_template_files(vec![(template_file_path, Some("report.html"))]);


I don't think this error should silently be ignored.

ctron · 2024-05-31T06:32:18Z

common/walker/templates/report.html

+    <meta charset="UTF-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1.0">
+    <title>Walker Report</title>
+    <link rel="stylesheet" href="https://unpkg.com/@patternfly/patternfly/patternfly.css">


To me it looks like there's still the link unpkg.

ctron · 2024-05-31T06:32:52Z

vexination/walker/src/lib.rs

+    pub report_enable: bool,
+
+    /// Path of the HTML output file
+    #[arg(long, default_value = "/tmp/share/reports")]


Suggested change

#[arg(long, default_value = "/tmp/share/reports")]

#[arg(long, env, default_value = "/tmp/share/reports")]

ctron · 2024-05-31T06:33:01Z

vexination/walker/src/lib.rs

+
+    /// Path of the HTML output file
+    #[arg(long, default_value = "/tmp/share/reports")]
+    pub report_path: Option<String>,


Suggested change

pub report_path: Option<String>,

pub report_path: String,

ctron

Looks like the previous review went to an older version of the PR. Not sure what happened, sorry.

ctron · 2024-05-31T06:35:38Z

bombastic/walker/src/lib.rs

+
+    /// Path of the HTML output file
+    #[arg(long, default_value = "/tmp/share/report")]
+    pub report_path: Option<String>,


You can keep the default value. Add the env marker and make it String.

deploy/k8s/charts/trustification/templates/init/dataset/020-Job.yaml

deploy/k8s/charts/trustification/templates/services/bombastic/walker/030-CronJob.yaml

ctron · 2024-05-31T06:38:23Z

deploy/k8s/charts/trustification/templates/services/report/060-Deployment.yaml

+          command: [ "/usr/sbin/nginx" ]
+          args: [ "-g", "daemon off;" ]
+          ports:
+            - containerPort: 8018


I still don't see port 8080 here, but there's no explanation why this is required.

deploy/k8s/charts/trustification/templates/services/vexination/walker/030-CronJob.yaml

vexination/walker/src/lib.rs

bxf12315 · 2024-06-06T13:57:58Z

@dejanb @ctron I add basic auth on report server, please check it. Thanks

deploy/compose/container_files/nginx/.htpasswd

ctron · 2024-06-06T14:41:30Z

deploy/k8s/charts/trustification/report-config/.htpasswd

@@ -0,0 +1 @@
+admin:$apr1$XFCFfzu4$vtfox1qeBPqcZv814kltL1


The next step is to make this configurable for the person deploying the application.

bxf12315 requested review from ctron and dejanb April 18, 2024 09:03

bxf12315 force-pushed the TC-910 branch from 3ec3ab6 to dc6af7c Compare April 18, 2024 09:17

dejanb reviewed Apr 18, 2024

View reviewed changes

common/walker/Cargo.toml Show resolved Hide resolved

deploy/compose/compose-walkers.yaml Outdated Show resolved Hide resolved

common/walker/src/report.rs Outdated Show resolved Hide resolved

bxf12315 force-pushed the TC-910 branch 3 times, most recently from d36147b to 30ce891 Compare April 19, 2024 03:50

bxf12315 force-pushed the TC-910 branch from 8fdf6f1 to e65eef4 Compare April 19, 2024 08:03

dejanb requested changes Apr 29, 2024

View reviewed changes

common/walker/Cargo.toml Show resolved Hide resolved

deploy/compose/compose.yaml Outdated Show resolved Hide resolved

deploy/compose/compose.yaml Outdated Show resolved Hide resolved

dejanb requested changes May 8, 2024

View reviewed changes

common/walker/src/report.rs Outdated Show resolved Hide resolved

common/walker/src/report.rs Outdated Show resolved Hide resolved

deploy/compose/.env Outdated Show resolved Hide resolved

deploy/compose/.env Outdated Show resolved Hide resolved

dejanb reviewed May 10, 2024

View reviewed changes

bxf12315 force-pushed the TC-910 branch from 404f00d to 75e880d Compare May 10, 2024 13:24

bxf12315 marked this pull request as ready for review May 10, 2024 13:24

bxf12315 force-pushed the TC-910 branch from 75e880d to b642d2c Compare May 10, 2024 13:32

dejanb reviewed May 13, 2024

View reviewed changes

common/walker/src/report.rs Outdated Show resolved Hide resolved

common/walker/src/report.rs Outdated Show resolved Hide resolved

ctron reviewed May 15, 2024

View reviewed changes

common/walker/src/report.rs Show resolved Hide resolved

bxf12315 force-pushed the TC-910 branch from cb09abe to 4cd927f Compare May 16, 2024 06:43

ctron requested changes May 16, 2024

View reviewed changes

common/walker/src/report.rs Outdated Show resolved Hide resolved

bxf12315 force-pushed the TC-910 branch 3 times, most recently from 02ea0fc to 015638d Compare May 16, 2024 14:23

ctron reviewed May 16, 2024

View reviewed changes

common/walker/src/report.rs Outdated Show resolved Hide resolved

ctron requested changes May 16, 2024

View reviewed changes

bxf12315 force-pushed the TC-910 branch 3 times, most recently from eca6bf8 to 7435cbf Compare May 19, 2024 09:58

bxf12315 force-pushed the TC-910 branch from 7435cbf to 4efc329 Compare May 19, 2024 10:17

ctron requested changes May 21, 2024

View reviewed changes

bxf12315 force-pushed the TC-910 branch from 18ed1a8 to 06f05d4 Compare May 28, 2024 11:35

bxf12315 force-pushed the TC-910 branch from 06f05d4 to 36c9fca Compare May 28, 2024 11:44

ctron requested changes May 28, 2024

View reviewed changes

bxf12315 force-pushed the TC-910 branch 3 times, most recently from 499ab23 to 5d6ecaf Compare May 30, 2024 10:41

ctron requested changes May 31, 2024

View reviewed changes

bxf12315 added 11 commits June 6, 2024 21:59

feat: Enhance walkers to create some kind of report

c151000

chore: Adjust report page's styles

9bf511c

chore: Move nginx to a specific file.

2c42c99

feat: new chart of report server

2a45cb6

chore: generate a report in JSON format

008175f

chore: new report flag for bombastic and vexination walker

12c7004

chore: Remove the feature for generating reports in JSON format.

33fb481

chore: fix issues from comment

e7a50ce

chore: new flag for report generate

4d5f998

chore: Adjust CSS import strategy

93df238

chore: basic auth of nginx on report server

3b69352

bxf12315 force-pushed the TC-910 branch from 582a89e to 3b69352 Compare June 6, 2024 14:01

ctron requested changes Jun 6, 2024

View reviewed changes

deploy/compose/container_files/nginx/.htpasswd Show resolved Hide resolved

ctron reviewed Jun 6, 2024

View reviewed changes

		{{- $mod := dict "root" . "name" "report-server" "component" "report" "module" .Values.modules.report -}}


		{{/---/}}

	#[arg(long, default_value = "/tmp/share/report")]
	#[arg(long, env, default_value = "/tmp/share/report")]

	handle_report(report, self.report_path, "Sbom".to_string()).await?;
	handle_report(report, self.report_path, "SBOM".to_string()).await?;

		@@ -0,0 +1 @@
		admin:$apr1$XFCFfzu4$vtfox1qeBPqcZv814kltL1

feat: Enhance walkers to create some kind of report #1289

Are you sure you want to change the base?

feat: Enhance walkers to create some kind of report #1289

Conversation

bxf12315 commented Apr 18, 2024

dejanb left a comment

Choose a reason for hiding this comment

trust-git-bot commented Apr 19, 2024 • edited

bxf12315 commented Apr 19, 2024

dejanb left a comment

Choose a reason for hiding this comment

bxf12315 commented Apr 29, 2024

dejanb left a comment

Choose a reason for hiding this comment

dejanb left a comment

Choose a reason for hiding this comment

dejanb left a comment

Choose a reason for hiding this comment

ctron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bxf12315 commented May 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bxf12315 commented Jun 6, 2024

Choose a reason for hiding this comment

trust-git-bot commented Apr 19, 2024 •

edited