diff --git a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go index c1f86b5cef3a..09b37e82b5c1 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go +++ b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go @@ -88,6 +88,7 @@ import ( "github.com/openshift/hypershift/support/metrics" "github.com/openshift/hypershift/support/netutil" "github.com/openshift/hypershift/support/releaseinfo" + "github.com/openshift/hypershift/support/statuspatching" "github.com/openshift/hypershift/support/upsert" "github.com/openshift/hypershift/support/util" "github.com/openshift/hypershift/support/validations" @@ -382,8 +383,8 @@ func (r *HostedControlPlaneReconciler) eventHandlers(scheme *runtime.Scheme, res return handlers } -func (r *HostedControlPlaneReconciler) reconcileDeletion(ctx context.Context, hostedControlPlane *hyperv1.HostedControlPlane, originalHostedControlPlane *hyperv1.HostedControlPlane) (ctrl.Result, error) { - condition := &metav1.Condition{ +func (r *HostedControlPlaneReconciler) reconcileDeletion(ctx context.Context, hostedControlPlane *hyperv1.HostedControlPlane) (ctrl.Result, error) { + condition := metav1.Condition{ Type: string(hyperv1.AWSDefaultSecurityGroupDeleted), } if shouldCleanupCloudResources(r.Log, hostedControlPlane) { @@ -394,9 +395,7 @@ func (r *HostedControlPlaneReconciler) reconcileDeletion(ctx context.Context, ho } condition.Reason = hyperv1.AWSErrorReason condition.Status = metav1.ConditionFalse - meta.SetStatusCondition(&hostedControlPlane.Status.Conditions, *condition) - - if err := r.Client.Status().Patch(ctx, hostedControlPlane, client.MergeFromWithOptions(originalHostedControlPlane, client.MergeFromWithOptimisticLock{})); err != nil { + if err := statuspatching.PatchStatusCondition(ctx, r.Client, hostedControlPlane, &hostedControlPlane.Status.Conditions, condition); err != nil { return ctrl.Result{}, fmt.Errorf("failed to update status on hcp for security group deletion: %w. Condition error message: %v", err, condition.Message) } @@ -414,9 +413,7 @@ func (r *HostedControlPlaneReconciler) reconcileDeletion(ctx context.Context, ho condition.Message = hyperv1.AllIsWellMessage condition.Reason = hyperv1.AsExpectedReason condition.Status = metav1.ConditionTrue - meta.SetStatusCondition(&hostedControlPlane.Status.Conditions, *condition) - - if err := r.Client.Status().Patch(ctx, hostedControlPlane, client.MergeFromWithOptions(originalHostedControlPlane, client.MergeFromWithOptimisticLock{})); err != nil { + if err := statuspatching.PatchStatusCondition(ctx, r.Client, hostedControlPlane, &hostedControlPlane.Status.Conditions, condition); err != nil { return ctrl.Result{}, fmt.Errorf("failed to update status on hcp for security group deletion: %w. Condition message: %v", err, condition.Message) } } @@ -601,7 +598,7 @@ func (r *HostedControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.R originalHostedControlPlane := hostedControlPlane.DeepCopy() if !hostedControlPlane.DeletionTimestamp.IsZero() { - return r.reconcileDeletion(ctx, hostedControlPlane, originalHostedControlPlane) + return r.reconcileDeletion(ctx, hostedControlPlane) } if !controllerutil.ContainsFinalizer(hostedControlPlane, finalizer) { @@ -1168,32 +1165,27 @@ func (r *HostedControlPlaneReconciler) update(ctx context.Context, hostedControl errs = append(errs, err) } - // Get the latest HCP in memory before we patch the status - if err = r.Client.Get(ctx, client.ObjectKeyFromObject(hostedControlPlane), hostedControlPlane); err != nil { - return reconcile.Result{}, err - } - - originalHostedControlPlane := hostedControlPlane.DeepCopy() missingImages := sets.New(releaseImageProvider.GetMissingImages()...).Insert(userReleaseImageProvider.GetMissingImages()...) - if missingImages.Len() == 0 { - meta.SetStatusCondition(&hostedControlPlane.Status.Conditions, metav1.Condition{ - Type: string(hyperv1.ValidReleaseInfo), - Status: metav1.ConditionTrue, - Reason: hyperv1.AsExpectedReason, - Message: hyperv1.AllIsWellMessage, - ObservedGeneration: hostedControlPlane.Generation, - }) - } else { - meta.SetStatusCondition(&hostedControlPlane.Status.Conditions, metav1.Condition{ - Type: string(hyperv1.ValidReleaseInfo), - Status: metav1.ConditionFalse, - Reason: hyperv1.MissingReleaseImagesReason, - Message: strings.Join(missingImages.UnsortedList(), ", "), - ObservedGeneration: hostedControlPlane.Generation, - }) - } - - if err := r.Client.Status().Patch(ctx, hostedControlPlane, client.MergeFromWithOptions(originalHostedControlPlane, client.MergeFromWithOptimisticLock{})); err != nil { + if err := statuspatching.PatchStatus(ctx, r.Client, hostedControlPlane, func() error { + if missingImages.Len() == 0 { + meta.SetStatusCondition(&hostedControlPlane.Status.Conditions, metav1.Condition{ + Type: string(hyperv1.ValidReleaseInfo), + Status: metav1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + Message: hyperv1.AllIsWellMessage, + ObservedGeneration: hostedControlPlane.Generation, + }) + } else { + meta.SetStatusCondition(&hostedControlPlane.Status.Conditions, metav1.Condition{ + Type: string(hyperv1.ValidReleaseInfo), + Status: metav1.ConditionFalse, + Reason: hyperv1.MissingReleaseImagesReason, + Message: strings.Join(sets.List(missingImages), ", "), + ObservedGeneration: hostedControlPlane.Generation, + }) + } + return nil + }); err != nil { errs = append(errs, fmt.Errorf("failed to update status: %w", err)) } @@ -2120,12 +2112,8 @@ func (r *HostedControlPlaneReconciler) reconcileValidIDPConfigurationCondition(c Message: fmt.Sprintf("failed to initialize identity providers: %v", err), } } - // Patch the condition on the HCP if it has changed - originalHCP := hcp.DeepCopy() - if meta.SetStatusCondition(&hcp.Status.Conditions, new) { - if err := r.Status().Patch(ctx, hcp, client.MergeFromWithOptions(originalHCP, client.MergeFromWithOptimisticLock{})); err != nil { - return fmt.Errorf("failed to patch valid IDP configuration condition: %w", err) - } + if err := statuspatching.PatchStatusCondition(ctx, r.Client, hcp, &hcp.Status.Conditions, new); err != nil { + return fmt.Errorf("failed to patch valid IDP configuration condition: %w", err) } return nil } @@ -2617,14 +2605,12 @@ func (r *HostedControlPlaneReconciler) removeCloudResources(ctx context.Context, if resourcesDestroyedCond != nil && resourcesDestroyedCond.Message != "" { message = fmt.Sprintf("%s (last status: %s)", message, resourcesDestroyedCond.Message) } - originalHCP := hcp.DeepCopy() - meta.SetStatusCondition(&hcp.Status.Conditions, metav1.Condition{ + if err := statuspatching.PatchStatusCondition(ctx, r.Client, hcp, &hcp.Status.Conditions, metav1.Condition{ Type: string(hyperv1.CloudResourcesDestroyed), Status: metav1.ConditionFalse, Reason: string(hyperv1.CloudResourcesDeletionTimedOutReason), Message: message, - }) - if err := r.Status().Patch(ctx, hcp, client.MergeFromWithOptions(originalHCP, client.MergeFromWithOptimisticLock{})); err != nil { + }); err != nil { return false, fmt.Errorf("failed to patch cloud resources destroyed condition: %w", err) } return true, nil @@ -2650,15 +2636,11 @@ func (r *HostedControlPlaneReconciler) removeCloudResources(ctx context.Context, return false, nil } if cvoScaledDownCond == nil || cvoScaledDownCond.Status != metav1.ConditionTrue { - originalHCP := hcp.DeepCopy() - cvoScaledDownCond = &metav1.Condition{ - Type: string(hyperv1.CVOScaledDown), - Status: metav1.ConditionTrue, - Reason: "CVOScaledDown", - LastTransitionTime: metav1.Now(), - } - meta.SetStatusCondition(&hcp.Status.Conditions, *cvoScaledDownCond) - if err := r.Status().Patch(ctx, hcp, client.MergeFromWithOptions(originalHCP, client.MergeFromWithOptimisticLock{})); err != nil { + if err := statuspatching.PatchStatusCondition(ctx, r.Client, hcp, &hcp.Status.Conditions, metav1.Condition{ + Type: string(hyperv1.CVOScaledDown), + Status: metav1.ConditionTrue, + Reason: "CVOScaledDown", + }); err != nil { return false, fmt.Errorf("failed to patch CVO scaled down condition: %w", err) } } @@ -2726,11 +2708,10 @@ func (r *HostedControlPlaneReconciler) reconcileDefaultSecurityGroup(ctx context return nil } - originalHCP := hcp.DeepCopy() - var condition *metav1.Condition + var condition metav1.Condition sgID, appliedTags, creationErr := createAWSDefaultSecurityGroup(ctx, r.ec2Client, hcp) if creationErr != nil { - condition = &metav1.Condition{ + condition = metav1.Condition{ Type: string(hyperv1.AWSDefaultSecurityGroupCreated), Status: metav1.ConditionFalse, Message: creationErr.Error(), @@ -2747,23 +2728,28 @@ func (r *HostedControlPlaneReconciler) reconcileDefaultSecurityGroup(ctx context }); err != nil { return fmt.Errorf("failed to update HostedControlPlane object: %w", err) } - originalHCP = hcp.DeepCopy() - condition = &metav1.Condition{ + condition = metav1.Condition{ Type: string(hyperv1.AWSDefaultSecurityGroupCreated), Status: metav1.ConditionTrue, Message: hyperv1.AllIsWellMessage, Reason: hyperv1.AsExpectedReason, } - hcp.Status.Platform = &hyperv1.PlatformStatus{ - AWS: &hyperv1.AWSPlatformStatus{ - DefaultWorkerSecurityGroupID: sgID, - }, - } } - meta.SetStatusCondition(&hcp.Status.Conditions, *condition) - if err := r.Client.Status().Patch(ctx, hcp, client.MergeFromWithOptions(originalHCP, client.MergeFromWithOptimisticLock{})); err != nil { + if err := statuspatching.PatchStatus(ctx, r.Client, hcp, func() error { + meta.SetStatusCondition(&hcp.Status.Conditions, condition) + if creationErr == nil { + if hcp.Status.Platform == nil { + hcp.Status.Platform = &hyperv1.PlatformStatus{} + } + if hcp.Status.Platform.AWS == nil { + hcp.Status.Platform.AWS = &hyperv1.AWSPlatformStatus{} + } + hcp.Status.Platform.AWS.DefaultWorkerSecurityGroupID = sgID + } + return nil + }); err != nil { return fmt.Errorf("failed to update status: %w", err) } diff --git a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go index 566db12edcc5..c3c1aff78390 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go +++ b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go @@ -4443,9 +4443,8 @@ func TestReconcileDeletion(t *testing.T) { ctx := ctrl.LoggerInto(t.Context(), ctrl.Log.WithName("test")) - // Re-read from fake client so the object has a ResourceVersion for OptimisticLock + // Re-read from fake client so the object has a ResourceVersion for PatchStatusCondition g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(hcp), hcp)).To(Succeed()) - originalHCP := hcp.DeepCopy() r := &HostedControlPlaneReconciler{ Client: fakeClient, @@ -4453,20 +4452,168 @@ func TestReconcileDeletion(t *testing.T) { ec2Client: mockEC2, } - _, err := r.reconcileDeletion(ctx, hcp, originalHCP) + _, err := r.reconcileDeletion(ctx, hcp) if tt.wantErr { g.Expect(err).To(HaveOccurred()) } else { g.Expect(err).ToNot(HaveOccurred()) } - cond := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.AWSDefaultSecurityGroupDeleted)) + // Re-read from server to verify persisted status. + updated := &hyperv1.HostedControlPlane{} + g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(hcp), updated)).To(Succeed()) + cond := meta.FindStatusCondition(updated.Status.Conditions, string(hyperv1.AWSDefaultSecurityGroupDeleted)) g.Expect(cond).ToNot(BeNil()) g.Expect(cond.Status).To(Equal(tt.wantCondStatus)) }) } } +func TestReconcileDefaultSecurityGroup(t *testing.T) { + tests := []struct { + name string + setupEC2Mock func(*gomock.Controller) *awsapi.MockEC2API + wantCondStatus metav1.ConditionStatus + wantCondReason string + wantPlatform bool + wantErr bool + }{ + { + name: "When creation fails it should set error condition via PatchStatus and not touch platform", + setupEC2Mock: func(mockCtrl *gomock.Controller) *awsapi.MockEC2API { + m := awsapi.NewMockEC2API(mockCtrl) + m.EXPECT().DescribeVpcs(gomock.Any(), gomock.Any()).Return(nil, + &smithy.GenericAPIError{Code: "VpcNotFound", Message: "vpc not found"}) + return m + }, + wantCondStatus: metav1.ConditionFalse, + wantCondReason: hyperv1.AWSErrorReason, + wantPlatform: false, + wantErr: true, + }, + { + name: "When identity provider is not ready it should skip without error", + setupEC2Mock: func(mockCtrl *gomock.Controller) *awsapi.MockEC2API { + return awsapi.NewMockEC2API(mockCtrl) + }, + wantErr: false, + }, + { + name: "When existing SG is found it should set success condition and platform status", + setupEC2Mock: func(mockCtrl *gomock.Controller) *awsapi.MockEC2API { + m := awsapi.NewMockEC2API(mockCtrl) + m.EXPECT().DescribeVpcs(gomock.Any(), gomock.Any()).Return(&ec2.DescribeVpcsOutput{ + Vpcs: []ec2types.Vpc{{VpcId: aws.String("vpc-123")}}, + }, nil) + m.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(&ec2.DescribeSecurityGroupsOutput{ + SecurityGroups: []ec2types.SecurityGroup{{ + GroupId: aws.String("sg-existing"), + OwnerId: aws.String("123456789012"), + Tags: []ec2types.Tag{ + {Key: aws.String("Name"), Value: aws.String("test-infra-default-sg")}, + }, + }}, + }, nil) + m.EXPECT().AuthorizeSecurityGroupIngress(gomock.Any(), gomock.Any()).Return( + &ec2.AuthorizeSecurityGroupIngressOutput{}, nil) + return m + }, + wantCondStatus: metav1.ConditionTrue, + wantCondReason: hyperv1.AsExpectedReason, + wantPlatform: true, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + mockCtrl := gomock.NewController(t) + mockEC2 := tt.setupEC2Mock(mockCtrl) + + conditions := []metav1.Condition{} + if tt.wantCondStatus != "" { + // Only add ValidAWSIdentityProvider=True for tests that should reach creation. + conditions = append(conditions, metav1.Condition{ + Type: string(hyperv1.ValidAWSIdentityProvider), + Status: metav1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + }) + } + + hcp := &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "test-ns", + }, + Spec: hyperv1.HostedControlPlaneSpec{ + InfraID: "test-infra", + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{ + CloudProviderConfig: &hyperv1.AWSCloudProviderConfig{ + VPC: "vpc-123", + }, + }, + }, + Networking: hyperv1.ClusterNetworking{ + MachineNetwork: []hyperv1.MachineNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.0.0.0/16")}, + }, + }, + }, + Status: hyperv1.HostedControlPlaneStatus{ + Conditions: conditions, + }, + } + fakeClient := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects(hcp). + WithStatusSubresource(&hyperv1.HostedControlPlane{}). + Build() + + ctx := ctrl.LoggerInto(t.Context(), ctrl.Log.WithName("test")) + g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(hcp), hcp)).To(Succeed()) + + r := &HostedControlPlaneReconciler{ + Client: fakeClient, + Log: ctrl.Log.WithName("test"), + ec2Client: mockEC2, + } + + err := r.reconcileDefaultSecurityGroup(ctx, hcp) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + + if tt.wantCondStatus == "" { + return + } + + // Re-read from server to verify persisted status. + updated := &hyperv1.HostedControlPlane{} + g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(hcp), updated)).To(Succeed()) + + cond := meta.FindStatusCondition(updated.Status.Conditions, string(hyperv1.AWSDefaultSecurityGroupCreated)) + g.Expect(cond).ToNot(BeNil(), "condition should be set") + g.Expect(cond.Status).To(Equal(tt.wantCondStatus)) + g.Expect(cond.Reason).To(Equal(tt.wantCondReason)) + + if tt.wantPlatform { + g.Expect(updated.Status.Platform).ToNot(BeNil()) + g.Expect(updated.Status.Platform.AWS).ToNot(BeNil()) + g.Expect(updated.Status.Platform.AWS.DefaultWorkerSecurityGroupID).ToNot(BeEmpty(), + "DefaultWorkerSecurityGroupID should be set on success") + } else { + g.Expect(updated.Status.Platform).To(BeNil(), + "platform status should remain nil when creation fails") + } + }) + } +} + func TestHealthCheckKASEndpoint(t *testing.T) { t.Parallel() tests := []struct { diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/reencryption/reencryption.go b/control-plane-operator/hostedclusterconfigoperator/controllers/reencryption/reencryption.go index d36c9b1e2a84..7456519ee34e 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/reencryption/reencryption.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/reencryption/reencryption.go @@ -14,6 +14,7 @@ import ( "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms" "github.com/openshift/hypershift/support/config" "github.com/openshift/hypershift/support/secretencryption" + "github.com/openshift/hypershift/support/statuspatching" "github.com/openshift/hypershift/support/util" "github.com/openshift/library-go/pkg/operator/encryption/controllers/migrators" @@ -62,24 +63,45 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco return reconcile.Result{}, fmt.Errorf("failed to get HCP: %w", err) } - originalHCP := hcp.DeepCopy() + // Snapshot pre-reconcile encryption state for metrics comparison. + previousEncryption := *hcp.Status.SecretEncryption.DeepCopy() + result, err := r.reconcile(ctx, log, hcp) if err != nil { return result, err } - if !equality.Semantic.DeepEqual(hcp.Status, originalHCP.Status) { - log.Info("Patching HCP status with secret encryption changes") - patch := crclient.MergeFrom(originalHCP) - if err := r.cpClient.Status().Patch(ctx, hcp, patch); err != nil { - return reconcile.Result{}, fmt.Errorf("failed to patch HCP status: %w", err) + // Capture desired status changes computed by reconcile(). + // Copy by value — PatchStatus re-fetches hcp, which replaces the backing slice. + desiredEncryption := *hcp.Status.SecretEncryption.DeepCopy() + var desiredCondition metav1.Condition + hasCondition := false + if c := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.EtcdDataEncryptionUpToDate)); c != nil { + desiredCondition = *c + hasCondition = true + } + + if patchErr := statuspatching.PatchStatus(ctx, r.cpClient, hcp, func() error { + hcp.Status.SecretEncryption = desiredEncryption + if hasCondition { + meta.SetStatusCondition(&hcp.Status.Conditions, desiredCondition) + } else { + meta.RemoveStatusCondition(&hcp.Status.Conditions, string(hyperv1.EtcdDataEncryptionUpToDate)) } - log.Info("Successfully patched HCP status") - recordMigrationState(r.hcpNamespace, r.hcpName, hcp.Status.SecretEncryption) - previousState := encryptionHistoryState(originalHCP.Status.SecretEncryption) - currentState := encryptionHistoryState(hcp.Status.SecretEncryption) + return nil + }); patchErr != nil { + return reconcile.Result{}, fmt.Errorf("failed to patch HCP status: %w", patchErr) + } + + // Always re-emit the current migration state gauge so it survives pod restarts. + recordMigrationState(r.hcpNamespace, r.hcpName, desiredEncryption) + + // Record duration only on actual state transitions. + if !equality.Semantic.DeepEqual(previousEncryption, desiredEncryption) { + previousState := encryptionHistoryState(previousEncryption) + currentState := encryptionHistoryState(desiredEncryption) if currentState == hyperv1.EncryptionMigrationStateCompleted && previousState != currentState { - recordMigrationDuration(r.hcpNamespace, r.hcpName, hcp.Status.SecretEncryption) + recordMigrationDuration(r.hcpNamespace, r.hcpName, desiredEncryption) } }