From 970756cd5a8364b4912f99859c7626d4db7f97b6 Mon Sep 17 00:00:00 2001 From: Quentin Machu Date: Fri, 8 Jan 2016 10:27:30 -0500 Subject: [PATCH] database: do insert/find layers (with their features and vulnerabilities) --- database/pgsql/feature.go | 56 ++++++++++++------ database/pgsql/layer.go | 112 +++++++++++++++++++++++++++++------ database/pgsql/layer_test.go | 23 ++++--- database/pgsql/pgsql.go | 13 ---- database/pgsql/queries.go | 52 ++++++++++++---- worker/worker.go | 8 +++ 6 files changed, 195 insertions(+), 69 deletions(-) diff --git a/database/pgsql/feature.go b/database/pgsql/feature.go index 5889e553..6d779533 100644 --- a/database/pgsql/feature.go +++ b/database/pgsql/feature.go @@ -2,11 +2,11 @@ package pgsql import ( "github.com/coreos/clair/database" + cerrors "github.com/coreos/clair/utils/errors" "github.com/coreos/clair/utils/types" - cerrors "github.com/coreos/clair/utils/errors" ) -func (pgSQL *pgSQL) insertFeature(feature database.Feature) (id int, err error) { +func (pgSQL *pgSQL) insertFeature(feature database.Feature) (int, error) { if feature.Name == "" { return 0, cerrors.NewBadRequestError("could not find/insert invalid Feature") } @@ -20,25 +20,29 @@ func (pgSQL *pgSQL) insertFeature(feature database.Feature) (id int, err error) // Find or create Namespace. namespaceID, err := pgSQL.insertNamespace(feature.Namespace) if err != nil { - return -1, err + return 0, err } // Find or create Feature. + var id int err = pgSQL.QueryRow(getQuery("soi_feature"), feature.Name, namespaceID).Scan(&id) + if err != nil { + return 0, err + } if pgSQL.cache != nil { pgSQL.cache.Add("feature:"+feature.Name, id) } - return + return id, nil } func (pgSQL *pgSQL) insertFeatureVersion(featureVersion database.FeatureVersion) (id int, err error) { - if featureVersion.Version.String() == "" { - return 0, cerrors.NewBadRequestError("could not find/insert invalid FeatureVersion") - } + if featureVersion.Version.String() == "" { + return 0, cerrors.NewBadRequestError("could not find/insert invalid FeatureVersion") + } - if pgSQL.cache != nil { + if pgSQL.cache != nil { if id, found := pgSQL.cache.Get("featureversion:" + featureVersion.Feature.Name + ":" + featureVersion.Version.String()); found { return id.(int), nil @@ -48,23 +52,23 @@ func (pgSQL *pgSQL) insertFeatureVersion(featureVersion database.FeatureVersion) // Find or create Feature first. featureID, err := pgSQL.insertFeature(featureVersion.Feature) if err != nil { - return -1, err + return 0, err } // Begin transaction. tx, err := pgSQL.Begin() if err != nil { tx.Rollback() - return -1, err + return 0, err } // Find or create FeatureVersion. var newOrExisting string - err = tx.QueryRow(getQuery("soi_featureversion"), featureID, featureVersion.Version). + err = tx.QueryRow(getQuery("soi_featureversion"), featureID, &featureVersion.Version). Scan(&newOrExisting, &featureVersion.ID) if err != nil { tx.Rollback() - return -1, err + return 0, err } if newOrExisting == "exi" { // That featureVersion already exists, return its id. @@ -79,14 +83,14 @@ func (pgSQL *pgSQL) insertFeatureVersion(featureVersion database.FeatureVersion) _, err = tx.Exec(getQuery("l_share_vulnerability_fixedin_feature")) if err != nil { tx.Rollback() - return -1, err + return 0, err } // Select every vulnerability and the fixed version that affect this Feature. rows, err := tx.Query(getQuery("s_vulnerability_fixedin_feature"), featureID) if err != nil { tx.Rollback() - return -1, err + return 0, err } defer rows.Close() @@ -96,17 +100,18 @@ func (pgSQL *pgSQL) insertFeatureVersion(featureVersion database.FeatureVersion) err := rows.Scan(&fixedInID, &vulnerabilityID, &fixedInVersion) if err != nil { tx.Rollback() - return -1, err + return 0, err } if featureVersion.Version.Compare(fixedInVersion) < 0 { // The version of the FeatureVersion we are inserting is lower than the fixed version on this // Vulnerability, thus, this FeatureVersion is affected by it. + // TODO(Quentin-M): Prepare. _, err := tx.Exec(getQuery("i_vulnerability_affects_featureversion"), vulnerabilityID, featureVersion.ID, fixedInID) if err != nil { tx.Rollback() - return -1, err + return 0, err } } } @@ -115,7 +120,7 @@ func (pgSQL *pgSQL) insertFeatureVersion(featureVersion database.FeatureVersion) err = tx.Commit() if err != nil { tx.Rollback() - return -1, err + return 0, err } if pgSQL.cache != nil { @@ -123,5 +128,20 @@ func (pgSQL *pgSQL) insertFeatureVersion(featureVersion database.FeatureVersion) featureVersion.Version.String(), featureVersion.ID) } - return 0, nil + return featureVersion.ID, nil +} + +// TODO(Quentin-M): Batch me +func (pgSQL *pgSQL) insertFeatureVersions(featureVersions []database.FeatureVersion) ([]int, error) { + IDs := make([]int, 0, len(featureVersions)) + + for i := 0; i < len(featureVersions); i++ { + id, err := pgSQL.insertFeatureVersion(featureVersions[i]) + if err != nil { + return IDs, err + } + IDs = append(IDs, id) + } + + return IDs, nil } diff --git a/database/pgsql/layer.go b/database/pgsql/layer.go index 93602dfb..361c8681 100644 --- a/database/pgsql/layer.go +++ b/database/pgsql/layer.go @@ -4,6 +4,7 @@ import ( "database/sql" "github.com/coreos/clair/database" + "github.com/coreos/clair/utils" cerrors "github.com/coreos/clair/utils/errors" "github.com/guregu/null/zero" ) @@ -11,11 +12,14 @@ import ( func (pgSQL *pgSQL) FindLayer(name string, withFeatures, withVulnerabilities bool) (database.Layer, error) { // Find the layer var layer database.Layer - var parentName sql.NullString + var parentID zero.Int + var parentName zero.String + var namespaceID zero.Int var namespaceName sql.NullString err := pgSQL.QueryRow(getQuery("s_layer"), name). - Scan(&layer.ID, &layer.Name, &layer.EngineVersion, &parentName, &namespaceName) + Scan(&layer.ID, &layer.Name, &layer.EngineVersion, &parentID, &parentName, &namespaceID, + &namespaceName) if err == sql.ErrNoRows { return layer, cerrors.ErrNotFound @@ -24,11 +28,17 @@ func (pgSQL *pgSQL) FindLayer(name string, withFeatures, withVulnerabilities boo return layer, err } - if parentName.Valid { - layer.Parent = &database.Layer{Name: parentName.String} + if !parentID.IsZero() { + layer.Parent = &database.Layer{ + Model: database.Model{ID: int(parentID.Int64)}, + Name: parentName.String, + } } - if namespaceName.Valid { - layer.Namespace = &database.Namespace{Name: namespaceName.String} + if !namespaceID.IsZero() { + layer.Namespace = &database.Namespace{ + Model: database.Model{ID: int(namespaceID.Int64)}, + Name: namespaceName.String, + } } // Find its features @@ -210,6 +220,11 @@ func (pgSQL *pgSQL) InsertLayer(layer database.Layer) error { return err } namespaceID = zero.IntFrom(int64(n)) + } else if layer.Namespace == nil && layer.Parent != nil { + // Import the Namespace from the parent if it has one and this layer doesn't specify one. + if layer.Parent.Namespace != nil { + namespaceID = zero.IntFrom(int64(layer.Parent.Namespace.ID)) + } } if layer.ID == 0 { @@ -222,11 +237,6 @@ func (pgSQL *pgSQL) InsertLayer(layer database.Layer) error { } parentID = zero.IntFrom(int64(layer.Parent.ID)) - - // Import the Namespace from the parent is this layer doesn't specify one. - if zero.IsNull(namespaceID) { - namespaceID = zero.IntFrom(int64(layer.Parent.Namespace.ID)) - } } err = tx.QueryRow(getQuery("i_layer"), layer.Name, layer.EngineVersion, parentID, namespaceID). @@ -247,10 +257,20 @@ func (pgSQL *pgSQL) InsertLayer(layer database.Layer) error { tx.Rollback() return err } + + // Remove all existing Layer_diff_FeatureVersion. + _, err = tx.Exec(getQuery("r_layer_diff_featureversion"), layer.ID) + if err != nil { + tx.Rollback() + return err + } } // Update Layer_diff_FeatureVersion now. - updateDiffFeatureVersions(tx, &layer, &existingLayer) + err = pgSQL.updateDiffFeatureVersions(tx, &layer, &existingLayer) + if err != nil { + return err + } // Commit transaction. err = tx.Commit() @@ -262,19 +282,73 @@ func (pgSQL *pgSQL) InsertLayer(layer database.Layer) error { return nil } -func updateDiffFeatureVersions(tx *sql.Tx, layer, existingLayer *database.Layer) { - // TODO - - if existingLayer != nil { - // We are updating a layer, we need to diff the Features with the existing Layer. +func (pgSQL *pgSQL) updateDiffFeatureVersions(tx *sql.Tx, layer, existingLayer *database.Layer) error { + // add and del are the FeatureVersion diff we should insert. + var add []database.FeatureVersion + var del []database.FeatureVersion - } else if layer.Parent == nil { + if layer.Parent == nil { // There is no parent, every Features are added. - + add = append(add, layer.Features...) } else if layer.Parent != nil { // There is a parent, we need to diff the Features with it. + // Build name:version strctures. + layerFeaturesMapNV, layerFeaturesNV := createNV(layer.Features) + parentLayerFeaturesMapNV, parentLayerFeaturesNV := createNV(layer.Parent.Features) + + // Calculate the added and deleted FeatureVersions name:version. + addNV := utils.CompareStringLists(layerFeaturesNV, parentLayerFeaturesNV) + delNV := utils.CompareStringLists(parentLayerFeaturesNV, layerFeaturesNV) + + // Fill the structures containing the added and deleted FeatureVersions + for _, nv := range addNV { + add = append(add, *layerFeaturesMapNV[nv]) + } + for _, nv := range delNV { + del = append(del, *parentLayerFeaturesMapNV[nv]) + } + } + + // Insert FeatureVersions in the database. + addIDs, err := pgSQL.insertFeatureVersions(add) + if err != nil { + return err + } + delIDs, err := pgSQL.insertFeatureVersions(del) + if err != nil { + return err + } + + // Insert diff in the database. + if len(addIDs) > 0 { + _, err = tx.Exec(getQuery("i_layer_diff_featureversion"), layer.ID, "add", buildInputArray(addIDs)) + if err != nil { + return err + } + } + if len(delIDs) > 0 { + _, err = tx.Exec(getQuery("i_layer_diff_featureversion"), layer.ID, "del", buildInputArray(delIDs)) + if err != nil { + return err + } + } + + return nil +} + +func createNV(features []database.FeatureVersion) (map[string]*database.FeatureVersion, []string) { + mapNV := make(map[string]*database.FeatureVersion, 0) + sliceNV := make([]string, 0, len(features)) + + for i := 0; i < len(features); i++ { + featureVersion := &features[i] + nv := featureVersion.Feature.Name + ":" + featureVersion.Version.String() + mapNV[nv] = featureVersion + sliceNV = append(sliceNV, nv) } + + return mapNV, sliceNV } func (pgSQL *pgSQL) DeleteLayer(name string) error { diff --git a/database/pgsql/layer_test.go b/database/pgsql/layer_test.go index 7c9622df..75fbef29 100644 --- a/database/pgsql/layer_test.go +++ b/database/pgsql/layer_test.go @@ -121,6 +121,8 @@ func testInsertLayerInvalid(t *testing.T, datastore database.Datastore) { } func testInsertLayerTree(t *testing.T, datastore database.Datastore) { + fmt.Println("- testInsertLayerTree") + f1 := database.FeatureVersion{ Feature: database.Feature{ Namespace: database.Namespace{Name: "TestInsertLayerNamespace2"}, @@ -152,7 +154,7 @@ func testInsertLayerTree(t *testing.T, datastore database.Datastore) { f5 := database.FeatureVersion{ Feature: database.Feature{ Namespace: database.Namespace{Name: "TestInsertLayerNamespace3"}, - Name: "TestInsertLayerFeature2", + Name: "TestInsertLayerFeature3", }, Version: types.NewVersionUnsafe("0.57"), } @@ -180,10 +182,11 @@ func testInsertLayerTree(t *testing.T, datastore database.Datastore) { Namespace: &database.Namespace{Name: "TestInsertLayerNamespace2"}, Features: []database.FeatureVersion{f1, f2, f3}, }, - // This layer covers the case where the last layer doesn't provide any Feature. + // This layer covers the case where the last layer doesn't provide any new Feature. database.Layer{ - Name: "TestInsertLayer4a", - Parent: &database.Layer{Name: "TestInsertLayer3"}, + Name: "TestInsertLayer4a", + Parent: &database.Layer{Name: "TestInsertLayer3"}, + Features: []database.FeatureVersion{f1, f2, f3}, }, // This layer covers the case where the last layer provides Features. // It also modifies the Namespace ("upgrade") but keeps some Features not upgraded, their @@ -221,7 +224,9 @@ func testInsertLayerTree(t *testing.T, datastore database.Datastore) { } l4a := retrievedLayers["TestInsertLayer4a"] - assert.Equal(t, "TestInsertLayerNamespace2", l4a.Namespace.Name) + if assert.NotNil(t, l4a.Namespace) { + assert.Equal(t, "TestInsertLayerNamespace2", l4a.Namespace.Name) + } assert.Len(t, l4a.Features, 3) for _, featureVersion := range l4a.Features { if cmpFV(featureVersion, f1) && cmpFV(featureVersion, f2) && cmpFV(featureVersion, f3) { @@ -230,9 +235,11 @@ func testInsertLayerTree(t *testing.T, datastore database.Datastore) { } l4b := retrievedLayers["TestInsertLayer4b"] - assert.Equal(t, "TestInsertLayerNamespace3", l4a.Namespace.Name) - assert.Len(t, l4a.Features, 3) - for _, featureVersion := range l4a.Features { + if assert.NotNil(t, l4b.Namespace) { + assert.Equal(t, "TestInsertLayerNamespace3", l4b.Namespace.Name) + } + assert.Len(t, l4b.Features, 3) + for _, featureVersion := range l4b.Features { if cmpFV(featureVersion, f2) && cmpFV(featureVersion, f5) && cmpFV(featureVersion, f6) { assert.Error(t, fmt.Errorf("TestInsertLayer4a contains an unexpected package: %#v. Should contain %#v and %#v and %#v.", featureVersion, f2, f4, f6)) } diff --git a/database/pgsql/pgsql.go b/database/pgsql/pgsql.go index 84dff78d..de98b3a9 100644 --- a/database/pgsql/pgsql.go +++ b/database/pgsql/pgsql.go @@ -6,7 +6,6 @@ import ( "io/ioutil" "path" "runtime" - "strconv" "strings" "bitbucket.org/liamstask/goose/lib/goose" @@ -171,18 +170,6 @@ func OpenForTest(name string, withTestData bool) (*pgSQLTest, error) { return &pgSQLTest{pgSQL: db.(*pgSQL), dataSource: dataSource, dbName: dbName}, nil } -// buildInputArray constructs a PostgreSQL input array from the specified integers. -// Useful to use the `= ANY($1::integer[])` syntax that let us use a IN clause while using -// a single placeholder. -func buildInputArray(ints []int) string { - str := "{" - for i := 0; i < len(ints)-1; i++ { - str = str + strconv.Itoa(ints[i]) + "," - } - str = str + strconv.Itoa(ints[len(ints)-1]) + "}" - return str -} - // isErrUniqueViolation determines is the given error is a unique contraint violation. func isErrUniqueViolation(err error) bool { pqErr, ok := err.(*pq.Error) diff --git a/database/pgsql/queries.go b/database/pgsql/queries.go index 9d1fa675..05d677f5 100644 --- a/database/pgsql/queries.go +++ b/database/pgsql/queries.go @@ -1,6 +1,9 @@ package pgsql -import "fmt" +import ( + "fmt" + "strconv" +) var queries map[string]string @@ -28,7 +31,7 @@ func init() { queries["soi_feature"] = ` WITH new_feature AS ( INSERT INTO Feature(name, namespace_id) - SELECT CAST($1 AS VARCHAR), CAST($2 AS VARCHAR) + SELECT CAST($1 AS VARCHAR), CAST($2 AS INTEGER) WHERE NOT EXISTS (SELECT id FROM Feature WHERE name = $1 AND namespace_id = $2) RETURNING id ) @@ -36,23 +39,25 @@ func init() { UNION SELECT id FROM new_feature` - queries["l_share_vulnerability_fixedin_feature"] = `LOCK Vulnerability_FixedIn_Feature IN SHARE MODE` + queries["l_share_vulnerability_fixedin_feature"] = ` + LOCK Vulnerability_FixedIn_Feature IN SHARE MODE + ` queries["soi_featureversion"] = ` WITH new_featureversion AS ( INSERT INTO FeatureVersion(feature_id, version) - SELECT CAST($1 AS VARCHAR), CAST($2 AS VARCHAR) - WHERE NOT EXISTS (SELECT id FROM Feature WHERE feature_id = $1 AND version = $2) + SELECT CAST($1 AS INTEGER), CAST($2 AS VARCHAR) + WHERE NOT EXISTS (SELECT id FROM FeatureVersion WHERE feature_id = $1 AND version = $2) RETURNING id ) - SELECT 'exi', id FROM Feature WHERE feature_id = $1 AND version = $2 + SELECT 'exi', id FROM FeatureVersion WHERE feature_id = $1 AND version = $2 UNION SELECT 'new', id FROM new_featureversion ` queries["s_vulnerability_fixedin_feature"] = ` SELECT id, vulnerability_id, version FROM Vulnerability_FixedIn_Feature - WHERE feature_id = ?` + WHERE feature_id = $1` queries["i_vulnerability_affects_featureversion"] = ` INSERT INTO Vulnerability_Affects_FeatureVersion(vulnerability_id, @@ -60,7 +65,7 @@ func init() { // layer.go queries["s_layer"] = ` - SELECT l.id, l.name, l.engineversion, p.name, n.name + SELECT l.id, l.name, l.engineversion, p.id, p.name, n.id, n.name FROM Layer l LEFT JOIN Layer p ON l.parent_id = p.id LEFT JOIN Namespace n ON l.namespace_id = n.id @@ -102,7 +107,8 @@ func init() { ORDER BY ltree.ordering` queries["s_featureversions_vulnerabilities"] = ` - SELECT vafv.featureversion_id, v.id, v.name, v.description, v.link, v.severity, vn.name, vfif.version + SELECT vafv.featureversion_id, v.id, v.name, v.description, v.link, v.severity, vn.name, + vfif.version FROM Vulnerability_Affects_FeatureVersion vafv, Vulnerability v, Namespace vn, Vulnerability_FixedIn_Feature vfif WHERE vafv.featureversion_id = ANY($1::integer[]) @@ -110,9 +116,21 @@ func init() { AND vafv.fixedin_id = vfif.id AND v.namespace_id = vn.id` - queries["i_layer"] = `INSERT INTO Layer(name, engine_version, parent_id, namespace_id) VALUES($1, $2, $3, $4) RETURNING id` + queries["i_layer"] = ` + INSERT INTO Layer(name, engineversion, parent_id, namespace_id) + VALUES($1, $2, $3, $4) RETURNING id` + + queries["u_layer"] = `UPDATE LAYER SET engineversion = $2, namespace_id = $3) WHERE id = $1` + + queries["r_layer_diff_featureversion"] = ` + DELETE FROM Layer_diff_FeatureVersion + WHERE layer_id = $1` - queries["u_layer"] = `UPDATE LAYER SET engine_version = $2, namespace_id = $3) WHERE id = $1` + queries["i_layer_diff_featureversion"] = ` + INSERT INTO Layer_diff_FeatureVersion(layer_id, featureversion_id, modification) + SELECT $1, fv.id, $2 + FROM FeatureVersion fv + WHERE fv.id = ANY($3::integer[])` } func getQuery(name string) string { @@ -121,3 +139,15 @@ func getQuery(name string) string { } panic(fmt.Sprintf("pgsql: unknown query %v", name)) } + +// buildInputArray constructs a PostgreSQL input array from the specified integers. +// Useful to use the `= ANY($1::integer[])` syntax that let us use a IN clause while using +// a single placeholder. +func buildInputArray(ints []int) string { + str := "{" + for i := 0; i < len(ints)-1; i++ { + str = str + strconv.Itoa(ints[i]) + "," + } + str = str + strconv.Itoa(ints[len(ints)-1]) + "}" + return str +} diff --git a/worker/worker.go b/worker/worker.go index 1a9140d1..260169e1 100644 --- a/worker/worker.go +++ b/worker/worker.go @@ -156,6 +156,14 @@ func detectContent(name, path, imageFormat string, parent *database.Layer) (name return } + // If there are no feature detected, use parent's features if possible. + // TODO(Quentin-M): We eventually want to give the choice to each detectors to use none/some + // parent's Features. It would be useful for dectectors that can't find their entire result using + // one Layer. + if len(features) == 0 && parent != nil { + features = parent.Features + } + log.Debugf("layer %s: detected %d features", name, len(features)) return }