Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn user if Pod/Service networks will be accessed via proxy. #52792

Merged
merged 1 commit into from Oct 21, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions cmd/kubeadm/app/preflight/BUILD
Expand Up @@ -19,6 +19,7 @@ go_library(
"//cmd/kubeadm/app/constants:go_default_library",
"//pkg/api/validation:go_default_library",
"//pkg/kubeapiserver/authorizer/modes:go_default_library",
"//pkg/registry/core/service/ipallocator:go_default_library",
"//pkg/util/initsystem:go_default_library",
"//pkg/util/version:go_default_library",
"//pkg/version:go_default_library",
Expand All @@ -27,6 +28,7 @@ go_library(
"//vendor/github.com/PuerkitoBio/purell:go_default_library",
"//vendor/github.com/blang/semver:go_default_library",
"//vendor/github.com/spf13/pflag:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/net:go_default_library",
],
)

Expand Down
56 changes: 55 additions & 1 deletion cmd/kubeadm/app/preflight/checks.go
Expand Up @@ -40,12 +40,14 @@ import (

"net/url"

netutil "k8s.io/apimachinery/pkg/util/net"
apiservoptions "k8s.io/kubernetes/cmd/kube-apiserver/app/options"
cmoptions "k8s.io/kubernetes/cmd/kube-controller-manager/app/options"
kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
"k8s.io/kubernetes/pkg/api/validation"
authzmodes "k8s.io/kubernetes/pkg/kubeapiserver/authorizer/modes"
"k8s.io/kubernetes/pkg/registry/core/service/ipallocator"
"k8s.io/kubernetes/pkg/util/initsystem"
versionutil "k8s.io/kubernetes/pkg/util/version"
kubeadmversion "k8s.io/kubernetes/pkg/version"
Expand Down Expand Up @@ -332,6 +334,56 @@ func (hst HTTPProxyCheck) Check() (warnings, errors []error) {
return nil, nil
}

// HTTPProxyCIDRCheck checks if https connection to specific subnet is going
// to be done directly or over proxy. If proxy detected, it will return warning.
// Similar to HTTPProxyCheck above, but operates with subnets and uses API
// machinery transport defaults to simulate kube-apiserver accessing cluster
// services and pods.
type HTTPProxyCIDRCheck struct {
Proto string
CIDR string
}

// Check validates http connectivity to first IP address in the CIDR.
// If it is not directly connected and goes via proxy it will produce warning.
func (subnet HTTPProxyCIDRCheck) Check() (warnings, errors []error) {

if len(subnet.CIDR) == 0 {
return nil, nil
}

_, cidr, err := net.ParseCIDR(subnet.CIDR)
if err != nil {
return nil, []error{fmt.Errorf("error parsing CIDR %q: %v", subnet.CIDR, err)}
}

testIP, err := ipallocator.GetIndexedIP(cidr, 1)
if err != nil {
return nil, []error{fmt.Errorf("unable to get first IP address from the given CIDR (%s): %v", cidr.String(), err)}
}

testIPstring := testIP.String()
if len(testIP) == net.IPv6len {
testIPstring = fmt.Sprintf("[%s]:1234", testIP)
}
url := fmt.Sprintf("%s://%s/", subnet.Proto, testIPstring)

req, err := http.NewRequest("GET", url, nil)
if err != nil {
return nil, []error{err}
}

// Utilize same transport defaults as it will be used by API server
proxy, err := netutil.SetOldTransportDefaults(&http.Transport{}).Proxy(req)
if err != nil {
return nil, []error{err}
}
if proxy != nil {
return []error{fmt.Errorf("connection to %q uses proxy %q. This may lead to malfunctional cluster setup. Make sure that Pod and Services IP ranges specified correctly as exceptions in proxy configuration", subnet.CIDR, proxy)}, nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm struggling to see why this is always an error and how we ensure we don't get false positives.

Would it be possible to add test cases here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will produce warning, not error. Test case potentially possible, just need to be careful with set/unset environment variables during test.

}
return nil, nil
}

// ExtraArgsCheck checks if arguments are valid.
type ExtraArgsCheck struct {
APIServerExtraArgs map[string]string
Expand Down Expand Up @@ -648,7 +700,6 @@ func RunInitMasterChecks(cfg *kubeadmapi.MasterConfiguration) error {
PortOpenCheck{port: 10250},
PortOpenCheck{port: 10251},
PortOpenCheck{port: 10252},
HTTPProxyCheck{Proto: "https", Host: cfg.API.AdvertiseAddress, Port: int(cfg.API.BindPort)},
DirAvailableCheck{Path: filepath.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.ManifestsSubDirName)},
FileContentCheck{Path: bridgenf, Content: []byte{'1'}},
SwapCheck{},
Expand All @@ -666,6 +717,9 @@ func RunInitMasterChecks(cfg *kubeadmapi.MasterConfiguration) error {
ControllerManagerExtraArgs: cfg.ControllerManagerExtraArgs,
SchedulerExtraArgs: cfg.SchedulerExtraArgs,
},
HTTPProxyCheck{Proto: "https", Host: cfg.API.AdvertiseAddress, Port: int(cfg.API.BindPort)},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still necessary?

Copy link
Member Author

@kad kad Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is another check. (master IP address is not part of pod/service CIDR checks). Just changed order, so all proxy related warnings will be in one place and not mixed with warnings/errors of something else.

HTTPProxyCIDRCheck{Proto: "https", CIDR: cfg.Networking.ServiceSubnet},
HTTPProxyCIDRCheck{Proto: "https", CIDR: cfg.Networking.PodSubnet},
}

if len(cfg.Etcd.Endpoints) == 0 {
Expand Down
106 changes: 106 additions & 0 deletions cmd/kubeadm/app/preflight/checks_test.go
Expand Up @@ -20,10 +20,12 @@ import (
"bytes"
"fmt"
"io/ioutil"
"strings"
"testing"

"github.com/renstrom/dedent"

"net/http"
"os"

kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
Expand Down Expand Up @@ -432,3 +434,107 @@ func TestKubernetesVersionCheck(t *testing.T) {
}
}
}

func TestHTTPProxyCIDRCheck(t *testing.T) {
var tests = []struct {
check HTTPProxyCIDRCheck
expectWarnings bool
}{
{
check: HTTPProxyCIDRCheck{
Proto: "https",
CIDR: "127.0.0.0/8",
}, // Loopback addresses never should produce proxy warnings
expectWarnings: false,
},
{
check: HTTPProxyCIDRCheck{
Proto: "https",
CIDR: "10.96.0.0/12",
}, // Expected to be accessed directly, we set NO_PROXY to 10.0.0.0/8
expectWarnings: false,
},
{
check: HTTPProxyCIDRCheck{
Proto: "https",
CIDR: "192.168.0.0/16",
}, // Expected to go via proxy as this range is not listed in NO_PROXY
expectWarnings: true,
},
{
check: HTTPProxyCIDRCheck{
Proto: "https",
CIDR: "2001:db8::/56",
}, // Expected to be accessed directly, part of 2001:db8::/48 in NO_PROXY
expectWarnings: false,
},
{
check: HTTPProxyCIDRCheck{
Proto: "https",
CIDR: "2001:db8:1::/56",
}, // Expected to go via proxy, range is not in 2001:db8::/48
expectWarnings: true,
},
}

// Save current content of *_proxy and *_PROXY variables.
savedEnv := resetProxyEnv()
defer restoreEnv(savedEnv)
t.Log("Saved environment: ", savedEnv)

os.Setenv("HTTP_PROXY", "http://proxy.example.com:3128")
os.Setenv("NO_PROXY", "example.com,10.0.0.0/8,2001:db8::/48")

// Check if we can reliably execute tests:
// ProxyFromEnvironment caches the *_proxy environment variables and
// if ProxyFromEnvironment already executed before our test with empty
// HTTP_PROXY it will make these tests return false positive failures
req, err := http.NewRequest("GET", "http://host.fake.tld/", nil)
if err != nil {
t.Fatalf("unexpected err: %v", err)
}
proxy, err := http.ProxyFromEnvironment(req)
if err != nil {
t.Fatalf("unexpected err: %v", err)
}
if proxy == nil {
t.Skip("test skipped as ProxyFromEnvironment already initialized in environment without defined HTTP proxy")
}
t.Log("http.ProxyFromEnvironment is usable, continue executing test")

for _, rt := range tests {
warning, _ := rt.check.Check()
if (warning != nil) != rt.expectWarnings {
t.Errorf(
"failed HTTPProxyCIDRCheck:\n\texpected: %t\n\t actual: %t (CIDR:%s). Warnings: %v",
rt.expectWarnings,
(warning != nil),
rt.check.CIDR,
warning,
)
}
}
}

// resetProxyEnv is helper function that unsets all *_proxy variables
// and return previously set values as map. This can be used to restore
// original state of the environment.
func resetProxyEnv() map[string]string {
savedEnv := make(map[string]string)
for _, e := range os.Environ() {
pair := strings.Split(e, "=")
if strings.HasSuffix(strings.ToLower(pair[0]), "_proxy") {
savedEnv[pair[0]] = pair[1]
os.Unsetenv(pair[0])
}
}
return savedEnv
}

// restoreEnv is helper function to restores values
// of environment variables from saved state in the map
func restoreEnv(e map[string]string) {
for k, v := range e {
os.Setenv(k, v)
}
}