fix(approval): preserve state and decisions when updating ApprovalRequest spec fields

api/internal/controller/apisubscription_trusted_teams_test.go

@@ -219,17 +219,19 @@ var _ = Describe("ApiSubscription Controller with Trusted Teams", Ordered, func(
 					updatedExposure.Spec.Approval.TrustedTeams[1],
 				}
 				g.Expect(teamNames).To(ConsistOf("group1--team1", "group3--team3"))
-			}, timeout*3, interval).Should(Succeed())
 
-			By("Checking that subscriptions were reprocessed after trusted teams update")
-			// Team1 should remain auto-approved
-			verifyApprovalStrategy(team1Sub, approvalapi.ApprovalStrategyAuto)
+				By("Checking that subscriptions were reprocessed after trusted teams update")
+				// Team1 should remain auto-approved
+				verifyApprovalStrategy(team1Sub, approvalapi.ApprovalStrategyAuto)
 
-			// Team2 should stay approved
-			verifyApprovalStrategy(team2Sub, approvalapi.ApprovalStrategyAuto)
+				// Team2 should stay approved
+				verifyApprovalStrategy(team2Sub, approvalapi.ApprovalStrategyAuto)
+
+				// Team3 should now be auto-approved
+				verifyApprovalStrategy(team3Sub, approvalapi.ApprovalStrategyAuto)
+
+			}, timeout*3, interval).Should(Succeed())
 
-			// Team3 should now be auto-approved
-			verifyApprovalStrategy(team3Sub, approvalapi.ApprovalStrategyAuto)
 		})
 	})
 })

approval/api/v1/builder/builder.go

