gRPC API & internals redesign #432

KeyboardNerd · 2017-07-12T21:25:18Z

This is a large Pull Request that will contain a number of chunks. For easier code review, the Clair V3 PR will contains these commits separately.

API changes
datastore interface changes
mock datastore, extensions update, worker, updater, notifier changes with tests
datastore pgsql implementation with tests
potential vendor clean up

jzelinskie · 2017-07-13T19:13:04Z

api/v2/clairpb/clair.proto

@@ -26,57 +27,63 @@ message Vulnerability {
 	string link = 4;
 	string severity = 5;
 	string metadata = 6;
+	// fixed_by only exists when querying features


Can we treat comments like Go ones? Full sentences 80 char lines.

jzelinskie · 2017-07-13T19:13:46Z

api/v2/clairpb/clair.proto

 }

-message Feature {
+message ClairStatus {
+	// all listers and detectors implemented in this clair version


Is this what Clair is built with or configured with?

all built-in listers/detectors. The listers and detectors are not configurable now, should I add that to this PR?

Nope, we can do that later, just as long as nothing here prevents us from doing so.

jzelinskie · 2017-07-13T19:24:23Z

api/v2/clairpb/clair.proto

+	string name = 2;
+}
+
+message PagedVulnerableAncestries {
 	string this_token = 1;
 	string next_token = 2;


maybe rename these 'current_page' and 'next_page'

jzelinskie · 2017-07-13T19:41:49Z

api/v2/rpc.go

+	if err != nil {
+		return nil, status.Error(codes.Internal, err.Error())
+	}
+


any reason you don't defer the rollback here and instead explicitly call it for each branch below?

I don't think Rollback() should ever be called after Commit() on success.

Rollback() from database/sql actually is safe to do this and is considered an idiomatic pattern to do so. I think it makes sense both ways, though.

KeyboardNerd · 2017-08-03T21:31:56Z

I'm using my analyze_local_images fork's py tools to test the APIs, which seems to work properly. I'll do more concurrency tests and the web hook tests.

Besides the implementation of new schema and analyzing functions, I also added the knob to control feature listers and namespace detectors to scan the layers and updaters to fetch vuln src.

jzelinskie

oh boy, here we go

jzelinskie · 2017-07-25T21:10:40Z

database/models.go


-	// For output purposes. Only make sense when the feature version is in the context of an image.
-	AddedBy Layer
+// AffectedFeature is used determine whether a namespaced feature is affected.


AffectedFeature is used to determine whether a namespaced feature is affected by a Vulnerability.

jzelinskie · 2017-08-07T15:29:35Z

api/v2/clairpb/convert.go

+		return nil, err
+	}
+
+	// encrypt page numbers


this isn't a particularly useful comment

jzelinskie · 2017-08-07T15:34:29Z

api/v2/clairpb/convert.go

+	}
+
+	// encrypt page numbers
+	nextToken, err := dbVuln.Next.Encrypt(paginationKey)


What happens when there is no next page? Is there a special constant for that or does this function need to handle nil?

jzelinskie · 2017-08-07T15:35:10Z

api/v2/clairpb/convert.go

-		},
-	}, nil
-}
+	pbOldVuln, err := PagedVulnerableAncestriesFromDatabaseModel(dbOldVuln, paginationKey)


What happens if there is no old vulnerability? Doesn't this need to handle nil?

jzelinskie · 2017-08-07T19:17:10Z

api/v2/rpc.go

+		}
+
+		if !ok {
+			return nil, status.Error(codes.NotFound, "requested ancestry \""+ancestry.Name+" \" is not found")


raw string literals will make this easier to read

return nil, status.Error(`requested ancestry "` + ancestry.Name + `" is not found`)

jzelinskie · 2017-08-08T19:44:13Z

updater.go

+	if err != nil {
+		return nil, err
+	}
+


why not defer rollback?

jzelinskie · 2017-08-08T19:46:13Z

worker.go

 	}

-	log.WithFields(log.Fields{logLayerName: name, "path": cleanURL(path), "engine version": Version, "parent layer": parentName, "format": imageFormat}).Debug("processing layer")
+	// TODO: to fix, when there are duplicated layers


actually it's not true! the layers are deduplicated before the function call

jzelinskie · 2017-08-08T19:47:01Z

worker.go

 	if err != nil {
-		return
+		return err
 	}


defer rollback?

jzelinskie · 2017-08-08T19:55:19Z

worker.go

