Merge pull request #418 from KeyboardNerd/multiplens

use namespace's versionfmt to specify listers scanning features

ext/featurefmt/apk/apk.go

@@ -29,7 +29,7 @@ import (
 )
 
 func init() {
-	featurefmt.RegisterLister("apk", &lister{})
+	featurefmt.RegisterLister("apk", dpkg.ParserName, &lister{})
 }
 
 type lister struct{}

ext/featurefmt/dpkg/dpkg.go

@@ -37,7 +37,7 @@ var (
 type lister struct{}
 
 func init() {
-	featurefmt.RegisterLister("dpkg", &lister{})
+	featurefmt.RegisterLister("dpkg", dpkg.ParserName, &lister{})
 }
 
 func (l lister) ListFeatures(files tarutil.FilesMap) ([]database.FeatureVersion, error) {

ext/featurefmt/driver.go

@@ -23,14 +23,17 @@ import (
 	"sync"
 	"testing"
 
+	log "github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/assert"
+
 	"github.com/coreos/clair/database"
 	"github.com/coreos/clair/pkg/tarutil"
-	"github.com/stretchr/testify/assert"
 )
 
 var (
-	listersM sync.RWMutex
-	listers  = make(map[string]Lister)
+	listersM             sync.RWMutex
+	listers              = make(map[string]Lister)
+	versionfmtListerName = make(map[string][]string)
 )
 
 // Lister represents an ability to list the features present in an image layer.
@@ -49,7 +52,7 @@ type Lister interface {
 //
 // If called twice with the same name, the name is blank, or if the provided
 // Lister is nil, this function panics.
-func RegisterLister(name string, l Lister) {
+func RegisterLister(name string, versionfmt string, l Lister) {
 	if name == "" {
 		panic("featurefmt: could not register a Lister with an empty name")
 	}
@@ -65,19 +68,35 @@ func RegisterLister(name string, l Lister) {
 	}
 
 	listers[name] = l
+	versionfmtListerName[versionfmt] = append(versionfmtListerName[versionfmt], name)
 }
 
 // ListFeatures produces the list of FeatureVersions in an image layer using
 // every registered Lister.
-func ListFeatures(files tarutil.FilesMap) ([]database.FeatureVersion, error) {
+func ListFeatures(files tarutil.FilesMap, namespace *database.Namespace) ([]database.FeatureVersion, error) {
 	listersM.RLock()
 	defer listersM.RUnlock()
 
-	var totalFeatures []database.FeatureVersion
-	for _, lister := range listers {
-		features, err := lister.ListFeatures(files)
+	var (
+		totalFeatures []database.FeatureVersion
+		listersName   []string
+		found         bool
+	)
+
+	if namespace == nil {
+		log.Debug("Can't detect features without namespace")
+		return totalFeatures, nil
+	}
+
+	if listersName, found = versionfmtListerName[namespace.VersionFormat]; !found {
+		log.WithFields(log.Fields{"namespace": namespace.Name, "version format": namespace.VersionFormat}).Debug("Unsupported Namespace")
+		return totalFeatures, nil
+	}
+
+	for _, listerName := range listersName {
+		features, err := listers[listerName].ListFeatures(files)
 		if err != nil {
-			return []database.FeatureVersion{}, err
+			return totalFeatures, err
 		}
 		totalFeatures = append(totalFeatures, features...)
 	}

ext/featurefmt/rpm/rpm.go

@@ -35,7 +35,7 @@ import (
 type lister struct{}
 
 func init() {
-	featurefmt.RegisterLister("rpm", &lister{})
+	featurefmt.RegisterLister("rpm", rpm.ParserName, &lister{})
 }
 
 func (l lister) ListFeatures(files tarutil.FilesMap) ([]database.FeatureVersion, error) {

ext/featurens/driver.go

@@ -72,7 +72,7 @@ func RegisterDetector(name string, d Detector) {
 func Detect(files tarutil.FilesMap) ([]database.Namespace, error) {
 	detectorsM.RLock()
 	defer detectorsM.RUnlock()
-	var namespaces []database.Namespace
+	namespaces := map[string]*database.Namespace{}
 	for name, detector := range detectors {
 		namespace, err := detector.Detect(files)
 		if err != nil {
@@ -82,11 +82,15 @@ func Detect(files tarutil.FilesMap) ([]database.Namespace, error) {
 
 		if namespace != nil {
 			log.WithFields(log.Fields{"name": name, "namespace": namespace.Name}).Debug("detected namespace")
-			namespaces = append(namespaces, *namespace)
+			namespaces[namespace.Name] = namespace
 		}
 	}
 
-	return namespaces, nil
+	nslist := []database.Namespace{}
+	for _, ns := range namespaces {
+		nslist = append(nslist, *ns)
+	}
+	return nslist, nil
 }
 
 // RequiredFilenames returns the total list of files required for all

worker.go

@@ -128,16 +128,11 @@ func detectContent(imageFormat, name, path string, headers map[string]string, pa
 		return
 	}
 
-	// Detect features.
-	var fv []database.FeatureVersion
-	// detect feature versions in all namespaces
-	for _, namespace := range namespaces {
-		fv, err = detectFeatureVersions(name, files, &namespace, parent)
-		if err != nil {
-			return
-		}
-		featureVersions = append(featureVersions, fv...)
+	featureVersions, err = detectFeatureVersions(name, files, namespaces, parent)
+	if err != nil {
+		return
 	}
+
 	if len(featureVersions) > 0 {
 		log.WithFields(log.Fields{logLayerName: name, "feature count": len(featureVersions)}).Debug("detected features")
 	}
@@ -153,43 +148,27 @@ func detectNamespaces(name string, files tarutil.FilesMap, parent *database.Laye
 		return
 	}
 
-	for _, ns := range nsCurrent {
-		nsSet[ns.Name] = &ns
-		log.WithFields(log.Fields{logLayerName: name, "detected namespace": ns.Name}).Debug("detected namespace")
-	}
-
 	if parent != nil {
 		for _, ns := range parent.Namespaces {
-			nsSet[ns.Name] = &ns
-			log.WithFields(log.Fields{logLayerName: name, "detected namespace": ns.Name}).Debug("detected namespace (from parent)")
+			// Under assumption that one version format corresponds to one type
+			// of namespace.
+			nsSet[ns.VersionFormat] = &ns
+			log.WithFields(log.Fields{logLayerName: name, "detected namespace": ns.Name, "version format": ns.VersionFormat}).Debug("detected namespace (from parent)")
 		}
 	}
 
+	for _, ns := range nsCurrent {
+		nsSet[ns.VersionFormat] = &ns
+		log.WithFields(log.Fields{logLayerName: name, "detected namespace": ns.Name, "version format": ns.VersionFormat}).Debug("detected namespace")
+	}
+
 	for _, ns := range nsSet {
 		namespaces = append(namespaces, *ns)
 	}
 	return
 }
 
-func detectFeatureVersions(name string, files tarutil.FilesMap, namespace *database.Namespace, parent *database.Layer) (features []database.FeatureVersion, err error) {
-	// TODO(Quentin-M): We need to pass the parent image to DetectFeatures because it's possible that
-	// some detectors would need it in order to produce the entire feature list (if they can only
-	// detect a diff). Also, we should probably pass the detected namespace so detectors could
-	// make their own decision.
-	features, err = featurefmt.ListFeatures(files)
-	if err != nil {
-		return
-	}
-
-	// If there are no FeatureVersions, use parent's FeatureVersions if possible.
-	// TODO(Quentin-M): We eventually want to give the choice to each detectors to use none/some of
-	// their parent's FeatureVersions. It would be useful for detectors that can't find their entire
-	// result using one Layer.
-	if len(features) == 0 && parent != nil {
-		features = parent.Features
-		return
-	}
-
+func detectFeatureVersions(name string, files tarutil.FilesMap, namespaces []database.Namespace, parent *database.Layer) (features []database.FeatureVersion, err error) {
 	// Build a map of the namespaces for each FeatureVersion in our parent layer.
 	parentFeatureNamespaces := make(map[string]database.Namespace)
 	if parent != nil {
@@ -198,28 +177,44 @@ func detectFeatureVersions(name string, files tarutil.FilesMap, namespace *datab
 		}
 	}
 
-	// Ensure that each FeatureVersion has an associated Namespace.
-	for i, feature := range features {
-		if feature.Feature.Namespace.Name != "" {
-			// There is a Namespace associated.
-			continue
+	for _, ns := range namespaces {
+		// TODO(Quentin-M): We need to pass the parent image to DetectFeatures because it's possible that
+		// some detectors would need it in order to produce the entire feature list (if they can only
+		// detect a diff). Also, we should probably pass the detected namespace so detectors could
+		// make their own decision.
+		detectedFeatures, err := featurefmt.ListFeatures(files, &ns)
+		if err != nil {
+			return features, err
 		}
 
-		if parentFeatureNamespace, ok := parentFeatureNamespaces[feature.Feature.Name+":"+feature.Version]; ok {
-			// The FeatureVersion is present in the parent layer; associate with their Namespace.
-			features[i].Feature.Namespace = parentFeatureNamespace
-			continue
-		}
+		// Ensure that each FeatureVersion has an associated Namespace.
+		for i, feature := range detectedFeatures {
+			if feature.Feature.Namespace.Name != "" {
+				// There is a Namespace associated.
+				continue
+			}
+
+			if parentFeatureNamespace, ok := parentFeatureNamespaces[feature.Feature.Name+":"+feature.Version]; ok {
+				// The FeatureVersion is present in the parent layer; associate
+				// with their Namespace.
+				// This might cause problem because a package  with same feature
+				// name and version could be different in parent layer's
+				// namespace and current layer's namespace
+				detectedFeatures[i].Feature.Namespace = parentFeatureNamespace
+				continue
+			}
 
-		if namespace != nil {
-			// The Namespace has been detected in this layer; associate it.
-			features[i].Feature.Namespace = *namespace
-			continue
+			detectedFeatures[i].Feature.Namespace = ns
 		}
+		features = append(features, detectedFeatures...)
+	}
 
-		log.WithFields(log.Fields{"feature name": feature.Feature.Name, "feature version": feature.Version, logLayerName: name}).Warning("Namespace unknown")
-		err = ErrUnsupported
-		return
+	// If there are no FeatureVersions, use parent's FeatureVersions if possible.
+	// TODO(Quentin-M): We eventually want to give the choice to each detectors to use none/some of
+	// their parent's FeatureVersions. It would be useful for detectors that can't find their entire
+	// result using one Layer.
+	if len(features) == 0 && parent != nil {
+		features = parent.Features
 	}
 
 	return

worker_test.go

@@ -101,16 +101,9 @@ func TestProcessWithDistUpgrade(t *testing.T) {
 	// Ensure that the 'wheezy' layer has the expected namespace and non-upgraded features.
 	jessie, ok := datastore.layers["jessie"]
 	if assert.True(t, ok, "layer 'jessie' not processed") {
-		if !assert.Len(t, jessie.Namespaces, 2) {
-			return
-		}
-		nsNames := []string{jessie.Namespaces[0].Name, jessie.Namespaces[1].Name}
-		// the old features and namespace should remain here with only the features in new namespaces detected
-		assert.Contains(t, nsNames, "debian:8")
-		assert.Contains(t, nsNames, "debian:7")
-
-		// because the featurefmt detects the features under "debian:8" and "debian:7", therefore the number of features is duplicated
-		assert.Len(t, jessie.Features, 148)
+		assert.Len(t, jessie.Namespaces, 1)
+		assert.Equal(t, "debian:8", jessie.Namespaces[0].Name)
+		assert.Len(t, jessie.Features, 74)
 
 		for _, nufv := range nonUpgradedFeatureVersions {
 			nufv.Feature.Namespace.Name = "debian:7"