diff --git a/api/internal/controller/apisubscription_trusted_teams_test.go b/api/internal/controller/apisubscription_trusted_teams_test.go index 87842ffe..00791359 100644 --- a/api/internal/controller/apisubscription_trusted_teams_test.go +++ b/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) }) }) }) diff --git a/approval/api/v1/builder/builder.go b/approval/api/v1/builder/builder.go index 0b9ab775..0b836ac6 100644 --- a/approval/api/v1/builder/builder.go +++ b/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, diff --git a/approval/api/v1/builder/builder_test.go b/approval/api/v1/builder/builder_test.go index f6e09a12..15a7f38e 100644 --- a/approval/api/v1/builder/builder_test.go +++ b/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() {