Merge pull request #322 from benluddy/drop-route-allocateshard

Drop AllocateRouterShard from hostname allocation interface.

pkg/route/apiserver/admission/routehostassignment/assignment.go

@@ -12,7 +12,6 @@ import (
 
 	"github.com/openshift/library-go/pkg/authorization/authorizationutil"
 	routeinternal "github.com/openshift/openshift-apiserver/pkg/route/apis/route"
-	"github.com/openshift/openshift-apiserver/pkg/route/apiserver/routeinterfaces"
 )
 
 // HostGeneratedAnnotationKey is the key for an annotation set to "true" if the route's host was generated
@@ -23,10 +22,14 @@ type SubjectAccessReviewCreator interface {
 	Create(ctx context.Context, sar *authorizationv1.SubjectAccessReview, opts metav1.CreateOptions) (*authorizationv1.SubjectAccessReview, error)
 }
 
+type HostnameGenerator interface {
+	GenerateHostname(*routeinternal.Route) (string, error)
+}
+
 // AllocateHost allocates a host name ONLY if the route doesn't specify a subdomain wildcard policy and
 // the host name on the route is empty and an allocator is configured.
 // It must first allocate the shard and may return an error if shard allocation fails.
-func AllocateHost(ctx context.Context, route *routeinternal.Route, sarc SubjectAccessReviewCreator, routeAllocator routeinterfaces.RouteAllocator) field.ErrorList {
+func AllocateHost(ctx context.Context, route *routeinternal.Route, sarc SubjectAccessReviewCreator, routeAllocator HostnameGenerator) field.ErrorList {
 	hostSet := len(route.Spec.Host) > 0
 	certSet := route.Spec.TLS != nil && (len(route.Spec.TLS.CACertificate) > 0 || len(route.Spec.TLS.Certificate) > 0 || len(route.Spec.TLS.DestinationCACertificate) > 0 || len(route.Spec.TLS.Key) > 0)
 	if hostSet || certSet {
@@ -70,11 +73,11 @@ func AllocateHost(ctx context.Context, route *routeinternal.Route, sarc SubjectA
 
 	if len(route.Spec.Subdomain) == 0 && len(route.Spec.Host) == 0 && routeAllocator != nil {
 		// TODO: this does not belong here, and should be removed
-		shard, err := routeAllocator.AllocateRouterShard(route)
+		host, err := routeAllocator.GenerateHostname(route)
 		if err != nil {
 			return field.ErrorList{field.InternalError(field.NewPath("spec", "host"), fmt.Errorf("allocation error: %v for route: %#v", err, route))}
 		}
-		route.Spec.Host = routeAllocator.GenerateHostname(route, shard)
+		route.Spec.Host = host
 		if route.Annotations == nil {
 			route.Annotations = map[string]string{}
 		}

pkg/route/apiserver/admission/routehostassignment/assignment_test.go

@@ -17,11 +17,8 @@ import (
 type testAllocator struct {
 }
 
-func (t testAllocator) AllocateRouterShard(*routeinternal.Route) (*routeinternal.RouterShard, error) {
-	return &routeinternal.RouterShard{}, nil
-}
-func (t testAllocator) GenerateHostname(*routeinternal.Route, *routeinternal.RouterShard) string {
-	return "mygeneratedhost.com"
+func (t testAllocator) GenerateHostname(*routeinternal.Route) (string, error) {
+	return "mygeneratedhost.com", nil
 }
 
 type testSAR struct {

pkg/route/apiserver/registry/route/etcd/etcd.go

@@ -17,7 +17,6 @@ import (
 
 	routeapi "github.com/openshift/openshift-apiserver/pkg/route/apis/route"
 	routeregistry "github.com/openshift/openshift-apiserver/pkg/route/apiserver/registry/route"
-	"github.com/openshift/openshift-apiserver/pkg/route/apiserver/routeinterfaces"
 	routeprinters "github.com/openshift/openshift-apiserver/pkg/route/printers/internalversion"
 )
 
@@ -33,8 +32,12 @@ func (r *REST) Categories() []string {
 	return []string{"all"}
 }
 
+type HostnameGenerator interface {
+	GenerateHostname(*routeapi.Route) (string, error)
+}
+
 // NewREST returns a RESTStorage object that will work against routes.
-func NewREST(optsGetter generic.RESTOptionsGetter, allocator routeinterfaces.RouteAllocator, sarClient routeregistry.SubjectAccessReviewInterface) (*REST, *StatusREST, error) {
+func NewREST(optsGetter generic.RESTOptionsGetter, allocator HostnameGenerator, sarClient routeregistry.SubjectAccessReviewInterface) (*REST, *StatusREST, error) {
 	strategy := routeregistry.NewStrategy(allocator, sarClient)
 
 	store := &registry.Store{

pkg/route/apiserver/registry/route/etcd/etcd_test.go

@@ -20,24 +20,18 @@ import (
 	routeapi "github.com/openshift/openshift-apiserver/pkg/route/apis/route"
 	_ "github.com/openshift/openshift-apiserver/pkg/route/apis/route/install"
 	"github.com/openshift/openshift-apiserver/pkg/route/apiserver/admission/routehostassignment"
-	"github.com/openshift/openshift-apiserver/pkg/route/apiserver/routeinterfaces"
 	"k8s.io/apiserver/pkg/registry/generic"
 )
 
 type testAllocator struct {
 	Hostname string
 	Err      error
-	Allocate bool
 	Generate bool
 }
 
-func (a *testAllocator) AllocateRouterShard(*routeapi.Route) (*routeapi.RouterShard, error) {
-	a.Allocate = true
-	return nil, a.Err
-}
-func (a *testAllocator) GenerateHostname(*routeapi.Route, *routeapi.RouterShard) string {
+func (a *testAllocator) GenerateHostname(*routeapi.Route) (string, error) {
 	a.Generate = true
-	return a.Hostname
+	return a.Hostname, a.Err
 }
 
 type testSAR struct {
@@ -55,7 +49,7 @@ func (t *testSAR) Create(_ context.Context, subjectAccessReview *authorizationap
 	}, t.err
 }
 
-func newStorage(t *testing.T, allocator routeinterfaces.RouteAllocator) (*REST, *etcdtesting.EtcdTestServer) {
+func newStorage(t *testing.T, allocator HostnameGenerator) (*REST, *etcdtesting.EtcdTestServer) {
 	server, etcdStorage := etcdtesting.NewUnsecuredEtcd3TestClientServer(t)
 	etcdStorage.Codec = legacyscheme.Codecs.LegacyCodec(schema.GroupVersion{Group: "route.openshift.io", Version: "v1"})
 	etcdStorageConfigForRoutes := &storagebackend.ConfigForResource{Config: *etcdStorage, GroupResource: schema.GroupResource{Group: "route.openshift.io", Resource: "routes"}}
@@ -113,8 +107,8 @@ func TestCreateWithAllocation(t *testing.T) {
 	if v, ok := result.Annotations[routehostassignment.HostGeneratedAnnotationKey]; !ok || v != "true" {
 		t.Fatalf("unexpected route: %#v", result)
 	}
-	if !allocator.Allocate || !allocator.Generate {
-		t.Fatalf("unexpected allocator: %#v", allocator)
+	if !allocator.Generate {
+		t.Fatalf("hostname generator not invoked: %#v", allocator)
 	}
 }
 

pkg/route/apiserver/registry/route/strategy.go

@@ -15,7 +15,6 @@ import (
 	routeapi "github.com/openshift/openshift-apiserver/pkg/route/apis/route"
 	"github.com/openshift/openshift-apiserver/pkg/route/apis/route/validation"
 	"github.com/openshift/openshift-apiserver/pkg/route/apiserver/admission/routehostassignment"
-	"github.com/openshift/openshift-apiserver/pkg/route/apiserver/routeinterfaces"
 )
 
 // Registry is an interface for performing subject access reviews
@@ -25,21 +24,25 @@ type SubjectAccessReviewInterface interface {
 
 var _ SubjectAccessReviewInterface = authorizationclient.SubjectAccessReviewInterface(nil)
 
+type HostnameGenerator interface {
+	GenerateHostname(*routeapi.Route) (string, error)
+}
+
 type routeStrategy struct {
 	runtime.ObjectTyper
 	names.NameGenerator
-	routeinterfaces.RouteAllocator
-	sarClient SubjectAccessReviewInterface
+	hostnameGenerator HostnameGenerator
+	sarClient         SubjectAccessReviewInterface
 }
 
 // NewStrategy initializes the default logic that applies when creating and updating
 // Route objects via the REST API.
-func NewStrategy(allocator routeinterfaces.RouteAllocator, sarClient SubjectAccessReviewInterface) routeStrategy {
+func NewStrategy(allocator HostnameGenerator, sarClient SubjectAccessReviewInterface) routeStrategy {
 	return routeStrategy{
-		ObjectTyper:    legacyscheme.Scheme,
-		NameGenerator:  names.SimpleNameGenerator,
-		RouteAllocator: allocator,
-		sarClient:      sarClient,
+		ObjectTyper:       legacyscheme.Scheme,
+		NameGenerator:     names.SimpleNameGenerator,
+		hostnameGenerator: allocator,
+		sarClient:         sarClient,
 	}
 }
 
@@ -68,7 +71,7 @@ func (s routeStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Ob
 
 func (s routeStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
 	route := obj.(*routeapi.Route)
-	errs := routehostassignment.AllocateHost(ctx, route, s.sarClient, s.RouteAllocator)
+	errs := routehostassignment.AllocateHost(ctx, route, s.sarClient, s.hostnameGenerator)
 	errs = append(errs, validation.ValidateRoute(route)...)
 	return errs
 }

pkg/route/apiserver/registry/route/strategy_test.go

@@ -18,11 +18,8 @@ import (
 type testAllocator struct {
 }
 
-func (t testAllocator) AllocateRouterShard(*routeapi.Route) (*routeapi.RouterShard, error) {
-	return &routeapi.RouterShard{}, nil
-}
-func (t testAllocator) GenerateHostname(*routeapi.Route, *routeapi.RouterShard) string {
-	return "mygeneratedhost.com"
+func (t testAllocator) GenerateHostname(*routeapi.Route) (string, error) {
+	return "mygeneratedhost.com", nil
 }
 
 type testSAR struct {

pkg/route/apiserver/simplerouteallocation/plugin.go

@@ -35,21 +35,12 @@ func NewSimpleAllocationPlugin(suffix string) (*SimpleAllocationPlugin, error) {
 	return &SimpleAllocationPlugin{DNSSuffix: suffix}, nil
 }
 
-// Allocate a router shard for the given route. This plugin always returns
-// the "global" router shard.
-// TODO: replace with per router allocation
-func (p *SimpleAllocationPlugin) AllocateRouterShard(route *routeapi.Route) (*routeapi.RouterShard, error) {
-	klog.V(4).Infof("Allocating global shard *.%s to Route: %s", p.DNSSuffix, route.Name)
-
-	return &routeapi.RouterShard{ShardName: "global", DNSSuffix: p.DNSSuffix}, nil
-}
-
 // GenerateHostname generates a host name for a route - using the service name,
 // namespace (if provided) and the router shard dns suffix.
 // TODO: move to router code, and have the routers set this back on the route status.
-func (p *SimpleAllocationPlugin) GenerateHostname(route *routeapi.Route, shard *routeapi.RouterShard) string {
+func (p *SimpleAllocationPlugin) GenerateHostname(route *routeapi.Route) (string, error) {
 	if len(route.Name) == 0 || len(route.Namespace) == 0 {
-		return ""
+		return "", nil
 	}
-	return fmt.Sprintf("%s-%s.%s", strings.Replace(route.Name, ".", "-", -1), route.Namespace, shard.DNSSuffix)
+	return fmt.Sprintf("%s-%s.%s", strings.Replace(route.Name, ".", "-", -1), route.Namespace, p.DNSSuffix), nil
 }

pkg/route/apiserver/simplerouteallocation/plugin_test.go

@@ -141,8 +141,7 @@ func TestSimpleAllocationPlugin(t *testing.T) {
 	}
 
 	for _, tc := range tests {
-		shard, _ := plugin.AllocateRouterShard(tc.route)
-		name := plugin.GenerateHostname(tc.route, shard)
+		name, _ := plugin.GenerateHostname(tc.route)
 		switch {
 		case len(name) == 0 && !tc.empty, len(name) != 0 && tc.empty:
 			t.Errorf("Test case %s got %d length name.", tc.name, len(name))
@@ -233,11 +232,10 @@ func TestSimpleAllocationPluginViaController(t *testing.T) {
 	}
 
 	for _, tc := range tests {
-		shard, err := plugin.AllocateRouterShard(tc.route)
+		name, err := plugin.GenerateHostname(tc.route)
 		if err != nil {
 			t.Errorf("Test case %s got an error %s", tc.name, err)
 		}
-		name := plugin.GenerateHostname(tc.route, shard)
 		switch {
 		case len(name) == 0 && !tc.empty, len(name) != 0 && tc.empty:
 			t.Errorf("Test case %s got %d length name.", tc.name, len(name))