Use resource descriptor GVK for NewSession (#120)

RedbackThomson · web-flow · commit e526d548a752 · 2023-03-22T11:05:54.000Z
Description of changes: Some version of K8s do not reliably return `TypeMeta` information when you call `apiReader.Get()` (see kubernetes/kubernetes#3030 and kubernetes/kubernetes#80609). This is a [known bug](kubernetes-sigs/controller-runtime#1517) in `controller-runtime` that they don't plan on fixing. Parts of the code, namely around setting up the user agent, currently rely on these fields - and they are currently being given an empty struct (with empty strings for all values). To work around this bug, this PR has introduced a new `GroupVersionKind()` getter in the `ResourceDescriptor` (replacing the existing `GroupKind`), which returns a static description of the GVK. Then, rather than referencing the `RuntimeObject` `TypeMeta` properties, we can reference this new GVK getter anywhere in the runtime. It also replaces any existing use of `GroupKind` to now use `GroupVersionKind`. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
diff --git a/mocks/pkg/types/aws_resource_descriptor.go b/mocks/pkg/types/aws_resource_descriptor.go
diff --git a/mocks/pkg/types/aws_resource_reconciler.go b/mocks/pkg/types/aws_resource_reconciler.go
diff --git a/mocks/pkg/types/field_export_reconciler.go b/mocks/pkg/types/field_export_reconciler.go
diff --git a/pkg/runtime/adoption_reconciler.go b/pkg/runtime/adoption_reconciler.go
@@ -89,7 +89,7 @@ func (r *adoptionReconciler) reconcile(ctx context.Context, req ctrlrt.Request)
 		controllerRMF = v
 		break
 	}
-	if gk.Group != controllerRMF.ResourceDescriptor().GroupKind().Group {
+	if gk.Group != controllerRMF.ResourceDescriptor().GroupVersionKind().Group {
 		ackrtlog.DebugAdoptedResource(r.log, res, "target resource API group is not of this service. no-op")
 		return nil
 	}
@@ -112,11 +112,9 @@ func (r *adoptionReconciler) reconcile(ctx context.Context, req ctrlrt.Request)
 	region := r.getRegion(res)
 	roleARN := r.getRoleARN(acctID)
 	endpointURL := r.getEndpointURL(res)
+	gvk := targetDescriptor.GroupVersionKind()
 
-	sess, err := r.sc.NewSession(
-		region, &endpointURL, roleARN,
-		targetDescriptor.EmptyRuntimeObject().GetObjectKind().GroupVersionKind(),
-	)
+	sess, err := r.sc.NewSession(region, &endpointURL, roleARN, gvk)
 	if err != nil {
 		return err
 	}
diff --git a/pkg/runtime/field_export_reconciler.go b/pkg/runtime/field_export_reconciler.go
@@ -22,8 +22,8 @@ import (
 	jq "github.com/itchyny/gojq"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	runtime "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	ctrlrt "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -106,7 +106,7 @@ func (r *fieldExportReconciler) reconcileFieldExport(ctx context.Context, req ct
 		controllerRMF = v
 		break
 	}
-	if sourceGK.Group != controllerRMF.ResourceDescriptor().GroupKind().Group {
+	if sourceGK.Group != controllerRMF.ResourceDescriptor().GroupVersionKind().Group {
 		ackrtlog.DebugFieldExport(r.log, feObject, "target resource API group is not of this service. no-op")
 		return nil
 	}
@@ -382,7 +382,7 @@ func (r *fieldExportReconciler) writeToSecret(
 
 func (r *fieldExportReconciler) GetFieldExportsForResource(
 	ctx context.Context,
-	gk metav1.GroupKind,
+	gk schema.GroupKind,
 	nsn types.NamespacedName,
 ) ([]ackv1alpha1.FieldExport, error) {
 	listed := &ackv1alpha1.FieldExportList{}
@@ -682,7 +682,7 @@ func (r *fieldExportResourceReconciler) reconcileSourceResource(ctx context.Cont
 
 	// Get each of the exports referencing this AWS resource
 	exports, err := r.GetFieldExportsForResource(ctx,
-		*r.rd.GroupKind(),
+		r.rd.GroupVersionKind().GroupKind(),
 		types.NamespacedName{
 			Namespace: res.MetaObject().GetNamespace(),
 			Name:      res.MetaObject().GetName(),
diff --git a/pkg/runtime/field_export_reconciler_test.go b/pkg/runtime/field_export_reconciler_test.go
@@ -24,9 +24,9 @@ import (
 	"github.com/stretchr/testify/require"
 	"go.uber.org/zap/zapcore"
 	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8sobj "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	k8srtschema "k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	ctrlrtzap "sigs.k8s.io/controller-runtime/pkg/log/zap"
@@ -94,8 +94,8 @@ func mockFieldExportReconcilerWithResourceDescriptor(rd *mocks.AWSResourceDescri
 
 func mockResourceDescriptor() *mocks.AWSResourceDescriptor {
 	rd := &mocks.AWSResourceDescriptor{}
-	rd.On("GroupKind").Return(
-		&metav1.GroupKind{
+	rd.On("GroupVersionKind").Return(
+		schema.GroupVersionKind{
 			Group: "bookstore.services.k8s.aws",
 			Kind:  "fakeBook",
 		},
@@ -431,7 +431,7 @@ func TestFilterAllExports_HappyCase(t *testing.T) {
 			}
 			*list = mockList
 		})
-	gk := metav1.GroupKind{
+	gk := schema.GroupKind{
 		Group: BookGVK.Group,
 		Kind:  BookGVK.Kind,
 	}
@@ -465,7 +465,7 @@ func TestSync_HappyCaseResourceNoExports(t *testing.T) {
 			}
 			*list = mockList
 		})
-	gk := metav1.GroupKind{
+	gk := schema.GroupKind{
 		Group: BookGVK.Group,
 		Kind:  BookGVK.Kind,
 	}
diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go
@@ -25,7 +25,7 @@ import (
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	ctrlrt "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
@@ -76,13 +76,14 @@ type resourceReconciler struct {
 	resyncPeriod time.Duration
 }
 
-// GroupKind returns the string containing the API group and kind reconciled by
-// this reconciler
-func (r *resourceReconciler) GroupKind() *metav1.GroupKind {
+// GroupVersionKind returns the string containing the API group, version and
+// kind reconciled by this reconciler
+func (r *resourceReconciler) GroupVersionKind() *schema.GroupVersionKind {
 	if r.rd == nil {
 		return nil
 	}
-	return r.rd.GroupKind()
+	gvk := r.rd.GroupVersionKind()
+	return &gvk
 }
 
 // BindControllerManager sets up the AWSResourceReconciler with an instance
@@ -157,7 +158,7 @@ func (r *resourceReconciler) Reconcile(ctx context.Context, req ctrlrt.Request)
 	region := r.getRegion(desired)
 	roleARN := r.getRoleARN(acctID)
 	endpointURL := r.getEndpointURL(desired)
-	gvk := desired.RuntimeObject().GetObjectKind().GroupVersionKind()
+	gvk := r.rd.GroupVersionKind()
 	sess, err := r.sc.NewSession(region, &endpointURL, roleARN, gvk)
 	if err != nil {
 		return ctrlrt.Result{}, err
@@ -170,7 +171,7 @@ func (r *resourceReconciler) Reconcile(ctx context.Context, req ctrlrt.Request)
 		"region", region,
 		// All the fields for a resource that do not change during reconciliation
 		// can be initialized during resourceLogger creation
-		"kind", r.rd.GroupKind().Kind,
+		"kind", r.rd.GroupVersionKind().Kind,
 		"namespace", req.Namespace,
 		"name", req.Name,
 	)
@@ -1106,7 +1107,7 @@ func getResyncPeriod(rmf acktypes.AWSResourceManagerFactory, cfg ackcfg.Config)
 
 	// First, try to use a resource-specific resync period if provided in the resource
 	// resync period configuration.
-	resourceKind := rmf.ResourceDescriptor().GroupKind().Kind
+	resourceKind := rmf.ResourceDescriptor().GroupVersionKind().Kind
 	if duration, ok := drc[strings.ToLower(resourceKind)]; ok && duration > 0 {
 		return time.Duration(duration) * time.Second
 	}
@@ -1158,7 +1159,7 @@ func NewReconcilerWithClient(
 	rtLog := log.WithName("ackrt")
 	resyncPeriod := getResyncPeriod(rmf, cfg)
 	rtLog.V(1).Info("Initiating reconciler",
-		"reconciler kind", rmf.ResourceDescriptor().GroupKind().Kind,
+		"reconciler kind", rmf.ResourceDescriptor().GroupVersionKind().Kind,
 		"resync period seconds", resyncPeriod.Seconds(),
 	)
 	return &resourceReconciler{
diff --git a/pkg/runtime/reconciler_test.go b/pkg/runtime/reconciler_test.go
@@ -24,8 +24,8 @@ import (
 	"github.com/stretchr/testify/require"
 	"go.uber.org/zap/zapcore"
 	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8sobj "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	k8srtschema "k8s.io/apimachinery/pkg/runtime/schema"
 	ctrlrtzap "sigs.k8s.io/controller-runtime/pkg/log/zap"
 
@@ -123,8 +123,8 @@ func managerFactoryMocks(
 	*ackmocks.AWSResourceDescriptor,
 ) {
 	rd := &ackmocks.AWSResourceDescriptor{}
-	rd.On("GroupKind").Return(
-		&metav1.GroupKind{
+	rd.On("GroupVersionKind").Return(
+		schema.GroupVersionKind{
 			Group: "bookstore.services.k8s.aws",
 			Kind:  "fakeBook",
 		},
diff --git a/pkg/runtime/registry.go b/pkg/runtime/registry.go
@@ -44,7 +44,7 @@ func (r *Registry) GetResourceManagerFactories() []types.AWSResourceManagerFacto
 func (r *Registry) RegisterResourceManagerFactory(f types.AWSResourceManagerFactory) {
 	r.Lock()
 	defer r.Unlock()
-	r.resourceManagerFactories[f.ResourceDescriptor().GroupKind().String()] = f
+	r.resourceManagerFactories[f.ResourceDescriptor().GroupVersionKind().GroupKind().String()] = f
 }
 
 // NewRegistry returns a thread-safe Registry object
diff --git a/pkg/runtime/registry_test.go b/pkg/runtime/registry_test.go
@@ -17,7 +17,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 
 	ackrt "github.com/aws-controllers-k8s/runtime/pkg/runtime"
 
@@ -28,8 +28,8 @@ func TestRegistry(t *testing.T) {
 	require := require.New(t)
 
 	rd := &mocks.AWSResourceDescriptor{}
-	rd.On("GroupKind").Return(
-		&metav1.GroupKind{
+	rd.On("GroupVersionKind").Return(
+		schema.GroupVersionKind{
 			Group: "bookstore.services.k8s.aws",
 			Kind:  "Book",
 		},
@@ -49,5 +49,5 @@ func TestRegistry(t *testing.T) {
 	require.Contains(rmfs, rmf)
 
 	rmf.AssertCalled(t, "ResourceDescriptor")
-	rd.AssertCalled(t, "GroupKind")
+	rd.AssertCalled(t, "GroupVersionKind")
 }
diff --git a/pkg/runtime/service_controller.go b/pkg/runtime/service_controller.go
@@ -167,7 +167,7 @@ func (c *serviceController) WithResourceManagerFactories(
 	}
 
 	for _, rmf := range rmfs {
-		c.rmFactories[rmf.ResourceDescriptor().GroupKind().String()] = rmf
+		c.rmFactories[rmf.ResourceDescriptor().GroupVersionKind().GroupKind().String()] = rmf
 	}
 	return c
 }
diff --git a/pkg/runtime/service_controller_test.go b/pkg/runtime/service_controller_test.go
@@ -131,8 +131,8 @@ func TestServiceController(t *testing.T) {
 	require := require.New(t)
 
 	rd := &mocks.AWSResourceDescriptor{}
-	rd.On("GroupKind").Return(
-		&metav1.GroupKind{
+	rd.On("GroupVersionKind").Return(
+		schema.GroupVersionKind{
 			Group: "bookstore.services.k8s.aws",
 			Kind:  "fakeBook",
 		},
@@ -180,7 +180,7 @@ func TestServiceController(t *testing.T) {
 
 	foundfakeBookRecon := false
 	for _, recon := range recons {
-		if recon.GroupKind().String() == "fakeBook.bookstore.services.k8s.aws" {
+		if recon.GroupVersionKind().GroupKind().String() == "fakeBook.bookstore.services.k8s.aws" {
 			foundfakeBookRecon = true
 		}
 	}
diff --git a/pkg/types/aws_resource_descriptor.go b/pkg/types/aws_resource_descriptor.go
@@ -14,7 +14,7 @@
 package types
 
 import (
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	rtclient "sigs.k8s.io/controller-runtime/pkg/client"
 
 	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
@@ -25,9 +25,10 @@ import (
 // prototype for that AWSResource, and the relationships between the
 // AWSResource and other AWSResources
 type AWSResourceDescriptor interface {
-	// GroupKind returns a Kubernetes metav1.GroupKind struct that describes
-	// the API Group and Kind of CRs described by the descriptor
-	GroupKind() *metav1.GroupKind
+	// GroupVersionKind returns a Kubernetes schema.GroupVersionKind struct that
+	// describes the API Group, Version and Kind of CRs described by the
+	// descriptor
+	GroupVersionKind() schema.GroupVersionKind
 	// EmptyRuntimeObject returns an empty object prototype that may be used in
 	// apimachinery and k8s client operations
 	EmptyRuntimeObject() rtclient.Object
diff --git a/pkg/types/aws_resource_reconciler.go b/pkg/types/aws_resource_reconciler.go
@@ -16,7 +16,7 @@ package types
 import (
 	"context"
 
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	ctrlrt "sigs.k8s.io/controller-runtime"
 )
 
@@ -34,10 +34,9 @@ type AWSResourceReconciler interface {
 	// BindControllerManager sets up the AWSResourceReconciler with an instance
 	// of an upstream controller-runtime.Manager
 	BindControllerManager(ctrlrt.Manager) error
-	// GroupKind returns the
-	// sigs.k8s.io/apimachinery/pkg/apis/meta/v1.GroupKind containing the API
-	// group and kind reconciled by this reconciler
-	GroupKind() *metav1.GroupKind
+	// GroupKind returns the schema.GroupVersionKind containing the API group,
+	// version and kind reconciled by this reconciler
+	GroupVersionKind() *schema.GroupVersionKind
 	// Sync ensures that the supplied AWSResource's backing API resource
 	// matches the supplied desired state.
 	//
diff --git a/pkg/types/field_export_reconciler.go b/pkg/types/field_export_reconciler.go

Original file line number	Diff line number	Diff line change
`@@ -44,7 +44,7 @@ func (r *Registry) GetResourceManagerFactories() []types.AWSResourceManagerFacto`
`44`	`44`	`func (r *Registry) RegisterResourceManagerFactory(f types.AWSResourceManagerFactory) {`
`45`	`45`	`r.Lock()`
`46`	`46`	`defer r.Unlock()`
`47`		`- r.resourceManagerFactories[f.ResourceDescriptor().GroupKind().String()] = f`
	`47`	`+ r.resourceManagerFactories[f.ResourceDescriptor().GroupVersionKind().GroupKind().String()] = f`
`48`	`48`	`}`
`49`	`49`
`50`	`50`	`// NewRegistry returns a thread-safe Registry object`