CNTRLPLANE-3562, CNTRLPLANE-3563: test(healthcheck): add unit tests for AWS identity provider

control-plane-operator/controllers/healthcheck/aws.go

@@ -7,6 +7,7 @@ import (
 
 	hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
 	"github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane"
+	"github.com/openshift/hypershift/support/awsapi"
 
 	"github.com/aws/aws-sdk-go-v2/service/ec2"
 	"github.com/aws/smithy-go"
@@ -33,6 +34,10 @@ func awsHealthCheckIdentityProvider(ctx context.Context, hcp *hyperv1.HostedCont
 	}
 
 	ec2Client, _ := hostedcontrolplane.GetEC2Client(ctx)
+	return validateAWSIdentityProvider(ctx, hcp, ec2Client)
+}
+
+func validateAWSIdentityProvider(ctx context.Context, hcp *hyperv1.HostedControlPlane, ec2Client awsapi.EC2API) error {
 	if ec2Client == nil {
 		// EC2 client is not available (token minting may have failed)
 		condition := metav1.Condition{

control-plane-operator/controllers/healthcheck/aws_test.go

@@ -1,39 +1,47 @@
 package healthcheck
 
 import (
+	"fmt"
 	"testing"
 
 	hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
 	"github.com/openshift/hypershift/support/api"
+	"github.com/openshift/hypershift/support/awsapi"
+
+	"github.com/aws/aws-sdk-go-v2/service/ec2"
+	"github.com/aws/smithy-go"
 
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	"go.uber.org/mock/gomock"
 )
 
 // TestAWSHealthCheckIdentityProviderConditionLogic tests the condition setting logic
-// when KAS is not available. This validates that the function correctly sets the
-// ValidAWSIdentityProvider condition to Unknown when it cannot perform validation.
+// for the ValidAWSIdentityProvider condition across different code paths.
 func TestAWSHealthCheckIdentityProviderConditionLogic(t *testing.T) {
 	testCases := []struct {
 		name            string
 		kasCondition    *metav1.Condition
+		setupEC2Mock    func(*gomock.Controller) awsapi.EC2API
 		expectedStatus  metav1.ConditionStatus
 		expectedReason  string
 		expectedMessage string
+		expectError     bool
 	}{
 		{
-			name:            "KAS not available - condition missing",
+			name:            "When KAS condition is missing, it should set ValidAWSIdentityProvider to Unknown",
 			kasCondition:    nil,
 			expectedStatus:  metav1.ConditionUnknown,
 			expectedReason:  hyperv1.StatusUnknownReason,
 			expectedMessage: "Cannot validate AWS identity provider while KubeAPIServer is not available",
 		},
 		{
-			name: "KAS not available - condition False",
+			name: "When KAS condition is False, it should set ValidAWSIdentityProvider to Unknown",
 			kasCondition: &metav1.Condition{
 				Type:   string(hyperv1.KubeAPIServerAvailable),
 				Status: metav1.ConditionFalse,
@@ -43,7 +51,7 @@ func TestAWSHealthCheckIdentityProviderConditionLogic(t *testing.T) {
 			expectedMessage: "Cannot validate AWS identity provider while KubeAPIServer is not available",
 		},
 		{
-			name: "KAS not available - condition Unknown",
+			name: "When KAS condition is Unknown, it should set ValidAWSIdentityProvider to Unknown",
 			kasCondition: &metav1.Condition{
 				Type:   string(hyperv1.KubeAPIServerAvailable),
 				Status: metav1.ConditionUnknown,
@@ -53,7 +61,7 @@ func TestAWSHealthCheckIdentityProviderConditionLogic(t *testing.T) {
 			expectedMessage: "Cannot validate AWS identity provider while KubeAPIServer is not available",
 		},
 		{
-			name: "KAS available",
+			name: "When KAS is available but EC2 client is nil, it should set ValidAWSIdentityProvider to Unknown",
 			kasCondition: &metav1.Condition{
 				Type:   string(hyperv1.KubeAPIServerAvailable),
 				Status: metav1.ConditionTrue,
@@ -62,11 +70,83 @@ func TestAWSHealthCheckIdentityProviderConditionLogic(t *testing.T) {
 			expectedReason:  hyperv1.StatusUnknownReason,
 			expectedMessage: "AWS EC2 client is not available",
 		},
+		{
+			name: "When DescribeVpcEndpoints returns WebIdentityErr, it should set ValidAWSIdentityProvider to False",
+			kasCondition: &metav1.Condition{
+				Type:   string(hyperv1.KubeAPIServerAvailable),
+				Status: metav1.ConditionTrue,
+			},
+			setupEC2Mock: func(ctrl *gomock.Controller) awsapi.EC2API {
+				m := awsapi.NewMockEC2API(ctrl)
+				m.EXPECT().DescribeVpcEndpoints(gomock.Any(), gomock.Any()).
+					Return(nil, &smithy.GenericAPIError{
+						Code:    "WebIdentityErr",
+						Message: "some AWS request details",
+					})
+				return m
+			},
+			expectedStatus:  metav1.ConditionFalse,
+			expectedReason:  hyperv1.InvalidIdentityProvider,
+			expectedMessage: "WebIdentityErr",
+			expectError:     true,
+		},
+		{
+			name: "When DescribeVpcEndpoints returns a non-WebIdentityErr API error, it should set ValidAWSIdentityProvider to Unknown",
+			kasCondition: &metav1.Condition{
+				Type:   string(hyperv1.KubeAPIServerAvailable),
+				Status: metav1.ConditionTrue,
+			},
+			setupEC2Mock: func(ctrl *gomock.Controller) awsapi.EC2API {
+				m := awsapi.NewMockEC2API(ctrl)
+				m.EXPECT().DescribeVpcEndpoints(gomock.Any(), gomock.Any()).
+					Return(nil, &smithy.GenericAPIError{
+						Code:    "UnauthorizedAccess",
+						Message: "access denied",
+					})
+				return m
+			},
+			expectedStatus:  metav1.ConditionUnknown,
+			expectedReason:  hyperv1.AWSErrorReason,
+			expectedMessage: "UnauthorizedAccess",
+			expectError:     true,
+		},
+		{
+			name: "When DescribeVpcEndpoints returns a non-API error, it should set ValidAWSIdentityProvider to Unknown",
+			kasCondition: &metav1.Condition{
+				Type:   string(hyperv1.KubeAPIServerAvailable),
+				Status: metav1.ConditionTrue,
+			},
+			setupEC2Mock: func(ctrl *gomock.Controller) awsapi.EC2API {
+				m := awsapi.NewMockEC2API(ctrl)
+				m.EXPECT().DescribeVpcEndpoints(gomock.Any(), gomock.Any()).
+					Return(nil, fmt.Errorf("network timeout"))
+				return m
+			},
+			expectedStatus:  metav1.ConditionUnknown,
+			expectedReason:  hyperv1.StatusUnknownReason,
+			expectedMessage: "network timeout",
+			expectError:     true,
+		},
+		{
+			name: "When DescribeVpcEndpoints succeeds, it should set ValidAWSIdentityProvider to True",
+			kasCondition: &metav1.Condition{
+				Type:   string(hyperv1.KubeAPIServerAvailable),
+				Status: metav1.ConditionTrue,
+			},
+			setupEC2Mock: func(ctrl *gomock.Controller) awsapi.EC2API {
+				m := awsapi.NewMockEC2API(ctrl)
+				m.EXPECT().DescribeVpcEndpoints(gomock.Any(), gomock.Any()).
+					Return(&ec2.DescribeVpcEndpointsOutput{}, nil)
+				return m
+			},
+			expectedStatus:  metav1.ConditionTrue,
+			expectedReason:  hyperv1.AsExpectedReason,
+			expectedMessage: hyperv1.AllIsWellMessage,
+		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			// Create a test HCP
 			hcp := &hyperv1.HostedControlPlane{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:       "test-hcp",
@@ -78,37 +158,50 @@ func TestAWSHealthCheckIdentityProviderConditionLogic(t *testing.T) {
 				},
 			}
 
-			// Add KAS condition if specified
 			if tc.kasCondition != nil {
 				meta.SetStatusCondition(&hcp.Status.Conditions, *tc.kasCondition)
 			}
 
-			err := awsHealthCheckIdentityProvider(t.Context(), hcp)
-			if err != nil {
-				t.Errorf("Expected no error but got: %v", err)
+			kasAvailable := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.KubeAPIServerAvailable))
+			if kasAvailable != nil && kasAvailable.Status == metav1.ConditionTrue {
+				var ec2Client awsapi.EC2API
+				if tc.setupEC2Mock != nil {
+					mockCtrl := gomock.NewController(t)
+					ec2Client = tc.setupEC2Mock(mockCtrl)
+				}
+				err := validateAWSIdentityProvider(t.Context(), hcp, ec2Client)
+				if tc.expectError && err == nil {
+					t.Error("expected error but got nil")
+				}
+				if !tc.expectError && err != nil {
+					t.Errorf("expected no error but got: %v", err)
+				}
+			} else {
+				err := awsHealthCheckIdentityProvider(t.Context(), hcp)
+				if err != nil {
+					t.Errorf("expected no error but got: %v", err)
+				}
 			}
 
-			// Verify the ValidAWSIdentityProvider condition was set correctly
 			condition := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.ValidAWSIdentityProvider))
 			if condition == nil {
 				t.Fatal("ValidAWSIdentityProvider condition was not set")
-				return
 			}
 
 			if condition.Status != tc.expectedStatus {
-				t.Errorf("Expected status %v, got %v", tc.expectedStatus, condition.Status)
+				t.Errorf("expected status %v, got %v", tc.expectedStatus, condition.Status)
 			}
 
 			if condition.Reason != tc.expectedReason {
-				t.Errorf("Expected reason %v, got %v", tc.expectedReason, condition.Reason)
+				t.Errorf("expected reason %v, got %v", tc.expectedReason, condition.Reason)
 			}
 
 			if tc.expectedMessage != "" && condition.Message != tc.expectedMessage {
-				t.Errorf("Expected message %q, got %q", tc.expectedMessage, condition.Message)
+				t.Errorf("expected message %q, got %q", tc.expectedMessage, condition.Message)
 			}
 
 			if condition.ObservedGeneration != hcp.Generation {
-				t.Errorf("Expected ObservedGeneration %v, got %v", hcp.Generation, condition.ObservedGeneration)
+				t.Errorf("expected ObservedGeneration %v, got %v", hcp.Generation, condition.ObservedGeneration)
 			}
 		})
 	}

hypershift-operator/controllers/hostedcluster/aws_endpoint_services_test.go

@@ -0,0 +1,154 @@
+package hostedcluster
+
+import (
+	"testing"
+	"time"
+
+	. "github.com/onsi/gomega"
+
+	hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
+	"github.com/openshift/hypershift/support/api"
+	"github.com/openshift/hypershift/support/testutil"
+
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	crclient "sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+func TestDeleteAWSEndpointServices(t *testing.T) {
+	cpoFinalizer := "hypershift.openshift.io/control-plane-operator-finalizer"
+	namespace := "clusters-test"
+
+	tests := []struct {
+		name                    string
+		hc                      *hyperv1.HostedCluster
+		endpoints               []hyperv1.AWSEndpointService
+		expectPending           bool
+		expectFinalizerRemoved  bool
+		expectDeletionTimestamp bool
+	}{
+		{
+			name: "When endpoint is deleting with invalid creds, it should remove CPO finalizer",
+			hc:   testutil.NewHostedClusterWithCredentialConditions(metav1.ConditionFalse, metav1.ConditionTrue),
+			endpoints: []hyperv1.AWSEndpointService{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:              "ep-1",
+						Namespace:         namespace,
+						DeletionTimestamp: &metav1.Time{Time: time.Now().Add(-1 * time.Minute)},
+						Finalizers:        []string{cpoFinalizer},
+					},
+				},
+			},
+			expectPending:          true,
+			expectFinalizerRemoved: true,
+		},
+		{
+			name: "When endpoint is deleting with valid creds past grace period, it should remove CPO finalizer",
+			hc:   testutil.NewHostedClusterWithCredentialConditions(metav1.ConditionTrue, metav1.ConditionTrue),
+			endpoints: []hyperv1.AWSEndpointService{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:              "ep-1",
+						Namespace:         namespace,
+						DeletionTimestamp: &metav1.Time{Time: time.Now().Add(-15 * time.Minute)},
+						Finalizers:        []string{cpoFinalizer},
+					},
+				},
+			},
+			expectPending:          true,
+			expectFinalizerRemoved: true,
+		},
+		{
+			name: "When endpoint is deleting with valid creds within grace period, it should keep CPO finalizer",
+			hc:   testutil.NewHostedClusterWithCredentialConditions(metav1.ConditionTrue, metav1.ConditionTrue),
+			endpoints: []hyperv1.AWSEndpointService{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:              "ep-1",
+						Namespace:         namespace,
+						DeletionTimestamp: &metav1.Time{Time: time.Now().Add(-1 * time.Minute)},
+						Finalizers:        []string{cpoFinalizer},
+					},
+				},
+			},
+			expectPending:          true,
+			expectFinalizerRemoved: false,
+		},
+		{
+			name:                   "When no endpoints exist, it should report no pending endpoints",
+			hc:                     testutil.NewHostedClusterWithCredentialConditions(metav1.ConditionTrue, metav1.ConditionTrue),
+			endpoints:              nil,
+			expectPending:          false,
+			expectFinalizerRemoved: false,
+		},
+		{
+			name: "When an endpoint is not deleting, it should delete the endpoint",
+			hc:   testutil.NewHostedClusterWithCredentialConditions(metav1.ConditionTrue, metav1.ConditionTrue),
+			endpoints: []hyperv1.AWSEndpointService{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:       "ep-1",
+						Namespace:  namespace,
+						Finalizers: []string{cpoFinalizer},
+					},
+				},
+			},
+			expectPending:           true,
+			expectFinalizerRemoved:  false,
+			expectDeletionTimestamp: true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+
+			objects := []crclient.Object{}
+			for i := range tc.endpoints {
+				objects = append(objects, &tc.endpoints[i])
+			}
+
+			fakeClient := fake.NewClientBuilder().
+				WithScheme(api.Scheme).
+				WithObjects(objects...).
+				Build()
+
+			pending, err := deleteAWSEndpointServices(t.Context(), fakeClient, tc.hc, namespace)
+
+			g.Expect(err).ToNot(HaveOccurred())
+
+			g.Expect(pending).To(Equal(tc.expectPending))
+
+			if tc.expectFinalizerRemoved {
+				for _, ep := range tc.endpoints {
+					updatedEP := &hyperv1.AWSEndpointService{}
+					err := fakeClient.Get(t.Context(), crclient.ObjectKey{
+						Namespace: ep.Namespace,
+						Name:      ep.Name,
+					}, updatedEP)
+					if apierrors.IsNotFound(err) {
+						continue
+					}
+					g.Expect(err).ToNot(HaveOccurred())
+					g.Expect(updatedEP.Finalizers).ToNot(ContainElement(cpoFinalizer))
+				}
+			} else {
+				for _, ep := range tc.endpoints {
+					updatedEP := &hyperv1.AWSEndpointService{}
+					err := fakeClient.Get(t.Context(), crclient.ObjectKey{
+						Namespace: ep.Namespace,
+						Name:      ep.Name,
+					}, updatedEP)
+					g.Expect(err).ToNot(HaveOccurred())
+					g.Expect(updatedEP.Finalizers).To(ContainElement(cpoFinalizer), "expected finalizer to be retained")
+					if tc.expectDeletionTimestamp {
+						g.Expect(updatedEP.DeletionTimestamp.IsZero()).To(BeFalse(), "expected endpoint to be marked for deletion")
+					}
+				}
+			}
+		})
+	}
+}