Merge pull request #340 from mortent/SeparateLabelAndNameInventoryStrategies

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

pkg/apply/applier.go

@@ -97,6 +97,29 @@ func (a *Applier) prepareObjects(localInv inventory.InventoryInfo, localObjs []*
 	if err := inventory.ValidateNoInventory(localObjs); err != nil {
 		return nil, err
 	}
+
+	// If the inventory uses the Name strategy and an inventory ID is provided,
+	// verify that the existing inventory object (if there is one) has an ID
+	// label that matches.
+	if localInv.Strategy() == inventory.NameStrategy && localInv.ID() != "" {
+		invObjs, err := a.invClient.GetClusterInventoryObjs(localInv)
+		if err != nil {
+			return nil, err
+		}
+
+		if len(invObjs) > 1 {
+			panic(fmt.Errorf("found %d inv objects with Name strategy", len(invObjs)))
+		}
+
+		if len(invObjs) == 1 {
+			invObj := invObjs[0]
+			val := invObj.GetLabels()[common.InventoryLabel]
+			if val != localInv.ID() {
+				return nil, fmt.Errorf("inventory-id of inventory object in cluster doesn't match provided id %q", localInv.ID())
+			}
+		}
+	}
+
 	// Ensures the namespace exists before applying the inventory object into it.
 	if invNamespace := inventoryNamespaceInSet(localInv, localObjs); invNamespace != nil {
 		klog.V(4).Infof("applier prepareObjects applying namespace %s", invNamespace.GetName())

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/fake-inventory-client.go

@@ -89,3 +89,7 @@ func (fic *FakeInventoryClient) GetClusterInventoryInfo(inv InventoryInfo) (*uns
 func (fic *FakeInventoryClient) UpdateLabels(inv InventoryInfo, labels map[string]string) error {
 	return nil
 }
+
+func (fic *FakeInventoryClient) GetClusterInventoryObjs(inv InventoryInfo) ([]*unstructured.Unstructured, error) {
+	return []*unstructured.Unstructured{}, nil
+}

pkg/inventory/inventory-client.go

@@ -46,6 +46,8 @@ type InventoryClient interface {
 	GetClusterInventoryInfo(inv InventoryInfo) (*unstructured.Unstructured, error)
 	// UpdateLabels updates the labels of the cluster inventory object if it exists.
 	UpdateLabels(InventoryInfo, map[string]string) error
+	// GetInventoryObjs looks up the inventory objects from the cluster.
+	GetClusterInventoryObjs(inv InventoryInfo) ([]*unstructured.Unstructured, error)
 }
 
 // ClusterInventoryClient is a concrete implementation of the
@@ -199,7 +201,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 +253,24 @@ 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) {
+	clusterInvObjects, err := cic.GetClusterInventoryObjs(inv)
+	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 +303,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 {
@@ -284,6 +348,24 @@ func (cic *ClusterInventoryClient) UpdateLabels(inv InventoryInfo, labels map[st
 	return cic.applyInventoryObj(obj)
 }
 
+func (cic *ClusterInventoryClient) GetClusterInventoryObjs(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()))
+	}
+	return clusterInvObjects, err
+}
+
 // mergeClusterInventory merges the inventory of multiple inventory objects
 // into one inventory object, and applies it. Deletes the remaining unnecessary
 // inventory objects. There should be only one inventory object stored in the
@@ -335,7 +417,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 +481,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/common_test.go

@@ -27,6 +27,16 @@ func randomString(prefix string) string {
 	return fmt.Sprintf("%s%s", prefix, randomSuffix)
 }
 
+func run(ch <-chan event.Event) error {
+	var err error
+	for e := range ch {
+		if e.Type == event.ErrorType {
+			err = e.ErrorEvent.Err
+		}
+	}
+	return err
+}
+
 func runWithNoErr(ch <-chan event.Event) {
 	runCollectNoErr(ch)
 }

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
@@ -44,8 +45,14 @@ type InventoryConfig struct {
 	InvCountVerifyFunc   invCountVerifyFunc
 }
 
+const (
+	ConfigMapTypeInvConfig = "ConfigMap"
+	CustomTypeInvConfig    = "Custom"
+)
+
 var inventoryConfigs = map[string]InventoryConfig{
-	"ConfigMap (default)": {
+	ConfigMapTypeInvConfig: {
+		InventoryStrategy:    inventory.LabelStrategy,
 		InventoryFactoryFunc: cmInventoryManifest,
 		InvWrapperFunc:       inventory.WrapInventoryInfoObj,
 		ApplierFactoryFunc:   newDefaultInvApplier,
@@ -53,7 +60,8 @@ var inventoryConfigs = map[string]InventoryConfig{
 		InvSizeVerifyFunc:    defaultInvSizeVerifyFunc,
 		InvCountVerifyFunc:   defaultInvCountVerifyFunc,
 	},
-	"Custom Type": {
+	CustomTypeInvConfig: {
+		InventoryStrategy:    inventory.NameStrategy,
 		InventoryFactoryFunc: customInventoryManifest,
 		InvWrapperFunc:       customprovider.WrapInventoryInfoObj,
 		ApplierFactoryFunc:   newCustomInvApplier,
@@ -141,6 +149,24 @@ var _ = Describe("Applier", func() {
 			})
 		})
 	}
+
+	Context("InventoryStrategy: Name", func() {
+		var namespace *v1.Namespace
+		var inventoryName string
+
+		BeforeEach(func() {
+			inventoryName = randomString("test-inv-")
+			namespace = createRandomNamespace(c)
+		})
+
+		AfterEach(func() {
+			deleteNamespace(c, namespace)
+		})
+
+		It("Apply with existing inventory", func() {
+			applyWithExistingInvTest(c, inventoryConfigs[CustomTypeInvConfig], inventoryName, namespace.GetName())
+		})
+	})
 })
 
 func createInventoryCRD(c client.Client) {
@@ -196,12 +222,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 +270,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) {

test/e2e/name_inv_strategy_test.go

@@ -0,0 +1,49 @@
+// Copyright 2021 The Kubernetes Authors.
+// SPDX-License-Identifier: Apache-2.0
+
+package e2e
+
+import (
+	"context"
+	"fmt"
+	"time"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"sigs.k8s.io/cli-utils/pkg/apply"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+func applyWithExistingInvTest(c client.Client, invConfig InventoryConfig, inventoryName, namespaceName string) {
+	By("Apply first set of resources")
+	applier := invConfig.ApplierFactoryFunc()
+	orgInventoryID := fmt.Sprintf("%s-%s", inventoryName, namespaceName)
+
+	orgApplyInv := invConfig.InvWrapperFunc(invConfig.InventoryFactoryFunc(inventoryName, namespaceName, orgInventoryID))
+
+	resources := []*unstructured.Unstructured{
+		deploymentManifest(namespaceName),
+	}
+
+	runWithNoErr(applier.Run(context.TODO(), orgApplyInv, resources, apply.Options{
+		ReconcileTimeout: 2 * time.Minute,
+		EmitStatusEvents: true,
+	}))
+
+	By("Verify inventory")
+	invConfig.InvSizeVerifyFunc(c, inventoryName, namespaceName, orgInventoryID, 1)
+
+	By("Apply second set of resources, using same inventory name but different ID")
+	secondInventoryID := fmt.Sprintf("%s-%s-2", inventoryName, namespaceName)
+	secondApplyInv := invConfig.InvWrapperFunc(invConfig.InventoryFactoryFunc(inventoryName, namespaceName, secondInventoryID))
+
+	err := run(applier.Run(context.TODO(), secondApplyInv, resources, apply.Options{
+		ReconcileTimeout: 2 * time.Minute,
+		EmitStatusEvents: true,
+	}))
+
+	By("Verify that we get the correct error")
+	Expect(err).To(HaveOccurred())
+	Expect(err.Error()).To(ContainSubstring("inventory-id of inventory object in cluster doesn't match provided id"))
+}