Add validation of inventory-id for name strategy if inventory object already exists

Author: mortent

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/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
@@ -251,20 +253,7 @@ 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()))
-	}
+	clusterInvObjects, err := cic.GetClusterInventoryObjs(inv)
 	if err != nil {
 		return nil, err
 	}
@@ -359,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

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/e2e_test.go

@@ -45,8 +45,13 @@ 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,
@@ -55,7 +60,7 @@ var inventoryConfigs = map[string]InventoryConfig{
 		InvSizeVerifyFunc:    defaultInvSizeVerifyFunc,
 		InvCountVerifyFunc:   defaultInvCountVerifyFunc,
 	},
-	"Custom Type": {
+	CustomTypeInvConfig: {
 		InventoryStrategy:    inventory.NameStrategy,
 		InventoryFactoryFunc: customInventoryManifest,
 		InvWrapperFunc:       customprovider.WrapInventoryInfoObj,
@@ -144,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) {

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