Merge pull request #41788 from sttts/sttts-scheme-registration-idem-potent

Automatic merge from submit-queue (batch tested with PRs 41234, 42186, 41615, 42028, 41788)

apimachinery: handle duplicated and conflicting type registration

Double registrations were leading to duplications in  `KnownKinds()`. Conflicting registrations with same gvk, but different types were not detected.

federation/pkg/federation-controller/util/eventsink/BUILD

@@ -18,6 +18,7 @@ go_library(
         "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1",
         "//vendor:k8s.io/apimachinery/pkg/conversion",
         "//vendor:k8s.io/apimachinery/pkg/runtime",
+        "//vendor:k8s.io/apimachinery/pkg/runtime/schema",
         "//vendor:k8s.io/apimachinery/pkg/types",
         "//vendor:k8s.io/client-go/pkg/api/v1",
         "//vendor:k8s.io/client-go/tools/record",

federation/pkg/federation-controller/util/eventsink/eventsink.go

@@ -22,6 +22,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/conversion"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	clientv1 "k8s.io/client-go/pkg/api/v1"
 	"k8s.io/client-go/tools/record"
@@ -48,10 +49,11 @@ func NewFederatedEventSink(clientset fedclientset.Interface) *FederatedEventSink
 var scheme = runtime.NewScheme()
 
 func init() {
-	scheme.AddKnownTypes(clientv1.SchemeGroupVersion,
+	// register client-go's and kube's Event type under two different GroupVersions
-		&clientv1.Event{},
+	// TODO: switch to client-go client for events
-		&kubev1.Event{},
+	scheme.AddKnownTypes(clientv1.SchemeGroupVersion, &clientv1.Event{})
-	)
+	scheme.AddKnownTypes(schema.GroupVersion{Group: "fake-kube-" + kubev1.SchemeGroupVersion.Group, Version: kubev1.SchemeGroupVersion.Version}, &kubev1.Event{})
+
 	if err := scheme.AddConversionFuncs(
 		metav1.Convert_unversioned_Time_To_unversioned_Time,
 	); err != nil {

pkg/api/serialization_test.go

@@ -174,7 +174,8 @@ func TestCommonKindsRegistered(t *testing.T) {
 				t.Error(err)
 			}
 			defaults := gv.WithKind("")
-			if _, got, err := api.Codecs.LegacyCodec().Decode([]byte(`{"kind":"`+kind+`"}`), &defaults, nil); err != nil || gvk != *got {
+			var got *schema.GroupVersionKind
+			if obj, got, err = api.Codecs.LegacyCodec().Decode([]byte(`{"kind":"`+kind+`"}`), &defaults, obj); err != nil || gvk != *got {
 				t.Errorf("expected %v: %v %v", gvk, got, err)
 			}
 			data, err := runtime.Encode(api.Codecs.LegacyCodec(*gv), obj)

pkg/apis/autoscaling/v2alpha1/register.go

@@ -20,7 +20,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/kubernetes/pkg/api/v1"
 )
 
 // GroupName is the group name use in this package
@@ -39,10 +38,6 @@ func addKnownTypes(scheme *runtime.Scheme) error {
 	scheme.AddKnownTypes(SchemeGroupVersion,
 		&HorizontalPodAutoscaler{},
 		&HorizontalPodAutoscalerList{},
-		&v1.ListOptions{},
-		&v1.DeleteOptions{},
-		&metav1.GetOptions{},
-		&metav1.ExportOptions{},
 	)
 	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
 	return nil

pkg/apis/rbac/register.go

@@ -17,7 +17,6 @@ limitations under the License.
 package rbac
 
 import (
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
@@ -55,6 +54,5 @@ func addKnownTypes(scheme *runtime.Scheme) error {
 		&ClusterRoleBindingList{},
 		&ClusterRoleList{},
 	)
-	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
 	return nil
 }

pkg/controller/garbagecollector/metaonly/metaonly.go

@@ -52,6 +52,10 @@ func NewMetadataCodecFactory() serializer.CodecFactory {
 		if kind.Version == runtime.APIVersionInternal {
 			continue
 		}
+		if kind == api.Unversioned.WithKind("Status") {
+			// this is added below as unversioned
+			continue
+		}
 		metaOnlyObject := gvkToMetadataOnlyObject(kind)
 		scheme.AddKnownTypeWithName(kind, metaOnlyObject)
 	}

staging/src/k8s.io/apimachinery/pkg/runtime/scheme.go

@@ -163,22 +163,13 @@ func (s *Scheme) AddUnversionedTypes(version schema.GroupVersion, types ...Objec
 // the struct becomes the "kind" field when encoding. Version may not be empty - use the
 // APIVersionInternal constant if you have a type that does not have a formal version.
 func (s *Scheme) AddKnownTypes(gv schema.GroupVersion, types ...Object) {
-	if len(gv.Version) == 0 {
-		panic(fmt.Sprintf("version is required on all types: %s %v", gv, types[0]))
-	}
 	for _, obj := range types {
 		t := reflect.TypeOf(obj)
 		if t.Kind() != reflect.Ptr {
 			panic("All types must be pointers to structs.")
 		}
 		t = t.Elem()
-		if t.Kind() != reflect.Struct {
+		s.AddKnownTypeWithName(gv.WithKind(t.Name()), obj)
-			panic("All types must be pointers to structs.")
-		}
-
-		gvk := gv.WithKind(t.Name())
-		s.gvkToType[gvk] = t
-		s.typeToGVK[t] = append(s.typeToGVK[t], gvk)
 	}
 }
 
@@ -199,7 +190,17 @@ func (s *Scheme) AddKnownTypeWithName(gvk schema.GroupVersionKind, obj Object) {
 		panic("All types must be pointers to structs.")
 	}
 
+	if oldT, found := s.gvkToType[gvk]; found && oldT != t {
+		panic(fmt.Sprintf("Double registration of different types for %v: old=%v.%v, new=%v.%v", gvk, oldT.PkgPath(), oldT.Name(), t.PkgPath(), t.Name()))
+	}
+
 	s.gvkToType[gvk] = t
+
+	for _, existingGvk := range s.typeToGVK[t] {
+		if existingGvk == gvk {
+			return
+		}
+	}
 	s.typeToGVK[t] = append(s.typeToGVK[t], gvk)
 }
 

staging/src/k8s.io/apimachinery/pkg/runtime/scheme_test.go

@@ -548,6 +548,79 @@ func TestKnownTypes(t *testing.T) {
 	}
 }
 
+func TestAddKnownTypesIdemPotent(t *testing.T) {
+	s := runtime.NewScheme()
+
+	gv := schema.GroupVersion{Group: "foo", Version: "v1"}
+	s.AddKnownTypes(gv, &InternalSimple{})
+	s.AddKnownTypes(gv, &InternalSimple{})
+	if len(s.KnownTypes(gv)) != 1 {
+		t.Errorf("expected only one %v type after double registration", gv)
+	}
+	if len(s.AllKnownTypes()) != 1 {
+		t.Errorf("expected only one type after double registration")
+	}
+
+	s.AddKnownTypeWithName(gv.WithKind("InternalSimple"), &InternalSimple{})
+	s.AddKnownTypeWithName(gv.WithKind("InternalSimple"), &InternalSimple{})
+	if len(s.KnownTypes(gv)) != 1 {
+		t.Errorf("expected only one %v type after double registration with custom name", gv)
+	}
+	if len(s.AllKnownTypes()) != 1 {
+		t.Errorf("expected only one type after double registration with custom name")
+	}
+
+	s.AddUnversionedTypes(gv, &InternalSimple{})
+	if len(s.KnownTypes(gv)) != 1 {
+		t.Errorf("expected only one %v type after double registration with custom name", gv)
+	}
+	if len(s.AllKnownTypes()) != 1 {
+		t.Errorf("expected only one type after double registration with custom name")
+	}
+
+	kinds, _, err := s.ObjectKinds(&InternalSimple{})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(kinds) != 1 {
+		t.Errorf("expected only one kind for InternalSimple after double registration", gv)
+	}
+}
+
+func TestConflictingAddKnownTypes(t *testing.T) {
+	s := runtime.NewScheme()
+	gv := schema.GroupVersion{Group: "foo", Version: "v1"}
+
+	panicked := make(chan bool)
+	go func() {
+		defer func() {
+			if recover() != nil {
+				panicked <- true
+			}
+		}()
+		s.AddKnownTypeWithName(gv.WithKind("InternalSimple"), &InternalSimple{})
+		s.AddKnownTypeWithName(gv.WithKind("InternalSimple"), &ExternalSimple{})
+		panicked <- false
+	}()
+	if !<-panicked {
+		t.Errorf("Expected AddKnownTypesWithName to panic with conflicting type registrations")
+	}
+
+	go func() {
+		defer func() {
+			if recover() != nil {
+				panicked <- true
+			}
+		}()
+		s.AddUnversionedTypes(gv, &InternalSimple{})
+		s.AddUnversionedTypes(gv, &InternalSimple{})
+		panicked <- false
+	}()
+	if !<-panicked {
+		t.Errorf("Expected AddUnversionedTypes to panic with conflicting type registrations")
+	}
+}
+
 func TestConvertToVersionBasic(t *testing.T) {
 	s := GetTestScheme()
 	tt := &TestType1{A: "I'm not a pointer object"}

staging/src/k8s.io/apiserver/pkg/apis/apiserver/register.go

@@ -17,7 +17,6 @@ limitations under the License.
 package apiserver
 
 import (
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
@@ -47,6 +46,5 @@ func addKnownTypes(scheme *runtime.Scheme) error {
 	scheme.AddKnownTypes(SchemeGroupVersion,
 		&AdmissionConfiguration{},
 	)
-	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
 	return nil
 }

staging/src/k8s.io/apiserver/pkg/apis/example/register.go

@@ -17,7 +17,6 @@ limitations under the License.
 package example
 
 import (
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
@@ -48,6 +47,5 @@ func addKnownTypes(scheme *runtime.Scheme) error {
 	scheme.AddKnownTypes(SchemeGroupVersion,
 		&Pod{},
 	)
-	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
 	return nil
 }

staging/src/k8s.io/apiserver/pkg/storage/etcd/etcd_helper_test.go

@@ -77,7 +77,6 @@ func testScheme(t *testing.T) (*runtime.Scheme, serializer.CodecFactory) {
 	scheme := runtime.NewScheme()
 	scheme.Log(t)
 	scheme.AddKnownTypes(schema.GroupVersion{Version: runtime.APIVersionInternal}, &storagetesting.TestResource{})
-	scheme.AddKnownTypes(schema.GroupVersion{Version: runtime.APIVersionInternal}, &storagetesting.TestResource{})
 	example.AddToScheme(scheme)
 	examplev1.AddToScheme(scheme)
 	if err := scheme.AddConversionFuncs(

staging/src/k8s.io/client-go/pkg/apis/autoscaling/v2alpha1/register.go

@@ -20,7 +20,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/client-go/pkg/api/v1"
 )
 
 // GroupName is the group name use in this package
@@ -39,10 +38,6 @@ func addKnownTypes(scheme *runtime.Scheme) error {
 	scheme.AddKnownTypes(SchemeGroupVersion,
 		&HorizontalPodAutoscaler{},
 		&HorizontalPodAutoscalerList{},
-		&v1.ListOptions{},
-		&v1.DeleteOptions{},
-		&metav1.GetOptions{},
-		&metav1.ExportOptions{},
 	)
 	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
 	return nil

staging/src/k8s.io/client-go/pkg/apis/rbac/register.go

@@ -17,7 +17,6 @@ limitations under the License.
 package rbac
 
 import (
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
@@ -55,6 +54,5 @@ func addKnownTypes(scheme *runtime.Scheme) error {
 		&ClusterRoleBindingList{},
 		&ClusterRoleList{},
 	)
-	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
 	return nil
 }

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/register.go

@@ -17,12 +17,8 @@ limitations under the License.
 package apiregistration
 
 import (
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-
-	// we register here until we separate our scheme, which requires updates to client gen
-	kapi "k8s.io/client-go/pkg/api"
 )
 
 const GroupName = "apiregistration.k8s.io"
@@ -50,11 +46,6 @@ func addKnownTypes(scheme *runtime.Scheme) error {
 	scheme.AddKnownTypes(SchemeGroupVersion,
 		&APIService{},
 		&APIServiceList{},
-
-		&kapi.ListOptions{},
-		&metav1.DeleteOptions{},
-		&metav1.GetOptions{},
 	)
-	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
 	return nil
 }

staging/src/k8s.io/sample-apiserver/pkg/apis/wardle/register.go

@@ -17,7 +17,6 @@ limitations under the License.
 package wardle
 
 import (
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
@@ -48,6 +47,5 @@ func addKnownTypes(scheme *runtime.Scheme) error {
 		&Flunder{},
 		&FlunderList{},
 	)
-	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
 	return nil
 }

vendor/BUILD

@@ -15836,7 +15836,6 @@ go_library(
         "//vendor:k8s.io/apimachinery/pkg/conversion",
         "//vendor:k8s.io/apimachinery/pkg/runtime",
         "//vendor:k8s.io/apimachinery/pkg/runtime/schema",
-        "//vendor:k8s.io/client-go/pkg/api",
     ],
 )