ROX-12238: Add node analysis package #911

jvdm · 2022-09-05T23:17:31Z

This PR is built on top of #904 (please filter commits in the diff for a narrowed view of the changes).

Add Analyze() to pkg/analyzer/nodes, including related changes to allow external packages (in particular, Compliance) to consume it. node.Analyze is built on the recently introduced interfaces added to pkg/analyzer, and re-uses clair.DetectContentFromFile().

Other marginal changes:

Move file size limits to the analyzer package.
Add metrics for node scanning.

Tests

UTs.

Local validation, here's the code (omitting imports):

func main() {
	components, err := nodes.Analyze("localhost", "/", true)
	if err != nil {
		panic(err)
	}

	s, err := json.MarshalIndent(components, "", "    ")
	if err != nil {
		panic(err)
	}
	fmt.Println(string(s))
}

Next steps: E2E.

ghost · 2022-09-06T06:49:35Z

Images are ready for the commit at 0ab82fb.

To use the images, use the tag 2.25.0-87-g0ab82fb88b.

c-du · 2022-09-07T07:33:57Z

This PR is built on top of https://github.com/stackrox/scanner/pull/904 (please filter commits in the diff for a narrowed view of the changes).

You can generate the diff by changing the merge into branch from master to 'jvdm/ROX-12238/1/pull' in this case.

pkg/analyzer/nodes/node.go

c-du · 2022-09-07T16:08:50Z

pkg/analyzer/nodes/node.go

