Allow multiple Pinnipeds to work on same cluster

Yes, this is a huge commit.

The middleware allows you to customize the API groups of all of the
*.pinniped.dev API groups.

Some notes about other small things in this commit:
- We removed the internal/client package in favor of pkg/conciergeclient. The
  two packages do basically the same thing. I don't think we use the former
  anymore.
- We re-enabled cluster-scoped owner assertions in the integration tests.
  This code was added in internal/ownerref. See a0546942 for when this
  assertion was removed.
- Note: the middlware code is in charge of restoring the GV of a request object,
  so we should never need to write mutations that do that.
- We updated the supervisor secret generation to no longer manually set an owner
  reference to the deployment since the middleware code now does this. I think we
  still need some way to make an initial event for the secret generator
  controller, which involves knowing the namespace and the name of the generated
  secret, so I still wired the deployment through. We could use a namespace/name
  tuple here, but I was lazy.

Signed-off-by: Andrew Keesler <akeesler@vmware.com>
Co-authored-by: Ryan Richard <richardry@vmware.com>

internal/ownerref/ownerref.go

@@ -4,36 +4,68 @@
 package ownerref
 
 import (
-	"net/http"
+	"context"
 
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"go.pinniped.dev/internal/kubeclient"
 )
 
-func New(ref metav1.OwnerReference) kubeclient.Middleware {
-	return ownerRefMiddleware(ref)
-}
+func New(refObj kubeclient.Object) kubeclient.Middleware {
+	ref := metav1.OwnerReference{
+		Name: refObj.GetName(),
+		UID:  refObj.GetUID(),
+	}
+	ref.APIVersion, ref.Kind = refObj.GetObjectKind().GroupVersionKind().ToAPIVersionAndKind()
+	refNamespace := refObj.GetNamespace()
 
-var _ kubeclient.Middleware = ownerRefMiddleware(metav1.OwnerReference{})
+	// if refNamespace is empty, we assume the owner ref is to a cluster scoped object which can own any object
+	refIsNamespaced := len(refNamespace) != 0
 
-type ownerRefMiddleware metav1.OwnerReference
-
-func (o ownerRefMiddleware) Handles(httpMethod string) bool {
-	return httpMethod == http.MethodPost // only handle create requests
-}
-
-// TODO this func assumes all objects are namespace scoped and are in the same namespace.
-//  i.e. it assumes all objects are safe to set an owner ref on
-//  i.e. the owner could be namespace scoped and thus cannot own a cluster scoped object
-//  this could be fixed by using a rest mapper to confirm the REST scoping
-//  or we could always use an owner ref to a cluster scoped object
-func (o ownerRefMiddleware) Mutate(obj metav1.Object) (mutated bool) {
-	// we only want to set the owner ref on create and when one is not already present
-	if len(obj.GetOwnerReferences()) != 0 {
-		return false
+	// special handling of namespaces to treat them as namespace scoped to themselves
+	if isNamespace(refObj) {
+		refNamespace = refObj.GetName()
+		refIsNamespaced = true
 	}
 
-	obj.SetOwnerReferences([]metav1.OwnerReference{metav1.OwnerReference(o)})
-	return true
+	return 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 {
+			return
+		}
+
+		// we probably do not want to set an owner ref on a subresource
+		if len(rt.Subresource()) != 0 {
+			return
+		}
+
+		// when ref is not cluster scoped, we ignore cluster scoped resources
+		if refIsNamespaced && !rt.NamespaceScoped() {
+			return
+		}
+
+		// when ref is not cluster scoped, we require refNamespace to match
+		// the request namespace since cross namespace ownership is disallowed
+		if refIsNamespaced && refNamespace != rt.Namespace() {
+			return
+		}
+
+		rt.MutateRequest(func(obj kubeclient.Object) {
+			// we only want to set the owner ref on create and when one is not already present
+			if len(obj.GetOwnerReferences()) != 0 {
+				return
+			}
+
+			obj.SetOwnerReferences([]metav1.OwnerReference{ref})
+		})
+	})
+}
+
+//nolint: gochecknoglobals
+var namespaceGVK = corev1.SchemeGroupVersion.WithKind("Namespace")
+
+func isNamespace(obj kubeclient.Object) bool {
+	_, ok := obj.(*corev1.Namespace)
+	return ok || obj.GetObjectKind().GroupVersionKind() == namespaceGVK
 }

internal/ownerref/ownerref_test.go

