Made address Ben's inital comments

Author: JacobTanenbaum

pkg/cmd/server/origin/master_config.go

@@ -69,7 +69,7 @@ import (
 	imageadmission "github.com/openshift/origin/pkg/image/admission"
 	imagepolicy "github.com/openshift/origin/pkg/image/admission/imagepolicy/api"
 	imageapi "github.com/openshift/origin/pkg/image/api"
-	ingressadmit "github.com/openshift/origin/pkg/ingress/admission"
+	ingressadmission "github.com/openshift/origin/pkg/ingress/admission"
 	accesstokenregistry "github.com/openshift/origin/pkg/oauth/registry/oauthaccesstoken"
 	accesstokenetcd "github.com/openshift/origin/pkg/oauth/registry/oauthaccesstoken/etcd"
 	projectauth "github.com/openshift/origin/pkg/project/auth"
@@ -362,7 +362,7 @@ var (
 		"SCCExecRestrictions",
 		"PersistentVolumeLabel",
 		"OwnerReferencesPermissionEnforcement",
-		ingressadmit.IngressAdmission,
+		ingressadmission.IngressAdmission,
 		// NOTE: quotaadmission and ClusterResourceQuota must be the last 2 plugins.
 		// DO NOT ADD ANY PLUGINS AFTER THIS LINE!
 		quotaadmission.PluginName,
@@ -400,7 +400,7 @@ var (
 		"SCCExecRestrictions",
 		"PersistentVolumeLabel",
 		"OwnerReferencesPermissionEnforcement",
-		ingressadmit.IngressAdmission,
+		ingressadmission.IngressAdmission,
 		// NOTE: quotaadmission and ClusterResourceQuota must be the last 2 plugins.
 		// DO NOT ADD ANY PLUGINS AFTER THIS LINE!
 		quotaadmission.PluginName,

pkg/ingress/admission/api/types.go

@@ -6,14 +6,15 @@ import (
 
 // IngressAdmissionConfig is the configuration for the the ingress
 // controller limiter plugin. It changes the behavior of ingress
-//objects to behave better with openshift routes and routers.
-//*NOTE* Disabling this plugin causes ingress objects to behave
-//the same as in upstream kubernetes
+// objects to behave better with openshift routes and routers.
+// *NOTE* This has security implications in the router when handling
+// ingress objects
 type IngressAdmissionConfig struct {
 	unversioned.TypeMeta
 
-	//UpstreamHostnameUpdate when true causes updates that attempt
-	//to add or modify hostnames to succeed. Otherwise those updates
-	//fail in order to ensure hostname behavior
-	UpstreamHostnameUpdate bool
+	// AllowHostnameChanges when false or unset openshift does not
+	// allow changing or adding hostnames to ingress objects. If set
+	// to true then hostnames can be added or modified which has
+	// security implications in the router.
+	AllowHostnameChanges bool
 }

pkg/ingress/admission/api/v1/defaults_test.go

@@ -42,7 +42,7 @@ func TestDefaults(t *testing.T) {
 		{
 			original: &ingressv1.IngressAdmissionConfig{},
 			expected: &ingressv1.IngressAdmissionConfig{
-				UpstreamHostnameUpdate: false,
+				AllowHostnameChanges: false,
 			},
 		},
 	}

pkg/ingress/admission/api/v1/register.go

@@ -5,7 +5,7 @@ import (
 	"k8s.io/kubernetes/pkg/runtime"
 )
 
-//SchemeGroupVersion is gorup version used to register these objects
+// SchemeGroupVersion is group version used to register these objects
 var SchemeGroupVersion = unversioned.GroupVersion{Group: "", Version: "v1"}
 
 var (

pkg/ingress/admission/api/v1/swagger_doc.go

@@ -6,8 +6,8 @@ package v1
 // ==== DO NOT EDIT THIS FILE MANUALLY ====
 
 var map_IngressAdmissionConfig = map[string]string{
-	"": "IngressAdmissionConfig is the configuration for the the ingress controller limiter plugin. It changes the behavior of ingress objects to behave better with openshift routes and routers. *NOTE* Disabling this plugin causes ingress objects to behave the same as in upstream kubernetes",
-	"upstreamHostnameUpdate": "UpstreamHostnameUpdate when true causes updates that attempt to add or modify hostnames to succeed. Otherwise those updates fail in order to ensure hostname behavior",
+	"": "IngressAdmissionConfig is the configuration for the the ingress controller limiter plugin. It changes the behavior of ingress objects to behave better with openshift routes and routers. *NOTE* This has security implications in the router when handling ingress objects",
+	"allowHostnameChanges": "AllowHostnameChanges when false or unset openshift does not allow changing or adding hostnames to ingress objects. If set to true then hostnames can be added or modified which has security implications in the router.",
 }
 
 func (IngressAdmissionConfig) SwaggerDoc() map[string]string {

pkg/ingress/admission/api/v1/types.go

@@ -6,14 +6,15 @@ import (
 
 // IngressAdmissionConfig is the configuration for the the ingress
 // controller limiter plugin. It changes the behavior of ingress
-//objects to behave better with openshift routes and routers.
-//*NOTE* Disabling this plugin causes ingress objects to behave
-//the same as in upstream kubernetes
+// objects to behave better with openshift routes and routers.
+// *NOTE* This has security implications in the router when handling
+// ingress objects
 type IngressAdmissionConfig struct {
 	unversioned.TypeMeta `json:",inline"`
 
-	//UpstreamHostnameUpdate when true causes updates that attempt
-	//to add or modify hostnames to succeed. Otherwise those updates
-	//fail in order to ensure hostname behavior
-	UpstreamHostnameUpdate bool `json:"upstreamHostnameUpdate"`
+	// AllowHostnameChanges when false or unset openshift does not
+	// allow changing or adding hostnames to ingress objects. If set
+	// to true then hostnames can be added or modified which has
+	// security implications in the router.
+	AllowHostnameChanges bool `json:"allowHostnameChanges"`
 }

pkg/ingress/admission/ingress_admission.go

@@ -10,6 +10,7 @@ import (
 	kadmission "k8s.io/kubernetes/pkg/admission"
 	kextensions "k8s.io/kubernetes/pkg/apis/extensions"
 	clientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
+	"k8s.io/kubernetes/pkg/util/sets"
 
 	configlatest "github.com/openshift/origin/pkg/cmd/server/api/latest"
 	"github.com/openshift/origin/pkg/ingress/admission/api"
@@ -62,7 +63,7 @@ func readConfig(reader io.Reader) (*api.IngressAdmissionConfig, error) {
 
 func (r *ingressAdmission) Admit(a kadmission.Attributes) error {
 	if a.GetResource().GroupResource() == kextensions.Resource("ingresses") && a.GetOperation() == kadmission.Update {
-		if r.config == nil || r.config.UpstreamHostnameUpdate == false {
+		if r.config == nil || r.config.AllowHostnameChanges == false {
 			oldIngress, ok := a.GetOldObject().(*kextensions.Ingress)
 			if !ok {
 				return nil
@@ -71,23 +72,22 @@ func (r *ingressAdmission) Admit(a kadmission.Attributes) error {
 			if !ok {
 				return nil
 			}
-			if !checkHostnames(oldIngress, newIngress) {
+			if !haveHostnamesChanged(oldIngress, newIngress) {
 				return fmt.Errorf("cannot change hostname")
 			}
 		}
 	}
 	return nil
 }
 
-func checkHostnames(oldIngress, newIngress *kextensions.Ingress) bool {
+func haveHostnamesChanged(oldIngress, newIngress *kextensions.Ingress) bool {
 	m := make(map[string]int)
 	for _, element := range oldIngress.Spec.Rules {
 		m[element.Host] = 1
 	}
 
 	for _, element := range newIngress.Spec.Rules {
-		_, present := m[element.Host]
-		if !present {
+		if _, present := m[element.Host]; !present {
 			return false
 		}
 	}

pkg/ingress/admission/ingress_admission_test.go

@@ -81,7 +81,7 @@ func TestAdmission(t *testing.T) {
 				},
 			}
 		} else {
-			//Used to test deleteing a hostname
+			//Used to test deleting a hostname
 			newIngress = &kextensions.Ingress{
 				ObjectMeta: kapi.ObjectMeta{Name: "test"},
 			}
@@ -120,12 +120,12 @@ func emptyConfig() *api.IngressAdmissionConfig {
 
 func testConfigUpdateAllow() *api.IngressAdmissionConfig {
 	return &api.IngressAdmissionConfig{
-		UpstreamHostnameUpdate: true,
+		AllowHostnameChanges: true,
 	}
 }
 
 func testConfigUpdateDeny() *api.IngressAdmissionConfig {
 	return &api.IngressAdmissionConfig{
-		UpstreamHostnameUpdate: false,
+		AllowHostnameChanges: false,
 	}
 }