diff --git a/control-plane-operator/controllers/healthcheck/aws.go b/control-plane-operator/controllers/healthcheck/aws.go index a99463c0a392..552f206c58c7 100644 --- a/control-plane-operator/controllers/healthcheck/aws.go +++ b/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{ diff --git a/control-plane-operator/controllers/healthcheck/aws_test.go b/control-plane-operator/controllers/healthcheck/aws_test.go index e8faf2ab4ea7..b7254efb04df 100644 --- a/control-plane-operator/controllers/healthcheck/aws_test.go +++ b/control-plane-operator/controllers/healthcheck/aws_test.go @@ -1,10 +1,15 @@ 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" @@ -12,28 +17,31 @@ import ( 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) } }) } diff --git a/hypershift-operator/controllers/hostedcluster/aws_endpoint_services_test.go b/hypershift-operator/controllers/hostedcluster/aws_endpoint_services_test.go new file mode 100644 index 000000000000..a0bedca65de0 --- /dev/null +++ b/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") + } + } + } + }) + } +} diff --git a/hypershift-operator/controllers/hostedcluster/aws_oidc_test.go b/hypershift-operator/controllers/hostedcluster/aws_oidc_test.go new file mode 100644 index 000000000000..aa6f8a55870d --- /dev/null +++ b/hypershift-operator/controllers/hostedcluster/aws_oidc_test.go @@ -0,0 +1,166 @@ +package hostedcluster + +import ( + "fmt" + "testing" + + . "github.com/onsi/gomega" + + 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/aws" + "github.com/aws/aws-sdk-go-v2/service/s3" + s3types "github.com/aws/aws-sdk-go-v2/service/s3/types" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + ctrl "sigs.k8s.io/controller-runtime" + crclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "go.uber.org/mock/gomock" +) + +func TestCleanupAWSOIDCBucketData(t *testing.T) { + tests := []struct { + name string + hcluster *hyperv1.HostedCluster + setupS3Mock func(*gomock.Controller) awsapi.S3API + bucketName string + expectErr bool + expectErrMsg string + expectFinalizer bool + }{ + { + name: "When cleanup succeeds, it should delete objects and remove finalizer", + hcluster: &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "clusters", + Finalizers: []string{oidcDocumentsFinalizer}, + }, + Spec: hyperv1.HostedClusterSpec{InfraID: "test-infra"}, + }, + setupS3Mock: func(ctrl *gomock.Controller) awsapi.S3API { + m := awsapi.NewMockS3API(ctrl) + m.EXPECT().DeleteObjects(gomock.Any(), gomock.Any()). + Return(&s3.DeleteObjectsOutput{}, nil) + return m + }, + bucketName: "my-bucket", + expectFinalizer: false, + }, + { + name: "When DeleteObjects fails with a non-NoSuchBucket error, it should return error and keep finalizer", + hcluster: &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "clusters", + Finalizers: []string{oidcDocumentsFinalizer}, + }, + Spec: hyperv1.HostedClusterSpec{InfraID: "test-infra"}, + }, + setupS3Mock: func(ctrl *gomock.Controller) awsapi.S3API { + m := awsapi.NewMockS3API(ctrl) + m.EXPECT().DeleteObjects(gomock.Any(), gomock.Any()). + Return(nil, fmt.Errorf("access denied")) + return m + }, + bucketName: "my-bucket", + expectErr: true, + expectErrMsg: "failed to delete OIDC objects", + expectFinalizer: true, + }, + { + name: "When DeleteObjects fails with NoSuchBucket, it should succeed and remove finalizer", + hcluster: &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "clusters", + Finalizers: []string{oidcDocumentsFinalizer}, + }, + Spec: hyperv1.HostedClusterSpec{InfraID: "test-infra"}, + }, + setupS3Mock: func(ctrl *gomock.Controller) awsapi.S3API { + m := awsapi.NewMockS3API(ctrl) + m.EXPECT().DeleteObjects(gomock.Any(), gomock.Any()). + Return(nil, &s3types.NoSuchBucket{Message: aws.String("bucket not found")}) + return m + }, + bucketName: "my-bucket", + expectFinalizer: false, + }, + { + name: "When DeleteObjects returns partial failure, it should return error and keep finalizer", + hcluster: &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "clusters", + Finalizers: []string{oidcDocumentsFinalizer}, + }, + Spec: hyperv1.HostedClusterSpec{InfraID: "test-infra"}, + }, + setupS3Mock: func(ctrl *gomock.Controller) awsapi.S3API { + m := awsapi.NewMockS3API(ctrl) + m.EXPECT().DeleteObjects(gomock.Any(), gomock.Any()). + Return(&s3.DeleteObjectsOutput{ + Errors: []s3types.Error{ + { + Key: aws.String("test-infra/.well-known/openid-configuration"), + Code: aws.String("AccessDenied"), + Message: aws.String("Access Denied"), + }, + }, + }, nil) + return m + }, + bucketName: "my-bucket", + expectErr: true, + expectErrMsg: "partial failure deleting OIDC objects", + expectFinalizer: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + fakeClient := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects(tc.hcluster). + Build() + + mockCtrl := gomock.NewController(t) + var s3Client awsapi.S3API + if tc.setupS3Mock != nil { + s3Client = tc.setupS3Mock(mockCtrl) + } + + r := &HostedClusterReconciler{ + Client: fakeClient, + S3Client: s3Client, + OIDCStorageProviderS3BucketName: tc.bucketName, + } + + err := r.cleanupOIDCBucketData(t.Context(), ctrl.Log, tc.hcluster) + + if tc.expectErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tc.expectErrMsg)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + + updatedHC := &hyperv1.HostedCluster{} + g.Expect(fakeClient.Get(t.Context(), crclient.ObjectKeyFromObject(tc.hcluster), updatedHC)).To(Succeed()) + + if tc.expectFinalizer { + g.Expect(updatedHC.Finalizers).To(ContainElement(oidcDocumentsFinalizer)) + } else { + g.Expect(updatedHC.Finalizers).ToNot(ContainElement(oidcDocumentsFinalizer)) + } + }) + } +} diff --git a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go index 4afdbad11cf3..8f12704374d1 100644 --- a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go +++ b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go @@ -3622,7 +3622,8 @@ func deleteAWSEndpointServices(ctx context.Context, c client.Client, hc *hyperv1 } for _, ep := range awsEndpointServiceList.Items { if ep.DeletionTimestamp != nil { - if platformaws.GetCredentialStatus(hc) == platformaws.CredentialStatusValid && time.Since(ep.DeletionTimestamp.Time) < awsEndpointDeletionGracePeriod { + credStatus := platformaws.GetCredentialStatus(hc) + if credStatus == platformaws.CredentialStatusValid && time.Since(ep.DeletionTimestamp.Time) < awsEndpointDeletionGracePeriod { continue } @@ -3634,7 +3635,12 @@ func deleteAWSEndpointServices(ctx context.Context, c client.Client, hc *hyperv1 return false, fmt.Errorf("failed to remove finalizer from awsendpointservice: %w", err) } } - log.Info("Removed CPO finalizer for awsendpointservice because the HC has no valid aws credentials", "name", ep.Name, "endpoint-id", ep.Status.EndpointID) + reason := "the HC has no valid aws credentials" + if credStatus == platformaws.CredentialStatusValid { + reason = "deletion grace period expired" + } + + log.Info("Removed CPO finalizer for awsendpointservice", "reason", reason, "name", ep.Name, "endpoint-id", ep.Status.EndpointID) continue } @@ -5028,14 +5034,19 @@ func (r *HostedClusterReconciler) cleanupOIDCBucketData(ctx context.Context, log }) } - if _, err := r.S3Client.DeleteObjects(ctx, &s3.DeleteObjectsInput{ + output, err := r.S3Client.DeleteObjects(ctx, &s3.DeleteObjectsInput{ Bucket: aws.String(r.OIDCStorageProviderS3BucketName), Delete: &s3types.Delete{Objects: objectsToDelete}, - }); err != nil { + }) + if err != nil { var nsbErr *s3types.NoSuchBucket if !errors.As(err, &nsbErr) { return fmt.Errorf("failed to delete OIDC objects from %s S3 bucket: %w", r.OIDCStorageProviderS3BucketName, err) } + } else if output != nil && len(output.Errors) > 0 { + return fmt.Errorf("partial failure deleting OIDC objects from %s S3 bucket: %d of %d objects failed, first error: %s", + r.OIDCStorageProviderS3BucketName, len(output.Errors), len(objectsToDelete), + aws.ToString(output.Errors[0].Message)) } controllerutil.RemoveFinalizer(hcluster, oidcDocumentsFinalizer) diff --git a/hypershift-operator/controllers/hostedcluster/internal/platform/aws/aws_test.go b/hypershift-operator/controllers/hostedcluster/internal/platform/aws/aws_test.go index 23c4b9dd5049..3984a644e0ff 100644 --- a/hypershift-operator/controllers/hostedcluster/internal/platform/aws/aws_test.go +++ b/hypershift-operator/controllers/hostedcluster/internal/platform/aws/aws_test.go @@ -3,16 +3,23 @@ package aws import ( "testing" + . "github.com/onsi/gomega" + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "github.com/openshift/hypershift/support/api" + "github.com/openshift/hypershift/support/testutil" configv1 "github.com/openshift/api/config/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" capiaws "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" + crclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" "github.com/blang/semver" "github.com/google/go-cmp/cmp" @@ -109,61 +116,61 @@ func TestGetCredentialStatus(t *testing.T) { expectedResult CredentialStatus }{ { - name: "When both conditions are True, return Valid", - ValidOIDCConfigurationConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionTrue}[0], - ValidAWSIdentityProviderConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionTrue}[0], + name: "When both conditions are True, it should return Valid", + ValidOIDCConfigurationConditionStatus: ptr.To(metav1.ConditionTrue), + ValidAWSIdentityProviderConditionStatus: ptr.To(metav1.ConditionTrue), expectedResult: CredentialStatusValid, }, { - name: "When OIDC is False, return Invalid", - ValidOIDCConfigurationConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionFalse}[0], - ValidAWSIdentityProviderConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionTrue}[0], + name: "When OIDC is False, it should return Invalid", + ValidOIDCConfigurationConditionStatus: ptr.To(metav1.ConditionFalse), + ValidAWSIdentityProviderConditionStatus: ptr.To(metav1.ConditionTrue), expectedResult: CredentialStatusInvalid, }, { - name: "When AWS Identity Provider is False, return Invalid", - ValidOIDCConfigurationConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionTrue}[0], - ValidAWSIdentityProviderConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionFalse}[0], + name: "When AWS Identity Provider is False, it should return Invalid", + ValidOIDCConfigurationConditionStatus: ptr.To(metav1.ConditionTrue), + ValidAWSIdentityProviderConditionStatus: ptr.To(metav1.ConditionFalse), expectedResult: CredentialStatusInvalid, }, { - name: "When both are False, return Invalid", - ValidOIDCConfigurationConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionFalse}[0], - ValidAWSIdentityProviderConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionFalse}[0], + name: "When both are False, it should return Invalid", + ValidOIDCConfigurationConditionStatus: ptr.To(metav1.ConditionFalse), + ValidAWSIdentityProviderConditionStatus: ptr.To(metav1.ConditionFalse), expectedResult: CredentialStatusInvalid, }, { - name: "When OIDC is Unknown, return Unknown", - ValidOIDCConfigurationConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionUnknown}[0], - ValidAWSIdentityProviderConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionTrue}[0], + name: "When OIDC is Unknown, it should return Unknown", + ValidOIDCConfigurationConditionStatus: ptr.To(metav1.ConditionUnknown), + ValidAWSIdentityProviderConditionStatus: ptr.To(metav1.ConditionTrue), expectedResult: CredentialStatusUnknown, }, { - name: "When AWS Identity Provider is Unknown, return Unknown", - ValidOIDCConfigurationConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionTrue}[0], - ValidAWSIdentityProviderConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionUnknown}[0], + name: "When AWS Identity Provider is Unknown, it should return Unknown", + ValidOIDCConfigurationConditionStatus: ptr.To(metav1.ConditionTrue), + ValidAWSIdentityProviderConditionStatus: ptr.To(metav1.ConditionUnknown), expectedResult: CredentialStatusUnknown, }, { - name: "When both are Unknown, return Unknown", - ValidOIDCConfigurationConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionUnknown}[0], - ValidAWSIdentityProviderConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionUnknown}[0], + name: "When both are Unknown, it should return Unknown", + ValidOIDCConfigurationConditionStatus: ptr.To(metav1.ConditionUnknown), + ValidAWSIdentityProviderConditionStatus: ptr.To(metav1.ConditionUnknown), expectedResult: CredentialStatusUnknown, }, { - name: "When OIDC condition is missing, return Unknown", + name: "When OIDC condition is missing, it should return Unknown", ValidOIDCConfigurationConditionStatus: nil, - ValidAWSIdentityProviderConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionTrue}[0], + ValidAWSIdentityProviderConditionStatus: ptr.To(metav1.ConditionTrue), expectedResult: CredentialStatusUnknown, }, { - name: "When AWS Identity Provider condition is missing, return Unknown", - ValidOIDCConfigurationConditionStatus: &[]metav1.ConditionStatus{metav1.ConditionTrue}[0], + name: "When AWS Identity Provider condition is missing, it should return Unknown", + ValidOIDCConfigurationConditionStatus: ptr.To(metav1.ConditionTrue), ValidAWSIdentityProviderConditionStatus: nil, expectedResult: CredentialStatusUnknown, }, { - name: "When both conditions are missing, return Unknown", + name: "When both conditions are missing, it should return Unknown", ValidOIDCConfigurationConditionStatus: nil, ValidAWSIdentityProviderConditionStatus: nil, expectedResult: CredentialStatusUnknown, @@ -384,3 +391,114 @@ func TestCAPIProviderDeploymentSpec(t *testing.T) { }) } } + +func TestDeleteOrphanedMachines(t *testing.T) { + namespace := "clusters-test" + deletionTime := metav1.Now() + + tests := []struct { + name string + hc *hyperv1.HostedCluster + machines []capiaws.AWSMachine + expectFinalizersCleared bool + }{ + { + name: "When credentials are valid, it should skip cleanup", + hc: testutil.NewHostedClusterWithCredentialConditions(metav1.ConditionTrue, metav1.ConditionTrue), + machines: []capiaws.AWSMachine{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-1", + Namespace: namespace, + DeletionTimestamp: &deletionTime, + Finalizers: []string{"test-finalizer"}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-2", + Namespace: namespace, + Finalizers: []string{"test-finalizer"}, + }, + }, + }, + expectFinalizersCleared: false, + }, + { + name: "When credentials are invalid and machines have deletion timestamps, it should clear finalizers", + hc: testutil.NewHostedClusterWithCredentialConditions(metav1.ConditionFalse, metav1.ConditionTrue), + machines: []capiaws.AWSMachine{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-1", + Namespace: namespace, + DeletionTimestamp: &deletionTime, + Finalizers: []string{"test-finalizer"}, + }, + }, + }, + expectFinalizersCleared: true, + }, + { + name: "When credentials are invalid and machines have no deletion timestamps, it should not modify them", + hc: testutil.NewHostedClusterWithCredentialConditions(metav1.ConditionFalse, metav1.ConditionTrue), + machines: []capiaws.AWSMachine{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-1", + Namespace: namespace, + Finalizers: []string{"test-finalizer"}, + }, + }, + }, + expectFinalizersCleared: false, + }, + { + name: "When no AWSMachines exist, it should return nil", + hc: testutil.NewHostedClusterWithCredentialConditions(metav1.ConditionFalse, metav1.ConditionTrue), + machines: []capiaws.AWSMachine{}, + expectFinalizersCleared: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + objects := []crclient.Object{} + for i := range tc.machines { + objects = append(objects, &tc.machines[i]) + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects(objects...). + Build() + + a := AWS{} + err := a.DeleteOrphanedMachines(t.Context(), fakeClient, tc.hc, namespace) + + g.Expect(err).ToNot(HaveOccurred()) + if tc.expectFinalizersCleared { + for _, machine := range tc.machines { + m := &capiaws.AWSMachine{} + err := fakeClient.Get(t.Context(), crclient.ObjectKey{Name: machine.Name, Namespace: namespace}, m) + if apierrors.IsNotFound(err) { + // Object was deleted (expected after finalizers cleared) + continue + } + g.Expect(err).ToNot(HaveOccurred(), "failed to get machine %s", machine.Name) + g.Expect(m.Finalizers).To(BeEmpty(), "expected finalizers to be cleared on machine %s", machine.Name) + } + } + + if !tc.expectFinalizersCleared && len(tc.machines) > 0 { + for _, machine := range tc.machines { + m := &capiaws.AWSMachine{} + g.Expect(fakeClient.Get(t.Context(), crclient.ObjectKey{Name: machine.Name, Namespace: namespace}, m)).To(Succeed(), "failed to get machine %s", machine.Name) + g.Expect(m.Finalizers).To(ContainElement("test-finalizer"), "expected finalizers to be preserved on machine %s", machine.Name) + } + } + }) + } +} diff --git a/support/testutil/hostedcluster.go b/support/testutil/hostedcluster.go new file mode 100644 index 000000000000..7b6ad9977947 --- /dev/null +++ b/support/testutil/hostedcluster.go @@ -0,0 +1,27 @@ +package testutil + +import ( + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// NewHostedClusterWithCredentialConditions creates a HostedCluster with the given +// OIDC and AWS identity provider condition statuses set. +func NewHostedClusterWithCredentialConditions(oidcStatus, awsIDPStatus metav1.ConditionStatus) *hyperv1.HostedCluster { + hc := &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "clusters"}, + } + meta.SetStatusCondition(&hc.Status.Conditions, metav1.Condition{ + Type: string(hyperv1.ValidOIDCConfiguration), + Status: oidcStatus, + Reason: "test", + }) + meta.SetStatusCondition(&hc.Status.Conditions, metav1.Condition{ + Type: string(hyperv1.ValidAWSIdentityProvider), + Status: awsIDPStatus, + Reason: "test", + }) + return hc +} diff --git a/support/testutil/hostedcluster_test.go b/support/testutil/hostedcluster_test.go new file mode 100644 index 000000000000..eac69c5331f8 --- /dev/null +++ b/support/testutil/hostedcluster_test.go @@ -0,0 +1,50 @@ +package testutil + +import ( + "testing" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestNewHostedClusterWithCredentialConditions(t *testing.T) { + hc := NewHostedClusterWithCredentialConditions(metav1.ConditionFalse, metav1.ConditionTrue) + + if hc.Name != "test" || hc.Namespace != "clusters" { + t.Errorf("expected HostedCluster to have name %q and namespace %q, got %q/%q", "test", "clusters", hc.Name, hc.Namespace) + } + + tests := []struct { + name string + conditionType string + expected metav1.ConditionStatus + }{ + { + name: "When OIDC status is false, it should set the OIDC condition to false", + conditionType: string(hyperv1.ValidOIDCConfiguration), + expected: metav1.ConditionFalse, + }, + { + name: "When AWS identity provider status is true, it should set the AWS identity provider condition to true", + conditionType: string(hyperv1.ValidAWSIdentityProvider), + expected: metav1.ConditionTrue, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + condition := meta.FindStatusCondition(hc.Status.Conditions, tc.conditionType) + if condition == nil { + t.Fatalf("expected condition %q to be present", tc.conditionType) + } + if condition.Status != tc.expected { + t.Errorf("expected condition %q to have status %q, got %q", tc.conditionType, tc.expected, condition.Status) + } + if condition.Reason != "test" { + t.Errorf("expected condition %q to have reason %q, got %q", tc.conditionType, "test", condition.Reason) + } + }) + } +}