Merge pull request #319 from shiftstack/merge-bot-release-4.16

OCPBUGS-37967: rebase CAPO for 4.16

OWNERS_ALIASES

@@ -19,9 +19,8 @@ aliases:
     - neolit123
     - vincepri
   cluster-api-openstack-maintainers:
+    - emilienm
     - jichenjc
     - lentzi90
     - mdbooth
   cluster-api-openstack-reviewers:
-    - emilienm
-    - dulek

api/v1alpha6/openstackcluster_conversion.go

@@ -364,7 +364,10 @@ func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha6_OpenStackClusterSpec(in *i
 	}
 
 	out.CloudName = in.IdentityRef.CloudName
-	out.IdentityRef = &OpenStackIdentityReference{Name: in.IdentityRef.Name}
+	out.IdentityRef = &OpenStackIdentityReference{}
+	if err := Convert_v1beta1_OpenStackIdentityReference_To_v1alpha6_OpenStackIdentityReference(&in.IdentityRef, out.IdentityRef, s); err != nil {
+		return err
+	}
 
 	if in.APIServerLoadBalancer != nil {
 		if err := Convert_v1beta1_APIServerLoadBalancer_To_v1alpha6_APIServerLoadBalancer(in.APIServerLoadBalancer, &out.APIServerLoadBalancer, s); err != nil {

api/v1alpha6/openstackmachine_conversion.go

@@ -373,7 +373,6 @@ func Convert_v1beta1_OpenStackMachineSpec_To_v1alpha6_OpenStackMachineSpec(in *i
 	}
 
 	if in.IdentityRef != nil {
-		out.IdentityRef = &OpenStackIdentityReference{Name: in.IdentityRef.Name}
 		out.CloudName = in.IdentityRef.CloudName
 	}
 

api/v1alpha6/types_conversion.go

@@ -534,6 +534,8 @@ func Convert_v1alpha6_OpenStackIdentityReference_To_v1beta1_OpenStackIdentityRef
 
 func Convert_v1beta1_OpenStackIdentityReference_To_v1alpha6_OpenStackIdentityReference(in *infrav1.OpenStackIdentityReference, out *OpenStackIdentityReference, _ apiconversion.Scope) error {
 	out.Name = in.Name
+	// Kind will be overwritten during restore if it was previously set to some other value, but if not then some value is still required
+	out.Kind = "Secret"
 	return nil
 }
 

api/v1alpha7/openstackcluster_conversion.go

@@ -361,7 +361,10 @@ func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha7_OpenStackClusterSpec(in *i
 	}
 
 	out.CloudName = in.IdentityRef.CloudName
-	out.IdentityRef = &OpenStackIdentityReference{Name: in.IdentityRef.Name}
+	out.IdentityRef = &OpenStackIdentityReference{}
+	if err := Convert_v1beta1_OpenStackIdentityReference_To_v1alpha7_OpenStackIdentityReference(&in.IdentityRef, out.IdentityRef, s); err != nil {
+		return err
+	}
 
 	if in.APIServerLoadBalancer != nil {
 		if err := Convert_v1beta1_APIServerLoadBalancer_To_v1alpha7_APIServerLoadBalancer(in.APIServerLoadBalancer, &out.APIServerLoadBalancer, s); err != nil {

api/v1alpha7/types_conversion.go

@@ -536,6 +536,8 @@ func Convert_v1alpha7_OpenStackIdentityReference_To_v1beta1_OpenStackIdentityRef
 
 func Convert_v1beta1_OpenStackIdentityReference_To_v1alpha7_OpenStackIdentityReference(in *infrav1.OpenStackIdentityReference, out *OpenStackIdentityReference, _ apiconversion.Scope) error {
 	out.Name = in.Name
+	// Kind will be overwritten during restore if it was previously set to some other value, but if not then some value is still required
+	out.Kind = "Secret"
 	return nil
 }
 

controllers/openstackmachine_controller.go

@@ -776,17 +776,22 @@ func (r *OpenStackMachineReconciler) getOrCreateInstance(logger logr.Logger, ope
 	}
 	if instanceStatus == nil {
 		// Check if there is an existing instance with machine name, in case where instance ID would not have been stored in machine status
-		if instanceStatus, err = computeService.GetInstanceStatusByName(openStackMachine, openStackMachine.Name); err == nil {
-			if instanceStatus != nil {
-				return instanceStatus, nil
-			}
-			if openStackMachine.Status.InstanceID != nil {
-				logger.Info("Not reconciling machine in failed state. The previously existing OpenStack instance is no longer available")
-				conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceNotFoundReason, clusterv1.ConditionSeverityError, "virtual machine no longer exists")
-				openStackMachine.SetFailure(capierrors.UpdateMachineError, errors.New("virtual machine no longer exists"))
-				return nil, nil
-			}
+		instanceStatus, err = computeService.GetInstanceStatusByName(openStackMachine, openStackMachine.Name)
+		if err != nil {
+			logger.Info("Unable to get OpenStack instance by name", "name", openStackMachine.Name)
+			conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceCreateFailedReason, clusterv1.ConditionSeverityError, err.Error())
+			return nil, err
+		}
+		if instanceStatus != nil {
+			return instanceStatus, nil
 		}
+		if openStackMachine.Status.InstanceID != nil {
+			logger.Info("Not reconciling machine in failed state. The previously existing OpenStack instance is no longer available")
+			conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceNotFoundReason, clusterv1.ConditionSeverityError, "virtual machine no longer exists")
+			openStackMachine.SetFailure(capierrors.UpdateMachineError, errors.New("virtual machine no longer exists"))
+			return nil, nil
+		}
+
 		instanceSpec, err := machineToInstanceSpec(openStackCluster, machine, openStackMachine, userData)
 		if err != nil {
 			return nil, err

docs/book/src/clusteropenstack/configuration.md

@@ -565,6 +565,7 @@ permitted from anywhere, as with the default rules).
 We can add security group rules that authorize traffic from all nodes via `allNodesSecurityGroupRules`.
 It takes a list of security groups rules that should be applied to selected nodes.
 The following rule fields are mutually exclusive: `remoteManagedGroups`, `remoteGroupID` and `remoteIPPrefix`.
+If none of these fields are set, the rule will have a remote IP prefix of `0.0.0.0/0` per Neutron default.
 
 Valid values for `remoteManagedGroups` are `controlplane`, `worker` and `bastion`.
 

docs/book/src/topics/crd-changes/v1alpha7-to-v1beta1.md

@@ -396,6 +396,9 @@ The field `managedSecurityGroups` is now a pointer to a `ManagedSecurityGroups`
 Also, we can now add security group rules that authorize traffic from all nodes via `allNodesSecurityGroupRules`.
 It takes a list of security groups rules that should be applied to selected nodes.
 The following rule fields are mutually exclusive: `remoteManagedGroups`, `remoteGroupID` and `remoteIPPrefix`.
+If none of these fields are set, the rule will have a remote IP prefix of `0.0.0.0/0` per Neutron default.
+
+```yaml
 Valid values for `remoteManagedGroups` are `controlplane`, `worker` and `bastion`.
 
 Also, `OpenStackCluster.Spec.AllowAllInClusterTraffic` moved under `ManagedSecurityGroups`.

pkg/cloud/services/loadbalancer/loadbalancer.go

@@ -92,9 +92,10 @@ func (s *Service) ReconcileLoadBalancer(openStackCluster *infrav1.OpenStackClust
 
 	if lb.ProvisioningStatus != loadBalancerProvisioningStatusActive {
 		var err error
-		lb, err = s.waitForLoadBalancerActive(lb.ID)
+		lbID := lb.ID
+		lb, err = s.waitForLoadBalancerActive(lbID)
 		if err != nil {
-			return false, fmt.Errorf("load balancer %q with id %s is not active after timeout: %v", loadBalancerName, lb.ID, err)
+			return false, fmt.Errorf("load balancer %q with id %s is not active after timeout: %v", loadBalancerName, lbID, err)
 		}
 	}
 

pkg/cloud/services/loadbalancer/loadbalancer_test.go

@@ -45,6 +45,13 @@ func Test_ReconcileLoadBalancer(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
 
+	// Shortcut wait timeout
+	backoffDurationPrev := backoff.Duration
+	backoff.Duration = 0
+	defer func() {
+		backoff.Duration = backoffDurationPrev
+	}()
+
 	// Stub the call to net.LookupHost
 	lookupHost = func(host string) (addrs *string, err error) {
 		if net.ParseIP(host) != nil {
@@ -139,6 +146,27 @@ func Test_ReconcileLoadBalancer(t *testing.T) {
 			},
 			wantError: nil,
 		},
+		{
+			name: "reconcile loadbalancer in non active state should timeout",
+			expectNetwork: func(*mock.MockNetworkClientMockRecorder) {
+				// add network api call results here
+			},
+			expectLoadBalancer: func(m *mock.MockLbClientMockRecorder) {
+				pendingLB := loadbalancers.LoadBalancer{
+					ID:                 "aaaaaaaa-bbbb-cccc-dddd-333333333333",
+					Name:               "k8s-clusterapi-cluster-AAAAA-kubeapi",
+					ProvisioningStatus: "PENDING_CREATE",
+				}
+
+				// return existing loadbalancer in non-active state
+				lbList := []loadbalancers.LoadBalancer{pendingLB}
+				m.ListLoadBalancers(loadbalancers.ListOpts{Name: pendingLB.Name}).Return(lbList, nil)
+
+				// wait for loadbalancer until it times out
+				m.GetLoadBalancer("aaaaaaaa-bbbb-cccc-dddd-333333333333").Return(&pendingLB, nil).Return(&pendingLB, nil).AnyTimes()
+			},
+			wantError: fmt.Errorf("load balancer \"k8s-clusterapi-cluster-AAAAA-kubeapi\" with id aaaaaaaa-bbbb-cccc-dddd-333333333333 is not active after timeout: timed out waiting for the condition"),
+		},
 	}
 	for _, tt := range lbtests {
 		t.Run(tt.name, func(t *testing.T) {

pkg/cloud/services/networking/securitygroups.go

@@ -297,10 +297,6 @@ func getAllNodesRules(remoteManagedGroups map[string]string, allNodesSecurityGro
 
 // validateRemoteManagedGroups validates that the remoteManagedGroups target existing managed security groups.
 func validateRemoteManagedGroups(remoteManagedGroups map[string]string, ruleRemoteManagedGroups []infrav1.ManagedSecurityGroupName) error {
-	if len(ruleRemoteManagedGroups) == 0 {
-		return fmt.Errorf("remoteManagedGroups is required")
-	}
-
 	for _, group := range ruleRemoteManagedGroups {
 		if _, ok := remoteManagedGroups[group.String()]; !ok {
 			return fmt.Errorf("remoteManagedGroups: %s is not a valid remote managed security group", group)

pkg/cloud/services/networking/securitygroups_test.go

@@ -49,19 +49,14 @@ func TestValidateRemoteManagedGroups(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "Valid rule with missing remoteManagedGroups",
+			name: "Valid rule with no remoteManagedGroups",
 			rule: infrav1.SecurityGroupRuleSpec{
-				PortRangeMin: ptr.To(22),
-				PortRangeMax: ptr.To(22),
-				Protocol:     ptr.To("tcp"),
+				PortRangeMin:   ptr.To(22),
+				PortRangeMax:   ptr.To(22),
+				Protocol:       ptr.To("tcp"),
+				RemoteIPPrefix: ptr.To("0.0.0.0/0"),
 			},
-			remoteManagedGroups: map[string]string{
-				"self":         "self",
-				"controlplane": "1",
-				"worker":       "2",
-				"bastion":      "3",
-			},
-			wantErr: true,
+			wantErr: false,
 		},
 		{
 			name: "Valid rule with remoteManagedGroups",
@@ -171,6 +166,70 @@ func TestGetAllNodesRules(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "Valid remoteIPPrefix in a rule",
+			remoteManagedGroups: map[string]string{
+				"controlplane": "1",
+				"worker":       "2",
+			},
+			allNodesSecurityGroupRules: []infrav1.SecurityGroupRuleSpec{
+				{
+					Protocol:       ptr.To("tcp"),
+					PortRangeMin:   ptr.To(22),
+					PortRangeMax:   ptr.To(22),
+					RemoteIPPrefix: ptr.To("0.0.0.0/0"),
+				},
+			},
+			wantRules: []resolvedSecurityGroupRuleSpec{
+				{
+					Protocol:       "tcp",
+					PortRangeMin:   22,
+					PortRangeMax:   22,
+					RemoteIPPrefix: "0.0.0.0/0",
+				},
+			},
+		},
+		{
+			name: "Valid allNodesSecurityGroupRules with no remote parameter",
+			remoteManagedGroups: map[string]string{
+				"controlplane": "1",
+				"worker":       "2",
+			},
+			allNodesSecurityGroupRules: []infrav1.SecurityGroupRuleSpec{
+				{
+					Protocol:     ptr.To("tcp"),
+					PortRangeMin: ptr.To(22),
+					PortRangeMax: ptr.To(22),
+				},
+			},
+			wantRules: []resolvedSecurityGroupRuleSpec{
+				{
+					Protocol:     "tcp",
+					PortRangeMin: 22,
+					PortRangeMax: 22,
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "Invalid allNodesSecurityGroupRules with bastion while remoteManagedGroups does not have bastion",
+			remoteManagedGroups: map[string]string{
+				"controlplane": "1",
+				"worker":       "2",
+			},
+			allNodesSecurityGroupRules: []infrav1.SecurityGroupRuleSpec{
+				{
+					Protocol:     ptr.To("tcp"),
+					PortRangeMin: ptr.To(22),
+					PortRangeMax: ptr.To(22),
+					RemoteManagedGroups: []infrav1.ManagedSecurityGroupName{
+						"bastion",
+					},
+				},
+			},
+			wantRules: nil,
+			wantErr:   true,
+		},
 		{
 			name: "Invalid allNodesSecurityGroupRules with wrong remoteManagedGroups",
 			remoteManagedGroups: map[string]string{