Merge pull request #41020 from luxas/kubeadm_cleanup

Automatic merge from submit-queue (batch tested with PRs 41061, 40888, 40664, 41020, 41085)

kubeadm: Small cleanup and fixes, validate the service subnet

**What this PR does / why we need it**:
 - Validate the minimum subnet cidr so there are always 10 available addresses
 - Remove an old proxy arg function, add clustercidr to the proxy manifest and automatically calculate the dns ip

**Special notes for your reviewer**:

**Release note**:

```release-note
NONE
```

@errordeveloper @pires @mikedanese @dmmcquay @dgoodwin

cmd/kubeadm/app/apis/kubeadm/validation/BUILD

@@ -5,6 +5,7 @@ licenses(["notice"])
 load(
     "@io_bazel_rules_go//go:def.bzl",
     "go_library",
+    "go_test",
 )
 
 go_library(
@@ -13,6 +14,8 @@ go_library(
     tags = ["automanaged"],
     deps = [
         "//cmd/kubeadm/app/apis/kubeadm:go_default_library",
+        "//cmd/kubeadm/app/constants:go_default_library",
+        "//pkg/registry/core/service/ipallocator:go_default_library",
         "//vendor:k8s.io/apimachinery/pkg/util/validation/field",
     ],
 )
@@ -29,3 +32,11 @@ filegroup(
     srcs = [":package-srcs"],
     tags = ["automanaged"],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["validation_test.go"],
+    library = ":go_default_library",
+    tags = ["automanaged"],
+    deps = ["//vendor:k8s.io/apimachinery/pkg/util/validation/field"],
+)

cmd/kubeadm/app/apis/kubeadm/validation/validation.go

@@ -17,13 +17,18 @@ limitations under the License.
 package validation
 
 import (
+	"net"
+
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
+	kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
+	"k8s.io/kubernetes/pkg/registry/core/service/ipallocator"
 )
 
 func ValidateMasterConfiguration(c *kubeadm.MasterConfiguration) field.ErrorList {
 	allErrs := field.ErrorList{}
 	allErrs = append(allErrs, ValidateDiscovery(&c.Discovery, field.NewPath("discovery"))...)
+	allErrs = append(allErrs, ValidateDiscovery(&c.Discovery, field.NewPath("service subnet"))...)
 	return allErrs
 }
 
@@ -68,3 +73,15 @@ func ValidateTokenDiscovery(c *kubeadm.TokenDiscovery, fldPath *field.Path) fiel
 	allErrs := field.ErrorList{}
 	return allErrs
 }
+
+func ValidateServiceSubnet(subnet string, fldPath *field.Path) field.ErrorList {
+	_, svcSubnet, err := net.ParseCIDR(subnet)
+	if err != nil {
+		return field.ErrorList{field.Invalid(fldPath, nil, "couldn't parse the service subnet")}
+	}
+	numAddresses := ipallocator.RangeSize(svcSubnet)
+	if numAddresses < kubeadmconstants.MinimumAddressesInServiceSubnet {
+		return field.ErrorList{field.Invalid(fldPath, nil, "service subnet is too small")}
+	}
+	return field.ErrorList{}
+}

cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go