@@ -4,46 +4,47 @@
 package ownerref
 
 import (
+	"context"
 	"net/http"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/meta"
+	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
+
+	"go.pinniped.dev/internal/kubeclient"
+	"go.pinniped.dev/internal/testutil"
 )
 
 func TestOwnerReferenceMiddleware(t *testing.T) {
-	ref1 := metav1.OwnerReference{
-		Name: "earth",
-		UID:  "0x11",
-	}
-	ref2 := metav1.OwnerReference{
-		Name: "mars",
-		UID:  "0x12",
-	}
-	ref3 := metav1.OwnerReference{
-		Name: "sun",
-		UID:  "0x13",
-	}
+	ref1 := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "earth", Namespace: "some-namespace", UID: "0x11"}}
+	ref2 := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "mars", Namespace: "some-namespace", UID: "0x12"}}
+	ref3 := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "sun", Namespace: "some-namespace", UID: "0x13"}}
+	clusterRef := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Name: "bananas", UID: "0x13"}}
 
-	secret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "twizzlers"}}
-	configMap := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "pandas"}}
+	secret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "twizzlers", Namespace: "some-namespace"}}
+	secretOtherNamespace := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "twizzlers", Namespace: "some-other-namespace"}}
+	configMap := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "pandas", Namespace: "some-namespace"}}
+	clusterRole := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Name: "bananas"}}
 
