Merge pull request #43553 from liggitt/discovery-order

Automatic merge from submit-queue

Preserve API group order in discovery, prefer extensions over apps

Fixes #42392, supercedes #43543

Kubectl 1.5 still uses compiled in types in kubectl edit and apply.

Because kubectl 1.5 does not have the apps/v1beta1/deployment resource compiled in, the preferred group order must have extensions come before apps. The preference order is determined by the order that the groups are listed by the discovery service, with the first elements preferred over the last elements.

This PR:
* updates the discovery code to preserve the order groups were registered in
* updates the registration order to move the `apps` group to the end of the list (for the same result as https://github.com/kubernetes/kubernetes/issues/43543)

This has the side benefit of making all TPR API groups (regardless of group name) come after the core API groups, instead of potentially appearing earlier in discovery order

```release-note
The API server discovery document now prioritizes the `extensions` API group over the `apps` API group. This ensures certain commands in 1.5 versions of kubectl (such as `kubectl edit deployment`) continue to function against a 1.6 API.
```

api/swagger-spec/resourceListing.json

@@ -21,14 +21,6 @@
     "path": "/api",
     "description": "get available API versions"
    },
-   {
-    "path": "/apis/apps/v1beta1",
-    "description": "API at /apis/apps/v1beta1"
-   },
-   {
-    "path": "/apis/apps",
-    "description": "get information of a group"
-   },
    {
     "path": "/apis/authentication.k8s.io/v1",
     "description": "API at /apis/authentication.k8s.io/v1"
@@ -113,6 +105,14 @@
     "path": "/apis/rbac.authorization.k8s.io",
     "description": "get information of a group"
    },
+   {
+    "path": "/apis/settings.k8s.io/v1alpha1",
+    "description": "API at /apis/settings.k8s.io/v1alpha1"
+   },
+   {
+    "path": "/apis/settings.k8s.io",
+    "description": "get information of a group"
+   },
    {
     "path": "/apis/storage.k8s.io/v1beta1",
     "description": "API at /apis/storage.k8s.io/v1beta1"
@@ -126,11 +126,11 @@
     "description": "get information of a group"
    },
    {
-    "path": "/apis/settings.k8s.io/v1alpha1",
-    "description": "API at /apis/settings.k8s.io/v1alpha1"
+    "path": "/apis/apps/v1beta1",
+    "description": "API at /apis/apps/v1beta1"
    },
    {
-    "path": "/apis/settings.k8s.io",
+    "path": "/apis/apps",
     "description": "get information of a group"
    }
   ],

hack/make-rules/test-cmd-util.sh

