Merge pull request #85968 from liggitt/fix_bug_in_str_split

Fix bug in apiserver service cidr split

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"],
+)

cmd/kube-apiserver/app/server.go

@@ -572,45 +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
-
+	apiServerServiceIP, primaryServiceIPRange, secondaryServiceIPRange, err := getServiceIPAndRanges(s.ServiceClusterIPRanges)
-	serviceClusterIPRangeList := strings.Split(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
 	}
-
+	s.PrimaryServiceClusterIPRange = primaryServiceIPRange
-	if len(serviceClusterIPRangeList) > 0 {
+	s.SecondaryServiceClusterIPRange = secondaryServiceIPRange
-		_, 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
 
 	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)
@@ -716,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
+}

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", "<nil>", false},
+		{"192.0.2.1/24", "192.0.2.1", "192.0.2.0/24", "<nil>", 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", "<nil>", "<nil>", "<nil>", 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)
+		}
+	}
+}