-	secretWithOwner := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "twizzlers", OwnerReferences: []metav1.OwnerReference{ref3}}}
-	configMapWithOwner := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "pandas", OwnerReferences: []metav1.OwnerReference{ref3}}}
+	secretWithOwner := withOwnerRef(t, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "twizzlers", Namespace: "some-namespace"}}, ref3)
+	configMapWithOwner := withOwnerRef(t, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "pandas", Namespace: "some-namespace"}}, ref3)
+
+	namespaceRef := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "solar-system", UID: "0x42"}}
+	secretInSameNamespaceAsNamespaceRef := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "venus", Namespace: "solar-system", UID: "0x11"}}
 
 	type args struct {
-		ref        metav1.OwnerReference
-		httpMethod string
-		obj        metav1.Object
+		ref         kubeclient.Object
+		httpMethod  string
+		subresource string
+		obj         kubeclient.Object
 	}
 	tests := []struct {
 		name                     string
 		args                     args
 		wantHandles, wantMutates bool
-		wantObj                  metav1.Object
+		wantObj                  kubeclient.Object
 	}{
 		{
 			name: "on update",
@@ -54,7 +55,36 @@ func TestOwnerReferenceMiddleware(t *testing.T) {
 			},
 			wantHandles: false,
 			wantMutates: false,
-			wantObj:     nil,
+		},
+		{
+			name: "on get",
+			args: args{
+				ref:        ref1,
+				httpMethod: http.MethodGet,
+				obj:        secret.DeepCopy(),
+			},
+			wantHandles: false,
+			wantMutates: false,
+		},
+		{
+			name: "on delete",
+			args: args{
+				ref:        ref1,
+				httpMethod: http.MethodDelete,
+				obj:        secret.DeepCopy(),
+			},
+			wantHandles: false,
+			wantMutates: false,
+		},
+		{
+			name: "on patch",
+			args: args{
+				ref:        ref1,
+				httpMethod: http.MethodPatch,
+				obj:        secret.DeepCopy(),
+			},
+			wantHandles: false,
+			wantMutates: false,
 		},
 		{
 			name: "on create",
@@ -67,6 +97,17 @@ func TestOwnerReferenceMiddleware(t *testing.T) {
 			wantMutates: true,
 			wantObj:     withOwnerRef(t, secret, ref1),
 		},
+		{
+			name: "on create when the ref object is a namespace",
+			args: args{
+				ref:        namespaceRef,
+				httpMethod: http.MethodPost,
+				obj:        secretInSameNamespaceAsNamespaceRef.DeepCopy(),
+			},
+			wantHandles: true,
+			wantMutates: true,
+			wantObj:     withOwnerRef(t, secretInSameNamespaceAsNamespaceRef, namespaceRef),
+		},
 		{
 			name: "on create config map",
 			args: args{
@@ -78,27 +119,78 @@ func TestOwnerReferenceMiddleware(t *testing.T) {
 			wantMutates: true,
 			wantObj:     withOwnerRef(t, configMap, ref2),
 		},
+		{
+			name: "on create with cluster-scoped owner",
+			args: args{
+				ref:        clusterRef,
+				httpMethod: http.MethodPost,
+				obj:        secret.DeepCopy(),
+			},
+			wantHandles: true,
+			wantMutates: true,
+			wantObj:     withOwnerRef(t, secret, clusterRef),
+		},
+		{
+			name: "on create of cluster-scoped resource with namespace-scoped owner",
+			args: args{
+				ref:        ref1,
+				httpMethod: http.MethodPost,
+				obj:        clusterRole.DeepCopy(),
+			},
+			wantHandles: false,
+			wantMutates: false,
+		},
+		{
+			name: "on create of cluster-scoped resource with cluster-scoped owner",
+			args: args{
+				ref:        clusterRef,
+				httpMethod: http.MethodPost,
+				obj:        clusterRole.DeepCopy(),
+			},
+			wantHandles: true,
+			wantMutates: true,
+			wantObj:     withOwnerRef(t, clusterRole, clusterRef),
+		},
 		{
 			name: "on create with pre-existing ref",
 			args: args{
 				ref:        ref1,
 				httpMethod: http.MethodPost,
-				obj:        secretWithOwner.DeepCopy(),
+				obj:        secretWithOwner.DeepCopyObject().(kubeclient.Object),
 			},
 			wantHandles: true,
 			wantMutates: false,
-			wantObj:     nil,
 		},
 		{
 			name: "on create with pre-existing ref config map",
 			args: args{
 				ref:        ref2,
 				httpMethod: http.MethodPost,
-				obj:        configMapWithOwner.DeepCopy(),
+				obj:        configMapWithOwner.DeepCopyObject().(kubeclient.Object),
 			},
 			wantHandles: true,
 			wantMutates: false,
-			wantObj:     nil,
+		},
+		{
+			name: "on create of subresource",
+			args: args{
+				ref:         ref2,
+				httpMethod:  http.MethodPost,
+				subresource: "some-subresource",
+				obj:         configMapWithOwner.DeepCopyObject().(kubeclient.Object),
+			},
+			wantHandles: false,
+			wantMutates: false,
+		},
+		{
+			name: "on create with namespace mismatch",
+			args: args{
+				ref:        ref2,
+				httpMethod: http.MethodPost,
+				obj:        secretOtherNamespace.DeepCopyObject().(kubeclient.Object),
+			},
+			wantHandles: false,
+			wantMutates: false,
 		},
 	}
 	for _, tt := range tests {
@@ -106,37 +198,63 @@ func TestOwnerReferenceMiddleware(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			middleware := New(tt.args.ref)
 
-			handles := middleware.Handles(tt.args.httpMethod)
-			require.Equal(t, tt.wantHandles, handles)
-
-			if !handles {
+			rt := (&testutil.RoundTrip{}).
+				WithVerb(verb(t, tt.args.httpMethod)).
+				WithNamespace(tt.args.obj.GetNamespace()).
+				WithSubresource(tt.args.subresource)
+			middleware.Handle(context.Background(), rt)
+			require.Empty(t, rt.MutateResponses, 1)
+			if !tt.wantHandles {
+				require.Empty(t, rt.MutateRequests)
 				return
 			}
+			require.Len(t, rt.MutateRequests, 1)
 
-			orig := tt.args.obj.(runtime.Object).DeepCopyObject()
-
-			mutates := middleware.Mutate(tt.args.obj)
-			require.Equal(t, tt.wantMutates, mutates)
-
-			if mutates {
+			orig := tt.args.obj.DeepCopyObject().(kubeclient.Object)
+			for _, mutateRequest := range rt.MutateRequests {
+				mutateRequest := mutateRequest
+				mutateRequest(tt.args.obj)
+			}
+			if !tt.wantMutates {
+				require.Equal(t, orig, tt.args.obj)
+			} else {
 				require.NotEqual(t, orig, tt.args.obj)
 				require.Equal(t, tt.wantObj, tt.args.obj)
-			} else {
-				require.Equal(t, orig, tt.args.obj)
 			}
 		})
 	}
 }
 
-func withOwnerRef(t *testing.T, obj runtime.Object, ref metav1.OwnerReference) metav1.Object {
+func withOwnerRef(t *testing.T, obj kubeclient.Object, ref kubeclient.Object) kubeclient.Object {
 	t.Helper()
 
-	obj = obj.DeepCopyObject()
-	accessor, err := meta.Accessor(obj)
-	require.NoError(t, err)
+	ownerRef := metav1.OwnerReference{
+		Name: ref.GetName(),
+		UID:  ref.GetUID(),
+	}
 
-	require.Len(t, accessor.GetOwnerReferences(), 0)
-	accessor.SetOwnerReferences([]metav1.OwnerReference{ref})
+	obj = obj.DeepCopyObject().(kubeclient.Object)
+	require.Len(t, obj.GetOwnerReferences(), 0)
+	obj.SetOwnerReferences([]metav1.OwnerReference{ownerRef})
 
-	return accessor
+	return obj
+}
+
+func verb(t *testing.T, v string) kubeclient.Verb {
+	t.Helper()
+	switch v {
+	case http.MethodGet:
+		return kubeclient.VerbGet
+	case http.MethodPut:
+		return kubeclient.VerbUpdate
+	case http.MethodPost:
+		return kubeclient.VerbCreate
+	case http.MethodDelete:
+		return kubeclient.VerbDelete
+	case http.MethodPatch:
+		return kubeclient.VerbPatch
+	default:
+		require.FailNowf(t, "unknown verb", "unknown verb: %q", v)
+		return kubeclient.VerbGet // shouldn't get here
+	}
 }