Add registrable version formats #298

jzelinskie · 2016-12-29T04:24:08Z

Version Formats are a new registrable interface for validating and comparing version strings.
Namespaces enforce that all Features use the same Version Format.

This PR adds the following formats:

dpkg
rpm

Fixes #222

jzelinskie · 2016-12-29T18:46:44Z

ext/versionfmt/rpm/parser_test.go

+		expected int
+		v2       string
+	}{
+


I need to get rid of these newlines

jzelinskie · 2016-12-29T18:47:28Z

ext/versionfmt/rpm/parser.go

+		if !unicode.IsDigit(rune(v.version[0])) {
+			return version{}, errors.New("version does not start with digit")
+		}
+	*/


I need to remove this comment.

jzelinskie · 2016-12-29T18:48:16Z

ext/versionfmt/dpkg/parser.go

+
+// newVersionUnsafe is just a wrapper around NewVersion that ignore potentiel
+// parsing error. Useful for test purposes
+func newVersionUnsafe(str string) version {


I think I can delete this func

Since we only ever used dpkg, this change shims everything into using dpkg.

This also filters unknown namespaces from the generic lsb-release and osrelease detectors.

Quentin-M · 2017-01-03T18:23:53Z

api/v1/models.go

 )

 var log = capnslog.NewPackageLogger("github.com/coreos/clair", "v1")

 type Error struct {
-	Message string `json:"Layer`
+	Message string `json:"Layer"`


Quentin-M · 2017-01-03T18:29:01Z

database/pgsql/complex_test.go

@@ -1,4 +1,4 @@
-// Copyright 2015 clair authors
+// Copyright 2016 clair authors


Should revert these and make a separated PR for 2017.

Rather than spamming the codebase, I'd rather just update them whenever the files are touched.

Quentin-M · 2017-01-03T18:30:12Z

database/pgsql/feature.go

+func (pgSQL *pgSQL) insertFeatureVersion(fv database.FeatureVersion) (id int, err error) {
+	err = versionfmt.Valid(fv.Feature.Namespace.VersionFormat, fv.Version)
+	if err != nil {
+		fmt.Println(err)


Quentin-M · 2017-01-03T18:34:32Z

database/pgsql/feature.go

-		if featureVersion.Version.Compare(affect.fixedInVersion) < 0 {
+		cmp, err := versionfmt.Compare(featureVersion.Feature.Namespace.VersionFormat, featureVersion.Version, affect.fixedInVersion)
+		if err != nil {
+			return handleError("searchVulnerabilityVersionComparison", err)


handleError is specifically for SQL errors, it does a bunch of stuff including adding entries to Prometheus. So it'll show as a pgsql error while it's really..

Quentin-M · 2017-01-03T18:46:20Z

database/pgsql/migrations/00006_add_version_format.go

+				return err
+			}
+
+			rows, err := tx.Query(`SELECT name FROM Namespace FOR UPDATE;`)


Even though I was pretty excited to use Go migrations, we could actually do it serve-side this time.

UPDATE Namespace SET version_format = 'rpm' WHERE name LIKE 'RHEL%' OR name LIKE 'centos'%' ...

With a slightly smarter query, we might even update both dpkg and rpm using a single pass.

Quentin-M · 2017-01-03T19:05:14Z

ext/versionfmt/rpm/parser_test.go

+		{"1.0~rc1~git123", LESS, "1.0~rc1"},
+		{"1.0~rc1", GREATER, "1.0~rc1~git123"},
+
+		// These are included here to document current, arguably buggy behaviors


Add note that these are imported as is from the rpm sources (arguably buggy = upstream)

Quentin-M · 2017-01-03T19:06:39Z

updater/fetchers/alpine/alpine.go

@@ -235,7 +239,7 @@ func parse33YAML(r io.Reader) (vulns []database.Vulnerability, err error) {
 							Namespace: database.Namespace{Name: "alpine:" + file.Distro},


Missing VersionFormat? Do we need it?

Also in parse34YAML, debian, etc.

Quentin-M · 2017-01-03T19:09:47Z

updater/fetchers/oracle/oracle_test.go

 				},
-				Version: types.NewVersionUnsafe("3.1.1-7.el7_1"),
+				Version: "0:3.1.1-7.el7_1",


Why changing the test? The 0 epoch is optional. Can do some tests with but some without but we should ensure it still works without.

Quentin-M · 2017-01-03T19:10:54Z

updater/fetchers/rhel/rhel_test.go

 				},
-				Version: types.NewVersionUnsafe("3.1.1-7.el7_1"),
+				Version: "0:3.1.1-7.el7_1",


Same as before?

The problem here is that we're comparing the objects with testify. It's doing a straight up string comparison. I think it's best to leave the epoch on because that's how it was presented in the source.

Quentin-M · 2017-01-03T19:12:37Z

worker/detectors/feature/dpkg/dpkg.go

@@ -74,28 +78,34 @@ func (detector *DpkgFeaturesDetector) Detect(data map[string][]byte) ([]database

 			pkg.Feature.Name = md["name"]
 			if md["version"] != "" {
-				pkg.Version, err = types.NewVersion(md["version"])
+				version := md["version"]
+				err = versionfmt.Valid("dpkg", version)


Seems we hardcore "dpkg" and "rpm" everyone. Should it be a constant in the parser?

Quentin-M · 2017-01-03T22:00:12Z

Other than https://github.com/coreos/clair/pull/298/files/8bedd0a3670dfe2bc60d3b750c981441f91d32c8#r94456926, LGTM.

jzelinskie added component/database kind/bug things are not as they seem kind/design relates to the fundamental design of a component labels Dec 29, 2016

jzelinskie requested a review from Quentin-M December 29, 2016 04:24

jzelinskie commented Dec 29, 2016

View reviewed changes

ext/versionfmt/rpm/parser_test.go

expected int

v2 string

}{

Copy link

Contributor Author

jzelinskie Dec 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to get rid of these newlines

jzelinskie commented Dec 29, 2016

View reviewed changes

jzelinskie added 4 commits December 30, 2016 12:51

add registerable version formats

033709e

Since we only ever used dpkg, this change shims everything into using dpkg.

versionfmt: init rpm versionfmt

6864a8e

backfill version_format column

9e39a26

worker: ns detectors now support VersionFormat

8bedd0a

This also filters unknown namespaces from the generic lsb-release and osrelease detectors.

Quentin-M suggested changes Jan 3, 2017

View reviewed changes

jzelinskie added 3 commits January 3, 2017 15:59

db/pgsql/migration: convert to pure SQL

8df8170

api/v1: fix JSON struct tag misnomer

ebd0170

versionfmt: convert to using constant over literal

8d29bf8

db/pgsql/feature: fix SQL error reporting

627b98e

Quentin-M approved these changes Jan 3, 2017

View reviewed changes

jzelinskie merged commit ab33f8c into quay:master Jan 3, 2017

jzelinskie deleted the versions branch January 3, 2017 22:10

Djelibeybi mentioned this pull request Jan 11, 2017

Dealing with non deb-version version schemes #294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add registrable version formats #298

Add registrable version formats #298

jzelinskie commented Dec 29, 2016 •

edited

jzelinskie Dec 29, 2016

jzelinskie Dec 29, 2016

jzelinskie Dec 29, 2016

Quentin-M Jan 3, 2017

Quentin-M Jan 3, 2017

jzelinskie Jan 3, 2017

Quentin-M Jan 3, 2017

Quentin-M Jan 3, 2017

Quentin-M Jan 3, 2017 •

edited

Quentin-M Jan 3, 2017

Quentin-M Jan 3, 2017

Quentin-M Jan 3, 2017

Quentin-M Jan 3, 2017

Quentin-M Jan 3, 2017

Quentin-M Jan 3, 2017

Quentin-M Jan 3, 2017

jzelinskie Jan 3, 2017

Quentin-M Jan 3, 2017

Quentin-M Jan 3, 2017

Quentin-M commented Jan 3, 2017

		@@ -1,4 +1,4 @@
		// Copyright 2015 clair authors
		// Copyright 2016 clair authors

		@@ -235,7 +239,7 @@ func parse33YAML(r io.Reader) (vulns []database.Vulnerability, err error) {
		Namespace: database.Namespace{Name: "alpine:" + file.Distro},

Add registrable version formats #298

Add registrable version formats #298

Conversation

jzelinskie commented Dec 29, 2016 • 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

Quentin-M Jan 3, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quentin-M commented Jan 3, 2017

jzelinskie commented Dec 29, 2016 •

edited

Quentin-M Jan 3, 2017 •

edited