OCPBUGS-37967: rebase CAPO for 4.16 #319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

openshift-merge-bot merged 13 commits into openshift:release-4.16 from shiftstack:merge-bot-release-4.16

Aug 5, 2024

OWNERS_ALIASES

-Original file line number
+Diff line change
@@ Expand Up / @@ -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

-Original file line number
+Diff line change
@@ Expand Up @@
     	}
     	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 {
@@ Expand Down @@

api/v1alpha6/openstackmachine_conversion.go

-Original file line number
+Diff line change
@@ Expand Up @@
     	}
     	if in.IdentityRef != nil {
-    		out.IdentityRef = &OpenStackIdentityReference{Name: in.IdentityRef.Name}
     		out.CloudName = in.IdentityRef.CloudName
     	}
@@ Expand Down @@

api/v1alpha6/types_conversion.go

-Original file line number
+Diff line change
@@ Expand Up @@
     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
     }
@@ Expand Down @@

api/v1alpha7/openstackcluster_conversion.go

-Original file line number
+Diff line change
@@ Expand Up @@
     	}
     	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 {
@@ Expand Down @@

api/v1alpha7/types_conversion.go

-Original file line number
+Diff line change
@@ Expand Up @@
     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
     }
@@ Expand Down @@

controllers/openstackmachine_controller.go

-Original file line number
+Diff line change
@@ Expand Up @@
     	}
     	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
@@ Expand Down @@

docs/book/src/clusteropenstack/configuration.md

-Original file line number
+Diff line change
@@ Expand Up / @@ -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`.
@@ Expand Down @@

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

-Original file line number
+Diff line change
@@ Expand Up @@
     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`.
@@ Expand Down @@

pkg/cloud/services/loadbalancer/loadbalancer.go

-Original file line number
+Diff line change
@@ Expand Up @@
     	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)
     		}
     	}
@@ Expand Down @@

pkg/cloud/services/loadbalancer/loadbalancer_test.go

-Original file line number
+Diff line change
@@ Expand Up / @@ -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 {
@@ Expand Down Expand Up / @@ -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) {
@@ Expand Down @@

pkg/cloud/services/networking/securitygroups.go

-Original file line number
+Diff line change
@@ Expand Up @@
     // 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)
@@ Expand Down @@

pkg/cloud/services/networking/securitygroups_test.go

-Original file line number
+Diff line change
@@ Expand Up / @@ -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",
@@ Expand Down Expand Up / @@ -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{
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-37967: rebase CAPO for 4.16 #319

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!