Add router option to bind ports only when ready

By default the router binds ports 80 and 443 even if no routing
configuration is available.  This may be desirable when haproxy is
serving traffic directly to clients.  However, the f5 loadbalancer will
treat bound ports as indicating that a backend is ready to receive
traffic.  This means that router pods that are starting or restarting
may be put into rotation before they are ready to serve the current
route state and clients may see 503s for perfectly healthy backend
endpoints.  To avoid this possibility, this change adds an
option (BIND_ONLY_WHEN_READY/--bind-only-when-ready) to the router that
ensures that ports are bound only when a router instance has route and
endpoint state available.

Reference: bz1383663

Author: marun

contrib/completions/bash/openshift

@@ -20096,6 +20096,8 @@ _openshift_infra_router()
     local_nonpersistent_flags+=("--allowed-domains=")
     flags+=("--as=")
     local_nonpersistent_flags+=("--as=")
+    flags+=("--bind-only-when-ready")
+    local_nonpersistent_flags+=("--bind-only-when-ready")
     flags+=("--certificate-authority=")
     flags_with_completion+=("--certificate-authority")
     flags_completion+=("_filedir")

contrib/completions/zsh/openshift

@@ -20257,6 +20257,8 @@ _openshift_infra_router()
     local_nonpersistent_flags+=("--allowed-domains=")
     flags+=("--as=")
     local_nonpersistent_flags+=("--as=")
+    flags+=("--bind-only-when-ready")
+    local_nonpersistent_flags+=("--bind-only-when-ready")
     flags+=("--certificate-authority=")
     flags_with_completion+=("--certificate-authority")
     flags_completion+=("_filedir")

images/router/haproxy/conf/haproxy-config.template

@@ -100,6 +100,9 @@ listen stats :1936
     stats auth {{.StatsUser}}:{{.StatsPassword}}
 {{ end }}
 
+{{ if (and .BindOnlyWhenReady (not (and .State .ServiceUnits))) }}
+# Avoiding binding ports until routing configuration has been synchronized.
+{{ else }}
 frontend public
   bind :{{env "ROUTER_SERVICE_HTTP_PORT" "80"}}
   mode http
@@ -529,6 +532,7 @@ backend be_secure_{{$cfgIdx}}
     {{ end }}{{/* end range over serviceUnitNames */}}
   {{ end }}{{/* end tls==reencrypt */}}
 {{ end }}{{/* end loop over routes */}}
+{{ end }}{{/* end bind only when ready */}}
 {{ end }}{{/* end haproxy config template */}}
 
 {{/*--------------------------------- END OF HAPROXY CONFIG, BELOW ARE MAPPING FILES ------------------------*/}}

pkg/cmd/infra/router/template.go

@@ -62,6 +62,7 @@ type TemplateRouter struct {
 	DefaultCertificateDir  string
 	ExtendedValidation     bool
 	RouterService          *ktypes.NamespacedName
+	BindOnlyWhenReady      bool
 }
 
 // reloadInterval returns how often to run the router reloads. The interval
