From ec8b9dbd0c37488521e4b2052148f0e5dd982b42 Mon Sep 17 00:00:00 2001 From: Amulyam24 Date: Wed, 28 Aug 2024 17:05:12 +0530 Subject: [PATCH] Refactor reconcilation of resources --- api/v1beta2/ibmpowervscluster_webhook.go | 32 +- cloud/scope/powervs_cluster.go | 276 +++--- cloud/scope/powervs_cluster_test.go | 984 +++++++++++--------- controllers/ibmpowervscluster_controller.go | 14 +- 4 files changed, 743 insertions(+), 563 deletions(-) diff --git a/api/v1beta2/ibmpowervscluster_webhook.go b/api/v1beta2/ibmpowervscluster_webhook.go index 52ce0086c..085a76cd2 100644 --- a/api/v1beta2/ibmpowervscluster_webhook.go +++ b/api/v1beta2/ibmpowervscluster_webhook.go @@ -102,11 +102,34 @@ func (r *IBMPowerVSCluster) validateIBMPowerVSClusterNetwork() *field.Error { return nil } +func (r *IBMPowerVSCluster) validateIBMPowerVSClusterLoadBalancers() (allErrs field.ErrorList) { + if err := r.validateIBMPowerVSClusterLoadBalancerNames(); err != nil { + allErrs = append(allErrs, err...) + } + + if len(r.Spec.LoadBalancers) == 0 { + return allErrs + } + + for _, loadbalancer := range r.Spec.LoadBalancers { + if *loadbalancer.Public { + return allErrs + } + } + + return append(allErrs, field.Invalid(field.NewPath("spec.LoadBalancers"), r.Spec.LoadBalancers, "Expect atleast one of the load balancer to be public")) +} + func (r *IBMPowerVSCluster) validateIBMPowerVSClusterLoadBalancerNames() (allErrs field.ErrorList) { found := make(map[string]bool) for i, loadbalancer := range r.Spec.LoadBalancers { + if loadbalancer.Name == "" { + continue + } + if found[loadbalancer.Name] { allErrs = append(allErrs, field.Duplicate(field.NewPath("spec", fmt.Sprintf("loadbalancers[%d]", i)), map[string]interface{}{"Name": loadbalancer.Name})) + continue } found[loadbalancer.Name] = true } @@ -117,8 +140,12 @@ func (r *IBMPowerVSCluster) validateIBMPowerVSClusterLoadBalancerNames() (allErr func (r *IBMPowerVSCluster) validateIBMPowerVSClusterVPCSubnetNames() (allErrs field.ErrorList) { found := make(map[string]bool) for i, subnet := range r.Spec.VPCSubnets { + if subnet.Name == nil { + continue + } if found[*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 } @@ -130,6 +157,9 @@ func (r *IBMPowerVSCluster) validateIBMPowerVSClusterTransitGateway() *field.Err if r.Spec.Zone == nil && r.Spec.VPC == nil { return nil } + if r.Spec.TransitGateway == nil { + return nil + } if _, globalRouting, _ := genUtil.GetTransitGatewayLocationAndRouting(r.Spec.Zone, r.Spec.VPC.Region); r.Spec.TransitGateway.GlobalRouting != nil && !*r.Spec.TransitGateway.GlobalRouting && globalRouting != nil && *globalRouting { return field.Invalid(field.NewPath("spec.transitGateway.globalRouting"), r.Spec.TransitGateway.GlobalRouting, "global routing is required since PowerVS and VPC region are from different region") } @@ -183,7 +213,7 @@ func (r *IBMPowerVSCluster) validateIBMPowerVSClusterCreateInfraPrereq() (allErr allErrs = append(allErrs, err...) } - if err := r.validateIBMPowerVSClusterLoadBalancerNames(); err != nil { + if err := r.validateIBMPowerVSClusterLoadBalancers(); err != nil { allErrs = append(allErrs, err...) } diff --git a/cloud/scope/powervs_cluster.go b/cloud/scope/powervs_cluster.go index af826943a..763c2f67d 100644 --- a/cloud/scope/powervs_cluster.go +++ b/cloud/scope/powervs_cluster.go @@ -384,14 +384,8 @@ func (s *PowerVSClusterScope) ServiceInstance() *infrav1beta2.IBMPowerVSResource return s.IBMPowerVSCluster.Spec.ServiceInstance } -// GetServiceInstanceID get the service instance id. +// GetServiceInstanceID returns service instance id set in status field of IBMPowerVSCluster object. If it doesn't exist, returns empty string. func (s *PowerVSClusterScope) GetServiceInstanceID() string { - if s.IBMPowerVSCluster.Spec.ServiceInstanceID != "" { - return s.IBMPowerVSCluster.Spec.ServiceInstanceID - } - if s.IBMPowerVSCluster.Spec.ServiceInstance != nil && s.IBMPowerVSCluster.Spec.ServiceInstance.ID != nil { - return *s.IBMPowerVSCluster.Spec.ServiceInstance.ID - } if s.IBMPowerVSCluster.Status.ServiceInstance != nil && s.IBMPowerVSCluster.Status.ServiceInstance.ID != nil { return *s.IBMPowerVSCluster.Status.ServiceInstance.ID } @@ -470,11 +464,8 @@ func (s *PowerVSClusterScope) Network() *infrav1beta2.IBMPowerVSResourceReferenc return &s.IBMPowerVSCluster.Spec.Network } -// GetDHCPServerID returns the DHCP id from spec or status of IBMPowerVSCluster object. +// GetDHCPServerID returns the DHCP id from status of IBMPowerVSCluster object. If it doesn't exist, returns nil. func (s *PowerVSClusterScope) GetDHCPServerID() *string { - if s.IBMPowerVSCluster.Spec.DHCPServer != nil && s.IBMPowerVSCluster.Spec.DHCPServer.ID != nil { - return s.IBMPowerVSCluster.Spec.DHCPServer.ID - } if s.IBMPowerVSCluster.Status.DHCPServer != nil { return s.IBMPowerVSCluster.Status.DHCPServer.ID } @@ -491,11 +482,8 @@ func (s *PowerVSClusterScope) VPC() *infrav1beta2.VPCResourceReference { return s.IBMPowerVSCluster.Spec.VPC } -// GetVPCID returns the VPC id. +// GetVPCID returns the VPC id set in status field of IBMPowerVSCluster object. If it doesn't exist, returns nil. func (s *PowerVSClusterScope) GetVPCID() *string { - if s.IBMPowerVSCluster.Spec.VPC != nil && s.IBMPowerVSCluster.Spec.VPC.ID != nil { - return s.IBMPowerVSCluster.Spec.VPC.ID - } if s.IBMPowerVSCluster.Status.VPC != nil { return s.IBMPowerVSCluster.Status.VPC.ID } @@ -516,15 +504,6 @@ func (s *PowerVSClusterScope) GetVPCSubnetID(subnetName string) *string { // GetVPCSubnetIDs returns all the VPC subnet ids. func (s *PowerVSClusterScope) GetVPCSubnetIDs() []*string { subnets := []*string{} - // use the vpc subnet id set by user. - for _, subnet := range s.IBMPowerVSCluster.Spec.VPCSubnets { - if subnet.ID != nil { - subnets = append(subnets, subnet.ID) - } - } - if len(subnets) != 0 { - return subnets - } if s.IBMPowerVSCluster.Status.VPCSubnet == nil { return nil } @@ -534,8 +513,8 @@ func (s *PowerVSClusterScope) GetVPCSubnetIDs() []*string { return subnets } -// SetVPCSubnetID set the VPC subnet id. -func (s *PowerVSClusterScope) SetVPCSubnetID(name string, resource infrav1beta2.ResourceReference) { +// SetVPCSubnetStatus set the VPC subnet id. +func (s *PowerVSClusterScope) SetVPCSubnetStatus(name string, resource infrav1beta2.ResourceReference) { s.V(3).Info("Setting status", "name", name, "resource", resource) if s.IBMPowerVSCluster.Status.VPCSubnet == nil { s.IBMPowerVSCluster.Status.VPCSubnet = make(map[string]infrav1beta2.ResourceReference) @@ -572,8 +551,8 @@ func (s *PowerVSClusterScope) GetVPCSecurityGroupByID(securityGroupID string) (* return nil, nil, nil } -// SetVPCSecurityGroup set the VPC security group id. -func (s *PowerVSClusterScope) SetVPCSecurityGroup(name string, resource infrav1beta2.VPCSecurityGroupStatus) { +// SetVPCSecurityGroupStatus set the VPC security group id. +func (s *PowerVSClusterScope) SetVPCSecurityGroupStatus(name string, resource infrav1beta2.VPCSecurityGroupStatus) { s.V(3).Info("Setting VPC security group status", "name", name, "resource", resource) if s.IBMPowerVSCluster.Status.VPCSecurityGroups == nil { s.IBMPowerVSCluster.Status.VPCSecurityGroups = make(map[string]infrav1beta2.VPCSecurityGroupStatus) @@ -599,23 +578,6 @@ func (s *PowerVSClusterScope) GetTransitGatewayID() *string { return nil } -// PublicLoadBalancer returns the cluster public loadBalancer information. -func (s *PowerVSClusterScope) PublicLoadBalancer() *infrav1beta2.VPCLoadBalancerSpec { - // if the user did not specify any loadbalancer then return the public loadbalancer created by the controller. - if len(s.IBMPowerVSCluster.Spec.LoadBalancers) == 0 { - return &infrav1beta2.VPCLoadBalancerSpec{ - Name: *s.GetServiceName(infrav1beta2.ResourceTypeLoadBalancer), - Public: ptr.To(true), - } - } - for _, lb := range s.IBMPowerVSCluster.Spec.LoadBalancers { - if lb.Public != nil && *lb.Public { - return &lb - } - } - return nil -} - // SetLoadBalancerStatus set the loadBalancer id. func (s *PowerVSClusterScope) SetLoadBalancerStatus(name string, loadBalancer infrav1beta2.VPCLoadBalancerStatus) { s.V(3).Info("Setting status", "name", name, "status", loadBalancer) @@ -652,18 +614,45 @@ func (s *PowerVSClusterScope) GetLoadBalancerState(name string) *infrav1beta2.VP return nil } -// GetLoadBalancerHostName will return the hostname of load balancer. -func (s *PowerVSClusterScope) GetLoadBalancerHostName(name string) *string { +// GetPublicLoadBalancerHostName will return the hostname of the public load balancer. +func (s *PowerVSClusterScope) GetPublicLoadBalancerHostName() (*string, error) { if s.IBMPowerVSCluster.Status.LoadBalancers == nil { - return nil + return nil, nil + } + + var name string + if len(s.IBMPowerVSCluster.Spec.LoadBalancers) == 0 { + name = *s.GetServiceName(infrav1beta2.ResourceTypeLoadBalancer) } + + for _, lb := range s.IBMPowerVSCluster.Spec.LoadBalancers { + if !*lb.Public { + continue + } + + if lb.Name != "" { + name = lb.Name + break + } + if lb.ID != nil { + loadBalancer, _, err := s.IBMVPCClient.GetLoadBalancer(&vpcv1.GetLoadBalancerOptions{ + ID: lb.ID, + }) + if err != nil { + return nil, err + } + name = *loadBalancer.Name + break + } + } + if val, ok := s.IBMPowerVSCluster.Status.LoadBalancers[name]; ok { - return val.Hostname + return val.Hostname, nil } - return nil + return nil, nil } -// GetResourceGroupID returns the resource group id if it present under spec or statue filed of IBMPowerVSCluster object +// GetResourceGroupID returns the resource group id if it present under spec or status filed of IBMPowerVSCluster object // or returns empty string. func (s *PowerVSClusterScope) GetResourceGroupID() string { if s.IBMPowerVSCluster.Spec.ResourceGroup != nil && s.IBMPowerVSCluster.Spec.ResourceGroup.ID != nil { @@ -716,10 +705,10 @@ func (s *PowerVSClusterScope) ReconcileResourceGroup() error { // ReconcilePowerVSServiceInstance reconciles Power VS service instance. func (s *PowerVSClusterScope) ReconcilePowerVSServiceInstance() (bool, error) { - // Verify if service instance id is set in spec or status field of IBMPowerVSCluster object. + // Verify if service instance id is set in status field of IBMPowerVSCluster object. serviceInstanceID := s.GetServiceInstanceID() if serviceInstanceID != "" { - s.V(3).Info("PowerVS service instance ID is set, fetching details", "id", serviceInstanceID) + s.V(3).Info("PowerVS service instance ID is set, fetching details", "serviceInstanceID", serviceInstanceID) // if serviceInstanceID is set, verify that it exist and in active state. serviceInstance, _, err := s.ResourceClient.GetResourceInstance(&resourcecontrollerv2.GetResourceInstanceOptions{ ID: &serviceInstanceID, @@ -745,6 +734,7 @@ func (s *PowerVSClusterScope) ReconcilePowerVSServiceInstance() (bool, error) { } // Set the status of IBMPowerVSCluster object with serviceInstanceID and ControllerCreated to false as PowerVS service instance is already exist in cloud. if serviceInstanceID != "" { + s.V(3).Info("Found PowerVS service instance in IBM Cloud", "serviceInstanceID", serviceInstanceID) s.SetStatus(infrav1beta2.ResourceTypeServiceInstance, infrav1beta2.ResourceReference{ID: &serviceInstanceID, ControllerCreated: ptr.To(false)}) return requeue, nil } @@ -758,7 +748,7 @@ func (s *PowerVSClusterScope) ReconcilePowerVSServiceInstance() (bool, error) { return false, fmt.Errorf("created PowerVS service instance is nil") } - s.Info("Created PowerVS service instance", "id", serviceInstance.GUID) + s.Info("Created PowerVS service instance", "serviceInstanceID", serviceInstance.GUID) // Set the status of IBMPowerVSCluster object with serviceInstanceID and ControllerCreated to true as new PowerVS service instance is created. s.SetStatus(infrav1beta2.ResourceTypeServiceInstance, infrav1beta2.ResourceReference{ID: serviceInstance.GUID, ControllerCreated: ptr.To(true)}) return true, nil @@ -782,17 +772,38 @@ func (s *PowerVSClusterScope) checkServiceInstanceState(instance resourcecontrol return false, fmt.Errorf("PowerVS service instance is in %s state", *instance.State) } -// checkServiceInstance checks PowerVS service instance exist in cloud. +// checkServiceInstance checks PowerVS service instance exist in cloud by ID or name. func (s *PowerVSClusterScope) isServiceInstanceExists() (string, bool, error) { s.V(3).Info("Checking for PowerVS service instance in IBM Cloud") - // Fetches service instance by name. - serviceInstance, err := s.getServiceInstance() + var ( + id string + err error + serviceInstance *resourcecontrollerv2.ResourceInstance + ) + + if s.IBMPowerVSCluster.Spec.ServiceInstanceID != "" { + id = s.IBMPowerVSCluster.Spec.ServiceInstanceID + } else if s.IBMPowerVSCluster.Spec.ServiceInstance != nil && s.IBMPowerVSCluster.Spec.ServiceInstance.ID != nil { + id = *s.IBMPowerVSCluster.Spec.ServiceInstance.ID + } + + if id != "" { + // Fetches service instance by ID. + serviceInstance, _, err = s.ResourceClient.GetResourceInstance(&resourcecontrollerv2.GetResourceInstanceOptions{ + ID: &id, + }) + } else { + // Fetches service instance by name. + serviceInstance, err = s.getServiceInstance() + } + if err != nil { s.Error(err, "failed to get PowerVS service instance") return "", false, err } + if serviceInstance == nil { - s.V(3).Info("PowerVS service instance with given name does not exist in IBM Cloud", "name", *s.GetServiceName(infrav1beta2.ResourceTypeServiceInstance)) + s.V(3).Info("PowerVS service instance with given ID or name does not exist in IBM Cloud") return "", false, nil } @@ -839,7 +850,7 @@ func (s *PowerVSClusterScope) createServiceInstance() (*resourcecontrollerv2.Res // ReconcileNetwork reconciles network. func (s *PowerVSClusterScope) ReconcileNetwork() (bool, error) { if s.GetDHCPServerID() != nil { - s.V(3).Info("DHCP server ID is set, fetching details", "id", s.GetDHCPServerID()) + s.V(3).Info("DHCP server ID is set, fetching details", "dhcpServerID", s.GetDHCPServerID()) active, err := s.isDHCPServerActive() if err != nil { return false, err @@ -855,46 +866,30 @@ func (s *PowerVSClusterScope) ReconcileNetwork() (bool, error) { return false, err } if networkID != nil { - s.V(3).Info("Found PowerVS network in IBM Cloud", "id", networkID) + s.V(3).Info("Found PowerVS network in IBM Cloud", "networkID", networkID) s.SetStatus(infrav1beta2.ResourceTypeNetwork, infrav1beta2.ResourceReference{ID: networkID, ControllerCreated: ptr.To(false)}) return true, nil } - dhcpServer, err := s.createDHCPServer() + dhcpServerID, err := s.createDHCPServer() if err != nil { s.Error(err, "Error creating DHCP server") return false, err } - s.Info("Created DHCP Server", "id", *dhcpServer) - s.SetStatus(infrav1beta2.ResourceTypeDHCPServer, infrav1beta2.ResourceReference{ID: dhcpServer, ControllerCreated: ptr.To(true)}) + s.Info("Created DHCP Server", "dhcpServerID", *dhcpServerID) + s.SetStatus(infrav1beta2.ResourceTypeDHCPServer, infrav1beta2.ResourceReference{ID: dhcpServerID, ControllerCreated: ptr.To(true)}) return false, nil } -// checkNetwork checks the network exist in cloud. +// checkNetwork checks if network exists in cloud with given name or ID mentioned in spec. func (s *PowerVSClusterScope) checkNetwork() (*string, error) { - // get network from cloud. - s.V(3).Info("Checking if PowerVS network exists in IBM Cloud") - networkID, err := s.getNetwork() - if err != nil { - s.Error(err, "failed to get PowerVS network") - return nil, err - } - if networkID == nil { - s.V(3).Info("Unable to find PowerVS network in IBM Cloud", "network", s.IBMPowerVSCluster.Spec.Network) - return nil, nil - } - return networkID, nil -} - -func (s *PowerVSClusterScope) getNetwork() (*string, error) { - // fetch the network associated with network id if s.IBMPowerVSCluster.Spec.Network.ID != nil { + s.V(3).Info("Checking if PowerVS network exists in IBM Cloud with ID", "networkID", *s.IBMPowerVSCluster.Spec.Network.ID) network, err := s.IBMPowerVSClient.GetNetworkByID(*s.IBMPowerVSCluster.Spec.Network.ID) if err != nil { return nil, err } - s.V(3).Info("Found the PowerVS network", "id", network.NetworkID) return network.NetworkID, nil } @@ -907,25 +902,22 @@ func (s *PowerVSClusterScope) getNetwork() (*string, error) { networkName = *s.GetServiceName(infrav1beta2.ResourceTypeNetwork) } + s.V(3).Info("Checking if PowerVS network exists in IBM Cloud with name", "name", networkName) // fetch the network associated with name network, err := s.IBMPowerVSClient.GetNetworkByName(networkName) if err != nil { return nil, err } - if network == nil { + if network == nil || network.NetworkID == nil { + s.V(3).Info("Unable to find PowerVS network in IBM Cloud", "network", s.IBMPowerVSCluster.Spec.Network) return nil, nil } return network.NetworkID, nil - //TODO: Support regular expression } // isDHCPServerActive checks if the DHCP server status is active. func (s *PowerVSClusterScope) isDHCPServerActive() (bool, error) { - dhcpID := *s.GetDHCPServerID() - if dhcpID == "" { - return false, fmt.Errorf("DHCP ID is empty") - } - dhcpServer, err := s.IBMPowerVSClient.GetDHCPServer(dhcpID) + dhcpServer, err := s.IBMPowerVSClient.GetDHCPServer(*s.GetDHCPServerID()) if err != nil { return false, err } @@ -941,7 +933,7 @@ func (s *PowerVSClusterScope) isDHCPServerActive() (bool, error) { // If state is active, true is returned. // In all other cases, it returns false. func (s *PowerVSClusterScope) checkDHCPServerStatus(dhcpServer models.DHCPServerDetail) (bool, error) { - s.V(3).Info("Checking the status of DHCP server", "id", *dhcpServer.ID) + s.V(3).Info("Checking the status of DHCP server", "dhcpServerID", *dhcpServer.ID) switch *dhcpServer.Status { case string(infrav1beta2.DHCPServerStateActive): s.V(3).Info("DHCP server is in active state") @@ -996,7 +988,7 @@ func (s *PowerVSClusterScope) ReconcileVPC() (bool, error) { // if VPC server id is set means the VPC is already created vpcID := s.GetVPCID() if vpcID != nil { - s.V(3).Info("VPC ID is set, fetching details", "id", *vpcID) + s.V(3).Info("VPC ID is set, fetching details", "vpcID", *vpcID) vpcDetails, _, err := s.IBMVPCClient.GetVPC(&vpcv1.GetVPCOptions{ ID: vpcID, }) @@ -1021,6 +1013,7 @@ func (s *PowerVSClusterScope) ReconcileVPC() (bool, error) { return false, err } if id != "" { + s.V(3).Info("VPC found in IBM Cloud", "vpcID", id) s.SetStatus(infrav1beta2.ResourceTypeVPC, infrav1beta2.ResourceReference{ID: &id, ControllerCreated: ptr.To(false)}) return false, nil } @@ -1033,14 +1026,25 @@ func (s *PowerVSClusterScope) ReconcileVPC() (bool, error) { if err != nil { return false, fmt.Errorf("failed to create VPC: %w", err) } - s.Info("Created VPC", "id", *vpcID) + s.Info("Created VPC", "vpcID", *vpcID) s.SetStatus(infrav1beta2.ResourceTypeVPC, infrav1beta2.ResourceReference{ID: vpcID, ControllerCreated: ptr.To(true)}) return true, nil } // checkVPC checks VPC exist in cloud. func (s *PowerVSClusterScope) checkVPC() (string, error) { - vpcDetails, err := s.getVPCByName() + var ( + err error + vpcDetails *vpcv1.VPC + ) + if s.IBMPowerVSCluster.Spec.VPC != nil && s.IBMPowerVSCluster.Spec.VPC.ID != nil { + vpcDetails, _, err = s.IBMVPCClient.GetVPC(&vpcv1.GetVPCOptions{ + ID: s.IBMPowerVSCluster.Spec.VPC.ID, + }) + } else { + vpcDetails, err = s.getVPCByName() + } + if err != nil { s.Error(err, "failed to get VPC") return "", err @@ -1049,7 +1053,7 @@ func (s *PowerVSClusterScope) checkVPC() (string, error) { s.V(3).Info("VPC not found in IBM Cloud", "vpc", s.IBMPowerVSCluster.Spec.VPC) return "", nil } - s.V(3).Info("VPC found in IBM Cloud", "id", *vpcDetails.ID) + s.V(3).Info("VPC found in IBM Cloud", "vpcID", *vpcDetails.ID) return *vpcDetails.ID, nil } @@ -1115,23 +1119,24 @@ func (s *PowerVSClusterScope) ReconcileVPCSubnets() (bool, error) { } subnets = append(subnets, subnet) } + } else { + subnets = append(subnets, s.IBMPowerVSCluster.Spec.VPCSubnets...) } - for index, subnet := range s.IBMPowerVSCluster.Spec.VPCSubnets { - if subnet.Name == nil { - subnet.Name = ptr.To(fmt.Sprintf("%s-%d", *s.GetServiceName(infrav1beta2.ResourceTypeSubnet), index)) - } - subnets = append(subnets, subnet) - } - for _, subnet := range subnets { + + for index, subnet := range subnets { s.Info("Reconciling VPC subnet", "subnet", subnet) var subnetID *string if subnet.ID != nil { subnetID = subnet.ID } else { + if subnet.Name == nil { + subnet.Name = ptr.To(fmt.Sprintf("%s-%d", *s.GetServiceName(infrav1beta2.ResourceTypeSubnet), index)) + } subnetID = s.GetVPCSubnetID(*subnet.Name) } + if subnetID != nil { - s.V(3).Info("VPC subnet ID is set, fetching details", "id", *subnetID) + s.V(3).Info("VPC subnet ID is set, fetching details", "subnetID", *subnetID) subnetDetails, _, err := s.IBMVPCClient.GetSubnet(&vpcv1.GetSubnetOptions{ ID: subnetID, }) @@ -1142,6 +1147,7 @@ func (s *PowerVSClusterScope) ReconcileVPCSubnets() (bool, error) { return false, fmt.Errorf("failed to get VPC subnet with ID %s", *subnetID) } // check for next subnet + s.SetVPCSubnetStatus(*subnetDetails.Name, infrav1beta2.ResourceReference{ID: subnetDetails.ID}) continue } @@ -1152,8 +1158,8 @@ func (s *PowerVSClusterScope) ReconcileVPCSubnets() (bool, error) { return false, err } if vpcSubnetID != "" { - s.V(3).Info("Found VPC subnet in IBM Cloud", "id", vpcSubnetID) - s.SetVPCSubnetID(*subnet.Name, infrav1beta2.ResourceReference{ID: &vpcSubnetID, ControllerCreated: ptr.To(false)}) + s.V(3).Info("Found VPC subnet in IBM Cloud", "subnetID", vpcSubnetID) + s.SetVPCSubnetStatus(*subnet.Name, infrav1beta2.ResourceReference{ID: &vpcSubnetID, ControllerCreated: ptr.To(false)}) // check for next subnet continue } @@ -1164,14 +1170,14 @@ func (s *PowerVSClusterScope) ReconcileVPCSubnets() (bool, error) { s.Error(err, "failed to create VPC subnet") return false, err } - s.Info("Created VPC subnet", "id", subnetID) - s.SetVPCSubnetID(*subnet.Name, infrav1beta2.ResourceReference{ID: subnetID, ControllerCreated: ptr.To(true)}) + s.Info("Created VPC subnet", "subnetID", subnetID) + s.SetVPCSubnetStatus(*subnet.Name, infrav1beta2.ResourceReference{ID: subnetID, ControllerCreated: ptr.To(true)}) return true, nil } return false, nil } -// checkVPCSubnet checks VPC subnet exist in cloud. +// checkVPCSubnet checks if VPC subnet by the given name exists in cloud. func (s *PowerVSClusterScope) checkVPCSubnet(subnetName string) (string, error) { vpcSubnet, err := s.IBMVPCClient.GetVPCSubnetByName(subnetName) if err != nil { @@ -1279,7 +1285,7 @@ func (s *PowerVSClusterScope) ReconcileVPCSecurityGroups() error { } if sg != nil { s.V(3).Info("VPC security group already exists", "name", *sg.Name) - s.SetVPCSecurityGroup(*sg.Name, infrav1beta2.VPCSecurityGroupStatus{ + s.SetVPCSecurityGroupStatus(*sg.Name, infrav1beta2.VPCSecurityGroupStatus{ ID: sg.ID, RuleIDs: ruleIDs, ControllerCreated: ptr.To(false), @@ -1292,7 +1298,7 @@ func (s *PowerVSClusterScope) ReconcileVPCSecurityGroups() error { return fmt.Errorf("failed to create VPC security group: %w", err) } s.Info("VPC security group created", "name", *securityGroup.Name) - s.SetVPCSecurityGroup(*securityGroup.Name, infrav1beta2.VPCSecurityGroupStatus{ + s.SetVPCSecurityGroupStatus(*securityGroup.Name, infrav1beta2.VPCSecurityGroupStatus{ ID: securityGroupID, ControllerCreated: ptr.To(true), }) @@ -1374,7 +1380,7 @@ func (s *PowerVSClusterScope) createVPCSecurityGroupRule(securityGroupID, direct rule := ruleIntf.(*vpcv1.SecurityGroupRuleSecurityGroupRuleProtocolIcmp) ruleID = rule.ID } - s.Info("Created VPC security group rule", "id", *ruleID) + s.Info("Created VPC security group rule", "ruleID", *ruleID) return ruleID, nil } @@ -1431,7 +1437,7 @@ func (s *PowerVSClusterScope) createVPCSecurityGroupRulesAndSetStatus(ogSecurity } s.Info("VPC security group rules created", "security group name", *securityGroupName) - s.SetVPCSecurityGroup(*securityGroupName, infrav1beta2.VPCSecurityGroupStatus{ + s.SetVPCSecurityGroupStatus(*securityGroupName, infrav1beta2.VPCSecurityGroupStatus{ ID: securityGroupID, RuleIDs: ruleIDs, ControllerCreated: ptr.To(true), @@ -1637,7 +1643,7 @@ func (s *PowerVSClusterScope) validateVPCSecurityGroup(securityGroup infrav1beta // ReconcileTransitGateway reconcile transit gateway. func (s *PowerVSClusterScope) ReconcileTransitGateway() (bool, error) { if s.GetTransitGatewayID() != nil { - s.V(3).Info("Transit gateway ID is set, fetching details", "id", s.GetTransitGatewayID()) + s.V(3).Info("Transit gateway ID is set, fetching details", "tgID", s.GetTransitGatewayID()) tg, _, err := s.TransitGatewayClient.GetTransitGateway(&tgapiv1.GetTransitGatewayOptions{ ID: s.GetTransitGatewayID(), }) @@ -1835,7 +1841,6 @@ func (s *PowerVSClusterScope) checkTransitGatewayConnectionStatus(con tgapiv1.Tr s.V(3).Info("Checking the status of transit gateway connection", "name", *con.Name) switch *con.Status { case string(infrav1beta2.TransitGatewayConnectionStateAttached): - s.V(3).Info("Transit gateway connection is in attached state") return false, nil case string(infrav1beta2.TransitGatewayConnectionStateFailed): return false, fmt.Errorf("failed to attach connection to transit gateway, current status: %s", *con.Status) @@ -1887,6 +1892,10 @@ func (s *PowerVSClusterScope) createTransitGateway() error { return fmt.Errorf("failed to fetch resource group ID for resource group %v, ID is empty", s.ResourceGroup()) } + if s.IBMPowerVSCluster.Status.ServiceInstance == nil || s.IBMPowerVSCluster.Status.VPC == nil { + return fmt.Errorf("failed to proeceed with transit gateway creation as either one of VPC or PowerVS service instance reconciliation is not successful") + } + location, globalRouting, err := genUtil.GetTransitGatewayLocationAndRouting(s.Zone(), s.VPC().Region) if err != nil { return fmt.Errorf("failed to get transit gateway location and routing: %w", err) @@ -1894,11 +1903,11 @@ func (s *PowerVSClusterScope) createTransitGateway() error { // throw error when user tries to use local routing where global routing is required. // TODO: Add a webhook validation for below condition. - if s.IBMPowerVSCluster.Spec.TransitGateway.GlobalRouting != nil && !*s.IBMPowerVSCluster.Spec.TransitGateway.GlobalRouting && *globalRouting { + if s.IBMPowerVSCluster.Spec.TransitGateway != nil && s.IBMPowerVSCluster.Spec.TransitGateway.GlobalRouting != nil && !*s.IBMPowerVSCluster.Spec.TransitGateway.GlobalRouting && *globalRouting { return fmt.Errorf("failed to use local routing for transit gateway since powervs and vpc are in different region and requires global routing") } // setting global routing to true when it is set by user. - if s.IBMPowerVSCluster.Spec.TransitGateway.GlobalRouting != nil && *s.IBMPowerVSCluster.Spec.TransitGateway.GlobalRouting { + if s.IBMPowerVSCluster.Spec.TransitGateway != nil && s.IBMPowerVSCluster.Spec.TransitGateway.GlobalRouting != nil && *s.IBMPowerVSCluster.Spec.TransitGateway.GlobalRouting { globalRouting = ptr.To(true) } @@ -1940,19 +1949,18 @@ func (s *PowerVSClusterScope) ReconcileLoadBalancers() (bool, error) { Public: ptr.To(true), } loadBalancers = append(loadBalancers, loadBalancer) - } - for index, loadBalancer := range s.IBMPowerVSCluster.Spec.LoadBalancers { - if loadBalancer.Name == "" { - loadBalancer.Name = fmt.Sprintf("%s-%d", *s.GetServiceName(infrav1beta2.ResourceTypeLoadBalancer), index) - } - loadBalancers = append(loadBalancers, loadBalancer) + } else { + loadBalancers = append(loadBalancers, s.IBMPowerVSCluster.Spec.LoadBalancers...) } - for _, loadBalancer := range loadBalancers { + for index, loadBalancer := range loadBalancers { var loadBalancerID *string if loadBalancer.ID != nil { loadBalancerID = loadBalancer.ID } else { + if loadBalancer.Name == "" { + loadBalancer.Name = fmt.Sprintf("%s-%d", *s.GetServiceName(infrav1beta2.ResourceTypeLoadBalancer), index) + } loadBalancerID = s.GetLoadBalancerID(loadBalancer.Name) } if loadBalancerID != nil { @@ -1983,6 +1991,7 @@ func (s *PowerVSClusterScope) ReconcileLoadBalancers() (bool, error) { return false, err } if loadBalancerStatus != nil { + s.V(3).Info("Found VPC load balancer in IBM Cloud", "loadBalancerID", *loadBalancerStatus.ID) s.SetLoadBalancerStatus(loadBalancer.Name, *loadBalancerStatus) continue } @@ -1999,7 +2008,7 @@ func (s *PowerVSClusterScope) ReconcileLoadBalancers() (bool, error) { if err != nil { return false, fmt.Errorf("failed to create VPC load balancer: %w", err) } - s.Info("Created VPC load balancer", "id", loadBalancerStatus.ID) + s.Info("Created VPC load balancer", "loadBalancerID", loadBalancerStatus.ID) s.SetLoadBalancerStatus(loadBalancer.Name, *loadBalancerStatus) return false, nil } @@ -2033,7 +2042,7 @@ func (s *PowerVSClusterScope) checkLoadBalancerPort(lb infrav1beta2.VPCLoadBalan return nil } -// checkLoadBalancer checks loadBalancer in cloud. +// checkLoadBalancer checks if VPC load balancer by the given name exists in cloud. func (s *PowerVSClusterScope) checkLoadBalancer(lb infrav1beta2.VPCLoadBalancerSpec) (*infrav1beta2.VPCLoadBalancerStatus, error) { loadBalancer, err := s.IBMVPCClient.GetLoadBalancerByName(lb.Name) if err != nil { @@ -2156,7 +2165,7 @@ func (s *PowerVSClusterScope) ReconcileCOSInstance() error { s.Error(err, "failed to create COS service instance") return err } - s.Info("Created COS service instance", "id", cosServiceInstanceStatus.GUID) + s.Info("Created COS service instance", "cosID", cosServiceInstanceStatus.GUID) s.SetStatus(infrav1beta2.ResourceTypeCOSInstance, infrav1beta2.ResourceReference{ID: cosServiceInstanceStatus.GUID, ControllerCreated: ptr.To(true)}) } @@ -2331,7 +2340,9 @@ func (s *PowerVSClusterScope) fetchResourceGroupID() (string, error) { func (s *PowerVSClusterScope) fetchVPCCRN() (*string, error) { vpcID := s.GetVPCID() if vpcID == nil { - return nil, fmt.Errorf("VPC ID is empty") + if s.IBMPowerVSCluster.Spec.VPC != nil && s.IBMPowerVSCluster.Spec.VPC.ID != nil { + vpcID = s.IBMPowerVSCluster.Spec.VPC.ID + } } vpcDetails, _, err := s.IBMVPCClient.GetVPC(&vpcv1.GetVPCOptions{ ID: vpcID, @@ -2345,6 +2356,13 @@ func (s *PowerVSClusterScope) fetchVPCCRN() (*string, error) { // fetchPowerVSServiceInstanceCRN returns Power VS service instance CRN. func (s *PowerVSClusterScope) fetchPowerVSServiceInstanceCRN() (*string, error) { serviceInstanceID := s.GetServiceInstanceID() + if serviceInstanceID == "" { + if s.IBMPowerVSCluster.Spec.ServiceInstanceID != "" { + serviceInstanceID = s.IBMPowerVSCluster.Spec.ServiceInstanceID + } else if s.IBMPowerVSCluster.Spec.ServiceInstance != nil && s.IBMPowerVSCluster.Spec.ServiceInstance.ID != nil { + serviceInstanceID = *s.IBMPowerVSCluster.Spec.ServiceInstance.ID + } + } pvsDetails, _, err := s.ResourceClient.GetResourceInstance(&resourcecontrollerv2.GetResourceInstanceOptions{ ID: &serviceInstanceID, }) @@ -2455,20 +2473,20 @@ func (s *PowerVSClusterScope) DeleteVPCSecurityGroups() error { ID: securityGroup.ID, }); err != nil { if resp != nil && resp.StatusCode == ResourceNotFoundCode { - s.Info("VPC security group has been already deleted", "ID", *securityGroup.ID) + s.Info("VPC security group has been already deleted", "securityGroupID", *securityGroup.ID) continue } return fmt.Errorf("failed to fetch VPC security group '%s': %w", *securityGroup.ID, err) } - s.V(3).Info("Deleting VPC security group", "ID", *securityGroup.ID) + s.V(3).Info("Deleting VPC security group", "securityGroupID", *securityGroup.ID) options := &vpcv1.DeleteSecurityGroupOptions{ ID: securityGroup.ID, } if _, err := s.IBMVPCClient.DeleteSecurityGroup(options); err != nil { return fmt.Errorf("failed to delete VPC security group '%s': %w", *securityGroup.ID, err) } - s.Info("VPC security group successfully deleted", "ID", *securityGroup.ID) + s.Info("VPC security group successfully deleted", "securityGroupID", *securityGroup.ID) } return nil } @@ -2604,7 +2622,7 @@ func (s *PowerVSClusterScope) deleteTransitGatewayConnections(tg *tgapiv1.Transi ID: connID, }) if resp.StatusCode == ResourceNotFoundCode { - s.V(3).Info("Connection deleted in transit gateway", "id", *connID) + s.V(3).Info("Connection deleted in transit gateway", "connectionID", *connID) return false, nil } if err != nil { diff --git a/cloud/scope/powervs_cluster_test.go b/cloud/scope/powervs_cluster_test.go index 8fd006903..35f7de90b 100644 --- a/cloud/scope/powervs_cluster_test.go +++ b/cloud/scope/powervs_cluster_test.go @@ -23,6 +23,7 @@ import ( "github.com/IBM/go-sdk-core/v5/core" "github.com/IBM/platform-services-go-sdk/resourcecontrollerv2" + "github.com/IBM/vpc-go-sdk/vpcv1" "go.uber.org/mock/gomock" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -38,6 +39,7 @@ import ( "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/resourcemanager" "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/transitgateway" "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/vpc" + "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/vpc/mock" . "github.com/onsi/gomega" ) @@ -172,30 +174,6 @@ func TestGetServiceInstanceID(t *testing.T) { IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{}, }, }, - { - name: "Service Instance ID is set in spec.ServiceInstanceID", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Spec: infrav1beta2.IBMPowerVSClusterSpec{ - ServiceInstanceID: "specServiceInstanceID", - }, - }, - }, - expectedID: "specServiceInstanceID", - }, - { - name: "Service Instance ID is set in spec.ServiceInstance.ID", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Spec: infrav1beta2.IBMPowerVSClusterSpec{ - ServiceInstance: &infrav1beta2.IBMPowerVSResourceReference{ - ID: ptr.To("specServiceInstanceID"), - }, - }, - }, - }, - expectedID: "specServiceInstanceID", - }, { name: "Service Instance ID is set in status.ServiceInstanceID", clusterScope: PowerVSClusterScope{ @@ -209,22 +187,6 @@ func TestGetServiceInstanceID(t *testing.T) { }, expectedID: "statusServiceInstanceID", }, - { - name: "Spec Service Instance ID takes precedence over status Service Instance ID", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Spec: infrav1beta2.IBMPowerVSClusterSpec{ - ServiceInstanceID: "specServiceInstanceID", - }, - Status: infrav1beta2.IBMPowerVSClusterStatus{ - ServiceInstance: &infrav1beta2.ResourceReference{ - ID: ptr.To("statusServiceInstanceID"), - }, - }, - }, - }, - expectedID: "specServiceInstanceID", - }, } for _, tc := range testCases { @@ -248,17 +210,6 @@ func TestGetDHCPServerID(t *testing.T) { IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{}, }, }, - { - name: "DHCP server ID is set in spec", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Spec: infrav1beta2.IBMPowerVSClusterSpec{ - DHCPServer: &infrav1beta2.DHCPServer{ID: ptr.To("dhcpserverid")}, - }, - }, - }, - expectedID: ptr.To("dhcpserverid"), - }, { name: "DHCP server ID is set in status", clusterScope: PowerVSClusterScope{ @@ -272,22 +223,6 @@ func TestGetDHCPServerID(t *testing.T) { }, expectedID: ptr.To("dhcpserverid"), }, - { - name: "Spec DHCP server ID takes precedence over status DHCP Server ID", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Spec: infrav1beta2.IBMPowerVSClusterSpec{ - DHCPServer: &infrav1beta2.DHCPServer{ID: ptr.To("dhcpserverid")}, - }, - Status: infrav1beta2.IBMPowerVSClusterStatus{ - DHCPServer: &infrav1beta2.ResourceReference{ - ID: ptr.To("dhcpserveridstatus"), - }, - }, - }, - }, - expectedID: ptr.To("dhcpserverid"), - }, } for _, tc := range testCases { @@ -311,17 +246,6 @@ func TestGetVPCID(t *testing.T) { IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{}, }, }, - { - name: "VPC ID is set in spec", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Spec: infrav1beta2.IBMPowerVSClusterSpec{ - VPC: &infrav1beta2.VPCResourceReference{ID: ptr.To("vpcID")}, - }, - }, - }, - expectedID: ptr.To("vpcID"), - }, { name: "VPC ID is set in status", clusterScope: PowerVSClusterScope{ @@ -335,22 +259,6 @@ func TestGetVPCID(t *testing.T) { }, expectedID: ptr.To("vpcID"), }, - { - name: "spec VPC ID takes precedence over status VPC ID", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Spec: infrav1beta2.IBMPowerVSClusterSpec{ - VPC: &infrav1beta2.VPCResourceReference{ID: ptr.To("vpcID")}, - }, - Status: infrav1beta2.IBMPowerVSClusterStatus{ - VPC: &infrav1beta2.ResourceReference{ - ID: ptr.To("vpcID1"), - }, - }, - }, - }, - expectedID: ptr.To("vpcID"), - }, } for _, tc := range testCases { @@ -453,24 +361,6 @@ func TestGetVPCSubnetIDs(t *testing.T) { IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{}, }, }, - { - name: "VPC subnet id is set in spec", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Spec: infrav1beta2.IBMPowerVSClusterSpec{ - VPCSubnets: []infrav1beta2.Subnet{ - { - ID: ptr.To("subnet1"), - }, - { - ID: ptr.To("subnet2"), - }, - }, - }, - }, - }, - expectedIDs: []*string{ptr.To("subnet1"), ptr.To("subnet2")}, - }, { name: "VPC subnet id is set in status", clusterScope: PowerVSClusterScope{ @@ -868,166 +758,337 @@ func TestGetLoadBalancerState(t *testing.T) { } func TestGetLoadBalancerHostName(t *testing.T) { - testCases := []struct { - name string - lbName string - expectedHostName *string - clusterScope PowerVSClusterScope - }{ - { - name: "LoadBalancer status is not set", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{}, + var ( + mockVPC *mock.MockVpc + mockCtrl *gomock.Controller + ) + setup := func(t *testing.T) { + t.Helper() + mockCtrl = gomock.NewController(t) + mockVPC = mock.NewMockVpc(mockCtrl) + } + teardown := func() { + mockCtrl.Finish() + } + + t.Run("Load balancer status is nil", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + clusterScope := PowerVSClusterScope{ + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Status: infrav1beta2.IBMPowerVSClusterStatus{}, }, - }, - { - name: "LoadBalancer status is empty", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Status: infrav1beta2.IBMPowerVSClusterStatus{ - LoadBalancers: make(map[string]infrav1beta2.VPCLoadBalancerStatus), + } + + hostName, err := clusterScope.GetPublicLoadBalancerHostName() + g.Expect(hostName).To(BeNil()) + g.Expect(err).To(BeNil()) + }) + t.Run("Load balancer name is not set in IBMPowerVSCluster spec", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + clusterScope := PowerVSClusterScope{ + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{}, + Status: infrav1beta2.IBMPowerVSClusterStatus{ + LoadBalancers: map[string]infrav1beta2.VPCLoadBalancerStatus{ + "-loadbalancer": { + Hostname: ptr.To("lb-hostname"), + }, }, }, }, - }, - { - name: "empty LoadBalancer name is passed", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Status: infrav1beta2.IBMPowerVSClusterStatus{ - LoadBalancers: map[string]infrav1beta2.VPCLoadBalancerStatus{ - "lb": { - Hostname: ptr.To("hostname"), - }, + } + + hostName, err := clusterScope.GetPublicLoadBalancerHostName() + g.Expect(hostName).To(Equal(ptr.To("lb-hostname"))) + g.Expect(err).To(BeNil()) + }) + t.Run("Invalid load balancer name is set in IBMPowerVSCluster spec", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + clusterScope := PowerVSClusterScope{ + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + LoadBalancers: []infrav1beta2.VPCLoadBalancerSpec{ + { + Name: "lb", + Public: core.BoolPtr(true), }, }, }, - }, - }, - { - name: "invalid LoadBalancer name is passed", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Status: infrav1beta2.IBMPowerVSClusterStatus{ - LoadBalancers: map[string]infrav1beta2.VPCLoadBalancerStatus{ - "lb": { - Hostname: ptr.To("hostname"), - }, + Status: infrav1beta2.IBMPowerVSClusterStatus{ + LoadBalancers: map[string]infrav1beta2.VPCLoadBalancerStatus{ + "loadbalancer": { + Hostname: ptr.To("lb-hostname"), }, }, }, }, - lbName: "lb2", - }, - { - name: "valid LoadBalancer name is passed", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Status: infrav1beta2.IBMPowerVSClusterStatus{ - LoadBalancers: map[string]infrav1beta2.VPCLoadBalancerStatus{ - "lb": { - Hostname: ptr.To("hostname"), - }, + } + + hostName, err := clusterScope.GetPublicLoadBalancerHostName() + g.Expect(hostName).To(BeNil()) + g.Expect(err).To(BeNil()) + }) + t.Run("Valid load balancer name is set in IBMPowerVSCluster spec", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + clusterScope := PowerVSClusterScope{ + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + LoadBalancers: []infrav1beta2.VPCLoadBalancerSpec{ + { + Name: "loadbalancer", + Public: core.BoolPtr(true), + }, + }, + }, + Status: infrav1beta2.IBMPowerVSClusterStatus{ + LoadBalancers: map[string]infrav1beta2.VPCLoadBalancerStatus{ + "loadbalancer": { + Hostname: ptr.To("lb-hostname"), }, }, }, }, - lbName: "lb", - expectedHostName: ptr.To("hostname"), - }, - } + } - for _, tc := range testCases { + hostName, err := clusterScope.GetPublicLoadBalancerHostName() + g.Expect(hostName).To(Equal(ptr.To("lb-hostname"))) + g.Expect(err).To(BeNil()) + }) + t.Run("Both public and private load balancer name is set in IBMPowerVSCluster spec", func(t *testing.T) { g := NewWithT(t) - t.Run(tc.name, func(_ *testing.T) { - lbName := tc.clusterScope.GetLoadBalancerHostName(tc.lbName) - g.Expect(utils.DereferencePointer(lbName)).To(Equal(utils.DereferencePointer(tc.expectedHostName))) - }) - } -} + setup(t) + t.Cleanup(teardown) -func TestPublicLoadBalancer(t *testing.T) { - testCases := []struct { - name string - expectedLB *infrav1beta2.VPCLoadBalancerSpec - clusterScope PowerVSClusterScope - }{ - { - name: "IBMPowerVSCluster spec is empty", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{}, - }, - expectedLB: &infrav1beta2.VPCLoadBalancerSpec{ - Name: "-loadbalancer", - Public: ptr.To(true), - }, - }, - { - name: "one public loadbalancer is configured", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Spec: infrav1beta2.IBMPowerVSClusterSpec{ - LoadBalancers: []infrav1beta2.VPCLoadBalancerSpec{ - { - Name: "lb", - Public: ptr.To(true), - }, + clusterScope := PowerVSClusterScope{ + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + LoadBalancers: []infrav1beta2.VPCLoadBalancerSpec{ + { + Name: "lb1", + Public: core.BoolPtr(false), + }, + { + Name: "lb2", + Public: core.BoolPtr(true), + }, + }, + }, + Status: infrav1beta2.IBMPowerVSClusterStatus{ + LoadBalancers: map[string]infrav1beta2.VPCLoadBalancerStatus{ + "lb1": { + Hostname: ptr.To("lb1-hostname"), + }, + "lb2": { + Hostname: ptr.To("lb2-hostname"), }, }, }, }, - expectedLB: &infrav1beta2.VPCLoadBalancerSpec{ - Name: "lb", - Public: ptr.To(true), + } + + hostName, err := clusterScope.GetPublicLoadBalancerHostName() + g.Expect(hostName).To(Equal(ptr.To("lb2-hostname"))) + g.Expect(err).To(BeNil()) + }) + t.Run("Multiple public load balancer names are set in IBMPowerVSCluster spec", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + clusterScope := PowerVSClusterScope{ + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + LoadBalancers: []infrav1beta2.VPCLoadBalancerSpec{ + { + Name: "lb1", + Public: core.BoolPtr(true), + }, + { + Name: "lb2", + Public: core.BoolPtr(true), + }, + }, + }, + Status: infrav1beta2.IBMPowerVSClusterStatus{ + LoadBalancers: map[string]infrav1beta2.VPCLoadBalancerStatus{ + "lb1": { + Hostname: ptr.To("lb1-hostname"), + }, + "lb2": { + Hostname: ptr.To("lb2-hostname"), + }, + }, + }, }, - }, - { - name: "multiple public loadbalancer is configured", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Spec: infrav1beta2.IBMPowerVSClusterSpec{ - LoadBalancers: []infrav1beta2.VPCLoadBalancerSpec{ - { - Name: "lb", - Public: ptr.To(true), - }, - { - Name: "lb2", - Public: ptr.To(true), - }, + } + + hostName, err := clusterScope.GetPublicLoadBalancerHostName() + g.Expect(hostName).To(Equal(ptr.To("lb1-hostname"))) + g.Expect(err).To(BeNil()) + }) + t.Run("Valid load balancer ID is set in IBMPowerVSCluster spec", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + clusterScope := PowerVSClusterScope{ + IBMVPCClient: mockVPC, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + LoadBalancers: []infrav1beta2.VPCLoadBalancerSpec{ + { + ID: ptr.To("loadbalancer-id"), + Public: core.BoolPtr(true), + }, + }, + }, + Status: infrav1beta2.IBMPowerVSClusterStatus{ + LoadBalancers: map[string]infrav1beta2.VPCLoadBalancerStatus{ + "loadbalancer": { + Hostname: ptr.To("lb-hostname"), }, }, }, }, - expectedLB: &infrav1beta2.VPCLoadBalancerSpec{ - Name: "lb", - Public: ptr.To(true), + } + lb := &vpcv1.LoadBalancer{ + Name: ptr.To("loadbalancer"), + } + mockVPC.EXPECT().GetLoadBalancer(gomock.Any()).Return(lb, &core.DetailedResponse{}, nil) + + hostName, err := clusterScope.GetPublicLoadBalancerHostName() + g.Expect(hostName).To(Equal(ptr.To("lb-hostname"))) + g.Expect(err).To(BeNil()) + }) + t.Run("Invalid load balancer ID is set in IBMPowerVSCluster spec", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + clusterScope := PowerVSClusterScope{ + IBMVPCClient: mockVPC, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + LoadBalancers: []infrav1beta2.VPCLoadBalancerSpec{ + { + ID: ptr.To("loadbalancer-id1"), + Public: core.BoolPtr(true), + }, + }, + }, + Status: infrav1beta2.IBMPowerVSClusterStatus{ + LoadBalancers: map[string]infrav1beta2.VPCLoadBalancerStatus{ + "loadbalancer": { + Hostname: ptr.To("lb-hostname"), + }, + }, + }, }, - }, - { - name: "only private loadbalancer is configured", - clusterScope: PowerVSClusterScope{ - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - Spec: infrav1beta2.IBMPowerVSClusterSpec{ - LoadBalancers: []infrav1beta2.VPCLoadBalancerSpec{ - { - Name: "lb", - Public: ptr.To(false), - }, + } + + mockVPC.EXPECT().GetLoadBalancer(gomock.Any()).Return(nil, &core.DetailedResponse{}, errors.New("failed to get the load balancer")) + + hostName, err := clusterScope.GetPublicLoadBalancerHostName() + g.Expect(hostName).To(BeNil()) + g.Expect(err).ToNot(BeNil()) + }) + t.Run("Multiple public load balancer IDs are set in IBMPowerVSCluster spec", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + clusterScope := PowerVSClusterScope{ + IBMVPCClient: mockVPC, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + LoadBalancers: []infrav1beta2.VPCLoadBalancerSpec{ + { + ID: ptr.To("lb1"), + Public: core.BoolPtr(true), + }, + { + ID: ptr.To("lb2"), + Public: core.BoolPtr(true), + }, + }, + }, + Status: infrav1beta2.IBMPowerVSClusterStatus{ + LoadBalancers: map[string]infrav1beta2.VPCLoadBalancerStatus{ + "loadbalancer1": { + Hostname: ptr.To("lb1-hostname"), + }, + "loadbalancer2": { + Hostname: ptr.To("lb2-hostname"), }, }, }, }, - }, - } + } - for _, tc := range testCases { + lb := &vpcv1.LoadBalancer{ + Name: ptr.To("loadbalancer1"), + } + mockVPC.EXPECT().GetLoadBalancer(gomock.Any()).Return(lb, &core.DetailedResponse{}, nil) + + hostName, err := clusterScope.GetPublicLoadBalancerHostName() + g.Expect(hostName).To(Equal(ptr.To("lb1-hostname"))) + g.Expect(err).To(BeNil()) + }) + + t.Run("Both private and public load balancer IDs are set in IBMPowerVSCluster spec", func(t *testing.T) { g := NewWithT(t) - t.Run(tc.name, func(_ *testing.T) { - lbSpec := tc.clusterScope.PublicLoadBalancer() - g.Expect(lbSpec).To(Equal(tc.expectedLB)) - }) - } + setup(t) + t.Cleanup(teardown) + + clusterScope := PowerVSClusterScope{ + IBMVPCClient: mockVPC, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + LoadBalancers: []infrav1beta2.VPCLoadBalancerSpec{ + { + ID: ptr.To("lb1"), + Public: core.BoolPtr(false), + }, + { + ID: ptr.To("lb2"), + Public: core.BoolPtr(true), + }, + }, + }, + Status: infrav1beta2.IBMPowerVSClusterStatus{ + LoadBalancers: map[string]infrav1beta2.VPCLoadBalancerStatus{ + "loadbalancer1": { + Hostname: ptr.To("lb1-hostname"), + }, + "loadbalancer2": { + Hostname: ptr.To("lb2-hostname"), + }, + }, + }, + }, + } + + lb := &vpcv1.LoadBalancer{ + Name: ptr.To("loadbalancer2"), + } + mockVPC.EXPECT().GetLoadBalancer(gomock.Any()).Return(lb, &core.DetailedResponse{}, nil) + + hostName, err := clusterScope.GetPublicLoadBalancerHostName() + g.Expect(hostName).To(Equal(ptr.To("lb2-hostname"))) + g.Expect(err).To(BeNil()) + }) } func TestGetResourceGroupID(t *testing.T) { @@ -1107,125 +1168,151 @@ func TestReconcilePowerVSServiceInstance(t *testing.T) { teardown := func() { mockCtrl.Finish() } - t.Run("When service instance id is set and GetResourceInstance returns error", func(t *testing.T) { + t.Run("When service instance id is set in status and GetResourceInstance returns error", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - mockResourceController.EXPECT().GetResourceInstance(gomock.Any()).Return(nil, nil, errors.New("failed to get resource instance")) - return mockResourceController, nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Status: infrav1beta2.IBMPowerVSClusterStatus{ + ServiceInstance: &infrav1beta2.ResourceReference{ + ID: ptr.To(serviceInstanceID), + }, + }, + }, } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) - clusterScope.IBMPowerVSCluster.Spec.ServiceInstanceID = serviceInstanceID + + mockResourceController.EXPECT().GetResourceInstance(gomock.Any()).Return(nil, nil, errors.New("failed to get resource instance")) requeue, err := clusterScope.ReconcilePowerVSServiceInstance() g.Expect(err).ToNot(BeNil()) g.Expect(requeue).To(BeFalse()) }) - t.Run("When service instance id is set and GetResourceInstance returns nil", func(t *testing.T) { + t.Run("When service instance id is set in status and GetResourceInstance returns nil", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - mockResourceController.EXPECT().GetResourceInstance(gomock.Any()).Return(nil, nil, nil) - return mockResourceController, nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Status: infrav1beta2.IBMPowerVSClusterStatus{ + ServiceInstance: &infrav1beta2.ResourceReference{ + ID: ptr.To(serviceInstanceID), + }, + }, + }, } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) - clusterScope.IBMPowerVSCluster.Spec.ServiceInstanceID = serviceInstanceID + + mockResourceController.EXPECT().GetResourceInstance(gomock.Any()).Return(nil, nil, nil) requeue, err := clusterScope.ReconcilePowerVSServiceInstance() g.Expect(err).ToNot(BeNil()) g.Expect(requeue).To(BeFalse()) }) - t.Run("When service instance id is set and instance in failed state", func(t *testing.T) { + t.Run("When service instance id is set in status and instance is in failed state", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - instance := &resourcecontrollerv2.ResourceInstance{ - Name: ptr.To("test-instance"), - State: ptr.To("failed"), - } - mockResourceController.EXPECT().GetResourceInstance(gomock.Any()).Return(instance, nil, nil) - return mockResourceController, nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Status: infrav1beta2.IBMPowerVSClusterStatus{ + ServiceInstance: &infrav1beta2.ResourceReference{ + ID: ptr.To(serviceInstanceID), + }, + }, + }, } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) - clusterScope.IBMPowerVSCluster.Spec.ServiceInstanceID = serviceInstanceID + + instance := &resourcecontrollerv2.ResourceInstance{ + Name: ptr.To("test-instance"), + State: ptr.To("failed"), + } + mockResourceController.EXPECT().GetResourceInstance(gomock.Any()).Return(instance, nil, nil) requeue, err := clusterScope.ReconcilePowerVSServiceInstance() g.Expect(err).ToNot(BeNil()) g.Expect(requeue).To(BeFalse()) }) - t.Run("When service instance id is set and instance in active state", func(t *testing.T) { + t.Run("When service instance id is set in status and instance is in active state", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - instance := &resourcecontrollerv2.ResourceInstance{ - Name: ptr.To("test-instance"), - State: ptr.To("active"), - } - mockResourceController.EXPECT().GetResourceInstance(gomock.Any()).Return(instance, nil, nil) - return mockResourceController, nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Status: infrav1beta2.IBMPowerVSClusterStatus{ + ServiceInstance: &infrav1beta2.ResourceReference{ + ID: ptr.To(serviceInstanceID), + }, + }, + }, } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) - clusterScope.IBMPowerVSCluster.Spec.ServiceInstanceID = serviceInstanceID + + instance := &resourcecontrollerv2.ResourceInstance{ + Name: ptr.To("test-instance"), + State: ptr.To("active"), + } + mockResourceController.EXPECT().GetResourceInstance(gomock.Any()).Return(instance, nil, nil) requeue, err := clusterScope.ReconcilePowerVSServiceInstance() g.Expect(err).To(BeNil()) g.Expect(requeue).To(BeFalse()) }) - t.Run("When isServiceInstanceExists returns error", func(t *testing.T) { + t.Run("When service instance id is set in spec and instance does not exist", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.New("failed to get service instance")) - return mockResourceController, nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ServiceInstance: &infrav1beta2.IBMPowerVSResourceReference{ + ID: ptr.To(serviceInstanceID), + }, + }, + }, } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) + + mockResourceController.EXPECT().GetResourceInstance(gomock.Any()).Return(nil, nil, errors.New("failed to get resource instance")) requeue, err := clusterScope.ReconcilePowerVSServiceInstance() g.Expect(err).ToNot(BeNil()) g.Expect(requeue).To(BeFalse()) }) - t.Run("When service instance exists in cloud and in active state", func(t *testing.T) { + t.Run("When service instance id is set in spec and instance exists in active state", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - instance := &resourcecontrollerv2.ResourceInstance{ - GUID: ptr.To("instance-GUID"), - Name: ptr.To("test-instance"), - State: ptr.To("active"), - } - mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(instance, nil) - return mockResourceController, nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ServiceInstance: &infrav1beta2.IBMPowerVSResourceReference{ + ID: ptr.To(serviceInstanceID), + }, + }, + }, } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) + + instance := &resourcecontrollerv2.ResourceInstance{ + Name: ptr.To("test-instance"), + State: ptr.To("active"), + GUID: ptr.To("instance-GUID"), + } + mockResourceController.EXPECT().GetResourceInstance(gomock.Any()).Return(instance, nil, nil) requeue, err := clusterScope.ReconcilePowerVSServiceInstance() g.Expect(err).To(BeNil()) @@ -1234,20 +1321,55 @@ func TestReconcilePowerVSServiceInstance(t *testing.T) { g.Expect(*clusterScope.IBMPowerVSCluster.Status.ServiceInstance.ControllerCreated).To(BeFalse()) }) - t.Run("When create service instance return error", func(t *testing.T) { + t.Run("When service instance id is set in both spec and status, ID from status takes precedence", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil) - mockResourceController.EXPECT().CreateResourceInstance(gomock.Any()).Return(nil, nil, errors.New("failed to create resource instance")) - return mockResourceController, nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ServiceInstance: &infrav1beta2.IBMPowerVSResourceReference{ + ID: ptr.To("serviceInstanceIDSpec"), + }, + }, + Status: infrav1beta2.IBMPowerVSClusterStatus{ + ServiceInstance: &infrav1beta2.ResourceReference{ + ID: ptr.To("serviceInstanceIDStatus"), + }, + }, + }, + } + + resource := &resourcecontrollerv2.GetResourceInstanceOptions{ + ID: clusterScope.IBMPowerVSCluster.Status.ServiceInstance.ID, } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) + + instance := &resourcecontrollerv2.ResourceInstance{ + Name: ptr.To("test-instance"), + State: ptr.To("active"), + } + mockResourceController.EXPECT().GetResourceInstance(resource).Return(instance, nil, nil) + + requeue, err := clusterScope.ReconcilePowerVSServiceInstance() g.Expect(err).To(BeNil()) - clusterScope.IBMPowerVSCluster.Spec.ResourceGroup = &infrav1beta2.IBMPowerVSResourceReference{ID: ptr.To("resourceGroupID")} + g.Expect(requeue).To(BeFalse()) + }) + + t.Run("When create service instance returns error", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{}, + }, + } + + mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil) requeue, err := clusterScope.ReconcilePowerVSServiceInstance() g.Expect(err).ToNot(BeNil()) @@ -1259,40 +1381,50 @@ func TestReconcilePowerVSServiceInstance(t *testing.T) { setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil) - mockResourceController.EXPECT().CreateResourceInstance(gomock.Any()).Return(nil, nil, nil) - return mockResourceController, nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ResourceGroup: &infrav1beta2.IBMPowerVSResourceReference{ + ID: ptr.To("resource-group-id"), + }, + Zone: ptr.To("zone1"), + }, + }, } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) - clusterScope.IBMPowerVSCluster.Spec.ResourceGroup = &infrav1beta2.IBMPowerVSResourceReference{ID: ptr.To("resourceGroupID")} + + mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil) + mockResourceController.EXPECT().CreateResourceInstance(gomock.Any()).Return(nil, nil, nil) requeue, err := clusterScope.ReconcilePowerVSServiceInstance() g.Expect(err).ToNot(BeNil()) g.Expect(requeue).To(BeFalse()) }) - t.Run("When successfully created new service instance", func(t *testing.T) { + t.Run("When successfully created a new service instance", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - instance := &resourcecontrollerv2.ResourceInstance{ - GUID: ptr.To("instance-GUID"), - Name: ptr.To("test-instance"), - } + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ResourceGroup: &infrav1beta2.IBMPowerVSResourceReference{ + ID: ptr.To("resource-group-id"), + }, + Zone: ptr.To("zone1"), + }, + }, + } - mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil) - mockResourceController.EXPECT().CreateResourceInstance(gomock.Any()).Return(instance, nil, nil) - return mockResourceController, nil + instance := &resourcecontrollerv2.ResourceInstance{ + GUID: ptr.To("instance-GUID"), + Name: ptr.To("test-instance"), } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) - clusterScope.IBMPowerVSCluster.Spec.ResourceGroup = &infrav1beta2.IBMPowerVSResourceReference{ID: ptr.To("resourceGroupID")} + + mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil) + mockResourceController.EXPECT().CreateResourceInstance(gomock.Any()).Return(instance, nil, nil) requeue, err := clusterScope.ReconcilePowerVSServiceInstance() g.Expect(err).To(BeNil()) @@ -1358,90 +1490,116 @@ func TestIsServiceInstanceExists(t *testing.T) { teardown := func() { mockCtrl.Finish() } - t.Run("When get service instance returns error", func(t *testing.T) { + + t.Run("When ServiceInstanceID is set in spec and GetResourceInstance returns error", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.New("failed to get service instance")) - return mockResourceController, nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ServiceInstanceID: "instance-id", + }, + }, } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) + + mockResourceController.EXPECT().GetResourceInstance(gomock.Any()).Return(nil, nil, errors.New("failed to get resource instance")) instanceID, requeue, err := clusterScope.isServiceInstanceExists() g.Expect(instanceID).To(Equal("")) g.Expect(requeue).To(BeFalse()) g.Expect(err).NotTo(BeNil()) }) - t.Run("When get service instance returns nil", func(t *testing.T) { + t.Run("When ServiceInstance.ID is set in spec and GetResourceInstance returns error", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil) - return mockResourceController, nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ServiceInstance: &infrav1beta2.IBMPowerVSResourceReference{ + ID: ptr.To("instance-id"), + }, + }, + }, } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) + + mockResourceController.EXPECT().GetResourceInstance(gomock.Any()).Return(nil, nil, errors.New("failed to get resource instance")) instanceID, requeue, err := clusterScope.isServiceInstanceExists() g.Expect(instanceID).To(Equal("")) g.Expect(requeue).To(BeFalse()) - g.Expect(err).To(BeNil()) + g.Expect(err).NotTo(BeNil()) }) - t.Run("When checkServiceInstanceState returns error", func(t *testing.T) { + t.Run("When ServiceInstance.Name is set in spec and GetResourceInstance returns nil", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(&resourcecontrollerv2.ResourceInstance{Name: ptr.To("instance"), State: ptr.To("unknown")}, nil) - return mockResourceController, nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ServiceInstance: &infrav1beta2.IBMPowerVSResourceReference{ + Name: ptr.To("instance-name"), + }, + }, + }, } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) + + mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil) instanceID, requeue, err := clusterScope.isServiceInstanceExists() g.Expect(instanceID).To(Equal("")) g.Expect(requeue).To(BeFalse()) - g.Expect(err).ToNot(BeNil()) + g.Expect(err).To(BeNil()) }) + t.Run("When checkServiceInstanceState returns error", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(&resourcecontrollerv2.ResourceInstance{Name: ptr.To("instance"), State: ptr.To("unknown")}, nil) - return mockResourceController, nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ServiceInstance: &infrav1beta2.IBMPowerVSResourceReference{ + Name: ptr.To("instance"), + }, + }, + }, } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) + + mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(&resourcecontrollerv2.ResourceInstance{Name: ptr.To("instance"), State: ptr.To("unknown")}, nil) instanceID, requeue, err := clusterScope.isServiceInstanceExists() g.Expect(instanceID).To(Equal("")) g.Expect(requeue).To(BeFalse()) g.Expect(err).ToNot(BeNil()) }) + t.Run("When isServiceInstanceExists returns success", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(&resourcecontrollerv2.ResourceInstance{GUID: ptr.To("guid"), Name: ptr.To("instance"), State: ptr.To("active")}, nil) - return mockResourceController, nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ServiceInstance: &infrav1beta2.IBMPowerVSResourceReference{ + Name: ptr.To("instance"), + }, + }, + }, } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) + + mockResourceController.EXPECT().GetServiceInstance(gomock.Any(), gomock.Any(), gomock.Any()).Return(&resourcecontrollerv2.ResourceInstance{GUID: ptr.To("guid"), Name: ptr.To("instance"), State: ptr.To("active")}, nil) instanceID, requeue, err := clusterScope.isServiceInstanceExists() g.Expect(instanceID).To(Equal("guid")) @@ -1468,10 +1626,12 @@ func TestCreateServiceInstance(t *testing.T) { setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.IBMPowerVSCluster.Spec.ResourceGroup = nil - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{}, + }, + } instance, err := clusterScope.createServiceInstance() g.Expect(instance).To(BeNil()) @@ -1482,10 +1642,16 @@ func TestCreateServiceInstance(t *testing.T) { setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) - clusterScope.IBMPowerVSCluster.Spec.Zone = nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ResourceGroup: &infrav1beta2.IBMPowerVSResourceReference{ + ID: ptr.To("resource-group-id"), + }, + }, + }, + } instance, err := clusterScope.createServiceInstance() g.Expect(instance).To(BeNil()) @@ -1496,13 +1662,19 @@ func TestCreateServiceInstance(t *testing.T) { setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - mockResourceController.EXPECT().CreateResourceInstance(gomock.Any()).Return(nil, nil, errors.New("failed to create resource instance")) - return mockResourceController, nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ResourceGroup: &infrav1beta2.IBMPowerVSResourceReference{ + ID: ptr.To("resource-group-id"), + }, + Zone: ptr.To("zone1"), + }, + }, } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) + + mockResourceController.EXPECT().CreateResourceInstance(gomock.Any()).Return(nil, nil, errors.New("failed to create resource instance")) instance, err := clusterScope.createServiceInstance() g.Expect(instance).To(BeNil()) @@ -1513,60 +1685,22 @@ func TestCreateServiceInstance(t *testing.T) { setup(t) t.Cleanup(teardown) - clusterScopeParams := getPowerVSClusterScopeParams() - clusterScopeParams.ResourceControllerFactory = func() (resourcecontroller.ResourceController, error) { - mockResourceController.EXPECT().CreateResourceInstance(gomock.Any()).Return(&resourcecontrollerv2.ResourceInstance{}, nil, nil) - return mockResourceController, nil + clusterScope := PowerVSClusterScope{ + ResourceClient: mockResourceController, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ResourceGroup: &infrav1beta2.IBMPowerVSResourceReference{ + ID: ptr.To("resource-group-id"), + }, + Zone: ptr.To("zone1"), + }, + }, } - clusterScope, err := NewPowerVSClusterScope(clusterScopeParams) - g.Expect(err).To(BeNil()) + + mockResourceController.EXPECT().CreateResourceInstance(gomock.Any()).Return(&resourcecontrollerv2.ResourceInstance{}, nil, nil) instance, err := clusterScope.createServiceInstance() g.Expect(instance).ToNot(BeNil()) g.Expect(err).To(BeNil()) }) } - -func getPowerVSClusterScopeParams() PowerVSClusterScopeParams { - return PowerVSClusterScopeParams{ - Client: testEnv.Client, - Cluster: newCluster(clusterName), - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{"powervs.cluster.x-k8s.io/create-infra": "true"}, - GenerateName: "powervs-test-", - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: capiv1beta1.GroupVersion.String(), - Kind: "Cluster", - Name: "capi-test", - UID: "1", - }}}, - Spec: infrav1beta2.IBMPowerVSClusterSpec{ - Zone: ptr.To("zone"), - VPC: &infrav1beta2.VPCResourceReference{Region: ptr.To("eu-gb")}, - ResourceGroup: &infrav1beta2.IBMPowerVSResourceReference{ID: ptr.To("rg-id")}, - }, - }, - ClientFactory: ClientFactory{ - AuthenticatorFactory: func() (core.Authenticator, error) { - return nil, nil - }, - PowerVSClientFactory: func() (powervs.PowerVS, error) { - return nil, nil - }, - VPCClientFactory: func() (vpc.Vpc, error) { - return nil, nil - }, - TransitGatewayFactory: func() (transitgateway.TransitGateway, error) { - return nil, nil - }, - ResourceControllerFactory: func() (resourcecontroller.ResourceController, error) { - return nil, nil - }, - ResourceManagerFactory: func() (resourcemanager.ResourceManager, error) { - return nil, nil - }, - }, - } -} diff --git a/controllers/ibmpowervscluster_controller.go b/controllers/ibmpowervscluster_controller.go index 59131a988..6021492e3 100644 --- a/controllers/ibmpowervscluster_controller.go +++ b/controllers/ibmpowervscluster_controller.go @@ -330,20 +330,18 @@ func (r *IBMPowerVSClusterReconciler) reconcile(clusterScope *scope.PowerVSClust return ctrl.Result{RequeueAfter: 30 * time.Second}, nil } - // update cluster object with loadbalancer host - loadBalancer := clusterScope.PublicLoadBalancer() - if loadBalancer == nil { - return reconcile.Result{}, fmt.Errorf("failed to fetch public loadbalancer") - } - clusterScope.Info("Getting load balancer host") - hostName := clusterScope.GetLoadBalancerHostName(loadBalancer.Name) + hostName, err := clusterScope.GetPublicLoadBalancerHostName() + if err != nil { + return reconcile.Result{}, fmt.Errorf("failed to fetch public loadbalancer: %w", err) + } if hostName == nil || *hostName == "" { clusterScope.Info("LoadBalancer hostname is not yet available, requeuing") return reconcile.Result{RequeueAfter: time.Minute}, nil } - clusterScope.IBMPowerVSCluster.Spec.ControlPlaneEndpoint.Host = *clusterScope.GetLoadBalancerHostName(loadBalancer.Name) + // update cluster object with loadbalancer host name + clusterScope.IBMPowerVSCluster.Spec.ControlPlaneEndpoint.Host = *hostName clusterScope.IBMPowerVSCluster.Spec.ControlPlaneEndpoint.Port = clusterScope.APIServerPort() clusterScope.IBMPowerVSCluster.Status.Ready = true return ctrl.Result{}, nil