+	matcher := requiredfilenames.SingletonOSMatcher()
+	files, err := extractFilesFromDirectory(rootFSdir, matcher)
+	if err != nil {
+		return Components{}, err


I am curious in case of error, why not returning (nil, err)?

No reason. Happy to change if there is an exact concern on returning an empty struct.

In that case, it is recommended to use nil.

Could you please share a reference of why this is the recommendation?

I've found https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices but that is for slices, which are treated as empty when nil. Could not find something specific for structs in general.

pkg/analyzer/nodes/node.go

c-du · 2022-09-07T16:10:32Z

pkg/analyzer/nodes/node.go

+	if err != nil {
+		return Components{}, nil
+	}
+	// File reading errors during analysis failed are not exposed to DetectFiles,


nit: ... during failed analysis...

c-du · 2022-09-07T16:16:20Z

pkg/metrics/extractfiles.go

+	fileExtractionErrorCountMetric = prometheus.NewHistogram(prometheus.HistogramOpts{
+		Name:    "file_extraction_error_count",
+		Help:    "Number of files in an node filesystem scan that failed to read",
+		Buckets: []float64{50, 100, 500, 1000},


Is 50 too small as error count histogram?

Same as above.

c-du · 2022-09-07T17:58:52Z

pkg/analyzer/nodes/node.go

+	return filepath.WalkDir(filepath.Join(n.root, dir), func(fullPath string, entry fs.DirEntry, err error) error {
+		if err != nil {
+			if filesIsNotAccessible(err) {
+				m.ErrorCount()


The errorcount is only for filesIsNotAccessible error? If so, is it good to specify it in the context of the error for the reader of the metrics?

Not sure if I follow. Are you suggesting we should modify the metric to make it more clear that it is tracking only inaccessible files, or something entirely different?

Notice that on every other error we fail the scan entirely.

The metrics by themselves suggest that it is the error count. Even if we fail the scan, the metrics readers are not in the context of the scan. You could either count all the errors or make it clear with either name or help of the metrics.

👍 Thanks for clarifying.

pkg/analyzer/nodes/node.go

c-du · 2022-09-07T18:00:40Z

pkg/analyzer/nodes/node.go

+	}, nil
+}
+
+// Get implements analyzer.Files


Can you explain more the function of Get function?

The details are explained in the documentation of analyzer.Files. If you have a better way of linking them together let me know.

jvdm · 2022-09-08T05:42:26Z

This PR is built on top of #904 (please filter commits in the diff for a narrowed view of the changes).

You can generate the diff by changing the merge into branch from master to 'jvdm/ROX-12238/1/pull' in this case.

If I target the PR into jvdm/ROX-12238/1/pull I won't trigger CI checks. Also, once 1/pull is merged, its commit will change (squash or rebased), and the target of this PR becomes obsolete.

jvdm · 2022-09-08T22:16:54Z

/retest

openshift-ci · 2022-09-08T22:40:27Z

@jvdm: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-tests	`0ab82fb`	link	false	`/test e2e-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

RTann · 2022-09-09T00:44:12Z

worker.go

+// the files of a filesystem or image layer. For layers, the parent layer should
+// be specified. For filesystems, which don't have the concept of intermediate
+// layers, or the root layer, use `nil`. Notice that language components are not
+// extracted by DetectFromFiles, but if provided they are annotated with


if provided they are annotated with certified RHEL dependencies

Not sure I follow

Language components are not detected by this function, rather they are detected by the analyzers embedded in the matcher. But we pass components here so that the certified RHEL path will annotate the components with package info.

Personally, I would leave that piece out of the detector, arguing that it should be its responsibility to annotate language components. But OK the way it is.

RTann · 2022-09-09T00:47:09Z

pkg/metrics/extractfiles.go

+		fileExtractionMatchCountMetric.Observe(m.fileCount)
+		fileExtractionInaccessibleCountMetric.Observe(m.inaccessibleCount)
+	}
+	*m = FileExtractionMetrics{}


if m == nil, then this will segfault

RTann · 2022-09-09T21:09:21Z

pkg/analyzer/nodes/node.go

+
+// Information about extracted files.
+type fileMetadata struct {
+	// If true, the file has executable permissions.


Is this also a regular file?

RTann · 2022-09-09T21:11:09Z

pkg/analyzer/nodes/node.go

+// will stop returning entries and the error will be available at readErr().
+// Analyzer are expected to handle missing files gracefully during analysis and
+// check readErr() to confirm all potential data was read.
+type filesMap struct {


can we add a line

var _ analyzer.Files = (*filesMap)(nil)

See https://github.com/uber-go/guide/blob/master/style.md#verify-interface-compliance

RTann · 2022-09-09T21:19:20Z

pkg/analyzer/nodes/node.go

+
+// readErr returns the first error encountered when reading file content, and
+// resets the error, enabling re-scans using the same map.
+func (n *filesMap) readErr() error {


who calls this?

Just after clair.DetectFromFiles() (now clair.DetectComponents()), to verify if there was any error when reading files during detection.

RTann · 2022-09-09T21:27:28Z

pkg/analyzer/nodes/node_test.go

+			wantErr:      assert.NoError,
+		},
+		{
+			name:        "when file is extractable and size is bigger then the limit then return nothing",


nit: than instead of then

RTann · 2022-09-09T21:29:05Z

pkg/analyzer/nodes/node_test.go

+			wantExists: true,
+		},
+		{
+			name: "when file is in map and is extractable but does not exist, then keep error",


how does this happen?

Between extraction and reading the file might have moved, was deleted, changed permission or something else happened to the underlying filesystem (e.g. failures).

RTann · 2022-09-09T21:30:49Z

pkg/analyzer/nodes/node.go

+	if err != nil {
+		return nil, nil
+	}
+	// File reading errors during analysis are not exposed to DetectFiles, hence we


nit: DetectFromFiles

jvdm force-pushed the jvdm/ROX-12238/2/pull branch from a506b38 to a5de780 Compare September 6, 2022 06:10

stackrox deleted a comment from openshift-ci bot Sep 6, 2022

jvdm requested review from daynewlee, c-du and RTann September 6, 2022 17:35

c-du changed the base branch from master to jvdm/ROX-12238/1/pull September 7, 2022 07:41

c-du reviewed Sep 7, 2022

View reviewed changes

jvdm force-pushed the jvdm/ROX-12238/1/pull branch 2 times, most recently from 41b9775 to c257e53 Compare September 7, 2022 21:58

jvdm changed the base branch from jvdm/ROX-12238/1/pull to master September 8, 2022 01:17

jvdm changed the base branch from master to jvdm/ROX-12238/1/pull September 8, 2022 05:01

jvdm force-pushed the jvdm/ROX-12238/2/pull branch from a5de780 to 1354404 Compare September 8, 2022 05:37

jvdm changed the base branch from jvdm/ROX-12238/1/pull to master September 8, 2022 05:39

jvdm requested a review from c-du September 8, 2022 05:49

jvdm added 3 commits September 8, 2022 13:02

Add node analysis package (revision 1)

0658da8

Add node analysis package (revision 2)

8e72b85

Add node analysis package (revision 3)

0ab82fb

jvdm force-pushed the jvdm/ROX-12238/2/pull branch from 1354404 to 0ab82fb Compare September 8, 2022 20:46

c-du approved these changes Sep 8, 2022

View reviewed changes

jvdm mentioned this pull request Sep 8, 2022

Refactor common detection function to its own package #926

Merged

jvdm merged commit ce27aff into master Sep 9, 2022

jvdm deleted the jvdm/ROX-12238/2/pull branch September 9, 2022 14:50

RTann reviewed Sep 9, 2022

View reviewed changes

jvdm added a commit that referenced this pull request Sep 9, 2022

Post-merge fixes for #911

45f470c

RTann mentioned this pull request Sep 20, 2022

Minor updates for #911 #936

Merged

RTann added a commit that referenced this pull request Sep 20, 2022

Minor updates for #911 (#936)

2901906

ROX-12238: Add node analysis package #911

ROX-12238: Add node analysis package #911

Uh oh!

Conversation

jvdm commented Sep 5, 2022

Tests

Uh oh!

ghost commented Sep 6, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

c-du commented Sep 7, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvdm commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvdm commented Sep 8, 2022

Uh oh!

openshift-ci bot commented Sep 8, 2022

Uh oh!

RTann Sep 9, 2022 • edited by jvdm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 6, 2022 •

edited by ghost

Loading

jvdm commented Sep 8, 2022 •

edited

Loading

RTann Sep 9, 2022 •

edited by jvdm

Loading