diff --git a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go index 3dca5cdeee0e..3ec77f75adce 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)) } @@ -2103,6 +2095,12 @@ func (r *HostedControlPlaneReconciler) cleanupOldRedHatMarketplaceCatalogResourc func (r *HostedControlPlaneReconciler) reconcileValidIDPConfigurationCondition(ctx context.Context, hcp *hyperv1.HostedControlPlane, releaseImageProvider imageprovider.ReleaseImageProvider, oauthHost string, oauthPort int32) error { p := oauth.NewOAuthServerParams(hcp, releaseImageProvider, oauthHost, oauthPort, r.SetDefaultSecurityContext) + // The condition below is evaluated from the spec at this generation. If the spec + // changes before we patch, the evaluation is stale and must not be applied blindly. + // Note: p.OauthConfigOverrides is derived from IDP-override annotations, which don't + // bump Generation — an annotation-only change in this window isn't caught by this guard. + evaluatedGeneration := hcp.Generation + // Report any IDP configuration errors as a condition on the HCP new := metav1.Condition{ Type: string(hyperv1.ValidIDPConfiguration), @@ -2120,12 +2118,14 @@ 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.PatchStatus(ctx, r.Client, hcp, func() error { + if hcp.Generation != evaluatedGeneration { + return fmt.Errorf("hcp generation changed from %d to %d while evaluating IDP configuration, requeueing to re-evaluate", evaluatedGeneration, hcp.Generation) } + meta.SetStatusCondition(&hcp.Status.Conditions, new) + return nil + }); err != nil { + return fmt.Errorf("failed to patch valid IDP configuration condition: %w", err) } return nil } @@ -2636,18 +2636,25 @@ func (r *HostedControlPlaneReconciler) removeCloudResources(ctx context.Context, if timeElapsed > resourceDeletionTimeout { log.Info("Giving up on resource deletion after timeout", "timeElapsed", duration.ShortHumanDuration(timeElapsed)) - message := fmt.Sprintf("Giving up on cloud resource deletion after %s", duration.ShortHumanDuration(timeElapsed)) - 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{ - 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 { + if err := statuspatching.PatchStatus(ctx, r.Client, hcp, func() error { + // HCCO writes this same condition concurrently when it actually finishes + // destroying resources. Re-check the freshly fetched object so we don't + // clobber a concurrent True with a stale timeout. + if fresh := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.CloudResourcesDestroyed)); fresh != nil && fresh.Status == metav1.ConditionTrue { + return nil + } + message := fmt.Sprintf("Giving up on cloud resource deletion after %s", duration.ShortHumanDuration(timeElapsed)) + if resourcesDestroyedCond != nil && resourcesDestroyedCond.Message != "" { + message = fmt.Sprintf("%s (last status: %s)", message, resourcesDestroyedCond.Message) + } + meta.SetStatusCondition(&hcp.Status.Conditions, metav1.Condition{ + Type: string(hyperv1.CloudResourcesDestroyed), + Status: metav1.ConditionFalse, + Reason: string(hyperv1.CloudResourcesDeletionTimedOutReason), + Message: message, + }) + return nil + }); err != nil { return false, fmt.Errorf("failed to patch cloud resources destroyed condition: %w", err) } return true, nil @@ -2673,15 +2680,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) } } @@ -2749,11 +2752,14 @@ func (r *HostedControlPlaneReconciler) reconcileDefaultSecurityGroup(ctx context return nil } - originalHCP := hcp.DeepCopy() - var condition *metav1.Condition + // The security group result below is derived from the spec at this generation + // (VPC, machine CIDR). If the spec changes before we patch, that result is stale. + evaluatedGeneration := hcp.Generation + + 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(), @@ -2770,23 +2776,31 @@ 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 { + if hcp.Generation != evaluatedGeneration { + return fmt.Errorf("hcp generation changed from %d to %d while creating default security group, requeueing to re-evaluate", evaluatedGeneration, hcp.Generation) + } + 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 e4b729d7ba47..5c2fdc2d09c3 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go +++ b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go @@ -2686,12 +2686,13 @@ func TestRemoveCloudResources(t *testing.T) { testNamespace := "test-namespace" testCases := []struct { - name string - hcp *hyperv1.HostedControlPlane - cvoDeployment *appsv1.Deployment - expectedDone bool - expectedError bool - expectedCondition *metav1.Condition + name string + hcp *hyperv1.HostedControlPlane + cvoDeployment *appsv1.Deployment + expectedDone bool + expectedError bool + expectedCondition *metav1.Condition + simulateConcurrentDestroy bool }{ { name: "When CloudResourcesDestroyed is True, it should return done", @@ -2809,6 +2810,32 @@ func TestRemoveCloudResources(t *testing.T) { Reason: string(hyperv1.CloudResourcesDeletionTimedOutReason), }, }, + { + name: "When deletion has timed out but CloudResourcesDestroyed concurrently becomes True, it should not overwrite it", + hcp: &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: testNamespace, + }, + Status: hyperv1.HostedControlPlaneStatus{ + Conditions: []metav1.Condition{ + { + Type: string(hyperv1.CVOScaledDown), + Status: metav1.ConditionTrue, + Reason: "CVOScaledDown", + LastTransitionTime: metav1.NewTime(time.Now().Add(-15 * time.Minute)), + }, + }, + }, + }, + simulateConcurrentDestroy: true, + expectedDone: true, + expectedCondition: &metav1.Condition{ + Type: string(hyperv1.CloudResourcesDestroyed), + Status: metav1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + }, + }, { name: "When CVO is scaled down and deletion has not timed out, it should return not done", hcp: &hyperv1.HostedControlPlane{ @@ -2863,11 +2890,46 @@ func TestRemoveCloudResources(t *testing.T) { objs = append(objs, tc.cvoDeployment) } - c := fake.NewClientBuilder(). + clientBuilder := fake.NewClientBuilder(). WithScheme(api.Scheme). WithObjects(objs...). - WithStatusSubresource(&hyperv1.HostedControlPlane{}). - Build() + WithStatusSubresource(&hyperv1.HostedControlPlane{}) + + if tc.simulateConcurrentDestroy { + // Simulate HCCO concurrently setting CloudResourcesDestroyed=True between + // our initial read and PatchStatus's internal re-fetch. Only the first Get + // (PatchStatus's own fetch) injects and persists the concurrent write; later + // Gets — including this test's own verification read — must see real stored + // state, not a value re-injected on every call, or the assertion below would + // pass regardless of whether the production code actually overwrote it. + var injected bool + clientBuilder = clientBuilder.WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if err := c.Get(ctx, key, obj, opts...); err != nil { + return err + } + if injected { + return nil + } + hcpObj, ok := obj.(*hyperv1.HostedControlPlane) + if !ok { + return nil + } + meta.SetStatusCondition(&hcpObj.Status.Conditions, metav1.Condition{ + Type: string(hyperv1.CloudResourcesDestroyed), + Status: metav1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + }) + if err := c.Status().Update(ctx, hcpObj); err != nil { + return err + } + injected = true + return nil + }, + }) + } + + c := clientBuilder.Build() r := &HostedControlPlaneReconciler{ Client: c, @@ -2896,7 +2958,9 @@ func TestRemoveCloudResources(t *testing.T) { g.Expect(condition).ToNot(BeNil()) g.Expect(condition.Status).To(Equal(tc.expectedCondition.Status)) g.Expect(condition.Reason).To(Equal(tc.expectedCondition.Reason)) - g.Expect(condition.Message).To(ContainSubstring("Giving up on cloud resource deletion")) + if tc.expectedCondition.Reason == string(hyperv1.CloudResourcesDeletionTimedOutReason) { + g.Expect(condition.Message).To(ContainSubstring("Giving up on cloud resource deletion")) + } if originalCloudResourcesCond != nil && originalCloudResourcesCond.Message != "" && @@ -4443,9 +4507,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 +4516,348 @@ 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()) + } + + // Re-read from server to verify persisted status. + updated := &hyperv1.HostedControlPlane{} + g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(hcp), updated)).To(Succeed()) + + if tt.wantCondStatus == "" { + // Short-circuit paths (e.g. identity provider not ready) must not persist any status update. + cond := meta.FindStatusCondition(updated.Status.Conditions, string(hyperv1.AWSDefaultSecurityGroupCreated)) + g.Expect(cond).To(BeNil(), "condition should not be set when reconcile short-circuits") + g.Expect(updated.Status.Platform).To(BeNil(), "platform status should remain nil when reconcile short-circuits") + return + } + + 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 TestReconcileValidIDPConfigurationCondition(t *testing.T) { + t.Parallel() + const testNamespace = "test-ns" + + tests := []struct { + name string + hcp *hyperv1.HostedControlPlane + storedGeneration int64 + wantCondStatus metav1.ConditionStatus + wantCondReason string + wantErr bool + }{ + { + name: "When IDP configuration is valid, it should set the condition to True", + hcp: &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "test-hcp", Namespace: testNamespace, Generation: 1}, + }, + wantCondStatus: metav1.ConditionTrue, + wantCondReason: "IDPConfigurationValid", + }, + { + name: "When IDP configuration is invalid, it should set the condition to False", + hcp: &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "test-hcp", Namespace: testNamespace, Generation: 1}, + Spec: hyperv1.HostedControlPlaneSpec{ + Configuration: &hyperv1.ClusterConfiguration{ + OAuth: &configv1.OAuthSpec{ + IdentityProviders: []configv1.IdentityProvider{ + { + IdentityProviderConfig: configv1.IdentityProviderConfig{ + Type: configv1.IdentityProviderTypeHTPasswd, + // HTPasswd left nil to force a conversion error. + }, + }, + }, + }, + }, + }, + }, + wantCondStatus: metav1.ConditionFalse, + wantCondReason: "IDPConfigurationError", + }, + { + name: "When hcp generation changed since evaluation started, it should not patch and return an error", + hcp: &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "test-hcp", Namespace: testNamespace, Generation: 1}, + }, + // Simulates a spec update landing (e.g. from the HostedCluster controller) + // between when this hcp was read and when reconcileValidIDPConfigurationCondition runs. + storedGeneration: 2, + wantErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + stored := tc.hcp.DeepCopy() + if tc.storedGeneration != 0 { + stored.Generation = tc.storedGeneration + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects(stored). + WithStatusSubresource(&hyperv1.HostedControlPlane{}). + Build() + + r := &HostedControlPlaneReconciler{ + Client: fakeClient, + } + + ctx := t.Context() + err := r.reconcileValidIDPConfigurationCondition(ctx, tc.hcp, testutil.FakeImageProvider(), "oauth.example.com", 443) + + if tc.wantErr { + g.Expect(err).To(HaveOccurred()) + updated := &hyperv1.HostedControlPlane{} + g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(tc.hcp), updated)).To(Succeed()) + g.Expect(meta.FindStatusCondition(updated.Status.Conditions, string(hyperv1.ValidIDPConfiguration))).To(BeNil(), + "condition should not be patched when generation changed underneath us") + return + } + g.Expect(err).ToNot(HaveOccurred()) + + updated := &hyperv1.HostedControlPlane{} + g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(tc.hcp), updated)).To(Succeed()) + cond := meta.FindStatusCondition(updated.Status.Conditions, string(hyperv1.ValidIDPConfiguration)) + g.Expect(cond).ToNot(BeNil()) + g.Expect(cond.Status).To(Equal(tc.wantCondStatus)) + g.Expect(cond.Reason).To(Equal(tc.wantCondReason)) + }) + } +} + +func TestReconcileDefaultSecurityGroup_GenerationConflict(t *testing.T) { + g := NewWithT(t) + mockCtrl := gomock.NewController(t) + mockEC2 := awsapi.NewMockEC2API(mockCtrl) + mockEC2.EXPECT().DescribeVpcs(gomock.Any(), gomock.Any()).Return(&ec2.DescribeVpcsOutput{ + Vpcs: []ec2types.Vpc{{VpcId: aws.String("vpc-123")}}, + }, nil) + mockEC2.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) + mockEC2.EXPECT().AuthorizeSecurityGroupIngress(gomock.Any(), gomock.Any()).Return( + &ec2.AuthorizeSecurityGroupIngressOutput{}, nil) + + hcp := &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "test-ns", + Generation: 1, + }, + 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: []metav1.Condition{ + {Type: string(hyperv1.ValidAWSIdentityProvider), Status: metav1.ConditionTrue, Reason: hyperv1.AsExpectedReason}, + }, + }, + } + + // The object stored in the API server has already moved to a later generation + // (e.g. a spec update landed) than the in-memory hcp used to compute the AWS + // security-group result below. + stored := hcp.DeepCopy() + stored.Generation = 2 + + fakeClient := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects(stored). + WithStatusSubresource(&hyperv1.HostedControlPlane{}). + Build() + + ctx := ctrl.LoggerInto(t.Context(), ctrl.Log.WithName("test")) + + r := &HostedControlPlaneReconciler{ + Client: fakeClient, + Log: ctrl.Log.WithName("test"), + ec2Client: mockEC2, + } + + err := r.reconcileDefaultSecurityGroup(ctx, hcp) + g.Expect(err).To(HaveOccurred(), "stale generation should be rejected rather than patched") + + updated := &hyperv1.HostedControlPlane{} + g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(hcp), updated)).To(Succeed()) + g.Expect(meta.FindStatusCondition(updated.Status.Conditions, string(hyperv1.AWSDefaultSecurityGroupCreated))).To(BeNil(), + "condition should not be patched when generation changed underneath us") + g.Expect(updated.Status.Platform).To(BeNil(), + "platform status should not be patched when generation changed underneath us") +} + 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..23191b9697ee 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/reencryption/reencryption.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/reencryption/reencryption.go @@ -14,13 +14,13 @@ 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" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -62,25 +62,40 @@ 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() - result, err := r.reconcile(ctx, log, hcp) + // Snapshot pre-reconcile encryption state for metrics comparison. + previousEncryption := *hcp.Status.SecretEncryption.DeepCopy() + + // Reconcile against a copy — PatchStatus below re-fetches hcp from the server, + // which would otherwise overwrite the in-memory mutations reconcile() makes. + workingCopy := hcp.DeepCopy() + result, err := r.reconcile(ctx, log, workingCopy) 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) - } - log.Info("Successfully patched HCP status") - recordMigrationState(r.hcpNamespace, r.hcpName, hcp.Status.SecretEncryption) - previousState := encryptionHistoryState(originalHCP.Status.SecretEncryption) - currentState := encryptionHistoryState(hcp.Status.SecretEncryption) - if currentState == hyperv1.EncryptionMigrationStateCompleted && previousState != currentState { - recordMigrationDuration(r.hcpNamespace, r.hcpName, hcp.Status.SecretEncryption) - } + // hcp.Status.SecretEncryption and the EtcdDataEncryptionUpToDate condition are + // written exclusively by this reconciler for this HCP — no other controller + // touches them, and controller-runtime serializes reconciles per object key, so + // re-applying the workingCopy snapshot on a PatchStatus retry is safe: there is + // no concurrent writer whose update could be clobbered. If either field ever + // gains another writer, this blind re-apply would need to recompute from the + // freshly fetched hcp instead. + if patchErr := statuspatching.PatchStatus(ctx, r.cpClient, hcp, func() error { + hcp.Status.SecretEncryption = workingCopy.Status.SecretEncryption + statuspatching.SyncCondition(workingCopy.Status.Conditions, &hcp.Status.Conditions, string(hyperv1.EtcdDataEncryptionUpToDate)) + 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, workingCopy.Status.SecretEncryption) + + // Record duration only on actual state transitions. + previousState := encryptionHistoryState(previousEncryption) + currentState := encryptionHistoryState(workingCopy.Status.SecretEncryption) + if currentState == hyperv1.EncryptionMigrationStateCompleted && previousState != currentState { + recordMigrationDuration(r.hcpNamespace, r.hcpName, workingCopy.Status.SecretEncryption) } return result, nil diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/reencryption/reencryption_test.go b/control-plane-operator/hostedclusterconfigoperator/controllers/reencryption/reencryption_test.go index 4c97d4e9a82a..f103e21cb7f9 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/reencryption/reencryption_test.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/reencryption/reencryption_test.go @@ -33,6 +33,7 @@ import ( "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/reconcile" ) @@ -963,6 +964,38 @@ func TestReconcile(t *testing.T) { } } +func TestReconcile_PatchStatusError(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // A stale TargetKey with encryption unconfigured guarantees reconcile() produces + // a real status change (handleEncryptionNotConfigured clears it), so PatchStatus + // attempts an actual Status().Patch call rather than a no-op skip. + hcp := newHCP(withTargetKey(aescbcKeyStatus("aescbc-key-1", "stale-hash"))) + + cpClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(hcp, convergedKASDeployment(testNamespace)). + WithStatusSubresource(&hyperv1.HostedControlPlane{}). + WithInterceptorFuncs(interceptor.Funcs{ + SubResourcePatch: func(ctx context.Context, c client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { + if subResourceName == "status" { + return fmt.Errorf("simulated status patch failure") + } + return c.SubResource(subResourceName).Patch(ctx, obj, patch, opts...) + }, + }). + Build() + + r := newReconciler(cpClient, nil, newFakeMigrator()) + + _, err := r.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{Namespace: testNamespace, Name: testHCPName}, + }) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to patch HCP status")) +} + func TestParseGroupResource(t *testing.T) { tests := []struct { name string diff --git a/support/statuspatching/statuspatching.go b/support/statuspatching/statuspatching.go index 674c7fbf9f9f..c1f6f1327fcd 100644 --- a/support/statuspatching/statuspatching.go +++ b/support/statuspatching/statuspatching.go @@ -58,3 +58,16 @@ func PatchStatusCondition(ctx context.Context, c client.Client, obj client.Objec return c.Status().Patch(ctx, obj, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})) }) } + +// SyncCondition copies a single condition by type from src to dst: if present in +// src it's set on dst, if absent it's removed from dst. Useful when a caller +// computes status on a working copy (to avoid PatchStatus's re-fetch clobbering +// in-memory mutations) and needs to carry a specific condition back into the +// live object's mutate callback. +func SyncCondition(src []metav1.Condition, dst *[]metav1.Condition, conditionType string) { + if c := meta.FindStatusCondition(src, conditionType); c != nil { + meta.SetStatusCondition(dst, *c) + } else { + meta.RemoveStatusCondition(dst, conditionType) + } +} diff --git a/support/statuspatching/statuspatching_test.go b/support/statuspatching/statuspatching_test.go index de3cf7a64282..2b36c713968f 100644 --- a/support/statuspatching/statuspatching_test.go +++ b/support/statuspatching/statuspatching_test.go @@ -443,3 +443,76 @@ func TestPatchStatusCondition_GetFailure(t *testing.T) { g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) g.Expect(recorder.called).To(BeFalse()) } + +func TestSyncCondition(t *testing.T) { + tests := []struct { + name string + src []metav1.Condition + dst []metav1.Condition + want []metav1.Condition + }{ + { + name: "When the condition is present in src, it should be set on dst", + src: []metav1.Condition{ + {Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}, + }, + dst: nil, + want: []metav1.Condition{ + {Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}, + }, + }, + { + name: "When the condition is present in both src and dst, it should overwrite dst", + src: []metav1.Condition{ + {Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}, + }, + dst: []metav1.Condition{ + {Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotReady"}, + }, + want: []metav1.Condition{ + {Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}, + }, + }, + { + name: "When the condition is absent from src, it should be removed from dst", + src: nil, + dst: []metav1.Condition{ + {Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}, + }, + want: []metav1.Condition{}, + }, + { + name: "When the condition is absent from both src and dst, it should leave dst unchanged", + src: nil, + dst: nil, + want: nil, + }, + { + name: "When src has other condition types, it should leave dst's unrelated conditions untouched", + src: []metav1.Condition{ + {Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}, + }, + dst: []metav1.Condition{ + {Type: "Other", Status: metav1.ConditionFalse, Reason: "Unrelated"}, + }, + want: []metav1.Condition{ + {Type: "Other", Status: metav1.ConditionFalse, Reason: "Unrelated"}, + {Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + dst := tc.dst + SyncCondition(tc.src, &dst, "Ready") + + // SetStatusCondition stamps LastTransitionTime, so compare everything else. + for i := range dst { + dst[i].LastTransitionTime = metav1.Time{} + } + g.Expect(dst).To(Equal(tc.want)) + }) + } +}