Add missing conditions on network error paths in OpenStackCluster reconciler

Several error paths in reconcileNetworkComponents and related functions
were missing proper condition updates, which meant the cluster status
would not accurately reflect failures in network reconciliation.

Changes:
- ReconcileExternalNetwork failure now sets NetworkReadyCondition=False
- ManagedSubnets > 1 validation error now sets NetworkReadyCondition=False
  and calls handleUpdateOSCError (previously returned error silently)
- resolveLoadBalancerNetwork failure now sets NetworkReadyCondition=False
- loadbalancer.NewService failure in reconcileControlPlaneEndpoint now
  calls handleUpdateOSCError to set ReadyCondition=False
- resolveLoadBalancerNetwork: always call handleUpdateOSCError on
  GetNetworkByParam failure, not only for ErrFilterMatch errors

Tests added for external network failure and ManagedSubnets validation.

Signed-off-by: Dong Ma <winterma.dong@gmail.com>

Author: larainema

controllers/openstackcluster_controller.go

@@ -687,9 +687,7 @@ func resolveLoadBalancerNetwork(openStackCluster *infrav1.OpenStackCluster, netw
 	if lbSpec.Network != nil {
 		lbNet, err := networkingService.GetNetworkByParam(lbSpec.Network)
 		if err != nil {
-			if errors.Is(err, capoerrors.ErrFilterMatch) {
-				handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to find loadbalancer network: %w", err))
-			}
+			handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to find loadbalancer network: %w", err))
 			return fmt.Errorf("failed to find network: %w", err)
 		}
 
@@ -742,6 +740,12 @@ func reconcileNetworkComponents(scope *scope.WithLogger, cluster *clusterv1.Clus
 
 	err = networkingService.ReconcileExternalNetwork(openStackCluster)
 	if err != nil {
+		conditions.Set(openStackCluster, metav1.Condition{
+			Type:    infrav1.NetworkReadyCondition,
+			Status:  metav1.ConditionFalse,
+			Reason:  infrav1.NetworkReconcileFailedReason,
+			Message: fmt.Sprintf("Failed to reconcile external network: %v", err),
+		})
 		handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to reconcile external network: %w", err))
 		return fmt.Errorf("failed to reconcile external network: %w", err)
 	}
@@ -755,11 +759,25 @@ func reconcileNetworkComponents(scope *scope.WithLogger, cluster *clusterv1.Clus
 			return err
 		}
 	} else {
-		return fmt.Errorf("failed to reconcile network: ManagedSubnets only supports one element, %d provided", len(openStackCluster.Spec.ManagedSubnets))
+		err := fmt.Errorf("failed to reconcile network: ManagedSubnets only supports one element, %d provided", len(openStackCluster.Spec.ManagedSubnets))
+		conditions.Set(openStackCluster, metav1.Condition{
+			Type:    infrav1.NetworkReadyCondition,
+			Status:  metav1.ConditionFalse,
+			Reason:  infrav1.NetworkReconcileFailedReason,
+			Message: err.Error(),
+		})
+		handleUpdateOSCError(openStackCluster, err)
+		return err
 	}
 
 	err = resolveLoadBalancerNetwork(openStackCluster, networkingService)
 	if err != nil {
+		conditions.Set(openStackCluster, metav1.Condition{
+			Type:    infrav1.NetworkReadyCondition,
+			Status:  metav1.ConditionFalse,
+			Reason:  infrav1.NetworkReconcileFailedReason,
+			Message: fmt.Sprintf("Failed to reconcile load balancer network: %v", err),
+		})
 		handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to reconcile loadbalancer network: %w", err))
 		return fmt.Errorf("failed to reconcile loadbalancer network: %w", err)
 	}
@@ -985,6 +1003,7 @@ func reconcileControlPlaneEndpoint(scope *scope.WithLogger, networkingService *n
 	case openStackCluster.Spec.APIServerLoadBalancer.IsEnabled():
 		loadBalancerService, err := loadbalancer.NewService(scope)
 		if err != nil {
+			handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to create load balancer service: %w", err))
 			return err
 		}
 

controllers/openstackcluster_controller_test.go

@@ -925,6 +925,95 @@ var _ = Describe("OpenStackCluster controller", func() {
 		Expect(conditions.IsTrue(testCluster, infrav1.APIEndpointReadyCondition)).To(BeTrue())
 	})
 
+	It("should set NetworkReadyCondition to False when ManagedSubnets has more than one element", func() {
+		testCluster.SetName("managed-subnets-too-many")
+		testCluster.Spec = infrav1.OpenStackClusterSpec{
+			IdentityRef: infrav1.OpenStackIdentityReference{
+				Name:      "test-creds",
+				CloudName: "openstack",
+			},
+			DisableExternalNetwork:     ptr.To(true),
+			DisableAPIServerFloatingIP: ptr.To(true),
+			APIServerFixedIP:           ptr.To("192.168.0.10"),
+			ManagedSubnets: []infrav1.SubnetSpec{
+				{CIDR: "192.168.0.0/24", DNSNameservers: []string{"8.8.8.8"}},
+			},
+		}
+		err := k8sClient.Create(ctx, testCluster)
+		Expect(err).To(BeNil())
+		err = k8sClient.Create(ctx, capiCluster)
+		Expect(err).To(BeNil())
+
+		// Add a second managed subnet in memory to bypass CRD validation
+		// (maxItems: 1) and test the controller-level check.
+		testCluster.Spec.ManagedSubnets = append(testCluster.Spec.ManagedSubnets,
+			infrav1.SubnetSpec{CIDR: "192.168.1.0/24", DNSNameservers: []string{"8.8.8.8"}},
+		)
+
+		log := GinkgoLogr
+		clientScope, err := mockScopeFactory.NewClientScopeFromObject(ctx, k8sClient, nil, log, testCluster)
+		Expect(err).To(BeNil())
+		scope := scope.NewWithLogger(clientScope, log)
+
+		err = reconcileNetworkComponents(scope, capiCluster, testCluster)
+		Expect(err).ToNot(BeNil())
+		Expect(err.Error()).To(ContainSubstring("ManagedSubnets only supports one element"))
+
+		// Verify NetworkReadyCondition is set to False
+		Expect(conditions.IsFalse(testCluster, infrav1.NetworkReadyCondition)).To(BeTrue())
+		condition := conditions.Get(testCluster, infrav1.NetworkReadyCondition)
+		Expect(condition).ToNot(BeNil())
+		Expect(condition.Reason).To(Equal(infrav1.NetworkReconcileFailedReason))
+
+		// Verify ReadyCondition is set to False
+		Expect(conditions.IsFalse(testCluster, clusterv1.ReadyCondition)).To(BeTrue())
+	})
+
+	It("should set NetworkReadyCondition to False when external network reconciliation fails", func() {
+		const externalNetworkID = "a42211a2-4d2c-426f-9413-830e4b4abbbc"
+
+		testCluster.SetName("external-network-failure")
+		testCluster.Spec = infrav1.OpenStackClusterSpec{
+			IdentityRef: infrav1.OpenStackIdentityReference{
+				Name:      "test-creds",
+				CloudName: "openstack",
+			},
+			ExternalNetwork: &infrav1.NetworkParam{
+				ID: ptr.To(externalNetworkID),
+			},
+			DisableAPIServerFloatingIP: ptr.To(true),
+			APIServerFixedIP:           ptr.To("192.168.0.10"),
+		}
+		err := k8sClient.Create(ctx, testCluster)
+		Expect(err).To(BeNil())
+		err = k8sClient.Create(ctx, capiCluster)
+		Expect(err).To(BeNil())
+
+		log := GinkgoLogr
+		clientScope, err := mockScopeFactory.NewClientScopeFromObject(ctx, k8sClient, nil, log, testCluster)
+		Expect(err).To(BeNil())
+		scope := scope.NewWithLogger(clientScope, log)
+
+		networkClientRecorder := mockScopeFactory.NetworkClient.EXPECT()
+
+		// External network lookup fails
+		networkClientRecorder.GetNetwork(externalNetworkID).Return(nil, fmt.Errorf("external network not found"))
+
+		err = reconcileNetworkComponents(scope, capiCluster, testCluster)
+		Expect(err).ToNot(BeNil())
+		Expect(err.Error()).To(ContainSubstring("failed to reconcile external network"))
+
+		// Verify NetworkReadyCondition is set to False
+		Expect(conditions.IsFalse(testCluster, infrav1.NetworkReadyCondition)).To(BeTrue())
+		condition := conditions.Get(testCluster, infrav1.NetworkReadyCondition)
+		Expect(condition).ToNot(BeNil())
+		Expect(condition.Reason).To(Equal(infrav1.NetworkReconcileFailedReason))
+		Expect(condition.Message).To(ContainSubstring("Failed to reconcile external network"))
+
+		// Verify ReadyCondition is set to False
+		Expect(conditions.IsFalse(testCluster, clusterv1.ReadyCondition)).To(BeTrue())
+	})
+
 	It("should set NetworkReadyCondition to False when network lookup fails", func() {
 		const clusterNetworkID = "6c90b532-7ba0-418a-a276-5ae55060b5b0"
 
@@ -1161,6 +1250,110 @@ var _ = Describe("OpenStackCluster controller", func() {
 		Expect(conditions.IsTrue(testCluster, infrav1.NetworkReadyCondition)).To(BeTrue())
 	})
 
+	It("should clear ReadyCondition when security group reconciliation fails on a previously ready cluster", func() {
+		const clusterNetworkID = "6c90b532-7ba0-418a-a276-5ae55060b5b0"
+		const clusterSubnetID = "cad5a91a-36de-4388-823b-b0cc82cadfdc"
+
+		testCluster.SetName("sg-failure-previously-ready")
+		testCluster.Spec = infrav1.OpenStackClusterSpec{
+			IdentityRef: infrav1.OpenStackIdentityReference{
+				Name:      "test-creds",
+				CloudName: "openstack",
+			},
+			Network: &infrav1.NetworkParam{
+				ID: ptr.To(clusterNetworkID),
+			},
+			DisableExternalNetwork:     ptr.To(true),
+			DisableAPIServerFloatingIP: ptr.To(true),
+			APIServerFixedIP:           ptr.To("192.168.0.10"),
+			ManagedSecurityGroups: &infrav1.ManagedSecurityGroups{
+				AllNodesSecurityGroupRules: []infrav1.SecurityGroupRuleSpec{
+					{
+						Direction: "ingress",
+						Protocol:  ptr.To("tcp"),
+						RemoteManagedGroups: []infrav1.ManagedSecurityGroupName{
+							"worker",
+						},
+					},
+				},
+			},
+		}
+		err := k8sClient.Create(ctx, testCluster)
+		Expect(err).To(BeNil())
+		err = k8sClient.Create(ctx, capiCluster)
+		Expect(err).To(BeNil())
+
+		// Simulate a previously successful reconcile by pre-setting conditions to True.
+		// This is the scenario from https://github.com/kubernetes-sigs/cluster-api-provider-openstack/issues/2993
+		// where the cluster was already Ready and a subsequent security group update fails.
+		conditions.Set(testCluster, metav1.Condition{
+			Type:   clusterv1.ReadyCondition,
+			Status: metav1.ConditionTrue,
+			Reason: infrav1.ReadyConditionReason,
+		})
+		conditions.Set(testCluster, metav1.Condition{
+			Type:   infrav1.SecurityGroupsReadyCondition,
+			Status: metav1.ConditionTrue,
+			Reason: infrav1.ReadyConditionReason,
+		})
+		conditions.Set(testCluster, metav1.Condition{
+			Type:   infrav1.NetworkReadyCondition,
+			Status: metav1.ConditionTrue,
+			Reason: infrav1.ReadyConditionReason,
+		})
+
+		// Verify preconditions: cluster appears Ready
+		Expect(conditions.IsTrue(testCluster, clusterv1.ReadyCondition)).To(BeTrue())
+		Expect(conditions.IsTrue(testCluster, infrav1.SecurityGroupsReadyCondition)).To(BeTrue())
+
+		log := GinkgoLogr
+		clientScope, err := mockScopeFactory.NewClientScopeFromObject(ctx, k8sClient, nil, log, testCluster)
+		Expect(err).To(BeNil())
+		scope := scope.NewWithLogger(clientScope, log)
+
+		networkClientRecorder := mockScopeFactory.NetworkClient.EXPECT()
+
+		// Network lookup succeeds
+		networkClientRecorder.GetNetwork(clusterNetworkID).Return(&networks.Network{
+			ID:   clusterNetworkID,
+			Name: "cluster-network",
+		}, nil)
+
+		// Subnet lookup succeeds
+		networkClientRecorder.ListSubnet(subnets.ListOpts{
+			NetworkID: clusterNetworkID,
+		}).Return([]subnets.Subnet{
+			{
+				ID:   clusterSubnetID,
+				Name: "cluster-subnet",
+				CIDR: "192.168.0.0/24",
+			},
+		}, nil)
+
+		// Security group reconciliation fails (e.g. rule conflict as reported in #2993)
+		networkClientRecorder.ListSecGroup(gomock.Any()).Return([]groups.SecGroup{}, nil).AnyTimes()
+		networkClientRecorder.CreateSecGroup(gomock.Any()).Return(nil, fmt.Errorf("SecurityGroupRuleExists")).AnyTimes()
+
+		err = reconcileNetworkComponents(scope, capiCluster, testCluster)
+		Expect(err).ToNot(BeNil())
+		Expect(err.Error()).To(ContainSubstring("failed to reconcile security groups"))
+
+		// Verify ReadyCondition is now False (was True before)
+		Expect(conditions.IsFalse(testCluster, clusterv1.ReadyCondition)).To(BeTrue())
+		readyCondition := conditions.Get(testCluster, clusterv1.ReadyCondition)
+		Expect(readyCondition).ToNot(BeNil())
+		Expect(readyCondition.Reason).To(Equal(infrav1.OpenStackErrorReason))
+
+		// Verify SecurityGroupsReadyCondition is now False (was True before)
+		Expect(conditions.IsFalse(testCluster, infrav1.SecurityGroupsReadyCondition)).To(BeTrue())
+		sgCondition := conditions.Get(testCluster, infrav1.SecurityGroupsReadyCondition)
+		Expect(sgCondition).ToNot(BeNil())
+		Expect(sgCondition.Reason).To(Equal(infrav1.SecurityGroupReconcileFailedReason))
+
+		// NetworkReadyCondition should remain True since network reconciliation succeeded
+		Expect(conditions.IsTrue(testCluster, infrav1.NetworkReadyCondition)).To(BeTrue())
+	})
+
 	It("should set APIEndpointReadyCondition to False when floating IP creation fails", func() {
 		const externalNetworkID = "a42211a2-4d2c-426f-9413-830e4b4abbbc"
 		const clusterNetworkID = "6c90b532-7ba0-418a-a276-5ae55060b5b0"