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 47421d1ede9..f15cf6b7336 100644 --- a/pkg/controller/nodeipam/node_ipam_controller_test.go +++ b/pkg/controller/nodeipam/node_ipam_controller_test.go @@ -21,6 +21,7 @@ package nodeipam import ( "context" + "errors" "net" "strings" "testing" @@ -31,7 +32,7 @@ import ( "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" featuregatetesting "k8s.io/component-base/featuregate/testing" - "k8s.io/klog/v2" + "k8s.io/klog/v2/ktesting" "k8s.io/kubernetes/pkg/controller/nodeipam/ipam" "k8s.io/kubernetes/pkg/controller/testutil" "k8s.io/kubernetes/pkg/features" @@ -75,24 +76,24 @@ 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::/48", "10.1.0.0/21", emptyServiceCIDR, []int{24, 64}, ipam.RangeAllocatorType, false}, - {"valid_range_allocator_dualstack_dualstackservice", "10.0.0.0/21,2000::/48", "10.1.0.0/21", "3000::/112", []int{24, 64}, ipam.RangeAllocatorType, 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}, - {"valid_CIDR_larger_than_mask_cloud_allocator", "10.0.0.0/16", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.CloudAllocatorType, false}, - {"invalid_cluster_CIDR", "", "10.1.0.0/21", emptyServiceCIDR, []int{24}, ipam.IPAMFromClusterAllocatorType, true}, - {"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/16", "10.0.0.0/8", 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(test.desc, func(t *testing.T) { - klog.Warningf("Test case start: %s", test.desc) t.Parallel() clusterCidrs, err := netutils.ParseCIDRs(strings.Split(test.clusterCIDR, ",")) if err != nil { @@ -106,20 +107,15 @@ func TestNewNodeIpamControllerWithCIDRMasks(t *testing.T) { if err != nil { secondaryServiceCIDRIpNet = nil } - // This is the subprocess which runs the actual code. - defer func() { - r := recover() - if r == nil && test.wantFatal { - t.Errorf("Test %s, the code did not panic", test.desc) - } else if r != nil && !test.wantFatal { - t.Errorf("Test %s, the code did panic", test.desc) + _, 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) } - }() - _, err = newTestNodeIpamController(clusterCidrs, serviceCIDRIpNet, secondaryServiceCIDRIpNet, test.maskSize, test.allocatorType) - // The code is inconsistent about returning error or panic - // but we have to keep both ways of existing for backwards compatibility - if (err != nil) != test.wantFatal { - t.Errorf("Test %s,Got error %v expected %v", test.desc, err, test.wantFatal) } }) } @@ -135,14 +131,13 @@ func TestNewNodeIpamControllerWithCIDRMasks2(t *testing.T) { secondaryServiceCIDR string maskSize []int allocatorType ipam.CIDRAllocatorType - wantFatal bool }{ - {"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::/48", "10.1.0.0/21", emptyServiceCIDR, []int{24, 64}, ipam.MultiCIDRRangeAllocatorType, false}, + {"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) { - klog.Warningf("Test case start: %s", test.desc) defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.MultiCIDRRangeAllocator, true)() clusterCidrs, err := netutils.ParseCIDRs(strings.Split(test.clusterCIDR, ",")) @@ -157,20 +152,9 @@ func TestNewNodeIpamControllerWithCIDRMasks2(t *testing.T) { if err != nil { secondaryServiceCIDRIpNet = nil } - // This is the subprocess which runs the actual code. - defer func() { - r := recover() - if r == nil && test.wantFatal { - t.Errorf("Test %s, the code did not panic", test.desc) - } else if r != nil && !test.wantFatal { - t.Errorf("Test %s, the code did panic", test.desc) - } - }() - _, err = newTestNodeIpamController(clusterCidrs, serviceCIDRIpNet, secondaryServiceCIDRIpNet, test.maskSize, test.allocatorType) - // The code is inconsistent about returning error or panic - // but we have to keep both ways of existing for backwards compatibility - if (err != nil) != test.wantFatal { - t.Errorf("Test %s,Got error %v expected %v", test.desc, err, test.wantFatal) + _, 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") }