Changing route and ingress host is allowed if the user has permission

Add "update" to the permissions that a user may have for
"routes/custom-host", and allow the spec.host field to be updated if
this value is set.

This allows administrators to change routes, but by default no one can.
An administrator can delegate this permission as necessary or fix the
route.

Author: smarterclayton

pkg/ingress/admission/ingress_admission.go

@@ -119,6 +119,24 @@ func (r *ingressAdmission) Admit(a admission.Attributes) error {
 					return nil
 				}
 				if !haveHostnamesChanged(oldIngress, newIngress) {
+					attr := authorizer.AttributesRecord{
+						User:            a.GetUserInfo(),
+						Verb:            "update",
+						Namespace:       a.GetNamespace(),
+						Name:            a.GetName(),
+						Resource:        "routes",
+						Subresource:     "custom-host",
+						APIGroup:        "route.openshift.io",
+						ResourceRequest: true,
+					}
+					kind := schema.GroupKind{Group: a.GetResource().Group, Kind: a.GetResource().Resource}
+					allow, _, err := r.authorizer.Authorize(attr)
+					if err != nil {
+						return errors.NewInvalid(kind, newIngress.Name, field.ErrorList{field.InternalError(field.NewPath("spec", "rules"), err)})
+					}
+					if allow {
+						return nil
+					}
 					return fmt.Errorf("cannot change hostname")
 				}
 			}

pkg/ingress/admission/ingress_admission_test.go

@@ -61,6 +61,15 @@ func TestAdmission(t *testing.T) {
 			oldHost:  "bar.com",
 			testName: "changing hostname should fail",
 		},
