OCPBUGS-23362: Set status on SG deletion when AWS returns error

Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
openshift · openshift-merge-bot · Dec 22, 2023 · Dec 12, 2023 · Dec 21, 2023 · 14fae36901cb9f2a81de5f3c635b78d8a3bdf394
commit 14fae36901cb9f2a81de5f3c635b78d8a3bdf394
diff --git a/api/hypershift/v1beta1/hostedcluster_conditions.go b/api/hypershift/v1beta1/hostedcluster_conditions.go
@@ -146,6 +146,12 @@ const (
 	// blocked from creating machines.
 	AWSDefaultSecurityGroupCreated ConditionType = "AWSDefaultSecurityGroupCreated"
 
+	// AWSDefaultSecurityGroupDeleted indicates whether the default security group
+	// for AWS workers has been deleted.
+	// A failure here indicates that the Security Group has some dependencies that
+	// there are still pending cloud resources to be deleted that are using that SG.
+	AWSDefaultSecurityGroupDeleted ConditionType = "AWSDefaultSecurityGroupDeleted"
+
 	// PlatformCredentialsFound indicates that credentials required for the
 	// desired platform are valid.
 	// A failure here is unlikely to resolve without the changing user input.

diff --git a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
@@ -306,15 +306,36 @@ func (r *HostedControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.R
 
 	// Return early if deleted
 	if !hostedControlPlane.DeletionTimestamp.IsZero() {
-
+		condition := &metav1.Condition{
+			Type: string(hyperv1.AWSDefaultSecurityGroupDeleted),
+		}
 		if shouldCleanupCloudResources(r.Log, hostedControlPlane) {
 			if err := r.destroyAWSDefaultSecurityGroup(ctx, hostedControlPlane); err != nil {
+				condition.Message = "failed to delete AWS default security group"
+				condition.Reason = hyperv1.AWSErrorReason
+				condition.Status = metav1.ConditionFalse
+				meta.SetStatusCondition(&hostedControlPlane.Status.Conditions, *condition)
+
+				if err := r.Client.Status().Patch(ctx, hostedControlPlane, client.MergeFromWithOptions(originalHostedControlPlane, client.MergeFromWithOptimisticLock{})); err != nil {
+					return ctrl.Result{}, fmt.Errorf("failed to update status on hcp for security group deletion: %w. Condition error message: %v", err, condition.Message)
+				}
+
 				if awsErrorCode(err) == "UnauthorizedOperation" {
 					r.Log.Info("Skipping AWS default security group deletion because the operator is not authorized to delete it.")
 				} else {
 					return ctrl.Result{}, fmt.Errorf("failed to delete AWS default security group: %w", err)
 				}
+			} else {
+				condition.Message = hyperv1.AllIsWellMessage
+				condition.Reason = hyperv1.AsExpectedReason
+				condition.Status = metav1.ConditionTrue
+				meta.SetStatusCondition(&hostedControlPlane.Status.Conditions, *condition)
+
+				if err := r.Client.Status().Patch(ctx, hostedControlPlane, client.MergeFromWithOptions(originalHostedControlPlane, client.MergeFromWithOptimisticLock{})); err != nil {
+					return ctrl.Result{}, fmt.Errorf("failed to update status on hcp for security group deletion: %w. Condition message: %v", err, condition.Message)
+				}
 			}
+
 			done, err := r.removeCloudResources(ctx, hostedControlPlane)
 			if err != nil {
 				return ctrl.Result{}, fmt.Errorf("failed to ensure cloud resources are removed: %w", err)
@@ -4307,6 +4328,8 @@ func createAWSDefaultSecurityGroup(ctx context.Context, ec2Client ec2iface.EC2AP
 }
 
 func (r *HostedControlPlaneReconciler) destroyAWSDefaultSecurityGroup(ctx context.Context, hcp *hyperv1.HostedControlPlane) error {
+	log := ctrl.LoggerFrom(ctx)
+
 	if hcp.Spec.Platform.Type != hyperv1.AWSPlatform {
 		return nil
 	}
@@ -4325,7 +4348,13 @@ func (r *HostedControlPlaneReconciler) destroyAWSDefaultSecurityGroup(ctx contex
 			GroupId:       sg.GroupId,
 			IpPermissions: sg.IpPermissions,
 		}); err != nil {
-			return fmt.Errorf("failed to revoke security group ingress permissions for %s: %w", awssdk.StringValue(sg.GroupId), err)
+			code := "UnknownError"
+			if awsErr, ok := err.(awserr.Error); ok {
+				code = awsErr.Code()
+			}
+			log.Error(err, "failed to revoke security group ingress permissions", "SecurityGroupID", awssdk.StringValue(sg.GroupId), "code", code)
+
+			return fmt.Errorf("failed to revoke security group ingress rules: %s", code)
 		}
 	}
 
@@ -4334,15 +4363,28 @@ func (r *HostedControlPlaneReconciler) destroyAWSDefaultSecurityGroup(ctx contex
 			GroupId:       sg.GroupId,
 			IpPermissions: sg.IpPermissionsEgress,
 		}); err != nil {
-			return fmt.Errorf("failed to revoke security group egress permissions for %s: %w", awssdk.StringValue(sg.GroupId), err)
+			code := "UnknownError"
+			if awsErr, ok := err.(awserr.Error); ok {
+				code = awsErr.Code()
+			}
+			log.Error(err, "failed to revoke security group egress permissions", "SecurityGroupID", awssdk.StringValue(sg.GroupId), "code", code)
+
+			return fmt.Errorf("failed to revoke security group egress rules: %s", code)
 		}
 	}
 
 	if _, err = r.ec2Client.DeleteSecurityGroupWithContext(ctx, &ec2.DeleteSecurityGroupInput{
 		GroupId: sg.GroupId,
 	}); err != nil {
-		return fmt.Errorf("failed to delete security group %s: %w", awssdk.StringValue(sg.GroupId), err)
+		code := "UnknownError"
+		if awsErr, ok := err.(awserr.Error); ok {
+			code = awsErr.Code()
+		}
+		log.Error(err, "failed to delete security group", "SecurityGroupID", awssdk.StringValue(sg.GroupId), "code", code)
+
+		return fmt.Errorf("failed to delete security group %s: %s", awssdk.StringValue(sg.GroupId), code)
 	}
+
 	return nil
 
 }

diff --git a/docs/content/reference/api.md b/docs/content/reference/api.md
@@ -2981,6 +2981,12 @@ for AWS workers has been created.
 A failure here indicates that NodePools without a security group will be
 blocked from creating machines.</p>
 </td>
+</tr><tr><td><p>&#34;AWSDefaultSecurityGroupDeleted&#34;</p></td>
+<td><p>AWSDefaultSecurityGroupDeleted indicates whether the default security group
+for AWS workers has been deleted.
+A failure here indicates that the Security Group has some dependencies that
+there are still pending cloud resources to be deleted that are using that SG.</p>
+</td>
 </tr><tr><td><p>&#34;AWSEndpointAvailable&#34;</p></td>
 <td><p>AWSEndpointServiceAvailable indicates whether the AWS Endpoint has been
 created in the guest VPC</p>

diff --git a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
@@ -376,6 +376,34 @@ func (r *HostedClusterReconciler) reconcile(ctx context.Context, req ctrl.Reques
 		}
 	}
 