@@ -86,6 +87,7 @@ func (o *TemplateRouter) Bind(flag *pflag.FlagSet) {
 	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", util.Env("EXTENDED_VALIDATION", "true") == "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.BindOnlyWhenReady, "bind-only-when-ready", util.Env("BIND_ONLY_WHEN_READY", "") == "true", "Only bind ports when route state has been synchronized")
 }
 
 type RouterStats struct {
@@ -188,6 +190,7 @@ func (o *TemplateRouterOptions) Run() error {
 		StatsUsername:          o.StatsUsername,
 		StatsPassword:          o.StatsPassword,
 		PeerService:            o.RouterService,
+		BindOnlyWhenReady:      o.BindOnlyWhenReady,
 		IncludeUDP:             o.RouterSelection.IncludeUDP,
 		AllowWildcardRoutes:    o.RouterSelection.AllowWildcardRoutes,
 	}

pkg/router/template/plugin.go

@@ -50,6 +50,7 @@ type TemplatePluginConfig struct {
 	IncludeUDP             bool
 	AllowWildcardRoutes    bool
 	PeerService            *ktypes.NamespacedName
+	BindOnlyWhenReady      bool
 }
 
 // routerInterface controls the interaction of the plugin with the underlying router implementation
@@ -143,6 +144,7 @@ func NewTemplatePlugin(cfg TemplatePluginConfig, lookupSvc ServiceLookup) (*Temp
 		statsPort:              cfg.StatsPort,
 		allowWildcardRoutes:    cfg.AllowWildcardRoutes,
 		peerEndpointsKey:       peerKey,
+		bindOnlyWhenReady:      cfg.BindOnlyWhenReady,
 	}
 	router, err := newTemplateRouter(templateRouterCfg)
 	return newDefaultTemplatePlugin(router, cfg.IncludeUDP, lookupSvc), err

pkg/router/template/router.go

@@ -86,6 +86,8 @@ type templateRouter struct {
 	lock sync.Mutex
 	// the router should only reload when the value is false
 	skipCommit bool
+	// If true, haproxy should only bind ports when it has route and endpoint state
+	bindOnlyWhenReady bool
 }
 
 // templateRouterCfg holds all configuration items required to initialize the template router
@@ -103,6 +105,7 @@ type templateRouterCfg struct {
 	allowWildcardRoutes    bool
 	peerEndpointsKey       string
 	includeUDP             bool
+	bindOnlyWhenReady      bool
 }
 
 // templateConfig is a subset of the templateRouter information that should be passed to the template for generating
@@ -124,6 +127,8 @@ type templateData struct {
 	StatsPassword string
 	//port to expose stats with (if the template supports it)
 	StatsPort int
+	// whether the router should bind the default ports only when ready
+	BindOnlyWhenReady bool
 }
 
 func newTemplateRouter(cfg templateRouterCfg) (*templateRouter, error) {
@@ -162,6 +167,7 @@ func newTemplateRouter(cfg templateRouterCfg) (*templateRouter, error) {
 		allowWildcardRoutes:    cfg.allowWildcardRoutes,
 		peerEndpointsKey:       cfg.peerEndpointsKey,
 		peerEndpoints:          []Endpoint{},
+		bindOnlyWhenReady:      cfg.bindOnlyWhenReady,
 
 		rateLimitedCommitFunction:    nil,
 		rateLimitedCommitStopChannel: make(chan struct{}),
@@ -394,6 +400,7 @@ func (r *templateRouter) writeConfig() error {
 			StatsUser:          r.statsUser,
 			StatsPassword:      r.statsPassword,
 			StatsPort:          r.statsPort,
+			BindOnlyWhenReady:  r.bindOnlyWhenReady,
 		}
 		if err := template.Execute(file, data); err != nil {
 			file.Close()

test/integration/router_test.go

@@ -1256,6 +1256,10 @@ u3YLAbyW/lHhOCiZu2iAI8AbmXem9lW6Tr7p/97s0w==
 // createAndStartRouterContainer is responsible for deploying the router image in docker.  It assumes that all router images
 // will use a command line flag that can take --master which points to the master url
 func createAndStartRouterContainer(dockerCli *dockerClient.Client, masterIp string, routerStatsPort int, reloadInterval int) (containerId string, err error) {
+	return createAndStartRouterContainerBindWhenReady(dockerCli, masterIp, routerStatsPort, reloadInterval, false)
+}
+
+func createAndStartRouterContainerBindWhenReady(dockerCli *dockerClient.Client, masterIp string, routerStatsPort int, reloadInterval int, bindOnlyWhenReady bool) (containerId string, err error) {
 	ports := []string{"80", "443"}
 	if routerStatsPort > 0 {
 		ports = append(ports, fmt.Sprintf("%d", routerStatsPort))
@@ -1291,6 +1295,7 @@ func createAndStartRouterContainer(dockerCli *dockerClient.Client, masterIp stri
 		fmt.Sprintf("STATS_USERNAME=%s", statsUser),
 		fmt.Sprintf("STATS_PASSWORD=%s", statsPassword),
 		fmt.Sprintf("DEFAULT_CERTIFICATE=%s", defaultCert),
+		fmt.Sprintf("BIND_ONLY_WHEN_READY=%s", strconv.FormatBool(bindOnlyWhenReady)),
 	}
 
 	reloadIntVar := fmt.Sprintf("RELOAD_INTERVAL=%ds", reloadInterval)
@@ -1580,10 +1585,131 @@ func TestRouterReloadCoalesce(t *testing.T) {
 	}
 }
 
+func waitForRouter(host string, port int, path string) {
+	hostAndPort := fmt.Sprintf("%s:%d", host, port)
+	uri := fmt.Sprintf("%s/%s", hostAndPort, path)
+	waitForRoute(uri, hostAndPort, "http", nil, "")
+}
+
 // waitForRouterToBecomeAvailable checks for the router start up and waits
 // till it becomes available.
 func waitForRouterToBecomeAvailable(host string, port int) {
-	hostAndPort := fmt.Sprintf("%s:%d", host, port)
-	uri := fmt.Sprintf("%s/healthz", hostAndPort)
-	waitForRoute(uri, hostAndPort, "http", nil, "")
+	waitForRouter(host, port, "healthz")
+}
+
+// waitForRouterToBecomeReady checks that the router is ready.
+func waitForRouterToBecomeReady(host string, port int) {
+	waitForRouter(host, port, "readyz")
+}
+
+func makeRouterRequest(t *testing.T, scheme string) (*http.Response, error) {
+	uri := fmt.Sprintf("%s://%s", scheme, getRouteAddress())
+	var tlsConfig *tls.Config
+	if scheme == "https" {
+		tlsConfig = &tls.Config{
+			InsecureSkipVerify: true,
+		}
+	}
+
+	httpClient := &http.Client{
+		Transport: knet.SetTransportDefaults(&http.Transport{
+			TLSClientConfig: tlsConfig,
+		}),
+	}
+	req, err := http.NewRequest("GET", uri, nil)
+	if err != nil {
+		t.Fatalf("Error creating %s request : %v", scheme, err)
+	}
+
+	return httpClient.Do(req)
+}
+
+// Ensure that when configured with BIND_ONLY_WHEN_READY=true, haproxy
+// binds ports only when it has routes and endpoints to expose.
+func TestRouterPortsBoundOnlyWhenReady(t *testing.T) {
+	// Create a new master but do not start it yet to simulate a router without api access.
+	fakeMasterAndPod := tr.NewTestHttpService()
+
+	dockerCli, err := testutil.NewDockerClient()
+	if err != nil {
+		t.Fatalf("Unable to get docker client: %v", err)
+	}
+
+	bindOnlyWhenReady := true
+	routerId, err := createAndStartRouterContainerBindWhenReady(dockerCli, fakeMasterAndPod.MasterHttpAddr, statsPort, 1, bindOnlyWhenReady)
+	if err != nil {
+		t.Fatalf("Error starting container %s : %v", getRouterImage(), err)
+	}
+	defer cleanUp(t, dockerCli, routerId)
+
+	waitForRouterToBecomeAvailable("127.0.0.1", statsPort)
+
+	// Validate that the default ports are not bound due to the router not being ready.
+	schemes := []string{"http", "https"}
+	for _, scheme := range schemes {
+		_, err = makeRouterRequest(t, scheme)
+		if err == nil {
+			t.Fatalf("Router is unexpectedly accepting connections via %v", scheme)
+		} else if !strings.HasSuffix(fmt.Sprintf("%v", err), "connection refused") {
+			t.Fatalf("Unexpected error when dispatching %v request: %v", scheme, err)
+		}
+	}
+
+	// Start the master
+	defer fakeMasterAndPod.Stop()
+	err = fakeMasterAndPod.Start()
+	validateServer(fakeMasterAndPod, t)
+	if err != nil {
+		t.Fatalf("Unable to start http server: %v", err)
+	}
+
+	// Create events that will allow the router to have enough state to consider itself 'ready'
+	httpEndpoint, err := getEndpoint(fakeMasterAndPod.PodHttpAddr)
+	if err != nil {
+		t.Fatalf("Couldn't get http endpoint: %v", err)
+	}
+	endpointEvent := &watch.Event{
+		Type: watch.Added,
+		Object: &kapi.Endpoints{
+			ObjectMeta: kapi.ObjectMeta{
+				Name:      "myService",
+				Namespace: "default",
+			},
+			Subsets: []kapi.EndpointSubset{httpEndpoint},
+		},
+	}
+	routeEvent := &watch.Event{
+		Type: watch.Added,
+		Object: &routeapi.Route{
+			ObjectMeta: kapi.ObjectMeta{
+				Name:      "path",
+				Namespace: "default",
+			},
+			Spec: routeapi.RouteSpec{
+				Host: "www.example.com",
+				Path: "/test",
+				To: routeapi.RouteTargetReference{
+					Name: "myService",
+				},
+				TLS: &routeapi.TLSConfig{},
+			},
+		},
+	}
+	sendTimeout(t, fakeMasterAndPod.EndpointChannel, eventString(endpointEvent), 30*time.Second)
+	sendTimeout(t, fakeMasterAndPod.RouteChannel, eventString(routeEvent), 30*time.Second)
+
+	waitForRouterToBecomeReady("127.0.0.1", statsPort)
+
+	// Validate that the default ports are now bound
+	for _, scheme := range schemes {
+		resp, err := makeRouterRequest(t, scheme)
+		if err != nil {
+			t.Fatalf("Error dispatching %s request : %v", scheme, err)
+		}
+		defer resp.Body.Close()
+
+		if resp.StatusCode != 503 {
+			t.Fatalf("Router %s response error, got %v expected 503.", scheme, resp.StatusCode)
+		}
+	}
 }