@@ -1151,7 +1151,7 @@ run_kubectl_get_tests() {
   kube::test::if_has_string "${output_message}" "/apis/apps/v1beta1/namespaces/default/statefulsets 200 OK"
   kube::test::if_has_string "${output_message}" "/apis/autoscaling/v1/namespaces/default/horizontalpodautoscalers 200"
   kube::test::if_has_string "${output_message}" "/apis/batch/v1/namespaces/default/jobs 200 OK"
-  kube::test::if_has_string "${output_message}" "/apis/apps/v1beta1/namespaces/default/deployments 200 OK"
+  kube::test::if_has_string "${output_message}" "/apis/extensions/v1beta1/namespaces/default/deployments 200 OK"
   kube::test::if_has_string "${output_message}" "/apis/extensions/v1beta1/namespaces/default/replicasets 200 OK"
 
   ### Test --allow-missing-template-keys

pkg/master/master.go

@@ -240,8 +240,10 @@ func (c completedConfig) New() (*Master, error) {
 		m.InstallLegacyAPI(c.Config, c.Config.GenericConfig.RESTOptionsGetter, legacyRESTStorageProvider)
 	}
 
+	// The order here is preserved in discovery.
+	// If resources with identical names exist in more than one of these groups (e.g. "deployments.apps"" and "deployments.extensions"),
+	// the order of this list determines which group an unqualified resource name (e.g. "deployments") should prefer.
 	restStorageProviders := []RESTStorageProvider{
-		appsrest.RESTStorageProvider{},
 		authenticationrest.RESTStorageProvider{Authenticator: c.GenericConfig.Authenticator},
 		authorizationrest.RESTStorageProvider{Authorizer: c.GenericConfig.Authorizer},
 		autoscalingrest.RESTStorageProvider{},
@@ -250,8 +252,11 @@ func (c completedConfig) New() (*Master, error) {
 		extensionsrest.RESTStorageProvider{ResourceInterface: thirdparty.NewThirdPartyResourceServer(s, c.StorageFactory)},
 		policyrest.RESTStorageProvider{},
 		rbacrest.RESTStorageProvider{Authorizer: c.GenericConfig.Authorizer},
-		storagerest.RESTStorageProvider{},
 		settingsrest.RESTStorageProvider{},
+		storagerest.RESTStorageProvider{},
+		// keep apps after extensions so legacy clients resolve the extensions versions of shared resource names.
+		// See https://github.com/kubernetes/kubernetes/issues/42392
+		appsrest.RESTStorageProvider{},
 	}
 	m.InstallAPIs(c.Config.APIResourceConfigSource, c.Config.GenericConfig.RESTOptionsGetter, restStorageProviders...)
 

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

@@ -20,7 +20,6 @@ import (
 	"fmt"
 	"mime"
 	"net/http"
-	"sort"
 	"strings"
 	"sync"
 	"time"
@@ -128,8 +127,10 @@ type GenericAPIServer struct {
 
 	// Map storing information about all groups to be exposed in discovery response.
 	// The map is from name to the group.
+	// The slice preserves group name insertion order.
 	apiGroupsForDiscoveryLock sync.RWMutex
 	apiGroupsForDiscovery     map[string]metav1.APIGroup
+	apiGroupNamesForDiscovery []string
 
 	// Enable swagger and/or OpenAPI if these configs are non-nil.
 	swaggerConfig *swagger.Config
@@ -334,6 +335,11 @@ func (s *GenericAPIServer) AddAPIGroupForDiscovery(apiGroup metav1.APIGroup) {
 	s.apiGroupsForDiscoveryLock.Lock()
 	defer s.apiGroupsForDiscoveryLock.Unlock()
 
+	// Insert the group into the ordered list if it is not already present
+	if _, exists := s.apiGroupsForDiscovery[apiGroup.Name]; !exists {
+		s.apiGroupNamesForDiscovery = append(s.apiGroupNamesForDiscovery, apiGroup.Name)
+	}
+
 	s.apiGroupsForDiscovery[apiGroup.Name] = apiGroup
 }
 
@@ -341,6 +347,16 @@ func (s *GenericAPIServer) RemoveAPIGroupForDiscovery(groupName string) {
 	s.apiGroupsForDiscoveryLock.Lock()
 	defer s.apiGroupsForDiscoveryLock.Unlock()
 
+	// Remove the group from the ordered list
+	// https://github.com/golang/go/wiki/SliceTricks#filtering-without-allocating
+	newOrder := s.apiGroupNamesForDiscovery[:0]
+	for _, orderedGroupName := range s.apiGroupNamesForDiscovery {
+		if orderedGroupName != groupName {
+			newOrder = append(newOrder, orderedGroupName)
+		}
+	}
+	s.apiGroupNamesForDiscovery = newOrder
+
 	delete(s.apiGroupsForDiscovery, groupName)
 }
 
@@ -385,14 +401,10 @@ func (s *GenericAPIServer) DynamicApisDiscovery() *restful.WebService {
 		s.apiGroupsForDiscoveryLock.RLock()
 		defer s.apiGroupsForDiscoveryLock.RUnlock()
 
-		// sort to have a deterministic order
 		sortedGroups := []metav1.APIGroup{}
-		groupNames := make([]string, 0, len(s.apiGroupsForDiscovery))
-		for groupName := range s.apiGroupsForDiscovery {
-			groupNames = append(groupNames, groupName)
-		}
-		sort.Strings(groupNames)
-		for _, groupName := range groupNames {
+
+		// ranging over apiGroupNamesForDiscovery preserves the registration order
+		for _, groupName := range s.apiGroupNamesForDiscovery {
 			sortedGroups = append(sortedGroups, s.apiGroupsForDiscovery[groupName])
 		}
 

staging/src/k8s.io/apiserver/pkg/server/genericapiserver_test.go

@@ -529,6 +529,64 @@ func TestDiscoveryAtAPIS(t *testing.T) {
 	assert.Equal(0, len(groupList.Groups))
 }
 
+func TestDiscoveryOrdering(t *testing.T) {
+	master, etcdserver, _, assert := newMaster(t)
+	defer etcdserver.Terminate(t)
+
+	server := httptest.NewServer(master.InsecureHandler)
+	groupList, err := getGroupList(server)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	assert.Equal(0, len(groupList.Groups))
+
+	// Register three groups
+	master.AddAPIGroupForDiscovery(metav1.APIGroup{Name: "x"})
+	master.AddAPIGroupForDiscovery(metav1.APIGroup{Name: "y"})
+	master.AddAPIGroupForDiscovery(metav1.APIGroup{Name: "z"})
+	// Register three additional groups that come earlier alphabetically
+	master.AddAPIGroupForDiscovery(metav1.APIGroup{Name: "a"})
+	master.AddAPIGroupForDiscovery(metav1.APIGroup{Name: "b"})
+	master.AddAPIGroupForDiscovery(metav1.APIGroup{Name: "c"})
+	// Make sure re-adding doesn't double-register or make a group lose its place
+	master.AddAPIGroupForDiscovery(metav1.APIGroup{Name: "x"})
+
+	groupList, err = getGroupList(server)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	assert.Equal(6, len(groupList.Groups))
+	assert.Equal("x", groupList.Groups[0].Name)
+	assert.Equal("y", groupList.Groups[1].Name)
+	assert.Equal("z", groupList.Groups[2].Name)
+	assert.Equal("a", groupList.Groups[3].Name)
+	assert.Equal("b", groupList.Groups[4].Name)
+	assert.Equal("c", groupList.Groups[5].Name)
+
+	// Remove a group.
+	master.RemoveAPIGroupForDiscovery("a")
+	groupList, err = getGroupList(server)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	assert.Equal(5, len(groupList.Groups))
+
+	// Re-adding should move to the end.
+	master.AddAPIGroupForDiscovery(metav1.APIGroup{Name: "a"})
+	groupList, err = getGroupList(server)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	assert.Equal(6, len(groupList.Groups))
+	assert.Equal("x", groupList.Groups[0].Name)
+	assert.Equal("y", groupList.Groups[1].Name)
+	assert.Equal("z", groupList.Groups[2].Name)
+	assert.Equal("b", groupList.Groups[3].Name)
+	assert.Equal("c", groupList.Groups[4].Name)
+	assert.Equal("a", groupList.Groups[5].Name)
+}
+
 func TestGetServerAddressByClientCIDRs(t *testing.T) {
 	publicAddressCIDRMap := []metav1.ServerAddressByClientCIDR{
 		{