Merge pull request #648 from sdowell/refactor-client-interface

chore: clean up inventory Client interface

Author: k8s-ci-robot

pkg/apply/applier.go

@@ -64,26 +64,6 @@ func (a *Applier) prepareObjects(localInv inventory.Info, localObjs object.Unstr
 	for _, localObj := range localObjs {
 		inventory.AddInventoryIDAnnotation(localObj, localInv)
 	}
-	// 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.
-	// TODO(seans): This inventory id validation should happen in destroy and status.
-	if localInv.Strategy() == inventory.NameStrategy && localInv.ID() != "" {
-		prevInvObjs, err := a.invClient.GetClusterInventoryObjs(localInv)
-		if err != nil {
-			return nil, nil, err
-		}
-		if len(prevInvObjs) > 1 {
-			panic(fmt.Errorf("found %d inv objects with Name strategy", len(prevInvObjs)))
-		}
-		if len(prevInvObjs) == 1 {
-			invObj := prevInvObjs[0]
-			val := invObj.GetLabels()[common.InventoryLabel]
-			if val != localInv.ID() {
-				return nil, nil, fmt.Errorf("inventory-id of inventory object in cluster doesn't match provided id %q", localInv.ID())
-			}
-		}
-	}
 	pruneObjs, err := a.pruner.GetPruneObjs(localInv, localObjs, prune.Options{
 		DryRunStrategy: o.DryRunStrategy,
 	})

pkg/apply/solver/solver.go

@@ -158,11 +158,13 @@ func (t *TaskQueueBuilder) Build(taskContext *taskrunner.TaskContext, o Options)
 		// InvAddTask creates the inventory and adds any objects being applied
 		klog.V(2).Infof("adding inventory add task (%d objects)", len(applyObjs))
 		tasks = append(tasks, &task.InvAddTask{
-			TaskName:  "inventory-add-0",
-			InvClient: t.InvClient,
-			InvInfo:   t.invInfo,
-			Objects:   applyObjs,
-			DryRun:    o.DryRunStrategy,
+			TaskName:      "inventory-add-0",
+			InvClient:     t.InvClient,
+			DynamicClient: t.DynamicClient,
+			Mapper:        t.Mapper,
+			InvInfo:       t.invInfo,
+			Objects:       applyObjs,
+			DryRun:        o.DryRunStrategy,
 		})
 	}
 

pkg/apply/solver/solver_test.go

@@ -803,6 +803,8 @@ func TestTaskQueueBuilder_ApplyBuild(t *testing.T) {
 					typedTask.Mapper = mapper
 				case *taskrunner.WaitTask:
 					typedTask.Mapper = mapper
+				case *task.InvAddTask:
+					typedTask.Mapper = mapper
 				}
 			}
 
@@ -1477,6 +1479,8 @@ func TestTaskQueueBuilder_PruneBuild(t *testing.T) {
 					typedTask.Pruner = &prune.Pruner{}
 				case *taskrunner.WaitTask:
 					typedTask.Mapper = mapper
+				case *task.InvAddTask:
+					typedTask.Mapper = mapper
 				}
 			}
 
@@ -1831,6 +1835,8 @@ func TestTaskQueueBuilder_ApplyPruneBuild(t *testing.T) {
 					typedTask.Pruner = &prune.Pruner{}
 				case *taskrunner.WaitTask:
 					typedTask.Mapper = mapper
+				case *task.InvAddTask:
+					typedTask.Mapper = mapper
 				}
 			}
 

pkg/apply/task/inv_add_task.go

