From 4b297fc1b542678964e4f7cbfe461cf5b6d2c6cc Mon Sep 17 00:00:00 2001 From: Shilpa-Gokul Date: Tue, 29 Oct 2024 21:03:30 +0530 Subject: [PATCH] Add validation in webhook --- api/v1beta2/ibmpowervscluster_webhook.go | 35 +++++- cloud/scope/powervs_cluster.go | 35 +++--- cloud/scope/powervs_cluster_test.go | 113 ++++++++---------- .../ibmpowervscluster_controller_test.go | 7 +- pkg/cloud/services/vpc/mock/vpc_generated.go | 15 --- pkg/cloud/services/vpc/service.go | 44 ------- pkg/cloud/services/vpc/vpc.go | 1 - util/util.go | 21 ---- 8 files changed, 99 insertions(+), 172 deletions(-) diff --git a/api/v1beta2/ibmpowervscluster_webhook.go b/api/v1beta2/ibmpowervscluster_webhook.go index aa548a800..3f5700864 100644 --- a/api/v1beta2/ibmpowervscluster_webhook.go +++ b/api/v1beta2/ibmpowervscluster_webhook.go @@ -138,21 +138,48 @@ func (r *IBMPowerVSCluster) validateIBMPowerVSClusterLoadBalancerNames() (allErr } func (r *IBMPowerVSCluster) validateIBMPowerVSClusterVPCSubnetNames() (allErrs field.ErrorList) { - found := make(map[string]bool) + subnetFound := make(map[string]bool) + zoneFound := make(map[string]bool) for i, subnet := range r.Spec.VPCSubnets { if subnet.Name == nil { continue } - if found[*subnet.Name] { + if subnetFound[*subnet.Name] { allErrs = append(allErrs, field.Duplicate(field.NewPath("spec", fmt.Sprintf("vpcSubnets[%d]", i)), map[string]interface{}{"Name": *subnet.Name})) continue } - found[*subnet.Name] = true + subnetFound[*subnet.Name] = true + if subnet.Zone == nil { + continue + } + if zoneFound[*subnet.Zone] { + allErrs = append(allErrs, field.Duplicate(field.NewPath("spec", fmt.Sprintf("vpcSubnets[%d]", i)), map[string]interface{}{"Zone": *subnet.Zone})) + continue + } + zoneFound[*subnet.Zone] = true } return allErrs } +func (r *IBMPowerVSCluster) validateIBMPowerVSClusterVPCSubnets() (allErrs field.ErrorList) { + if err := r.validateIBMPowerVSClusterVPCSubnetNames(); err != nil { + allErrs = append(allErrs, err...) + } + + if r.Spec.VPC != nil && r.Spec.VPC.Region != nil { + vpcZones, err := regionUtil.VPCZonesForVPCRegion(*r.Spec.VPC.Region) + if err != nil { + ibmpowervsclusterlog.Error(fmt.Errorf("error validating vpc zones"), err.Error()) + return allErrs + } + if len(r.Spec.VPCSubnets) > len(vpcZones) { + allErrs = append(allErrs, &field.Error{Type: field.ErrorTypeInvalid, BadValue: "spec.vpcSubnets", Detail: fmt.Sprintf("%s vpc region supports only %d subnets but %d subnets were provided", *r.Spec.VPC.Region, len(vpcZones), len(r.Spec.VPCSubnets))}) + } + } + return allErrs +} + func (r *IBMPowerVSCluster) validateIBMPowerVSClusterTransitGateway() *field.Error { if r.Spec.Zone == nil && r.Spec.VPC == nil { return nil @@ -209,7 +236,7 @@ func (r *IBMPowerVSCluster) validateIBMPowerVSClusterCreateInfraPrereq() (allErr if r.Spec.ResourceGroup == nil { allErrs = append(allErrs, field.Invalid(field.NewPath("spec.resourceGroup"), r.Spec.ResourceGroup, "value of resource group is empty")) } - if err := r.validateIBMPowerVSClusterVPCSubnetNames(); err != nil { + if err := r.validateIBMPowerVSClusterVPCSubnets(); err != nil { allErrs = append(allErrs, err...) } diff --git a/cloud/scope/powervs_cluster.go b/cloud/scope/powervs_cluster.go index 3396663c0..3f0271504 100644 --- a/cloud/scope/powervs_cluster.go +++ b/cloud/scope/powervs_cluster.go @@ -25,7 +25,6 @@ import ( "github.com/go-logr/logr" regionUtil "github.com/ppc64le-cloud/powervs-utils" - "k8s.io/klog/v2" "github.com/IBM-Cloud/power-go-client/ibmpisession" "github.com/IBM-Cloud/power-go-client/power/models" @@ -40,6 +39,7 @@ import ( "github.com/IBM/vpc-go-sdk/vpcv1" kerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/klog/v2" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -74,7 +74,12 @@ var ( ) // powerEdgeRouter is identifier for PER. -const powerEdgeRouter = "power-edge-router" +const ( + powerEdgeRouter = "power-edge-router" + // vpcSubnetIPAddressCount is the total IP Addresses for the subnet. + // Support for custom address prefixes will be added at a later time. Currently, we use the ip count for subnet creation. + vpcSubnetIPAddressCount int64 = 256 +) // PowerVSClusterScopeParams defines the input parameters used to create a new PowerVSClusterScope. type PowerVSClusterScopeParams struct { @@ -1123,9 +1128,6 @@ func (s *PowerVSClusterScope) ReconcileVPCSubnets() (bool, error) { } else { subnets = append(subnets, s.IBMPowerVSCluster.Spec.VPCSubnets...) } - if len(subnets) > len(vpcZones) { - return false, fmt.Errorf("error validating input, total vpc subnets to be created cannot be more than the available vpc zones. subnets-%d, vpczones-%d", len(subnets), len(vpcZones)) - } for index, subnet := range subnets { s.Info("Reconciling VPC subnet", "subnet", subnet) @@ -1167,21 +1169,19 @@ func (s *PowerVSClusterScope) ReconcileVPCSubnets() (bool, error) { // check for next subnet continue } - var zone string - if subnet.Zone != nil { - zone = *subnet.Zone - } else { - zone = vpcZones[index] + + if subnet.Zone == nil { + subnet.Zone = &vpcZones[index] } - s.V(3).Info("Creating VPC subnet-shilpa") - subnetID, err = s.createVPCSubnet(subnet, zone) + s.V(3).Info("Creating VPC subnet") + subnetID, err = s.createVPCSubnet(subnet) if err != nil { s.Error(err, "failed to create VPC subnet") return false, err } - s.Info("Created VPC subnet", "id", subnetID) + s.Info("Created VPC subnet", "subnetID", subnetID) s.SetVPCSubnetStatus(*subnet.Name, infrav1beta2.ResourceReference{ID: subnetID, ControllerCreated: ptr.To(true)}) - // Requeue only when all subnets' creation are triggered + // Requeue only when the creation of all subnets has been triggered. if index == len(subnets)-1 { return true, nil } @@ -1203,7 +1203,7 @@ func (s *PowerVSClusterScope) checkVPCSubnet(subnetName string) (string, error) } // createVPCSubnet creates a VPC subnet. -func (s *PowerVSClusterScope) createVPCSubnet(subnet infrav1beta2.Subnet, zone string) (*string, error) { +func (s *PowerVSClusterScope) createVPCSubnet(subnet infrav1beta2.Subnet) (*string, error) { // TODO(karthik-k-n): consider moving to clusterscope // fetch resource group id resourceGroupID := s.GetResourceGroupID() @@ -1217,19 +1217,18 @@ func (s *PowerVSClusterScope) createVPCSubnet(subnet infrav1beta2.Subnet, zone s return nil, fmt.Errorf("VPC ID is empty") } - var ipCount int64 = 256 ipVersion := vpcSubnetIPVersion4 options := &vpcv1.CreateSubnetOptions{} options.SetSubnetPrototype(&vpcv1.SubnetPrototype{ IPVersion: &ipVersion, - TotalIpv4AddressCount: ptr.To(ipCount), + TotalIpv4AddressCount: ptr.To(vpcSubnetIPAddressCount), Name: subnet.Name, VPC: &vpcv1.VPCIdentity{ ID: vpcID, }, Zone: &vpcv1.ZoneIdentity{ - Name: &zone, + Name: subnet.Zone, }, ResourceGroup: &vpcv1.ResourceGroupIdentity{ ID: &resourceGroupID, diff --git a/cloud/scope/powervs_cluster_test.go b/cloud/scope/powervs_cluster_test.go index b8eba5de2..dbe11b452 100644 --- a/cloud/scope/powervs_cluster_test.go +++ b/cloud/scope/powervs_cluster_test.go @@ -3364,6 +3364,7 @@ func TestReconcileVPCSubnets(t *testing.T) { IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ ObjectMeta: metav1.ObjectMeta{Name: "ClusterName"}, Spec: infrav1beta2.IBMPowerVSClusterSpec{ + VPC: &infrav1beta2.VPCResourceReference{Region: ptr.To("eu-de")}, VPCSubnets: []infrav1beta2.Subnet{{ID: ptr.To("subnet1ID"), Name: ptr.To("subnet1Name")}, {ID: ptr.To("subnet2ID"), Name: ptr.To("subnet2Name")}}}, }, } @@ -3379,6 +3380,7 @@ func TestReconcileVPCSubnets(t *testing.T) { g.Expect(clusterScope.IBMPowerVSCluster.Status.VPCSubnet[*subnet1Details.Name].ID).To(Equal(subnet1Details.ID)) g.Expect(clusterScope.IBMPowerVSCluster.Status.VPCSubnet[*subnet2Details.Name].ID).To(Equal(subnet2Details.ID)) g.Expect(clusterScope.IBMPowerVSCluster.Status.VPCSubnet[*subnet1Details.Name].ControllerCreated).To(BeNil()) + g.Expect(clusterScope.IBMPowerVSCluster.Status.VPCSubnet[*subnet2Details.Name].ControllerCreated).To(BeNil()) }) t.Run("When VPCSubnets are not set in spec and subnet doesnot exist in cloud", func(t *testing.T) { @@ -3399,13 +3401,41 @@ func TestReconcileVPCSubnets(t *testing.T) { }, } subnet1Details := &vpcv1.Subnet{ID: ptr.To("subnet1ID"), Name: ptr.To("ClusterName-vpcsubnet-eu-de-1")} + mockVPC.EXPECT().GetVPCSubnetByName(gomock.Any()).Return(nil, nil).Times(3) + mockVPC.EXPECT().CreateSubnet(gomock.Any()).Return(subnet1Details, nil, nil).Times(3) + requeue, err := clusterScope.ReconcileVPCSubnets() + g.Expect(requeue).To(BeTrue()) + g.Expect(err).To(BeNil()) + g.Expect(len(clusterScope.IBMPowerVSCluster.Status.VPCSubnet)).To(Equal(3)) + g.Expect(clusterScope.IBMPowerVSCluster.Status.VPCSubnet[*subnet1Details.Name].ID).To(Equal(subnet1Details.ID)) + g.Expect(*clusterScope.IBMPowerVSCluster.Status.VPCSubnet[*subnet1Details.Name].ControllerCreated).To(BeTrue()) + }) + + t.Run("When VPCSubnets are set in spec but zone is not specified", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + clusterScope := PowerVSClusterScope{ + IBMVPCClient: mockVPC, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "ClusterName"}, + Status: infrav1beta2.IBMPowerVSClusterStatus{ + VPC: &infrav1beta2.ResourceReference{ID: ptr.To("vpcID")}, + }, + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ResourceGroup: &infrav1beta2.IBMPowerVSResourceReference{ID: ptr.To("resourceGroupID")}, + VPC: &infrav1beta2.VPCResourceReference{Region: ptr.To("eu-de")}, + VPCSubnets: []infrav1beta2.Subnet{{Name: ptr.To("subnet1Name")}}}, + }, + } + subnet1Details := &vpcv1.Subnet{ID: ptr.To("subnet1ID"), Name: ptr.To("subnet1Name")} mockVPC.EXPECT().GetVPCSubnetByName(gomock.Any()).Return(nil, nil) - mockVPC.EXPECT().GetSubnetAddrPrefix(gomock.Any(), "eu-de-1").Return("10.240.20.0/18", nil) mockVPC.EXPECT().CreateSubnet(gomock.Any()).Return(subnet1Details, nil, nil) + requeue, err := clusterScope.ReconcileVPCSubnets() g.Expect(requeue).To(BeTrue()) g.Expect(err).To(BeNil()) - g.Expect(len(clusterScope.IBMPowerVSCluster.Status.VPCSubnet)).To(Equal(1)) g.Expect(clusterScope.IBMPowerVSCluster.Status.VPCSubnet[*subnet1Details.Name].ID).To(Equal(subnet1Details.ID)) g.Expect(*clusterScope.IBMPowerVSCluster.Status.VPCSubnet[*subnet1Details.Name].ControllerCreated).To(BeTrue()) }) @@ -3441,7 +3471,7 @@ func TestReconcileVPCSubnets(t *testing.T) { g.Expect(requeue).To(BeFalse()) g.Expect(err).ToNot(BeNil()) }) - t.Run("When subnetID and subnetName is nil", func(t *testing.T) { + t.Run("When subnetID and subnetName is nil and vpc subnet exists in cloud", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) @@ -3451,7 +3481,9 @@ func TestReconcileVPCSubnets(t *testing.T) { IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ ObjectMeta: metav1.ObjectMeta{Name: "ClusterName"}, Spec: infrav1beta2.IBMPowerVSClusterSpec{ - VPCSubnets: []infrav1beta2.Subnet{{}}}, + VPC: &infrav1beta2.VPCResourceReference{Region: ptr.To("eu-de")}, + VPCSubnets: []infrav1beta2.Subnet{{Zone: ptr.To("eu-de-1")}}, + }, Status: infrav1beta2.IBMPowerVSClusterStatus{ VPCSubnet: map[string]infrav1beta2.ResourceReference{ "subnet1Name": {ID: ptr.To("subnet1ID"), ControllerCreated: ptr.To(true)}, @@ -3460,7 +3492,7 @@ func TestReconcileVPCSubnets(t *testing.T) { } vpcSubnet1Name := fmt.Sprintf("%s-vpcsubnet-0", clusterScope.IBMPowerVSCluster.ObjectMeta.Name) subnet1Details := &vpcv1.Subnet{ID: ptr.To("subnet1ID"), Name: &vpcSubnet1Name} - mockVPC.EXPECT().GetVPCSubnetByName(vpcSubnet1Name).Return(subnet1Details, nil) + mockVPC.EXPECT().GetVPCSubnetByName(gomock.Any()).Return(subnet1Details, nil) requeue, err := clusterScope.ReconcileVPCSubnets() g.Expect(requeue).To(BeFalse()) @@ -3479,6 +3511,7 @@ func TestReconcileVPCSubnets(t *testing.T) { IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ ObjectMeta: metav1.ObjectMeta{Name: "ClusterName"}, Spec: infrav1beta2.IBMPowerVSClusterSpec{ + VPC: &infrav1beta2.VPCResourceReference{Region: ptr.To("eu-de")}, VPCSubnets: []infrav1beta2.Subnet{ { ID: ptr.To("subnet1ID"), @@ -3504,6 +3537,7 @@ func TestReconcileVPCSubnets(t *testing.T) { IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ ObjectMeta: metav1.ObjectMeta{Name: "ClusterName"}, Spec: infrav1beta2.IBMPowerVSClusterSpec{ + VPC: &infrav1beta2.VPCResourceReference{Region: ptr.To("eu-de")}, VPCSubnets: []infrav1beta2.Subnet{ { ID: ptr.To("subnet1ID"), @@ -3529,7 +3563,8 @@ func TestReconcileVPCSubnets(t *testing.T) { IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ ObjectMeta: metav1.ObjectMeta{Name: "ClusterName"}, Spec: infrav1beta2.IBMPowerVSClusterSpec{ - VPCSubnets: []infrav1beta2.Subnet{{}}}, + VPC: &infrav1beta2.VPCResourceReference{Region: ptr.To("eu-de")}, + }, }, } mockVPC.EXPECT().GetVPCSubnetByName(gomock.Any()).Return(nil, fmt.Errorf("GetVPCSubnetByName returns error")) @@ -3548,7 +3583,8 @@ func TestReconcileVPCSubnets(t *testing.T) { IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ ObjectMeta: metav1.ObjectMeta{Name: "ClusterName"}, Spec: infrav1beta2.IBMPowerVSClusterSpec{ - VPCSubnets: []infrav1beta2.Subnet{{}}}, + VPC: &infrav1beta2.VPCResourceReference{Region: ptr.To("eu-de")}, + }, }, } mockVPC.EXPECT().GetVPCSubnetByName(gomock.Any()).Return(nil, nil) @@ -3571,7 +3607,7 @@ func TestSetVPCSubnetStatus(t *testing.T) { clusterScope: PowerVSClusterScope{ IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{}, }, - resource: infrav1beta2.ResourceReference{ID: ptr.To("ID1")}, + resource: infrav1beta2.ResourceReference{ID: ptr.To("ID12")}, }, { name: "VPC subnet status is not nil", @@ -3581,7 +3617,7 @@ func TestSetVPCSubnetStatus(t *testing.T) { Status: infrav1beta2.IBMPowerVSClusterStatus{ VPCSubnet: map[string]infrav1beta2.ResourceReference{ "subnet1Name": { - ControllerCreated: ptr.To(false), + ControllerCreated: ptr.To(true), }, }, }, @@ -3686,7 +3722,6 @@ func TestCreateVPCSubnet(t *testing.T) { subnet := infrav1beta2.Subnet{Name: ptr.To("ClusterName-vpcsubnet-eu-de-1"), Zone: ptr.To("eu-de-1")} subnet1Details := &vpcv1.Subnet{ID: ptr.To("subnet1ID"), Name: ptr.To("ClusterName-vpcsubnet-eu-de-1")} - mockVPC.EXPECT().GetSubnetAddrPrefix(*clusterScope.IBMPowerVSCluster.Status.VPC.ID, *subnet.Zone).Return("10.240.20.0/18", nil) mockVPC.EXPECT().CreateSubnet(gomock.Any()).Return(subnet1Details, nil, nil) subnetID, err := clusterScope.createVPCSubnet(subnet) g.Expect(subnetID).To(Equal(subnet1Details.ID)) @@ -3710,7 +3745,6 @@ func TestCreateVPCSubnet(t *testing.T) { subnet := infrav1beta2.Subnet{Name: ptr.To("ClusterName-vpcsubnet-eu-de-1")} subnet1Details := &vpcv1.Subnet{ID: ptr.To("subnet1ID"), Name: ptr.To("ClusterName-vpcsubnet-eu-de-1")} - mockVPC.EXPECT().GetSubnetAddrPrefix(*clusterScope.IBMPowerVSCluster.Status.VPC.ID, "eu-de-1").Return("10.240.20.0/18", nil) mockVPC.EXPECT().CreateSubnet(gomock.Any()).Return(subnet1Details, nil, nil) subnetID, err := clusterScope.createVPCSubnet(subnet) g.Expect(subnetID).To(Equal(subnet1Details.ID)) @@ -3740,30 +3774,13 @@ func TestCreateVPCSubnet(t *testing.T) { clusterScope := PowerVSClusterScope{ IBMVPCClient: mockVPC, IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Spec: infrav1beta2.IBMPowerVSClusterSpec{ResourceGroup: &infrav1beta2.IBMPowerVSResourceReference{ID: ptr.To("resourceGroupID")}}, Status: infrav1beta2.IBMPowerVSClusterStatus{}}, + Spec: infrav1beta2.IBMPowerVSClusterSpec{ResourceGroup: &infrav1beta2.IBMPowerVSResourceReference{ID: ptr.To("resourceGroupID")}}}, } subnet := infrav1beta2.Subnet{Name: ptr.To("ClusterName-vpcsubnet-eu-de-1"), Zone: ptr.To("eu-de-1")} subnetID, err := clusterScope.createVPCSubnet(subnet) g.Expect(subnetID).To(BeNil()) g.Expect(err).ToNot(BeNil()) }) - t.Run("When GetSubnetAddrPrefix returns error", func(t *testing.T) { - g := NewWithT(t) - setup(t) - t.Cleanup(teardown) - - clusterScope := PowerVSClusterScope{ - IBMVPCClient: mockVPC, - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Spec: infrav1beta2.IBMPowerVSClusterSpec{ResourceGroup: &infrav1beta2.IBMPowerVSResourceReference{ID: ptr.To("resourceGroupID")}}, - Status: infrav1beta2.IBMPowerVSClusterStatus{VPC: &infrav1beta2.ResourceReference{ID: ptr.To("vpcID")}}}, - } - subnet := infrav1beta2.Subnet{Name: ptr.To("ClusterName-vpcsubnet-eu-de-1"), Zone: ptr.To("eu-de-1")} - mockVPC.EXPECT().GetSubnetAddrPrefix(gomock.Any(), gomock.Any()).Return("", fmt.Errorf("GetSubnetAddrPrefix returns error")) - subnetID, err := clusterScope.createVPCSubnet(subnet) - g.Expect(subnetID).To(BeNil()) - g.Expect(err).ToNot(BeNil()) - }) t.Run("When CreateSubnet returns error", func(t *testing.T) { g := NewWithT(t) @@ -3777,8 +3794,7 @@ func TestCreateVPCSubnet(t *testing.T) { Status: infrav1beta2.IBMPowerVSClusterStatus{VPC: &infrav1beta2.ResourceReference{ID: ptr.To("vpcID")}}}, } subnet := infrav1beta2.Subnet{Name: ptr.To("ClusterName-vpcsubnet-eu-de-1"), Zone: ptr.To("eu-de-1")} - mockVPC.EXPECT().GetSubnetAddrPrefix(gomock.Any(), gomock.Any()).Return("10.10.1.10/24", nil) - mockVPC.EXPECT().CreateSubnet(gomock.Any()).Return(nil, nil, fmt.Errorf("GetSubnetAddrPrefix returns error")) + mockVPC.EXPECT().CreateSubnet(gomock.Any()).Return(nil, nil, fmt.Errorf("error creating subnet")) subnetID, err := clusterScope.createVPCSubnet(subnet) g.Expect(subnetID).To(BeNil()) g.Expect(err).ToNot(BeNil()) @@ -3795,46 +3811,11 @@ func TestCreateVPCSubnet(t *testing.T) { Status: infrav1beta2.IBMPowerVSClusterStatus{VPC: &infrav1beta2.ResourceReference{ID: ptr.To("vpcID")}}}, } subnet := infrav1beta2.Subnet{Name: ptr.To("ClusterName-vpcsubnet-eu-de-1"), Zone: ptr.To("eu-de-1")} - mockVPC.EXPECT().GetSubnetAddrPrefix(gomock.Any(), gomock.Any()).Return("10.10.1.10/24", nil) mockVPC.EXPECT().CreateSubnet(gomock.Any()).Return(nil, nil, nil) subnetID, err := clusterScope.createVPCSubnet(subnet) g.Expect(subnetID).To(BeNil()) g.Expect(err).ToNot(BeNil()) }) - t.Run("When VPCZonesForVPCRegion returns error", func(t *testing.T) { - g := NewWithT(t) - setup(t) - t.Cleanup(teardown) - - clusterScope := PowerVSClusterScope{ - IBMVPCClient: mockVPC, - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Spec: infrav1beta2.IBMPowerVSClusterSpec{ResourceGroup: &infrav1beta2.IBMPowerVSResourceReference{ID: ptr.To("resourceGroupID")}, - VPC: &infrav1beta2.VPCResourceReference{Region: ptr.To("aadd")}}, - }, - } - subnet := infrav1beta2.Subnet{Name: ptr.To("ClusterName-vpcsubnet-eu-de-1")} - subnetID, err := clusterScope.createVPCSubnet(subnet) - g.Expect(subnetID).To(BeNil()) - g.Expect(err).ToNot(BeNil()) - }) - t.Run("When VPCZonesForVPCRegion returns zero zones for the region set in spec", func(t *testing.T) { - g := NewWithT(t) - setup(t) - t.Cleanup(teardown) - - clusterScope := PowerVSClusterScope{ - IBMVPCClient: mockVPC, - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Spec: infrav1beta2.IBMPowerVSClusterSpec{ResourceGroup: &infrav1beta2.IBMPowerVSResourceReference{ID: ptr.To("resourceGroupID")}, - VPC: &infrav1beta2.VPCResourceReference{Region: ptr.To("")}}, - }, - } - subnet := infrav1beta2.Subnet{Name: ptr.To("ClusterName-vpcsubnet-eu-de-1")} - subnetID, err := clusterScope.createVPCSubnet(subnet) - g.Expect(subnetID).To(BeNil()) - g.Expect(err).ToNot(BeNil()) - }) } func TestPowerVSDeleteLoadBalancer(t *testing.T) { var ( diff --git a/controllers/ibmpowervscluster_controller_test.go b/controllers/ibmpowervscluster_controller_test.go index 4b5711487..7db1783a7 100644 --- a/controllers/ibmpowervscluster_controller_test.go +++ b/controllers/ibmpowervscluster_controller_test.go @@ -31,6 +31,7 @@ import ( tgapiv1 "github.com/IBM/networking-go-sdk/transitgatewayapisv1" "github.com/IBM/platform-services-go-sdk/resourcecontrollerv2" "github.com/IBM/vpc-go-sdk/vpcv1" + regionUtil "github.com/ppc64le-cloud/powervs-utils" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -1381,11 +1382,11 @@ func TestReconcileVPCResources(t *testing.T) { }, }, } + vpcZones, _ := regionUtil.VPCZonesForVPCRegion("us-south") mockVPC := vpcmock.NewMockVpc(gomock.NewController(t)) mockVPC.EXPECT().GetVPC(gomock.Any()).Return(&vpcv1.VPC{Status: ptr.To("active")}, nil, nil) - mockVPC.EXPECT().GetVPCSubnetByName(gomock.Any()).Return(nil, nil) - mockVPC.EXPECT().GetSubnetAddrPrefix(gomock.Any(), gomock.Any()).Return("cidr", nil) - mockVPC.EXPECT().CreateSubnet(gomock.Any()).Return(&vpcv1.Subnet{Status: ptr.To("active")}, nil, nil) + mockVPC.EXPECT().GetVPCSubnetByName(gomock.Any()).Return(nil, nil).Times(len(vpcZones)) + mockVPC.EXPECT().CreateSubnet(gomock.Any()).Return(&vpcv1.Subnet{Status: ptr.To("active")}, nil, nil).Times(len(vpcZones)) clusterScope.IBMVPCClient = mockVPC return clusterScope }, diff --git a/pkg/cloud/services/vpc/mock/vpc_generated.go b/pkg/cloud/services/vpc/mock/vpc_generated.go index cb49b89a8..bd7b52780 100644 --- a/pkg/cloud/services/vpc/mock/vpc_generated.go +++ b/pkg/cloud/services/vpc/mock/vpc_generated.go @@ -461,21 +461,6 @@ func (mr *MockVpcMockRecorder) GetSubnet(arg0 any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetSubnet", reflect.TypeOf((*MockVpc)(nil).GetSubnet), arg0) } -// GetSubnetAddrPrefix mocks base method. -func (m *MockVpc) GetSubnetAddrPrefix(vpcID, zone string) (string, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetSubnetAddrPrefix", vpcID, zone) - ret0, _ := ret[0].(string) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetSubnetAddrPrefix indicates an expected call of GetSubnetAddrPrefix. -func (mr *MockVpcMockRecorder) GetSubnetAddrPrefix(vpcID, zone any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetSubnetAddrPrefix", reflect.TypeOf((*MockVpc)(nil).GetSubnetAddrPrefix), vpcID, zone) -} - // GetSubnetPublicGateway mocks base method. func (m *MockVpc) GetSubnetPublicGateway(options *vpcv1.GetSubnetPublicGatewayOptions) (*vpcv1.PublicGateway, *core.DetailedResponse, error) { m.ctrl.T.Helper() diff --git a/pkg/cloud/services/vpc/service.go b/pkg/cloud/services/vpc/service.go index 5134edb17..fb5b6a5e4 100644 --- a/pkg/cloud/services/vpc/service.go +++ b/pkg/cloud/services/vpc/service.go @@ -390,50 +390,6 @@ func (s *Service) GetLoadBalancerByName(loadBalancerName string) (*vpcv1.LoadBal return loadBalancer, nil } -// GetSubnetAddrPrefix returns subnets address prefix. -func (s *Service) GetSubnetAddrPrefix(vpcID, zone string) (string, error) { - var addrPrefix *vpcv1.AddressPrefix - f := func(start string) (bool, string, error) { - // check for existing vpcAddressPrefixes - listVPCAddressPrefixesOptions := &vpcv1.ListVPCAddressPrefixesOptions{ - VPCID: &vpcID, - } - if start != "" { - listVPCAddressPrefixesOptions.Start = &start - } - - vpcAddressPrefixesList, _, err := s.ListVPCAddressPrefixes(listVPCAddressPrefixesOptions) - if err != nil { - return false, "", err - } - - if vpcAddressPrefixesList == nil { - return false, "", fmt.Errorf("vpcAddressPrefix list returned is nil") - } - - for i, addressPrefix := range vpcAddressPrefixesList.AddressPrefixes { - if (*addressPrefix.Zone.Name) == zone { - addrPrefix = &vpcAddressPrefixesList.AddressPrefixes[i] - return true, "", nil - } - } - - if vpcAddressPrefixesList.Next != nil && *vpcAddressPrefixesList.Next.Href != "" { - return false, *vpcAddressPrefixesList.Next.Href, nil - } - return true, "", nil - } - - if err := utils.PagingHelper(f); err != nil { - return "", err - } - - if addrPrefix != nil { - return *addrPrefix.CIDR, nil - } - return "", fmt.Errorf("not found a valid CIDR for VPC %s in zone %s", vpcID, zone) -} - // CreateSecurityGroup creates a new security group. func (s *Service) CreateSecurityGroup(options *vpcv1.CreateSecurityGroupOptions) (*vpcv1.SecurityGroup, *core.DetailedResponse, error) { return s.vpcService.CreateSecurityGroup(options) diff --git a/pkg/cloud/services/vpc/vpc.go b/pkg/cloud/services/vpc/vpc.go index 63202a47e..ec4242ed1 100644 --- a/pkg/cloud/services/vpc/vpc.go +++ b/pkg/cloud/services/vpc/vpc.go @@ -62,7 +62,6 @@ type Vpc interface { GetSubnet(*vpcv1.GetSubnetOptions) (*vpcv1.Subnet, *core.DetailedResponse, error) GetVPCSubnetByName(subnetName string) (*vpcv1.Subnet, error) GetLoadBalancerByName(loadBalancerName string) (*vpcv1.LoadBalancer, error) - GetSubnetAddrPrefix(vpcID, zone string) (string, error) CreateSecurityGroup(options *vpcv1.CreateSecurityGroupOptions) (*vpcv1.SecurityGroup, *core.DetailedResponse, error) DeleteSecurityGroup(options *vpcv1.DeleteSecurityGroupOptions) (*core.DetailedResponse, error) ListSecurityGroups(options *vpcv1.ListSecurityGroupsOptions) (*vpcv1.SecurityGroupCollection, *core.DetailedResponse, error) diff --git a/util/util.go b/util/util.go index c42088d3b..552e3e797 100644 --- a/util/util.go +++ b/util/util.go @@ -18,12 +18,9 @@ package util import ( "fmt" - "math" - "net" regionUtil "github.com/ppc64le-cloud/powervs-utils" - "github.com/apparentlymart/go-cidr/cidr" "k8s.io/utils/ptr" "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/endpoints" @@ -50,21 +47,3 @@ func GetTransitGatewayLocationAndRouting(powerVSZone *string, vpcRegion *string) // since VPC region is not set and used PowerVS region to calculate the transit gateway location, hence returning local routing as default. return &location, ptr.To(false), nil } - -func GetSubnetAddr(networkNum int, addrPrefix string) (string, error) { - _, ipv4Net, err := net.ParseCIDR(addrPrefix) - if err != nil { - return "", fmt.Errorf("error parsing CIDR address prefix: %w", err) - } - mask, _ := ipv4Net.Mask.Size() - // totalIPAddresses defines the prefix length of the subnet to be created - // TODO: totalIPAddresses should be provided by user instead of hard coding - totalIPAddresses := 256 - subnetPrefixBits := 32 - int(math.Ceil(math.Log2(float64(totalIPAddresses)))) - subnet, err := cidr.Subnet(ipv4Net, subnetPrefixBits-mask, networkNum) - if err != nil { - return "", fmt.Errorf("error fetching subnet address: %w", err) - } - subnetAddr := fmt.Sprintf("%s/%d", subnet.IP, subnetPrefixBits) - return subnetAddr, nil -}