allocate route host on update if host is empty in spec

Author: Paul Weil

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

@@ -10,6 +10,7 @@ import (
 	"k8s.io/kubernetes/pkg/runtime"
 	etcdtesting "k8s.io/kubernetes/pkg/storage/etcd/testing"
 
+	routetypes "github.com/openshift/origin/pkg/route"
 	"github.com/openshift/origin/pkg/route/api"
 	_ "github.com/openshift/origin/pkg/route/api/install"
 	"github.com/openshift/origin/pkg/route/registry/route"
@@ -32,7 +33,7 @@ func (a *testAllocator) GenerateHostname(*api.Route, *api.RouterShard) string {
 	return a.Hostname
 }
 
-func newStorage(t *testing.T, allocator *testAllocator) (*REST, *etcdtesting.EtcdTestServer) {
+func newStorage(t *testing.T, allocator routetypes.RouteAllocator) (*REST, *etcdtesting.EtcdTestServer) {
 	etcdStorage, server := registrytest.NewEtcdStorage(t, "")
 	storage, _, err := NewREST(restoptions.NewSimpleGetter(etcdStorage), allocator)
 	if err != nil {
@@ -56,7 +57,7 @@ func validRoute() *api.Route {
 }
 
 func TestCreate(t *testing.T) {
-	storage, server := newStorage(t, &testAllocator{})
+	storage, server := newStorage(t, nil)
 	defer server.Terminate(t)
 	test := registrytest.New(t, storage.Etcd)
 	test.TestCreate(
@@ -114,6 +115,51 @@ func TestUpdate(t *testing.T) {
 		},
 	)
 }
+
+func TestUpdateWithAllocation(t *testing.T) {
+	allocator := &testAllocator{Hostname: "bar"}
+	storage, server := newStorage(t, allocator)
+	defer server.Terminate(t)
+
+	// create a route with a populated host
+	originalRoute := validRoute()
+	originalRoute.Spec.Host = "foo"
+	created, err := storage.Create(kapi.NewDefaultContext(), originalRoute)
+	if err != nil {
+		t.Fatalf("error creating valid route to test allocations: %v", err)
+	}
+
+	createdRoute := created.(*api.Route)
+	if createdRoute.Spec.Host != "foo" {
+		t.Fatalf("unexpected host on createdRoute: %#v", createdRoute)
+	}
+	if _, ok := createdRoute.Annotations[route.HostGeneratedAnnotationKey]; ok {
+		t.Fatalf("created route should not have the generated host annotation")
+	}
+
+	// update the route to set the host to empty
+	createdRoute.Spec.Host = ""
+	updated, _, err := storage.Update(kapi.NewDefaultContext(), createdRoute)
+	if err != nil {
+		t.Fatalf("error updating route to test allocations: %v", err)
+	}
+
+	// route should now have the allocated host of bar and the generated host annotation
+	updatedRoute := updated.(*api.Route)
+	if updatedRoute == nil {
+		t.Fatalf("expected updatedRoute to not be nil")
+	}
+	if updatedRoute.Spec.Host != "bar" {
+		t.Fatalf("unexpected route: %#v", updatedRoute)
+	}
+	if v, ok := updatedRoute.Annotations[route.HostGeneratedAnnotationKey]; !ok || v != "true" {
+		t.Fatalf("unexpected route: %#v", updatedRoute)
+	}
+	if !allocator.Allocate || !allocator.Generate {
+		t.Fatalf("unexpected allocator: %#v", allocator)
+	}
+}
+
 func TestList(t *testing.T) {
 	storage, server := newStorage(t, nil)
 	defer server.Terminate(t)

pkg/route/registry/route/strategy.go

@@ -46,29 +46,46 @@ func (s routeStrategy) PrepareForCreate(obj runtime.Object) {
 	// Limit to kind/name
 	// TODO: convert to LocalObjectReference
 	route.Spec.To = kapi.ObjectReference{Kind: route.Spec.To.Kind, Name: route.Spec.To.Name}
+	err := s.allocateHost(route)
+	if err != nil {
+		// TODO: this will be changed when moved to a controller
+		utilruntime.HandleError(errors.NewInternalError(fmt.Errorf("allocation error: %v for route: %#v", err, obj)))
+	}
+}
+
+func (s routeStrategy) PrepareForUpdate(obj, old runtime.Object) {
+	route := obj.(*api.Route)
+	oldRoute := old.(*api.Route)
+	route.Status = oldRoute.Status
+	// Limit to kind/name
+	// TODO: convert to LocalObjectReference
+	route.Spec.To = kapi.ObjectReference{Kind: route.Spec.To.Kind, Name: route.Spec.To.Name}
+
+	// if the route host has been updated to empty we should allocate the host
+	err := s.allocateHost(route)
+	if err != nil {
+		// TODO: this will be changed when moved to a controller
+		utilruntime.HandleError(errors.NewInternalError(fmt.Errorf("allocation error: %v for route: %#v", err, obj)))
+	}
+}
+
+// allocateHost allocates a host name ONLY if 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 (s routeStrategy) allocateHost(route *api.Route) error {
 	if len(route.Spec.Host) == 0 && s.RouteAllocator != nil {
 		// TODO: this does not belong here, and should be removed
 		shard, err := s.RouteAllocator.AllocateRouterShard(route)
 		if err != nil {
-			// TODO: this will be changed when moved to a controller
-			utilruntime.HandleError(errors.NewInternalError(fmt.Errorf("allocation error: %v for route: %#v", err, obj)))
-			return
+			return errors.NewInternalError(fmt.Errorf("allocation error: %v for route: %#v", err, route))
 		}
 		route.Spec.Host = s.RouteAllocator.GenerateHostname(route, shard)
 		if route.Annotations == nil {
 			route.Annotations = map[string]string{}
 		}
 		route.Annotations[HostGeneratedAnnotationKey] = "true"
 	}
-}
-
-func (routeStrategy) PrepareForUpdate(obj, old runtime.Object) {
-	route := obj.(*api.Route)
-	oldRoute := old.(*api.Route)
-	route.Status = oldRoute.Status
-	// Limit to kind/name
-	// TODO: convert to LocalObjectReference
-	route.Spec.To = kapi.ObjectReference{Kind: route.Spec.To.Kind, Name: route.Spec.To.Name}
+	return nil
 }
 
 func (routeStrategy) Validate(ctx kapi.Context, obj runtime.Object) field.ErrorList {