feat(approval-expiration): approvals can now expire and notifications are sent before they do by julius-malcovsky · Pull Request #387 · telekom/controlplane

julius-malcovsky · 2026-05-04T13:55:53Z

Notification templates will be added in a separate request

… are sent before they do Co-authored-by: Björn Kottner <BjoernKarma@users.noreply.github.com> Co-authored-by: Ismael Garba <iagarba@users.noreply.github.com> Co-authored-by: Stefan Siber <stefan-ctrl@users.noreply.github.com> Co-authored-by: Ron Gummich <ron96g@users.noreply.github.com>

Copilot

Pull request overview

This PR introduces an approval-expiration feature by adding an Expired approval state, a new ApprovalExpiration CRD/controller to drive expiration, and reminder-notification plumbing so expiring approvals can notify stakeholders ahead of time.

Changes:

Add ApprovalExpiration CRD + controller/handler to track expiration dates, trigger expiration, and send reminders.
Extend the Approval FSM/API types to support Expired and a system-only Expire action (plus re-approval from Expired).
Add notification placeholders + a reminder notification sender, and introduce an env-driven expiration config loader.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
`approval/internal/webhook/v1/approval_webhook.go`	Adds webhook validation intended to block manual transitions to `Expired`.
`approval/internal/webhook/v1/approval_webhook_test.go`	Adds tests around the new “system-only expire” validation behavior.
`approval/internal/handler/util/notification.go`	Adds reminder-specific placeholders, fields, and a `SendReminderNotification` helper.
`approval/internal/handler/approvalexpiration/handler.go`	Implements the core expiration/reminder logic for `ApprovalExpiration` objects.
`approval/internal/handler/approvalexpiration/handler_test.go`	Unit tests for reminder eligibility and next-reconcile scheduling logic.
`approval/internal/handler/approval/handler.go`	Integrates expiration lifecycle management into the Approval handler and hides system-only transitions from status.
`approval/internal/handler/approval/fsm.go`	Extends FSM transitions to include `Expire` and allow actions from `Expired`.
`approval/internal/controller/suite_test.go`	Registers the new `ApprovalExpirationReconciler` in the controller test suite.
`approval/internal/controller/approvalexpiration_controller.go`	Adds the new controller reconciler wiring and RBAC markers.
`approval/internal/controller/approvalexpiration_controller_test.go`	Integration tests for Approval↔ApprovalExpiration lifecycle behavior.
`approval/internal/config/expiration.go`	Adds env-backed expiration/reminder configuration loading.
`approval/internal/condition/condition.go`	Adds an “Expired” condition helper.
`approval/config/rbac/role.yaml`	Grants RBAC permissions for `approvalexpirations` resources.
`approval/config/crd/bases/approval.cp.ei.telekom.de_approvals.yaml`	Updates the Approval CRD enum to include `Expired`.
`approval/config/crd/bases/approval.cp.ei.telekom.de_approvalexpirations.yaml`	Introduces the new `ApprovalExpiration` CRD manifest.
`approval/cmd/main.go`	Loads expiration config and registers the new controller.
`approval/api/v1/zz_generated.deepcopy.go`	Adds autogenerated deepcopy functions for `ApprovalExpiration` types.
`approval/api/v1/common_types.go`	Adds the `Expire` action constant (and related type support).
`approval/api/v1/builder/builder.go`	Treats `Expired` approvals as non-provisionable (denied) during build decisions.
`approval/api/v1/approvalexpiration_types.go`	Adds API types for the new `ApprovalExpiration` resource.
`approval/api/v1/approval_types.go`	Extends kubebuilder validation enums to include the `Expired` state.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ron96g · 2026-05-05T08:50:06Z

+// validateExpireTransition blocks manual transitions to EXPIRED state (system-only action)
+func validateExpireTransition(newObj *approvalv1.Approval) error {
+	if newObj.Spec.State != approvalv1.ApprovalStateExpired {
+		return nil
+	}
+
+	// Check if this is a legitimate system transition
+	for _, decision := range newObj.Spec.Decisions {
+		if decision.Name == "System" && decision.ResultingState == approvalv1.ApprovalStateExpired {
+			return nil // Valid system transition
+		}
+	}
+
+	return apierrors.NewBadRequest("Expire action is system-only and cannot be triggered manually")


Good point by AI ;)

lukas016 · 2026-05-04T21:53:16Z

+		daysRemaining = 0
+	case now.After(ae.Spec.DailyReminder.Time):
+		reminderType = "daily"
+		daysRemaining = int64(ae.Spec.Expiration.Time.Sub(now).Hours() / 24)


optional: this looks as something what can be in "utils" package.

ron96g

Looks solid. Just a few questions/open points.

Add some diagram that explains the logic in more details? Maybe inside of the README similiar to the other digrams there.

