From 667dc64e79ca7b7896108e477b045f991173c5e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Mon, 6 Feb 2017 19:34:06 +0200 Subject: [PATCH 1/4] Validate the minimum subnet cidr so there are always 10 available addresses --- cmd/kubeadm/app/apis/kubeadm/validation/BUILD | 1 + .../app/apis/kubeadm/validation/validation.go | 18 ++++++++++++++++++ cmd/kubeadm/app/constants/constants.go | 4 ++++ 3 files changed, 23 insertions(+) diff --git a/cmd/kubeadm/app/apis/kubeadm/validation/BUILD b/cmd/kubeadm/app/apis/kubeadm/validation/BUILD index bbf096babf8..f1cc58fe170 100644 --- a/cmd/kubeadm/app/apis/kubeadm/validation/BUILD +++ b/cmd/kubeadm/app/apis/kubeadm/validation/BUILD @@ -13,6 +13,7 @@ go_library( tags = ["automanaged"], deps = [ "//cmd/kubeadm/app/apis/kubeadm:go_default_library", + "//cmd/kubeadm/app/constants:go_default_library", "//vendor:k8s.io/apimachinery/pkg/util/validation/field", ], ) diff --git a/cmd/kubeadm/app/apis/kubeadm/validation/validation.go b/cmd/kubeadm/app/apis/kubeadm/validation/validation.go index 3de5a0c0aa5..0fc78054112 100644 --- a/cmd/kubeadm/app/apis/kubeadm/validation/validation.go +++ b/cmd/kubeadm/app/apis/kubeadm/validation/validation.go @@ -17,13 +17,18 @@ limitations under the License. package validation import ( + "math" + "net" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm" + kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants" ) 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,16 @@ 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")} + } + cidrBytesMask, _ := svcSubnet.Mask.Size() + numAddresses := int32(math.Pow(2, float64(32-cidrBytesMask))) + if numAddresses < kubeadmconstants.MinimumAddressesInServiceSubnet { + return field.ErrorList{field.Invalid(fldPath, nil, "service subnet is too small")} + } + return field.ErrorList{} +} diff --git a/cmd/kubeadm/app/constants/constants.go b/cmd/kubeadm/app/constants/constants.go index fb69147aef4..b1ce9609428 100644 --- a/cmd/kubeadm/app/constants/constants.go +++ b/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 ) From 407722b378f6a10e34657e4cdf0a03f547a79d9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Mon, 6 Feb 2017 23:09:27 +0200 Subject: [PATCH 2/4] Remove an old proxy arg function, add clustercidr to the proxy manifest and automatically calculate the dns ip --- cmd/kubeadm/app/master/manifests.go | 6 --- cmd/kubeadm/app/master/manifests_test.go | 32 --------------- cmd/kubeadm/app/phases/addons/BUILD | 2 +- cmd/kubeadm/app/phases/addons/addons.go | 45 +++++++++++++--------- cmd/kubeadm/app/phases/addons/manifests.go | 4 +- cmd/kubeadm/app/util/template.go | 2 +- 6 files changed, 31 insertions(+), 60 deletions(-) diff --git a/cmd/kubeadm/app/master/manifests.go b/cmd/kubeadm/app/master/manifests.go index 0c3a993e2ae..e831277f2ea 100644 --- a/cmd/kubeadm/app/master/manifests.go +++ b/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() { diff --git a/cmd/kubeadm/app/master/manifests_test.go b/cmd/kubeadm/app/master/manifests_test.go index 181246b3b88..df04770f42b 100644 --- a/cmd/kubeadm/app/master/manifests_test.go +++ b/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], - ) - } - } - } -} diff --git a/cmd/kubeadm/app/phases/addons/BUILD b/cmd/kubeadm/app/phases/addons/BUILD index 293c8d873e9..0600c3e52ac 100644 --- a/cmd/kubeadm/app/phases/addons/BUILD +++ b/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", ], diff --git a/cmd/kubeadm/app/phases/addons/addons.go b/cmd/kubeadm/app/phases/addons/addons.go index b968106f7c9..9c4ec4ddd16 100644 --- a/cmd/kubeadm/app/phases/addons/addons.go +++ b/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 }{ - ImageRepository: kubeadmapi.GlobalEnvParams.RepositoryPrefix, - Arch: runtime.GOARCH, - // TODO: Fetch the version from the {API Server IP}/version - Version: cfg.KubernetesVersion, + proxyDaemonSetBytes, err := kubeadmutil.ParseTemplate(KubeProxyDaemonSet, struct{ Image, ClusterCIDR string }{ + Image: images.GetCoreImage("proxy", cfg, kubeadmapi.GlobalEnvParams.HyperkubeImage), + ClusterCIDR: getClusterCIDR(cfg.Networking.PodSubnet), }) 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(cfg.Networking.ServiceSubnet) + dnsip, err := getDNSIP(client) 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 -// 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. -// This way, we don't need the information about the subnet in this phase => good -func getDNSIP(subnet string) (net.IP, error) { - _, n, err := net.ParseCIDR(subnet) +// getDNSIP fetches the kubernetes service's ClusterIP and appends a "0" to it in order to get the DNS IP +func getDNSIP(client *clientset.Clientset) (net.IP, error) { + k8ssvc, err := client.CoreV1().Services(metav1.NamespaceDefault).Get("kubernetes", metav1.GetOptions{}) 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 { - return nil, fmt.Errorf("unable to allocate IP address for kube-dns addon from the given CIDR %q: [%v]", subnet, err) + + if len(k8ssvc.Spec.ClusterIP) == 0 { + 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 } diff --git a/cmd/kubeadm/app/phases/addons/manifests.go b/cmd/kubeadm/app/phases/addons/manifests.go index f79b7e111a7..661a8f31bfd 100644 --- a/cmd/kubeadm/app/phases/addons/manifests.go +++ b/cmd/kubeadm/app/phases/addons/manifests.go @@ -71,11 +71,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: diff --git a/cmd/kubeadm/app/util/template.go b/cmd/kubeadm/app/util/template.go index 7e1e8702c5f..164362c5551 100644 --- a/cmd/kubeadm/app/util/template.go +++ b/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. From 8cf23139e66c1fb211504e514c49912582f36767 Mon Sep 17 00:00:00 2001 From: Derek McQuay Date: Mon, 6 Feb 2017 14:40:52 -0800 Subject: [PATCH 3/4] kubeadm: tests for apis/kubeadn/validation pkg --- .../kubeadm/validation/validation_test.go | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go diff --git a/cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go b/cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go new file mode 100644 index 00000000000..5ea16980bb4 --- /dev/null +++ b/cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go @@ -0,0 +1,31 @@ +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 + {"192.0.2.0/1", nil, false}, // CIDR too smal + {"192.0.2.0/24", nil, true}, + } + 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), + ) + } + } +} From f6647fc15207f7ac6f6f0ce798d0408316d09bc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Tue, 7 Feb 2017 18:25:25 +0200 Subject: [PATCH 4/4] Update bazel, the validation test and use ipallocator.RangeSize --- cmd/kubeadm/app/apis/kubeadm/validation/BUILD | 10 +++++++++ .../app/apis/kubeadm/validation/validation.go | 5 ++--- .../kubeadm/validation/validation_test.go | 21 +++++++++++++++++-- 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/cmd/kubeadm/app/apis/kubeadm/validation/BUILD b/cmd/kubeadm/app/apis/kubeadm/validation/BUILD index f1cc58fe170..62a7ebc6608 100644 --- a/cmd/kubeadm/app/apis/kubeadm/validation/BUILD +++ b/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( @@ -14,6 +15,7 @@ go_library( 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", ], ) @@ -30,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"], +) diff --git a/cmd/kubeadm/app/apis/kubeadm/validation/validation.go b/cmd/kubeadm/app/apis/kubeadm/validation/validation.go index 0fc78054112..8afc92e82db 100644 --- a/cmd/kubeadm/app/apis/kubeadm/validation/validation.go +++ b/cmd/kubeadm/app/apis/kubeadm/validation/validation.go @@ -17,12 +17,12 @@ limitations under the License. package validation import ( - "math" "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 { @@ -79,8 +79,7 @@ func ValidateServiceSubnet(subnet string, fldPath *field.Path) field.ErrorList { if err != nil { return field.ErrorList{field.Invalid(fldPath, nil, "couldn't parse the service subnet")} } - cidrBytesMask, _ := svcSubnet.Mask.Size() - numAddresses := int32(math.Pow(2, float64(32-cidrBytesMask))) + numAddresses := ipallocator.RangeSize(svcSubnet) if numAddresses < kubeadmconstants.MinimumAddressesInServiceSubnet { return field.ErrorList{field.Invalid(fldPath, nil, "service subnet is too small")} } diff --git a/cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go b/cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go index 5ea16980bb4..8fb314efdec 100644 --- a/cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go +++ b/cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go @@ -1,3 +1,19 @@ +/* +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 ( @@ -15,8 +31,9 @@ func TestValidateServiceSubnet(t *testing.T) { {"", nil, false}, {"this is not a cidr", nil, false}, // not a CIDR {"10.0.0.1", nil, false}, // not a CIDR - {"192.0.2.0/1", nil, false}, // CIDR too smal - {"192.0.2.0/24", nil, true}, + {"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)