From ba98c8cc14a77baa636cb4adfff3bd384e2f3471 Mon Sep 17 00:00:00 2001
From: Ryan Richard <richardry@vmware.com>
Date: Wed, 21 Sep 2022 21:30:44 -0700
Subject: [PATCH] Enhance Kube middleware to rewrite API group of ownerRefs on
 update verb

When oidcclientsecretstorage.Set() wants to update the contents of the
storage Secret, it also wants to keep the original ownerRef of the
storage Secret, so it needs the middleware to rewrite the API group
of the ownerRef again during the update (just like it had initially done
during the create of the Secret).
---
 internal/groupsuffix/groupsuffix.go      |  7 ++--
 internal/groupsuffix/groupsuffix_test.go | 42 +++++++++++++++++++++---
 2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/internal/groupsuffix/groupsuffix.go b/internal/groupsuffix/groupsuffix.go
index 8d2d4477..cb1927c8 100644
--- a/internal/groupsuffix/groupsuffix.go
+++ b/internal/groupsuffix/groupsuffix.go
@@ -51,8 +51,11 @@ func New(apiGroupSuffix string) kubeclient.Middleware {
 		}),
 
 		kubeclient.MiddlewareFunc(func(_ context.Context, rt kubeclient.RoundTrip) {
-			// we should not mess with owner refs on things we did not create
-			if rt.Verb() != kubeclient.VerbCreate {
+			// Only mess with ownerRefs on requests to perform edits.
+			// Not needed on deletes since the object is getting deleted anyway.
+			// WARNING: This code might need to be enhanced to handle the patch verb
+			// if we start using patches for objects that have ownerRefs.
+			if rt.Verb() != kubeclient.VerbCreate && rt.Verb() != kubeclient.VerbUpdate {
 				return
 			}
 
diff --git a/internal/groupsuffix/groupsuffix_test.go b/internal/groupsuffix/groupsuffix_test.go
index 21ff239f..3afcd82e 100644
--- a/internal/groupsuffix/groupsuffix_test.go
+++ b/internal/groupsuffix/groupsuffix_test.go
@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package groupsuffix
@@ -302,7 +302,7 @@ func TestMiddlware(t *testing.T) {
 			wantResponseObj:     podWithPinnipedOwner,
 		},
 		{
-			// test that multiple of our middleware request mutations play nicely with each other
+			// test that multiple of our middleware request mutations play nicely with each other on create
 			name:           "create resource with pinniped.dev and with owner ref that has pinniped.dev owner",
 			apiGroupSuffix: newSuffix,
 			rt: (&testutil.RoundTrip{}).
@@ -317,25 +317,57 @@ func TestMiddlware(t *testing.T) {
 			wantResponseObj:     federationDomainWithNewGroupAndPinnipedOwner, // the middleware will reset object GVK for us
 		},
 		{
-			name:           "update resource without pinniped.dev",
+			// test that multiple of our middleware request mutations play nicely with each other on update
+			name:           "update resource with pinniped.dev and with owner ref that has pinniped.dev owner",
+			apiGroupSuffix: newSuffix,
+			rt: (&testutil.RoundTrip{}).
+				WithVerb(kubeclient.VerbUpdate).
+				WithNamespace("some-namespace").
+				WithResource(configv1alpha1.SchemeGroupVersion.WithResource("federationdomains")),
+			requestObj:          federationDomainWithPinnipedOwner,
+			responseObj:         federationDomainWithNewGroupAndPinnipedOwnerWithNewGroup,
+			wantMutateRequests:  2,
+			wantMutateResponses: 1,
+			wantRequestObj:      federationDomainWithNewGroupAndPinnipedOwnerWithNewGroup,
+			wantResponseObj:     federationDomainWithNewGroupAndPinnipedOwner, // the middleware will reset object GVK for us
+		},
+		{
+			name:           "update resource without pinniped.dev and without owner ref",
 			apiGroupSuffix: newSuffix,
 			rt: (&testutil.RoundTrip{}).
 				WithVerb(kubeclient.VerbUpdate).
 				WithNamespace("some-namespace").
 				WithResource(corev1.SchemeGroupVersion.WithResource("pods")),
+			requestObj:          podWithoutOwner,
 			responseObj:         podWithoutOwner,
+			wantMutateRequests:  1,
 			wantMutateResponses: 1,
+			wantRequestObj:      podWithoutOwner,
 			wantResponseObj:     podWithoutOwner,
 		},
 		{
-			name:           "update resource with pinniped.dev",
+			name:           "update resource without pinniped.dev and with owner ref that has pinniped.dev owner",
+			apiGroupSuffix: newSuffix,
+			rt: (&testutil.RoundTrip{}).
+				WithVerb(kubeclient.VerbUpdate).
+				WithNamespace("some-namespace").
+				WithResource(corev1.SchemeGroupVersion.WithResource("pods")),
+			requestObj:          podWithPinnipedOwner,
+			responseObj:         podWithPinnipedOwnerWithNewGroup,
+			wantMutateRequests:  1,
+			wantMutateResponses: 1,
+			wantRequestObj:      podWithPinnipedOwnerWithNewGroup,
+			wantResponseObj:     podWithPinnipedOwner,
+		},
+		{
+			name:           "update resource with pinniped.dev and without owner ref",
 			apiGroupSuffix: newSuffix,
 			rt: (&testutil.RoundTrip{}).
 				WithVerb(kubeclient.VerbUpdate).
 				WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")),
 			requestObj:          tokenCredentialRequest,
 			responseObj:         tokenCredentialRequestWithNewGroup,
-			wantMutateRequests:  1,
+			wantMutateRequests:  2,
 			wantMutateResponses: 1,
 			wantRequestObj:      tokenCredentialRequestWithNewGroup,
 			wantResponseObj:     tokenCredentialRequestWithNewGroup, // the middleware will reset object GVK for us