automatically detect node ip during install for noproxy (#1186)

* add a note to readme specifying a folder to place the license * get default interface * wip default noproxies and prompts * disable most tests * prompt for new noproxy * store provided noproxy values and total noproxy values independently * dedup code * simplify noproxy subnet * regen CRDs * update unit tests * fix no-prompt case * ? * FFFF * ??? * test bigger noproxy * reuse proxy object, don't rederive from flags * reorder imports * Revert "disable most tests" This reverts commit 9479324. * misspelling * rename to combineNoProxySuppliedValuesAndDefaults, remove debug logs * add link to k0s logic * recursively prompt for noproxy * remove prompts for proxy, just add the default subnet if the current IP is not covered * maybePromptForNoProxy -> includeLocalIPInNoProxy * check the noproxy config on node join * test that should fail to ensure that join command preflights are working properly * fix 'adding "blah" to the noproxy you provided' message * test failed successfully, restoring it * restore all tests, again * Accept Alex's suggestions for noproxy messaging Co-authored-by: Alex Parker <7272359+ajp-io@users.noreply.github.com> --------- Co-authored-by: Alex Parker <7272359+ajp-io@users.noreply.github.com>
replicatedhq · Sep 19, 2024 · 058761e · 058761e
1 parent 734a6c5
commit 058761e
Show file tree

Hide file tree

Showing 13 changed files with 228 additions and 35 deletions.
diff --git a/README.md b/README.md
@@ -87,6 +87,7 @@ Additionally, it includes a Registry when deployed in air gap mode.
     ```
 
 1. In the Vendor Portal, create and download a license that is assigned to the channel.
+We recommend storing this license in the `local-dev/` directory, as it is gitignored and not otherwise used by the CI.
 
 1. Install Embedded Cluster:
     ```bash

diff --git a/cmd/embedded-cluster/install.go b/cmd/embedded-cluster/install.go
@@ -441,7 +441,7 @@ func waitForK0s() error {
 }
 
 // installAndWaitForK0s installs the k0s binary and waits for it to be ready
-func installAndWaitForK0s(c *cli.Context, applier *addons.Applier) (*k0sconfig.ClusterConfig, error) {
+func installAndWaitForK0s(c *cli.Context, applier *addons.Applier, proxy *ecv1beta1.ProxySpec) (*k0sconfig.ClusterConfig, error) {
 	loading := spinner.Start()
 	defer loading.Close()
 	loading.Infof("Installing %s node", defaults.BinaryName())
@@ -452,7 +452,6 @@ func installAndWaitForK0s(c *cli.Context, applier *addons.Applier) (*k0sconfig.C
 		metrics.ReportApplyFinished(c, err)
 		return nil, err
 	}
-	proxy := getProxySpecFromFlags(c)
 	logrus.Debugf("creating systemd unit files")
 	if err := createSystemdUnitFiles(false, proxy); err != nil {
 		err := fmt.Errorf("unable to create systemd unit files: %w", err)
@@ -580,7 +579,13 @@ var installCommand = &cli.Command{
 		},
 	)),
 	Action: func(c *cli.Context) error {
+		var err error
 		proxy := getProxySpecFromFlags(c)
+		proxy, err = includeLocalIPInNoProxy(c, proxy)
+		if err != nil {
+			metrics.ReportApplyFinished(c, err)
+			return err
+		}
 		setProxyEnv(proxy)
 
 		logrus.Debugf("checking if %s is already installed", binName)
@@ -621,12 +626,13 @@ var installCommand = &cli.Command{
 			metrics.ReportApplyFinished(c, err)
 			return err
 		}
+
 		logrus.Debugf("materializing binaries")
 		if err := materializeFiles(c); err != nil {
 			metrics.ReportApplyFinished(c, err)
 			return err
 		}
-		applier, err := getAddonsApplier(c, adminConsolePwd)
+		applier, err := getAddonsApplier(c, adminConsolePwd, proxy)
 		if err != nil {
 			metrics.ReportApplyFinished(c, err)
 			return err
@@ -641,7 +647,7 @@ var installCommand = &cli.Command{
 			metrics.ReportApplyFinished(c, err)
 			return err
 		}
-		cfg, err := installAndWaitForK0s(c, applier)
+		cfg, err := installAndWaitForK0s(c, applier, proxy)
 		if err != nil {
 			return err
 		}
@@ -655,7 +661,7 @@ var installCommand = &cli.Command{
 	},
 }
 
-func getAddonsApplier(c *cli.Context, adminConsolePwd string) (*addons.Applier, error) {
+func getAddonsApplier(c *cli.Context, adminConsolePwd string, proxy *ecv1beta1.ProxySpec) (*addons.Applier, error) {
 	opts := []addons.Option{}
 	if c.Bool("no-prompt") {
 		opts = append(opts, addons.WithoutPrompt())
@@ -666,7 +672,6 @@ func getAddonsApplier(c *cli.Context, adminConsolePwd string) (*addons.Applier,
 	if ab := c.String("airgap-bundle"); ab != "" {
 		opts = append(opts, addons.WithAirgapBundle(ab))
 	}
-	proxy := getProxySpecFromFlags(c)
 	if proxy != nil {
 		opts = append(opts, addons.WithProxy(proxy.HTTPProxy, proxy.HTTPSProxy, proxy.NoProxy))
 	}

diff --git a/cmd/embedded-cluster/join.go b/cmd/embedded-cluster/join.go
@@ -190,6 +190,13 @@ var joinCommand = &cli.Command{
 		}
 
 		setProxyEnv(jcmd.Proxy)
+		proxyOK, localIP, err := checkProxyConfigForLocalIP(jcmd.Proxy)
+		if err != nil {
+			return fmt.Errorf("failed to check proxy config for local IP: %w", err)
+		}
+		if !proxyOK {
+			return fmt.Errorf("no-proxy config %q does not allow access to local IP %q", jcmd.Proxy.NoProxy, localIP)
+		}
 
 		isAirgap := c.String("airgap-bundle") != ""
 
@@ -207,7 +214,7 @@ var joinCommand = &cli.Command{
 			return err
 		}
 
-		applier, err := getAddonsApplier(c, "")
+		applier, err := getAddonsApplier(c, "", jcmd.Proxy)
 		if err != nil {
 			metrics.ReportJoinFailed(c.Context, jcmd.MetricsBaseURL, jcmd.ClusterID, err)
 			return err

diff --git a/cmd/embedded-cluster/preflights.go b/cmd/embedded-cluster/preflights.go
@@ -41,7 +41,12 @@ var installRunPreflightsCommand = &cli.Command{
 		return nil
 	},
 	Action: func(c *cli.Context) error {
+		var err error
 		proxy := getProxySpecFromFlags(c)
+		proxy, err = includeLocalIPInNoProxy(c, proxy)
+		if err != nil {
+			return err
+		}
 		setProxyEnv(proxy)
 
 		license, err := getLicenseFromFilepath(c.String("license"))
@@ -56,7 +61,7 @@ var installRunPreflightsCommand = &cli.Command{
 			return err
 		}
 
-		applier, err := getAddonsApplier(c, "")
+		applier, err := getAddonsApplier(c, "", proxy)
 		if err != nil {
 			return err
 		}
@@ -113,6 +118,13 @@ var joinRunPreflightsCommand = &cli.Command{
 		}
 
 		setProxyEnv(jcmd.Proxy)
+		proxyOK, localIP, err := checkProxyConfigForLocalIP(jcmd.Proxy)
+		if err != nil {
+			return fmt.Errorf("failed to check proxy config for local IP: %w", err)
+		}
+		if !proxyOK {
+			return fmt.Errorf("no-proxy config %q does not allow access to local IP %q", jcmd.Proxy.NoProxy, localIP)
+		}
 
 		isAirgap := c.String("airgap-bundle") != ""
 
@@ -121,7 +133,7 @@ var joinRunPreflightsCommand = &cli.Command{
 			return err
 		}
 
-		applier, err := getAddonsApplier(c, "")
+		applier, err := getAddonsApplier(c, "", jcmd.Proxy)
 		if err != nil {
 			return err
 		}

diff --git a/cmd/embedded-cluster/proxy.go b/cmd/embedded-cluster/proxy.go
@@ -1,11 +1,15 @@
 package main
 
 import (
+	"fmt"
+	"net"
 	"os"
 	"strings"
 
 	ecv1beta1 "github.com/replicatedhq/embedded-cluster/kinds/apis/v1beta1"
 	"github.com/replicatedhq/embedded-cluster/pkg/defaults"
+	"github.com/replicatedhq/embedded-cluster/pkg/netutils"
+	"github.com/sirupsen/logrus"
 	"github.com/urfave/cli/v2"
 )
 
@@ -36,12 +40,12 @@ func withProxyFlags(flags []cli.Flag) []cli.Flag {
 
 func getProxySpecFromFlags(c *cli.Context) *ecv1beta1.ProxySpec {
 	proxy := &ecv1beta1.ProxySpec{}
-	var noProxy []string
+	var providedNoProxy []string
 	if c.Bool("proxy") {
 		proxy.HTTPProxy = os.Getenv("HTTP_PROXY")
 		proxy.HTTPSProxy = os.Getenv("HTTPS_PROXY")
 		if os.Getenv("NO_PROXY") != "" {
-			noProxy = append(noProxy, os.Getenv("NO_PROXY"))
+			providedNoProxy = append(providedNoProxy, os.Getenv("NO_PROXY"))
 		}
 	}
 	if c.IsSet("http-proxy") {
@@ -51,17 +55,26 @@ func getProxySpecFromFlags(c *cli.Context) *ecv1beta1.ProxySpec {
 		proxy.HTTPSProxy = c.String("https-proxy")
 	}
 	if c.String("no-proxy") != "" {
-		noProxy = append(noProxy, c.String("no-proxy"))
+		providedNoProxy = append(providedNoProxy, c.String("no-proxy"))
 	}
+	proxy.ProvidedNoProxy = strings.Join(providedNoProxy, ",")
+	combineNoProxySuppliedValuesAndDefaults(c, proxy)
+	if proxy.HTTPProxy == "" && proxy.HTTPSProxy == "" && proxy.NoProxy == "" {
+		return nil
+	}
+	return proxy
+}
+
+func combineNoProxySuppliedValuesAndDefaults(c *cli.Context, proxy *ecv1beta1.ProxySpec) {
+	if proxy.ProvidedNoProxy == "" {
+		return
+	}
+	noProxy := strings.Split(proxy.ProvidedNoProxy, ",")
 	if len(noProxy) > 0 || proxy.HTTPProxy != "" || proxy.HTTPSProxy != "" {
 		noProxy = append(defaults.DefaultNoProxy, noProxy...)
 		noProxy = append(noProxy, c.String("pod-cidr"), c.String("service-cidr"))
 		proxy.NoProxy = strings.Join(noProxy, ",")
 	}
-	if proxy.HTTPProxy == "" && proxy.HTTPSProxy == "" && proxy.NoProxy == "" {
-		return nil
-	}
-	return proxy
 }
 
 // setProxyEnv sets the HTTP_PROXY, HTTPS_PROXY, and NO_PROXY environment variables based on the provided ProxySpec.
@@ -80,3 +93,81 @@ func setProxyEnv(proxy *ecv1beta1.ProxySpec) {
 		os.Setenv("NO_PROXY", proxy.NoProxy)
 	}
 }
+
+func includeLocalIPInNoProxy(c *cli.Context, proxy *ecv1beta1.ProxySpec) (*ecv1beta1.ProxySpec, error) {
+	if proxy != nil && (proxy.HTTPProxy != "" || proxy.HTTPSProxy != "") {
+		// if there is a proxy set, then there needs to be a no proxy set
+		// if it is not set, prompt with a default (the local IP or subnet)
+		// if it is set, we need to check that it covers the local IP
+		defaultIPNet, err := netutils.GetDefaultIPNet()
+		if err != nil {
+			return nil, fmt.Errorf("failed to get default IPNet: %w", err)
+		}
+		cleanDefaultIPNet, err := cleanCIDR(defaultIPNet)
+		if err != nil {
+			return nil, fmt.Errorf("failed to clean subnet: %w", err)
+		}
+		if proxy.ProvidedNoProxy == "" {
+			logrus.Infof("--no-proxy was not set. Adding the default interface's subnet (%q) to the no-proxy list.", cleanDefaultIPNet)
+			proxy.ProvidedNoProxy = cleanDefaultIPNet
+			combineNoProxySuppliedValuesAndDefaults(c, proxy)
+			return proxy, nil
+		} else {
+			isValid, err := validateNoProxy(proxy.NoProxy, defaultIPNet.IP.String())
+			if err != nil {
+				return nil, fmt.Errorf("failed to validate no-proxy: %w", err)
+			} else if !isValid {
+				logrus.Infof("The node IP (%q) is not included in the provided no-proxy list (%q). Adding the default interface's subnet (%q) to the no-proxy list.", defaultIPNet.IP.String(), proxy.ProvidedNoProxy, cleanDefaultIPNet)
+				proxy.ProvidedNoProxy = cleanDefaultIPNet
+				combineNoProxySuppliedValuesAndDefaults(c, proxy)
+				return proxy, nil
+			}
+		}
+	}
+	return proxy, nil
+}
+
+// cleanCIDR returns a `.0/x` subnet instead of a `.2/x` etc subnet
+func cleanCIDR(defaultIPNet *net.IPNet) (string, error) {
+	_, newNet, err := net.ParseCIDR(defaultIPNet.String())
+	if err != nil {
+		return "", fmt.Errorf("failed to parse local inet CIDR %q: %w", defaultIPNet.String(), err)
+	}
+	return newNet.String(), nil
+}
+
+func validateNoProxy(newNoProxy string, localIP string) (bool, error) {
+	foundLocal := false
+	for _, oneEntry := range strings.Split(newNoProxy, ",") {
+		if oneEntry == localIP {
+			foundLocal = true
+		} else if strings.Contains(oneEntry, "/") {
+			_, ipnet, err := net.ParseCIDR(oneEntry)
+			if err != nil {
+				return false, fmt.Errorf("failed to parse CIDR within no-proxy: %w", err)
+			}
+			if ipnet.Contains(net.ParseIP(localIP)) {
+				foundLocal = true
+			}
+		}
+	}
+
+	return foundLocal, nil
+}
+
+func checkProxyConfigForLocalIP(proxy *ecv1beta1.ProxySpec) (bool, string, error) {
+	if proxy == nil {
+		return true, "", nil // no proxy is fine
+	}
+	if proxy.HTTPProxy == "" && proxy.HTTPSProxy == "" {
+		return true, "", nil // no proxy is fine
+	}
+
+	defaultIPNet, err := netutils.GetDefaultIPNet()
+	if err != nil {
+		return false, "", fmt.Errorf("failed to get default IPNet: %w", err)
+	}
+
+	ok, err := validateNoProxy(proxy.NoProxy, defaultIPNet.IP.String())
+	return ok, defaultIPNet.IP.String(), err
+}
diff --git a/cmd/embedded-cluster/proxy_test.go b/cmd/embedded-cluster/proxy_test.go
@@ -38,9 +38,10 @@ func Test_getProxySpecFromFlags(t *testing.T) {
 				flagSet.Set("proxy", "true")
 			},
 			want: &ecv1beta1.ProxySpec{
-				HTTPProxy:  "http://proxy",
-				HTTPSProxy: "https://proxy",
-				NoProxy:    "localhost,127.0.0.1,.cluster.local,.svc,no-proxy-1,no-proxy-2,10.244.0.0/16,10.96.0.0/12",
+				HTTPProxy:       "http://proxy",
+				HTTPSProxy:      "https://proxy",
+				ProvidedNoProxy: "no-proxy-1,no-proxy-2",
+				NoProxy:         "localhost,127.0.0.1,.cluster.local,.svc,no-proxy-1,no-proxy-2,10.244.0.0/16,10.96.0.0/12",
 			},
 		},
 		{
@@ -66,13 +67,14 @@ func Test_getProxySpecFromFlags(t *testing.T) {
 				flagSet.Set("no-proxy", "other-no-proxy-1,other-no-proxy-2")
 			},
 			want: &ecv1beta1.ProxySpec{
-				HTTPProxy:  "http://other-proxy",
-				HTTPSProxy: "https://other-proxy",
-				NoProxy:    "localhost,127.0.0.1,.cluster.local,.svc,other-no-proxy-1,other-no-proxy-2,10.244.0.0/16,10.96.0.0/12",
+				HTTPProxy:       "http://other-proxy",
+				HTTPSProxy:      "https://other-proxy",
+				ProvidedNoProxy: "other-no-proxy-1,other-no-proxy-2",
+				NoProxy:         "localhost,127.0.0.1,.cluster.local,.svc,other-no-proxy-1,other-no-proxy-2,10.244.0.0/16,10.96.0.0/12",
 			},
 		},
 		{
-			name: "proxy flags should override proxy from env",
+			name: "proxy flags should override proxy from env, but merge no-proxy",
 			init: func(t *testing.T, flagSet *flag.FlagSet) {
 				t.Setenv("HTTP_PROXY", "http://proxy")
 				t.Setenv("HTTPS_PROXY", "https://proxy")
@@ -84,9 +86,10 @@ func Test_getProxySpecFromFlags(t *testing.T) {
 				flagSet.Set("no-proxy", "other-no-proxy-1,other-no-proxy-2")
 			},
 			want: &ecv1beta1.ProxySpec{
-				HTTPProxy:  "http://other-proxy",
-				HTTPSProxy: "https://other-proxy",
-				NoProxy:    "localhost,127.0.0.1,.cluster.local,.svc,no-proxy-1,no-proxy-2,other-no-proxy-1,other-no-proxy-2,10.244.0.0/16,10.96.0.0/12",
+				HTTPProxy:       "http://other-proxy",
+				HTTPSProxy:      "https://other-proxy",
+				ProvidedNoProxy: "no-proxy-1,no-proxy-2,other-no-proxy-1,other-no-proxy-2",
+				NoProxy:         "localhost,127.0.0.1,.cluster.local,.svc,no-proxy-1,no-proxy-2,other-no-proxy-1,other-no-proxy-2,10.244.0.0/16,10.96.0.0/12",
 			},
 		},
 		{
@@ -100,9 +103,10 @@ func Test_getProxySpecFromFlags(t *testing.T) {
 				flagSet.Set("service-cidr", "2.2.2.2/24")
 			},
 			want: &ecv1beta1.ProxySpec{
-				HTTPProxy:  "http://other-proxy",
-				HTTPSProxy: "https://other-proxy",
-				NoProxy:    "localhost,127.0.0.1,.cluster.local,.svc,other-no-proxy-1,other-no-proxy-2,1.1.1.1/24,2.2.2.2/24",
+				HTTPProxy:       "http://other-proxy",
+				HTTPSProxy:      "https://other-proxy",
+				ProvidedNoProxy: "other-no-proxy-1,other-no-proxy-2",
+				NoProxy:         "localhost,127.0.0.1,.cluster.local,.svc,other-no-proxy-1,other-no-proxy-2,1.1.1.1/24,2.2.2.2/24",
 			},
 		},
 	}

diff --git a/cmd/embedded-cluster/restore.go b/cmd/embedded-cluster/restore.go
@@ -942,7 +942,7 @@ var restoreCommand = &cli.Command{
 			}
 		}
 
-		applier, err := getAddonsApplier(c, "")
+		applier, err := getAddonsApplier(c, "", proxy)
 		if err != nil {
 			return err
 		}

diff --git a/e2e/proxy_test.go b/e2e/proxy_test.go
@@ -48,7 +48,6 @@ func TestProxiedEnvironment(t *testing.T) {
 	line := []string{"single-node-install.sh", "ui"}
 	line = append(line, "--http-proxy", cluster.HTTPProxy)
 	line = append(line, "--https-proxy", cluster.HTTPProxy)
-	line = append(line, "--no-proxy", strings.Join(tc.IPs, ","))
 	if _, _, err := RunCommandOnNode(t, tc, 0, line, withProxyEnv(tc.IPs)); err != nil {
 		t.Fatalf("fail to install embedded-cluster on node %s: %v", tc.Nodes[0], err)
 	}
@@ -283,7 +282,6 @@ func TestInstallWithMITMProxy(t *testing.T) {
 	line := []string{"single-node-install.sh", "ui"}
 	line = append(line, "--http-proxy", cluster.HTTPMITMProxy)
 	line = append(line, "--https-proxy", cluster.HTTPMITMProxy)
-	line = append(line, "--no-proxy", strings.Join(tc.IPs, ","))
 	line = append(line, "--private-ca", "/usr/local/share/ca-certificates/proxy/ca.crt")
 	_, _, err := RunCommandOnNode(t, tc, 0, line, withMITMProxyEnv(tc.IPs))
 	require.NoError(t, err, "failed to install embedded-cluster on node 0")

diff --git a/kinds/apis/v1beta1/installation_types.go b/kinds/apis/v1beta1/installation_types.go
@@ -68,9 +68,10 @@ type ArtifactsLocation struct {
 
 // ProxySpec holds the proxy configuration.
 type ProxySpec struct {
-	HTTPProxy  string `json:"httpProxy,omitempty"`
-	HTTPSProxy string `json:"httpsProxy,omitempty"`
-	NoProxy    string `json:"noProxy,omitempty"`
+	HTTPProxy       string `json:"httpProxy,omitempty"`
+	HTTPSProxy      string `json:"httpsProxy,omitempty"`
+	ProvidedNoProxy string `json:"providedNoProxy,omitempty"`
+	NoProxy         string `json:"noProxy,omitempty"`
 }
 
 // NetworkSpec holds the network configuration.