Add git ingester #775

JAORMX · 2023-08-28T14:11:18Z

The git ingester will take a specific branch, clone it in a temporary
directory, and set it as a context for the policy to evaluate on top
of it.

The intention is to be able to run policies on git contents.

jhrozek

left some comments inline. I'm a bit confused about the tracking. Also cloning on every ingest might get expensive fast, should we think about implementing a timeout or LRU?

jhrozek · 2023-08-29T07:10:41Z

internal/engine/ingester/git/git.go

+// Git is the engine for a rule type that uses git data ingest
+type Git struct {
+	accessToken string
+	fileTrack   map[string]struct{}


could the ingester ever be called concurrently? (thinking about syncMap)

No, there is a single instance of the ingester being executed at a time.

for anyone else confused like me, we do instantiate a per-rule instance for every rule which in turns instantiates a new evaluator per rule (thanks @JAORMX for the explanation on slack)

internal/engine/ingester/git/git.go

jhrozek

one more comment about auth - didn't see it earlier, sorry. I guess in order for this PR to not be WIP, we also need to a new Message to mediator.proto that represents the git ingester along with the plumbing to instantiate it?

internal/engine/ingester/git/git.go

evankanderson

I'm not sure where you're headed with this yet, but the git clone part looks reasonable.

evankanderson · 2023-08-29T21:51:46Z

internal/engine/eval/jq/jq.go

 // Eval calls the jq library to evaluate the rule
-func (jqe *Evaluator) Eval(ctx context.Context, pol map[string]any, obj any) error {
+func (jqe *Evaluator) Eval(ctx context.Context, pol map[string]any, res *engif.Result) error {


Add a type assertion that Evaluator fulfils engif.Evaluator?

Suggested change

// Eval calls the jq library to evaluate the rule

func (jqe *Evaluator) Eval(ctx context.Context, pol map[string]any, obj any) error {

func (jqe *Evaluator) Eval(ctx context.Context, pol map[string]any, res *engif.Result) error {

var _ engif.Evaluator = (*Evaluator)(nil)

// Eval calls the jq library to evaluate the rule

func (jqe *Evaluator) Eval(ctx context.Context, pol map[string]any, res *engif.Result) error {

internal/engine/ingester/git/git.go

evankanderson · 2023-08-29T22:09:31Z

internal/engine/ingester/git/git.go

+	// We clone to the memfs go-billy filesystem driver, which doesn't
+	// allow for direct access to the underlying filesystem. This is


This is confusing -- I think you mean "doesn't store any contents on the local filesystem". The fs object can still be used to read files from the repo.

One future enhancement item might be to be able to further filter the files included in the fetch, so we don't need to allocate hundreds of megabytes for large repos. I think a TODO is sufficient for that case, though.

internal/engine/ingester/git/git.go

evankanderson · 2023-08-29T22:12:53Z

internal/engine/ingester/git/git.go

+	}
+
+	return &engif.Result{
+		Object: struct{}{},


Should Object be nil here or r, rather than an empty struct?

(I'm not sure I understand the semantics of Result, I guess.

I'm confused, too, is this supposed to ensure that evaluators that consume an Object will not throw an error, but rather never evaluate? I would say that nil might be more explicit and safer (thinking about an jq evaluator that makes sure that an input does NOT have a path, I think this empty object would pass that evaluator..)

lukehinds

👍 from me. I spoke with @JAORMX and we discussed allowing multiple ways of accessing files, as cloning could be costly for our ingress. @JAORMX added #793 as a countermeasure.

jhrozek

Some nits and questions, but overall looks good. Thanks for adding the tests!

internal/engine/eval/rego/eval.go

jhrozek · 2023-09-01T09:23:38Z

internal/engine/interfaces/interface.go

+	// Fs is the filesystem that was created as a result of the ingestion. This
+	// is normally used by the evaluator to do rule evaluation. The filesystem
+	// may be a git repo, or a memory filesystem.
+	Fs billy.Filesystem


What kind of evaluator will be plugged into the git ingestor that consumes the FS? Evaluating vulnerabilities comes to mind, but what I wondered when I read this code is if we want to be able to express a contract between an ingestor and evaluator and throw an error on parsing the rule that ingestor X cannot work with evaluator Y rather than throwing a runtime error saying that an object or a FS is missing during evaluation..

jhrozek · 2023-09-01T09:24:17Z

internal/engine/ingester/git/config.go

+// in different calls as opposed to having to set it in the rule type.
+type IngesterConfig struct {
+	Branch   string `json:"branch" yaml:"branch" mapstructure:"branch"`
+	CloneURL string `json:"clone_url" yaml:"clone_url" mapstructure:"clone_url"`


I guess the size limit option would come at a later point?

that's right

internal/engine/ingester/git/config.go

jhrozek · 2023-09-01T09:28:51Z

internal/engine/ingester/git/git.go

+	}
+
+	return &engif.Result{
+		Object: struct{}{},


I'm confused, too, is this supposed to ensure that evaluators that consume an Object will not throw an error, but rather never evaluate? I would say that nil might be more explicit and safer (thinking about an jq evaluator that makes sure that an input does NOT have a path, I think this empty object would pass that evaluator..)

jhrozek · 2023-09-01T09:29:53Z

internal/engine/ingester/git/git.go

+	}
+
+	// If the entity is a repository get it from the entity
+	// else, get it from the configuration


This comment doesn't seem to match what the code does (IOW, the code prefers the config)

btw what is the use-case for git ingester w/o a repository?

I don't have a solid answer right now, but I'm guessing we could do something interesting with comparing contents from artifacts with something in a git repo.

jhrozek · 2023-09-01T09:38:08Z

internal/engine/ingester/git/git_test.go

+	gi := gitengine.NewGitIngester(&pb.GitType{}, "")
+	got, err := gi.Ingest(context.Background(), &pb.Artifact{}, map[string]any{
+		"clone_url": "https://github.com/octocat/Hello-World.git",
+		"branch":    "test",


Is it possible to assert that we cloned the right branch here?

That branch has other contents, that the master branch doesn't have, maybe it would be sufficient to check that?

yes, but it's fine to do it later

jhrozek

actually let me approve this so that you're not blocked (I'd be curious about the questions I had either way)

The git ingester will take a specific branch, clone it in a temporary directory, and set it as a context for the policy to evaluate on top of it. The intention is to be able to run policies on git contents.

jhrozek

LGTM, there is some follow-up work to do I guess, but this PR can go in as-is

JAORMX changed the title ~~Add git ingester~~ WIP: Add git ingester Aug 28, 2023

JAORMX force-pushed the git-ingester branch 3 times, most recently from 1ff0075 to 85a06f8 Compare August 28, 2023 14:28

jhrozek reviewed Aug 29, 2023

View reviewed changes

JAORMX force-pushed the git-ingester branch from 85a06f8 to b9df0ae Compare August 29, 2023 10:15

jhrozek reviewed Aug 29, 2023

View reviewed changes

internal/engine/ingester/git/git.go Outdated Show resolved Hide resolved

JAORMX force-pushed the git-ingester branch from b9df0ae to 64095e7 Compare August 29, 2023 13:07

JAORMX requested a review from jhrozek August 29, 2023 13:07

JAORMX force-pushed the git-ingester branch from 64095e7 to d637fef Compare August 29, 2023 13:08

evankanderson previously approved these changes Aug 29, 2023

View reviewed changes

lukehinds previously approved these changes Aug 30, 2023

View reviewed changes

JAORMX dismissed stale reviews from lukehinds and evankanderson via 42a9691 September 1, 2023 06:53

JAORMX force-pushed the git-ingester branch 2 times, most recently from 42a9691 to f5b2ff5 Compare September 1, 2023 08:19

JAORMX changed the title ~~WIP: Add git ingester~~ Add git ingester Sep 1, 2023

JAORMX requested review from evankanderson and lukehinds September 1, 2023 08:19

JAORMX force-pushed the git-ingester branch from f5b2ff5 to 26c6c8f Compare September 1, 2023 08:21

jhrozek reviewed Sep 1, 2023

View reviewed changes

jhrozek previously approved these changes Sep 1, 2023

View reviewed changes

JAORMX dismissed jhrozek’s stale review via d3afda4 September 1, 2023 09:52

JAORMX force-pushed the git-ingester branch 3 times, most recently from d4214cf to a3461ca Compare September 1, 2023 09:55

Add git ingester

742b3bb

The git ingester will take a specific branch, clone it in a temporary directory, and set it as a context for the policy to evaluate on top of it. The intention is to be able to run policies on git contents.

JAORMX force-pushed the git-ingester branch from a3461ca to 742b3bb Compare September 1, 2023 09:55

JAORMX requested a review from jhrozek September 1, 2023 09:59

jhrozek approved these changes Sep 1, 2023

View reviewed changes

JAORMX merged commit 5cea6d9 into main Sep 1, 2023
14 checks passed

JAORMX deleted the git-ingester branch September 1, 2023 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add git ingester #775

Add git ingester #775

JAORMX commented Aug 28, 2023

jhrozek left a comment

jhrozek Aug 29, 2023

JAORMX Aug 29, 2023

jhrozek Aug 29, 2023

jhrozek left a comment

evankanderson left a comment

evankanderson Aug 29, 2023

evankanderson Aug 29, 2023

evankanderson Aug 29, 2023

evankanderson Aug 29, 2023

jhrozek Sep 1, 2023

lukehinds left a comment

jhrozek left a comment

jhrozek Sep 1, 2023

jhrozek Sep 1, 2023

JAORMX Sep 1, 2023

jhrozek Sep 1, 2023

jhrozek Sep 1, 2023

jhrozek Sep 1, 2023

JAORMX Sep 1, 2023

jhrozek Sep 1, 2023

JAORMX Sep 1, 2023

jhrozek Sep 1, 2023

jhrozek left a comment

jhrozek left a comment

		// We clone to the memfs go-billy filesystem driver, which doesn't
		// allow for direct access to the underlying filesystem. This is

Add git ingester #775

Add git ingester #775

Conversation

JAORMX commented Aug 28, 2023

jhrozek 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

jhrozek left a comment

Choose a reason for hiding this comment

evankanderson 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

lukehinds left a comment

Choose a reason for hiding this comment

jhrozek 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

jhrozek left a comment

Choose a reason for hiding this comment

jhrozek left a comment

Choose a reason for hiding this comment