Skip to content

Commit

Permalink
Do not initialize custom REST handler during ApiVersion initialization
Browse files Browse the repository at this point in the history
ApiVersion variable from the versioned package is initialized both
in the apiserver and in the controller-manager because they both
import the versioned package.
When a custom REST handler is defined, its NewFooREST() constructor is
being called directly during ApiVersion initialization. This constructor
may contain some other logic besides simply initializing the custom REST
registry type. For example it may establish a connection to some service.
And we'll end up with an extra unused connection to the external
service, established from the controller manager.

Passing NewFooREST as a function value to builders.NewApiResourceWithStorage()
and then calling it from versionedResourceBuilder.registerEndpoints()
solves this problem.

Fixes #92.
  • Loading branch information
ibazhitov committed Jul 13, 2017
1 parent 4fbef6f commit 8cc3006
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 12 deletions.
20 changes: 17 additions & 3 deletions cmd/apiregister-gen/generators/versioned_generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,28 @@ func CreateVersionedGenerator(apiversion *APIVersion, apigroup *APIGroup, filena
}
}

func hasSubresources(version *APIVersion) bool {
for _, v := range version.Resources {
if len(v.Subresources) != 0 {
return true
}
}
return false
}

func (d *versionedGenerator) Imports(c *generator.Context) []string {
return []string{
imports := []string{
"metav1 \"k8s.io/apimachinery/pkg/apis/meta/v1\"",
"k8s.io/apimachinery/pkg/runtime",
"github.com/kubernetes-incubator/apiserver-builder/pkg/builders",
"k8s.io/apimachinery/pkg/runtime/schema",
d.apigroup.Pkg.Path,
}
if hasSubresources(d.apiversion) {
imports = append(imports, "k8s.io/apiserver/pkg/registry/rest")
}

return imports
}

func (d *versionedGenerator) Finalize(context *generator.Context, w io.Writer) error {
Expand All @@ -64,7 +78,7 @@ var (
{{.Kind}}SchemeFns{},
func() runtime.Object { return &{{ $api.Kind }}{} }, // Register versioned resource
func() runtime.Object { return &{{ $api.Kind }}List{} }, // Register versioned resource list
New{{ $api.REST }}(),
New{{ $api.REST }},
)
{{ else -}}
{{$api.Group}}{{$api.Kind}}Storage = builders.NewApiResource( // Resource status endpoint
Expand Down Expand Up @@ -95,7 +109,7 @@ var (
builders.SchemeFnsSingleton,
func() runtime.Object { return &{{ $subresource.Request }}{} }, // Register versioned resource
nil,
&{{ $subresource.REST }}{ {{$api.Group}}.New{{$api.Kind}}Registry({{$api.Group}}{{$api.Kind}}Storage) },
func() rest.Storage { return &{{ $subresource.REST }}{ {{$api.Group}}.New{{$api.Kind}}Registry({{$api.Group}}{{$api.Kind}}Storage) } },
),
{{ end -}}
{{ end -}}
Expand Down
18 changes: 9 additions & 9 deletions pkg/builders/api_versioned_resource_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,15 +61,15 @@ func NewApiResourceWithStorage(
unversionedBuilder UnversionedResourceBuilder,
schemeFns SchemeFns,
new, newList func() runtime.Object,
storage rest.Storage) *versionedResourceBuilder {
RESTFunc func() rest.Storage) *versionedResourceBuilder {
v := &versionedResourceBuilder{
unversionedBuilder, schemeFns, new, newList, nil, storage, nil,
unversionedBuilder, schemeFns, new, newList, nil, RESTFunc, nil,
}
if new == nil {
panic(fmt.Errorf("Cannot call NewApiResourceWithStorage with nil new function."))
}
if storage == nil {
panic(fmt.Errorf("Cannot call NewApiResourceWithStorage with nil new storage."))
if RESTFunc == nil {
panic(fmt.Errorf("Cannot call NewApiResourceWithStorage with nil RESTFunc function."))
}
return v
}
Expand All @@ -84,11 +84,11 @@ type versionedResourceBuilder struct {
// NewListFunc returns and empty unversioned instance of a resource List
NewListFunc func() runtime.Object

// Store is used to modify the default storage, mutually exclusive with RESTFunc
// StorageBuilder is used to modify the default storage, mutually exclusive with RESTFunc
StorageBuilder StorageBuilder

// REST a rest.Store implementation, mutually exclusive with StoreFunc
REST rest.Storage
// RESTFunc returns a rest.Storage implementation, mutually exclusive with StorageBuilder
RESTFunc func() rest.Storage

Storage rest.StandardStorage
}
Expand Down Expand Up @@ -173,9 +173,9 @@ func (b *versionedResourceBuilder) registerEndpoints(
path = b.Unversioned.GetName()
}

if b.REST != nil {
if b.RESTFunc != nil {
// Use the REST implementation directly.
registry[path] = b.REST
registry[path] = b.RESTFunc()
} else {
// Create a new REST implementation wired to storage.
registry[path] = b.
Expand Down

0 comments on commit 8cc3006

Please sign in to comment.