diff ingester: Allow configurable wildcards to match ecosystem file names, plus some cleanups #975

jhrozek · 2023-09-18T12:53:10Z

I started adding wildcard support for the package ecosystem matching in the
diff ingester and ended up doing a little cleanup after the code had been
rushed a bit to me demoable. I ended up adding pagination support, removed
some code that was just needed after a refactoring and added the wildcard
support with a basic test. More tests are incoming, there was only so much
time on the plane :-)

Fixes: #954

JAORMX

I dig the cleanup and tests, but let's move the github dependencies to one place as we prepare for the pluggable providers to land

JAORMX · 2023-09-18T13:03:12Z

internal/engine/ingester/diff/diff.go


+	"github.com/google/go-github/v53/github"


Can we not use the github API directly? Instead, let's include it in the github provider implementation. This way, we'll be able to better abstract this in the future.

This is not the code calling the GH API directly, but just this line:

listOpts := &github.ListOptions{PerPage: 30}

I can modify the ListFiles method to accept a generic parameter struct instead, that would get rid of the import, but note that the REST API currently returns GH specific types and there are other methods that accept GH specific types so maybe a bigger cleanup is in order?

We are indeed gonna need a bigger cleanup

JAORMX · 2023-09-18T13:03:35Z

internal/engine/ingester/diff/diff.go

-	if err != nil {
-		return nil, fmt.Errorf("error getting pull request files: %w", err)
-	}
+	logger := zerolog.Ctx(ctx).With().


Do we have a bug for migrating everything to zero log?

I don't think so, I think @evankanderson pointed me to code that showed that calling log triggers calling zerolog already? So far I've been just adding zerolog calls to code that I'm writing from scratch or cleaning up, but I don't think we track any systematical cleanup.

evankanderson · 2023-09-18T15:06:11Z

internal/engine/ingester/diff/diff.go

