[WIP] Remove GVK from typed objects

- Fix all the tests that required or assumed typed objects always
  have GKV set.
- Panic from IDOf, GKNNOf, and GVKNNOf if the typed object is not
  in the scheme. In most cases this only affects tests. Non-tests
  will need to be refactored to handle the possible error without
  panicing.
- Fix multiple fake client bugs
- Add unit test for reconciler-manager that use a simulator to
  update deployment status.
- Validate that RootSync persists until all managed objects are
  deleted, even if they were blocked with finalizers.

TODO: break up into multiple PRs.

cmd/nomoserrors/examples/examples.go

@@ -289,7 +289,7 @@ func Generate() AllExamples {
 	result.add(status.ManagementConflictErrorWrap(fake.Role(), declared.ResourceManager(declared.RootReconciler, configsync.RootSyncName)))
 
 	// 1061
-	result.add(validate.MissingGitRepo(fake.RepoSyncObjectV1Beta1("bookstore", configsync.RepoSyncName)))
+	result.add(validate.MissingGitRepo(fake.RepoSyncObjectV1Beta1("bookstore", configsync.RepoSyncName), configsync.RepoSyncKind))
 
 	// 1062 is Deprecated.
 	result.markDeprecated("1062")

e2e/nomostest/config_sync.go

@@ -31,6 +31,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"kpt.dev/configsync/e2e"
 	"kpt.dev/configsync/e2e/nomostest/gitproviders"
@@ -936,17 +937,21 @@ func SetRepoSyncDependencies(nt *NT, rs client.Object) error {
 		nt.RepoSyncClusterRole(),
 		RepoSyncRoleBinding(rsNN),
 	}
-	return SetDependencies(rs, dependencies...)
+	return SetDependencies(rs, nt.Scheme, dependencies...)
 }
 
 // SetDependencies sets the specified objects as dependencies of the first object.