@@ -0,0 +1,48 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package validation
+
+import (
+	"testing"
+
+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+func TestValidateServiceSubnet(t *testing.T) {
+	var tests = []struct {
+		s        string
+		f        *field.Path
+		expected bool
+	}{
+		{"", nil, false},
+		{"this is not a cidr", nil, false}, // not a CIDR
+		{"10.0.0.1", nil, false},           // not a CIDR
+		{"10.96.0.1/29", nil, false},       // CIDR too small, only 8 addresses and we require at least 10
+		{"10.96.0.1/28", nil, true},        // a /28 subnet is ok because it can contain 16 addresses
+		{"10.96.0.1/12", nil, true},        // the default subnet should obviously pass as well
+	}
+	for _, rt := range tests {
+		actual := ValidateServiceSubnet(rt.s, rt.f)
+		if (len(actual) == 0) != rt.expected {
+			t.Errorf(
+				"failed ValidateServiceSubnet:\n\texpected: %t\n\t  actual: %t",
+				rt.expected,
+				(len(actual) == 0),
+			)
+		}
+	}
+}

cmd/kubeadm/app/constants/constants.go

@@ -48,4 +48,8 @@ const (
 
 	// APICallRetryInterval defines how long kubeadm should wait before retrying a failed API operation
 	APICallRetryInterval = 500 * time.Millisecond
+
+	// Minimum amount of nodes the Service subnet should allow.
+	// We need at least ten, because the DNS service is always at the tenth cluster clusterIP
+	MinimumAddressesInServiceSubnet = 10
 )

cmd/kubeadm/app/master/manifests.go

@@ -424,12 +424,6 @@ func getSchedulerCommand(cfg *kubeadmapi.MasterConfiguration, selfHosted bool) [
 	return command
 }
 
-func getProxyCommand(cfg *kubeadmapi.MasterConfiguration) []string {
-	return append(getComponentBaseCommand(proxy),
-		"--cluster-cidr="+cfg.Networking.PodSubnet,
-	)
-}
-
 func getProxyEnvVars() []api.EnvVar {
 	envs := []api.EnvVar{}
 	for _, env := range os.Environ() {

cmd/kubeadm/app/master/manifests_test.go

@@ -552,35 +552,3 @@ func TestGetSchedulerCommand(t *testing.T) {
 		}
 	}
 }
-
-func TestGetProxyCommand(t *testing.T) {
-	var tests = []struct {
-		cfg      *kubeadmapi.MasterConfiguration
-		expected []string
-	}{
-		{
-			cfg: &kubeadmapi.MasterConfiguration{
-				Networking: kubeadm.Networking{
-					PodSubnet: "bar",
-				},
-			},
-			expected: []string{
-				"kube-proxy",
-				"--cluster-cidr=bar",
-			},
-		},
-	}
-
-	for _, rt := range tests {
-		actual := getProxyCommand(rt.cfg)
-		for i := range actual {
-			if actual[i] != rt.expected[i] {
-				t.Errorf(
-					"failed getProxyCommand:\n\texpected: %s\n\t  actual: %s",
-					rt.expected[i],
-					actual[i],
-				)
-			}
-		}
-	}
-}

cmd/kubeadm/app/phases/addons/BUILD

@@ -16,12 +16,12 @@ go_library(
     tags = ["automanaged"],
     deps = [
         "//cmd/kubeadm/app/apis/kubeadm:go_default_library",
+        "//cmd/kubeadm/app/images:go_default_library",
         "//cmd/kubeadm/app/util:go_default_library",
         "//pkg/api:go_default_library",
         "//pkg/api/v1:go_default_library",
         "//pkg/apis/extensions/v1beta1:go_default_library",
         "//pkg/client/clientset_generated/clientset:go_default_library",
-        "//pkg/registry/core/service/ipallocator:go_default_library",
         "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1",
         "//vendor:k8s.io/apimachinery/pkg/runtime",
     ],

cmd/kubeadm/app/phases/addons/addons.go

@@ -25,17 +25,16 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	kuberuntime "k8s.io/apimachinery/pkg/runtime"
 	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
+	"k8s.io/kubernetes/cmd/kubeadm/app/images"
 	kubeadmutil "k8s.io/kubernetes/cmd/kubeadm/app/util"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/v1"
 	extensions "k8s.io/kubernetes/pkg/apis/extensions/v1beta1"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
-	"k8s.io/kubernetes/pkg/registry/core/service/ipallocator"
 )
 
 // CreateEssentialAddons creates the kube-proxy and kube-dns addons
 func CreateEssentialAddons(cfg *kubeadmapi.MasterConfiguration, client *clientset.Clientset) error {
-
 	proxyConfigMapBytes, err := kubeadmutil.ParseTemplate(KubeProxyConfigMap, struct{ MasterEndpoint string }{
 		// Fetch this value from the kubeconfig file
 		MasterEndpoint: fmt.Sprintf("https://%s:%d", cfg.API.AdvertiseAddresses[0], cfg.API.Port),
@@ -44,11 +43,9 @@ func CreateEssentialAddons(cfg *kubeadmapi.MasterConfiguration, client *clientse
 		return fmt.Errorf("error when parsing kube-proxy configmap template: %v", err)
 	}
 
-	proxyDaemonSetBytes, err := kubeadmutil.ParseTemplate(KubeProxyDaemonSet, struct{ ImageRepository, Arch, Version string }{
+	proxyDaemonSetBytes, err := kubeadmutil.ParseTemplate(KubeProxyDaemonSet, struct{ Image, ClusterCIDR string }{
-		ImageRepository: kubeadmapi.GlobalEnvParams.RepositoryPrefix,
+		Image:       images.GetCoreImage("proxy", cfg, kubeadmapi.GlobalEnvParams.HyperkubeImage),
-		Arch:            runtime.GOARCH,
+		ClusterCIDR: getClusterCIDR(cfg.Networking.PodSubnet),
-		// TODO: Fetch the version from the {API Server IP}/version
-		Version: cfg.KubernetesVersion,
 	})
 	if err != nil {
 		return fmt.Errorf("error when parsing kube-proxy daemonset template: %v", err)
@@ -69,8 +66,7 @@ func CreateEssentialAddons(cfg *kubeadmapi.MasterConfiguration, client *clientse
 		return fmt.Errorf("error when parsing kube-dns deployment template: %v", err)
 	}
 
-	// Get the DNS IP
+	dnsip, err := getDNSIP(client)
-	dnsip, err := getDNSIP(cfg.Networking.ServiceSubnet)
 	if err != nil {
 		return err
 	}
@@ -139,17 +135,28 @@ func CreateKubeDNSAddon(deploymentBytes, serviceBytes []byte, client *clientset.
 	return nil
 }
 
-// TODO: Instead of looking at the subnet given to kubeadm, it should be possible to only use /28 or larger subnets and then
+// getDNSIP fetches the kubernetes service's ClusterIP and appends a "0" to it in order to get the DNS IP
-// kubeadm should look at the kubernetes service (e.g. 10.96.0.1 or 10.0.0.1) and just append a "0" at the end.
+func getDNSIP(client *clientset.Clientset) (net.IP, error) {
-// This way, we don't need the information about the subnet in this phase => good
+	k8ssvc, err := client.CoreV1().Services(metav1.NamespaceDefault).Get("kubernetes", metav1.GetOptions{})
-func getDNSIP(subnet string) (net.IP, error) {
-	_, n, err := net.ParseCIDR(subnet)
 	if err != nil {
-		return nil, fmt.Errorf("could not parse %q: %v", subnet, err)
+		return nil, fmt.Errorf("couldn't fetch information about the kubernetes service: %v", err)
 	}
-	ip, err := ipallocator.GetIndexedIP(n, 10)
+
-	if err != nil {
+	if len(k8ssvc.Spec.ClusterIP) == 0 {
-		return nil, fmt.Errorf("unable to allocate IP address for kube-dns addon from the given CIDR %q: [%v]", subnet, err)
+		return nil, fmt.Errorf("couldn't fetch a valid clusterIP from the kubernetes service")
 	}
-	return ip, nil
+
+	// Build an IP by taking the kubernetes service's clusterIP and appending a "0" and checking that it's valid
+	dnsIP := net.ParseIP(fmt.Sprintf("%s0", k8ssvc.Spec.ClusterIP))
+	if dnsIP == nil {
+		return nil, fmt.Errorf("could not parse dns ip %q: %v", dnsIP, err)
+	}
+	return dnsIP, nil
+}
+
+func getClusterCIDR(podsubnet string) string {
+	if len(podsubnet) == 0 {
+		return ""
+	}
+	return "--cluster-cidr" + podsubnet
 }

cmd/kubeadm/app/phases/addons/manifests.go

@@ -69,11 +69,13 @@ spec:
     spec:
       containers:
       - name: kube-proxy
-        image: {{ .ImageRepository }}/kube-proxy-{{ .Arch }}:{{ .Version }}
+        image: {{ .Image }}
         imagePullPolicy: IfNotPresent
+        # TODO: This is gonna work with hyperkube v1.6.0-alpha.2+: https://github.com/kubernetes/kubernetes/pull/41017
         command:
         - kube-proxy
         - --kubeconfig=/var/lib/kube-proxy/kubeconfig.conf
+        {{ .ClusterCIDR }}
         securityContext:
           privileged: true
         volumeMounts:

cmd/kubeadm/app/util/template.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2016 The Kubernetes Authors.
+Copyright 2017 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.