Merge pull request #383 from enj/enj/i/avoid_scheme_double_register

Avoid double registering types in server scheme

internal/concierge/server/server.go

@@ -179,60 +179,9 @@ func getAggregatedAPIServerConfig(
 		return nil, fmt.Errorf("cannot make api group from %s/%s", loginv1alpha1.GroupName, apiGroupSuffix)
 	}
 
-	// standard set up of the server side scheme
+	scheme := getAggregatedAPIServerScheme(apiGroup)
-	scheme := runtime.NewScheme()
 	codecs := serializer.NewCodecFactory(scheme)
 
-	utilruntime.Must(loginv1alpha1.AddToScheme(scheme))
-	utilruntime.Must(loginapi.AddToScheme(scheme))
-
-	// add the options to empty v1
-	metav1.AddToGroupVersion(scheme, schema.GroupVersion{Version: "v1"})
-
-	unversioned := schema.GroupVersion{Group: "", Version: "v1"}
-	scheme.AddUnversionedTypes(unversioned,
-		&metav1.Status{},
-		&metav1.APIVersions{},
-		&metav1.APIGroupList{},
-		&metav1.APIGroup{},
-		&metav1.APIResourceList{},
-	)
-
-	// use closure to avoid mutating scheme during iteration
-	var addPinnipedTypeToAPIGroup []func() //nolint: prealloc  // expected slice size is unknown
-	for gvk := range scheme.AllKnownTypes() {
-		gvk := gvk
-
-		if apiGroup == loginv1alpha1.GroupName {
-			break // bail out early if using the standard group
-		}
-
-		if gvk.Group != loginv1alpha1.GroupName {
-			continue // ignore types that are not in the aggregated API group
-		}
-
-		// re-register the existing type but with the new group
-		f := func() {
-			obj, err := scheme.New(gvk)
-			if err != nil {
-				panic(err) // programmer error, scheme internal code is broken
-			}
-			newGVK := schema.GroupVersionKind{
-				Group:   apiGroup,
-				Version: gvk.Version,
-				Kind:    gvk.Kind,
-			}
-			scheme.AddKnownTypeWithName(newGVK, obj)
-		}
-
-		addPinnipedTypeToAPIGroup = append(addPinnipedTypeToAPIGroup, f)
-	}
-
-	// run the closures to mutate the scheme to understand the types at the new group
-	for _, f := range addPinnipedTypeToAPIGroup {
-		f()
-	}
-
 	defaultEtcdPathPrefix := fmt.Sprintf("/registry/%s", apiGroup)
 	groupVersion := schema.GroupVersion{
 		Group:   apiGroup,
@@ -274,3 +223,52 @@ func getAggregatedAPIServerConfig(
 	}
 	return apiServerConfig, nil
 }
+
+func getAggregatedAPIServerScheme(apiGroup string) *runtime.Scheme {
+	// standard set up of the server side scheme
+	scheme := runtime.NewScheme()
+
+	// add the options to empty v1
+	metav1.AddToGroupVersion(scheme, metav1.Unversioned)
+
+	// nothing fancy is required if using the standard group
+	if apiGroup == loginv1alpha1.GroupName {
+		utilruntime.Must(loginv1alpha1.AddToScheme(scheme))
+		utilruntime.Must(loginapi.AddToScheme(scheme))
+		return scheme
+	}
+
+	// we need a temporary place to register our types to avoid double registering them
+	tmpScheme := runtime.NewScheme()
+	utilruntime.Must(loginv1alpha1.AddToScheme(tmpScheme))
+	utilruntime.Must(loginapi.AddToScheme(tmpScheme))
+
+	for gvk := range tmpScheme.AllKnownTypes() {
+		if gvk.GroupVersion() == metav1.Unversioned {
+			continue // metav1.AddToGroupVersion registers types outside of our aggregated API group that we need to ignore
+		}
+
+		if gvk.Group != loginv1alpha1.GroupName {
+			panic("tmp scheme has types not in the aggregated API group: " + gvk.Group) // programmer error
+		}
+
+		obj, err := tmpScheme.New(gvk)
+		if err != nil {
+			panic(err) // programmer error, scheme internal code is broken
+		}
+		newGVK := schema.GroupVersionKind{
+			Group:   apiGroup,
+			Version: gvk.Version,
+			Kind:    gvk.Kind,
+		}
+
+		// register the existing type but with the new group in the correct scheme
+		scheme.AddKnownTypeWithName(newGVK, obj)
+	}
+
+	// manually register conversions and defaulting into the correct scheme since we cannot directly call loginv1alpha1.AddToScheme
+	utilruntime.Must(loginv1alpha1.RegisterConversions(scheme))
+	utilruntime.Must(loginv1alpha1.RegisterDefaults(scheme))
+
+	return scheme
+}

