[WIP] Remove GVK from typed objects

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"
@@ -979,17 +980,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

@@ -411,7 +411,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
@@ -442,7 +441,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))

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

pkg/core/id.go

@@ -19,7 +19,10 @@ import (
 	"sort"
 	"strings"
 
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/klog/v2"
+	"kpt.dev/configsync/pkg/kinds"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -30,13 +33,36 @@ type ID struct {
 }
 
 // IDOf converts an Object to its ID.
+//
+// Panics if the GroupKind is not set and not registered in core.Scheme.
+//
+// Deprecated: Typed objects should not include GroupKind! Use LookupID instead,
+// and handle the error, instead of panicking or returning an ID with empty
+// GroupKind.
 func IDOf(o client.Object) ID {
+	gvk, err := kinds.Lookup(o, Scheme)
+	if err != nil {
+		klog.Fatalf("IDOf: Failed to lookup GroupKind of %T: %v", o, err)
+	}
 	return ID{
-		GroupKind: o.GetObjectKind().GroupVersionKind().GroupKind(),
+		GroupKind: gvk.GroupKind(),
 		ObjectKey: client.ObjectKey{Namespace: o.GetNamespace(), Name: o.GetName()},
 	}
 }
 
+// LookupID returns the ID of the Object.
+// Returns an error if the GroupKind is not set and not registered in the scheme.
+func LookupID(obj client.Object, scheme *runtime.Scheme) (ID, error) {
+	gvk, err := kinds.Lookup(obj, scheme)
+	if err != nil {
+		return ID{}, fmt.Errorf("failed to lookup GroupKind of %T: %w", obj, err)
+	}
+	return ID{
+		GroupKind: gvk.GroupKind(),
+		ObjectKey: client.ObjectKeyFromObject(obj),
+	}, nil
+}
+
 // String implements fmt.Stringer.
 func (i ID) String() string {
 	return fmt.Sprintf("%s, %s/%s", i.GroupKind.String(), i.Namespace, i.Name)
@@ -45,13 +71,24 @@ func (i ID) String() string {
 // GKNN is used to set and verify the `configsync.gke.io/resource-id` annotation.
 // Changing this function should be avoided, since it may
 // introduce incompability across different Config Sync versions.
+//
+// Panics if the GroupKind is not set and not registered in core.Scheme.
+//
+// Deprecated: Typed objects should not include GroupKind! Use LookupGKNN instead,
+// and handle the error, instead of panicking or returning a GKNN with empty
+// GroupKind.
 func GKNN(o client.Object) string {
 	if o == nil {
 		return ""
 	}
 
-	group := o.GetObjectKind().GroupVersionKind().Group
-	kind := o.GetObjectKind().GroupVersionKind().Kind
+	gvk, err := kinds.Lookup(o, Scheme)
+	if err != nil {
+		klog.Fatalf("GKNN: Failed to lookup GroupKind of %T: %v", o, err)
+	}
+
+	group := gvk.Group
+	kind := gvk.Kind
 	if o.GetNamespace() == "" {
 		return fmt.Sprintf("%s_%s_%s", group, strings.ToLower(kind), o.GetName())
 	}
@@ -60,6 +97,12 @@ func GKNN(o client.Object) string {
 
 // GKNNs returns the `configsync.gke.io/resource-id` annotations of th given objects as
 // a string slice in increasing order.
+//
+// Panics if the GroupKind is not set and not registered in core.Scheme.
+//
+// Deprecated: Typed objects should not include GroupKind! Use LookupGKNNs
+// instead, and handle the error, instead of panicking or returning a GKNN with
+// empty GroupKind.
 func GKNNs(objs []client.Object) []string {
 	var result []string
 	for _, obj := range objs {