-func SetDependencies(obj client.Object, dependencies ...client.Object) error {
+func SetDependencies(obj client.Object, scheme *runtime.Scheme, dependencies ...client.Object) error {
 	var deps dependson.DependencySet
 	for _, dep := range dependencies {
-		deps = append(deps, applier.ObjMetaFromObject(dep))
+		objMeta, err := applier.ObjMetaFromObject(dep, scheme)
+		if err != nil {
+			return errors.Wrapf(err, "failed to set dependencies on %s", kinds.ObjectSummary(obj))
+		}
+		deps = append(deps, objMeta)
 	}
 	if err := setDependsOnAnnotation(obj, deps); err != nil {
-		return errors.Wrapf(err, "failed to set dependencies on %T %s", obj, client.ObjectKeyFromObject(obj))
+		return errors.Wrapf(err, "failed to set dependencies on %s", kinds.ObjectSummary(obj))
 	}
 	return nil
 }

e2e/nomostest/wait_for_sync.go

@@ -389,7 +389,6 @@ func (nt *NT) WaitForRootSyncStalledError(rsNamespace, rsName, reason, message s
 					Name:      rsName,
 					Namespace: rsNamespace,
 				},
-				TypeMeta: fake.ToTypeMeta(kinds.RootSyncV1Beta1()),
 			}
 			if err := nt.KubeClient.Get(rsName, rsNamespace, rs); err != nil {
 				return err
@@ -420,7 +419,6 @@ func (nt *NT) WaitForRepoSyncStalledError(rsNamespace, rsName, reason, message s
 					Name:      rsName,
 					Namespace: rsNamespace,
 				},
-				TypeMeta: fake.ToTypeMeta(kinds.RepoSyncV1Beta1()),
 			}
 			if err := nt.KubeClient.Get(rsName, rsNamespace, rs); err != nil {
 				return err

e2e/testcases/composition_test.go

@@ -118,7 +118,7 @@ func TestComposition(t *testing.T) {
 	// and RootSync level-1 is manage by RootSync root-sync (level-0),
 	// we need to make RepoSync level-1 depend on both RoleBindings,
 	// so the RoleBindings are deleted after both RepoSyncs.
-	if err := nomostest.SetDependencies(lvl1Sync, lvl2RB, lvl3RB); err != nil {
+	if err := nomostest.SetDependencies(lvl1Sync, nt.Scheme, lvl2RB, lvl3RB); err != nil {
 		nt.T.Fatal(err)
 	}
 

e2e/testcases/multi_sync_test.go

@@ -177,7 +177,7 @@ func TestMultiSyncs_Unstructured_MixedControl(t *testing.T) {
 	nt.NonRootRepos[nn3] = nn3Repo
 	nrs3 := nomostest.RepoSyncObjectV1Alpha1FromNonRootRepo(nt, nn3)
 	// Ensure the RoleBinding is deleted after the RepoSync
-	if err := nomostest.SetDependencies(nrs3, nrb3); err != nil {
+	if err := nomostest.SetDependencies(nrs3, nt.Scheme, nrb3); err != nil {
 		nt.T.Fatal(err)
 	}
 	nt.Must(nt.RootRepos[configsync.RootSyncName].Add(fmt.Sprintf("acme/reposyncs/%s.yaml", nn3.Name), nrs3))
@@ -204,7 +204,7 @@ func TestMultiSyncs_Unstructured_MixedControl(t *testing.T) {
 	nt.NonRootRepos[nn5] = nn5Repo
 	nrs5 := nomostest.RepoSyncObjectV1Beta1FromNonRootRepo(nt, nn5)
 	// Ensure the RoleBinding is deleted after the RepoSync
-	if err := nomostest.SetDependencies(nrs5, nrb5); err != nil {
+	if err := nomostest.SetDependencies(nrs5, nt.Scheme, nrb5); err != nil {
 		nt.T.Fatal(err)
 	}
 	nt.Must(nt.RootRepos[rr1].Add(fmt.Sprintf("acme/reposyncs/%s.yaml", nn5.Name), nrs5))

e2e/testcases/oci_sync_test.go

@@ -334,7 +334,7 @@ func TestSwitchFromGitToOci(t *testing.T) {
 		Auth:   configsync.AuthNone,
 	}
 	// Ensure the RoleBinding & ClusterRole are deleted after the RepoSync
-	if err := nomostest.SetDependencies(repoSyncGit, rsRB, rsCR); err != nil {
+	if err := nomostest.SetDependencies(repoSyncGit, nt.Scheme, rsRB, rsCR); err != nil {
 		nt.T.Fatal(err)
 	}
 
@@ -406,7 +406,7 @@ func TestSwitchFromGitToOci(t *testing.T) {
 		Auth:  configsync.AuthNone,
 	}
 	// Ensure the RoleBinding & ClusterRole are deleted after the RepoSync
-	if err := nomostest.SetDependencies(repoSyncOCI, rsRB, rsCR); err != nil {
+	if err := nomostest.SetDependencies(repoSyncOCI, nt.Scheme, rsRB, rsCR); err != nil {
 		nt.T.Fatal(err)
 	}
 	nt.Must(nt.RootRepos[configsync.RootSyncName].Add(repoSyncPath, repoSyncOCI))

manifests/reposync-crd.yaml

@@ -589,6 +589,7 @@ spec:
                   digest.
                 type: string
               observedGeneration:
+                default: 0
                 description: observedGeneration is the most recent generation observed
                   for the sync resource. It corresponds to the it's generation, which
                   is updated on mutation by the API Server.
@@ -1637,6 +1638,7 @@ spec:
                   digest.
                 type: string
               observedGeneration:
+                default: 0
                 description: observedGeneration is the most recent generation observed
                   for the sync resource. It corresponds to the it's generation, which
                   is updated on mutation by the API Server.

manifests/resourcegroup-crd.yaml

@@ -165,6 +165,7 @@ spec:
                   type: object
                 type: array
               observedGeneration:
+                default: 0
                 description: observedGeneration is the most recent generation observed.
                   It corresponds to the Object's generation, which is updated on mutation
                   by the API Server. Everytime the controller does a successful reconcile,

manifests/rootsync-crd.yaml

@@ -644,6 +644,7 @@ spec:
                   digest.
                 type: string
               observedGeneration:
+                default: 0
                 description: observedGeneration is the most recent generation observed
                   for the sync resource. It corresponds to the it's generation, which
                   is updated on mutation by the API Server.
@@ -1747,6 +1748,7 @@ spec:
                   digest.
                 type: string
               observedGeneration:
+                default: 0
                 description: observedGeneration is the most recent generation observed
                   for the sync resource. It corresponds to the it's generation, which
                   is updated on mutation by the API Server.

pkg/api/configsync/v1alpha1/sync_types.go

@@ -22,7 +22,7 @@ import (
 type Status struct {
 	// observedGeneration is the most recent generation observed for the sync resource.
 	// It corresponds to the it's generation, which is updated on mutation by the API Server.
-	// +optional
+	// +kubebuilder:default:=0
 	ObservedGeneration int64 `json:"observedGeneration,omitempty"`
 
 	// reconciler is the name of the reconciler process which corresponds to the

pkg/api/configsync/v1beta1/sync_types.go

@@ -22,7 +22,7 @@ import (
 type Status struct {
 	// observedGeneration is the most recent generation observed for the sync resource.
 	// It corresponds to the it's generation, which is updated on mutation by the API Server.
-	// +optional
+	// +kubebuilder:default:=0
 	ObservedGeneration int64 `json:"observedGeneration,omitempty"`
 
 	// reconciler is the name of the reconciler process which corresponds to the

pkg/api/kpt.dev/v1alpha1/resourcegroup_types.go

@@ -67,15 +67,19 @@ type ResourceGroupStatus struct {
 	// mutation by the API Server.
 	// Everytime the controller does a successful reconcile, it sets
 	// observedGeneration to match ResourceGroup.metadata.generation.
+	// +kubebuilder:default:=0
 	ObservedGeneration int64 `json:"observedGeneration"`
 
 	// resourceStatuses lists the status for each resource in the group
+	// +optional
 	ResourceStatuses []ResourceStatus `json:"resourceStatuses,omitempty"`
 
 	// subgroupStatuses lists the status for each subgroup.
+	// +optional
 	SubgroupStatuses []GroupStatus `json:"subgroupStatuses,omitempty"`
 
 	// conditions lists the conditions of the current status for the group
+	// +optional
 	Conditions []Condition `json:"conditions,omitempty"`
 }
 

pkg/applier/applier.go

@@ -828,7 +828,10 @@ func (h *eventHandler) removeFromInventory(rg *live.InventoryResourceGroup, objs
 	if err != nil {
 		return err
 	}
-	newObjs := removeFrom(oldObjs, objs)
+	newObjs, err := removeFrom(oldObjs, objs, h.clientSet.Client.Scheme())
+	if err != nil {
+		return err
+	}
 	err = rg.Store(newObjs, nil)
 	if err != nil {
 		return err

pkg/applier/utils.go

@@ -16,14 +16,17 @@ package applier
 
 import (
 	"encoding/json"
+	"fmt"
 	"sort"
 	"strings"
 
 	"github.com/GoogleContainerTools/kpt/pkg/live"
 	"golang.org/x/net/context"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
 	"kpt.dev/configsync/pkg/core"
+	"kpt.dev/configsync/pkg/kinds"
 	"kpt.dev/configsync/pkg/metadata"
 	"kpt.dev/configsync/pkg/status"
 	syncerreconcile "kpt.dev/configsync/pkg/syncer/reconcile"
@@ -61,12 +64,19 @@ func toUnstructured(objs []client.Object) ([]*unstructured.Unstructured, status.
 }
 
 // ObjMetaFromObject constructs an ObjMetadata representing the Object.
-func ObjMetaFromObject(obj client.Object) object.ObjMetadata {
+//
+// Errors if the GroupKind is not set and not registered in core.Scheme.
+func ObjMetaFromObject(obj client.Object, scheme *runtime.Scheme) (object.ObjMetadata, error) {
+	gvk, err := kinds.Lookup(obj, scheme)
+	if err != nil {
+		return object.ObjMetadata{},
+			fmt.Errorf("ObjMetaFromObject: failed to lookup GroupKind of %T: %w", obj, err)
+	}
 	return object.ObjMetadata{
 		Namespace: obj.GetNamespace(),
 		Name:      obj.GetName(),
-		GroupKind: obj.GetObjectKind().GroupVersionKind().GroupKind(),
-	}
+		GroupKind: gvk.GroupKind(),
+	}, nil
 }
 
 func objMetaFromID(id core.ID) object.ObjMetadata {
@@ -97,25 +107,24 @@ func idFromInventory(rg *live.InventoryResourceGroup) core.ID {
 	}
 }
 
-func removeFrom(all []object.ObjMetadata, toRemove []client.Object) []object.ObjMetadata {
+func removeFrom(all []object.ObjMetadata, toRemove []client.Object, scheme *runtime.Scheme) ([]object.ObjMetadata, error) {
 	m := map[object.ObjMetadata]bool{}
 	for _, a := range all {
 		m[a] = true
 	}
 
 	for _, r := range toRemove {
-		meta := object.ObjMetadata{
-			Namespace: r.GetNamespace(),
-			Name:      r.GetName(),
-			GroupKind: r.GetObjectKind().GroupVersionKind().GroupKind(),
+		meta, err := ObjMetaFromObject(r, scheme)
+		if err != nil {
+			return nil, err
 		}
 		delete(m, meta)
 	}
 	var results []object.ObjMetadata
 	for key := range m {
 		results = append(results, key)
 	}
-	return results
+	return results, nil
 }
 
 func getObjectSize(u *unstructured.Unstructured) (int, error) {

pkg/applier/utils_test.go

@@ -19,6 +19,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"kpt.dev/configsync/pkg/api/configsync"
 	"kpt.dev/configsync/pkg/core"
@@ -86,15 +87,15 @@ func TestObjMetaFrom(t *testing.T) {
 			Kind:  "Deployment",
 		},
 	}
-	actual := ObjMetaFromObject(d)
+	actual := mustObjMetaFromObject(t, d)
 	if actual != expected {
 		t.Errorf("expected %v but got %v", expected, actual)
 	}
 }
 
 func TestIDFrom(t *testing.T) {
 	d := fake.DeploymentObject(core.Name("deploy"), core.Namespace("default"))
-	meta := ObjMetaFromObject(d)
+	meta := mustObjMetaFromObject(t, d)
 	id := idFrom(meta)
 	if id != core.IDOf(d) {
 		t.Errorf("expected %v but got %v", core.IDOf(d), id)
@@ -111,47 +112,47 @@ func TestRemoveFrom(t *testing.T) {
 		{
 			name: "toRemove is empty",
 			allObjMeta: []object.ObjMetadata{
-				ObjMetaFromObject(fake.DeploymentObject(core.Name("deploy"), core.Namespace("default"))),
-				ObjMetaFromObject(fake.ServiceObject(core.Name("service"), core.Namespace("default"))),
+				mustObjMetaFromObject(t, fake.DeploymentObject(core.Name("deploy"), core.Namespace("default"))),
+				mustObjMetaFromObject(t, fake.ServiceObject(core.Name("service"), core.Namespace("default"))),
 			},
 			objs: nil,
 			expected: []object.ObjMetadata{
-				ObjMetaFromObject(fake.DeploymentObject(core.Name("deploy"), core.Namespace("default"))),
-				ObjMetaFromObject(fake.ServiceObject(core.Name("service"), core.Namespace("default"))),
+				mustObjMetaFromObject(t, fake.DeploymentObject(core.Name("deploy"), core.Namespace("default"))),
+				mustObjMetaFromObject(t, fake.ServiceObject(core.Name("service"), core.Namespace("default"))),
 			},
 		},
 		{
 			name: "all toRemove are in the original list",
 			allObjMeta: []object.ObjMetadata{
-				ObjMetaFromObject(fake.DeploymentObject(core.Name("deploy"), core.Namespace("default"))),
-				ObjMetaFromObject(fake.ServiceObject(core.Name("service"), core.Namespace("default"))),
+				mustObjMetaFromObject(t, fake.DeploymentObject(core.Name("deploy"), core.Namespace("default"))),
+				mustObjMetaFromObject(t, fake.ServiceObject(core.Name("service"), core.Namespace("default"))),
 			},
 			objs: []client.Object{
 				fake.ServiceObject(core.Name("service"), core.Namespace("default")),
 			},
 			expected: []object.ObjMetadata{
-				ObjMetaFromObject(fake.DeploymentObject(core.Name("deploy"), core.Namespace("default"))),
+				mustObjMetaFromObject(t, fake.DeploymentObject(core.Name("deploy"), core.Namespace("default"))),
 			},
 		},
 		{
 			name: "some toRemove are not in the original list",
 			allObjMeta: []object.ObjMetadata{
-				ObjMetaFromObject(fake.DeploymentObject(core.Name("deploy"), core.Namespace("default"))),
-				ObjMetaFromObject(fake.ServiceObject(core.Name("service"), core.Namespace("default"))),
+				mustObjMetaFromObject(t, fake.DeploymentObject(core.Name("deploy"), core.Namespace("default"))),
+				mustObjMetaFromObject(t, fake.ServiceObject(core.Name("service"), core.Namespace("default"))),
 			},
 			objs: []client.Object{
 				fake.ServiceObject(core.Name("service"), core.Namespace("default")),
 				fake.ConfigMapObject(core.Name("cm"), core.Namespace("default")),
 			},
 			expected: []object.ObjMetadata{
-				ObjMetaFromObject(fake.DeploymentObject(core.Name("deploy"), core.Namespace("default"))),
+				mustObjMetaFromObject(t, fake.DeploymentObject(core.Name("deploy"), core.Namespace("default"))),
 			},
 		},
 		{
 			name: "toRemove are the same as original objects",
 			allObjMeta: []object.ObjMetadata{
-				ObjMetaFromObject(fake.DeploymentObject(core.Name("deploy"), core.Namespace("default"))),
-				ObjMetaFromObject(fake.ServiceObject(core.Name("service"), core.Namespace("default"))),
+				mustObjMetaFromObject(t, fake.DeploymentObject(core.Name("deploy"), core.Namespace("default"))),
+				mustObjMetaFromObject(t, fake.ServiceObject(core.Name("service"), core.Namespace("default"))),
 			},
 			objs: []client.Object{
 				fake.DeploymentObject(core.Name("deploy"), core.Namespace("default")),
@@ -162,7 +163,8 @@ func TestRemoveFrom(t *testing.T) {
 	}
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
-			actual := removeFrom(tc.allObjMeta, tc.objs)
+			actual, err := removeFrom(tc.allObjMeta, tc.objs, core.Scheme)
+			require.NoError(t, err)
 			if diff := cmp.Diff(tc.expected, actual, cmpopts.SortSlices(
 				func(x, y object.ObjMetadata) bool { return x.String() < y.String() })); diff != "" {
 				t.Errorf("%s: Diff of removeFrom is: %s", tc.name, diff)
@@ -181,3 +183,14 @@ func TestGetObjectSize(t *testing.T) {
 		t.Fatalf("An empty inventory object shouldn't have a large size: %d", size)
 	}
 }
+
+// mustObjMetaFromObject constructs an ObjMetadata representing the Object.
+//
+// Fails the test if the GroupKind is not set and not registered in core.Scheme.
+func mustObjMetaFromObject(t *testing.T, obj client.Object) object.ObjMetadata {
+	objMeta, err := ObjMetaFromObject(obj, core.Scheme)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return objMeta
+}