From 9561d623c29394dddca0823721d7d3622b3dec65 Mon Sep 17 00:00:00 2001 From: Sida Chen Date: Thu, 22 Jun 2017 14:01:41 -0400 Subject: [PATCH] featurefmt: use namespace's versionfmt to specify listers use namespace's versionfmt to specify listers used to scan features the content detection functions are changed accordingly in worker --- ext/featurefmt/apk/apk.go | 2 +- ext/featurefmt/dpkg/dpkg.go | 2 +- ext/featurefmt/driver.go | 37 +++++++++---- ext/featurefmt/rpm/rpm.go | 2 +- worker.go | 101 +++++++++++++++++------------------- worker_test.go | 13 ++--- 6 files changed, 82 insertions(+), 75 deletions(-) diff --git a/ext/featurefmt/apk/apk.go b/ext/featurefmt/apk/apk.go index 2729a52b..ff63880d 100644 --- a/ext/featurefmt/apk/apk.go +++ b/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{} diff --git a/ext/featurefmt/dpkg/dpkg.go b/ext/featurefmt/dpkg/dpkg.go index 3a77ce4f..a0653580 100644 --- a/ext/featurefmt/dpkg/dpkg.go +++ b/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) { diff --git a/ext/featurefmt/driver.go b/ext/featurefmt/driver.go index 5b9fc2eb..8e8d593d 100644 --- a/ext/featurefmt/driver.go +++ b/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...) } diff --git a/ext/featurefmt/rpm/rpm.go b/ext/featurefmt/rpm/rpm.go index a54d69bc..9e62f0fc 100644 --- a/ext/featurefmt/rpm/rpm.go +++ b/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) { diff --git a/worker.go b/worker.go index 06f36df0..d407253f 100644 --- a/worker.go +++ b/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 namespace != nil { - // The Namespace has been detected in this layer; associate it. - features[i].Feature.Namespace = *namespace - 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 + } - log.WithFields(log.Fields{"feature name": feature.Feature.Name, "feature version": feature.Version, logLayerName: name}).Warning("Namespace unknown") - err = ErrUnsupported - return + detectedFeatures[i].Feature.Namespace = ns + } + features = append(features, detectedFeatures...) + } + + // 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 diff --git a/worker_test.go b/worker_test.go index c63a6674..950c7689 100644 --- a/worker_test.go +++ b/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"