Make sure inventories are consistenly looked up based on either name or label

Author: mortent

pkg/apply/task/apply_task_test.go

@@ -921,6 +921,10 @@ func (fi *fakeInventoryInfo) ID() string {
 	return "id"
 }
 
+func (fi *fakeInventoryInfo) Strategy() inventory.InventoryStrategy {
+	return inventory.NameStrategy
+}
+
 func addOwningInventory(obj *unstructured.Unstructured, id string) *unstructured.Unstructured {
 	if obj == nil {
 		return nil

pkg/inventory/inventory-client.go

@@ -199,7 +199,30 @@ func (cic *ClusterInventoryClient) replaceInventory(inv *unstructured.Unstructur
 
 // DeleteInventoryObj deletes the inventory object from the cluster.
 func (cic *ClusterInventoryClient) DeleteInventoryObj(localInv InventoryInfo) error {
-	return cic.deleteInventoryObj(cic.invToUnstructuredFunc(localInv))
+	if localInv == nil {
+		return fmt.Errorf("retrieving cluster inventory object with nil local inventory")
+	}
+	switch localInv.Strategy() {
+	case NameStrategy:
+		return cic.deleteInventoryObjByName(cic.invToUnstructuredFunc(localInv))
+	case LabelStrategy:
+		return cic.deleteInventoryObjsByLabel(localInv)
+	default:
+		panic(fmt.Errorf("unknown inventory strategy: %s", localInv.Strategy()))
+	}
+}
+
+func (cic *ClusterInventoryClient) deleteInventoryObjsByLabel(inv InventoryInfo) error {
+	clusterInvObjs, err := cic.getClusterInventoryObjsByLabel(inv)
+	if err != nil {
+		return err
+	}
+	for _, invObj := range clusterInvObjs {
+		if err := cic.deleteInventoryObjByName(invObj); err != nil {
+			return err
+		}
+	}
+	return nil
 }
 
 // GetClusterObjs returns the objects stored in the cluster inventory object, or
@@ -228,6 +251,37 @@ func (cic *ClusterInventoryClient) GetClusterObjs(localInv InventoryInfo) ([]obj
 // TODO(seans3): Remove the special case code to merge multiple cluster inventory
 // objects once we've determined that this case is no longer possible.
 func (cic *ClusterInventoryClient) GetClusterInventoryInfo(inv InventoryInfo) (*unstructured.Unstructured, error) {
+	if inv == nil {
+		return nil, fmt.Errorf("inventoryInfo must be specified")
+	}
+
+	var clusterInvObjects []*unstructured.Unstructured
+	var err error
+	switch inv.Strategy() {
+	case NameStrategy:
+		clusterInvObjects, err = cic.getClusterInventoryObjsByName(inv)
+	case LabelStrategy:
+		clusterInvObjects, err = cic.getClusterInventoryObjsByLabel(inv)
+	default:
+		panic(fmt.Errorf("unknown inventory strategy: %s", inv.Strategy()))
+	}
+	if err != nil {
+		return nil, err
+	}
+
+	var clusterInv *unstructured.Unstructured
+	if len(clusterInvObjects) == 1 {
+		clusterInv = clusterInvObjects[0]
+	} else if len(clusterInvObjects) > 1 {
+		clusterInv, err = cic.mergeClusterInventory(clusterInvObjects)
+		if err != nil {
+			return nil, err
+		}
+	}
+	return clusterInv, nil
+}
+
+func (cic *ClusterInventoryClient) getClusterInventoryObjsByLabel(inv InventoryInfo) ([]*unstructured.Unstructured, error) {
 	localInv := cic.invToUnstructuredFunc(inv)
 	if localInv == nil {
 		return nil, fmt.Errorf("retrieving cluster inventory object with nil local inventory")
@@ -260,16 +314,37 @@ func (cic *ClusterInventoryClient) GetClusterInventoryInfo(inv InventoryInfo) (*
 	if err != nil {
 		return nil, err
 	}
-	var clusterInv *unstructured.Unstructured
-	if len(retrievedInventoryInfos) == 1 {
-		clusterInv = object.InfoToUnstructured(retrievedInventoryInfos[0])
-	} else if len(retrievedInventoryInfos) > 1 {
-		clusterInv, err = cic.mergeClusterInventory(object.InfosToUnstructureds(retrievedInventoryInfos))
-		if err != nil {
-			return nil, err
-		}
+	return object.InfosToUnstructureds(retrievedInventoryInfos), nil
+}
+
+func (cic *ClusterInventoryClient) getClusterInventoryObjsByName(inv InventoryInfo) ([]*unstructured.Unstructured, error) {
+	localInv := cic.invToUnstructuredFunc(inv)
+	if localInv == nil {
+		return nil, fmt.Errorf("retrieving cluster inventory object with nil local inventory")
 	}
-	return clusterInv, nil
+
+	invInfo, err := cic.toInfo(localInv)
+	if err != nil {
+		return nil, err
+	}
+
+	helper, err := cic.helperFromInfo(invInfo)
+	if err != nil {
+		return nil, err
+	}
+
+	res, err := helper.Get(inv.Namespace(), inv.Name())
+	if err != nil && !apierrors.IsNotFound(err) {
+		return nil, err
+	}
+	if apierrors.IsNotFound(err) {
+		return []*unstructured.Unstructured{}, nil
+	}
+	clusterInv, ok := res.(*unstructured.Unstructured)
+	if !ok {
+		return nil, fmt.Errorf("retrieved cluster inventory object is not of type *Unstructured")
+	}
+	return []*unstructured.Unstructured{clusterInv}, nil
 }
 
 func (cic *ClusterInventoryClient) UpdateLabels(inv InventoryInfo, labels map[string]string) error {
@@ -335,7 +410,7 @@ func (cic *ClusterInventoryClient) mergeClusterInventory(invObjs []*unstructured
 	// Finally, delete the other inventory objects.
 	for i := 1; i < len(invObjs); i++ {
 		merge := invObjs[i]
-		if err := cic.deleteInventoryObj(merge); err != nil {
+		if err := cic.deleteInventoryObjByName(merge); err != nil {
 			return nil, err
 		}
 	}
@@ -399,9 +474,9 @@ func (cic *ClusterInventoryClient) createInventoryObj(obj *unstructured.Unstruct
 	return invInfo.Refresh(createdObj, ignoreError)
 }
 
-// deleteInventoryObj deletes the passed inventory object from the APIServer, or
+// deleteInventoryObjByName deletes the passed inventory object from the APIServer, or
 // an error if one occurs.
-func (cic *ClusterInventoryClient) deleteInventoryObj(obj *unstructured.Unstructured) error {
+func (cic *ClusterInventoryClient) deleteInventoryObjByName(obj *unstructured.Unstructured) error {
 	if cic.dryRunStrategy.ClientOrServerDryRun() {
 		klog.V(4).Infof("dry-run delete inventory object: not deleted")
 		return nil

pkg/inventory/inventory-client_test.go

@@ -470,7 +470,7 @@ func TestDeleteInventoryObj(t *testing.T) {
 				if inv != nil {
 					inv = storeObjsInInventory(tc.inv, tc.localObjs)
 				}
-				err := invClient.deleteInventoryObj(inv)
+				err := invClient.deleteInventoryObjByName(inv)
 				if err != nil {
 					t.Fatalf("unexpected error received: %s", err)
 				}

pkg/inventory/inventory-info.go

@@ -3,6 +3,13 @@
 
 package inventory
 
+type InventoryStrategy string
+
+const (
+	NameStrategy  InventoryStrategy = "name"
+	LabelStrategy InventoryStrategy = "label"
+)
+
 // InventoryInfo provides the minimal information for the applier
 // to create, look up and update an inventory.
 // The inventory object can be any type, the Provider in the applier
@@ -21,4 +28,6 @@ type InventoryInfo interface {
 	// The Provider contained in the applier should know
 	// if the Id is necessary and how to use it for pruning objects.
 	ID() string
+
+	Strategy() InventoryStrategy
 }

pkg/inventory/inventorycm.go

@@ -70,6 +70,10 @@ func (icm *InventoryConfigMap) ID() string {
 	return strings.TrimSpace(inventoryLabel)
 }
 
+func (icm *InventoryConfigMap) Strategy() InventoryStrategy {
+	return LabelStrategy
+}
+
 func (icm *InventoryConfigMap) UnstructuredInventory() *unstructured.Unstructured {
 	return icm.inv
 }

pkg/inventory/policy_test.go

@@ -25,6 +25,10 @@ func (i *fakeInventoryInfo) ID() string {
 	return i.id
 }
 
+func (i *fakeInventoryInfo) Strategy() InventoryStrategy {
+	return NameStrategy
+}
+
 func testObjectWithAnnotation(key, val string) *unstructured.Unstructured {
 	obj := &unstructured.Unstructured{
 		Object: map[string]interface{}{

test/e2e/apply_and_destroy_test.go

@@ -5,6 +5,7 @@ package e2e
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	. "github.com/onsi/ginkgo"
@@ -19,14 +20,15 @@ import (
 func applyAndDestroyTest(c client.Client, invConfig InventoryConfig, inventoryName, namespaceName string) {
 	By("Apply resources")
 	applier := invConfig.ApplierFactoryFunc()
+	inventoryID := fmt.Sprintf("%s-%s", inventoryName, namespaceName)
 
-	inv := invConfig.InvWrapperFunc(invConfig.InventoryFactoryFunc(inventoryName, namespaceName, "test"))
+	applyInv := createInventoryInfo(invConfig, inventoryName, namespaceName, inventoryID)
 
 	resources := []*unstructured.Unstructured{
 		deploymentManifest(namespaceName),
 	}
 
-	applyCh := applier.Run(context.TODO(), inv, resources, apply.Options{
+	applyCh := applier.Run(context.TODO(), applyInv, resources, apply.Options{
 		ReconcileTimeout: 2 * time.Minute,
 		EmitStatusEvents: true,
 	})
@@ -53,13 +55,14 @@ func applyAndDestroyTest(c client.Client, invConfig InventoryConfig, inventoryNa
 	Expect(err).ToNot(HaveOccurred())
 
 	By("Verify inventory")
-	invConfig.InvSizeVerifyFunc(c, inventoryName, namespaceName, 1)
+	invConfig.InvSizeVerifyFunc(c, inventoryName, namespaceName, inventoryID, 1)
 
 	By("Destroy resources")
 	destroyer := invConfig.DestroyerFactoryFunc()
 
+	destroyInv := createInventoryInfo(invConfig, inventoryName, namespaceName, inventoryID)
 	option := &apply.DestroyerOption{InventoryPolicy: inventory.AdoptIfNoInventory}
-	destroyerEvents := runCollectNoErr(destroyer.Run(inv, option))
+	destroyerEvents := runCollectNoErr(destroyer.Run(destroyInv, option))
 	err = verifyEvents([]expEvent{
 		{
 			eventType: event.DeleteType,
@@ -70,3 +73,14 @@ func applyAndDestroyTest(c client.Client, invConfig InventoryConfig, inventoryNa
 	}, destroyerEvents)
 	Expect(err).ToNot(HaveOccurred())
 }
+
+func createInventoryInfo(invConfig InventoryConfig, inventoryName, namespaceName, inventoryID string) inventory.InventoryInfo {
+	switch invConfig.InventoryStrategy {
+	case inventory.NameStrategy:
+		return invConfig.InvWrapperFunc(invConfig.InventoryFactoryFunc(inventoryName, namespaceName, randomString("inventory-")))
+	case inventory.LabelStrategy:
+		return invConfig.InvWrapperFunc(invConfig.InventoryFactoryFunc(randomString("inventory-"), namespaceName, inventoryID))
+	default:
+		panic(fmt.Errorf("unknown inventory strategy %q", invConfig.InventoryStrategy))
+	}
+}

test/e2e/customprovider/provider.go

@@ -127,6 +127,10 @@ func (i InventoryCustomType) Name() string {
 	return i.inv.GetName()
 }
 
+func (i InventoryCustomType) Strategy() inventory.InventoryStrategy {
+	return inventory.NameStrategy
+}
+
 func (i InventoryCustomType) ID() string {
 	labels := i.inv.GetLabels()
 	id, found := labels[common.InventoryLabel]

test/e2e/e2e_test.go

@@ -32,10 +32,11 @@ type inventoryFactoryFunc func(name, namespace, id string) *unstructured.Unstruc
 type invWrapperFunc func(*unstructured.Unstructured) inventory.InventoryInfo
 type applierFactoryFunc func() *apply.Applier
 type destroyerFactoryFunc func() *apply.Destroyer
-type invSizeVerifyFunc func(c client.Client, name, namespace string, count int)
+type invSizeVerifyFunc func(c client.Client, name, namespace, id string, count int)
 type invCountVerifyFunc func(c client.Client, namespace string, count int)
 
 type InventoryConfig struct {
+	InventoryStrategy    inventory.InventoryStrategy
 	InventoryFactoryFunc inventoryFactoryFunc
 	InvWrapperFunc       invWrapperFunc
 	ApplierFactoryFunc   applierFactoryFunc
@@ -46,6 +47,7 @@ type InventoryConfig struct {
 
 var inventoryConfigs = map[string]InventoryConfig{
 	"ConfigMap (default)": {
+		InventoryStrategy:    inventory.LabelStrategy,
 		InventoryFactoryFunc: cmInventoryManifest,
 		InvWrapperFunc:       inventory.WrapInventoryInfoObj,
 		ApplierFactoryFunc:   newDefaultInvApplier,
@@ -54,6 +56,7 @@ var inventoryConfigs = map[string]InventoryConfig{
 		InvCountVerifyFunc:   defaultInvCountVerifyFunc,
 	},
 	"Custom Type": {
+		InventoryStrategy:    inventory.NameStrategy,
 		InventoryFactoryFunc: customInventoryManifest,
 		InvWrapperFunc:       customprovider.WrapInventoryInfoObj,
 		ApplierFactoryFunc:   newCustomInvApplier,
@@ -196,12 +199,15 @@ func newDefaultInvProvider() provider.Provider {
 	return provider.NewProvider(newFactory())
 }
 
-func defaultInvSizeVerifyFunc(c client.Client, name, namespace string, count int) {
-	var cm v1.ConfigMap
-	err := c.Get(context.TODO(), types.NamespacedName{
-		Name:      name,
-		Namespace: namespace,
-	}, &cm)
+func defaultInvSizeVerifyFunc(c client.Client, name, namespace, id string, count int) {
+	var cmList v1.ConfigMapList
+	err := c.List(context.TODO(), &cmList,
+		client.MatchingLabels(map[string]string{common.InventoryLabel: id}),
+		client.InNamespace(namespace))
+	Expect(err).ToNot(HaveOccurred())
+
+	Expect(len(cmList.Items)).To(Equal(1))
+	cm := cmList.Items[0]
 	Expect(err).ToNot(HaveOccurred())
 
 	data := cm.Data
@@ -241,8 +247,8 @@ func newFactory() util.Factory {
 	return util.NewFactory(matchVersionKubeConfigFlags)
 }
 
-func customInvSizeVerifyFunc(c client.Client, name, namespace string, count int) {
-	var u unstructured.Unstructured
+func customInvSizeVerifyFunc(c client.Client, name, namespace, _ string, count int) {
+	var u unstructured.UnstructuredList
 	u.SetGroupVersionKind(customprovider.InventoryGVK)
 	err := c.Get(context.TODO(), types.NamespacedName{
 		Name:      name,

test/e2e/inventory_policy_test.go

@@ -141,7 +141,7 @@ func inventoryPolicyAdoptIfNoInventoryTest(c client.Client, invConfig InventoryC
 	Expect(d.ObjectMeta.Annotations["config.k8s.io/owning-inventory"]).To(Equal(invName))
 
 	invConfig.InvCountVerifyFunc(c, namespaceName, 1)
-	invConfig.InvSizeVerifyFunc(c, invName, namespaceName, 1)
+	invConfig.InvSizeVerifyFunc(c, invName, namespaceName, invName, 1)
 }
 
 func inventoryPolicyAdoptAllTest(c client.Client, invConfig InventoryConfig, namespaceName string) {