ron96g · 2026-05-05T08:36:02Z

 	if approvalExists {
 		log.V(2).Info("Approval exists")
-		isDenied := b.Approval.Spec.State == v1.ApprovalStateRejected || b.Approval.Spec.State == v1.ApprovalStateSuspended
+		isDenied := b.Approval.Spec.State == v1.ApprovalStateRejected ||


I know that you explained it in the Review but is it really the goal to enforce this? Currently its just purely informational

not sure I understand the question :/ if we dont treat an expired one as denied wouldnt it result in access not being revoked ? maybe we can discuss it ...

Yes, but that is how its currently works. Its a toothless tiger - purely informational. So maybe it could be a policy like

enum ApprovalExpirationPolicy { IGNORE REJECT }

or something like this? And the default would be IGNORE

ron96g · 2026-05-05T08:36:37Z

+	Expiration metav1.Time `json:"expiration"`
+
+	// WeeklyReminder is the date from which weekly reminders start
+	WeeklyReminder metav1.Time `json:"weeklyReminder"`


Can we use the reminder logic in common for this? That was implemented for graceful-client-secret-rotation?

ron96g · 2026-05-05T08:39:35Z

+		LastWeeksWithDailyReminder:   viper.GetInt("EXPIRATION_DAILY_REMINDER_WEEKS"),
+	}
+
+	if config.ExpirationPeriodMonths <= 0 {


This could be done using https://github.com/go-playground/validator, however, the bigger question here is, can we integrate this into the common-config logic? The story to align config-management is still in the backlog, but can we already think about this?

ron96g · 2026-05-05T08:41:39Z

+
+// SetupWithManager sets up the controller with the Manager.
+func (r *ApprovalExpirationReconciler) SetupWithManager(mgr ctrl.Manager) error {
+	r.Recorder = mgr.GetEventRecorderFor("approvalexpiration-controller")


This API is deprecated and we must migrate at some point. Maybe already use the new one for newer controllers? Dont forget about the kubebuilder:rbac annotation

ron96g · 2026-05-05T08:46:20Z

+
+// filterSystemActions removes system-only actions (Expire) from the available transitions
+func filterSystemActions(transitions approvalv1.AvailableTransitions) approvalv1.AvailableTransitions {
+	filtered := make(approvalv1.AvailableTransitions, 0, len(transitions))


This could allocate +1 slots if Expire is not added

ron96g · 2026-05-05T08:47:42Z

+func NewExpiredCondition() metav1.Condition {
+	return metav1.Condition{
+		Type:    "Approved",
+		Status:  metav1.ConditionTrue,


Is this correct?

ron96g · 2026-05-05T08:49:39Z

+
+	// Check if this is a legitimate system transition
+	for _, decision := range newObj.Spec.Decisions {
+		if decision.Name == "System" && decision.ResultingState == approvalv1.ApprovalStateExpired {


Should "System" be a const somewhere?

ron96g · 2026-05-05T08:50:06Z

+// validateExpireTransition blocks manual transitions to EXPIRED state (system-only action)
+func validateExpireTransition(newObj *approvalv1.Approval) error {
+	if newObj.Spec.State != approvalv1.ApprovalStateExpired {
+		return nil
+	}
+
+	// Check if this is a legitimate system transition
+	for _, decision := range newObj.Spec.Decisions {
+		if decision.Name == "System" && decision.ResultingState == approvalv1.ApprovalStateExpired {
+			return nil // Valid system transition
+		}
+	}
+
+	return apierrors.NewBadRequest("Expire action is system-only and cannot be triggered manually")


Good point by AI ;)

Co-authored-by: Björn Kottner <BjoernKarma@users.noreply.github.com> Co-authored-by: Ismael Garba <iagarba@users.noreply.github.com> Co-authored-by: Stefan Siber <stefan-ctrl@users.noreply.github.com> Co-authored-by: Ron Gummich <ron96g@users.noreply.github.com>

Copilot AI review requested due to automatic review settings May 4, 2026 13:55

Copilot started reviewing on behalf of julius-malcovsky May 4, 2026 13:56 View session

fix(approval-expiry): add new CR to kustomization

d979b38

Copilot AI reviewed May 4, 2026

View reviewed changes

lukas016 reviewed May 4, 2026

View reviewed changes

Comment thread approval/internal/handler/util/notification.go

lukas016 reviewed May 4, 2026

View reviewed changes

Comment thread approval/internal/handler/util/notification.go

lukas016 reviewed May 4, 2026

View reviewed changes

ron96g requested changes May 5, 2026

View reviewed changes

Merge branch 'main' into feat/approval-expiry

837d5fe

ron96g assigned julius-malcovsky May 7, 2026

julius-malcovsky and others added 2 commits May 7, 2026 11:38

Merge branch 'main' into feat/approval-expiry

9809a99

Conversation

julius-malcovsky commented May 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ron96g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants