Prevent route theft by removing the ability to update spec.host

Once created, spec.host is immutable. This preserves the ordering of
route creation which makes route ordering safe.

This rolls back the "on update" allocation of spec.host, since that
complicates allocation.

A route with an empty spec.host may be defaulted by the caller (it means
they don't care) - clients should be looking at route.status anyway.

Author: smarterclayton

pkg/route/api/validation/validation.go

@@ -64,6 +64,7 @@ func ValidateRoute(route *routeapi.Route) field.ErrorList {
 
 func ValidateRouteUpdate(route *routeapi.Route, older *routeapi.Route) field.ErrorList {
 	allErrs := validation.ValidateObjectMetaUpdate(&route.ObjectMeta, &older.ObjectMeta, field.NewPath("metadata"))
+	allErrs = append(allErrs, validation.ValidateImmutableField(route.Spec.Host, older.Spec.Host, field.NewPath("spec", "host"))...)
 	allErrs = append(allErrs, ValidateRoute(route)...)
 	return allErrs
 }

pkg/route/api/validation/validation_test.go

@@ -575,6 +575,85 @@ func TestValidateTLSInsecureEdgeTerminationPolicy(t *testing.T) {
 	}
 }
 
+// TestValidateRouteBad ensures not specifying a required field results in error and a fully specified
+// route passes successfully
+func TestValidateRouteUpdate(t *testing.T) {
+	tests := []struct {
+		name           string
+		route          *api.Route
+		change         func(route *api.Route)
+		expectedErrors int
+	}{
+		{
+			route: &api.Route{
+				ObjectMeta: kapi.ObjectMeta{
+					Name:            "bar",
+					Namespace:       "foo",
+					ResourceVersion: "1",
+				},
+				Spec: api.RouteSpec{
+					Host: "host",
+					To: kapi.ObjectReference{
+						Name: "serviceName",
+						Kind: "Service",
+					},
+				},
+			},
+			change:         func(route *api.Route) { route.Spec.Host = "" },
+			expectedErrors: 1,
+		},
+		{
+			route: &api.Route{
+				ObjectMeta: kapi.ObjectMeta{
+					Name:            "bar",
+					Namespace:       "foo",
+					ResourceVersion: "1",
+				},
+				Spec: api.RouteSpec{
+					Host: "host",
+					To: kapi.ObjectReference{
+						Name: "serviceName",
+						Kind: "Service",
+					},
+				},
+			},
+			change:         func(route *api.Route) { route.Spec.Host = "other" },
+			expectedErrors: 1,
+		},
+		{
+			route: &api.Route{
+				ObjectMeta: kapi.ObjectMeta{
+					Name:            "bar",
+					Namespace:       "foo",
+					ResourceVersion: "1",
+				},
+				Spec: api.RouteSpec{
+					Host: "host",
+					To: kapi.ObjectReference{
+						Name: "serviceName",
+						Kind: "Service",
+					},
+				},
+			},
+			change:         func(route *api.Route) { route.Name = "baz" },
+			expectedErrors: 1,
+		},
+	}
+
+	for i, tc := range tests {
+		copied, err := kapi.Scheme.Copy(tc.route)
+		if err != nil {
+			t.Fatal(err)
+		}
+		newRoute := copied.(*api.Route)
+		tc.change(newRoute)
+		errs := ValidateRouteUpdate(newRoute, tc.route)
+		if len(errs) != tc.expectedErrors {
+			t.Errorf("%d: expected %d error(s), got %d. %v", i, tc.expectedErrors, len(errs), errs)
+		}
+	}
+}
+
 func TestValidateInsecureEdgeTerminationPolicy(t *testing.T) {
 	tests := []struct {
 		name           string

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

@@ -116,50 +116,6 @@ 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

@@ -60,13 +60,6 @@ func (s routeStrategy) PrepareForUpdate(obj, old 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}
-
-	// 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