@@ -154,19 +154,38 @@ func (b *approvalBuilder) Build(ctx context.Context) (finalResult ApprovalResult
 
 	approvalReq := b.Request.DeepCopy()
 	mutate := func() error {
-		if approvalReq.Spec.State != "" && approvalReq.Spec.State != v1.ApprovalStatePending {
-			return nil // no need to create approval request as it already exists
-		}
+		// Preserve the server-side state and decisions before applying spec updates.
+		// After controllerutil.CreateOrUpdate fetches the existing object, approvalReq
+		// contains the server-side values. We must not overwrite the state set by a
+		// decider, nor lose any recorded decisions.
+		currentState := approvalReq.Spec.State
+		currentStrategy := approvalReq.Spec.Strategy
+		currentDecisions := approvalReq.Spec.Decisions
 
 		if err := controllerutil.SetControllerReference(b.Owner, approvalReq, b.Client.Scheme()); err != nil {
 			return errors.Wrap(err, "failed to set controller reference")
 		}
 
+		// Apply desired spec fields from the builder
+		approvalReq.Spec.Target = b.Request.Spec.Target
+		approvalReq.Spec.Requester = b.Request.Spec.Requester
+		approvalReq.Spec.Decider = b.Request.Spec.Decider
+		approvalReq.Spec.Action = b.Request.Spec.Action
+		approvalReq.Spec.Strategy = b.Request.Spec.Strategy
+
 		if b.isRequesterFromTrustedRequesters() {
 			approvalReq.Spec.Strategy = v1.ApprovalStrategyAuto
 		}
 
-		if approvalReq.Spec.Strategy == v1.ApprovalStrategyAuto {
+		// Restore decisions — these are managed by the decider, not the owner
+		approvalReq.Spec.Decisions = currentDecisions
+
+		// Preserve the decider's decision and strategy once a request is decided.
+		// Only pending/new requests should be recalculated from desired inputs.
+		if currentState != "" && currentState != v1.ApprovalStatePending {
+			approvalReq.Spec.State = currentState
+			approvalReq.Spec.Strategy = currentStrategy
+		} else if approvalReq.Spec.Strategy == v1.ApprovalStrategyAuto {
 			approvalReq.Spec.State = v1.ApprovalStateGranted
 			if len(approvalReq.Spec.Decisions) == 0 {
 				approvalReq.Spec.Decisions = append(approvalReq.Spec.Decisions, v1.Decision{
@@ -175,6 +194,8 @@ func (b *approvalBuilder) Build(ctx context.Context) (finalResult ApprovalResult
 					ResultingState: v1.ApprovalStateGranted,
 				})
 			}
+		} else {
+			approvalReq.Spec.State = v1.ApprovalStatePending
 		}
 
 		v1.SetApprovalLabels(approvalReq, approvalReq.Spec.Target,

approval/api/v1/builder/builder_test.go

@@ -146,6 +146,82 @@ var _ = Describe("Approval Builder", Ordered, func() {
 		})
 	})
 
+	Context("Spec fields update on granted ApprovalRequest", func() {
+
+		It("should update spec fields while preserving state and decisions", func() {
+			By("creating the initial ApprovalRequest via the builder")
+
+			err := requester.SetProperties(properties)
+			Expect(err).NotTo(HaveOccurred())
+
+			jclient := cclient.NewJanitorClient(cclient.NewScopedClient(k8sm.GetClient(), testEnvironment))
+
+			owner := test.NewObject("apisub", testNamespace)
+			owner.SetUID(types.UID("99d819b2-7dcb-41dd-abac-415719674737"))
+			owner.SetLabels(map[string]string{
+				config.EnvironmentLabelKey: testEnvironment,
+			})
+
+			builder := NewApprovalBuilder(jclient, owner)
+			builder.WithHashValue(requester.Properties)
+			builder.WithRequester(requester)
+			builder.WithStrategy(approvalv1.ApprovalStrategySimple)
+
+			res, err := builder.Build(ctx)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(res).To(Equal(ApprovalResultPending))
+
+			arKey := client.ObjectKey{
+				Name:      builder.GetApprovalRequest().Name,
+				Namespace: testNamespace,
+			}
+
+			By("simulating a decider granting the ApprovalRequest with decisions")
+			ar := &approvalv1.ApprovalRequest{}
+			err = k8sClient.Get(ctx, arKey, ar)
+			Expect(err).ToNot(HaveOccurred())
+
+			ar.Spec.State = approvalv1.ApprovalStateGranted
+			ar.Spec.Decisions = []approvalv1.Decision{
+				{Name: "John Doe", Email: "john.doe@telekom.de", Comment: "Approved!", ResultingState: approvalv1.ApprovalStateGranted},
+			}
+			err = k8sClient.Update(ctx, ar)
+			Expect(err).ToNot(HaveOccurred())
+
+			By("running the builder again with updated requester reason")
+			updatedRequester := &approvalv1.Requester{
+				TeamName:  "Max",
+				TeamEmail: "max.mustermann@telekom.de",
+				Reason:    "Updated reason for access",
+			}
+			err = updatedRequester.SetProperties(properties)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Need a fresh builder since it can only run once
+			builder2 := NewApprovalBuilder(jclient, owner)
+			builder2.WithHashValue(updatedRequester.Properties) // same hash so it targets the same AR
+			builder2.WithRequester(updatedRequester)
+			builder2.WithStrategy(approvalv1.ApprovalStrategySimple)
+
+			res, err = builder2.Build(ctx)
+			Expect(err).NotTo(HaveOccurred())
+
+			By("verifying spec fields are updated, state and decisions preserved")
+			updatedAR := &approvalv1.ApprovalRequest{}
+			err = k8sClient.Get(ctx, arKey, updatedAR)
+			Expect(err).ToNot(HaveOccurred())
+
+			// Spec fields should be updated
+			Expect(updatedAR.Spec.Requester.Reason).To(Equal("Updated reason for access"))
+			// State should be preserved (Granted, not reset to Pending)
+			Expect(updatedAR.Spec.State).To(Equal(approvalv1.ApprovalStateGranted))
+			// Decisions should be preserved
+			Expect(updatedAR.Spec.Decisions).To(HaveLen(1))
+			Expect(updatedAR.Spec.Decisions[0].Name).To(Equal("John Doe"))
+			Expect(updatedAR.Spec.Decisions[0].Comment).To(Equal("Approved!"))
+		})
+	})
+
 	Context("Required Requester missing", func() {
 
 		It("should fail creating the resources", func() {