From 206c4b63f363b1ff3cc745c70c944e38a5253d9e Mon Sep 17 00:00:00 2001 From: Nico Schieder Date: Tue, 1 Sep 2026 15:10:54 +0200 Subject: [PATCH] feat(gcp): GCP-503 implement OrphanDeleter with credential health check Removes CAPG finalizers from terminating GCPMachines when WIF credentials become invalid, preventing cluster teardown from getting stuck. - Adds a GCP identity-provider health check in the CPO that validates WIF configuration and credentials by making a Compute API Regions.Get call. Sets ValidGCPWorkloadIdentity and ValidGCPCredentials on the HostedControlPlane; these bubble up to the HostedCluster via Phase 1 of the HC controller so DeleteOrphanedMachines always sees a fresh signal. - Implements GCP DeleteOrphanedMachines: when credentials are explicitly Invalid (CredentialStatusInvalid), removes the CAPG finalizer from each terminating GCPMachine. The guard is != Invalid (not == Valid) so Unknown state never triggers cleanup. - Latches Invalid during teardown: ComputeGCPCredentialConditions skips overwriting an existing False condition when the HCP reports Unknown (i.e. KAS is gone). This prevents a KAS-unavailable signal mid-teardown from silently stopping orphan machine cleanup. - Differentiates the two conditions by error type: - oauth2.RetrieveError (400/401/403): WIF token exchange failed -> ValidGCPWorkloadIdentity=False, ValidGCPCredentials=Unknown - googleapi 401: WIF succeeded, Compute API rejected the credential -> ValidGCPWorkloadIdentity=True, ValidGCPCredentials=False - Missing GCP spec: both Unknown (config defect, not a credential failure) - Transient / other errors -> both Unknown - Adds compile-time OrphanDeleter assertions for GCP and Azure in platform.go alongside the existing AWS entry. - Removes ReconcileCredentials writing ValidGCP* conditions directly; those are now owned exclusively by the CPO health check, mirroring the AWS pattern. - gcpRegionChecker is a package-level var to allow test injection without real GCP credentials. - ComputeGCPCredentialConditions returns bool (changed flag); the []metav1.Condition return was dropped as it was never used in production. - isPermanentGCPCredentialError removed (no production callers); replaced by TestIsWIFTokenError and TestIsComputeAuthError. - Compute-client init errors are logged at V(4) and wrapped into the errComputeClientUnavailable sentinel so the root cause is preserved. Signed-off-by: Nico Schieder Commit-Message-Assisted-by: Claude (via Claude Code) --- .../controllers/healthcheck/gcp.go | 152 +++++++ .../controllers/healthcheck/gcp_test.go | 396 ++++++++++++++++++ .../healthcheck/healthcheck_controller.go | 5 + .../hostedcluster/hostedcluster_controller.go | 12 + .../internal/platform/gcp/gcp.go | 155 +++---- .../platform/gcp/gcp_conditions_test.go | 254 +++++------ .../internal/platform/gcp/gcp_test.go | 222 +++++++++- .../internal/platform/platform.go | 4 +- 8 files changed, 1007 insertions(+), 193 deletions(-) create mode 100644 control-plane-operator/controllers/healthcheck/gcp.go create mode 100644 control-plane-operator/controllers/healthcheck/gcp_test.go diff --git a/control-plane-operator/controllers/healthcheck/gcp.go b/control-plane-operator/controllers/healthcheck/gcp.go new file mode 100644 index 000000000000..4fda2d26d686 --- /dev/null +++ b/control-plane-operator/controllers/healthcheck/gcp.go @@ -0,0 +1,152 @@ +package healthcheck + +import ( + "context" + "errors" + "fmt" + "os" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + ctrl "sigs.k8s.io/controller-runtime" + + "golang.org/x/oauth2" + "golang.org/x/oauth2/google" + "google.golang.org/api/compute/v1" + "google.golang.org/api/googleapi" + "google.golang.org/api/option" +) + +// gcpRegionChecker is the function used to check GCP region access. +// It is a package-level variable to allow injection in tests. +var gcpRegionChecker = func(ctx context.Context, project, region string) error { + computeService, err := initGCPComputeClient(ctx) + if err != nil { + wrapped := fmt.Errorf("%w: %w", errComputeClientUnavailable, err) + ctrl.LoggerFrom(ctx).V(4).Info("GCP compute client not available, skipping credential check", "reason", err.Error()) + return wrapped + } + _, err = computeService.Regions.Get(project, region).Context(ctx).Do() + return err +} + +// errComputeClientUnavailable is returned by gcpRegionChecker when the GCP +// compute client cannot be initialized (e.g. missing credentials file). +var errComputeClientUnavailable = fmt.Errorf("GCP compute client unavailable") + +func gcpHealthCheckIdentityProvider(ctx context.Context, hcp *hyperv1.HostedControlPlane) error { + kasAvailable := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.KubeAPIServerAvailable)) + if kasAvailable == nil || kasAvailable.Status != metav1.ConditionTrue { + setGCPConditions(hcp, metav1.ConditionUnknown, hyperv1.StatusUnknownReason, + "Cannot validate GCP credentials while KubeAPIServer is not available") + return nil + } + + if hcp.Spec.Platform.GCP == nil { + // Use Unknown rather than False: a missing GCP spec is a configuration + // defect, not a credential failure. Setting False would cause + // GetCredentialStatus to return CredentialStatusInvalid and trigger + // DeleteOrphanedMachines to strip GCPMachine finalizers, potentially + // leaking cloud resources. The latch in ComputeGCPCredentialConditions + // would then persist that False through teardown. + setGCPConditions(hcp, metav1.ConditionUnknown, "MissingGCPConfiguration", + "GCP platform configuration is missing from HostedControlPlane spec") + return fmt.Errorf("GCP platform configuration is missing from HostedControlPlane spec") + } + + project := hcp.Spec.Platform.GCP.Project + region := hcp.Spec.Platform.GCP.Region + if err := gcpRegionChecker(ctx, project, region); err != nil { + if errors.Is(err, errComputeClientUnavailable) { + setGCPConditions(hcp, metav1.ConditionUnknown, hyperv1.StatusUnknownReason, + "GCP compute client is not available") + return nil //nolint:nilerr // missing compute client is not a reconciler error; conditions are set to Unknown + } + if isWIFTokenError(err) { + // WIF token exchange failed: the workload identity configuration is + // invalid. Credentials cannot be validated because the token cannot + // be obtained; mark ValidGCPCredentials as Unknown. + setGCPCondition(hcp, hyperv1.ValidGCPWorkloadIdentity, metav1.ConditionFalse, + hyperv1.InvalidIdentityProvider, "GCP Workload Identity Federation token exchange failed") + setGCPCondition(hcp, hyperv1.ValidGCPCredentials, metav1.ConditionUnknown, + hyperv1.StatusUnknownReason, "Cannot validate GCP credentials: WIF token exchange failed") + return fmt.Errorf("error health checking GCP identity provider: %w", err) + } + if isComputeAuthError(err) { + // The WIF token was obtained successfully (WorkloadIdentity is valid) + // but the Compute API rejected the resulting credential with HTTP 401. + // Only ValidGCPCredentials is False; ValidGCPWorkloadIdentity stays True + // because the token exchange itself succeeded. + setGCPCondition(hcp, hyperv1.ValidGCPWorkloadIdentity, metav1.ConditionTrue, + hyperv1.AsExpectedReason, "GCP Workload Identity Federation token exchange succeeded") + setGCPCondition(hcp, hyperv1.ValidGCPCredentials, metav1.ConditionFalse, + hyperv1.InvalidIdentityProvider, "GCP credential validation failed: Compute API rejected the credential") + return fmt.Errorf("error health checking GCP identity provider: %w", err) + } + + setGCPConditions(hcp, metav1.ConditionUnknown, hyperv1.StatusUnknownReason, + "GCP API error during credential validation") + return fmt.Errorf("error health checking GCP identity provider: %w", err) + } + + setGCPConditions(hcp, metav1.ConditionTrue, hyperv1.AsExpectedReason, hyperv1.AllIsWellMessage) + return nil +} + +func setGCPCondition(hcp *hyperv1.HostedControlPlane, condType hyperv1.ConditionType, status metav1.ConditionStatus, reason, message string) { + meta.SetStatusCondition(&hcp.Status.Conditions, metav1.Condition{ + Type: string(condType), + ObservedGeneration: hcp.Generation, + Status: status, + Reason: reason, + Message: message, + }) +} + +func setGCPConditions(hcp *hyperv1.HostedControlPlane, status metav1.ConditionStatus, reason, message string) { + setGCPCondition(hcp, hyperv1.ValidGCPWorkloadIdentity, status, reason, message) + setGCPCondition(hcp, hyperv1.ValidGCPCredentials, status, reason, message) +} + +// isWIFTokenError returns true if the error indicates a non-transient WIF +// token-exchange failure (oauth2.RetrieveError with HTTP 400/401/403). +// These errors mean the workload identity pool/provider/SA is misconfigured; +// no Compute API call was made yet. +func isWIFTokenError(err error) bool { + var retrieveErr *oauth2.RetrieveError + if errors.As(err, &retrieveErr) && retrieveErr.Response != nil { + code := retrieveErr.Response.StatusCode + return code == 400 || code == 401 || code == 403 + } + return false +} + +// isComputeAuthError returns true if the Compute API itself rejected the +// request with HTTP 401 (authentication rejected). This indicates the WIF +// token was obtained but not accepted by the Compute API. +// +// Compute API 403 is NOT treated as an auth error because it means +// authentication succeeded but authorization failed (missing IAM permission, +// quota, etc.). +func isComputeAuthError(err error) bool { + var apiErr *googleapi.Error + return errors.As(err, &apiErr) && apiErr.Code == 401 +} + +func initGCPComputeClient(ctx context.Context) (*compute.Service, error) { + credentialsFile := os.Getenv("GOOGLE_APPLICATION_CREDENTIALS") + if credentialsFile == "" { + return nil, fmt.Errorf("GOOGLE_APPLICATION_CREDENTIALS not set") + } + if _, err := os.Stat(credentialsFile); err != nil { + return nil, fmt.Errorf("credentials file not accessible at %s: %w", credentialsFile, err) + } + httpClient, err := google.DefaultClient(ctx, compute.CloudPlatformScope) + if err != nil { + return nil, fmt.Errorf("failed to create Google Cloud client: %w", err) + } + return compute.NewService(ctx, option.WithHTTPClient(httpClient)) +} diff --git a/control-plane-operator/controllers/healthcheck/gcp_test.go b/control-plane-operator/controllers/healthcheck/gcp_test.go new file mode 100644 index 000000000000..9bc84044db4d --- /dev/null +++ b/control-plane-operator/controllers/healthcheck/gcp_test.go @@ -0,0 +1,396 @@ +package healthcheck + +import ( + "context" + "fmt" + "net/http" + "testing" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "github.com/openshift/hypershift/support/api" + + "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" + + "golang.org/x/oauth2" + "google.golang.org/api/googleapi" +) + +func TestGCPHealthCheckIdentityProviderConditionLogic(t *testing.T) { + testCases := []struct { + name string + kasCondition *metav1.Condition + gcpSpec *hyperv1.GCPPlatformSpec + expectError bool + expectedStatus metav1.ConditionStatus + expectedReason string + expectedMessage string + }{ + { + name: "When KAS condition is missing, it should set credentials condition to Unknown", + kasCondition: nil, + expectError: false, + expectedStatus: metav1.ConditionUnknown, + expectedReason: hyperv1.StatusUnknownReason, + expectedMessage: "Cannot validate GCP credentials while KubeAPIServer is not available", + }, + { + name: "When KAS condition is False, it should set credentials condition to Unknown", + kasCondition: &metav1.Condition{ + Type: string(hyperv1.KubeAPIServerAvailable), + Status: metav1.ConditionFalse, + }, + expectError: false, + expectedStatus: metav1.ConditionUnknown, + expectedReason: hyperv1.StatusUnknownReason, + expectedMessage: "Cannot validate GCP credentials while KubeAPIServer is not available", + }, + { + name: "When KAS is available but GCP spec is missing, it should set credentials condition to Unknown", + kasCondition: &metav1.Condition{ + Type: string(hyperv1.KubeAPIServerAvailable), + Status: metav1.ConditionTrue, + }, + gcpSpec: nil, + expectError: true, + expectedStatus: metav1.ConditionUnknown, + expectedReason: "MissingGCPConfiguration", + expectedMessage: "GCP platform configuration is missing from HostedControlPlane spec", + }, + { + name: "When KAS is available with GCP spec but no credentials, it should set credentials condition to Unknown", + kasCondition: &metav1.Condition{ + Type: string(hyperv1.KubeAPIServerAvailable), + Status: metav1.ConditionTrue, + }, + gcpSpec: &hyperv1.GCPPlatformSpec{ + Project: "test-project", + Region: "us-central1", + }, + expectError: false, + expectedStatus: metav1.ConditionUnknown, + expectedReason: hyperv1.StatusUnknownReason, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + hcp := &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "test-namespace", + Generation: 1, + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.GCPPlatform, + GCP: tc.gcpSpec, + }, + }, + Status: hyperv1.HostedControlPlaneStatus{ + Conditions: []metav1.Condition{}, + }, + } + + if tc.kasCondition != nil { + meta.SetStatusCondition(&hcp.Status.Conditions, *tc.kasCondition) + } + + err := gcpHealthCheckIdentityProvider(t.Context(), hcp) + if tc.expectError && err == nil { + t.Fatal("expected error but got nil") + } + if !tc.expectError && err != nil { + t.Fatalf("expected no error but got: %v", err) + } + + for _, condType := range []string{ + string(hyperv1.ValidGCPWorkloadIdentity), + string(hyperv1.ValidGCPCredentials), + } { + condition := meta.FindStatusCondition(hcp.Status.Conditions, condType) + if condition == nil { + t.Fatalf("%s condition was not set", condType) + } + if condition.Status != tc.expectedStatus { + t.Errorf("%s: expected status %v, got %v", condType, tc.expectedStatus, condition.Status) + } + if condition.Reason != tc.expectedReason { + t.Errorf("%s: expected reason %v, got %v", condType, tc.expectedReason, condition.Reason) + } + if tc.expectedMessage != "" && condition.Message != tc.expectedMessage { + t.Errorf("%s: expected message %q, got %q", condType, tc.expectedMessage, condition.Message) + } + if condition.ObservedGeneration != hcp.Generation { + t.Errorf("%s: expected ObservedGeneration %v, got %v", condType, hcp.Generation, condition.ObservedGeneration) + } + } + }) + } +} + +func TestUpdateRunsGCPIdentityCheckDuringDeletion(t *testing.T) { + now := metav1.Now() + hcp := &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "test-namespace", + Generation: 1, + DeletionTimestamp: &now, + Finalizers: []string{"test-finalizer"}, + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.GCPPlatform, + GCP: &hyperv1.GCPPlatformSpec{ + Project: "test-project", + Region: "us-central1", + }, + }, + }, + Status: hyperv1.HostedControlPlaneStatus{ + Conditions: []metav1.Condition{}, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects(hcp). + WithStatusSubresource(&hyperv1.HostedControlPlane{}). + Build() + + ctx := ctrl.LoggerInto(t.Context(), ctrl.Log.WithName("test")) + + hcu := &HealthCheckUpdater{ + Client: fakeClient, + HostedControlPlane: client.ObjectKeyFromObject(hcp), + log: ctrl.Log.WithName("test"), + } + + if err := hcu.update(ctx); err != nil { + t.Fatalf("update() should succeed when GCP credentials are not available, got: %v", err) + } + + updated := &hyperv1.HostedControlPlane{} + if err := fakeClient.Get(ctx, client.ObjectKeyFromObject(hcp), updated); err != nil { + t.Fatalf("failed to get updated HCP: %v", err) + } + + for _, condType := range []string{ + string(hyperv1.ValidGCPWorkloadIdentity), + string(hyperv1.ValidGCPCredentials), + } { + condition := meta.FindStatusCondition(updated.Status.Conditions, condType) + if condition == nil { + t.Fatalf("%s condition was not set during deletion", condType) + } + if condition.Status != metav1.ConditionUnknown { + t.Errorf("%s: expected status %v, got %v", condType, metav1.ConditionUnknown, condition.Status) + } + } +} + +func TestGCPHealthCheckConditionDifferentiation(t *testing.T) { + kasTrue := &metav1.Condition{ + Type: string(hyperv1.KubeAPIServerAvailable), + Status: metav1.ConditionTrue, + } + gcpSpec := &hyperv1.GCPPlatformSpec{Project: "test-project", Region: "us-central1"} + + testCases := []struct { + name string + regionErr error + expectedWIFStatus metav1.ConditionStatus + expectedCredStatus metav1.ConditionStatus + expectError bool + }{ + { + name: "When WIF token exchange fails, it should set WIF to False and Credentials to Unknown", + regionErr: &oauth2.RetrieveError{Response: &http.Response{StatusCode: http.StatusUnauthorized}}, + expectedWIFStatus: metav1.ConditionFalse, + expectedCredStatus: metav1.ConditionUnknown, + expectError: true, + }, + { + name: "When Compute API returns 401, it should set WIF to True and Credentials to False", + regionErr: &googleapi.Error{Code: 401, Message: "Unauthorized"}, + expectedWIFStatus: metav1.ConditionTrue, + expectedCredStatus: metav1.ConditionFalse, + expectError: true, + }, + { + name: "When Compute API returns transient error, it should set both conditions to Unknown", + regionErr: &googleapi.Error{Code: 500, Message: "Internal Server Error"}, + expectedWIFStatus: metav1.ConditionUnknown, + expectedCredStatus: metav1.ConditionUnknown, + expectError: true, + }, + { + name: "When Compute API succeeds, it should set both conditions to True", + regionErr: nil, + expectedWIFStatus: metav1.ConditionTrue, + expectedCredStatus: metav1.ConditionTrue, + expectError: false, + }, + { + name: "When compute client is unavailable, it should set both conditions to Unknown without returning an error", + regionErr: errComputeClientUnavailable, + expectedWIFStatus: metav1.ConditionUnknown, + expectedCredStatus: metav1.ConditionUnknown, + expectError: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + injectedErr := tc.regionErr + old := gcpRegionChecker + gcpRegionChecker = func(_ context.Context, _, _ string) error { return injectedErr } + defer func() { gcpRegionChecker = old }() + + hcp := &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{Type: hyperv1.GCPPlatform, GCP: gcpSpec}, + }, + Status: hyperv1.HostedControlPlaneStatus{Conditions: []metav1.Condition{}}, + } + meta.SetStatusCondition(&hcp.Status.Conditions, *kasTrue) + + err := gcpHealthCheckIdentityProvider(t.Context(), hcp) + if tc.expectError && err == nil { + t.Fatal("expected error but got nil") + } + if !tc.expectError && err != nil { + t.Fatalf("expected no error but got: %v", err) + } + + wifCond := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.ValidGCPWorkloadIdentity)) + if wifCond == nil { + t.Fatal("ValidGCPWorkloadIdentity condition was not set") + } + if wifCond.Status != tc.expectedWIFStatus { + t.Errorf("ValidGCPWorkloadIdentity: expected status %v, got %v", tc.expectedWIFStatus, wifCond.Status) + } + + credCond := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.ValidGCPCredentials)) + if credCond == nil { + t.Fatal("ValidGCPCredentials condition was not set") + } + if credCond.Status != tc.expectedCredStatus { + t.Errorf("ValidGCPCredentials: expected status %v, got %v", tc.expectedCredStatus, credCond.Status) + } + }) + } +} + +func TestIsWIFTokenError(t *testing.T) { + testCases := []struct { + name string + err error + want bool + }{ + { + name: "When OAuth2 returns HTTP 401, it should classify the error as a WIF token error", + err: &oauth2.RetrieveError{Response: &http.Response{StatusCode: http.StatusUnauthorized}}, + want: true, + }, + { + name: "When OAuth2 returns HTTP 400, it should classify the error as a WIF token error", + err: &oauth2.RetrieveError{Response: &http.Response{StatusCode: http.StatusBadRequest}}, + want: true, + }, + { + name: "When OAuth2 returns HTTP 403, it should classify the error as a WIF token error", + err: &oauth2.RetrieveError{Response: &http.Response{StatusCode: http.StatusForbidden}}, + want: true, + }, + { + name: "When OAuth2 returns HTTP 429, it should not classify the error as a WIF token error", + err: &oauth2.RetrieveError{Response: &http.Response{StatusCode: http.StatusTooManyRequests}}, + want: false, + }, + { + name: "When a googleapi 401 error occurs, it should not classify the error as a WIF token error", + err: &googleapi.Error{Code: 401, Message: "Unauthorized"}, + want: false, + }, + { + name: "When a generic error occurs, it should not classify the error as a WIF token error", + err: fmt.Errorf("network timeout"), + want: false, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if got := isWIFTokenError(tc.err); got != tc.want { + t.Errorf("isWIFTokenError() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestIsComputeAuthError(t *testing.T) { + testCases := []struct { + name string + err error + want bool + }{ + { + name: "When a 401 googleapi error occurs, it should classify the error as a compute auth error", + err: &googleapi.Error{Code: 401, Message: "Unauthorized"}, + want: true, + }, + { + name: "When a wrapped googleapi 401 error occurs, it should classify the error as a compute auth error", + err: fmt.Errorf("compute call failed: %w", &googleapi.Error{Code: 401}), + want: true, + }, + { + name: "When a 403 googleapi error occurs, it should not classify the error as a compute auth error", + err: &googleapi.Error{Code: 403, Message: "Forbidden"}, + want: false, + }, + { + name: "When a 403 googleapi error with rateLimitExceeded occurs, it should not classify the error as a compute auth error", + err: &googleapi.Error{Code: 403, Errors: []googleapi.ErrorItem{{Reason: "rateLimitExceeded"}}}, + want: false, + }, + { + name: "When a 403 googleapi error with quotaExceeded occurs, it should not classify the error as a compute auth error", + err: &googleapi.Error{Code: 403, Errors: []googleapi.ErrorItem{{Reason: "quotaExceeded"}}}, + want: false, + }, + { + name: "When a 403 googleapi error with accessNotConfigured occurs, it should not classify the error as a compute auth error", + err: &googleapi.Error{Code: 403, Errors: []googleapi.ErrorItem{{Reason: "accessNotConfigured"}}}, + want: false, + }, + { + name: "When a 500 googleapi error occurs, it should not classify the error as a compute auth error", + err: &googleapi.Error{Code: 500, Message: "Internal Server Error"}, + want: false, + }, + { + name: "When an OAuth2 error occurs, it should not classify the error as a compute auth error", + err: &oauth2.RetrieveError{Response: &http.Response{StatusCode: http.StatusUnauthorized}}, + want: false, + }, + { + name: "When a generic error occurs, it should not classify the error as a compute auth error", + err: fmt.Errorf("network timeout"), + want: false, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if got := isComputeAuthError(tc.err); got != tc.want { + t.Errorf("isComputeAuthError() = %v, want %v", got, tc.want) + } + }) + } +} diff --git a/control-plane-operator/controllers/healthcheck/healthcheck_controller.go b/control-plane-operator/controllers/healthcheck/healthcheck_controller.go index 3d69b277f5b6..938223b4d954 100644 --- a/control-plane-operator/controllers/healthcheck/healthcheck_controller.go +++ b/control-plane-operator/controllers/healthcheck/healthcheck_controller.go @@ -81,7 +81,12 @@ func (hcu *HealthCheckUpdater) update(ctx context.Context) error { if err := awsHealthCheckIdentityProvider(ctx, hostedControlPlane); err != nil { errs = append(errs, err) } + } + if hostedControlPlane.Spec.Platform.Type == hyperv1.GCPPlatform { + if err := gcpHealthCheckIdentityProvider(ctx, hostedControlPlane); err != nil { + errs = append(errs, err) + } } // Update the status diff --git a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go index 4afdbad11cf3..eb191b2f5fca 100644 --- a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go +++ b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go @@ -40,6 +40,7 @@ import ( "github.com/openshift/hypershift/control-plane-pki-operator/certificates" "github.com/openshift/hypershift/hypershift-operator/controllers/hostedcluster/internal/platform" platformaws "github.com/openshift/hypershift/hypershift-operator/controllers/hostedcluster/internal/platform/aws" + platformgcp "github.com/openshift/hypershift/hypershift-operator/controllers/hostedcluster/internal/platform/gcp" "github.com/openshift/hypershift/hypershift-operator/controllers/hostedcluster/internal/proxy" hcmetrics "github.com/openshift/hypershift/hypershift-operator/controllers/hostedcluster/metrics" validations "github.com/openshift/hypershift/hypershift-operator/controllers/hostedcluster/validations" @@ -451,6 +452,17 @@ func (r *HostedClusterReconciler) reconcile(ctx context.Context, req ctrl.Reques } } + // Bubble up ValidGCPWorkloadIdentity and ValidGCPCredentials conditions from the hostedControlPlane. + // We set these conditions even if the HC is being deleted so that + // DeleteOrphanedMachines has a fresh signal for credential validity. + if hcluster.Spec.Platform.Type == hyperv1.GCPPlatform { + if changed := platformgcp.ComputeGCPCredentialConditions(hcluster, hcp); changed { + if err := r.Client.Status().Update(ctx, hcluster); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to update status: %w", err) + } + } + } + // Bubble up AWSDefaultSecurityGroupDeleted condition from the hostedControlPlane to report blocking objects on deletion. if condition, changed := computeAWSDefaultSGDeletedCondition(hcluster, hcp); changed { meta.SetStatusCondition(&hcluster.Status.Conditions, *condition) diff --git a/hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp.go b/hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp.go index 4358b9b9fc00..34e8b8b2f012 100644 --- a/hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp.go +++ b/hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp.go @@ -34,24 +34,24 @@ import ( "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/utils/ptr" capigcp "sigs.k8s.io/cluster-api-provider-gcp/api/v1beta1" capiv1 "sigs.k8s.io/cluster-api/api/core/v1beta1" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/blang/semver" ) -// CredentialStatus represents the status of GCP credentials +// CredentialStatus represents the status of GCP credentials. type CredentialStatus int const ( - // CredentialStatusValid indicates that GCP credentials are valid - CredentialStatusValid CredentialStatus = 0 - // CredentialStatusInvalid indicates that GCP credentials are invalid + CredentialStatusValid CredentialStatus = 0 CredentialStatusInvalid CredentialStatus = 1 - // CredentialStatusUnknown indicates that GCP credential status is unknown CredentialStatusUnknown CredentialStatus = 2 ) @@ -316,28 +316,14 @@ func (p GCP) ReconcileCredentials(ctx context.Context, c client.Client, createOr hcluster *hyperv1.HostedCluster, controlPlaneNamespace string, ) error { - // Validate GCP platform configuration is present if hcluster.Spec.Platform.GCP == nil { - setCondition(hcluster, hyperv1.ValidGCPWorkloadIdentity, metav1.ConditionFalse, "MissingGCPConfiguration", "GCP platform configuration is missing") - if updateErr := c.Status().Update(ctx, hcluster); updateErr != nil { - return fmt.Errorf("GCP platform configuration is missing (failed to update status: %w)", updateErr) - } return fmt.Errorf("GCP platform configuration is missing") } - // Validate Workload Identity Federation configuration (required) - if err := p.validateWorkloadIdentityConfiguration(hcluster); err != nil { - setCondition(hcluster, hyperv1.ValidGCPWorkloadIdentity, metav1.ConditionFalse, "InvalidWIFConfiguration", fmt.Sprintf("Workload Identity Federation configuration is invalid: %v", err)) - if updateErr := c.Status().Update(ctx, hcluster); updateErr != nil { - return fmt.Errorf("invalid workload identity configuration: %w (failed to update status: %w)", err, updateErr) - } + if err := validateWorkloadIdentityConfiguration(hcluster); err != nil { return fmt.Errorf("invalid workload identity configuration: %w", err) } - // Set successful WIF validation condition - setCondition(hcluster, hyperv1.ValidGCPWorkloadIdentity, metav1.ConditionTrue, "ValidWIFConfiguration", "Workload Identity Federation configuration is valid and ready") - - // Create credential secrets following AWS pattern var errs []error syncSecret := func(secret *corev1.Secret, serviceAccountEmail string) error { credentials, err := gcputil.BuildWorkloadIdentityCredentials(hcluster.Spec.Platform.GCP.WorkloadIdentity, serviceAccountEmail) @@ -354,7 +340,6 @@ func (p GCP) ReconcileCredentials(ctx context.Context, c client.Client, createOr return nil } - // Create credential secrets for all configured service accounts credentialSecrets := map[hyperv1.GCPServiceAccountEmail]*corev1.Secret{ hcluster.Spec.Platform.GCP.WorkloadIdentity.ServiceAccountsEmails.NodePool: NodePoolManagementCredsSecret(controlPlaneNamespace), hcluster.Spec.Platform.GCP.WorkloadIdentity.ServiceAccountsEmails.ControlPlane: ControlPlaneOperatorCredsSecret(controlPlaneNamespace), @@ -371,21 +356,9 @@ func (p GCP) ReconcileCredentials(ctx context.Context, c client.Client, createOr } if len(errs) > 0 { - setCondition(hcluster, hyperv1.ValidGCPCredentials, metav1.ConditionFalse, "CredentialsError", fmt.Sprintf("Failed to reconcile credentials: %v", errs)) - if updateErr := c.Status().Update(ctx, hcluster); updateErr != nil { - return fmt.Errorf("failed to reconcile GCP credentials: %v (failed to update status: %w)", errs, updateErr) - } return fmt.Errorf("failed to reconcile GCP credentials: %v", errs) } - // Set credentials condition to indicate federation is ready - setCondition(hcluster, hyperv1.ValidGCPCredentials, metav1.ConditionTrue, "WIFReady", "GCP Workload Identity Federation is configured and ready") - - // Persist status condition changes to the API server - if err := c.Status().Update(ctx, hcluster); err != nil { - return fmt.Errorf("failed to update HostedCluster status conditions: %w", err) - } - return nil } @@ -481,27 +454,8 @@ func (p GCP) DeleteCredentials(ctx context.Context, c client.Client, hcluster *h return nil } -// ValidCredentials checks if GCP credentials are valid and ready for use. -// This function validates Workload Identity Federation configuration and status. -func ValidCredentials(hc *hyperv1.HostedCluster) bool { - // Check if GCP Workload Identity Federation is configured and valid - validWIF := meta.FindStatusCondition(hc.Status.Conditions, string(hyperv1.ValidGCPWorkloadIdentity)) - if validWIF == nil || validWIF.Status != metav1.ConditionTrue { - return false - } - - // Check if GCP credentials condition indicates WIF is ready - validCredentials := meta.FindStatusCondition(hc.Status.Conditions, string(hyperv1.ValidGCPCredentials)) - if validCredentials == nil || validCredentials.Status != metav1.ConditionTrue { - return false - } - - return true -} - -// GetCredentialStatus returns the GCP credential status (valid/invalid/unknown) +// GetCredentialStatus returns the GCP credential status (valid/invalid/unknown). func GetCredentialStatus(hc *hyperv1.HostedCluster) CredentialStatus { - // Get GCP Workload Identity Federation status var wifStatus metav1.ConditionStatus validWIF := meta.FindStatusCondition(hc.Status.Conditions, string(hyperv1.ValidGCPWorkloadIdentity)) if validWIF == nil { @@ -510,19 +464,14 @@ func GetCredentialStatus(hc *hyperv1.HostedCluster) CredentialStatus { wifStatus = validWIF.Status } - // Get GCP credentials status var credsStatus metav1.ConditionStatus - validCreds := meta.FindStatusCondition(hc.Status.Conditions, string(hyperv1.ValidGCPCredentials)) - if validCreds == nil { + validCredentials := meta.FindStatusCondition(hc.Status.Conditions, string(hyperv1.ValidGCPCredentials)) + if validCredentials == nil { credsStatus = metav1.ConditionUnknown } else { - credsStatus = validCreds.Status + credsStatus = validCredentials.Status } - // Combine the results: - // - If either is explicitly False → Invalid - // - If both are True → Valid - // - Otherwise → Unknown if wifStatus == metav1.ConditionFalse || credsStatus == metav1.ConditionFalse { return CredentialStatusInvalid } @@ -532,9 +481,56 @@ func GetCredentialStatus(hc *hyperv1.HostedCluster) CredentialStatus { return CredentialStatusUnknown } +// ComputeGCPCredentialConditions bubbles up ValidGCPWorkloadIdentity and +// ValidGCPCredentials from the HostedControlPlane to the HostedCluster. +// Returns whether any condition changed. +// +// Invalid is latched: if the HC already has a False condition and the HCP now +// reports Unknown (e.g. because KAS is gone during teardown), the existing +// False is kept so that DeleteOrphanedMachines continues to fire and teardown +// does not get stuck again. +func ComputeGCPCredentialConditions(hc *hyperv1.HostedCluster, hcp *hyperv1.HostedControlPlane) bool { + var changed bool + for _, condType := range []hyperv1.ConditionType{ + hyperv1.ValidGCPWorkloadIdentity, + hyperv1.ValidGCPCredentials, + } { + var hcpCond *metav1.Condition + if hcp != nil { + hcpCond = meta.FindStatusCondition(hcp.Status.Conditions, string(condType)) + } + + var fresh metav1.Condition + if hcpCond == nil || hcpCond.Status == metav1.ConditionUnknown { + // Latch: keep an existing False on the HC rather than downgrading to + // Unknown. This prevents a KAS-unavailable signal during teardown from + // clobbering a previously confirmed Invalid state and silently stopping + // orphan machine cleanup. + existing := meta.FindStatusCondition(hc.Status.Conditions, string(condType)) + if existing != nil && existing.Status == metav1.ConditionFalse { + continue // keep the latched Invalid; nothing to update + } + fresh = metav1.Condition{ + Type: string(condType), + Status: metav1.ConditionUnknown, + Reason: hyperv1.StatusUnknownReason, + ObservedGeneration: hc.Generation, + } + } else { + fresh = *hcpCond + fresh.ObservedGeneration = hc.Generation + } + + if meta.SetStatusCondition(&hc.Status.Conditions, fresh) { + changed = true + } + } + return changed +} + // validateWorkloadIdentityConfiguration validates the Workload Identity Federation configuration. // This ensures all required fields are present and properly formatted. -func (p GCP) validateWorkloadIdentityConfiguration(hcluster *hyperv1.HostedCluster) error { +func validateWorkloadIdentityConfiguration(hcluster *hyperv1.HostedCluster) error { // Note: GCP platform configuration nil check is handled by caller wif := hcluster.Spec.Platform.GCP.WorkloadIdentity @@ -582,14 +578,29 @@ func (p GCP) validateWorkloadIdentityConfiguration(hcluster *hyperv1.HostedClust return nil } -// setCondition updates or creates a condition on the HostedCluster. -// This follows the standard HyperShift pattern for condition management. -func setCondition(hcluster *hyperv1.HostedCluster, conditionType hyperv1.ConditionType, status metav1.ConditionStatus, reason, message string) { - meta.SetStatusCondition(&hcluster.Status.Conditions, metav1.Condition{ - Type: string(conditionType), - Status: status, - Reason: reason, - Message: message, - LastTransitionTime: metav1.Now(), - }) +func (GCP) DeleteOrphanedMachines(ctx context.Context, c client.Client, hc *hyperv1.HostedCluster, controlPlaneNamespace string) error { + if GetCredentialStatus(hc) != CredentialStatusInvalid { + return nil + } + gcpMachineList := capigcp.GCPMachineList{} + if err := c.List(ctx, &gcpMachineList, client.InNamespace(controlPlaneNamespace)); err != nil { + return fmt.Errorf("failed to list GCPMachines in %s: %w", controlPlaneNamespace, err) + } + logger := ctrl.LoggerFrom(ctx) + var errs []error + for i := range gcpMachineList.Items { + gcpMachine := &gcpMachineList.Items[i] + if gcpMachine.DeletionTimestamp.IsZero() { + continue + } + if removed := controllerutil.RemoveFinalizer(gcpMachine, capigcp.MachineFinalizer); !removed { + continue + } + if err := c.Update(ctx, gcpMachine); err != nil { + errs = append(errs, fmt.Errorf("failed to remove finalizer from GCPMachine %s/%s: %w", gcpMachine.Namespace, gcpMachine.Name, err)) + continue + } + logger.Info("removed CAPG finalizer from orphaned gcpmachine due to invalid GCP credentials", "machine", client.ObjectKeyFromObject(gcpMachine)) + } + return utilerrors.NewAggregate(errs) } diff --git a/hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp_conditions_test.go b/hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp_conditions_test.go index 3a7bdbaed993..a00d2fa4d82a 100644 --- a/hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp_conditions_test.go +++ b/hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp_conditions_test.go @@ -8,126 +8,12 @@ import ( hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" capigcp "sigs.k8s.io/cluster-api-provider-gcp/api/v1beta1" ) -// TestValidCredentials tests the ValidCredentials method for various condition states. -// This tests the logic for checking both ValidGCPWorkloadIdentity and ValidGCPCredentials conditions. -func TestValidCredentials(t *testing.T) { - tests := []struct { - name string - conditions []metav1.Condition - expected bool - description string - }{ - { - name: "When both conditions are true, it should return true", - conditions: []metav1.Condition{ - { - Type: string(hyperv1.ValidGCPWorkloadIdentity), - Status: metav1.ConditionTrue, - }, - { - Type: string(hyperv1.ValidGCPCredentials), - Status: metav1.ConditionTrue, - }, - }, - expected: true, - description: "Both conditions are present and true", - }, - { - name: "When ValidGCPWorkloadIdentity is false, it should return false", - conditions: []metav1.Condition{ - { - Type: string(hyperv1.ValidGCPWorkloadIdentity), - Status: metav1.ConditionFalse, - }, - { - Type: string(hyperv1.ValidGCPCredentials), - Status: metav1.ConditionTrue, - }, - }, - expected: false, - description: "ValidGCPWorkloadIdentity is false", - }, - { - name: "When ValidGCPCredentials is false, it should return false", - conditions: []metav1.Condition{ - { - Type: string(hyperv1.ValidGCPWorkloadIdentity), - Status: metav1.ConditionTrue, - }, - { - Type: string(hyperv1.ValidGCPCredentials), - Status: metav1.ConditionFalse, - }, - }, - expected: false, - description: "ValidGCPCredentials is false", - }, - { - name: "When both conditions are false, it should return false", - conditions: []metav1.Condition{ - { - Type: string(hyperv1.ValidGCPWorkloadIdentity), - Status: metav1.ConditionFalse, - }, - { - Type: string(hyperv1.ValidGCPCredentials), - Status: metav1.ConditionFalse, - }, - }, - expected: false, - description: "Both conditions are false", - }, - { - name: "When ValidGCPWorkloadIdentity is missing, it should return false", - conditions: []metav1.Condition{ - { - Type: string(hyperv1.ValidGCPCredentials), - Status: metav1.ConditionTrue, - }, - }, - expected: false, - description: "ValidGCPWorkloadIdentity condition is missing", - }, - { - name: "When ValidGCPCredentials is missing, it should return false", - conditions: []metav1.Condition{ - { - Type: string(hyperv1.ValidGCPWorkloadIdentity), - Status: metav1.ConditionTrue, - }, - }, - expected: false, - description: "ValidGCPCredentials condition is missing", - }, - { - name: "When no conditions exist, it should return false", - conditions: []metav1.Condition{}, - expected: false, - description: "No conditions present", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - hc := &hyperv1.HostedCluster{ - Status: hyperv1.HostedClusterStatus{ - Conditions: tt.conditions, - }, - } - - result := ValidCredentials(hc) - g.Expect(result).To(Equal(tt.expected), tt.description) - }) - } -} - // TestGetCredentialStatus tests the GetCredentialStatus function for various condition states. // This tests the tri-state logic (valid/invalid/unknown) for GCP credential conditions. func TestGetCredentialStatus(t *testing.T) { @@ -267,7 +153,6 @@ func TestGetCredentialStatus(t *testing.T) { // This expands on the existing TestValidateWorkloadIdentityConfiguration with more comprehensive coverage. func TestWorkloadIdentityValidationScenarios(t *testing.T) { g := NewWithT(t) - platform := New("test-utilities-image", "test-capg-image", nil) tests := []struct { name string @@ -348,7 +233,7 @@ func TestWorkloadIdentityValidationScenarios(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := platform.validateWorkloadIdentityConfiguration(tt.hcluster) + err := validateWorkloadIdentityConfiguration(tt.hcluster) if tt.expectError { g.Expect(err).ToNot(BeNil()) if tt.errorMsg != "" { @@ -406,6 +291,141 @@ func TestNetworkConfigAccessSafety(t *testing.T) { g.Expect(gcpCluster.Spec.Network.Name).To(BeNil()) } +func TestComputeGCPCredentialConditions(t *testing.T) { + tests := []struct { + name string + hcConditions []metav1.Condition + hcp *hyperv1.HostedControlPlane + expectedChanged bool + expectedWIFStatus metav1.ConditionStatus + expectedWIFReason string + expectedCredStatus metav1.ConditionStatus + }{ + { + name: "When HCP has True conditions, it should bubble them up to HC", + hcp: &hyperv1.HostedControlPlane{ + Status: hyperv1.HostedControlPlaneStatus{ + Conditions: []metav1.Condition{ + {Type: string(hyperv1.ValidGCPWorkloadIdentity), Status: metav1.ConditionTrue, Reason: hyperv1.AsExpectedReason}, + {Type: string(hyperv1.ValidGCPCredentials), Status: metav1.ConditionTrue, Reason: hyperv1.AsExpectedReason}, + }, + }, + }, + expectedChanged: true, + expectedWIFStatus: metav1.ConditionTrue, + expectedWIFReason: hyperv1.AsExpectedReason, + expectedCredStatus: metav1.ConditionTrue, + }, + { + name: "When HCP is nil, it should set conditions to Unknown", + hcp: nil, + expectedChanged: true, + expectedWIFStatus: metav1.ConditionUnknown, + expectedWIFReason: hyperv1.StatusUnknownReason, + expectedCredStatus: metav1.ConditionUnknown, + }, + { + name: "When HCP has no conditions, it should set conditions to Unknown", + hcp: &hyperv1.HostedControlPlane{ + Status: hyperv1.HostedControlPlaneStatus{ + Conditions: []metav1.Condition{}, + }, + }, + expectedChanged: true, + expectedWIFStatus: metav1.ConditionUnknown, + expectedWIFReason: hyperv1.StatusUnknownReason, + expectedCredStatus: metav1.ConditionUnknown, + }, + { + name: "When HCP has False conditions, it should propagate them to HC", + hcp: &hyperv1.HostedControlPlane{ + Status: hyperv1.HostedControlPlaneStatus{ + Conditions: []metav1.Condition{ + {Type: string(hyperv1.ValidGCPWorkloadIdentity), Status: metav1.ConditionFalse, Reason: hyperv1.InvalidIdentityProvider}, + {Type: string(hyperv1.ValidGCPCredentials), Status: metav1.ConditionFalse, Reason: hyperv1.InvalidIdentityProvider}, + }, + }, + }, + expectedChanged: true, + expectedWIFStatus: metav1.ConditionFalse, + expectedWIFReason: hyperv1.InvalidIdentityProvider, + expectedCredStatus: metav1.ConditionFalse, + }, + { + name: "When HC already has the same conditions, it should not report a change", + hcConditions: []metav1.Condition{ + {Type: string(hyperv1.ValidGCPWorkloadIdentity), Status: metav1.ConditionTrue, Reason: hyperv1.AsExpectedReason, ObservedGeneration: 3}, + {Type: string(hyperv1.ValidGCPCredentials), Status: metav1.ConditionTrue, Reason: hyperv1.AsExpectedReason, ObservedGeneration: 3}, + }, + hcp: &hyperv1.HostedControlPlane{ + Status: hyperv1.HostedControlPlaneStatus{ + Conditions: []metav1.Condition{ + {Type: string(hyperv1.ValidGCPWorkloadIdentity), Status: metav1.ConditionTrue, Reason: hyperv1.AsExpectedReason}, + {Type: string(hyperv1.ValidGCPCredentials), Status: metav1.ConditionTrue, Reason: hyperv1.AsExpectedReason}, + }, + }, + }, + expectedChanged: false, + expectedWIFStatus: metav1.ConditionTrue, + expectedWIFReason: hyperv1.AsExpectedReason, + expectedCredStatus: metav1.ConditionTrue, + }, + { + // Latch: during teardown KAS goes away, HCP sends Unknown. An existing + // False on the HC must not be clobbered so DeleteOrphanedMachines keeps firing. + name: "When HC has False conditions and HCP sends Unknown, it should latch False and not update", + hcConditions: []metav1.Condition{ + {Type: string(hyperv1.ValidGCPWorkloadIdentity), Status: metav1.ConditionFalse, Reason: hyperv1.InvalidIdentityProvider, ObservedGeneration: 3}, + {Type: string(hyperv1.ValidGCPCredentials), Status: metav1.ConditionFalse, Reason: hyperv1.InvalidIdentityProvider, ObservedGeneration: 3}, + }, + hcp: nil, + expectedChanged: false, + expectedWIFStatus: metav1.ConditionFalse, + expectedWIFReason: hyperv1.InvalidIdentityProvider, + expectedCredStatus: metav1.ConditionFalse, + }, + { + // Partial latch: WIF is False (latched), Credentials was Unknown — HCP now + // sends Unknown for both. WIF stays False; Credentials stays Unknown (no change). + name: "When HC has False WIF and Unknown Credentials and HCP sends Unknown, it should latch False WIF only", + hcConditions: []metav1.Condition{ + {Type: string(hyperv1.ValidGCPWorkloadIdentity), Status: metav1.ConditionFalse, Reason: hyperv1.InvalidIdentityProvider, ObservedGeneration: 3}, + {Type: string(hyperv1.ValidGCPCredentials), Status: metav1.ConditionUnknown, Reason: hyperv1.StatusUnknownReason, ObservedGeneration: 3}, + }, + hcp: nil, + expectedChanged: false, + expectedWIFStatus: metav1.ConditionFalse, + expectedWIFReason: hyperv1.InvalidIdentityProvider, + expectedCredStatus: metav1.ConditionUnknown, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + hc := &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{Generation: 3}, + Status: hyperv1.HostedClusterStatus{ + Conditions: tt.hcConditions, + }, + } + + changed := ComputeGCPCredentialConditions(hc, tt.hcp) + g.Expect(changed).To(Equal(tt.expectedChanged), "changed flag mismatch") + + wifCond := meta.FindStatusCondition(hc.Status.Conditions, string(hyperv1.ValidGCPWorkloadIdentity)) + g.Expect(wifCond).ToNot(BeNil(), "ValidGCPWorkloadIdentity condition must be set") + g.Expect(wifCond.Status).To(Equal(tt.expectedWIFStatus), "ValidGCPWorkloadIdentity status mismatch") + g.Expect(wifCond.Reason).To(Equal(tt.expectedWIFReason), "ValidGCPWorkloadIdentity reason mismatch") + + credCond := meta.FindStatusCondition(hc.Status.Conditions, string(hyperv1.ValidGCPCredentials)) + g.Expect(credCond).ToNot(BeNil(), "ValidGCPCredentials condition must be set") + g.Expect(credCond.Status).To(Equal(tt.expectedCredStatus), "ValidGCPCredentials status mismatch") + }) + } +} + // TestServiceAccountEmailValidation tests that the regex pattern validation for service account emails is working correctly. // This addresses CodeRabbit feedback about hardening the service account email pattern. func TestServiceAccountEmailValidation(t *testing.T) { diff --git a/hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp_test.go b/hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp_test.go index f2dce1d90f77..b931cc4daee8 100644 --- a/hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp_test.go +++ b/hypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp_test.go @@ -2,6 +2,7 @@ package gcp import ( "context" + "fmt" "testing" . "github.com/onsi/gomega" @@ -18,6 +19,7 @@ import ( capigcp "sigs.k8s.io/cluster-api-provider-gcp/api/v1beta1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/blang/semver" @@ -301,8 +303,6 @@ func TestDeleteCredentials(t *testing.T) { func TestValidateWorkloadIdentityConfiguration(t *testing.T) { g := NewWithT(t) - platform := New("test-utilities-image", "test-capg-image", &semver.Version{Major: 4, Minor: 17, Patch: 0}) - tests := []struct { name string mutate func(*hyperv1.HostedCluster) @@ -362,7 +362,7 @@ func TestValidateWorkloadIdentityConfiguration(t *testing.T) { if tt.mutate != nil { tt.mutate(hc) } - err := platform.validateWorkloadIdentityConfiguration(hc) + err := validateWorkloadIdentityConfiguration(hc) if tt.errorMsg != "" { g.Expect(err).ToNot(BeNil()) g.Expect(err.Error()).To(ContainSubstring(tt.errorMsg)) @@ -527,3 +527,219 @@ func TestCAPIProviderDeploymentSpecWithTLS(t *testing.T) { }) } } + +func TestDeleteOrphanedMachines(t *testing.T) { + buildScheme := func(g Gomega) *runtime.Scheme { + scheme := runtime.NewScheme() + g.Expect(clientgoscheme.AddToScheme(scheme)).To(Succeed()) + g.Expect(hyperv1.AddToScheme(scheme)).To(Succeed()) + g.Expect(capigcp.AddToScheme(scheme)).To(Succeed()) + return scheme + } + + gcpMachines := func() []client.Object { + return []client.Object{ + &capigcp.GCPMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-xl-1", + Namespace: "test-control-plane-namespace", + DeletionTimestamp: ptr.To(metav1.Now()), + Finalizers: []string{capigcp.MachineFinalizer, "other-controller-finalizer"}, + }, + }, + &capigcp.GCPMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-xl-2", + Namespace: "test-control-plane-namespace", + Finalizers: []string{capigcp.MachineFinalizer, "other-controller-finalizer"}, + }, + }, + } + } + + t.Run("When credentials are invalid, it should strip the CAPG finalizer from deleting machines", func(t *testing.T) { + g := NewWithT(t) + platform := New("test-utilities-image", "test-capg-image", &semver.Version{Major: 4, Minor: 17, Patch: 0}) + + hc := validHostedCluster() + hc.Status.Conditions = []metav1.Condition{ + {Type: string(hyperv1.ValidGCPWorkloadIdentity), Status: metav1.ConditionFalse}, + {Type: string(hyperv1.ValidGCPCredentials), Status: metav1.ConditionFalse}, + } + + fakeClient := fake.NewClientBuilder(). + WithObjects(gcpMachines()...). + WithScheme(buildScheme(g)). + Build() + + err := platform.DeleteOrphanedMachines(t.Context(), fakeClient, hc, "test-control-plane-namespace") + g.Expect(err).To(BeNil()) + + gcpMachineList := &capigcp.GCPMachineList{} + g.Expect(fakeClient.List(t.Context(), gcpMachineList)).To(Succeed()) + + for _, gcpMachine := range gcpMachineList.Items { + if !gcpMachine.DeletionTimestamp.IsZero() { + g.Expect(controllerutil.ContainsFinalizer(&gcpMachine, capigcp.MachineFinalizer)).To(BeFalse(), "CAPG finalizer should be removed") + g.Expect(gcpMachine.Finalizers).To(Equal([]string{"other-controller-finalizer"}), "other finalizers should be preserved") + } else { + g.Expect(gcpMachine.Finalizers).To(Equal([]string{capigcp.MachineFinalizer, "other-controller-finalizer"})) + } + } + }) + + t.Run("When credentials are valid, it should leave finalizers unchanged", func(t *testing.T) { + g := NewWithT(t) + platform := New("test-utilities-image", "test-capg-image", &semver.Version{Major: 4, Minor: 17, Patch: 0}) + + hc := validHostedCluster() + hc.Status.Conditions = []metav1.Condition{ + {Type: string(hyperv1.ValidGCPWorkloadIdentity), Status: metav1.ConditionTrue}, + {Type: string(hyperv1.ValidGCPCredentials), Status: metav1.ConditionTrue}, + } + + fakeClient := fake.NewClientBuilder(). + WithObjects(gcpMachines()...). + WithScheme(buildScheme(g)). + Build() + + err := platform.DeleteOrphanedMachines(t.Context(), fakeClient, hc, "test-control-plane-namespace") + g.Expect(err).To(BeNil()) + + gcpMachineList := &capigcp.GCPMachineList{} + g.Expect(fakeClient.List(t.Context(), gcpMachineList)).To(Succeed()) + + for _, gcpMachine := range gcpMachineList.Items { + g.Expect(gcpMachine.Finalizers).To(Equal([]string{capigcp.MachineFinalizer, "other-controller-finalizer"})) + } + }) + + t.Run("When credentials are unknown, it should leave finalizers unchanged", func(t *testing.T) { + g := NewWithT(t) + platform := New("test-utilities-image", "test-capg-image", &semver.Version{Major: 4, Minor: 17, Patch: 0}) + + hc := validHostedCluster() + // No conditions set — GetCredentialStatus returns Unknown + + fakeClient := fake.NewClientBuilder(). + WithObjects(gcpMachines()...). + WithScheme(buildScheme(g)). + Build() + + err := platform.DeleteOrphanedMachines(t.Context(), fakeClient, hc, "test-control-plane-namespace") + g.Expect(err).To(BeNil()) + + gcpMachineList := &capigcp.GCPMachineList{} + g.Expect(fakeClient.List(t.Context(), gcpMachineList)).To(Succeed()) + + for _, gcpMachine := range gcpMachineList.Items { + g.Expect(gcpMachine.Finalizers).To(Equal([]string{capigcp.MachineFinalizer, "other-controller-finalizer"})) + } + }) + + t.Run("When c.List fails, it should return the error", func(t *testing.T) { + g := NewWithT(t) + platform := New("test-utilities-image", "test-capg-image", &semver.Version{Major: 4, Minor: 17, Patch: 0}) + + hc := validHostedCluster() + hc.Status.Conditions = []metav1.Condition{ + {Type: string(hyperv1.ValidGCPWorkloadIdentity), Status: metav1.ConditionFalse}, + {Type: string(hyperv1.ValidGCPCredentials), Status: metav1.ConditionFalse}, + } + + listErr := fmt.Errorf("list failed") + fakeClient := interceptor.NewClient( + fake.NewClientBuilder().WithScheme(buildScheme(g)).Build(), + interceptor.Funcs{ + List: func(_ context.Context, _ client.WithWatch, _ client.ObjectList, _ ...client.ListOption) error { + return listErr + }, + }, + ) + + err := platform.DeleteOrphanedMachines(t.Context(), fakeClient, hc, "test-control-plane-namespace") + g.Expect(err).To(MatchError(ContainSubstring("failed to list GCPMachines")), "expected list error to be wrapped") + }) + + t.Run("When c.Update fails for a machine, it should aggregate errors and continue", func(t *testing.T) { + g := NewWithT(t) + platform := New("test-utilities-image", "test-capg-image", &semver.Version{Major: 4, Minor: 17, Patch: 0}) + + hc := validHostedCluster() + hc.Status.Conditions = []metav1.Condition{ + {Type: string(hyperv1.ValidGCPWorkloadIdentity), Status: metav1.ConditionFalse}, + {Type: string(hyperv1.ValidGCPCredentials), Status: metav1.ConditionFalse}, + } + + // Two deleted machines; Update fails for both. + machines := []client.Object{ + &capigcp.GCPMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-m-1", + Namespace: "test-control-plane-namespace", + DeletionTimestamp: ptr.To(metav1.Now()), + Finalizers: []string{capigcp.MachineFinalizer}, + }, + }, + &capigcp.GCPMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-m-2", + Namespace: "test-control-plane-namespace", + DeletionTimestamp: ptr.To(metav1.Now()), + Finalizers: []string{capigcp.MachineFinalizer}, + }, + }, + } + + updateErr := fmt.Errorf("update failed") + fakeClient := interceptor.NewClient( + fake.NewClientBuilder().WithObjects(machines...).WithScheme(buildScheme(g)).Build(), + interceptor.Funcs{ + Update: func(_ context.Context, _ client.WithWatch, _ client.Object, _ ...client.UpdateOption) error { + return updateErr + }, + }, + ) + + err := platform.DeleteOrphanedMachines(t.Context(), fakeClient, hc, "test-control-plane-namespace") + g.Expect(err).To(HaveOccurred(), "expected aggregated error") + g.Expect(err.Error()).To(ContainSubstring("test-m-1"), "error should mention first machine") + g.Expect(err.Error()).To(ContainSubstring("test-m-2"), "error should mention second machine") + }) + + t.Run("When GCPMachine has no CAPG finalizer, it should skip it without error", func(t *testing.T) { + g := NewWithT(t) + platform := New("test-utilities-image", "test-capg-image", &semver.Version{Major: 4, Minor: 17, Patch: 0}) + + hc := validHostedCluster() + hc.Status.Conditions = []metav1.Condition{ + {Type: string(hyperv1.ValidGCPWorkloadIdentity), Status: metav1.ConditionFalse}, + {Type: string(hyperv1.ValidGCPCredentials), Status: metav1.ConditionFalse}, + } + + // Deleted machine, but without the CAPG finalizer (already removed by another path). + machines := []client.Object{ + &capigcp.GCPMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-no-finalizer", + Namespace: "test-control-plane-namespace", + DeletionTimestamp: ptr.To(metav1.Now()), + Finalizers: []string{"some-other-finalizer"}, + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithObjects(machines...). + WithScheme(buildScheme(g)). + Build() + + err := platform.DeleteOrphanedMachines(t.Context(), fakeClient, hc, "test-control-plane-namespace") + g.Expect(err).To(BeNil(), "machine without CAPG finalizer should be skipped without error") + + // Verify the other finalizer is untouched. + gcpMachineList := &capigcp.GCPMachineList{} + g.Expect(fakeClient.List(t.Context(), gcpMachineList)).To(Succeed()) + g.Expect(gcpMachineList.Items[0].Finalizers).To(Equal([]string{"some-other-finalizer"}), "other finalizers must not be modified") + }) +} diff --git a/hypershift-operator/controllers/hostedcluster/internal/platform/platform.go b/hypershift-operator/controllers/hostedcluster/internal/platform/platform.go index 91482db99c96..4b71d026387f 100644 --- a/hypershift-operator/controllers/hostedcluster/internal/platform/platform.go +++ b/hypershift-operator/controllers/hostedcluster/internal/platform/platform.go @@ -43,7 +43,9 @@ var ( _ Platform = agent.Agent{} _ Platform = kubevirt.Kubevirt{} _ Platform = gcp.GCP{} - _ OrphanDeleter = &azure.Azure{} + _ OrphanDeleter = aws.AWS{} + _ OrphanDeleter = azure.Azure{} + _ OrphanDeleter = gcp.GCP{} ) type Platform interface {