+		{
+			admit:    true,
+			allow:    true,
+			config:   emptyConfig(),
+			op:       admission.Update,
+			newHost:  "foo.com",
+			oldHost:  "bar.com",
+			testName: "changing hostname should succeed if the user has permission",
+		},
 		{
 			admit:    false,
 			config:   nil,

pkg/route/api/validation/validation.go

@@ -11,7 +11,6 @@ import (
 	kvalidation "k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/kubernetes/pkg/api/validation"
-	kval "k8s.io/kubernetes/pkg/api/validation"
 
 	oapi "github.com/openshift/origin/pkg/api"
 	cmdutil "github.com/openshift/origin/pkg/cmd/util"
@@ -21,7 +20,7 @@ import (
 // ValidateRoute tests if required fields in the route are set.
 func ValidateRoute(route *routeapi.Route) field.ErrorList {
 	//ensure meta is set properly
-	result := kval.ValidateObjectMeta(&route.ObjectMeta, true, oapi.GetNameValidationFunc(kval.ValidatePodName), field.NewPath("metadata"))
+	result := validation.ValidateObjectMeta(&route.ObjectMeta, true, oapi.GetNameValidationFunc(validation.ValidatePodName), field.NewPath("metadata"))
 
 	specPath := field.NewPath("spec")
 
@@ -94,7 +93,6 @@ 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, validation.ValidateImmutableField(route.Spec.WildcardPolicy, older.Spec.WildcardPolicy, field.NewPath("spec", "wildcardPolicy"))...)
 	allErrs = append(allErrs, ValidateRoute(route)...)
 	return allErrs

pkg/route/api/validation/validation_test.go

@@ -1113,7 +1113,7 @@ func TestValidateRouteUpdate(t *testing.T) {
 				},
 			},
 			change:         func(route *api.Route) { route.Spec.Host = "" },
-			expectedErrors: 1,
+			expectedErrors: 0, // now controlled by rbac
 		},
 		{
 			route: &api.Route{
@@ -1131,7 +1131,7 @@ func TestValidateRouteUpdate(t *testing.T) {
 				},
 			},
 			change:         func(route *api.Route) { route.Spec.Host = "other" },
-			expectedErrors: 1,
+			expectedErrors: 0, // now controlled by rbac
 		},
 		{
 			route: &api.Route{

pkg/route/registry/route/strategy.go

@@ -11,6 +11,7 @@ import (
 	"k8s.io/apiserver/pkg/storage"
 	"k8s.io/apiserver/pkg/storage/names"
 	kapi "k8s.io/kubernetes/pkg/api"
+	kvalidation "k8s.io/kubernetes/pkg/api/validation"
 
 	authorizationapi "github.com/openshift/origin/pkg/authorization/api"
 	"github.com/openshift/origin/pkg/route"
@@ -128,10 +129,40 @@ func (routeStrategy) AllowCreateOnUpdate() bool {
 func (routeStrategy) Canonicalize(obj runtime.Object) {
 }
 
-func (routeStrategy) ValidateUpdate(ctx apirequest.Context, obj, old runtime.Object) field.ErrorList {
+func (s routeStrategy) ValidateUpdate(ctx apirequest.Context, obj, old runtime.Object) field.ErrorList {
 	oldRoute := old.(*api.Route)
 	objRoute := obj.(*api.Route)
-	return validation.ValidateRouteUpdate(objRoute, oldRoute)
+	errs := s.validateHostUpdate(ctx, objRoute, oldRoute)
+	errs = append(errs, validation.ValidateRouteUpdate(objRoute, oldRoute)...)
+	return errs
+}
+
+func (s routeStrategy) validateHostUpdate(ctx apirequest.Context, route, older *api.Route) field.ErrorList {
+	if route.Spec.Host == older.Spec.Host {
+		return nil
+	}
+	user, ok := apirequest.UserFrom(ctx)
+	if !ok {
+		return field.ErrorList{field.InternalError(field.NewPath("spec", "host"), fmt.Errorf("unable to verify host field can be changed"))}
+	}
+	res, err := s.sarClient.CreateSubjectAccessReview(
+		ctx,
+		&authorizationapi.SubjectAccessReview{
+			User: user.GetName(),
+			Action: authorizationapi.Action{
+				Verb:     "update",
+				Group:    "route.openshift.io",
+				Resource: "routes/custom-host",
+			},
+		},
+	)
+	if err != nil {
+		return field.ErrorList{field.InternalError(field.NewPath("spec", "host"), err)}
+	}
+	if !res.Allowed {
+		return kvalidation.ValidateImmutableField(route.Spec.Host, older.Spec.Host, field.NewPath("spec", "host"))
+	}
+	return nil
 }
 
 func (routeStrategy) AllowUnconditionalUpdate() bool {

pkg/route/registry/route/strategy_test.go

@@ -5,6 +5,7 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apiserver/pkg/authentication/user"
 	apirequest "k8s.io/apiserver/pkg/endpoints/request"
 	kapi "k8s.io/kubernetes/pkg/api"
@@ -71,6 +72,7 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 	tests := []struct {
 		name           string
 		host           string
+		oldHost        string
 		wildcardPolicy api.WildcardPolicyType
 		expected       string
 		errs           int
@@ -128,9 +130,30 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: api.WildcardPolicyNone,
 			allow:          false,
 		},
+		{
+			name:           "update-changed-host-denied",
+			host:           "new.host",
+			expected:       "new.host",
+			oldHost:        "original.host",
+			wildcardPolicy: api.WildcardPolicyNone,
+			allow:          false,
+			errs:           1,
+		},
+		{
+			name:           "update-changed-host-allowed",
+			host:           "new.host",
+			expected:       "new.host",
+			oldHost:        "original.host",
+			wildcardPolicy: api.WildcardPolicyNone,
+			allow:          true,
+			errs:           0,
+		},
 	}
 
 	for _, tc := range tests {
+		sar := &testSAR{allow: tc.allow}
+		strategy := NewStrategy(testAllocator{}, sar)
+
 		route := &api.Route{
 			ObjectMeta: metav1.ObjectMeta{
 				Namespace:       "wildcard",
@@ -147,10 +170,30 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 				},
 			},
 		}
-		sar := &testSAR{allow: tc.allow}
-		strategy := NewStrategy(testAllocator{}, sar)
 
-		errs := strategy.Validate(ctx, route)
+		var errs field.ErrorList
+		if len(tc.oldHost) > 0 {
+			oldRoute := &api.Route{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace:       "wildcard",
+					Name:            tc.name,
+					UID:             types.UID("wild"),
+					ResourceVersion: "1",
+				},
+				Spec: api.RouteSpec{
+					Host:           tc.oldHost,
+					WildcardPolicy: tc.wildcardPolicy,
+					To: api.RouteTargetReference{
+						Name: "test",
+						Kind: "Service",
+					},
+				},
+			}
+			errs = strategy.ValidateUpdate(ctx, route, oldRoute)
+		} else {
+			errs = strategy.Validate(ctx, route)
+		}
+
 		if route.Spec.Host != tc.expected {
 			t.Fatalf("test case %s expected host %s, got %s", tc.name, tc.expected, route.Spec.Host)
 		}