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: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>

… denied 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>

ron96g · 2026-05-21T06:47:57Z

 	ApprovalStateGranted     ApprovalState = "Granted"
 	ApprovalStateRejected    ApprovalState = "Rejected"
 	ApprovalStateSuspended   ApprovalState = "Suspended"
-	ApprovalStateExpired     ApprovalState = "Expired"


Why remove the State?

ron96g · 2026-05-21T06:50:24Z

+}
+
+// LoadExpirationConfig loads the expiration configuration from environment variables
+func LoadExpirationConfig() (*ExpirationConfig, error) {


Will this clash with our current controller-config? I think we should either re-use the stuff there or create a new local Viper-Instance here, but mixing them could result in bugs

ron96g · 2026-05-21T06:52:27Z

+	var thresholds []reminder.Threshold
+
+	// Weekly reminder threshold (if configured)
+	if weeklyReminderMonths > 0 {


Why configure it like this and not via a object? I think Viper is capable of loading complex data like JSON-objects and lists... Maybe check this.

ron96g · 2026-05-21T06:53:57Z

+
+// eventsRecorderAdapter adapts events.EventRecorder (new API) to record.EventRecorder (old API)
+// for compatibility with common controller until it's migrated to the new API.
+type eventsRecorderAdapter struct {


Why is this here? I mean, I see the comment but for the time being we can just use both the old and new API until we can fully migrate common to the new one.

ron96g · 2026-05-21T06:54:40Z

 //
 // SPDX-License-Identifier: Apache-2.0

-package controller //nolint:dupl // approval and approvalrequest controllers are intentionally similar


Not sure about removing this but if the linter does not complain, fine by me

ron96g · 2026-05-21T07:04:06Z

+
+	ae := &approvalv1.ApprovalExpiration{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      fmt.Sprintf("%s--expiration", approval.Name),


[minor] suffix not really needed as CR kind already is called ApprovalExpiration

ron96g · 2026-05-21T07:06:59Z

 	Actor                  Actor
 	Action                 string
+	// Expiration-specific fields
+	ExpirationDate string


Why not split it to keep it more clear what is present in which scenario

ron96g · 2026-05-21T07:07:55Z

+	purposeStringBuilder.WriteString("reminder")
+	purposeStringBuilder.WriteString(DELIMITER)
+	purposeStringBuilder.WriteString(string(data.Actor))
+	purpose := purposeStringBuilder.String()


Is there already a template for this?

ron96g · 2026-05-21T07:10:07Z

+	nameStringBuilder.WriteString(data.Target.GetName())
+	name := nameStringBuilder.String()
+
+	notificationBuilder := builder.New().


A bit of a controversial thing but can we actually send the same Notification to both teams? Like "hey teams, you share a subscription which is about to expire. As team-provider please re-grant" or something?

ron96g · 2026-05-21T07:10:42Z


 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
-func (a *ApprovalCustomValidator) ValidateUpdate(_ context.Context, oldObj, newObj *approvalv1.Approval) (warnings admission.Warnings, err error) {
+func (a *ApprovalCustomValidator) ValidateUpdate(ctx context.Context, oldObj, newObj *approvalv1.Approval) (warnings admission.Warnings, err error) {


If linter does not care, I also do not care

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 4 commits May 7, 2026 11:38

Merge branch 'main' into feat/approval-expiry

9809a99

ron96g reviewed May 21, 2026

View reviewed changes

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!

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!

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!

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