Merge pull request #17420 from smarterclayton/router_consistency

Automatic merge from submit-queue.

Make updating status on the router optional

User can specify --update-status=false on the router to have a router that doesn't write back to routes. The default is true for backwards compatibilty. Allows testing routers when you don't have route status/update.

Also unify a number of host setup paths. Makes F5 and template more consistent.

Question for reviewer - on the F5 plugin we weren't performing a number of "standard" operations like setting the canonical hostname. Was there a concrete reason for that?

@openshift/sig-networking

Author: openshift-merge-robot

pkg/cmd/infra/router/f5.go

@@ -18,6 +18,7 @@ import (
 	projectinternalclientset "github.com/openshift/origin/pkg/project/generated/internalclientset"
 	routeapi "github.com/openshift/origin/pkg/route/apis/route"
 	routeinternalclientset "github.com/openshift/origin/pkg/route/generated/internalclientset"
+	"github.com/openshift/origin/pkg/router"
 	"github.com/openshift/origin/pkg/router/controller"
 	f5plugin "github.com/openshift/origin/pkg/router/f5"
 	"github.com/openshift/origin/pkg/version"
@@ -46,8 +47,6 @@ type F5RouterOptions struct {
 
 // F5Router is the config necessary to start an F5 router plugin.
 type F5Router struct {
-	RouterName string
-
 	// Host specifies the hostname or IP address of the F5 BIG-IP host.
 	Host string
 
@@ -95,7 +94,6 @@ type F5Router struct {
 
 // Bind binds F5Router arguments to flags
 func (o *F5Router) Bind(flag *pflag.FlagSet) {
-	flag.StringVar(&o.RouterName, "name", util.Env("ROUTER_SERVICE_NAME", "public"), "The name the router will identify itself with in the route status")
 	flag.StringVar(&o.Host, "f5-host", util.Env("ROUTER_EXTERNAL_HOST_HOSTNAME", ""), "The host of F5 BIG-IP's management interface")
 	flag.StringVar(&o.Username, "f5-username", util.Env("ROUTER_EXTERNAL_HOST_USERNAME", ""), "The username for F5 BIG-IP's management utility")
 	flag.StringVar(&o.Password, "f5-password", util.Env("ROUTER_EXTERNAL_HOST_PASSWORD", ""), "The password for F5 BIG-IP's management utility")
@@ -230,9 +228,18 @@ func (o *F5RouterOptions) Run() error {
 		return err
 	}
 
-	statusPlugin := controller.NewStatusAdmitter(f5Plugin, routeclient.Route(), o.RouterName, "")
-	uniqueHostPlugin := controller.NewUniqueHost(statusPlugin, o.RouteSelectionFunc(), o.RouterSelection.DisableNamespaceOwnershipCheck, statusPlugin)
-	plugin := controller.NewHostAdmitter(uniqueHostPlugin, o.F5RouteAdmitterFunc(), false, o.RouterSelection.DisableNamespaceOwnershipCheck, statusPlugin)
+	var recorder controller.RejectionRecorder = controller.LogRejections
+	var plugin router.Plugin = f5Plugin
+	if o.UpdateStatus {
+		status := controller.NewStatusAdmitter(plugin, routeclient.Route(), o.RouterName, o.RouterCanonicalHostname)
+		recorder = status
+		plugin = status
+	}
+	if o.ExtendedValidation {
+		plugin = controller.NewExtendedValidator(plugin, recorder)
+	}
+	plugin = controller.NewUniqueHost(plugin, o.RouteSelectionFunc(), o.RouterSelection.DisableNamespaceOwnershipCheck, recorder)
+	plugin = controller.NewHostAdmitter(plugin, o.F5RouteAdmitterFunc(), o.AllowWildcardRoutes, o.RouterSelection.DisableNamespaceOwnershipCheck, recorder)
 
 	factory := o.RouterSelection.NewFactory(routeclient, projectclient.Project().Projects(), kc)
 	watchNodes := (len(o.InternalAddress) != 0 && len(o.VxlanGateway) != 0)

pkg/cmd/infra/router/router.go

@@ -11,6 +11,7 @@ import (
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/validation"
 	kclientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
 
 	cmdutil "github.com/openshift/origin/pkg/cmd/util"
@@ -25,8 +26,13 @@ import (
 // RouterSelection controls what routes and resources on the server are considered
 // part of this router.
 type RouterSelection struct {
+	RouterName              string
+	RouterCanonicalHostname string
+
 	ResyncInterval time.Duration
 
+	UpdateStatus bool
+
 	HostnameTemplate string
 	OverrideHostname bool
 
@@ -52,13 +58,18 @@ type RouterSelection struct {
 
 	DisableNamespaceOwnershipCheck bool
 
+	ExtendedValidation bool
+
 	EnableIngress bool
 
 	ListenAddr string
 }
 
 // Bind sets the appropriate labels
 func (o *RouterSelection) Bind(flag *pflag.FlagSet) {
+	flag.StringVar(&o.RouterName, "name", cmdutil.Env("ROUTER_SERVICE_NAME", "public"), "The name the router will identify itself with in the route status")
+	flag.StringVar(&o.RouterCanonicalHostname, "router-canonical-hostname", cmdutil.Env("ROUTER_CANONICAL_HOSTNAME", ""), "CanonicalHostname is the external host name for the router that can be used as a CNAME for the host requested for this route. This value is optional and may not be set in all cases.")
+	flag.BoolVar(&o.UpdateStatus, "update-status", isTrue(cmdutil.Env("ROUTER_UPDATE_STATUS", "true")), "If true, the router will update admitted route status.")
 	flag.DurationVar(&o.ResyncInterval, "resync-interval", controllerfactory.DefaultResyncInterval, "The interval at which the route list should be fully refreshed")
 	flag.StringVar(&o.HostnameTemplate, "hostname-template", cmdutil.Env("ROUTER_SUBDOMAIN", ""), "If specified, a template that should be used to generate the hostname for a route without spec.host (e.g. '${name}-${namespace}.myapps.mycompany.com')")
 	flag.BoolVar(&o.OverrideHostname, "override-hostname", isTrue(cmdutil.Env("ROUTER_OVERRIDE_HOSTNAME", "")), "Override the spec.host value for a route with --hostname-template")
@@ -72,6 +83,7 @@ func (o *RouterSelection) Bind(flag *pflag.FlagSet) {
 	flag.BoolVar(&o.AllowWildcardRoutes, "allow-wildcard-routes", isTrue(cmdutil.Env("ROUTER_ALLOW_WILDCARD_ROUTES", "")), "Allow wildcard host names for routes")
 	flag.BoolVar(&o.DisableNamespaceOwnershipCheck, "disable-namespace-ownership-check", isTrue(cmdutil.Env("ROUTER_DISABLE_NAMESPACE_OWNERSHIP_CHECK", "")), "Disables the namespace ownership checks for a route host with different paths or for overlapping host names in the case of wildcard routes. Please be aware that if namespace ownership checks are disabled, routes in a different namespace can use this mechanism to 'steal' sub-paths for existing domains. This is only safe if route creation privileges are restricted, or if all the users can be trusted.")
 	flag.BoolVar(&o.EnableIngress, "enable-ingress", isTrue(cmdutil.Env("ROUTER_ENABLE_INGRESS", "")), "Enable configuration via ingress resources")
+	flag.BoolVar(&o.ExtendedValidation, "extended-validation", isTrue(cmdutil.Env("EXTENDED_VALIDATION", "true")), "If set, then an additional extended validation step is performed on all routes admitted in by this router. Defaults to true and enables the extended validation checks.")
 	flag.StringVar(&o.ListenAddr, "listen-addr", cmdutil.Env("ROUTER_LISTEN_ADDR", ""), "The name of an interface to listen on to expose metrics and health checking. If not specified, will not listen. Overrides stats port.")
 }
 
@@ -205,6 +217,15 @@ func (o *RouterSelection) Complete() error {
 	o.BlacklistedDomains = sets.NewString(o.DeniedDomains...)
 	o.WhitelistedDomains = sets.NewString(o.AllowedDomains...)
 
+	if routerCanonicalHostname := o.RouterCanonicalHostname; len(routerCanonicalHostname) > 0 {
+		if errs := validation.IsDNS1123Subdomain(routerCanonicalHostname); len(errs) != 0 {
+			return fmt.Errorf("invalid canonical hostname: %s", routerCanonicalHostname)
+		}
+		if errs := validation.IsValidIP(routerCanonicalHostname); len(errs) == 0 {
+			return fmt.Errorf("canonical hostname must not be an IP address: %s", routerCanonicalHostname)
+		}
+	}
+
 	return nil
 }
 

pkg/cmd/infra/router/template.go

@@ -17,7 +17,6 @@ import (
 
 	ktypes "k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
-	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apiserver/pkg/authentication/authenticatorfactory"
 	"k8s.io/apiserver/pkg/authorization/authorizer"
 	"k8s.io/apiserver/pkg/authorization/authorizerfactory"
@@ -70,8 +69,6 @@ type TemplateRouterOptions struct {
 }
 
 type TemplateRouter struct {
-	RouterName               string
-	RouterCanonicalHostname  string
 	WorkingDir               string
 	TemplateFile             string
 	ReloadScript             string
@@ -80,7 +77,6 @@ type TemplateRouter struct {
 	DefaultCertificatePath   string
 	DefaultCertificateDir    string
 	DefaultDestinationCAPath string
-	ExtendedValidation       bool
 	RouterService            *ktypes.NamespacedName
 	BindPortsAfterSync       bool
 	MaxConnections           string
@@ -108,8 +104,6 @@ func reloadInterval() time.Duration {
 }
 
 func (o *TemplateRouter) Bind(flag *pflag.FlagSet) {
-	flag.StringVar(&o.RouterName, "name", util.Env("ROUTER_SERVICE_NAME", "public"), "The name the router will identify itself with in the route status")
-	flag.StringVar(&o.RouterCanonicalHostname, "router-canonical-hostname", util.Env("ROUTER_CANONICAL_HOSTNAME", ""), "CanonicalHostname is the external host name for the router that can be used as a CNAME for the host requested for this route. This value is optional and may not be set in all cases.")
 	flag.StringVar(&o.WorkingDir, "working-dir", "/var/lib/haproxy/router", "The working directory for the router plugin")
 	flag.StringVar(&o.DefaultCertificate, "default-certificate", util.Env("DEFAULT_CERTIFICATE", ""), "The contents of a default certificate to use for routes that don't expose a TLS server cert; in PEM format")
 	flag.StringVar(&o.DefaultCertificatePath, "default-certificate-path", util.Env("DEFAULT_CERTIFICATE_PATH", ""), "A path to default certificate to use for routes that don't expose a TLS server cert; in PEM format")
@@ -118,8 +112,7 @@ func (o *TemplateRouter) Bind(flag *pflag.FlagSet) {
 	flag.StringVar(&o.TemplateFile, "template", util.Env("TEMPLATE_FILE", ""), "The path to the template file to use")
 	flag.StringVar(&o.ReloadScript, "reload", util.Env("RELOAD_SCRIPT", ""), "The path to the reload script to use")
 	flag.DurationVar(&o.ReloadInterval, "interval", reloadInterval(), "Controls how often router reloads are invoked. Mutiple router reload requests are coalesced for the duration of this interval since the last reload time.")
-	flag.BoolVar(&o.ExtendedValidation, "extended-validation", isTrue(util.Env("EXTENDED_VALIDATION", "true")), "If set, then an additional extended validation step is performed on all routes admitted in by this router. Defaults to true and enables the extended validation checks.")
-	flag.BoolVar(&o.BindPortsAfterSync, "bind-ports-after-sync", isTrue(util.Env("ROUTER_BIND_PORTS_AFTER_SYNC", "")), "Bind ports only after route state has been synchronized")
+	flag.BoolVar(&o.BindPortsAfterSync, "bind-ports-after-sync", util.Env("ROUTER_BIND_PORTS_AFTER_SYNC", "") == "true", "Bind ports only after route state has been synchronized")
 	flag.StringVar(&o.MaxConnections, "max-connections", util.Env("ROUTER_MAX_CONNECTIONS", ""), "Specifies the maximum number of concurrent connections.")
 	flag.StringVar(&o.Ciphers, "ciphers", util.Env("ROUTER_CIPHERS", ""), "Specifies the cipher suites to use. You can choose a predefined cipher set ('modern', 'intermediate', or 'old') or specify exact cipher suites by passing a : separated list.")
 	flag.BoolVar(&o.StrictSNI, "strict-sni", isTrue(util.Env("ROUTER_STRICT_SNI", "")), "Use strict-sni bind processing (do not use default cert).")
@@ -180,7 +173,6 @@ func NewCommandTemplateRouter(name string) *cobra.Command {
 func (o *TemplateRouterOptions) Complete() error {
 	routerSvcName := util.Env("ROUTER_SERVICE_NAME", "")
 	routerSvcNamespace := util.Env("ROUTER_SERVICE_NAMESPACE", "")
-	routerCanonicalHostname := util.Env("ROUTER_CANONICAL_HOSTNAME", "")
 	if len(routerSvcName) > 0 {
 		if len(routerSvcNamespace) == 0 {
 			return fmt.Errorf("ROUTER_SERVICE_NAMESPACE is required when ROUTER_SERVICE_NAME is specified")
@@ -219,15 +211,6 @@ func (o *TemplateRouterOptions) Complete() error {
 		return fmt.Errorf("invalid reload interval: %v - must be a positive duration", nsecs)
 	}
 
-	if len(routerCanonicalHostname) > 0 {
-		if errs := validation.IsDNS1123Subdomain(routerCanonicalHostname); len(errs) != 0 {
-			return fmt.Errorf("invalid canonical hostname: %s", routerCanonicalHostname)
-		}
-		if errs := validation.IsValidIP(routerCanonicalHostname); len(errs) == 0 {
-			return fmt.Errorf("canonical hostname must not be an IP address: %s", routerCanonicalHostname)
-		}
-	}
-
 	return o.RouterSelection.Complete()
 }
 
@@ -238,7 +221,7 @@ func (o *TemplateRouterOptions) Validate() error {
 	if len(o.MetricsType) > 0 && !supportedMetricsTypes.Has(o.MetricsType) {
 		return fmt.Errorf("supported metrics types are: %s", strings.Join(supportedMetricsTypes.List(), ", "))
 	}
-	if len(o.RouterName) == 0 {
+	if len(o.RouterName) == 0 && o.UpdateStatus {
 		return errors.New("router must have a name to identify itself in route status")
 	}
 	if len(o.TemplateFile) == 0 {
@@ -421,13 +404,18 @@ func (o *TemplateRouterOptions) Run() error {
 		return err
 	}
 
-	statusPlugin := controller.NewStatusAdmitter(templatePlugin, routeclient.Route(), o.RouterName, o.RouterCanonicalHostname)
-	var nextPlugin router.Plugin = statusPlugin
+	var recorder controller.RejectionRecorder = controller.LogRejections
+	var plugin router.Plugin = templatePlugin
+	if o.UpdateStatus {
+		status := controller.NewStatusAdmitter(plugin, routeclient.Route(), o.RouterName, o.RouterCanonicalHostname)
+		recorder = status
+		plugin = status
+	}
 	if o.ExtendedValidation {
-		nextPlugin = controller.NewExtendedValidator(nextPlugin, controller.RejectionRecorder(statusPlugin))
+		plugin = controller.NewExtendedValidator(plugin, recorder)
 	}
-	uniqueHostPlugin := controller.NewUniqueHost(nextPlugin, o.RouteSelectionFunc(), o.RouterSelection.DisableNamespaceOwnershipCheck, controller.RejectionRecorder(statusPlugin))
-	plugin := controller.NewHostAdmitter(uniqueHostPlugin, o.RouteAdmissionFunc(), o.AllowWildcardRoutes, o.RouterSelection.DisableNamespaceOwnershipCheck, controller.RejectionRecorder(statusPlugin))
+	plugin = controller.NewUniqueHost(plugin, o.RouteSelectionFunc(), o.RouterSelection.DisableNamespaceOwnershipCheck, recorder)
+	plugin = controller.NewHostAdmitter(plugin, o.RouteAdmissionFunc(), o.AllowWildcardRoutes, o.RouterSelection.DisableNamespaceOwnershipCheck, recorder)
 
 	factory := o.RouterSelection.NewFactory(routeclient, projectclient.Project().Projects(), kc)
 	controller := factory.Create(plugin, false, o.EnableIngress)

pkg/router/controller/status.go

@@ -24,6 +24,15 @@ type RejectionRecorder interface {
 	RecordRouteRejection(route *routeapi.Route, reason, message string)
 }
 
+// LogRejections writes rejection messages to the log.
+var LogRejections = logRecorder{}
+
+type logRecorder struct{}
+
+func (logRecorder) RecordRouteRejection(route *routeapi.Route, reason, message string) {
+	glog.V(3).Infof("Rejected route %s in namespace %s: %s: %s", route.Name, route.Namespace, reason, message)
+}
+
 // StatusAdmitter ensures routes added to the plugin have status set.
 type StatusAdmitter struct {
 	plugin                  router.Plugin

pkg/router/controller/unique_host.go

@@ -25,14 +25,6 @@ func HostForRoute(route *routeapi.Route) string {
 type HostToRouteMap map[string][]*routeapi.Route
 type RouteToHostMap map[string]string
 
-var LogRejections = logRecorder{}
-
-type logRecorder struct{}
-
-func (logRecorder) RecordRouteRejection(route *routeapi.Route, reason, message string) {
-	glog.V(4).Infof("Rejected route %s: %s: %s", route.Name, reason, message)
-}
-
 // UniqueHost implements the router.Plugin interface to provide
 // a template based, backend-agnostic router.
 type UniqueHost struct {

test/extended/router/headers.go

@@ -18,7 +18,7 @@ import (
 	exutil "github.com/openshift/origin/test/extended/util"
 )
 
-var _ = g.Describe("[Conformance][Area:Networking][Feature:Router] router headers", func() {
+var _ = g.Describe("[Conformance][Area:Networking][Feature:Router]", func() {
 	defer g.GinkgoRecover()
 	var (
 		configPath = exutil.FixturePath("testdata", "router-http-echo-server.yaml")

test/extended/router/metrics.go

@@ -23,7 +23,7 @@ import (
 	exutil "github.com/openshift/origin/test/extended/util"
 )
 
-var _ = g.Describe("[Conformance][Area:Networking][Feature:Router] openshift router metrics", func() {
+var _ = g.Describe("[Conformance][Area:Networking][Feature:Router]", func() {
 	defer g.GinkgoRecover()
 	var (
 		oc = exutil.NewCLI("router-metrics", exutil.KubeConfigPath())

test/extended/router/scoped.go

@@ -11,16 +11,18 @@ import (
 	g "github.com/onsi/ginkgo"
 	o "github.com/onsi/gomega"
 
+	routeapi "github.com/openshift/origin/pkg/route/apis/route"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
+	kapi "k8s.io/kubernetes/pkg/apis/core"
 	e2e "k8s.io/kubernetes/test/e2e/framework"
 
 	exutil "github.com/openshift/origin/test/extended/util"
 )
 
 const changeTimeoutSeconds = 3 * 60
 
-var _ = g.Describe("[Conformance][Area:Networking][Feature:Router] openshift routers", func() {
+var _ = g.Describe("[Conformance][Area:Networking][Feature:Router]", func() {
 	defer g.GinkgoRecover()
 	var (
 		configPath = exutil.FixturePath("testdata", "scoped-router.yaml")
@@ -32,19 +34,16 @@ var _ = g.Describe("[Conformance][Area:Networking][Feature:Router] openshift rou
 		if len(imagePrefix) == 0 {
 			imagePrefix = "openshift/origin"
 		}
-		err := oc.AsAdmin().Run("adm").Args("policy", "add-cluster-role-to-user", "system:router", oc.Username()).Execute()
-		o.Expect(err).NotTo(o.HaveOccurred())
-		err = oc.Run("new-app").Args("-f", configPath, "-p", "IMAGE="+imagePrefix+"-haproxy-router").Execute()
+		err := oc.AsAdmin().Run("new-app").Args("-f", configPath, "-p", "IMAGE="+imagePrefix+"-haproxy-router").Execute()
 		o.Expect(err).NotTo(o.HaveOccurred())
 	})
 
 	g.Describe("The HAProxy router", func() {
 		g.It("should serve the correct routes when scoped to a single namespace and label set", func() {
 			defer func() {
-				// This should be done if the test fails but
-				// for now always dump the logs.
-				// if g.CurrentGinkgoTestDescription().Failed
-				dumpScopedRouterLogs(oc, g.CurrentGinkgoTestDescription().FullTestText)
+				if g.CurrentGinkgoTestDescription().Failed {
+					dumpScopedRouterLogs(oc, g.CurrentGinkgoTestDescription().FullTestText)
+				}
 			}()
 			oc.SetOutputDir(exutil.TestContext.OutputDir)
 			ns := oc.KubeFramework().Namespace.Name
@@ -91,10 +90,9 @@ var _ = g.Describe("[Conformance][Area:Networking][Feature:Router] openshift rou
 
 		g.It("should override the route host with a custom value", func() {
 			defer func() {
-				// This should be done if the test fails but
-				// for now always dump the logs.
-				// if g.CurrentGinkgoTestDescription().Failed
-				dumpScopedRouterLogs(oc, g.CurrentGinkgoTestDescription().FullTestText)
+				if g.CurrentGinkgoTestDescription().Failed {
+					dumpScopedRouterLogs(oc, g.CurrentGinkgoTestDescription().FullTestText)
+				}
 			}()
 			oc.SetOutputDir(exutil.TestContext.OutputDir)
 			ns := oc.KubeFramework().Namespace.Name
@@ -142,6 +140,16 @@ var _ = g.Describe("[Conformance][Area:Networking][Feature:Router] openshift rou
 				err = expectRouteStatusCodeExec(ns, execPodName, routerURL+"/Letter", host, http.StatusOK)
 				o.Expect(err).NotTo(o.HaveOccurred())
 			}
+
+			g.By("checking that the router reported the correct ingress and override")
+			r, err := oc.RouteClient().Route().Routes(ns).Get("route-1", metav1.GetOptions{})
+			o.Expect(err).NotTo(o.HaveOccurred())
+			ingress := ingressForName(r, "test-override")
+			o.Expect(ingress).NotTo(o.BeNil())
+			o.Expect(ingress.Host).To(o.Equal(fmt.Sprintf(pattern, "route-1", ns)))
+			status, condition := routeapi.IngressConditionStatus(ingress, routeapi.RouteAdmitted)
+			o.Expect(status).To(o.Equal(kapi.ConditionTrue))
+			o.Expect(condition.LastTransitionTime).NotTo(o.BeNil())
 		})
 	})
 })
@@ -223,3 +231,12 @@ func dumpScopedRouterLogs(oc *exutil.CLI, name string) {
 	log, _ := e2e.GetPodLogs(oc.AdminKubeClient(), oc.KubeFramework().Namespace.Name, "scoped-router", "router")
 	e2e.Logf("Scoped Router test %s logs:\n %s", name, log)
 }
+
+func ingressForName(r *routeapi.Route, name string) *routeapi.RouteIngress {
+	for i, ingress := range r.Status.Ingress {
+		if ingress.RouterName == name {
+			return &r.Status.Ingress[i]
+		}
+	}
+	return nil
+}