Merge pull request #31245 from deads2k/api-02

Automatic merge from submit-queue

privatize, document, and scrub GenericAPIServer

I've gone through more of the `GenericAPIServer` struct, started documenting what the fields do and privatizing ones that aren't used elsewhere or are only used by components that need some refactoring too.

pkg/genericapiserver/genericapiserver.go

@@ -58,7 +58,6 @@ import (
 	"k8s.io/kubernetes/pkg/runtime"
 	"k8s.io/kubernetes/pkg/ui"
 	"k8s.io/kubernetes/pkg/util"
-	"k8s.io/kubernetes/pkg/util/async"
 	"k8s.io/kubernetes/pkg/util/crypto"
 	utilnet "k8s.io/kubernetes/pkg/util/net"
 	utilruntime "k8s.io/kubernetes/pkg/util/runtime"
@@ -207,30 +206,48 @@ type Config struct {
 
 // GenericAPIServer contains state for a Kubernetes cluster api server.
 type GenericAPIServer struct {
-	// "Inputs", Copied from Config
+	// ServiceClusterIPRange is used to build cluster IPs for discovery.  It is exposed so that `master.go` can
+	// construct service storage.
+	// TODO refactor this so that `master.go` drives the value used for discovery and the value here isn't exposed.
+	// that structure will force usage in the correct direction where the "owner" of the value is the source of
+	// truth for its value.
 	ServiceClusterIPRange *net.IPNet
-	ServiceNodePortRange  utilnet.PortRange
-	cacheTimeout          time.Duration
-	MinRequestTimeout     time.Duration
 
-	mux                   apiserver.Mux
+	// ServiceNodePortRange is only used for `master.go` to construct its RESTStorage for the legacy API group
-	MuxHelper             *apiserver.MuxHelper
+	// TODO refactor this closer to the point of use.
-	HandlerContainer      *restful.Container
+	ServiceNodePortRange utilnet.PortRange
-	RootWebService        *restful.WebService
+
-	enableLogsSupport     bool
+	// minRequestTimeout is how short the request timeout can be.  This is used to build the RESTHandler
-	enableUISupport       bool
+	minRequestTimeout time.Duration
-	enableSwaggerSupport  bool
+
-	enableSwaggerUI       bool
+	// enableSwaggerSupport indicates that swagger should be served.  This is currently separate because
-	enableProfiling       bool
+	// the API group routes are created *after* initialization and you can't generate the swagger routes until
-	enableWatchCache      bool
+	// after those are available.
-	APIPrefix             string
+	// TODO eventually we should be able to factor this out to take place during initialization.
-	APIGroupPrefix        string
+	enableSwaggerSupport bool
-	corsAllowedOriginList []string
+
-	authenticator         authenticator.Request
+	// legacyAPIPrefix is the prefix used for legacy API groups that existed before we had API groups
-	authorizer            authorizer.Authorizer
+	// usuallly /api
-	AdmissionControl      admission.Interface
+	legacyAPIPrefix string
-	MasterCount           int
+
-	RequestContextMapper  api.RequestContextMapper
+	// apiPrefix is the prefix where API groups live, usually /apis
+	apiPrefix string
+
+	// admissionControl is used to build the RESTStorage that backs an API Group.
+	admissionControl admission.Interface
+
+	// requestContextMapper provides a way to get the context for a request.  It may be nil.
+	requestContextMapper api.RequestContextMapper
+
+	// storageDecorator provides a decoration function for storage.  It will never be nil.
+	// TODO: this may be an abstraction at the wrong layer.  It doesn't seem like a genericAPIServer
+	// should be determining the backing storage for the RESTStorage interfaces
+	storageDecorator generic.StorageDecorator
+
+	mux              apiserver.Mux
+	MuxHelper        *apiserver.MuxHelper
+	HandlerContainer *restful.Container
+	MasterCount      int
 
 	// ExternalAddress is the address (hostname or IP and port) that should be used in
 	// external (public internet) URLs for this GenericAPIServer.
@@ -240,7 +257,6 @@ type GenericAPIServer struct {
 	PublicReadWritePort  int
 	ServiceReadWriteIP   net.IP
 	ServiceReadWritePort int
-	masterServices       *async.Runner
 	ExtraServicePorts    []api.ServicePort
 	ExtraEndpointPorts   []api.EndpointPort
 
@@ -272,10 +288,19 @@ type GenericAPIServer struct {
 }
 
 func (s *GenericAPIServer) StorageDecorator() generic.StorageDecorator {
-	if s.enableWatchCache {
+	return s.storageDecorator
-		return registry.StorageWithCacher
+}
-	}
+
-	return generic.UndecoratedStorage
+// RequestContextMapper is exposed so that third party resource storage can be build in a different location.
+// TODO refactor third party resource storage
+func (s *GenericAPIServer) RequestContextMapper() api.RequestContextMapper {
+	return s.requestContextMapper
+}
+
+// MinRequestTimeout is exposed so that third party resource storage can be build in a different location.
+// TODO refactor third party resource storage
+func (s *GenericAPIServer) MinRequestTimeout() time.Duration {
+	return s.minRequestTimeout
 }
 
 // setDefaults fills in any fields not set that are required to have valid data.
@@ -365,24 +390,14 @@ func New(c *Config) (*GenericAPIServer, error) {
 	s := &GenericAPIServer{
 		ServiceClusterIPRange: c.ServiceClusterIPRange,
 		ServiceNodePortRange:  c.ServiceNodePortRange,
-		RootWebService:        new(restful.WebService),
+		legacyAPIPrefix:       c.APIPrefix,
-		enableLogsSupport:     c.EnableLogsSupport,
+		apiPrefix:             c.APIGroupPrefix,
-		enableUISupport:       c.EnableUISupport,
+		admissionControl:      c.AdmissionControl,
-		enableSwaggerSupport:  c.EnableSwaggerSupport,
+		requestContextMapper:  c.RequestContextMapper,
-		enableSwaggerUI:       c.EnableSwaggerUI,
-		enableProfiling:       c.EnableProfiling,
-		enableWatchCache:      c.EnableWatchCache,
-		APIPrefix:             c.APIPrefix,
-		APIGroupPrefix:        c.APIGroupPrefix,
-		corsAllowedOriginList: c.CorsAllowedOriginList,
-		authenticator:         c.Authenticator,
-		authorizer:            c.Authorizer,
-		AdmissionControl:      c.AdmissionControl,
-		RequestContextMapper:  c.RequestContextMapper,
 		Serializer:            c.Serializer,
 
-		cacheTimeout:      c.CacheTimeout,
+		minRequestTimeout:    time.Duration(c.MinRequestTimeout) * time.Second,
-		MinRequestTimeout: time.Duration(c.MinRequestTimeout) * time.Second,
+		enableSwaggerSupport: c.EnableSwaggerSupport,
 
 		MasterCount:          c.MasterCount,
 		ExternalAddress:      c.ExternalHost,
@@ -401,6 +416,12 @@ func New(c *Config) (*GenericAPIServer, error) {
 		openAPIDefaultResponse: c.OpenAPIDefaultResponse,
 	}
 
+	if c.EnableWatchCache {
+		s.storageDecorator = registry.StorageWithCacher
+	} else {
+		s.storageDecorator = generic.UndecoratedStorage
+	}
+
 	if c.RestfulContainer != nil {
 		s.mux = c.RestfulContainer.ServeMux
 		s.HandlerContainer = c.RestfulContainer
@@ -420,8 +441,8 @@ func New(c *Config) (*GenericAPIServer, error) {
 
 func (s *GenericAPIServer) NewRequestInfoResolver() *apiserver.RequestInfoResolver {
 	return &apiserver.RequestInfoResolver{
-		APIPrefixes:          sets.NewString(strings.Trim(s.APIPrefix, "/"), strings.Trim(s.APIGroupPrefix, "/")), // all possible API prefixes
+		APIPrefixes:          sets.NewString(strings.Trim(s.legacyAPIPrefix, "/"), strings.Trim(s.apiPrefix, "/")), // all possible API prefixes
-		GrouplessAPIPrefixes: sets.NewString(strings.Trim(s.APIPrefix, "/")),                                      // APIPrefixes that won't have groups (legacy)
+		GrouplessAPIPrefixes: sets.NewString(strings.Trim(s.legacyAPIPrefix, "/")),                                 // APIPrefixes that won't have groups (legacy)
 	}
 }
 
@@ -473,7 +494,7 @@ func (s *GenericAPIServer) init(c *Config) {
 		apiserver.InstallLogsSupport(s.MuxHelper, s.HandlerContainer)
 	}
 	if c.EnableUISupport {
-		ui.InstallSupport(s.MuxHelper, s.enableSwaggerSupport && s.enableSwaggerUI)
+		ui.InstallSupport(s.MuxHelper, c.EnableSwaggerSupport && c.EnableSwaggerUI)
 	}
 
 	if c.EnableProfiling {
@@ -500,8 +521,8 @@ func (s *GenericAPIServer) init(c *Config) {
 
 	s.InsecureHandler = handler
 
-	attributeGetter := apiserver.NewRequestAttributeGetter(s.RequestContextMapper, s.NewRequestInfoResolver())
+	attributeGetter := apiserver.NewRequestAttributeGetter(c.RequestContextMapper, s.NewRequestInfoResolver())
-	handler = apiserver.WithAuthorizationCheck(handler, attributeGetter, s.authorizer)
+	handler = apiserver.WithAuthorizationCheck(handler, attributeGetter, c.Authorizer)
 	if len(c.AuditLogPath) != 0 {
 		// audit handler must comes before the impersonationFilter to read the original user
 		writer := &lumberjack.Logger{
@@ -510,14 +531,14 @@ func (s *GenericAPIServer) init(c *Config) {
 			MaxBackups: c.AuditLogMaxBackups,
 			MaxSize:    c.AuditLogMaxSize,
 		}
-		handler = audit.WithAudit(handler, s.RequestContextMapper, writer)
+		handler = audit.WithAudit(handler, c.RequestContextMapper, writer)
 		defer writer.Close()
 	}
-	handler = apiserver.WithImpersonation(handler, s.RequestContextMapper, s.authorizer)
+	handler = apiserver.WithImpersonation(handler, c.RequestContextMapper, c.Authorizer)
 
 	// Install Authenticator
 	if c.Authenticator != nil {
-		authenticatedHandler, err := handlers.NewRequestAuthenticator(s.RequestContextMapper, c.Authenticator, handlers.Unauthorized(c.SupportsBasicAuth), handler)
+		authenticatedHandler, err := handlers.NewRequestAuthenticator(c.RequestContextMapper, c.Authenticator, handlers.Unauthorized(c.SupportsBasicAuth), handler)
 		if err != nil {
 			glog.Fatalf("Could not initialize authenticator: %v", err)
 		}
@@ -529,13 +550,13 @@ func (s *GenericAPIServer) init(c *Config) {
 
 	// After all wrapping is done, put a context filter around both handlers
 	var err error
-	handler, err = api.NewRequestContextFilter(s.RequestContextMapper, s.Handler)
+	handler, err = api.NewRequestContextFilter(c.RequestContextMapper, s.Handler)
 	if err != nil {
 		glog.Fatalf("Could not initialize request context filter for s.Handler: %v", err)
 	}
 	s.Handler = handler
 
-	handler, err = api.NewRequestContextFilter(s.RequestContextMapper, s.InsecureHandler)
+	handler, err = api.NewRequestContextFilter(c.RequestContextMapper, s.InsecureHandler)
 	if err != nil {
 		glog.Fatalf("Could not initialize request context filter for s.InsecureHandler: %v", err)
 	}
@@ -556,7 +577,7 @@ func (s *GenericAPIServer) InstallAPIGroups(groupsInfo []APIGroupInfo) error {
 
 // Installs handler at /apis to list all group versions for discovery
 func (s *GenericAPIServer) installGroupsDiscoveryHandler() {
-	apiserver.AddApisWebService(s.Serializer, s.HandlerContainer, s.APIGroupPrefix, func(req *restful.Request) []unversioned.APIGroup {
+	apiserver.AddApisWebService(s.Serializer, s.HandlerContainer, s.apiPrefix, func(req *restful.Request) []unversioned.APIGroup {
 		// Return the list of supported groups in sorted order (to have a deterministic order).
 		groups := []unversioned.APIGroup{}
 		groupNames := make([]string, len(s.apiGroupsForDiscovery))
@@ -774,9 +795,9 @@ func (s *GenericAPIServer) Run(options *options.ServerRunOptions) {
 
 // Exposes the given group version in API.
 func (s *GenericAPIServer) InstallAPIGroup(apiGroupInfo *APIGroupInfo) error {
-	apiPrefix := s.APIGroupPrefix
+	apiPrefix := s.apiPrefix
 	if apiGroupInfo.IsLegacyGroup {
-		apiPrefix = s.APIPrefix
+		apiPrefix = s.legacyAPIPrefix
 	}
 
 	// Install REST handlers for all the versions in this group.
@@ -896,9 +917,9 @@ func (s *GenericAPIServer) newAPIGroupVersion(apiGroupInfo *APIGroupInfo, groupV
 		Linker: apiGroupInfo.GroupMeta.SelfLinker,
 		Mapper: apiGroupInfo.GroupMeta.RESTMapper,
 
-		Admit:             s.AdmissionControl,
+		Admit:             s.admissionControl,
-		Context:           s.RequestContextMapper,
+		Context:           s.RequestContextMapper(),
-		MinRequestTimeout: s.MinRequestTimeout,
+		MinRequestTimeout: s.minRequestTimeout,
 	}, nil
 }
 

pkg/genericapiserver/genericapiserver_test.go

@@ -76,19 +76,11 @@ func TestNew(t *testing.T) {
 	defer etcdserver.Terminate(t)
 
 	// Verify many of the variables match their config counterparts
-	assert.Equal(s.enableLogsSupport, config.EnableLogsSupport)
-	assert.Equal(s.enableUISupport, config.EnableUISupport)
 	assert.Equal(s.enableSwaggerSupport, config.EnableSwaggerSupport)
-	assert.Equal(s.enableSwaggerUI, config.EnableSwaggerUI)
+	assert.Equal(s.legacyAPIPrefix, config.APIPrefix)
-	assert.Equal(s.enableProfiling, config.EnableProfiling)
+	assert.Equal(s.apiPrefix, config.APIGroupPrefix)
-	assert.Equal(s.APIPrefix, config.APIPrefix)
+	assert.Equal(s.admissionControl, config.AdmissionControl)
-	assert.Equal(s.APIGroupPrefix, config.APIGroupPrefix)
+	assert.Equal(s.RequestContextMapper(), config.RequestContextMapper)
-	assert.Equal(s.corsAllowedOriginList, config.CorsAllowedOriginList)
-	assert.Equal(s.authenticator, config.Authenticator)
-	assert.Equal(s.authorizer, config.Authorizer)
-	assert.Equal(s.AdmissionControl, config.AdmissionControl)
-	assert.Equal(s.RequestContextMapper, config.RequestContextMapper)
-	assert.Equal(s.cacheTimeout, config.CacheTimeout)
 	assert.Equal(s.ExternalAddress, config.ExternalHost)
 	assert.Equal(s.ClusterIP, config.PublicAddress)
 	assert.Equal(s.PublicReadWritePort, config.ReadWritePort)

pkg/master/master.go

@@ -760,9 +760,9 @@ func (m *Master) thirdpartyapi(group, kind, version, pluralResource string) *api
 		Serializer:     thirdpartyresourcedata.NewNegotiatedSerializer(api.Codecs, kind, externalVersion, internalVersion),
 		ParameterCodec: thirdpartyresourcedata.NewThirdPartyParameterCodec(api.ParameterCodec),
 
-		Context: m.RequestContextMapper,
+		Context: m.RequestContextMapper(),
 
-		MinRequestTimeout: m.MinRequestTimeout,
+		MinRequestTimeout: m.MinRequestTimeout(),
 
 		ResourceLister: dynamicLister{m, makeThirdPartyPath(group)},
 	}

pkg/master/master_test.go

@@ -173,9 +173,7 @@ func TestNew(t *testing.T) {
 	// Verify many of the variables match their config counterparts
 	assert.Equal(master.enableCoreControllers, config.EnableCoreControllers)
 	assert.Equal(master.tunneler, config.Tunneler)
-	assert.Equal(master.APIPrefix, config.APIPrefix)
+	assert.Equal(master.RequestContextMapper(), config.RequestContextMapper)
-	assert.Equal(master.APIGroupPrefix, config.APIGroupPrefix)
-	assert.Equal(master.RequestContextMapper, config.RequestContextMapper)
 	assert.Equal(master.MasterCount, config.MasterCount)
 	assert.Equal(master.ClusterIP, config.PublicAddress)
 	assert.Equal(master.PublicReadWritePort, config.ReadWritePort)