Merge pull request #50013 from deads2k/tpr-20-remove-bad-discovery

Automatic merge from submit-queue

cleanup dead installer code

cleans up some installer code that was dead and reorders a little of the flow to reduce complexity.

@kubernetes/sig-api-machinery-misc

staging/src/k8s.io/apiserver/pkg/endpoints/BUILD

@@ -70,7 +70,6 @@ go_library(
     tags = ["automanaged"],
     deps = [
         "//vendor/github.com/emicklei/go-restful:go_default_library",
-        "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/conversion:go_default_library",

staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go

@@ -3228,91 +3228,6 @@ func TestCreateChecksDecode(t *testing.T) {
 	}
 }
 
-// TestUpdateREST tests that you can add new rest implementations to a pre-existing
-// web service.
-func TestUpdateREST(t *testing.T) {
-	makeGroup := func(storage map[string]rest.Storage) *APIGroupVersion {
-		return &APIGroupVersion{
-			Storage:   storage,
-			Root:      "/" + prefix,
-			Creater:   scheme,
-			Convertor: scheme,
-			Copier:    scheme,
-			Defaulter: scheme,
-			Typer:     scheme,
-			Linker:    selfLinker,
-
-			Admit:   admissionControl,
-			Context: requestContextMapper,
-			Mapper:  namespaceMapper,
-
-			GroupVersion:           newGroupVersion,
-			OptionsExternalVersion: &newGroupVersion,
-
-			Serializer:     codecs,
-			ParameterCodec: parameterCodec,
-		}
-	}
-
-	makeStorage := func(paths ...string) map[string]rest.Storage {
-		storage := map[string]rest.Storage{}
-		for _, s := range paths {
-			storage[s] = &SimpleRESTStorage{}
-		}
-		return storage
-	}
-
-	testREST := func(t *testing.T, container *restful.Container, barCode int) {
-		handler := genericapifilters.WithRequestInfo(container, newTestRequestInfoResolver(), requestContextMapper)
-		handler = request.WithRequestContext(handler, requestContextMapper)
-
-		w := httptest.NewRecorder()
-		handler.ServeHTTP(w, &http.Request{Method: "GET", URL: &url.URL{Path: "/" + prefix + "/" + newGroupVersion.Group + "/" + newGroupVersion.Version + "/namespaces/test/foo/test"}})
-		if w.Code != http.StatusOK {
-			t.Fatalf("expected OK: %#v", w)
-		}
-
-		w = httptest.NewRecorder()
-		handler.ServeHTTP(w, &http.Request{Method: "GET", URL: &url.URL{Path: "/" + prefix + "/" + newGroupVersion.Group + "/" + newGroupVersion.Version + "/namespaces/test/bar/test"}})
-		if w.Code != barCode {
-			t.Errorf("expected response code %d for GET to bar but received %d", barCode, w.Code)
-		}
-	}
-
-	storage1 := makeStorage("foo")
-	group1 := makeGroup(storage1)
-
-	storage2 := makeStorage("bar")
-	group2 := makeGroup(storage2)
-
-	container := restful.NewContainer()
-
-	// install group1.  Ensure that
-	// 1. Foo storage is accessible
-	// 2. Bar storage is not accessible
-	if err := group1.InstallREST(container); err != nil {
-		t.Fatal(err)
-	}
-	testREST(t, container, http.StatusNotFound)
-
-	// update with group2.  Ensure that
-	// 1.  Foo storage is still accessible
-	// 2.  Bar storage is now accessible
-	if err := group2.UpdateREST(container); err != nil {
-		t.Fatal(err)
-	}
-	testREST(t, container, http.StatusOK)
-
-	// try to update a group that does not have an existing webservice with a matching prefix
-	// should not affect the existing registered webservice
-	invalidGroup := makeGroup(storage1)
-	invalidGroup.Root = "bad"
-	if err := invalidGroup.UpdateREST(container); err == nil {
-		t.Fatal("expected an error from UpdateREST when updating a non-existing prefix but got none")
-	}
-	testREST(t, container, http.StatusOK)
-}
-
 func TestParentResourceIsRequired(t *testing.T) {
 	storage := &SimpleTypedStorage{
 		baseType: &genericapitesting.SimpleRoot{}, // a root scoped type

staging/src/k8s.io/apiserver/pkg/endpoints/groupversion.go

@@ -17,13 +17,11 @@ limitations under the License.
 package endpoints
 
 import (
-	"fmt"
 	"path"
 	"time"
 
 	"github.com/emicklei/go-restful"
 
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -84,10 +82,6 @@ type APIGroupVersion struct {
 	// match the keys in the Storage map above for subresources.
 	SubresourceGroupVersionKind map[string]schema.GroupVersionKind
 
-	// ResourceLister is an interface that knows how to list resources
-	// for this API Group.
-	ResourceLister discovery.APIResourceLister
-
 	// EnableAPIResponseCompression indicates whether API Responses should support compression
 	// if the client requests it via Accept-Encoding
 	EnableAPIResponseCompression bool
@@ -97,49 +91,6 @@ type APIGroupVersion struct {
 // It is expected that the provided path root prefix will serve all operations. Root MUST NOT end
 // in a slash.
 func (g *APIGroupVersion) InstallREST(container *restful.Container) error {
-	installer := g.newInstaller()
-	ws := installer.NewWebService()
-	apiResources, registrationErrors := installer.Install(ws)
-	lister := g.ResourceLister
-	if lister == nil {
-		lister = staticLister{apiResources}
-	}
-	versionDiscoveryHandler := discovery.NewAPIVersionHandler(g.Serializer, g.GroupVersion, lister, g.Context)
-	versionDiscoveryHandler.AddToWebService(ws)
-	container.Add(ws)
-	return utilerrors.NewAggregate(registrationErrors)
-}
-
-// UpdateREST registers the REST handlers for this APIGroupVersion to an existing web service
-// in the restful Container.  It will use the prefix (root/version) to find the existing
-// web service.  If a web service does not exist within the container to support the prefix
-// this method will return an error.
-func (g *APIGroupVersion) UpdateREST(container *restful.Container) error {
-	installer := g.newInstaller()
-	var ws *restful.WebService = nil
-
-	for i, s := range container.RegisteredWebServices() {
-		if s.RootPath() == installer.prefix {
-			ws = container.RegisteredWebServices()[i]
-			break
-		}
-	}
-
-	if ws == nil {
-		return apierrors.NewInternalError(fmt.Errorf("unable to find an existing webservice for prefix %s", installer.prefix))
-	}
-	apiResources, registrationErrors := installer.Install(ws)
-	lister := g.ResourceLister
-	if lister == nil {
-		lister = staticLister{apiResources}
-	}
-	versionDiscoveryHandler := discovery.NewAPIVersionHandler(g.Serializer, g.GroupVersion, lister, g.Context)
-	versionDiscoveryHandler.AddToWebService(ws)
-	return utilerrors.NewAggregate(registrationErrors)
-}
-
-// newInstaller is a helper to create the installer.  Used by InstallREST and UpdateREST.
-func (g *APIGroupVersion) newInstaller() *APIInstaller {
 	prefix := path.Join(g.Root, g.GroupVersion.Group, g.GroupVersion.Version)
 	installer := &APIInstaller{
 		group:                        g,
@@ -147,7 +98,12 @@ func (g *APIGroupVersion) newInstaller() *APIInstaller {
 		minRequestTimeout:            g.MinRequestTimeout,
 		enableAPIResponseCompression: g.EnableAPIResponseCompression,
 	}
-	return installer
+
+	apiResources, ws, registrationErrors := installer.Install()
+	versionDiscoveryHandler := discovery.NewAPIVersionHandler(g.Serializer, g.GroupVersion, staticLister{apiResources}, g.Context)
+	versionDiscoveryHandler.AddToWebService(ws)
+	container.Add(ws)
+	return utilerrors.NewAggregate(registrationErrors)
 }
 
 // staticLister implements the APIResourceLister interface

staging/src/k8s.io/apiserver/pkg/endpoints/installer.go

@@ -85,8 +85,10 @@ var toDiscoveryKubeVerb = map[string]string{
 }
 
 // Install handlers for API resources.
-func (a *APIInstaller) Install(ws *restful.WebService) (apiResources []metav1.APIResource, errors []error) {
+func (a *APIInstaller) Install() ([]metav1.APIResource, *restful.WebService, []error) {
-	errors = make([]error, 0)
+	var apiResources []metav1.APIResource
+	var errors []error
+	ws := a.newWebService()
 
 	proxyHandler := (&handlers.ProxyHandler{
 		Prefix:     a.prefix + "/proxy/",
@@ -112,11 +114,11 @@ func (a *APIInstaller) Install(ws *restful.WebService) (apiResources []metav1.AP
 			apiResources = append(apiResources, *apiResource)
 		}
 	}
-	return apiResources, errors
+	return apiResources, ws, errors
 }
 
-// NewWebService creates a new restful webservice with the api installer's prefix and version.
+// newWebService creates a new restful webservice with the api installer's prefix and version.
-func (a *APIInstaller) NewWebService() *restful.WebService {
+func (a *APIInstaller) newWebService() *restful.WebService {
 	ws := new(restful.WebService)
 	ws.Path(a.prefix)
 	// a.prefix contains "prefix/group/version"

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

@@ -108,9 +108,6 @@ type GenericAPIServer struct {
 	// external (public internet) URLs for this GenericAPIServer.
 	ExternalAddress string
 
-	// storage contains the RESTful endpoints exposed by this GenericAPIServer
-	storage map[string]rest.Storage
-
 	// Serializer controls how common API objects not in a group/version prefix are serialized for this server.
 	// Individual APIGroups may define their own serializers.
 	Serializer runtime.NegotiatedSerializer