diff --git a/pkg/controller/nodeipam/legacyprovider.go b/pkg/controller/nodeipam/legacyprovider.go index 8ad692bd5a8..00701a88afb 100644 --- a/pkg/controller/nodeipam/legacyprovider.go +++ b/pkg/controller/nodeipam/legacyprovider.go @@ -20,6 +20,7 @@ limitations under the License. package nodeipam import ( + "fmt" "net" coreinformers "k8s.io/client-go/informers/core/v1" @@ -40,7 +41,7 @@ func createLegacyIPAM( clusterCIDRs []*net.IPNet, serviceCIDR *net.IPNet, nodeCIDRMaskSizes []int, -) *ipam.Controller { +) (*ipam.Controller, error) { cfg := &ipam.Config{ Resync: ipamResyncInterval, MaxBackoff: ipamMaxBackoff, @@ -63,12 +64,10 @@ func createLegacyIPAM( } ipamc, err := ipam.NewController(cfg, kubeClient, cloud, cidr, serviceCIDR, nodeCIDRMaskSizes[0]) if err != nil { - logger.Error(err, "Error creating ipam controller") - klog.FlushAndExit(klog.ExitFlushTimeout, 1) + return nil, fmt.Errorf("error creating ipam controller: %w", err) } if err := ipamc.Start(logger, nodeInformer); err != nil { - logger.Error(err, "Error trying to Init()") - klog.FlushAndExit(klog.ExitFlushTimeout, 1) + return nil, fmt.Errorf("error trying to Init(): %w", err) } - return ipamc + return ipamc, nil } diff --git a/pkg/controller/nodeipam/node_ipam_controller.go b/pkg/controller/nodeipam/node_ipam_controller.go index 55b91b9d6fd..cf547f81d75 100644 --- a/pkg/controller/nodeipam/node_ipam_controller.go +++ b/pkg/controller/nodeipam/node_ipam_controller.go @@ -18,6 +18,7 @@ package nodeipam import ( "context" + "fmt" "net" "time" @@ -93,22 +94,19 @@ func NewNodeIpamController( logger := klog.FromContext(ctx) if kubeClient == nil { - logger.Error(nil, "kubeClient is nil when starting Controller") - klog.FlushAndExit(klog.ExitFlushTimeout, 1) + return nil, fmt.Errorf("kubeClient is nil when starting Controller") } // Cloud CIDR allocator does not rely on clusterCIDR or nodeCIDRMaskSize for allocation. if allocatorType != ipam.CloudAllocatorType { if len(clusterCIDRs) == 0 { - logger.Error(nil, "Controller: Must specify --cluster-cidr if --allocate-node-cidrs is set") - klog.FlushAndExit(klog.ExitFlushTimeout, 1) + return nil, fmt.Errorf("Controller: Must specify --cluster-cidr if --allocate-node-cidrs is set") } for idx, cidr := range clusterCIDRs { mask := cidr.Mask if maskSize, _ := mask.Size(); maskSize > nodeCIDRMaskSizes[idx] { - logger.Error(nil, "Controller: Invalid --cluster-cidr, mask size of cluster CIDR must be less than or equal to --node-cidr-mask-size configured for CIDR family") - klog.FlushAndExit(klog.ExitFlushTimeout, 1) + return nil, fmt.Errorf("Controller: Invalid --cluster-cidr, mask size of cluster CIDR must be less than or equal to --node-cidr-mask-size configured for CIDR family") } } } @@ -126,7 +124,11 @@ func NewNodeIpamController( // TODO: Abstract this check into a generic controller manager should run method. if ic.allocatorType == ipam.IPAMFromClusterAllocatorType || ic.allocatorType == ipam.IPAMFromCloudAllocatorType { - ic.legacyIPAM = createLegacyIPAM(logger, ic, nodeInformer, cloud, kubeClient, clusterCIDRs, serviceCIDR, nodeCIDRMaskSizes) + var err error + ic.legacyIPAM, err = createLegacyIPAM(logger, ic, nodeInformer, cloud, kubeClient, clusterCIDRs, serviceCIDR, nodeCIDRMaskSizes) + if err != nil { + return nil, err + } } else { var err error diff --git a/pkg/controller/nodeipam/node_ipam_controller_test.go b/pkg/controller/nodeipam/node_ipam_controller_test.go index 4bdeeeee28b..f15cf6b7336 100644 --- a/pkg/controller/nodeipam/node_ipam_controller_test.go +++ b/pkg/controller/nodeipam/node_ipam_controller_test.go @@ -21,20 +21,21 @@ package nodeipam import ( "context" + "errors" "net" - "os" - "os/exec" "strings" "testing" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" + featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/klog/v2/ktesting" - "k8s.io/kubernetes/pkg/controller" "k8s.io/kubernetes/pkg/controller/nodeipam/ipam" "k8s.io/kubernetes/pkg/controller/testutil" + "k8s.io/kubernetes/pkg/features" "k8s.io/legacy-cloud-providers/gce" netutils "k8s.io/utils/net" ) @@ -48,7 +49,7 @@ func newTestNodeIpamController(ctx context.Context, clusterCIDR []*net.IPNet, se Clientset: fake.NewSimpleClientset(), } fakeClient := &fake.Clientset{} - fakeInformerFactory := informers.NewSharedInformerFactory(fakeClient, controller.NoResyncPeriodFunc()) + fakeInformerFactory := informers.NewSharedInformerFactory(fakeClient, 0) fakeNodeInformer := fakeInformerFactory.Core().V1().Nodes() fakeClusterCIDRInformer := fakeInformerFactory.Networking().V1alpha1().ClusterCIDRs() @@ -75,51 +76,85 @@ func TestNewNodeIpamControllerWithCIDRMasks(t *testing.T) { secondaryServiceCIDR string maskSize []int allocatorType ipam.CIDRAllocatorType - wantFatal bool + expectedError error }{ - {"valid_range_allocator", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.RangeAllocatorType, false}, - - {"valid_range_allocator_dualstack", "10.0.0.0/21,2000::/10", "10.1.0.0/21", emptyServiceCIDR, []int{24, 98}, ipam.RangeAllocatorType, false}, - {"valid_range_allocator_dualstack_dualstackservice", "10.0.0.0/21,2000::/10", "10.1.0.0/21", "3000::/10", []int{24, 98}, ipam.RangeAllocatorType, false}, - - {"valid_multi_cidr_range_allocator", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.MultiCIDRRangeAllocatorType, false}, - {"valid_multi_cidr_range_allocator_dualstack", "10.0.0.0/21,2000::/10", "10.1.0.0/21", emptyServiceCIDR, []int{24, 98}, ipam.MultiCIDRRangeAllocatorType, false}, - - {"valid_cloud_allocator", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.CloudAllocatorType, false}, - {"valid_ipam_from_cluster", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromClusterAllocatorType, false}, - {"valid_ipam_from_cloud", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromCloudAllocatorType, false}, - {"valid_skip_cluster_CIDR_validation_for_cloud_allocator", "invalid", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.CloudAllocatorType, false}, - {"invalid_cluster_CIDR", "invalid", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromClusterAllocatorType, true}, - {"valid_CIDR_smaller_than_mask_cloud_allocator", "10.0.0.0/26", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.CloudAllocatorType, false}, - {"invalid_CIDR_smaller_than_mask_other_allocators", "10.0.0.0/26", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromCloudAllocatorType, true}, - {"invalid_serviceCIDR_contains_clusterCIDR", "10.0.0.0/23", "10.0.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromClusterAllocatorType, true}, - {"invalid_CIDR_mask_size", "10.0.0.0/24,2000::/64", "10.1.0.0/21", emptyServiceCIDR, []int{24, 48}, ipam.IPAMFromClusterAllocatorType, true}, + {"valid_range_allocator", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.RangeAllocatorType, nil}, + {"valid_range_allocator_dualstack", "10.0.0.0/21,2000::/48", "10.1.0.0/21", emptyServiceCIDR, []int{24, 64}, ipam.RangeAllocatorType, nil}, + {"valid_range_allocator_dualstack_dualstackservice", "10.0.0.0/21,2000::/48", "10.1.0.0/21", "3000::/112", []int{24, 64}, ipam.RangeAllocatorType, nil}, + {"valid_cloud_allocator", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.CloudAllocatorType, nil}, + {"valid_ipam_from_cluster", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromClusterAllocatorType, nil}, + {"valid_ipam_from_cloud", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromCloudAllocatorType, nil}, + {"valid_skip_cluster_CIDR_validation_for_cloud_allocator", "invalid", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.CloudAllocatorType, nil}, + {"valid_CIDR_larger_than_mask_cloud_allocator", "10.0.0.0/16", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.CloudAllocatorType, nil}, + {"invalid_cluster_CIDR", "", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromClusterAllocatorType, errors.New("Controller: Must specify --cluster-cidr if --allocate-node-cidrs is set")}, + {"invalid_CIDR_smaller_than_mask_other_allocators", "10.0.0.0/26", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromCloudAllocatorType, errors.New("Controller: Invalid --cluster-cidr, mask size of cluster CIDR must be less than or equal to --node-cidr-mask-size configured for CIDR family")}, + {"invalid_serviceCIDR_contains_clusterCIDR", "10.0.0.0/16", "10.0.0.0/8", emptyServiceCIDR, []int{24}, ipam.IPAMFromClusterAllocatorType, errors.New("error creating ipam controller: failed after occupy serviceCIDR: CIDR allocation failed; there are no remaining CIDRs left to allocate in the accepted range")}, + {"invalid_CIDR_mask_size", "10.0.0.0/24,2000::/64", "10.1.0.0/21", emptyServiceCIDR, []int{24, 48}, ipam.IPAMFromClusterAllocatorType, errors.New("Controller: Invalid --cluster-cidr, mask size of cluster CIDR must be less than or equal to --node-cidr-mask-size configured for CIDR family")}, } { + test := tc _, ctx := ktesting.NewTestContext(t) - t.Run(tc.desc, func(t *testing.T) { - clusterCidrs, _ := netutils.ParseCIDRs(strings.Split(tc.clusterCIDR, ",")) - _, serviceCIDRIpNet, _ := netutils.ParseCIDRSloppy(tc.serviceCIDR) - _, secondaryServiceCIDRIpNet, _ := netutils.ParseCIDRSloppy(tc.secondaryServiceCIDR) - - if os.Getenv("EXIT_ON_FATAL") == "1" { - // This is the subprocess which runs the actual code. - newTestNodeIpamController(ctx, clusterCidrs, serviceCIDRIpNet, secondaryServiceCIDRIpNet, tc.maskSize, tc.allocatorType) - return - } - // This is the host process that monitors the exit code of the subprocess. - cmd := exec.Command(os.Args[0], "-test.run=TestNewNodeIpamControllerWithCIDRMasks/"+tc.desc) - cmd.Env = append(os.Environ(), "EXIT_ON_FATAL=1") - err := cmd.Run() - var gotFatal bool + t.Run(test.desc, func(t *testing.T) { + t.Parallel() + clusterCidrs, err := netutils.ParseCIDRs(strings.Split(test.clusterCIDR, ",")) if err != nil { - exitErr, ok := err.(*exec.ExitError) - if !ok { - t.Fatalf("Failed to run subprocess: %v", err) - } - gotFatal = !exitErr.Success() + clusterCidrs = nil } - if gotFatal != tc.wantFatal { - t.Errorf("newTestNodeIpamController(%v, %v, %v, %v) : gotFatal = %t ; wantFatal = %t", clusterCidrs, serviceCIDRIpNet, tc.maskSize, tc.allocatorType, gotFatal, tc.wantFatal) + _, serviceCIDRIpNet, err := netutils.ParseCIDRSloppy(test.serviceCIDR) + if err != nil { + serviceCIDRIpNet = nil + } + _, secondaryServiceCIDRIpNet, err := netutils.ParseCIDRSloppy(test.secondaryServiceCIDR) + if err != nil { + secondaryServiceCIDRIpNet = nil + } + _, err = newTestNodeIpamController(ctx, clusterCidrs, serviceCIDRIpNet, secondaryServiceCIDRIpNet, test.maskSize, test.allocatorType) + if test.expectedError == nil { + if err != nil { + t.Errorf("Test %s, unexpected error: %v", test.desc, err) + } + } else { + if err.Error() != test.expectedError.Error() { + t.Errorf("Test %s, got error: %v, expected error: %v", test.desc, err, test.expectedError) + } + } + }) + } +} + +// MultiCIDRRangeAllocatorType need enable feature gate +func TestNewNodeIpamControllerWithCIDRMasks2(t *testing.T) { + emptyServiceCIDR := "" + for _, tc := range []struct { + desc string + clusterCIDR string + serviceCIDR string + secondaryServiceCIDR string + maskSize []int + allocatorType ipam.CIDRAllocatorType + }{ + {"valid_multi_cidr_range_allocator", "10.0.0.0/21", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.MultiCIDRRangeAllocatorType}, + {"valid_multi_cidr_range_allocator_dualstack", "10.0.0.0/21,2000::/48", "10.1.0.0/21", emptyServiceCIDR, []int{24, 64}, ipam.MultiCIDRRangeAllocatorType}, + } { + test := tc + _, ctx := ktesting.NewTestContext(t) + t.Run(test.desc, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.MultiCIDRRangeAllocator, true)() + + clusterCidrs, err := netutils.ParseCIDRs(strings.Split(test.clusterCIDR, ",")) + if err != nil { + clusterCidrs = nil + } + _, serviceCIDRIpNet, err := netutils.ParseCIDRSloppy(test.serviceCIDR) + if err != nil { + serviceCIDRIpNet = nil + } + _, secondaryServiceCIDRIpNet, err := netutils.ParseCIDRSloppy(test.secondaryServiceCIDR) + if err != nil { + secondaryServiceCIDRIpNet = nil + } + _, err = newTestNodeIpamController(ctx, clusterCidrs, serviceCIDRIpNet, secondaryServiceCIDRIpNet, test.maskSize, test.allocatorType) + if err != nil { + t.Errorf("Test %s, got error %v", test.desc, err) } }) } diff --git a/pkg/controller/nodeipam/nolegacyprovider.go b/pkg/controller/nodeipam/nolegacyprovider.go index dc71e9b717f..ba1578c3e7b 100644 --- a/pkg/controller/nodeipam/nolegacyprovider.go +++ b/pkg/controller/nodeipam/nolegacyprovider.go @@ -21,6 +21,7 @@ package nodeipam import ( "context" + "errors" "net" coreinformers "k8s.io/client-go/informers/core/v1" @@ -45,8 +46,6 @@ func createLegacyIPAM( clusterCIDRs []*net.IPNet, serviceCIDR *net.IPNet, nodeCIDRMaskSizes []int, -) *fakeController { - logger.Error(nil, "Error trying to Init(): legacy cloud provider support disabled at build time") - klog.FlushAndExit(klog.ExitFlushTimeout, 1) - return &fakeController{} +) (*fakeController, error) { + return nil, errors.New("Error trying to Init(): legacy cloud provider support disabled at build time") }