-			continue
+	listOpts := &github.ListOptions{PerPage: 30}
+	for {
+		prFiles, resp, err := di.cli.ListFiles(ctx, pr.RepoOwner, pr.RepoName, int(pr.Number), listOpts)


nit Long term, it would be nice to be able to use a channel to feed the results from reading the paginated list, but this seems fine for now. (Like using a generator in some other languages.)

I want to use channels in the vulnerability evaluator to have a pipeline between the database and the package repository, there is actually seems like it will cut down on the wait time quite a bit and even simplify the code. If you don't mind, in this module I would wait and implement that later (maybe we implement the pagination in the REST API module by returning a channel?)

For the moment a simple loop seems fine because 99% of the list results would be just discarded right away. (most PRs don't add deps..)

Yeah, my thought was that later we could implement lazy pagination (rather than the eager pagination here) by pulling from a channel and having the channel's capacity block further pagination calls.

Don't do this before November, though. We have bigger fish to fry.

and more fish :-)
I filed #996 in the meantime linking back here so we are reminded of this discussion later.

evankanderson · 2023-09-18T15:10:10Z

internal/engine/ingester/diff/diff.go

+			eco := di.getEcosystemForFile(*file.Filename)
+			if eco == DepEcosystemNone {
+				logger.Debug().
+					Str("filename", *file.Filename).
+					Msg("No ecosystem found, skipping")
+				continue
+			}
+
+			logger.Debug().
+				Str("filename", *file.Filename).
+				Str("package-ecosystem", string(eco)).
+				Msg("No ecosystem found, skipping")
+
+			parser := newEcosystemParser(eco)
+			if parser == nil {
+				return nil, fmt.Errorf("no parser found for ecosystem %s", eco)
+			}
+
+			depBatch, err := parser(*file.Patch)
+			if err != nil {
+				return nil, fmt.Errorf("error parsing file %s: %w", *file.Filename, err)
+			}
+
+			for i := range depBatch {
+				dep := depBatch[i]
+				allDiffs = append(allDiffs, &pb.PrDependencies_ContextualDependency{
+					Dep: dep,
+					File: &pb.PrDependencies_ContextualDependency_FilePatch{
+						Name:     file.GetFilename(),
+						PatchUrl: file.GetRawURL(),
+					},
+				})
+			}


Do you want to extract this into a separate function?

done locally, will push after a round of testing

evankanderson · 2023-09-18T15:10:59Z

internal/engine/ingester/diff/diff.go

+			logger.Debug().
+				Str("filename", *file.Filename).
+				Str("package-ecosystem", string(eco)).
+				Msg("No ecosystem found, skipping")


Did you mean to have this message here?

oops, I fixed this already locally, but /in a different branch/ (I have already been building more tests atop this other branch)
I did mean to leave a debug message there, but it shouldn't say that no ecosystem matched..

evankanderson · 2023-09-18T15:13:02Z

internal/engine/ingester/diff/diff.go

+			if parser == nil {
+				return nil, fmt.Errorf("no parser found for ecosystem %s", eco)
+			}


Do we need both this and the eco == DepEcosystemNone?

I guess not at the moment and not strictly needed, but at the same time I think it's better to check each call's return value (or alternatively we might want to encapsulate the two calls into one function)

evankanderson · 2023-09-18T15:17:10Z

internal/engine/ingester/diff/diff.go

+				allDiffs = append(allDiffs, &pb.PrDependencies_ContextualDependency{
+					Dep: dep,
+					File: &pb.PrDependencies_ContextualDependency_FilePatch{
+						Name:     file.GetFilename(),


Why do we use file.GetFilename() here, but *file.Filename on lines 88, 91, and 97?

no reason except inconsistency. Thanks, fixed.

evankanderson · 2023-09-18T15:25:48Z

internal/engine/ingester/diff/diff.go

+		if strings.HasPrefix(ecoMapping.Depfile, wildcard) {
+			strippedDepfile := strings.TrimPrefix(ecoMapping.Depfile, wildcard)
+			if lastComponent == strippedDepfile {
+				return DependencyEcosystem(ecoMapping.Name)
+			}
+		} else if filename == ecoMapping.Depfile {


I don't really understand what this is supposed to do -- it feels like this should be a glob (and maybe matching with regexp), but the code doesn't look like the code I'd expect for a glob.

if the code was difficult to parse, then please block the PR merge that's what reviews are for :-)

Does the current version with filepath.Match read better?
(I wasn't sure initially about fifepatch.Match matching in the middle of the path, seemed like a footgun, but in the end the code is readable and more generic with Match...)

internal/engine/ingester/diff/diff_test.go

evankanderson · 2023-09-18T15:29:53Z

proto/mediator/v1/mediator.proto

-    repeated FilePatch patches = 6; // The list of files changed in the PR. Does not include file contents
-
-    int64 author_id = 7;    // The author of the PR, will be used to check if we can request changes
+    int64 author_id = 6;    // The author of the PR, will be used to check if we can request changes


Intentional to renumber these proto fields?

yes, this protobuf struct is only ever used between mediator server and the policy engine and not exposed to the clients, so I think it's better to renumber while removing the FilePatch attribute. When/if we send this struct to the clients, renumbering fields should probably be a hard nack.

evankanderson · 2023-09-20T00:03:13Z

internal/engine/ingester/diff/diff.go

-			continue
+	listOpts := &github.ListOptions{PerPage: 30}
+	for {
+		prFiles, resp, err := di.cli.ListFiles(ctx, pr.RepoOwner, pr.RepoName, int(pr.Number), listOpts)


Yeah, my thought was that later we could implement lazy pagination (rather than the eager pagination here) by pulling from a channel and having the channel's capacity block further pagination calls.

Don't do this before November, though. We have bigger fish to fry.

evankanderson · 2023-09-20T00:06:21Z

internal/engine/ingester/diff/diff.go

+		Str("package-ecosystem", string(eco)).
+		Msg("matched ecosystem")
+
+	parser := newEcosystemParser(eco)


(Following up from the earlier comment)

My suggestion about the check for eco == DepEcosystemNone was, in fact, to merge the two function calls into one:

if parser == nil { logger.Debug().Str("filename", filename).Msg("No ecosystem found, skipping") return nil, nil } logger.Debug().Str("filename", filename).Str("package-ecosystem", parser.Ecosystem()). Msg("matched ecosystem") // Can also use .Send() IIRC

internal/engine/ingester/diff/diff_test.go

jhrozek · 2023-09-25T16:42:12Z

@evankanderson would you mind approving this version again? There are no functional changes, just a rebase to resolve conflicts in the autogenerated protobuf files.

JAORMX requested changes Sep 18, 2023

View reviewed changes

evankanderson previously approved these changes Sep 18, 2023

View reviewed changes

JAORMX self-requested a review September 19, 2023 06:38

JAORMX previously approved these changes Sep 19, 2023

View reviewed changes

jhrozek dismissed stale reviews from JAORMX and evankanderson via 932feee September 19, 2023 06:46

jhrozek force-pushed the diff_cleanup_wildcard branch from cd2dc71 to 932feee Compare September 19, 2023 06:46

evankanderson reviewed Sep 20, 2023

View reviewed changes

jhrozek force-pushed the diff_cleanup_wildcard branch from 932feee to 3d17288 Compare September 22, 2023 16:31

jhrozek mentioned this pull request Sep 22, 2023

Support lazy pagination for GitHub API #996

Closed

jhrozek force-pushed the diff_cleanup_wildcard branch from 3d17288 to 51e6da6 Compare September 22, 2023 16:40

evankanderson previously approved these changes Sep 25, 2023

View reviewed changes

jhrozek added 5 commits September 25, 2023 18:41

diff ingestor: Support pagination

f3b68d7

Remove useless PR list from the PR webhook

680d91c

diff ingester: Remove unused struct

17ecae4

diff ingester: Allow matching on wildcards

f680616

diff ingestor: split ingesting a single file into a separate function

01e26b2

jhrozek dismissed evankanderson’s stale review via 01e26b2 September 25, 2023 16:41

jhrozek force-pushed the diff_cleanup_wildcard branch from 51e6da6 to 01e26b2 Compare September 25, 2023 16:41

evankanderson approved these changes Sep 25, 2023

View reviewed changes

jhrozek merged commit e2a22fe into mindersec:main Sep 25, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

diff ingester: Allow configurable wildcards to match ecosystem file names, plus some cleanups #975

diff ingester: Allow configurable wildcards to match ecosystem file names, plus some cleanups #975

jhrozek commented Sep 18, 2023

JAORMX left a comment

JAORMX Sep 18, 2023

jhrozek Sep 18, 2023 •

edited

Loading

JAORMX Sep 18, 2023

JAORMX Sep 18, 2023

jhrozek Sep 18, 2023

evankanderson Sep 18, 2023

jhrozek Sep 19, 2023

evankanderson Sep 20, 2023

jhrozek Sep 22, 2023

evankanderson Sep 18, 2023

jhrozek Sep 19, 2023

evankanderson Sep 18, 2023

jhrozek Sep 19, 2023

evankanderson Sep 18, 2023

jhrozek Sep 19, 2023

evankanderson Sep 18, 2023

jhrozek Sep 19, 2023

evankanderson Sep 18, 2023

jhrozek Sep 19, 2023

evankanderson Sep 18, 2023

jhrozek Sep 19, 2023

evankanderson Sep 20, 2023

evankanderson Sep 20, 2023

jhrozek Sep 22, 2023

jhrozek commented Sep 25, 2023

diff ingester: Allow configurable wildcards to match ecosystem file names, plus some cleanups #975

diff ingester: Allow configurable wildcards to match ecosystem file names, plus some cleanups #975

Conversation

jhrozek commented Sep 18, 2023

JAORMX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhrozek Sep 18, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhrozek commented Sep 25, 2023

jhrozek Sep 18, 2023 •

edited

Loading