internal/concierge/server/server_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package server
@@ -6,12 +6,19 @@ package server
 import (
 	"bytes"
 	"context"
+	"reflect"
 	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+
+	loginapi "go.pinniped.dev/generated/1.20/apis/concierge/login"
+	loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1"
 )
 
 const knownGoodUsage = `
@@ -88,3 +95,129 @@ func TestCommand(t *testing.T) {
 		})
 	}
 }
+
+func Test_getAggregatedAPIServerScheme(t *testing.T) {
+	// the standard group
+	regularGV := schema.GroupVersion{
+		Group:   "login.concierge.pinniped.dev",
+		Version: "v1alpha1",
+	}
+	regularGVInternal := schema.GroupVersion{
+		Group:   "login.concierge.pinniped.dev",
+		Version: runtime.APIVersionInternal,
+	}
+
+	// the canonical other group
+	otherGV := schema.GroupVersion{
+		Group:   "login.concierge.walrus.tld",
+		Version: "v1alpha1",
+	}
+	otherGVInternal := schema.GroupVersion{
+		Group:   "login.concierge.walrus.tld",
+		Version: runtime.APIVersionInternal,
+	}
+
+	// kube's core internal
+	internalGV := schema.GroupVersion{
+		Group:   "",
+		Version: runtime.APIVersionInternal,
+	}
+
+	tests := []struct {
+		name     string
+		apiGroup string
+		want     map[schema.GroupVersionKind]reflect.Type
+	}{
+		{
+			name:     "regular api group",
+			apiGroup: "login.concierge.pinniped.dev",
+			want: map[schema.GroupVersionKind]reflect.Type{
+				// all the types that are in the aggregated API group
+
+				regularGV.WithKind("TokenCredentialRequest"):     reflect.TypeOf(&loginv1alpha1.TokenCredentialRequest{}).Elem(),
+				regularGV.WithKind("TokenCredentialRequestList"): reflect.TypeOf(&loginv1alpha1.TokenCredentialRequestList{}).Elem(),
+
+				regularGVInternal.WithKind("TokenCredentialRequest"):     reflect.TypeOf(&loginapi.TokenCredentialRequest{}).Elem(),
+				regularGVInternal.WithKind("TokenCredentialRequestList"): reflect.TypeOf(&loginapi.TokenCredentialRequestList{}).Elem(),
+
+				regularGV.WithKind("CreateOptions"): reflect.TypeOf(&metav1.CreateOptions{}).Elem(),
+				regularGV.WithKind("DeleteOptions"): reflect.TypeOf(&metav1.DeleteOptions{}).Elem(),
+				regularGV.WithKind("ExportOptions"): reflect.TypeOf(&metav1.ExportOptions{}).Elem(),
+				regularGV.WithKind("GetOptions"):    reflect.TypeOf(&metav1.GetOptions{}).Elem(),
+				regularGV.WithKind("ListOptions"):   reflect.TypeOf(&metav1.ListOptions{}).Elem(),
+				regularGV.WithKind("PatchOptions"):  reflect.TypeOf(&metav1.PatchOptions{}).Elem(),
+				regularGV.WithKind("UpdateOptions"): reflect.TypeOf(&metav1.UpdateOptions{}).Elem(),
+				regularGV.WithKind("WatchEvent"):    reflect.TypeOf(&metav1.WatchEvent{}).Elem(),
+
+				regularGVInternal.WithKind("WatchEvent"): reflect.TypeOf(&metav1.InternalEvent{}).Elem(),
+
+				// the types below this line do not really matter to us because they are in the core group
+
+				internalGV.WithKind("WatchEvent"): reflect.TypeOf(&metav1.InternalEvent{}).Elem(),
+
+				metav1.Unversioned.WithKind("APIGroup"):        reflect.TypeOf(&metav1.APIGroup{}).Elem(),
+				metav1.Unversioned.WithKind("APIGroupList"):    reflect.TypeOf(&metav1.APIGroupList{}).Elem(),
+				metav1.Unversioned.WithKind("APIResourceList"): reflect.TypeOf(&metav1.APIResourceList{}).Elem(),
+				metav1.Unversioned.WithKind("APIVersions"):     reflect.TypeOf(&metav1.APIVersions{}).Elem(),
+				metav1.Unversioned.WithKind("CreateOptions"):   reflect.TypeOf(&metav1.CreateOptions{}).Elem(),
+				metav1.Unversioned.WithKind("DeleteOptions"):   reflect.TypeOf(&metav1.DeleteOptions{}).Elem(),
+				metav1.Unversioned.WithKind("ExportOptions"):   reflect.TypeOf(&metav1.ExportOptions{}).Elem(),
+				metav1.Unversioned.WithKind("GetOptions"):      reflect.TypeOf(&metav1.GetOptions{}).Elem(),
+				metav1.Unversioned.WithKind("ListOptions"):     reflect.TypeOf(&metav1.ListOptions{}).Elem(),
+				metav1.Unversioned.WithKind("PatchOptions"):    reflect.TypeOf(&metav1.PatchOptions{}).Elem(),
+				metav1.Unversioned.WithKind("Status"):          reflect.TypeOf(&metav1.Status{}).Elem(),
+				metav1.Unversioned.WithKind("UpdateOptions"):   reflect.TypeOf(&metav1.UpdateOptions{}).Elem(),
+				metav1.Unversioned.WithKind("WatchEvent"):      reflect.TypeOf(&metav1.WatchEvent{}).Elem(),
+			},
+		},
+		{
+			name:     "other api group",
+			apiGroup: "login.concierge.walrus.tld",
+			want: map[schema.GroupVersionKind]reflect.Type{
+				// all the types that are in the aggregated API group
+
+				otherGV.WithKind("TokenCredentialRequest"):     reflect.TypeOf(&loginv1alpha1.TokenCredentialRequest{}).Elem(),
+				otherGV.WithKind("TokenCredentialRequestList"): reflect.TypeOf(&loginv1alpha1.TokenCredentialRequestList{}).Elem(),
+
+				otherGVInternal.WithKind("TokenCredentialRequest"):     reflect.TypeOf(&loginapi.TokenCredentialRequest{}).Elem(),
+				otherGVInternal.WithKind("TokenCredentialRequestList"): reflect.TypeOf(&loginapi.TokenCredentialRequestList{}).Elem(),
+
+				otherGV.WithKind("CreateOptions"): reflect.TypeOf(&metav1.CreateOptions{}).Elem(),
+				otherGV.WithKind("DeleteOptions"): reflect.TypeOf(&metav1.DeleteOptions{}).Elem(),
+				otherGV.WithKind("ExportOptions"): reflect.TypeOf(&metav1.ExportOptions{}).Elem(),
+				otherGV.WithKind("GetOptions"):    reflect.TypeOf(&metav1.GetOptions{}).Elem(),
+				otherGV.WithKind("ListOptions"):   reflect.TypeOf(&metav1.ListOptions{}).Elem(),
+				otherGV.WithKind("PatchOptions"):  reflect.TypeOf(&metav1.PatchOptions{}).Elem(),
+				otherGV.WithKind("UpdateOptions"): reflect.TypeOf(&metav1.UpdateOptions{}).Elem(),
+				otherGV.WithKind("WatchEvent"):    reflect.TypeOf(&metav1.WatchEvent{}).Elem(),
+
+				otherGVInternal.WithKind("WatchEvent"): reflect.TypeOf(&metav1.InternalEvent{}).Elem(),
+
+				// the types below this line do not really matter to us because they are in the core group
+
+				internalGV.WithKind("WatchEvent"): reflect.TypeOf(&metav1.InternalEvent{}).Elem(),
+
+				metav1.Unversioned.WithKind("APIGroup"):        reflect.TypeOf(&metav1.APIGroup{}).Elem(),
+				metav1.Unversioned.WithKind("APIGroupList"):    reflect.TypeOf(&metav1.APIGroupList{}).Elem(),
+				metav1.Unversioned.WithKind("APIResourceList"): reflect.TypeOf(&metav1.APIResourceList{}).Elem(),
+				metav1.Unversioned.WithKind("APIVersions"):     reflect.TypeOf(&metav1.APIVersions{}).Elem(),
+				metav1.Unversioned.WithKind("CreateOptions"):   reflect.TypeOf(&metav1.CreateOptions{}).Elem(),
+				metav1.Unversioned.WithKind("DeleteOptions"):   reflect.TypeOf(&metav1.DeleteOptions{}).Elem(),
+				metav1.Unversioned.WithKind("ExportOptions"):   reflect.TypeOf(&metav1.ExportOptions{}).Elem(),
+				metav1.Unversioned.WithKind("GetOptions"):      reflect.TypeOf(&metav1.GetOptions{}).Elem(),
+				metav1.Unversioned.WithKind("ListOptions"):     reflect.TypeOf(&metav1.ListOptions{}).Elem(),
+				metav1.Unversioned.WithKind("PatchOptions"):    reflect.TypeOf(&metav1.PatchOptions{}).Elem(),
+				metav1.Unversioned.WithKind("Status"):          reflect.TypeOf(&metav1.Status{}).Elem(),
+				metav1.Unversioned.WithKind("UpdateOptions"):   reflect.TypeOf(&metav1.UpdateOptions{}).Elem(),
+				metav1.Unversioned.WithKind("WatchEvent"):      reflect.TypeOf(&metav1.WatchEvent{}).Elem(),
+			},
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			scheme := getAggregatedAPIServerScheme(tt.apiGroup)
+			require.Equal(t, tt.want, scheme.AllKnownTypes())
+		})
+	}
+}