+	// version format -> feature ID -> feature
+	features := map[string]map[string]database.NamespacedFeature{}
+	for _, layer := range ancestryLayers {
+		// at start of the loop, namespaces and features always contain the previous


capital at the start of sentences, end with periods, lol grammar

jzelinskie · 2017-08-08T19:55:59Z

worker_test.go

 	_ "github.com/coreos/clair/ext/featurefmt/dpkg"
+	_ "github.com/coreos/clair/ext/featurefmt/rpm"
+	"github.com/coreos/clair/ext/featurens"


pull these up to where they used to be

Main Clair logic is changed in worker, updater, notifier for better adapting ancestry schema. Extensions are updated with the new model and feature lister and namespace detector drivers are able to specify the specific listers and detectors used to process layer's content. InRange and GetFixedIn interfaces are added to Version format for adapting ranged affected features and next available fixed in in the future. Tests for worker, updater and extensions are fixed.

jzelinskie · 2017-08-16T18:41:15Z

I think we should rename the API package to just "api" or "v3" (because Clair v3) in order to avoid confusion.

After that I think this is safe to merge!

gRPC API & internals redesign

KeyboardNerd force-pushed the ancestry_ branch from f583b87 to bcdf5aa Compare July 12, 2017 21:39

jzelinskie self-requested a review July 13, 2017 15:36

KeyboardNerd force-pushed the ancestry_ branch from bcdf5aa to 02d3c8f Compare July 13, 2017 17:12

jzelinskie suggested changes Jul 13, 2017

View reviewed changes

KeyboardNerd force-pushed the ancestry_ branch 5 times, most recently from 2a479a9 to 47e2e2a Compare July 20, 2017 21:21

KeyboardNerd force-pushed the ancestry_ branch 3 times, most recently from 3e3b9c8 to 4c0b6ac Compare July 26, 2017 23:31

KeyboardNerd force-pushed the ancestry_ branch from 4c0b6ac to 167a97c Compare August 3, 2017 21:28

jzelinskie suggested changes Aug 8, 2017

View reviewed changes

KeyboardNerd added 4 commits August 10, 2017 11:19

API: drop v1 api, changed v2 api for Clair v3.

a378cb0

Datastore: updated for Clair V3, decoupled interfaces and models

57b146d

database: postgres implementation with tests.

a5c6400

KeyboardNerd force-pushed the ancestry_ branch from 167a97c to a5c6400 Compare August 10, 2017 15:26

pgSQL: fixed invalidating vulnerability cache query.

57a4f97

jzelinskie changed the title ~~Clair V3~~ gRPC API & internals redesign Aug 14, 2017

This was referenced Aug 14, 2017

support multiple namespaces in one layer #193

Closed

Enable runtime fetcher disable/enable #351

Closed

implement nodejs fetcher #175

Closed

api: renamed V2 API to V3 API for consistency.

58022d9

jzelinskie approved these changes Aug 17, 2017

View reviewed changes

jzelinskie merged commit 6663bce into quay:master Aug 17, 2017

KeyboardNerd pushed a commit to KeyboardNerd/clair that referenced this pull request Feb 2, 2018

Merge pull request quay#432 from KeyboardNerd/ancestry_

4eeeef0

gRPC API & internals redesign

jzelinskie mentioned this pull request Sep 26, 2018

Disable fetcher(s)? #221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gRPC API & internals redesign #432

gRPC API & internals redesign #432

KeyboardNerd commented Jul 12, 2017 •

edited

Loading

jzelinskie Jul 13, 2017

jzelinskie Jul 13, 2017

KeyboardNerd Jul 13, 2017

jzelinskie Jul 13, 2017

jzelinskie Jul 13, 2017

jzelinskie Jul 13, 2017

KeyboardNerd Jul 13, 2017

jzelinskie Jul 13, 2017

KeyboardNerd commented Aug 3, 2017 •

edited

Loading

jzelinskie left a comment

jzelinskie Jul 25, 2017

jzelinskie Aug 7, 2017

jzelinskie Aug 7, 2017

jzelinskie Aug 7, 2017

jzelinskie Aug 7, 2017

jzelinskie Aug 8, 2017

jzelinskie Aug 8, 2017

KeyboardNerd Aug 9, 2017 •

edited

Loading

jzelinskie Aug 8, 2017

jzelinskie Aug 8, 2017

jzelinskie Aug 8, 2017

jzelinskie commented Aug 16, 2017 •

edited

Loading

gRPC API & internals redesign #432

gRPC API & internals redesign #432

Conversation

KeyboardNerd commented Jul 12, 2017 • 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

KeyboardNerd commented Aug 3, 2017 • edited Loading

jzelinskie 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

KeyboardNerd Aug 9, 2017 • 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

jzelinskie commented Aug 16, 2017 • edited Loading

KeyboardNerd commented Jul 12, 2017 •

edited

Loading

KeyboardNerd commented Aug 3, 2017 •

edited

Loading

KeyboardNerd Aug 9, 2017 •

edited

Loading

jzelinskie commented Aug 16, 2017 •

edited

Loading