Merge pull request #340 from mortent/SeparateLabelAndNameInventoryStrategies

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

Author: k8s-ci-robot

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)
 }