feat(approval): implement FourEyes approval strategy with Semigranted state#299
Open
feat(approval): implement FourEyes approval strategy with Semigranted state#299
Conversation
… state Add the FourEyes dual-approval workflow requiring two distinct approvers before an ApprovalRequest is granted, using an intermediate Semigranted state. Key changes: - Add Semigranted to Approval/ApprovalRequest state enums (CRD + types) - Extend Decision struct with Timestamp and ResultingState fields to align with the controlplane-api GraphQL model - Add Semigranted condition and handler logic for both ApprovalRequest and Approval controllers (skip requester notification on Semigranted) - Enforce distinct-decider webhook validation for FourEyes Semigranted->Granted transitions (case-insensitive email comparison) - Require at least one decision for any non-Auto state change - Auto-approve strategy now records a system-generated Decision entry - Add comprehensive tests for FourEyes lifecycle, webhook validation, and builder behavior - Add design doc, implementation plan, and FourEyes sample CR
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements the FourEyes dual-approval workflow in the approval/ domain by introducing an intermediate Semigranted state, extending decision/audit data, and enforcing the distinct-approver rule via admission webhooks.
Changes:
- Add Semigranted state handling across CRD schemas, handlers/controllers, and reconciliation conditions (including requester-notification behavior).
- Extend
DecisionwithtimestampandresultingState, and record a system-generated decision for Auto approvals. - Add/extend validation and controller/webhook tests plus supporting design/plan docs and a sample FourEyes ApprovalRequest.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| approval/internal/webhook/v1/approvalrequest_webhook_test.go | Adds tests for auto-decision defaulting, decision-required updates, and distinct-decider validation helper. |
| approval/internal/webhook/v1/approvalrequest_webhook.go | Adds auto-decision defaulting, decision-required update validation, and distinct-decider enforcement for FourEyes Semigranted→Granted. |
| approval/internal/webhook/v1/approval_webhook.go | Mirrors decision-required + distinct-decider validation logic for Approval resources. |
| approval/internal/handler/approvalrequest/handler.go | Adds Semigranted reconciliation branch and suppresses requester notifications for Semigranted. |
| approval/internal/handler/approval/handler.go | Adds Semigranted reconciliation branch and condition semantics for Approval. |
| approval/internal/controller/approvalrequest_controller_test.go | Expands controller integration tests for Simple and FourEyes flows (including notifications and decision propagation). |
| approval/internal/controller/approval_controller_test.go | Adds reconciliation test coverage for Semigranted Approval state. |
| approval/internal/condition/condition.go | Adds NewSemigrantedCondition() for Approved=False with Semigranted reason. |
| approval/docs/four_eyes/plan.md | Adds implementation plan for Semigranted + FourEyes enforcement and roll-out steps. |
| approval/docs/four_eyes/design.md | Adds design doc describing Semigranted semantics, enforcement points, and risks. |
| approval/config/samples/approval_v1_approvalrequest_foureyes.yaml | Adds a sample ApprovalRequest CR for FourEyes strategy. |
| approval/config/crd/bases/approval.cp.ei.telekom.de_approvals.yaml | Regenerates CRD schema (Semigranted enum + Decision timestamp/resultingState). |
| approval/config/crd/bases/approval.cp.ei.telekom.de_approvalrequests.yaml | Regenerates CRD schema (Decision timestamp/resultingState). |
| approval/api/v1/zz_generated.deepcopy.go | Updates deepcopy generation to properly deep-copy Decision.Timestamp. |
| approval/api/v1/common_types.go | Adds AutoApprovedComment and extends Decision with Timestamp + ResultingState. |
| approval/api/v1/builder/builder_test.go | Updates builder tests to assert the system auto-decision is recorded. |
| approval/api/v1/builder/builder.go | Ensures Auto strategy writes a system decision (deduped) into spec.decisions. |
| approval/api/v1/approval_types.go | Adds Semigranted to Approval state/lastState enum validations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace subscription-specific wording in AutoApprovedComment with a generic message, since approvals can apply to any resource type.
- Fix StateChanged() using stale Status.LastState by comparing oldObj.Spec.State != newObj.Spec.State in both webhooks - Fix validateDistinctDeciders empty-email bypass by requiring non-empty trimmed emails; move to shared validation_helpers.go - Replace hasAutoApprovedDecision string-matching with len check in both webhooks and builder - Suppress requester notification on Semigranted in Approval handler - Make Decision.ResultingState required at CRD level; add defaultDecisionFields webhook helper that auto-populates Timestamp and ResultingState on every decision - Add comprehensive webhook unit tests for defaulting, validation, distinct-deciders, empty-email, and stateChanged behavior - Update all Decision literals in controller integration tests to include ResultingState for CRD schema compliance
…h operations
Default the Decisions field to an empty array [] on both Approval and
ApprovalRequest resources, so JSON patch operations like
`add /spec/decisions/- {...}` work without first needing to create the
array.
Changes:
- Add +kubebuilder:default={} marker and remove omitempty from Decisions
field in both ApprovalSpec and ApprovalRequestSpec
- Initialize nil Decisions to empty slice in defaulting webhooks as a
safety net for existing resources
- Regenerate CRD manifests with default: [] on decisions field
ron96g
requested review from
BjoernKarma,
iagarba,
julius-malcovsky and
stefan-ctrl
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
approval/config/crd/bases/approval.cp.ei.telekom.de_approvalrequests.yaml
Show resolved
Hide resolved
…l ApprovalRequests Replace the Status.AvailableTransitions-based FSM check in both Approval and ApprovalRequest webhooks with on-the-fly computation from the canonical FSM definitions. This fixes two bugs: stale status fields could allow invalid transitions (Bug 1), and nil status fields skipped the FSM check entirely (Bug 2). Additionally: - Broaden the FourEyes distinct-decider check to trigger on ANY transition to Granted, not only Semigranted -> Granted (Bug 3) - Allow re-approval from Rejected state in Simple and FourEyes FSMs - Block all spec changes on Granted ApprovalRequests (terminal state freeze); Rejected ARs remain mutable for re-approval - Add isTerminalApprovalRequestState helper - Add comprehensive webhook tests covering all three bug fixes
…resources Add 6 labels (target.kind, target.name, requester.team, decider.team, action, approval.strategy) to enable discovering ApprovalRequest and Approval resources without knowing their exact hashed names. Labels are set via a shared SetApprovalLabels helper: - On ApprovalRequest in the builder's Build() mutate function, after the trusted-team strategy override so labels reflect effective values - On Approval in the ApprovalRequest handler's handleGranted(), when the Approval is created from a granted request
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add the FourEyes dual-approval workflow requiring two distinct approvers
before an ApprovalRequest is granted, using an intermediate Semigranted
state.
Key changes:
align with the controlplane-api GraphQL model
and Approval controllers (skip requester notification on Semigranted)
Semigranted->Granted transitions (case-insensitive email comparison)
and builder behavior