cmd/containerboot: load containerboot serve config that does not contain HTTPS endpoint in tailnets with HTTPS disabled (#14538)

cmd/containerboot: load containerboot serve config that does not contain HTTPS endpoint in tailnets with HTTPS disabled

Fixes an issue where, if a tailnet has HTTPS disabled, no serve config
set via TS_SERVE_CONFIG was loaded, even if it does not contain an HTTPS endpoint.
Now for tailnets with HTTPS disabled serve config provided to containerboot is considered invalid
(and therefore not loaded) only if there is an HTTPS endpoint defined in the config.

Fixes tailscale/tailscale#14495

Signed-off-by: Irbe Krumina <irbe@tailscale.com>

cmd/containerboot/serve.go

@@ -68,7 +68,6 @@ func watchServeConfigChanges(ctx context.Context, path string, cdChanged <-chan
 		if prevServeConfig != nil && reflect.DeepEqual(sc, prevServeConfig) {
 			continue
 		}
-		validateHTTPSServe(certDomain, sc)
 		if err := updateServeConfig(ctx, sc, certDomain, lc); err != nil {
 			log.Fatalf("serve proxy: error updating serve config: %v", err)
 		}
@@ -88,27 +87,34 @@ func certDomainFromNetmap(nm *netmap.NetworkMap) string {
 	return nm.DNS.CertDomains[0]
 }
 
-func updateServeConfig(ctx context.Context, sc *ipn.ServeConfig, certDomain string, lc *tailscale.LocalClient) error {
+// localClient is a subset of tailscale.LocalClient that can be mocked for testing.
-	// TODO(irbekrm): This means that serve config that does not expose HTTPS endpoint will not be set for a tailnet
+type localClient interface {
-	// that does not have HTTPS enabled. We probably want to fix this.
+	SetServeConfig(context.Context, *ipn.ServeConfig) error
-	if certDomain == kubetypes.ValueNoHTTPS {
+}
+
+func updateServeConfig(ctx context.Context, sc *ipn.ServeConfig, certDomain string, lc localClient) error {
+	if !isValidHTTPSConfig(certDomain, sc) {
 		return nil
 	}
 	log.Printf("serve proxy: applying serve config")
 	return lc.SetServeConfig(ctx, sc)
 }
 
-func validateHTTPSServe(certDomain string, sc *ipn.ServeConfig) {
+func isValidHTTPSConfig(certDomain string, sc *ipn.ServeConfig) bool {
-	if certDomain != kubetypes.ValueNoHTTPS || !hasHTTPSEndpoint(sc) {
+	if certDomain == kubetypes.ValueNoHTTPS && hasHTTPSEndpoint(sc) {
-		return
+		log.Printf(
-	}
+			`serve proxy: this node is configured as a proxy that exposes an HTTPS endpoint to tailnet,
-	log.Printf(
-		`serve proxy: this node is configured as a proxy that exposes an HTTPS endpoint to tailnet,
 		(perhaps a Kubernetes operator Ingress proxy) but it is not able to issue TLS certs, so this will likely not work.
 		To make it work, ensure that HTTPS is enabled for your tailnet, see https://tailscale.com/kb/1153/enabling-https for more details.`)
+		return false
+	}
+	return true
 }
 
 func hasHTTPSEndpoint(cfg *ipn.ServeConfig) bool {
+	if cfg == nil {
+		return false
+	}
 	for _, tcpCfg := range cfg.TCP {
 		if tcpCfg.HTTPS {
 			return true
@@ -127,6 +133,12 @@ func readServeConfig(path, certDomain string) (*ipn.ServeConfig, error) {
 	if err != nil {
 		return nil, err
 	}
+	// Serve config can be provided by users as well as the Kubernetes Operator (for its proxies). User-provided
+	// config could be empty for reasons.
+	if len(j) == 0 {
+		log.Printf("serve proxy: serve config file is empty, skipping")
+		return nil, nil
+	}
 	j = bytes.ReplaceAll(j, []byte("${TS_CERT_DOMAIN}"), []byte(certDomain))
 	var sc ipn.ServeConfig
 	if err := json.Unmarshal(j, &sc); err != nil {

cmd/containerboot/serve_test.go

@@ -0,0 +1,267 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+//go:build linux
+
+package main
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"tailscale.com/client/tailscale"
+	"tailscale.com/ipn"
+	"tailscale.com/kube/kubetypes"
+)
+
+func TestUpdateServeConfig(t *testing.T) {
+	tests := []struct {
+		name       string
+		sc         *ipn.ServeConfig
+		certDomain string
+		wantCall   bool
+	}{
+		{
+			name: "no_https_no_cert_domain",
+			sc: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					80: {HTTP: true},
+				},
+			},
+			certDomain: kubetypes.ValueNoHTTPS, // tailnet has HTTPS disabled
+			wantCall:   true,                   // should set serve config as it doesn't have HTTPS endpoints
+		},
+		{
+			name: "https_with_cert_domain",
+			sc: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					443: {HTTPS: true},
+				},
+				Web: map[ipn.HostPort]*ipn.WebServerConfig{
+					"${TS_CERT_DOMAIN}:443": {
+						Handlers: map[string]*ipn.HTTPHandler{
+							"/": {Proxy: "http://10.0.1.100:8080"},
+						},
+					},
+				},
+			},
+			certDomain: "test-node.tailnet.ts.net",
+			wantCall:   true,
+		},
+		{
+			name: "https_without_cert_domain",
+			sc: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					443: {HTTPS: true},
+				},
+			},
+			certDomain: kubetypes.ValueNoHTTPS,
+			wantCall:   false, // incorrect configuration- should not set serve config
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			fakeLC := &fakeLocalClient{}
+			err := updateServeConfig(context.Background(), tt.sc, tt.certDomain, fakeLC)
+			if err != nil {
+				t.Errorf("updateServeConfig() error = %v", err)
+			}
+			if fakeLC.setServeCalled != tt.wantCall {
+				t.Errorf("SetServeConfig() called = %v, want %v", fakeLC.setServeCalled, tt.wantCall)
+			}
+		})
+	}
+}
+
+func TestReadServeConfig(t *testing.T) {
+	tests := []struct {
+		name       string
+		gotSC      string
+		certDomain string
+		wantSC     *ipn.ServeConfig
+		wantErr    bool
+	}{
+		{
+			name: "empty_file",
+		},
+		{
+			name: "valid_config_with_cert_domain_placeholder",
+			gotSC: `{
+				"TCP": {
+					"443": {
+						"HTTPS": true
+					}
+				},
+				"Web": {
+					"${TS_CERT_DOMAIN}:443": {
+					"Handlers": {
+						"/api": {
+							"Proxy": "https://10.2.3.4/api"
+						}}}}}`,
+			certDomain: "example.com",
+			wantSC: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					443: {
+						HTTPS: true,
+					},
+				},
+				Web: map[ipn.HostPort]*ipn.WebServerConfig{
+					ipn.HostPort("example.com:443"): {
+						Handlers: map[string]*ipn.HTTPHandler{
+							"/api": {
+								Proxy: "https://10.2.3.4/api",
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "valid_config_for_http_proxy",
+			gotSC: `{
+				"TCP": {
+					"80": {
+						"HTTP": true
+					}
+				}}`,
+			wantSC: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					80: {
+						HTTP: true,
+					},
+				},
+			},
+		},
+		{
+			name: "config_without_cert_domain",
+			gotSC: `{
+				"TCP": {
+					"443": {
+						"HTTPS": true
+					}
+				},
+				"Web": {
+					"localhost:443": {
+					"Handlers": {
+						"/api": {
+							"Proxy": "https://10.2.3.4/api"
+						}}}}}`,
+			certDomain: "",
+			wantErr:    false,
+			wantSC: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					443: {
+						HTTPS: true,
+					},
+				},
+				Web: map[ipn.HostPort]*ipn.WebServerConfig{
+					ipn.HostPort("localhost:443"): {
+						Handlers: map[string]*ipn.HTTPHandler{
+							"/api": {
+								Proxy: "https://10.2.3.4/api",
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name:    "invalid_json",
+			gotSC:   "invalid json",
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			dir := t.TempDir()
+			path := filepath.Join(dir, "serve-config.json")
+			if err := os.WriteFile(path, []byte(tt.gotSC), 0644); err != nil {
+				t.Fatal(err)
+			}
+
+			got, err := readServeConfig(path, tt.certDomain)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("readServeConfig() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !cmp.Equal(got, tt.wantSC) {
+				t.Errorf("readServeConfig() diff (-got +want):\n%s", cmp.Diff(got, tt.wantSC))
+			}
+		})
+	}
+}
+
+type fakeLocalClient struct {
+	*tailscale.LocalClient
+	setServeCalled bool
+}
+
+func (m *fakeLocalClient) SetServeConfig(ctx context.Context, cfg *ipn.ServeConfig) error {
+	m.setServeCalled = true
+	return nil
+}
+
+func TestHasHTTPSEndpoint(t *testing.T) {
+	tests := []struct {
+		name string
+		cfg  *ipn.ServeConfig
+		want bool
+	}{
+		{
+			name: "nil_config",
+			cfg:  nil,
+			want: false,
+		},
+		{
+			name: "empty_config",
+			cfg:  &ipn.ServeConfig{},
+			want: false,
+		},
+		{
+			name: "no_https_endpoints",
+			cfg: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					80: {
+						HTTPS: false,
+					},
+				},
+			},
+			want: false,
+		},
+		{
+			name: "has_https_endpoint",
+			cfg: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					443: {
+						HTTPS: true,
+					},
+				},
+			},
+			want: true,
+		},
+		{
+			name: "mixed_endpoints",
+			cfg: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					80:  {HTTPS: false},
+					443: {HTTPS: true},
+				},
+			},
+			want: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := hasHTTPSEndpoint(tt.cfg)
+			if got != tt.want {
+				t.Errorf("hasHTTPSEndpoint() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}