+	// Bubble up AWSDefaultSecurityGroupDeleted condition from the hostedControlPlane.
+	// We set this condition even if the HC is being deleted, so we can report blocking objects on deletion.
+	{
+		if hcp != nil && hcp.DeletionTimestamp != nil {
+			freshCondition := &metav1.Condition{
+				Type:               string(hyperv1.AWSDefaultSecurityGroupDeleted),
+				Status:             metav1.ConditionUnknown,
+				Reason:             hyperv1.StatusUnknownReason,
+				ObservedGeneration: hcluster.Generation,
+			}
+
+			securityGroupDeletionCondition := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.AWSDefaultSecurityGroupDeleted))
+			if securityGroupDeletionCondition != nil {
+				freshCondition = securityGroupDeletionCondition
+			}
+
+			oldCondition := meta.FindStatusCondition(hcluster.Status.Conditions, string(hyperv1.AWSDefaultSecurityGroupDeleted))
+			if oldCondition == nil || oldCondition.Message != freshCondition.Message {
+				freshCondition.ObservedGeneration = hcluster.Generation
+				meta.SetStatusCondition(&hcluster.Status.Conditions, *freshCondition)
+				// Persist status updates
+				if err := r.Client.Status().Update(ctx, hcluster); err != nil {
+					return ctrl.Result{}, fmt.Errorf("failed to update status: %w", err)
+				}
+			}
+		}
+	}
+
 	// Bubble up CloudResourcesDestroyed condition from the hostedControlPlane.
 	// We set this condition even if the HC is being deleted, so we can construct SLIs for deletion times.
 	{