From fdd25ec96820c7d802c7ec83f5ff5c49ac20a61c Mon Sep 17 00:00:00 2001 From: darshanime Date: Thu, 5 Dec 2019 11:31:11 +0530 Subject: [PATCH 1/2] Fix bug in apiserver service cluster cidr split Signed-off-by: darshanime --- cmd/kube-apiserver/app/server.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cmd/kube-apiserver/app/server.go b/cmd/kube-apiserver/app/server.go index 91f99af1c87..c051bb753cf 100644 --- a/cmd/kube-apiserver/app/server.go +++ b/cmd/kube-apiserver/app/server.go @@ -572,8 +572,10 @@ func Complete(s *options.ServerRunOptions) (completedServerRunOptions, error) { // process s.ServiceClusterIPRange from list to Primary and Secondary // we process secondary only if provided by user - - serviceClusterIPRangeList := strings.Split(s.ServiceClusterIPRanges, ",") + serviceClusterIPRangeList := []string{} + if s.ServiceClusterIPRanges != "" { + serviceClusterIPRangeList = strings.Split(s.ServiceClusterIPRanges, ",") + } var apiServerServiceIP net.IP var serviceIPRange net.IPNet From f4d1674827eef37b5e08f63a64b7a7a105db5cb9 Mon Sep 17 00:00:00 2001 From: darshanime Date: Fri, 6 Dec 2019 00:31:58 +0530 Subject: [PATCH 2/2] Refactor parsing logic for service IP and ranges, add tests Signed-off-by: darshanime --- cmd/kube-apiserver/app/BUILD | 8 ++- cmd/kube-apiserver/app/server.go | 89 +++++++++++++++------------ cmd/kube-apiserver/app/server_test.go | 56 +++++++++++++++++ 3 files changed, 112 insertions(+), 41 deletions(-) create mode 100644 cmd/kube-apiserver/app/server_test.go diff --git a/cmd/kube-apiserver/app/BUILD b/cmd/kube-apiserver/app/BUILD index 66570c567e5..95162cf8760 100644 --- a/cmd/kube-apiserver/app/BUILD +++ b/cmd/kube-apiserver/app/BUILD @@ -1,4 +1,4 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", @@ -99,3 +99,9 @@ filegroup( tags = ["automanaged"], visibility = ["//visibility:public"], ) + +go_test( + name = "go_default_test", + srcs = ["server_test.go"], + embed = [":go_default_library"], +) diff --git a/cmd/kube-apiserver/app/server.go b/cmd/kube-apiserver/app/server.go index c051bb753cf..f12a55f8ac8 100644 --- a/cmd/kube-apiserver/app/server.go +++ b/cmd/kube-apiserver/app/server.go @@ -572,47 +572,12 @@ func Complete(s *options.ServerRunOptions) (completedServerRunOptions, error) { // process s.ServiceClusterIPRange from list to Primary and Secondary // we process secondary only if provided by user - serviceClusterIPRangeList := []string{} - if s.ServiceClusterIPRanges != "" { - serviceClusterIPRangeList = strings.Split(s.ServiceClusterIPRanges, ",") + apiServerServiceIP, primaryServiceIPRange, secondaryServiceIPRange, err := getServiceIPAndRanges(s.ServiceClusterIPRanges) + if err != nil { + return options, err } - - var apiServerServiceIP net.IP - var serviceIPRange net.IPNet - var err error - // nothing provided by user, use default range (only applies to the Primary) - if len(serviceClusterIPRangeList) == 0 { - var primaryServiceClusterCIDR net.IPNet - serviceIPRange, apiServerServiceIP, err = master.ServiceIPRange(primaryServiceClusterCIDR) - if err != nil { - return options, fmt.Errorf("error determining service IP ranges: %v", err) - } - s.PrimaryServiceClusterIPRange = serviceIPRange - } - - if len(serviceClusterIPRangeList) > 0 { - _, primaryServiceClusterCIDR, err := net.ParseCIDR(serviceClusterIPRangeList[0]) - if err != nil { - return options, fmt.Errorf("service-cluster-ip-range[0] is not a valid cidr") - } - - serviceIPRange, apiServerServiceIP, err = master.ServiceIPRange(*(primaryServiceClusterCIDR)) - if err != nil { - return options, fmt.Errorf("error determining service IP ranges for primary service cidr: %v", err) - } - s.PrimaryServiceClusterIPRange = serviceIPRange - } - - // user provided at least two entries - if len(serviceClusterIPRangeList) > 1 { - _, secondaryServiceClusterCIDR, err := net.ParseCIDR(serviceClusterIPRangeList[1]) - if err != nil { - return options, fmt.Errorf("service-cluster-ip-range[1] is not an ip net") - } - - s.SecondaryServiceClusterIPRange = *(secondaryServiceClusterCIDR) - } - //note: validation asserts that the list is max of two dual stack entries + s.PrimaryServiceClusterIPRange = primaryServiceIPRange + s.SecondaryServiceClusterIPRange = secondaryServiceIPRange if err := s.SecureServing.MaybeDefaultWithSelfSignedCerts(s.GenericServerRunOptions.AdvertiseAddress.String(), []string{"kubernetes.default.svc", "kubernetes.default", "kubernetes"}, []net.IP{apiServerServiceIP}); err != nil { return options, fmt.Errorf("error creating self-signed certificates: %v", err) @@ -718,3 +683,47 @@ func buildServiceResolver(enabledAggregatorRouting bool, hostname string, inform } return serviceResolver } + +func getServiceIPAndRanges(serviceClusterIPRanges string) (net.IP, net.IPNet, net.IPNet, error) { + serviceClusterIPRangeList := []string{} + if serviceClusterIPRanges != "" { + serviceClusterIPRangeList = strings.Split(serviceClusterIPRanges, ",") + } + + var apiServerServiceIP net.IP + var primaryServiceIPRange net.IPNet + var secondaryServiceIPRange net.IPNet + var err error + // nothing provided by user, use default range (only applies to the Primary) + if len(serviceClusterIPRangeList) == 0 { + var primaryServiceClusterCIDR net.IPNet + primaryServiceIPRange, apiServerServiceIP, err = master.ServiceIPRange(primaryServiceClusterCIDR) + if err != nil { + return net.IP{}, net.IPNet{}, net.IPNet{}, fmt.Errorf("error determining service IP ranges: %v", err) + } + return apiServerServiceIP, primaryServiceIPRange, net.IPNet{}, nil + } + + if len(serviceClusterIPRangeList) > 0 { + _, primaryServiceClusterCIDR, err := net.ParseCIDR(serviceClusterIPRangeList[0]) + if err != nil { + return net.IP{}, net.IPNet{}, net.IPNet{}, fmt.Errorf("service-cluster-ip-range[0] is not a valid cidr") + } + + primaryServiceIPRange, apiServerServiceIP, err = master.ServiceIPRange(*(primaryServiceClusterCIDR)) + if err != nil { + return net.IP{}, net.IPNet{}, net.IPNet{}, fmt.Errorf("error determining service IP ranges for primary service cidr: %v", err) + } + } + + // user provided at least two entries + // note: validation asserts that the list is max of two dual stack entries + if len(serviceClusterIPRangeList) > 1 { + _, secondaryServiceClusterCIDR, err := net.ParseCIDR(serviceClusterIPRangeList[1]) + if err != nil { + return net.IP{}, net.IPNet{}, net.IPNet{}, fmt.Errorf("service-cluster-ip-range[1] is not an ip net") + } + secondaryServiceIPRange = *secondaryServiceClusterCIDR + } + return apiServerServiceIP, primaryServiceIPRange, secondaryServiceIPRange, nil +} diff --git a/cmd/kube-apiserver/app/server_test.go b/cmd/kube-apiserver/app/server_test.go new file mode 100644 index 00000000000..6cf61fc25f9 --- /dev/null +++ b/cmd/kube-apiserver/app/server_test.go @@ -0,0 +1,56 @@ +/* +Copyright 2019 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 app + +import ( + "testing" +) + +func TestGetServiceIPAndRanges(t *testing.T) { + tests := []struct { + body string + apiServerServiceIP string + primaryServiceIPRange string + secondaryServiceIPRange string + expectedError bool + }{ + {"", "10.0.0.1", "10.0.0.0/24", "", false}, + {"192.0.2.1/24", "192.0.2.1", "192.0.2.0/24", "", false}, + {"192.0.2.1/24,192.168.128.0/17", "192.0.2.1", "192.0.2.0/24", "192.168.128.0/17", false}, + {"192.0.2.1/30,192.168.128.0/17", "", "", "", true}, + } + + for _, test := range tests { + apiServerServiceIP, primaryServiceIPRange, secondaryServiceIPRange, err := getServiceIPAndRanges(test.body) + + if apiServerServiceIP.String() != test.apiServerServiceIP { + t.Errorf("expected apiServerServiceIP: %s, got: %s", test.apiServerServiceIP, apiServerServiceIP.String()) + } + + if primaryServiceIPRange.String() != test.primaryServiceIPRange { + t.Errorf("expected primaryServiceIPRange: %s, got: %s", test.primaryServiceIPRange, primaryServiceIPRange.String()) + } + + if secondaryServiceIPRange.String() != test.secondaryServiceIPRange { + t.Errorf("expected secondaryServiceIPRange: %s, got: %s", test.secondaryServiceIPRange, secondaryServiceIPRange.String()) + } + + if (err == nil) == test.expectedError { + t.Errorf("expected err to be: %t, but it was %t", test.expectedError, !test.expectedError) + } + } +}