@@ -4,9 +4,16 @@
 package task
 
 import (
+	"context"
+	"fmt"
+
+	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/dynamic"
 	"k8s.io/klog/v2"
+	"k8s.io/kubectl/pkg/util"
 	"sigs.k8s.io/cli-utils/pkg/apply/event"
 	"sigs.k8s.io/cli-utils/pkg/apply/taskrunner"
 	"sigs.k8s.io/cli-utils/pkg/common"
@@ -22,11 +29,13 @@ var (
 // into the cluster. The InvAddTask should add/merge inventory references
 // before the actual object is applied.
 type InvAddTask struct {
-	TaskName  string
-	InvClient inventory.Client
-	InvInfo   inventory.Info
-	Objects   object.UnstructuredSet
-	DryRun    common.DryRunStrategy
+	TaskName      string
+	InvClient     inventory.Client
+	DynamicClient dynamic.Interface
+	Mapper        meta.RESTMapper
+	InvInfo       inventory.Info
+	Objects       object.UnstructuredSet
+	DryRun        common.DryRunStrategy
 }
 
 func (i *InvAddTask) Name() string {
@@ -50,12 +59,14 @@ func (i *InvAddTask) Start(taskContext *taskrunner.TaskContext) {
 			i.sendTaskResult(taskContext, err)
 			return
 		}
-		// Ensures the namespace exists before applying the inventory object into it.
-		if invNamespace := inventoryNamespaceInSet(i.InvInfo, i.Objects); invNamespace != nil {
-			klog.V(4).Infof("applying inventory namespace %s", invNamespace.GetName())
-			if err := i.InvClient.ApplyInventoryNamespace(invNamespace, i.DryRun); err != nil {
-				i.sendTaskResult(taskContext, err)
-				return
+		// If the inventory is namespaced, ensure the namespace exists
+		if i.InvInfo.Namespace() != "" {
+			if invNamespace := inventoryNamespaceInSet(i.InvInfo, i.Objects); invNamespace != nil {
+				if err := i.createNamespace(context.TODO(), invNamespace, i.DryRun); err != nil {
+					err = fmt.Errorf("failed to create inventory namespace: %w", err)
+					i.sendTaskResult(taskContext, err)
+					return
+				}
 			}
 		}
 		klog.V(4).Infof("merging %d local objects into inventory", len(i.Objects))
@@ -90,6 +101,35 @@ func inventoryNamespaceInSet(inv inventory.Info, objs object.UnstructuredSet) *u
 	return nil
 }
 
+// createNamespace creates the specified namespace object
+func (i *InvAddTask) createNamespace(ctx context.Context, obj *unstructured.Unstructured, dryRun common.DryRunStrategy) error {
+	if dryRun.ClientOrServerDryRun() {
+		klog.V(4).Infof("skipped applying inventory namespace (dry-run): %s", obj.GetName())
+		return nil
+	}
+	klog.V(4).Infof("applying inventory namespace: %s", obj.GetName())
+
+	nsObj := obj.DeepCopy()
+	object.StripKyamlAnnotations(nsObj)
+	if err := util.CreateApplyAnnotation(nsObj, unstructured.UnstructuredJSONScheme); err != nil {
+		return err
+	}
+
+	mapping, err := i.getMapping(obj)
+	if err != nil {
+		return err
+	}
+
+	_, err = i.DynamicClient.Resource(mapping.Resource).Create(ctx, nsObj, metav1.CreateOptions{})
+	return err
+}
+
+// getMapping returns the RESTMapping for the provided resource.
+func (i *InvAddTask) getMapping(obj *unstructured.Unstructured) (*meta.RESTMapping, error) {
+	gvk := obj.GroupVersionKind()
+	return i.Mapper.RESTMapping(gvk.GroupKind(), gvk.Version)
+}
+
 func (i *InvAddTask) sendTaskResult(taskContext *taskrunner.TaskContext, err error) {
 	klog.V(2).Infof("inventory add task completing (name: %q)", i.Name())
 	taskContext.TaskChannel() <- taskrunner.TaskResult{

pkg/apply/task/inv_add_task_test.go

@@ -7,6 +7,9 @@ import (
 	"testing"
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
+	clienttesting "k8s.io/client-go/testing"
+	cmdtesting "k8s.io/kubectl/pkg/cmd/testing"
 	"sigs.k8s.io/cli-utils/pkg/apply/cache"
 	"sigs.k8s.io/cli-utils/pkg/apply/event"
 	"sigs.k8s.io/cli-utils/pkg/apply/taskrunner"
@@ -66,17 +69,30 @@ var obj3 = &unstructured.Unstructured{
 	},
 }
 
+var nsObj = &unstructured.Unstructured{
+	Object: map[string]interface{}{
+		"apiVersion": "v1",
+		"kind":       "Namespace",
+		"metadata": map[string]interface{}{
+			"name": namespace,
+		},
+	},
+}
+
 const taskName = "test-inventory-task"
 
 func TestInvAddTask(t *testing.T) {
 	id1 := object.UnstructuredToObjMetadata(obj1)
 	id2 := object.UnstructuredToObjMetadata(obj2)
 	id3 := object.UnstructuredToObjMetadata(obj3)
+	idNs := object.UnstructuredToObjMetadata(nsObj)
 
 	tests := map[string]struct {
-		initialObjs  object.ObjMetadataSet
-		applyObjs    []*unstructured.Unstructured
-		expectedObjs object.ObjMetadataSet
+		initialObjs           object.ObjMetadataSet
+		applyObjs             []*unstructured.Unstructured
+		expectedObjs          object.ObjMetadataSet
+		reactorError          error
+		expectCreateNamespace bool
 	}{
 		"no initial inventory and no apply objects; no merged inventory": {
 			initialObjs:  object.ObjMetadataSet{},
@@ -103,6 +119,12 @@ func TestInvAddTask(t *testing.T) {
 			applyObjs:    []*unstructured.Unstructured{obj2, obj3},
 			expectedObjs: object.ObjMetadataSet{id1, id2, id3},
 		},
+		"namespace of inventory inside inventory": {
+			initialObjs:           object.ObjMetadataSet{},
+			applyObjs:             []*unstructured.Unstructured{nsObj},
+			expectedObjs:          object.ObjMetadataSet{idNs},
+			expectCreateNamespace: true,
+		},
 	}
 
 	for name, tc := range tests {
@@ -111,12 +133,27 @@ func TestInvAddTask(t *testing.T) {
 			eventChannel := make(chan event.Event)
 			resourceCache := cache.NewResourceCacheMap()
 			context := taskrunner.NewTaskContext(eventChannel, resourceCache)
+			tf := cmdtesting.NewTestFactory().WithNamespace(namespace)
+			defer tf.Cleanup()
+
+			createdNamespace := false
+			tf.FakeDynamicClient.PrependReactor("create", "namespaces", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {
+				createdNamespace = true
+				return true, nil, tc.reactorError
+			})
+
+			mapper, err := tf.ToRESTMapper()
+			if err != nil {
+				t.Fatal(err)
+			}
 
 			task := InvAddTask{
-				TaskName:  taskName,
-				InvClient: client,
-				InvInfo:   nil,
-				Objects:   tc.applyObjs,
+				TaskName:      taskName,
+				InvClient:     client,
+				InvInfo:       localInv,
+				Objects:       tc.applyObjs,
+				DynamicClient: tf.FakeDynamicClient,
+				Mapper:        mapper,
 			}
 			if taskName != task.Name() {
 				t.Errorf("expected task name (%s), got (%s)", taskName, task.Name())
@@ -134,6 +171,9 @@ func TestInvAddTask(t *testing.T) {
 			if !tc.expectedObjs.Equal(actual) {
 				t.Errorf("expected merged inventory (%s), got (%s)", tc.expectedObjs, actual)
 			}
+			if createdNamespace != tc.expectCreateNamespace {
+				t.Errorf("expected create namespace %v, got %v", tc.expectCreateNamespace, createdNamespace)
+			}
 		})
 	}
 }

pkg/inventory/inventory-client.go

@@ -16,7 +16,6 @@ import (
 	"k8s.io/client-go/dynamic"
 	"k8s.io/klog/v2"
 	cmdutil "k8s.io/kubectl/pkg/cmd/util"
-	"k8s.io/kubectl/pkg/util"
 	"sigs.k8s.io/cli-utils/pkg/apis/actuation"
 	"sigs.k8s.io/cli-utils/pkg/common"
 	"sigs.k8s.io/cli-utils/pkg/object"
@@ -39,12 +38,6 @@ type Client interface {
 	Replace(inv Info, objs object.ObjMetadataSet, status []actuation.ObjectStatus, dryRun common.DryRunStrategy) error
 	// DeleteInventoryObj deletes the passed inventory object from the APIServer.
 	DeleteInventoryObj(inv Info, dryRun common.DryRunStrategy) error
-	// ApplyInventoryNamespace applies the Namespace that the inventory object should be in.
-	ApplyInventoryNamespace(invNamespace *unstructured.Unstructured, dryRun common.DryRunStrategy) error
-	// GetClusterInventoryInfo returns the cluster inventory object.
-	GetClusterInventoryInfo(inv Info) (*unstructured.Unstructured, error)
-	// GetClusterInventoryObjs looks up the inventory objects from the cluster.
-	GetClusterInventoryObjs(inv Info) (object.UnstructuredSet, error)
 	// ListClusterInventoryObjs returns a map mapping from inventory name to a list of cluster inventory objects
 	ListClusterInventoryObjs(ctx context.Context) (map[string]object.ObjMetadataSet, error)
 }
@@ -105,7 +98,7 @@ func NewClient(factory cmdutil.Factory,
 func (cic *ClusterClient) Merge(localInv Info, objs object.ObjMetadataSet, dryRun common.DryRunStrategy) (object.ObjMetadataSet, error) {
 	pruneIds := object.ObjMetadataSet{}
 	invObj := cic.invToUnstructuredFunc(localInv)
-	clusterInv, err := cic.GetClusterInventoryInfo(localInv)
+	clusterInv, err := cic.getClusterInventoryInfo(localInv)
 	if err != nil {
 		return pruneIds, err
 	}
@@ -174,7 +167,7 @@ func (cic *ClusterClient) Replace(localInv Info, objs object.ObjMetadataSet, sta
 		klog.V(4).Infoln("dry-run replace inventory object: not applied")
 		return nil
 	}
-	clusterInv, err := cic.GetClusterInventoryInfo(localInv)
+	clusterInv, err := cic.getClusterInventoryInfo(localInv)
 	if err != nil {
 		return fmt.Errorf("failed to read inventory from cluster: %w", err)
 	}
@@ -256,7 +249,7 @@ func (cic *ClusterClient) deleteInventoryObjsByLabel(inv Info, dryRun common.Dry
 // an error if one occurred.
 func (cic *ClusterClient) GetClusterObjs(localInv Info) (object.ObjMetadataSet, error) {
 	var objs object.ObjMetadataSet
-	clusterInv, err := cic.GetClusterInventoryInfo(localInv)
+	clusterInv, err := cic.getClusterInventoryInfo(localInv)
 	if err != nil {
 		return objs, fmt.Errorf("failed to read inventory from cluster: %w", err)
 	}
@@ -277,8 +270,8 @@ func (cic *ClusterClient) GetClusterObjs(localInv Info) (object.ObjMetadataSet,
 //
 // 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 *ClusterClient) GetClusterInventoryInfo(inv Info) (*unstructured.Unstructured, error) {
-	clusterInvObjects, err := cic.GetClusterInventoryObjs(inv)
+func (cic *ClusterClient) getClusterInventoryInfo(inv Info) (*unstructured.Unstructured, error) {
+	clusterInvObjects, err := cic.getClusterInventoryObjs(inv)
 	if err != nil {
 		return nil, fmt.Errorf("failed to read inventory objects from cluster: %w", err)
 	}
@@ -344,10 +337,18 @@ func (cic *ClusterClient) getClusterInventoryObjsByName(inv Info) (object.Unstru
 	if apierrors.IsNotFound(err) {
 		return object.UnstructuredSet{}, nil
 	}
+	if inv.ID() != "" {
+		if inventoryID, err := retrieveInventoryLabel(clusterInv); err != nil {
+			return nil, err
+		} else if inv.ID() != inventoryID {
+			return nil, fmt.Errorf("inventory-id of inventory object %s/%s in cluster doesn't match provided id %q",
+				inv.Namespace(), inv.Name(), inv.ID())
+		}
+	}
 	return object.UnstructuredSet{clusterInv}, nil
 }
 
-func (cic *ClusterClient) GetClusterInventoryObjs(inv Info) (object.UnstructuredSet, error) {
+func (cic *ClusterClient) getClusterInventoryObjs(inv Info) (object.UnstructuredSet, error) {
 	if inv == nil {
 		return nil, fmt.Errorf("inventoryInfo must be specified")
 	}
@@ -443,33 +444,6 @@ func (cic *ClusterClient) deleteInventoryObjByName(obj *unstructured.Unstructure
 		Delete(context.TODO(), obj.GetName(), metav1.DeleteOptions{})
 }
 
-// ApplyInventoryNamespace creates the passed namespace if it does not already
-// exist, or returns an error if one happened. NOTE: No error if already exists.
-func (cic *ClusterClient) ApplyInventoryNamespace(obj *unstructured.Unstructured, dryRun common.DryRunStrategy) error {
-	if dryRun.ClientOrServerDryRun() {
-		klog.V(4).Infof("dry-run apply inventory namespace (%s): not applied", obj.GetName())
-		return nil
-	}
-
-	invNamespace := obj.DeepCopy()
-	klog.V(4).Infof("applying inventory namespace: %s", obj.GetName())
-	object.StripKyamlAnnotations(invNamespace)
-	if err := util.CreateApplyAnnotation(invNamespace, unstructured.UnstructuredJSONScheme); err != nil {
-		return err
-	}
-
-	mapping, err := cic.getMapping(obj)
-	if err != nil {
-		return err
-	}
-
-	_, err = cic.dc.Resource(mapping.Resource).Create(context.TODO(), invNamespace, metav1.CreateOptions{})
-	if apierrors.IsAlreadyExists(err) {
-		return nil
-	}
-	return err
-}
-
 // getMapping returns the RESTMapping for the provided resource.
 func (cic *ClusterClient) getMapping(obj *unstructured.Unstructured) (*meta.RESTMapping, error) {
 	return cic.mapper.RESTMapping(obj.GroupVersionKind().GroupKind(), obj.GroupVersionKind().Version)