From 9213aa01504e6cf4b390e407b2a04fe4b7bf3311 Mon Sep 17 00:00:00 2001
From: Hasan Turken <turkenh@gmail.com>
Date: Wed, 7 Feb 2024 10:30:08 +0300
Subject: [PATCH] Do not use version with indexing used resources

Signed-off-by: Hasan Turken <turkenh@gmail.com>
(cherry picked from commit f3f5ece2f65afc19ea8d16c88e5ef12889e8d813)
---
 .../apiextensions/usage/reconciler.go         | 10 +++++++++
 .../apiextensions/usage/reconciler_test.go    | 21 +++++++++++++++++++
 internal/usage/handler.go                     | 10 ++++++++-
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/internal/controller/apiextensions/usage/reconciler.go b/internal/controller/apiextensions/usage/reconciler.go
index 614751e27..e66b2df54 100644
--- a/internal/controller/apiextensions/usage/reconciler.go
+++ b/internal/controller/apiextensions/usage/reconciler.go
@@ -28,6 +28,7 @@ import (
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
@@ -70,6 +71,7 @@ const (
 	errAddFinalizer         = "cannot add finalizer"
 	errRemoveFinalizer      = "cannot remove finalizer"
 	errUpdateStatus         = "cannot update status of usage"
+	errParseAPIVersion      = "cannot parse APIVersion"
 )
 
 // Event reasons.
@@ -216,6 +218,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
 		log.Debug(errGetUsage, "error", err)
 		return reconcile.Result{}, errors.Wrap(xpresource.IgnoreNotFound(err), errGetUsage)
 	}
+
+	// Validate APIVersion of used object provided as input.
+	// We parse this value while indexing the objects, and we need to make sure it is valid.
+	_, err := schema.ParseGroupVersion(u.Spec.Of.APIVersion)
+	if err != nil {
+		return reconcile.Result{}, errors.Wrap(err, errParseAPIVersion)
+	}
+
 	orig := u.DeepCopy()
 
 	if err := r.usage.resolveSelectors(ctx, u); err != nil {
diff --git a/internal/controller/apiextensions/usage/reconciler_test.go b/internal/controller/apiextensions/usage/reconciler_test.go
index 3db85f9cf..3b10e3d1f 100644
--- a/internal/controller/apiextensions/usage/reconciler_test.go
+++ b/internal/controller/apiextensions/usage/reconciler_test.go
@@ -82,6 +82,27 @@ func TestReconcile(t *testing.T) {
 				r: reconcile.Result{},
 			},
 		},
+		"CannotParseApiVersion": {
+			reason: "We should return an error if we cannot parse APIVersion of used resource.",
+			args: args{
+				mgr: &fake.Manager{},
+				opts: []ReconcilerOption{
+					WithClientApplicator(xpresource.ClientApplicator{
+						Client: &test.MockClient{
+							MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
+								o := obj.(*v1alpha1.Usage)
+								o.Spec.Of.APIVersion = "/invalid/"
+								o.Spec.Of.ResourceSelector = &v1alpha1.ResourceSelector{MatchLabels: map[string]string{"foo": "bar"}}
+								return nil
+							}),
+						},
+					}),
+				},
+			},
+			want: want{
+				err: errors.Wrap(errors.New("unexpected GroupVersion string: /invalid/"), errParseAPIVersion),
+			},
+		},
 		"CannotResolveSelectors": {
 			reason: "We should return an error if we cannot resolve selectors.",
 			args: args{
diff --git a/internal/usage/handler.go b/internal/usage/handler.go
index 9a17ebc0d..2865c4621 100644
--- a/internal/usage/handler.go
+++ b/internal/usage/handler.go
@@ -25,6 +25,7 @@ import (
 	admissionv1 "k8s.io/api/admission/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -53,7 +54,14 @@ func IndexValueForObject(u *unstructured.Unstructured) string {
 }
 
 func indexValue(apiVersion, kind, name string) string {
-	return fmt.Sprintf("%s.%s.%s", apiVersion, kind, name)
+	// There are two sources for "apiVersion" input, one is from the unstructured objects fetched from k8s and the other
+	// is from the Usage spec. The one from the objects from k8s is already validated by the k8s API server, so we don't
+	// need to validate it again. The one from the Usage spec is validated by the Usage controller, so we don't need to
+	// validate it as well. So we can safely ignore the error here.
+	// Another reason to ignore the error is that the IndexerFunc using this value to index the objects does not return
+	// an error, so we cannot bubble up the error here.
+	gr, _ := schema.ParseGroupVersion(apiVersion)
+	return fmt.Sprintf("%s.%s.%s", gr.Group, kind, name)
 }
 
 // SetupWebhookWithManager sets up the webhook with the manager.