Internally version all detected content by extension #620

KeyboardNerd · 2018-09-12T21:21:29Z

Conceptual change:

Detector now represents anything that extracts information from a layer blob. Detector has two different types for now: Namespace ( Detector to extract namespace) and Feature ( Detector to extract features )
Processor and Lister are renamed to Detector

Changes in API:

Add Detector message type, Namespace message type
Add detector field to Feature/Namespace to indicate which detector finds a specific feature or namespace
Change all scanned_* to be just detector

Changes in Database Model:

Every features and namespaces are associated with the detectors that found them.
Aggregated concepts:
- Entities: objects relating the vulnerabilities and the ancestries as the result of Analysis.
- Ancestries: layers and ancestries scanned with detectors with metadata (name/structure/relation to entities/detectors) saved in the database.
- Vulnerabilities: vulns fetched by the updaters.
- Notification: vulnerability notifications spawned by vulnerability changes.
- UpdaterLock: lock and keyValue.

Changes in Behavior:

Remove Clair side (dis)enable detectors in configuration. Now, every layer is scanned with every detectors.

Additional responsibility to Client:

Client should filter out the features that are not necessary by filtering the detectors ( This responsibility maybe moved to API level on server side in the future )

KeyboardNerd · 2018-09-12T21:28:36Z

Please review clair.proto and models.go to confirm the expectation. Those are expected to not be changed in the following commits.

KeyboardNerd

Self comments

KeyboardNerd · 2018-09-14T16:23:05Z

database/pgsql/migrations/00001_initial_schema.go

