Merge pull request #720 from KeyboardNerd/update_ns

clair: Fix namespace update logic

ancestry.go

@@ -17,6 +17,7 @@ package clair
 import (
 	"crypto/sha256"
 	"encoding/hex"
+	"encoding/json"
 	"strings"
 
 	log "github.com/sirupsen/logrus"
@@ -25,14 +26,14 @@ import (
 )
 
 type layerIndexedFeature struct {
-	feature      *database.LayerFeature
+	Feature      *database.LayerFeature
-	namespace    *layerIndexedNamespace
+	Namespace    *layerIndexedNamespace
-	introducedIn int
+	IntroducedIn int
 }
 
 type layerIndexedNamespace struct {
-	namespace    database.LayerNamespace
+	Namespace    database.LayerNamespace `json:"namespace"`
-	introducedIn int
+	IntroducedIn int                     `json:"introducedIn"`
 }
 
 // AncestryBuilder builds an Ancestry, which contains an ordered list of layers
@@ -41,7 +42,7 @@ type AncestryBuilder struct {
 	layerIndex int
 	layerNames []string
 	detectors  []database.Detector
-	namespaces map[database.Detector]*layerIndexedNamespace
+	namespaces []layerIndexedNamespace // unique namespaces
 	features   map[database.Detector][]layerIndexedFeature
 }
 
@@ -53,7 +54,7 @@ func NewAncestryBuilder(detectors []database.Detector) *AncestryBuilder {
 	return &AncestryBuilder{
 		layerIndex: 0,
 		detectors:  detectors,
-		namespaces: make(map[database.Detector]*layerIndexedNamespace),
+		namespaces: make([]layerIndexedNamespace, 0),
 		features:   make(map[database.Detector][]layerIndexedFeature),
 	}
 }
@@ -105,7 +106,7 @@ func (b *AncestryBuilder) addLayerFeatures(detector database.Detector, features
 	for i := range existingFeatures {
 		feature := existingFeatures[i]
 		for j := range features {
-			if features[j] == *feature.feature {
+			if features[j] == *feature.Feature {
 				currentFeatures = append(currentFeatures, feature)
 				break
 			}
@@ -116,7 +117,7 @@ func (b *AncestryBuilder) addLayerFeatures(detector database.Detector, features
 	for i := range features {
 		found := false
 		for j := range existingFeatures {
-			if *existingFeatures[j].feature == features[i] {
+			if *existingFeatures[j].Feature == features[i] {
 				found = true
 				break
 			}
@@ -125,7 +126,6 @@ func (b *AncestryBuilder) addLayerFeatures(detector database.Detector, features
 		if !found {
 			namespace, found := b.lookupNamespace(&features[i])
 			if !found {
-				log.WithField("Layer Hashes", b.layerNames).Error("skip, could not find the proper namespace for feature")
 				continue
 			}
 
@@ -144,28 +144,58 @@ func (b *AncestryBuilder) addLayerFeatures(detector database.Detector, features
 func (b *AncestryBuilder) updateNamespace(layerNamespace *database.LayerNamespace) {
 	var (
 		previous     *layerIndexedNamespace
-		ok       bool
+		foundUpgrade bool
 	)
 
-	if previous, ok = b.namespaces[layerNamespace.By]; !ok {
+	newNSNames := strings.Split(layerNamespace.Name, ":")
-		b.namespaces[layerNamespace.By] = &layerIndexedNamespace{
+	if len(newNSNames) != 2 {
-			namespace:    *layerNamespace,
+		log.Error("invalid namespace name")
-			introducedIn: b.layerIndex,
 	}
 
+	newNSName := newNSNames[0]
+	newNSVersion := newNSNames[1]
+	for i, ns := range b.namespaces {
+		nsNames := strings.Split(ns.Namespace.Name, ":")
+		if len(nsNames) != 2 {
+			log.Error("invalid namespace name")
+			continue
+		}
+
+		nsName := nsNames[0]
+		nsVersion := nsNames[1]
+		if ns.Namespace.VersionFormat == layerNamespace.VersionFormat && nsName == newNSName {
+			if nsVersion != newNSVersion {
+				previous = &b.namespaces[i]
+				foundUpgrade = true
+				break
+			} else {
+				// not changed
+				return
+			}
+		}
+	}
+
+	// we didn't found the namespace is a upgrade from another namespace, so we
+	// simply add it.
+	if !foundUpgrade {
+		b.namespaces = append(b.namespaces, layerIndexedNamespace{
+			Namespace:    *layerNamespace,
+			IntroducedIn: b.layerIndex,
+		})
+
 		return
 	}
 
 	// All features referencing to this namespace are now pointing to the new namespace.
 	// Also those features are now treated as introduced in the same layer as
 	// when this new namespace is introduced.
-	previous.namespace = *layerNamespace
+	previous.Namespace = *layerNamespace
-	previous.introducedIn = b.layerIndex
+	previous.IntroducedIn = b.layerIndex
 
 	for _, features := range b.features {
 		for i, feature := range features {
-			if feature.namespace == previous {
+			if feature.Namespace == previous {
-				features[i].introducedIn = previous.introducedIn
+				features[i].IntroducedIn = previous.IntroducedIn
 			}
 		}
 	}
@@ -173,19 +203,37 @@ func (b *AncestryBuilder) updateNamespace(layerNamespace *database.LayerNamespac
 
 func (b *AncestryBuilder) createLayerIndexedFeature(namespace *layerIndexedNamespace, feature *database.LayerFeature) layerIndexedFeature {
 	return layerIndexedFeature{
-		feature:      feature,
+		Feature:      feature,
-		namespace:    namespace,
+		Namespace:    namespace,
-		introducedIn: b.layerIndex,
+		IntroducedIn: b.layerIndex,
 	}
 }
 
 func (b *AncestryBuilder) lookupNamespace(feature *database.LayerFeature) (*layerIndexedNamespace, bool) {
-	for _, namespace := range b.namespaces {
+	matchedNamespaces := []*layerIndexedNamespace{}
-		if namespace.namespace.VersionFormat == feature.VersionFormat {
+	for i, namespace := range b.namespaces {
-			return namespace, true
+		if namespace.Namespace.VersionFormat == feature.VersionFormat {
+			matchedNamespaces = append(matchedNamespaces, &b.namespaces[i])
 		}
 	}
 
+	if len(matchedNamespaces) == 1 {
+		return matchedNamespaces[0], true
+	}
+
+	serialized, _ := json.Marshal(matchedNamespaces)
+	fields := log.Fields{
+		"feature.Name":               feature.Name,
+		"feature.VersionFormat":      feature.VersionFormat,
+		"ancestryBuilder.namespaces": string(serialized),
+	}
+
+	if len(matchedNamespaces) > 1 {
+		log.WithFields(fields).Warn("skip features with ambiguous namespaces")
+	} else {
+		log.WithFields(fields).Warn("skip features with no matching namespace")
+	}
+
 	return nil, false
 }
 
@@ -193,14 +241,14 @@ func (b *AncestryBuilder) ancestryFeatures(index int) []database.AncestryFeature
 	ancestryFeatures := []database.AncestryFeature{}
 	for detector, features := range b.features {
 		for _, feature := range features {
-			if feature.introducedIn == index {
+			if feature.IntroducedIn == index {
 				ancestryFeatures = append(ancestryFeatures, database.AncestryFeature{
 					NamespacedFeature: database.NamespacedFeature{
-						Feature:   feature.feature.Feature,
+						Feature:   feature.Feature.Feature,
-						Namespace: feature.namespace.namespace.Namespace,
+						Namespace: feature.Namespace.Namespace.Namespace,
 					},
 					FeatureBy:   detector,
-					NamespaceBy: feature.namespace.namespace.By,
+					NamespaceBy: feature.Namespace.Namespace.By,
 				})
 			}
 		}

ancestry_test.go

@@ -28,10 +28,13 @@ var (
 	pip        = database.NewFeatureDetector("pip", "1.0")
 	python     = database.NewNamespaceDetector("python", "1.0")
 	osrelease  = database.NewNamespaceDetector("os-release", "1.0")
+	aptsources = database.NewNamespaceDetector("apt-sources", "1.0")
 	ubuntu     = *database.NewNamespace("ubuntu:14.04", "dpkg")
 	ubuntu16   = *database.NewNamespace("ubuntu:16.04", "dpkg")
+	debian     = *database.NewNamespace("debian:7", "dpkg")
 	python2    = *database.NewNamespace("python:2", "pip")
 	sed        = *database.NewSourcePackage("sed", "4.4-2", "dpkg")
+	sedByRPM   = *database.NewBinaryPackage("sed", "4.4-2", "rpm")
 	sedBin     = *database.NewBinaryPackage("sed", "4.4-2", "dpkg")
 	tar        = *database.NewBinaryPackage("tar", "1.29b-2", "dpkg")
 	scipy      = *database.NewSourcePackage("scipy", "3.0.0", "pip")
@@ -40,6 +43,34 @@ var (
 	multinamespaceDetectors = []database.Detector{dpkg, osrelease, pip}
 )
 
+type ancestryBuilder struct {
+	ancestry *database.Ancestry
+}
+
+func newAncestryBuilder(name string) *ancestryBuilder {
+	return &ancestryBuilder{&database.Ancestry{Name: name}}
+}
+
+func (b *ancestryBuilder) addDetectors(d ...database.Detector) *ancestryBuilder {
+	b.ancestry.By = append(b.ancestry.By, d...)
+	return b
+}
+
+func (b *ancestryBuilder) addLayer(hash string, f ...database.AncestryFeature) *ancestryBuilder {
+	l := database.AncestryLayer{Hash: hash}
+	l.Features = append(l.Features, f...)
+	b.ancestry.Layers = append(b.ancestry.Layers, l)
+	return b
+}
+
+func ancestryFeature(namespace database.Namespace, feature database.Feature, nsBy database.Detector, fBy database.Detector) database.AncestryFeature {
+	return database.AncestryFeature{
+		NamespacedFeature: database.NamespacedFeature{feature, namespace},
+		FeatureBy:         fBy,
+		NamespaceBy:       nsBy,
+	}
+}
+
 // layerBuilder is for helping constructing the layer test artifacts.
 type layerBuilder struct {
 	layer *database.Layer
@@ -49,6 +80,15 @@ func newLayerBuilder(hash string) *layerBuilder {
 	return &layerBuilder{&database.Layer{Hash: hash, By: detectors}}
 }
 
+func newLayerBuilderWithoutDetector(hash string) *layerBuilder {
+	return &layerBuilder{&database.Layer{Hash: hash}}
+}
+
+func (b *layerBuilder) addDetectors(d ...database.Detector) *layerBuilder {
+	b.layer.By = append(b.layer.By, d...)
+	return b
+}
+
 func (b *layerBuilder) addNamespace(detector database.Detector, ns database.Namespace) *layerBuilder {
 	b.layer.Namespaces = append(b.layer.Namespaces, database.LayerNamespace{
 		Namespace: ns,
@@ -85,177 +125,155 @@ var testImage = []*database.Layer{
 	newLayerBuilder("7").addFeature(dpkg, sed).layer,
 }
 
-var clairLimit = []*database.Layer{
-	// TODO(sidac): how about install rpm package under ubuntu?
-	newLayerBuilder("1").addNamespace(osrelease, ubuntu).layer,
-	newLayerBuilder("2").addFeature(rpm, sed).layer,
-}
-
-var multipleNamespace = []*database.Layer{
-	// TODO(sidac): support for multiple namespaces
-}
-
 var invalidNamespace = []*database.Layer{
 	// add package without namespace, this indicates that the namespace detector
 	// could not detect the namespace.
 	newLayerBuilder("0").addFeature(dpkg, sed).layer,
 }
 
+var noMatchingNamespace = []*database.Layer{
+	newLayerBuilder("0").addFeature(rpm, sedByRPM).addFeature(dpkg, sed).addNamespace(osrelease, ubuntu).layer,
+}
+
 var multiplePackagesOnFirstLayer = []*database.Layer{
 	newLayerBuilder("0").addFeature(dpkg, sed).addFeature(dpkg, tar).addFeature(dpkg, sedBin).addNamespace(osrelease, ubuntu16).layer,
 }
 
+var twoNamespaceDetectorsWithSameResult = []*database.Layer{
+	newLayerBuilderWithoutDetector("0").addDetectors(dpkg, aptsources, osrelease).addFeature(dpkg, sed).addNamespace(aptsources, ubuntu).addNamespace(osrelease, ubuntu).layer,
+}
+
+var sameVersionFormatDiffName = []*database.Layer{
+	newLayerBuilder("0").addFeature(dpkg, sed).addNamespace(aptsources, ubuntu).addNamespace(osrelease, debian).layer,
+}
+
 func TestAddLayer(t *testing.T) {
 	cases := []struct {
 		title               string
 		image               []*database.Layer
-
+		nonDefaultDetectors []database.Detector
 		expectedAncestry    database.Ancestry
 	}{
 		{
 			title:            "empty image",
-			expectedAncestry: database.Ancestry{Name: ancestryName([]string{}), By: detectors},
+			expectedAncestry: *newAncestryBuilder(ancestryName([]string{})).addDetectors(detectors...).ancestry,
 		},
 		{
 			title: "empty layer",
 			image: testImage[:1],
-			expectedAncestry: database.Ancestry{Name: ancestryName([]string{"0"}), By: detectors, Layers: []database.AncestryLayer{{Hash: "0"}}},
+			expectedAncestry: *newAncestryBuilder(ancestryName([]string{"0"})).addDetectors(detectors...).
+				addLayer("0").ancestry,
 		},
 		{
 			title: "ubuntu",
 			image: testImage[:2],
-			expectedAncestry: database.Ancestry{
+			expectedAncestry: *newAncestryBuilder(ancestryName([]string{"0", "1"})).addDetectors(detectors...).
-				Name:   ancestryName([]string{"0", "1"}),
+				addLayer("0").
-				By:     detectors,
+				addLayer("1").ancestry,
-				Layers: []database.AncestryLayer{{Hash: "0"}, {Hash: "1"}},
-			},
 		},
 		{
 			title: "ubuntu install sed",
 			image: testImage[:3],
-			expectedAncestry: database.Ancestry{
+			expectedAncestry: *newAncestryBuilder(ancestryName([]string{"0", "1", "2"})).addDetectors(detectors...).
-				Name: ancestryName([]string{"0", "1", "2"}),
+				addLayer("0").
-				By:   detectors,
+				addLayer("1").
-				Layers: []database.AncestryLayer{{Hash: "0"}, {Hash: "1"}, {Hash: "2", Features: []database.AncestryFeature{
+				addLayer("2", ancestryFeature(ubuntu, sed, osrelease, dpkg)).ancestry,
-					{
-						NamespacedFeature: database.NamespacedFeature{Feature: sed, Namespace: ubuntu},
-						FeatureBy:         dpkg,
-						NamespaceBy:       osrelease,
-					},
-				}}},
-			},
 		},
 		{
 			title: "ubuntu install tar",
 			image: testImage[:4],
-			expectedAncestry: database.Ancestry{
+			expectedAncestry: *newAncestryBuilder(ancestryName([]string{"0", "1", "2", "3"})).addDetectors(detectors...).
-				Name: ancestryName([]string{"0", "1", "2", "3"}),
+				addLayer("0").
-				By:   detectors,
+				addLayer("1").
-				Layers: []database.AncestryLayer{{Hash: "0"}, {Hash: "1"}, {Hash: "2", Features: []database.AncestryFeature{
+				addLayer("2", ancestryFeature(ubuntu, sed, osrelease, dpkg)).
-					{
+				addLayer("3", ancestryFeature(ubuntu, tar, osrelease, dpkg)).ancestry,
-						NamespacedFeature: database.NamespacedFeature{Feature: sed, Namespace: ubuntu},
-						FeatureBy:         dpkg,
-						NamespaceBy:       osrelease,
-					},
-				}}, {
-					Hash: "3", Features: []database.AncestryFeature{
-						{
-							NamespacedFeature: database.NamespacedFeature{Feature: tar, Namespace: ubuntu},
-							FeatureBy:         dpkg,
-							NamespaceBy:       osrelease,
-						},
-					},
-				}},
-			},
 		}, {
 			title: "ubuntu uninstall tar",
 			image: testImage[:5],
-			expectedAncestry: database.Ancestry{
+			expectedAncestry: *newAncestryBuilder(ancestryName([]string{"0", "1", "2", "3", "4"})).addDetectors(detectors...).
-				Name: ancestryName([]string{"0", "1", "2", "3", "4"}),
+				addLayer("0").
-				By:   detectors,
+				addLayer("1").
-				Layers: []database.AncestryLayer{{Hash: "0"}, {Hash: "1"}, {Hash: "2", Features: []database.AncestryFeature{
+				addLayer("2", ancestryFeature(ubuntu, sed, osrelease, dpkg)).
-					{
+				addLayer("3").
-						NamespacedFeature: database.NamespacedFeature{Feature: sed, Namespace: ubuntu},
+				addLayer("4").ancestry,
-						FeatureBy:         dpkg,
-						NamespaceBy:       osrelease,
-					},
-				}}, {Hash: "3"}, {Hash: "4"}},
-			},
 		}, {
 			title: "ubuntu upgrade",
 			image: testImage[:6],
-			expectedAncestry: database.Ancestry{
+			expectedAncestry: *newAncestryBuilder(ancestryName([]string{"0", "1", "2", "3", "4", "5"})).addDetectors(detectors...).
-				Name: ancestryName([]string{"0", "1", "2", "3", "4", "5"}),
+				addLayer("0").
-				By:   detectors,
+				addLayer("1").
-				Layers: []database.AncestryLayer{{Hash: "0"}, {Hash: "1"}, {Hash: "2"}, {Hash: "3"}, {Hash: "4"}, {Hash: "5", Features: []database.AncestryFeature{
+				addLayer("2").
-					{
+				addLayer("3").
-						NamespacedFeature: database.NamespacedFeature{Feature: sed, Namespace: ubuntu16},
+				addLayer("4").
-						FeatureBy:         dpkg,
+				addLayer("5", ancestryFeature(ubuntu16, sed, osrelease, dpkg)).ancestry,
-						NamespaceBy:       osrelease,
-					}}},
-				},
-			},
 		}, {
 			title: "no change to the detectable files",
 			image: testImage[:7],
-			expectedAncestry: database.Ancestry{
+			expectedAncestry: *newAncestryBuilder(ancestryName([]string{"0", "1", "2", "3", "4", "5", "6"})).addDetectors(detectors...).
-				Name: ancestryName([]string{"0", "1", "2", "3", "4", "5", "6"}),
+				addLayer("0").
-				By:   detectors,
+				addLayer("1").
-				Layers: []database.AncestryLayer{{Hash: "0"}, {Hash: "1"}, {Hash: "2"}, {Hash: "3"}, {Hash: "4"}, {Hash: "5", Features: []database.AncestryFeature{
+				addLayer("2").
-					{
+				addLayer("3").
-						NamespacedFeature: database.NamespacedFeature{Feature: sed, Namespace: ubuntu16},
+				addLayer("4").
-						FeatureBy:         dpkg,
+				addLayer("5", ancestryFeature(ubuntu16, sed, osrelease, dpkg)).
-						NamespaceBy:       osrelease,
+				addLayer("6").ancestry,
-					}}}, {Hash: "6"}},
-			},
 		}, {
 			title: "change to the package installer database but no features are affected.",
 			image: testImage[:8],
-			expectedAncestry: database.Ancestry{
+			expectedAncestry: *newAncestryBuilder(ancestryName([]string{"0", "1", "2", "3", "4", "5", "6", "7"})).addDetectors(detectors...).
-				Name: ancestryName([]string{"0", "1", "2", "3", "4", "5", "6", "7"}),
+				addLayer("0").
-				By:   detectors,
+				addLayer("1").
-				Layers: []database.AncestryLayer{{Hash: "0"}, {Hash: "1"}, {Hash: "2"}, {Hash: "3"}, {Hash: "4"}, {Hash: "5", Features: []database.AncestryFeature{
+				addLayer("2").
-					{
+				addLayer("3").
-						NamespacedFeature: database.NamespacedFeature{Feature: sed, Namespace: ubuntu16},
+				addLayer("4").
-						FeatureBy:         dpkg,
+				addLayer("5", ancestryFeature(ubuntu16, sed, osrelease, dpkg)).
-						NamespaceBy:       osrelease,
+				addLayer("6").
-					}}}, {Hash: "6"}, {Hash: "7"}},
+				addLayer("7").ancestry,
-			},
 		}, {
 			title: "layers with features and namespace.",
 			image: multiplePackagesOnFirstLayer,
-			expectedAncestry: database.Ancestry{
+			expectedAncestry: *newAncestryBuilder(ancestryName([]string{"0"})).addDetectors(detectors...).
-				Name: ancestryName([]string{"0"}),
+				addLayer("0",
-				By:   detectors,
+					ancestryFeature(ubuntu16, sed, osrelease, dpkg),
-				Layers: []database.AncestryLayer{
+					ancestryFeature(ubuntu16, sedBin, osrelease, dpkg),
-					{
+					ancestryFeature(ubuntu16, tar, osrelease, dpkg)).
-						Hash: "0",
+				ancestry,
-						Features: []database.AncestryFeature{
+		}, {
-							{
+			title:               "two namespace detectors giving same namespace.",
-								NamespacedFeature: database.NamespacedFeature{Feature: sed, Namespace: ubuntu16},
+			image:               twoNamespaceDetectorsWithSameResult,
-								FeatureBy:         dpkg,
+			nonDefaultDetectors: []database.Detector{osrelease, aptsources, dpkg},
-								NamespaceBy:       osrelease,
+			expectedAncestry: *newAncestryBuilder(ancestryName([]string{"0"})).addDetectors(osrelease, aptsources, dpkg).
-							},
+				addLayer("0", ancestryFeature(ubuntu, sed, aptsources, dpkg)).
-							{
+				ancestry,
-								NamespacedFeature: database.NamespacedFeature{Feature: sedBin, Namespace: ubuntu16},
+		}, {
-								FeatureBy:         dpkg,
+			title: "feature without namespace",
-								NamespaceBy:       osrelease,
+			image: invalidNamespace,
-							},
+			expectedAncestry: *newAncestryBuilder(ancestryName([]string{"0"})).addDetectors(detectors...).
-							{
+				addLayer("0").
-								NamespacedFeature: database.NamespacedFeature{Feature: tar, Namespace: ubuntu16},
+				ancestry,
-								FeatureBy:         dpkg,
+		}, {
-								NamespaceBy:       osrelease,
+			title: "two namespaces with the same version format but different names",
-							},
+			image: sameVersionFormatDiffName,
-						},
+			// failure of matching a namespace will result in the package not being added.
-					},
+			expectedAncestry: *newAncestryBuilder(ancestryName([]string{"0"})).addDetectors(detectors...).
-				},
+				addLayer("0").
-			},
+				ancestry,
+		}, {
+			title:            "noMatchingNamespace",
+			image:            noMatchingNamespace,
+			expectedAncestry: *newAncestryBuilder(ancestryName([]string{"0"})).addDetectors(detectors...).addLayer("0", ancestryFeature(ubuntu, sed, osrelease, dpkg)).ancestry,
 		},
 	}
 
 	for _, test := range cases {
 		t.Run(test.title, func(t *testing.T) {
-			builder := NewAncestryBuilder(detectors)
+			var builder *AncestryBuilder
+			if len(test.nonDefaultDetectors) != 0 {
+				builder = NewAncestryBuilder(test.nonDefaultDetectors)
+			} else {
+				builder = NewAncestryBuilder(detectors)
+			}
+
 			for _, layer := range test.image {
 				builder.AddLeafLayer(layer)
 			}