+	}
+
+	// detector is analysis extensions used by the worker.
+	detector = dbutil.MigrationQuery{


New Table: Detectors

Detector should contain all metadata information of all the extensions used to scan a layer.

database/pgsql/migrations/00001_initial_schema.go

KeyboardNerd · 2018-09-14T16:33:03Z

database/pgsql/migrations/00001_initial_schema.go

+var (
+	// entities are the basic building blocks to relate the vulnerabilities with
+	// the ancestry.
+	entities = dbutil.MigrationQuery{


Entities are the real immutable content in the database.

On a higher level, Layer ( layer hash with layer content ) is not really immutable because a new extension can scan a layer and add new associated features.

KeyboardNerd · 2018-09-14T16:34:10Z

ext/featurefmt/driver.go

-	if name == "" {
-		panic("featurefmt: could not register a Lister with an empty name")
-	}
+func RegisterLister(l Lister) {


I will change Lister to detector in the following PRs.

KeyboardNerd · 2018-09-14T16:35:23Z

database/detector.go

+
+// Detector is an extention to scan a layer's content.
+type Detector struct {
+	// Name of the detector


Please review this design to use string to represent the Detector Type.
I can also define FeatureDetector and NamespaceDetector struct types for the same purpose.

config.yaml.sample

cmd/clair/main.go

api/v3/clairpb/clair.proto

KeyboardNerd · 2018-09-14T21:30:53Z

database/models.go

@@ -71,6 +65,7 @@ type Layer struct {
 type Namespace struct {
 	Name          string
 	VersionFormat string
+	DetectedBy    Detector


Using Detector here maybe not valid. As the reason shows in the database, namespace/feature may be detected by two different detectors in two different layers.

database/detector.go

jzelinskie · 2018-09-13T17:09:49Z

database/detector.go

+	// Version of the detector
+	Version string
+	// Type of the detector
+	Type DetectorType


type is a keyword so I'm a little uneasy about using this name.
As a counter-point, the reflect standard library package does define their own type Type interface {...}

https://github.com/coreos/clair/blob/master/database/database.go#L42 haha

jzelinskie · 2018-09-13T18:36:16Z

database/detector.go

+}
+
+// NewNamespaceDetector returns a new namespace detector.
+func NewNamespaceDetector(name string, version string) Detector {


name, verison string

jzelinskie · 2018-09-13T18:36:27Z

database/detector.go

+}
+
+// NewFeatureDetector returns a new feature detector.
+func NewFeatureDetector(name string, version string) Detector {


name, version string

jzelinskie · 2018-09-13T18:42:12Z

api/v3/clairpb/clair.proto

+  }
+  // The name of the detector.
+  string name = 1;
+  // The Version of the detector.


Version -> version

jzelinskie · 2018-09-17T21:16:59Z

database/pgsql/migrations/00001_initial_schema.go

+	RegisterMigration(dbutil.NewSimpleMigration(1,
+		[]dbutil.MigrationQuery{
+			entities, detector, layer, ancestry,
+			vulnerability, updaterLock, notification,


one line each

jzelinskie · 2018-09-17T21:19:51Z

pkg/dbutil/migrationutil.go

@@ -0,0 +1,30 @@
+package dbutil


copyright headers

and let's move this into database/psql/migrations

All processors will now be used to process the layers.

KeyboardNerd · 2018-09-19T20:00:08Z

database/models.go

+type Layer struct {
+	Hash             string
+	Namespaces       DetectedNamespaces
+	DetectedFeatures DetectedFeatures


DetectedFeatures -> Features

KeyboardNerd · 2018-09-21T14:03:47Z

database/models.go

-// ProcessedBy.
+// DetectedNamespaces contains a map from detector to the detected namespace.
+// Nil detected namespace means it's not found.
+type DetectedNamespaces map[Detector]*Namespace


The new map type introduces a lot more complexity. It makes the worker complicated and also the database implementation. Maybe, we don't want this map to organize since the map will at most contain around 1000 features.

Flatten the map to be []DetectedNamespace, which contains Detector and Namespace.
This is for simplifying the data structure, we can then use pointers and a new data structure if we need to improve the performance.

KeyboardNerd · 2018-09-28T02:39:11Z

api/v3/clairpb/clair.proto

@@ -44,17 +44,42 @@ message Vulnerability {
  repeated Feature affected_versions = 8;
 }

+message Detector {
+  enum Type {
+    DETECTOR_TYPE_INVALID = 0;


This name is a requirement by the prototool linter.

api/v3/util.go

KeyboardNerd · 2018-09-28T02:43:36Z

api/v3/util.go

+	// this feature for now, we should refactor the implementation if there's
+	// any performance issue. It's expected that the number of features is less
+	// than 1000.
+


nit: remove line

api/v3/util.go

KeyboardNerd · 2018-09-28T02:51:23Z

cmd/clair/main.go

-	clair.EnabledUpdaters = strutil.CompareStringListsInBoth(config.Updater.EnabledUpdaters, updaters)
+		"Detectors": strings.Join(database.SerializeDetectors(clair.EnabledDetectors), ","),
+		"Updaters":  strings.Join(clair.EnabledUpdaters, ","),
+	}).Info("Enabled Clair components")


nit: lower case

KeyboardNerd · 2018-09-28T03:26:00Z

ext/featurens/driver.go

-		nslist = append(nslist, *ns)
-	}
-	return nslist, nil
+	return namespaces, nil
 }

 // RequiredFilenames returns the total list of files required for all


update the comment

KeyboardNerd · 2018-09-28T03:26:19Z

ext/featurens/driver_test.go

@@ -3,11 +3,12 @@ package featurens_test
 import (
 	"testing"

+	"github.com/coreos/clair/ext/featurens"


pkg/dbutil/dbutil.go

KeyboardNerd · 2018-09-28T03:31:51Z

pkg/dbutil/dbutil.go

+	return nil
+}
+
+func CacheRelatedVulnerability(datastore database.Datastore, features []database.NamespacedFeature) error {


TODO: I start to think that the abstraction on database layer is wrong. Somehow we need to figure out a way to get ride of the implicit database feature relationship.

KeyboardNerd · 2018-09-28T03:32:09Z

pkg/dbutil/dbutil.go

+	return FilterDetectors(detectors, database.NamespaceType)
+}
+
+func FilterDetectors(detectors []database.Detector, dtype database.DetectorType) []database.Detector {


is this being used? remove

KeyboardNerd

Note to other PR reviewer
TODO means it's a thing for the future PR not this one
NIT means it's not blocking merging the PR
Not related means it's a old issue and can be not addressed in this PR
Otherwise it means blocking the PR.

database/pgsql/migrations/00001_initial_schema.go

KeyboardNerd · 2018-09-28T18:01:18Z

api/v3/clairpb/clair.proto

-    repeated string scanned_listers = 4;
-    // The configured list of namespace detectors used to scan an ancestry.
-    repeated string scanned_detectors = 5;
+    // The detectors used to scan this Ancestry. It maybe not the current set of


Rephrase: The detectors used to scan this Ancestry. It may not be the same set of detectors in the Clair instance.

api/v3/util.go

ext/featurefmt/driver.go

ext/featurens/driver.go

worker.go

KeyboardNerd · 2018-09-28T18:20:43Z

worker.go

-	}
-	if !ok {
-		return false, nil
+	ancestry, ok, err := dbutil.FindAncestry(datastore, name)


It's wasteful to find ancestry and all the features associated here. We should later consider to have this function on database interface.

worker.go

jzelinskie

This is my first pass. I'm going to need to take a look much harder at the database changes.

jzelinskie · 2018-10-01T18:16:52Z

api/v3/clairpb/convert.go

+	return &Detector{
+		Name:    detector.Name,
+		Version: detector.Version,
+		Type:    Detector_Type(Detector_Type_value[string(detector.DType)]),


Wow this kinda sucks. I wonder if gogo protobuf generates something more ergonomic than this.

jzelinskie · 2018-10-01T20:03:17Z

database/detector.go

+	// NamespaceDetectorType is a type of detector that extracts the namespaces.
+	NamespaceDetectorType DetectorType = "DETECTOR_TYPE_NAMESPACE"
+	// FeatureDetectorType is a type of detector that extracts the features.
+	FeatureDetectorType DetectorType = "DETECTOR_TYPE_FEATURE"


Is there a reason to even use a string? Why not have an integer?

const ( NamespaceDectectorType DetectorType = iota FeatureDetectorType ... )

because I want to store enum in the database. Well I can write a conversion to make this work anyway.

Enums are most often just integers in the database that have special meaning for the application. You use enums because it makes the column more simple (thus easier to migrate, copy, compare values etc...)

Yes, that's the point isn't it. DetectorType (Detector Enum) is just like Severity Enum. You can look up the database schema for that.

jzelinskie · 2018-10-01T20:08:14Z

database/detector.go

+}
+
+// SerializeDetectors returns the string representation of given detectors.
+func SerializeDetectors(detectors []Detector) []string {


We could put this in strutil if you change the function to be:

func StringifyList(xs []fmt.Stringer) (ys []string) { for _, x := range xs { ys = append(ys, x.String()) } return }

ok, then it will require the type conversion from []database.Detector to []fmt.Stringer, which sucks

Yeah. This is super unfortunate... It's because interfaces are different memory layouts and creating a list of them is O(N), so you have to explicitly make the copy yourself rather than having Go do it for you when you pass it into the function.

It makes me think if this kind of design is wrong after all. I think we are thinking about functions using generics but interfaces are not generics.

I think I should keep the original design for the serialization.

jzelinskie · 2018-10-01T21:51:57Z

database/models.go

-
-	// DetectedFeatures are the features introduced by this layer when it was
+	// Hash is the name of the ancestry's layer.
+	Hash string


This is comment also wrong. Hash is the sha256 digest of the manifest for a particular image.

database/pgsql/detector.go

jzelinskie · 2018-10-01T22:37:44Z

pkg/dbutil/dbutil.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package dbutil


I think everything in here makes sense to just have in the database package rather than in a utility package.

jzelinskie · 2018-10-01T22:42:31Z

pkg/testutil/testutil.go

@@ -0,0 +1,285 @@
+package testutil


I think all of this is useful for only the database. I think this would also be fine being just in the database package.

Also, copyright header

Yes, those are for comparing the database models but it's not only for the database tests. It's also used in the worker test and in the future updater test.

jzelinskie

I think the general approach for the database is good. We discussed it enough beforehand that it's pretty much exactly what I expected. I think that there were a bunch of things I noticed for database optimizations, but I don't think we should really spend time on that until we're positive it's the finalized schema.

some additional thoughts... i ran the codebase and sat pondering our current schema for a bit and came up with a few things that might not necessarily need to be done in this PR:

Get rid of namespaced_feature

ancestry_feature -> (id, ancestry_layer_id, feat_id, feat_detector_id, ns_id, ns_detector_id)

make kv table consistent

rename fields to updater/$NAMESPACE
prefix value to include the SCM: git-sha:hf92dk9

detector type

rename column type -> dtype
change column to this enum: CREATE TYPE detector_type AS ENUM ('feature', 'namespace');

debian:unstable ns needs to be mapped to a number

we should lstrip "v" off of the alpine namespace version numbers, no other namespaces do that

jzelinskie · 2018-10-05T22:07:48Z

cmd/clair/main.go

@@ -180,6 +154,18 @@ func Boot(config *Config) {
 	st.Stop()
 }

+func initLogger(flagLogLevel *string) {


I avoid using the word init to avoid confusion around the keyword.

jzelinskie · 2018-10-05T22:07:53Z

cmd/clair/main.go

-
-	clair.EnabledUpdaters = strutil.CompareStringListsInBoth(config.Updater.EnabledUpdaters, updaters)
+		"Detectors": strings.Join(database.SerializeDetectors(clair.EnabledDetectors), ","),
+		"Updaters":  strings.Join(clair.EnabledUpdaters, ","),


Do these have to be strings? I'd be nice to keep them as arrays in the JSON logs.
Also the detectors string is borderline unreadable:

{"Detectors":"DETECTOR_TYPE_FEATUREDetector/apk/1.0,DETECTOR_TYPE_FEATUREDetector/dpkg/1.0,DETECTOR_TYPE_FEATUREDetector/rpm/1.0,DETECTOR_TYPE_NAMESPACEDetector/alpine-release/1.0,DETECTOR_TYPE_NAMESPACEDetector/apt-sources/1.0,DETECTOR_TYPE_NAMESPACEDetector/lsb-release/1.0,DETECTOR_TYPE_NAMESPACEDetector/os-release/1.0,DETECTOR_TYPE_NAMESPACEDetector/redhat-release/1.0"}

database/dbutil.go

jzelinskie · 2018-10-05T22:08:37Z

database/dbutil.go

+
+// PersistNamespaces wraps session PersistNamespaces function with begin and
+// commit.
+func PersistNamespaces(datastore Datastore, namespaces []Namespace) error {


We should come up with a naming convention for the functions that are just wrapping calls in a transaction.

database/dbutil.go

jzelinskie · 2018-10-05T22:08:52Z

database/detector.go

+		}
+	}
+
+	return "", ErrFailedToParseSeverity


jzelinskie · 2018-10-05T22:08:55Z

database/detector.go

+
+// String returns a unique string representation of the detector.
+func (d Detector) String() string {
+	return fmt.Sprintf("%sDetector/%s/%s", d.DType, d.Name, d.Version)


I comment on main, but yeah this is pretty verbose and hard to read

jzelinskie · 2018-10-05T22:08:59Z

database/detector.go

+}
+
+// SerializeDetectors returns the string representation of given detectors.
+func SerializeDetectors(detectors []Detector) []string {


jzelinskie · 2018-10-05T22:09:03Z

database/models.go

 // Ancestry is a manifest that keeps all layers in an image in order.
 type Ancestry struct {
+	// Name is expected to be the sha-256 digest of the manifest for a


Name is a globally unique value for a set of layers. This is often the sha256 digest of an OCI/Docker manifest.

All detected features and namespaces under the context of Layer and Ancestry will now have the detectors associated, so that the API can provide the detection information to the Client.

'detector' table is added to store the metadata of detectors. 'layer_feature', 'layer_namespace', and 'ancestry_feature' tables are modified to store the detection relationship between the feature/namespace with the detector.

Golang-set library is added to make it easier to support set operations.

1. Every Lister and Detector are versioned 2. detected content, are returned in a map with detector info as the key

Aggregate queries in their corresponding files instead of having the single file for every queries because the database is more complicated.

Change the V3 implementation to accommondate the detectors.

The worker is changed to accommodate the new database model and API. Worker is refactored to move the database query helper functions to pkg.

* Refactor layer and ancestry * Add tests * Fix bugs introduced when the queries were moved

KeyboardNerd · 2018-10-08T15:47:46Z

This is done:
detector type
rename column type -> dtype
change column to this enum: CREATE TYPE detector_type AS ENUM ('feature', 'namespace');

Move dbutil and testutil to database from pkg Rename all "result"

All database utility functions are renamed to explicitly say if it will commit changes or rollback changes on success.

Rename detector type to DType because all reserved key words should be avoided used as type name or variable name.

KeyboardNerd requested a review from jzelinskie September 12, 2018 21:27

KeyboardNerd commented Sep 14, 2018

View reviewed changes

jzelinskie suggested changes Sep 17, 2018

View reviewed changes

KeyboardNerd force-pushed the feature/detector branch from ff97d47 to 3ff6ac9 Compare September 19, 2018 18:31

KeyboardNerd added 2 commits September 19, 2018 14:33

pgsql: update the query format

5343309

config: removed worker config

0609ed9

All processors will now be used to process the layers.

KeyboardNerd force-pushed the feature/detector branch 4 times, most recently from b67ac8c to 11e5efa Compare September 19, 2018 19:03

KeyboardNerd commented Sep 19, 2018

View reviewed changes

KeyboardNerd force-pushed the feature/detector branch from 3e02efa to 8938ec9 Compare September 20, 2018 19:48

KeyboardNerd commented Sep 21, 2018

View reviewed changes

KeyboardNerd force-pushed the feature/detector branch 5 times, most recently from c6d9b77 to 51a6a87 Compare September 27, 2018 22:09

KeyboardNerd changed the title ~~WIP: Associate Detector with Feature and Namespace~~ Internally version all detected content by extension Sep 28, 2018

KeyboardNerd commented Sep 28, 2018

View reviewed changes

KeyboardNerd force-pushed the feature/detector branch 2 times, most recently from 9e42bed to 4804d41 Compare September 28, 2018 16:42

KeyboardNerd commented Sep 28, 2018

View reviewed changes

v3: associate feature and namespace with detector

9f5d1ea

KeyboardNerd force-pushed the feature/detector branch from 4804d41 to 52a3e03 Compare October 1, 2018 15:09

jzelinskie suggested changes Oct 1, 2018

View reviewed changes

KeyboardNerd force-pushed the feature/detector branch from 52a3e03 to c1c8b29 Compare October 2, 2018 14:52

jzelinskie suggested changes Oct 5, 2018

View reviewed changes

KeyboardNerd added 7 commits October 8, 2018 10:42

database: Update database model and interface for detectors

db2db8b

All detected features and namespaces under the context of Layer and Ancestry will now have the detectors associated, so that the API can provide the detection information to the Client.

pgsql: Add detector to database schema

dca2d4e

'detector' table is added to store the metadata of detectors. 'layer_feature', 'layer_namespace', and 'ancestry_feature' tables are modified to store the detection relationship between the feature/namespace with the detector.

vendor: Add golang-set dependency

34d0e51

Golang-set library is added to make it easier to support set operations.

ext: Lister and Detector returns detector info with detected content

53bf19a

1. Every Lister and Detector are versioned 2. detected content, are returned in a map with detector info as the key

pgsql: Move queries to corresponding files

9c49d9d

Aggregate queries in their corresponding files instead of having the single file for every queries because the database is more complicated.

api: Add detectors for RPC

48427e9

Change the V3 implementation to accommondate the detectors.

clair: Implement worker detector support

0283240

The worker is changed to accommodate the new database model and API. Worker is refactored to move the database query helper functions to pkg.

KeyboardNerd force-pushed the feature/detector branch from c1c8b29 to 8b60a78 Compare October 8, 2018 15:20

pgsql: Implement database queries for detector relationship

0c1b80b

* Refactor layer and ancestry * Add tests * Fix bugs introduced when the queries were moved

KeyboardNerd force-pushed the feature/detector branch from 8b60a78 to b50a745 Compare October 8, 2018 15:27

database: move dbutil and testutil to database from pkg

e657d26

Move dbutil and testutil to database from pkg Rename all "result"

KeyboardNerd force-pushed the feature/detector branch from b50a745 to e657d26 Compare October 8, 2018 16:11

KeyboardNerd added 2 commits October 8, 2018 13:12

database: rename utility functions with commit/rollback

a3e9b5b

All database utility functions are renamed to explicitly say if it will commit changes or rollback changes on success.

api: Rename detector type to DType

69c0c84

Rename detector type to DType because all reserved key words should be avoided used as type name or variable name.

KeyboardNerd force-pushed the feature/detector branch from 851c8ec to 69c0c84 Compare October 8, 2018 18:34

jzelinskie approved these changes Oct 8, 2018

View reviewed changes

KeyboardNerd merged commit 3c72fa2 into quay:master Oct 8, 2018

KeyboardNerd deleted the feature/detector branch October 8, 2018 19:16

Internally version all detected content by extension #620

Internally version all detected content by extension #620

Conversation

KeyboardNerd commented Sep 12, 2018 • edited

KeyboardNerd commented Sep 12, 2018

KeyboardNerd 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KeyboardNerd left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

KeyboardNerd Oct 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KeyboardNerd Oct 2, 2018 • edited

Choose a reason for hiding this comment

jzelinskie Oct 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzelinskie Oct 2, 2018 • edited

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

jzelinskie left a comment • edited

Choose a reason for hiding this comment

Get rid of namespaced_feature

make kv table consistent

detector type

debian:unstable ns needs to be mapped to a number

we should lstrip "v" off of the alpine namespace version numbers, no other namespaces do that

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 Oct 8, 2018

KeyboardNerd commented Sep 12, 2018 •

edited

KeyboardNerd left a comment •

edited

KeyboardNerd Oct 2, 2018 •

edited

KeyboardNerd Oct 2, 2018 •

edited

jzelinskie Oct 1, 2018 •

edited

jzelinskie Oct 2, 2018 •

edited

jzelinskie left a comment •

edited