From 866cdd6eecb4d8b429d65bfb1f52da89512a441c Mon Sep 17 00:00:00 2001 From: Julius Malcovsky Date: Mon, 4 May 2026 12:47:13 +0200 Subject: [PATCH 1/5] feat(approval-expiration): approvals can now expire and notifications are sent before they do MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Björn Kottner Co-authored-by: Ismael Garba Co-authored-by: Stefan Siber Co-authored-by: Ron Gummich --- approval/api/v1/approval_types.go | 4 +- approval/api/v1/approvalexpiration_types.go | 81 ++++++ approval/api/v1/builder/builder.go | 8 +- approval/api/v1/common_types.go | 1 + approval/api/v1/zz_generated.deepcopy.go | 108 ++++++++ approval/cmd/main.go | 20 ++ ....cp.ei.telekom.de_approvalexpirations.yaml | 182 +++++++++++++ .../approval.cp.ei.telekom.de_approvals.yaml | 2 + approval/config/rbac/role.yaml | 3 + approval/internal/condition/condition.go | 9 + approval/internal/config/expiration.go | 54 ++++ .../approvalexpiration_controller.go | 58 ++++ .../approvalexpiration_controller_test.go | 246 +++++++++++++++++ approval/internal/controller/suite_test.go | 15 ++ approval/internal/handler/approval/fsm.go | 14 +- approval/internal/handler/approval/handler.go | 141 +++++++++- .../handler/approvalexpiration/handler.go | 247 ++++++++++++++++++ .../approvalexpiration/handler_test.go | 237 +++++++++++++++++ .../internal/handler/util/notification.go | 76 +++++- .../internal/webhook/v1/approval_webhook.go | 20 ++ .../webhook/v1/approval_webhook_test.go | 76 ++++++ 21 files changed, 1588 insertions(+), 14 deletions(-) create mode 100644 approval/api/v1/approvalexpiration_types.go create mode 100644 approval/config/crd/bases/approval.cp.ei.telekom.de_approvalexpirations.yaml create mode 100644 approval/internal/config/expiration.go create mode 100644 approval/internal/controller/approvalexpiration_controller.go create mode 100644 approval/internal/controller/approvalexpiration_controller_test.go create mode 100644 approval/internal/handler/approvalexpiration/handler.go create mode 100644 approval/internal/handler/approvalexpiration/handler_test.go diff --git a/approval/api/v1/approval_types.go b/approval/api/v1/approval_types.go index d1aacf373..76b26297d 100644 --- a/approval/api/v1/approval_types.go +++ b/approval/api/v1/approval_types.go @@ -37,7 +37,7 @@ type ApprovalSpec struct { Strategy ApprovalStrategy `json:"strategy"` // State defines the state of the approval - // +kubebuilder:validation:Enum=Pending;Semigranted;Granted;Rejected;Suspended + // +kubebuilder:validation:Enum=Pending;Semigranted;Granted;Rejected;Suspended;Expired // +kubebuilder:default=Pending State ApprovalState `json:"state"` @@ -57,7 +57,7 @@ type ApprovalStatus struct { AvailableTransitions AvailableTransitions `json:"availableTransitions,omitempty"` // LastState defines the last state of the approval - // +kubebuilder:validation:Enum=Pending;Semigranted;Granted;Rejected;Suspended + // +kubebuilder:validation:Enum=Pending;Semigranted;Granted;Rejected;Suspended;Expired // +kubebuilder:default=Pending LastState ApprovalState `json:"lastState,omitempty"` diff --git a/approval/api/v1/approvalexpiration_types.go b/approval/api/v1/approvalexpiration_types.go new file mode 100644 index 000000000..3a6a83ceb --- /dev/null +++ b/approval/api/v1/approvalexpiration_types.go @@ -0,0 +1,81 @@ +// Copyright 2025 Deutsche Telekom IT GmbH +// +// SPDX-License-Identifier: Apache-2.0 + +package v1 + +import ( + "github.com/telekom/controlplane/common/pkg/types" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// ApprovalExpirationSpec defines the desired state of ApprovalExpiration +type ApprovalExpirationSpec struct { + // Approval is a reference to the parent Approval resource + Approval types.ObjectRef `json:"approval"` + + // Expiration is the absolute date when the approval expires + Expiration metav1.Time `json:"expiration"` + + // WeeklyReminder is the date from which weekly reminders start + WeeklyReminder metav1.Time `json:"weeklyReminder"` + + // DailyReminder is the date from which daily reminders start + DailyReminder metav1.Time `json:"dailyReminder"` +} + +// ApprovalExpirationStatus defines the observed state of ApprovalExpiration +type ApprovalExpirationStatus struct { + // +listType=map + // +listMapKey=type + // +patchStrategy=merge + // +patchMergeKey=type + // +optional + Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"` + + // LastReminder is the timestamp when the last reminder was sent + // +optional + LastReminder *metav1.Time `json:"lastReminder,omitempty"` + + // LastNotificationRef is a reference to the last sent reminder notification + // +optional + LastNotificationRef *types.ObjectRef `json:"lastNotificationRef,omitempty"` +} + +// +kubebuilder:object:root=true +// +kubebuilder:subresource:status + +// ApprovalExpiration is the Schema for the approvalexpirations API +// +kubebuilder:printcolumn:name="Approval",type="string",JSONPath=".spec.approval.name",description="The parent Approval" +// +kubebuilder:printcolumn:name="Expiration",type="date",JSONPath=".spec.expiration",description="When the approval expires" +type ApprovalExpiration struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + + Spec ApprovalExpirationSpec `json:"spec,omitempty"` + Status ApprovalExpirationStatus `json:"status,omitempty"` +} + +// +kubebuilder:object:root=true + +// ApprovalExpirationList contains a list of ApprovalExpiration +type ApprovalExpirationList struct { + metav1.TypeMeta `json:",inline"` + metav1.ListMeta `json:"metadata,omitempty"` + Items []ApprovalExpiration `json:"items"` +} + +// GetConditions returns the conditions of the ApprovalExpiration +func (ae *ApprovalExpiration) GetConditions() []metav1.Condition { + return ae.Status.Conditions +} + +// SetCondition sets the condition of the ApprovalExpiration +func (ae *ApprovalExpiration) SetCondition(condition metav1.Condition) bool { + return meta.SetStatusCondition(&ae.Status.Conditions, condition) +} + +func init() { + SchemeBuilder.Register(&ApprovalExpiration{}, &ApprovalExpirationList{}) +} diff --git a/approval/api/v1/builder/builder.go b/approval/api/v1/builder/builder.go index 27596a9b0..a6e303fa6 100644 --- a/approval/api/v1/builder/builder.go +++ b/approval/api/v1/builder/builder.go @@ -241,11 +241,13 @@ func (b *approvalBuilder) Build(ctx context.Context) (finalResult ApprovalResult // Approval was found 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 || + b.Approval.Spec.State == v1.ApprovalStateSuspended || + b.Approval.Spec.State == v1.ApprovalStateExpired if isDenied { - log.V(1).Info("Approval is rejected or suspended and must not be provisioned") - b.Owner.SetCondition(newApprovalGrantedCondition(b.Approval.Spec.State, "Approval has been rejected or suspended")) + log.V(1).Info("Approval is rejected, suspended, or expired and must not be provisioned") + b.Owner.SetCondition(newApprovalGrantedCondition(b.Approval.Spec.State, "Approval has been rejected, suspended, or expired")) return ApprovalResultDenied, nil } } diff --git a/approval/api/v1/common_types.go b/approval/api/v1/common_types.go index 61e23c201..004744493 100644 --- a/approval/api/v1/common_types.go +++ b/approval/api/v1/common_types.go @@ -32,6 +32,7 @@ const ( ApprovalActionDeny ApprovalAction = "Deny" ApprovalActionSuspend ApprovalAction = "Suspend" ApprovalActionResume ApprovalAction = "Resume" + ApprovalActionExpire ApprovalAction = "Expire" ) func (a ApprovalAction) String() string { diff --git a/approval/api/v1/zz_generated.deepcopy.go b/approval/api/v1/zz_generated.deepcopy.go index 81991ad0e..692db05c5 100644 --- a/approval/api/v1/zz_generated.deepcopy.go +++ b/approval/api/v1/zz_generated.deepcopy.go @@ -41,6 +41,114 @@ func (in *Approval) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ApprovalExpiration) DeepCopyInto(out *ApprovalExpiration) { + *out = *in + out.TypeMeta = in.TypeMeta + in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) + in.Spec.DeepCopyInto(&out.Spec) + in.Status.DeepCopyInto(&out.Status) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ApprovalExpiration. +func (in *ApprovalExpiration) DeepCopy() *ApprovalExpiration { + if in == nil { + return nil + } + out := new(ApprovalExpiration) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *ApprovalExpiration) DeepCopyObject() runtime.Object { + if c := in.DeepCopy(); c != nil { + return c + } + return nil +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ApprovalExpirationList) DeepCopyInto(out *ApprovalExpirationList) { + *out = *in + out.TypeMeta = in.TypeMeta + in.ListMeta.DeepCopyInto(&out.ListMeta) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]ApprovalExpiration, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ApprovalExpirationList. +func (in *ApprovalExpirationList) DeepCopy() *ApprovalExpirationList { + if in == nil { + return nil + } + out := new(ApprovalExpirationList) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *ApprovalExpirationList) DeepCopyObject() runtime.Object { + if c := in.DeepCopy(); c != nil { + return c + } + return nil +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ApprovalExpirationSpec) DeepCopyInto(out *ApprovalExpirationSpec) { + *out = *in + in.Approval.DeepCopyInto(&out.Approval) + in.Expiration.DeepCopyInto(&out.Expiration) + in.WeeklyReminder.DeepCopyInto(&out.WeeklyReminder) + in.DailyReminder.DeepCopyInto(&out.DailyReminder) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ApprovalExpirationSpec. +func (in *ApprovalExpirationSpec) DeepCopy() *ApprovalExpirationSpec { + if in == nil { + return nil + } + out := new(ApprovalExpirationSpec) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ApprovalExpirationStatus) DeepCopyInto(out *ApprovalExpirationStatus) { + *out = *in + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]metav1.Condition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } + if in.LastReminder != nil { + in, out := &in.LastReminder, &out.LastReminder + *out = (*in).DeepCopy() + } + if in.LastNotificationRef != nil { + in, out := &in.LastNotificationRef, &out.LastNotificationRef + *out = (*in).DeepCopy() + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ApprovalExpirationStatus. +func (in *ApprovalExpirationStatus) DeepCopy() *ApprovalExpirationStatus { + if in == nil { + return nil + } + out := new(ApprovalExpirationStatus) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ApprovalList) DeepCopyInto(out *ApprovalList) { *out = *in diff --git a/approval/cmd/main.go b/approval/cmd/main.go index b0809a05b..59f7cb6a3 100644 --- a/approval/cmd/main.go +++ b/approval/cmd/main.go @@ -22,6 +22,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" approvalv1 "github.com/telekom/controlplane/approval/api/v1" + "github.com/telekom/controlplane/approval/internal/config" "github.com/telekom/controlplane/approval/internal/controller" webhookv1 "github.com/telekom/controlplane/approval/internal/webhook/v1" notificationv1 "github.com/telekom/controlplane/notification/api/v1" @@ -79,6 +80,17 @@ func main() { ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts))) + // Load expiration configuration + expirationConfig, err := config.LoadExpirationConfig() + if err != nil { + setupLog.Error(err, "unable to load expiration config") + os.Exit(1) + } + setupLog.Info("loaded expiration config", + "expirationPeriodMonths", expirationConfig.ExpirationPeriodMonths, + "weeklyReminderMonths", expirationConfig.LastMonthsWithWeeklyReminder, + "dailyReminderWeeks", expirationConfig.LastWeeksWithDailyReminder) + // if the enable-http2 flag is false (the default), http/2 should be disabled // due to its vulnerabilities. More specifically, disabling http/2 will // prevent from being vulnerable to the HTTP/2 Stream Cancellation and @@ -135,6 +147,14 @@ func main() { setupLog.Error(err, "unable to create controller", "controller", "ApprovalRequest") os.Exit(1) } + if err = (&controller.ApprovalExpirationReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + ExpirationConfig: expirationConfig, + }).SetupWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create controller", "controller", "ApprovalExpiration") + os.Exit(1) + } if os.Getenv("ENABLE_WEBHOOKS") != "false" { if err = webhookv1.SetupApprovalRequestWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "ApprovalRequest") diff --git a/approval/config/crd/bases/approval.cp.ei.telekom.de_approvalexpirations.yaml b/approval/config/crd/bases/approval.cp.ei.telekom.de_approvalexpirations.yaml new file mode 100644 index 000000000..8dd42d055 --- /dev/null +++ b/approval/config/crd/bases/approval.cp.ei.telekom.de_approvalexpirations.yaml @@ -0,0 +1,182 @@ +# SPDX-FileCopyrightText: 2025 Deutsche Telekom IT GmbH +# +# SPDX-License-Identifier: Apache-2.0 +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.20.1 + name: approvalexpirations.approval.cp.ei.telekom.de +spec: + group: approval.cp.ei.telekom.de + names: + kind: ApprovalExpiration + listKind: ApprovalExpirationList + plural: approvalexpirations + singular: approvalexpiration + scope: Namespaced + versions: + - additionalPrinterColumns: + - description: The parent Approval + jsonPath: .spec.approval.name + name: Approval + type: string + - description: When the approval expires + jsonPath: .spec.expiration + name: Expiration + type: date + name: v1 + schema: + openAPIV3Schema: + description: ApprovalExpiration is the Schema for the approvalexpirations + API + properties: + apiVersion: + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources + type: string + kind: + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds + type: string + metadata: + type: object + spec: + description: ApprovalExpirationSpec defines the desired state of ApprovalExpiration + properties: + approval: + description: Approval is a reference to the parent Approval resource + properties: + name: + type: string + namespace: + type: string + uid: + description: |- + UID is a type that holds unique ID values, including UUIDs. Because we + don't ONLY use UUIDs, this is an alias to string. Being a type captures + intent and helps make sure that UIDs and names do not get conflated. + type: string + required: + - name + - namespace + type: object + dailyReminder: + description: DailyReminder is the date from which daily reminders + start + format: date-time + type: string + expiration: + description: Expiration is the absolute date when the approval expires + format: date-time + type: string + weeklyReminder: + description: WeeklyReminder is the date from which weekly reminders + start + format: date-time + type: string + required: + - approval + - dailyReminder + - expiration + - weeklyReminder + type: object + status: + description: ApprovalExpirationStatus defines the observed state of ApprovalExpiration + properties: + conditions: + items: + description: Condition contains details for one aspect of the current + state of this API Resource. + properties: + lastTransitionTime: + description: |- + lastTransitionTime is the last time the condition transitioned from one status to another. + This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: |- + message is a human readable message indicating details about the transition. + This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: |- + observedGeneration represents the .metadata.generation that the condition was set based upon. + For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date + with respect to the current state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: |- + reason contains a programmatic identifier indicating the reason for the condition's last transition. + Producers of specific condition types may define expected values and meanings for this field, + and whether the values are considered a guaranteed API. + The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map + lastNotificationRef: + description: LastNotificationRef is a reference to the last sent reminder + notification + properties: + name: + type: string + namespace: + type: string + uid: + description: |- + UID is a type that holds unique ID values, including UUIDs. Because we + don't ONLY use UUIDs, this is an alias to string. Being a type captures + intent and helps make sure that UIDs and names do not get conflated. + type: string + required: + - name + - namespace + type: object + lastReminder: + description: LastReminder is the timestamp when the last reminder + was sent + format: date-time + type: string + type: object + type: object + served: true + storage: true + subresources: + status: {} diff --git a/approval/config/crd/bases/approval.cp.ei.telekom.de_approvals.yaml b/approval/config/crd/bases/approval.cp.ei.telekom.de_approvals.yaml index d87b8ac54..5db4cabe7 100644 --- a/approval/config/crd/bases/approval.cp.ei.telekom.de_approvals.yaml +++ b/approval/config/crd/bases/approval.cp.ei.telekom.de_approvals.yaml @@ -222,6 +222,7 @@ spec: - Granted - Rejected - Suspended + - Expired type: string strategy: default: Auto @@ -356,6 +357,7 @@ spec: - Granted - Rejected - Suspended + - Expired type: string notificationRefs: description: NotificationRefs is a reference to the notifications diff --git a/approval/config/rbac/role.yaml b/approval/config/rbac/role.yaml index c92f518d7..4063e8718 100644 --- a/approval/config/rbac/role.yaml +++ b/approval/config/rbac/role.yaml @@ -17,6 +17,7 @@ rules: - apiGroups: - approval.cp.ei.telekom.de resources: + - approvalexpirations - approvalrequests - approvals verbs: @@ -30,6 +31,7 @@ rules: - apiGroups: - approval.cp.ei.telekom.de resources: + - approvalexpirations/finalizers - approvalrequests/finalizers - approvals/finalizers verbs: @@ -37,6 +39,7 @@ rules: - apiGroups: - approval.cp.ei.telekom.de resources: + - approvalexpirations/status - approvalrequests/status - approvals/status verbs: diff --git a/approval/internal/condition/condition.go b/approval/internal/condition/condition.go index 6c7c28762..d87b61ff8 100644 --- a/approval/internal/condition/condition.go +++ b/approval/internal/condition/condition.go @@ -54,3 +54,12 @@ func NewSemigrantedCondition() metav1.Condition { Message: "Request has been partially approved, awaiting second approval", } } + +func NewExpiredCondition() metav1.Condition { + return metav1.Condition{ + Type: "Approved", + Status: metav1.ConditionTrue, + Reason: "Expired", + Message: "Request has expired and requires re-approval", + } +} diff --git a/approval/internal/config/expiration.go b/approval/internal/config/expiration.go new file mode 100644 index 000000000..266eec1d2 --- /dev/null +++ b/approval/internal/config/expiration.go @@ -0,0 +1,54 @@ +// Copyright 2025 Deutsche Telekom IT GmbH +// +// SPDX-License-Identifier: Apache-2.0 + +package config + +import ( + "github.com/pkg/errors" + "github.com/spf13/viper" +) + +// ExpirationConfig holds the configuration for approval expiration +type ExpirationConfig struct { + // ExpirationPeriodMonths is the number of months until an approval expires + ExpirationPeriodMonths int + + // LastMonthsWithWeeklyReminder is the number of months before expiration when weekly reminders start + LastMonthsWithWeeklyReminder int + + // LastWeeksWithDailyReminder is the number of weeks before expiration when daily reminders start + LastWeeksWithDailyReminder int +} + +// LoadExpirationConfig loads the expiration configuration from environment variables +func LoadExpirationConfig() (*ExpirationConfig, error) { + setExpirationConfigDefaults() + + viper.AutomaticEnv() + viper.SetEnvPrefix("APPROVAL") + + config := &ExpirationConfig{ + ExpirationPeriodMonths: viper.GetInt("EXPIRATION_PERIOD_MONTHS"), + LastMonthsWithWeeklyReminder: viper.GetInt("EXPIRATION_WEEKLY_REMINDER_MONTHS"), + LastWeeksWithDailyReminder: viper.GetInt("EXPIRATION_DAILY_REMINDER_WEEKS"), + } + + if config.ExpirationPeriodMonths <= 0 { + return nil, errors.New("APPROVAL_EXPIRATION_PERIOD_MONTHS must be greater than 0") + } + if config.LastMonthsWithWeeklyReminder < 0 { + return nil, errors.New("APPROVAL_EXPIRATION_WEEKLY_REMINDER_MONTHS must be >= 0") + } + if config.LastWeeksWithDailyReminder < 0 { + return nil, errors.New("APPROVAL_EXPIRATION_DAILY_REMINDER_WEEKS must be >= 0") + } + + return config, nil +} + +func setExpirationConfigDefaults() { + viper.SetDefault("EXPIRATION_PERIOD_MONTHS", 12) + viper.SetDefault("EXPIRATION_WEEKLY_REMINDER_MONTHS", 2) + viper.SetDefault("EXPIRATION_DAILY_REMINDER_WEEKS", 2) +} diff --git a/approval/internal/controller/approvalexpiration_controller.go b/approval/internal/controller/approvalexpiration_controller.go new file mode 100644 index 000000000..6eb565a29 --- /dev/null +++ b/approval/internal/controller/approvalexpiration_controller.go @@ -0,0 +1,58 @@ +// Copyright 2025 Deutsche Telekom IT GmbH +// +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "context" + + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/record" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" + + approvalv1 "github.com/telekom/controlplane/approval/api/v1" + "github.com/telekom/controlplane/approval/internal/config" + "github.com/telekom/controlplane/approval/internal/handler/approvalexpiration" + cconfig "github.com/telekom/controlplane/common/pkg/config" + cc "github.com/telekom/controlplane/common/pkg/controller" +) + +// ApprovalExpirationReconciler reconciles an ApprovalExpiration object +type ApprovalExpirationReconciler struct { + client.Client + Scheme *runtime.Scheme + Recorder record.EventRecorder + ExpirationConfig *config.ExpirationConfig + + cc.Controller[*approvalv1.ApprovalExpiration] +} + +// +kubebuilder:rbac:groups=approval.cp.ei.telekom.de,resources=approvalexpirations,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=approval.cp.ei.telekom.de,resources=approvalexpirations/status,verbs=get;update;patch +// +kubebuilder:rbac:groups=approval.cp.ei.telekom.de,resources=approvalexpirations/finalizers,verbs=update +// +kubebuilder:rbac:groups=approval.cp.ei.telekom.de,resources=approvals,verbs=get;list;watch;update;patch +// +kubebuilder:rbac:groups=notification.cp.ei.telekom.de,resources=notifications,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=notification.cp.ei.telekom.de,resources=notificationchannels,verbs=get;list;watch + +// Reconcile is part of the main kubernetes reconciliation loop +func (r *ApprovalExpirationReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + return r.Controller.Reconcile(ctx, req, &approvalv1.ApprovalExpiration{}) +} + +// SetupWithManager sets up the controller with the Manager. +func (r *ApprovalExpirationReconciler) SetupWithManager(mgr ctrl.Manager) error { + r.Recorder = mgr.GetEventRecorderFor("approvalexpiration-controller") + handler := approvalexpiration.NewHandler(r.Client, r.ExpirationConfig) + r.Controller = cc.NewController(handler, r.Client, r.Recorder) + + return ctrl.NewControllerManagedBy(mgr). + For(&approvalv1.ApprovalExpiration{}). + WithOptions(controller.Options{ + MaxConcurrentReconciles: cconfig.MaxConcurrentReconciles, + RateLimiter: cc.NewRateLimiter(), + }). + Complete(r) +} diff --git a/approval/internal/controller/approvalexpiration_controller_test.go b/approval/internal/controller/approvalexpiration_controller_test.go new file mode 100644 index 000000000..5e1070f82 --- /dev/null +++ b/approval/internal/controller/approvalexpiration_controller_test.go @@ -0,0 +1,246 @@ +// Copyright 2025 Deutsche Telekom IT GmbH +// +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "time" + + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + approvalv1 "github.com/telekom/controlplane/approval/api/v1" + "github.com/telekom/controlplane/common/pkg/condition" + commonconfig "github.com/telekom/controlplane/common/pkg/config" + ctypes "github.com/telekom/controlplane/common/pkg/types" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("ApprovalExpiration Controller", Ordered, func() { + const resourceName = "test-expiration-approval" + + typeNamespacedName := types.NamespacedName{ + Name: resourceName, + Namespace: testNamespace, + } + + decider := approvalv1.Decider{ + TeamName: "test--decider", + TeamEmail: "test@decider.com", + ApplicationRef: &ctypes.TypedObjectRef{ + TypeMeta: metav1.TypeMeta{}, + ObjectRef: ctypes.ObjectRef{ + Name: "decider-app-name", + Namespace: testNamespace, + UID: "", + }, + }, + } + + requester := approvalv1.Requester{ + TeamName: "test--requester", + TeamEmail: "max.mustermann@telekom.de", + Reason: "I need access to this API", + ApplicationRef: &ctypes.TypedObjectRef{ + TypeMeta: metav1.TypeMeta{}, + ObjectRef: ctypes.ObjectRef{ + Name: "requester-app-name", + Namespace: testNamespace, + UID: "", + }, + }, + } + + resource := ctypes.TypedObjectRef{ + TypeMeta: metav1.TypeMeta{ + Kind: "Subscription", + }, + ObjectRef: ctypes.ObjectRef{ + Name: resourceName, + Namespace: testNamespace, + }, + } + + BeforeAll(func() { + By("creating the Approval in GRANTED state") + approval := &approvalv1.Approval{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "approval.cp.ei.telekom.de/v1", + Kind: "Approval", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: resourceName, + Namespace: testNamespace, + Labels: map[string]string{ + commonconfig.EnvironmentLabelKey: testEnvironment, + }, + }, + Spec: approvalv1.ApprovalSpec{ + Strategy: approvalv1.ApprovalStrategySimple, + State: approvalv1.ApprovalStateGranted, + Requester: requester, + Target: resource, + Decider: decider, + Action: "subscribe", + Decisions: []approvalv1.Decision{ + { + Name: "Alice", + Email: "alice@telekom.de", + Comment: "Approved", + ResultingState: approvalv1.ApprovalStateGranted, + }, + }, + }, + } + Expect(k8sClient.Create(ctx, approval)).To(Succeed()) + + Eventually(func(g Gomega) { + err := k8sClient.Get(ctx, typeNamespacedName, approval) + g.Expect(err).NotTo(HaveOccurred()) + readyCondition := meta.FindStatusCondition(approval.Status.Conditions, condition.ConditionTypeReady) + g.Expect(readyCondition).ToNot(BeNil()) + g.Expect(readyCondition.Status).To(Equal(metav1.ConditionTrue)) + }, timeout, interval).Should(Succeed()) + }) + + AfterAll(func() { + By("cleaning up test resources") + approval := &approvalv1.Approval{} + err := k8sClient.Get(ctx, typeNamespacedName, approval) + if err == nil { + Expect(k8sClient.Delete(ctx, approval)).To(Succeed()) + } + + expirationName := types.NamespacedName{ + Name: resourceName + "--expiration", + Namespace: testNamespace, + } + ae := &approvalv1.ApprovalExpiration{} + err = k8sClient.Get(ctx, expirationName, ae) + if err == nil { + Expect(k8sClient.Delete(ctx, ae)).To(Succeed()) + } + }) + + It("should create ApprovalExpiration when Approval is GRANTED", func() { + expirationName := types.NamespacedName{ + Name: resourceName + "--expiration", + Namespace: testNamespace, + } + + Eventually(func(g Gomega) { + ae := &approvalv1.ApprovalExpiration{} + err := k8sClient.Get(ctx, expirationName, ae) + g.Expect(err).NotTo(HaveOccurred()) + + By("checking ApprovalExpiration has correct owner reference") + g.Expect(ae.OwnerReferences).To(HaveLen(1)) + g.Expect(ae.OwnerReferences[0].Name).To(Equal(resourceName)) + g.Expect(ae.OwnerReferences[0].Kind).To(Equal("Approval")) + g.Expect(*ae.OwnerReferences[0].Controller).To(BeTrue()) + + By("checking ApprovalExpiration has expiration dates set") + g.Expect(ae.Spec.Expiration.Time).To(BeTemporally(">", time.Now())) + g.Expect(ae.Spec.WeeklyReminder.Time).To(BeTemporally("<", ae.Spec.Expiration.Time)) + g.Expect(ae.Spec.DailyReminder.Time).To(BeTemporally("<", ae.Spec.Expiration.Time)) + g.Expect(ae.Spec.DailyReminder.Time).To(BeTemporally(">", ae.Spec.WeeklyReminder.Time)) + + By("checking ApprovalExpiration references the Approval") + g.Expect(ae.Spec.Approval.Name).To(Equal(resourceName)) + g.Expect(ae.Spec.Approval.Namespace).To(Equal(testNamespace)) + }, timeout, interval).Should(Succeed()) + }) + + It("should delete ApprovalExpiration when Approval transitions to REJECTED", func() { + By("transitioning Approval to REJECTED") + approval := &approvalv1.Approval{} + Expect(k8sClient.Get(ctx, typeNamespacedName, approval)).To(Succeed()) + + approval.Spec.State = approvalv1.ApprovalStateRejected + approval.Spec.Decisions = append(approval.Spec.Decisions, approvalv1.Decision{ + Name: "Bob", + Email: "bob@telekom.de", + Comment: "Rejected", + ResultingState: approvalv1.ApprovalStateRejected, + }) + Expect(k8sClient.Update(ctx, approval)).To(Succeed()) + + expirationName := types.NamespacedName{ + Name: resourceName + "--expiration", + Namespace: testNamespace, + } + + By("checking ApprovalExpiration is deleted") + Eventually(func(g Gomega) { + ae := &approvalv1.ApprovalExpiration{} + err := k8sClient.Get(ctx, expirationName, ae) + g.Expect(errors.IsNotFound(err)).To(BeTrue()) + }, timeout, interval).Should(Succeed()) + }) + + It("should recreate ApprovalExpiration when Approval transitions back to GRANTED", func() { + By("transitioning Approval back to GRANTED") + approval := &approvalv1.Approval{} + Expect(k8sClient.Get(ctx, typeNamespacedName, approval)).To(Succeed()) + + approval.Spec.State = approvalv1.ApprovalStateGranted + approval.Spec.Decisions = append(approval.Spec.Decisions, approvalv1.Decision{ + Name: "Charlie", + Email: "charlie@telekom.de", + Comment: "Re-approved", + ResultingState: approvalv1.ApprovalStateGranted, + }) + Expect(k8sClient.Update(ctx, approval)).To(Succeed()) + + expirationName := types.NamespacedName{ + Name: resourceName + "--expiration", + Namespace: testNamespace, + } + + By("checking ApprovalExpiration is recreated") + Eventually(func(g Gomega) { + ae := &approvalv1.ApprovalExpiration{} + err := k8sClient.Get(ctx, expirationName, ae) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(ae.Spec.Expiration.Time).To(BeTemporally(">", time.Now())) + }, timeout, interval).Should(Succeed()) + }) + + It("should keep ApprovalExpiration when Approval transitions to SUSPENDED", func() { + expirationName := types.NamespacedName{ + Name: resourceName + "--expiration", + Namespace: testNamespace, + } + + By("getting current expiration time before SUSPENDED") + ae := &approvalv1.ApprovalExpiration{} + Expect(k8sClient.Get(ctx, expirationName, ae)).To(Succeed()) + originalExpiration := ae.Spec.Expiration.Time + + By("transitioning Approval to SUSPENDED") + approval := &approvalv1.Approval{} + Expect(k8sClient.Get(ctx, typeNamespacedName, approval)).To(Succeed()) + + approval.Spec.State = approvalv1.ApprovalStateSuspended + approval.Spec.Decisions = append(approval.Spec.Decisions, approvalv1.Decision{ + Name: "Dave", + Email: "dave@telekom.de", + Comment: "Suspended", + ResultingState: approvalv1.ApprovalStateSuspended, + }) + Expect(k8sClient.Update(ctx, approval)).To(Succeed()) + + By("checking ApprovalExpiration still exists with same expiration time") + Consistently(func(g Gomega) { + ae := &approvalv1.ApprovalExpiration{} + err := k8sClient.Get(ctx, expirationName, ae) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(ae.Spec.Expiration.Time).To(Equal(originalExpiration)) + }, 2*time.Second, interval).Should(Succeed()) + }) +}) diff --git a/approval/internal/controller/suite_test.go b/approval/internal/controller/suite_test.go index 369ba41ef..af1992ad6 100644 --- a/approval/internal/controller/suite_test.go +++ b/approval/internal/controller/suite_test.go @@ -24,6 +24,7 @@ import ( crscheme "sigs.k8s.io/controller-runtime/pkg/scheme" approvalv1 "github.com/telekom/controlplane/approval/api/v1" + "github.com/telekom/controlplane/approval/internal/config" "github.com/telekom/controlplane/common/pkg/test" "github.com/telekom/controlplane/common/pkg/test/mock" "github.com/telekom/controlplane/common/pkg/test/testutil" @@ -135,6 +136,20 @@ var _ = BeforeSuite(func() { }).SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) + expirationConfig := &config.ExpirationConfig{ + ExpirationPeriodMonths: 12, + LastMonthsWithWeeklyReminder: 2, + LastWeeksWithDailyReminder: 2, + } + + err = (&ApprovalExpirationReconciler{ + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + Recorder: &mock.EventRecorder{}, + ExpirationConfig: expirationConfig, + }).SetupWithManager(k8sManager) + Expect(err).ToNot(HaveOccurred()) + go func() { defer GinkgoRecover() err = k8sManager.Start(ctx) diff --git a/approval/internal/handler/approval/fsm.go b/approval/internal/handler/approval/fsm.go index 5be527cb8..8d879fbea 100644 --- a/approval/internal/handler/approval/fsm.go +++ b/approval/internal/handler/approval/fsm.go @@ -10,25 +10,29 @@ import ( ) var auto = fsm.Transitions{ - {Action: v1.ApprovalActionAllow, Src: []v1.ApprovalState{v1.ApprovalStateRejected}, Dst: v1.ApprovalStateGranted}, - {Action: v1.ApprovalActionDeny, Src: []v1.ApprovalState{v1.ApprovalStateGranted}, Dst: v1.ApprovalStateRejected}, + {Action: v1.ApprovalActionAllow, Src: []v1.ApprovalState{v1.ApprovalStateRejected, v1.ApprovalStateExpired}, Dst: v1.ApprovalStateGranted}, + {Action: v1.ApprovalActionDeny, Src: []v1.ApprovalState{v1.ApprovalStateGranted, v1.ApprovalStateExpired}, Dst: v1.ApprovalStateRejected}, {Action: v1.ApprovalActionSuspend, Src: []v1.ApprovalState{v1.ApprovalStateGranted}, Dst: v1.ApprovalStateSuspended}, {Action: v1.ApprovalActionResume, Src: []v1.ApprovalState{v1.ApprovalStateSuspended}, Dst: v1.ApprovalStateGranted}, + {Action: v1.ApprovalActionExpire, Src: []v1.ApprovalState{v1.ApprovalStateGranted, v1.ApprovalStateSuspended}, Dst: v1.ApprovalStateExpired}, } var simple = fsm.Transitions{ - {Action: v1.ApprovalActionAllow, Src: []v1.ApprovalState{v1.ApprovalStatePending, v1.ApprovalStateRejected}, Dst: v1.ApprovalStateGranted}, - {Action: v1.ApprovalActionDeny, Src: []v1.ApprovalState{v1.ApprovalStatePending, v1.ApprovalStateGranted}, Dst: v1.ApprovalStateRejected}, + {Action: v1.ApprovalActionAllow, Src: []v1.ApprovalState{v1.ApprovalStatePending, v1.ApprovalStateRejected, v1.ApprovalStateExpired}, Dst: v1.ApprovalStateGranted}, + {Action: v1.ApprovalActionDeny, Src: []v1.ApprovalState{v1.ApprovalStatePending, v1.ApprovalStateGranted, v1.ApprovalStateExpired}, Dst: v1.ApprovalStateRejected}, {Action: v1.ApprovalActionSuspend, Src: []v1.ApprovalState{v1.ApprovalStateGranted}, Dst: v1.ApprovalStateSuspended}, {Action: v1.ApprovalActionResume, Src: []v1.ApprovalState{v1.ApprovalStateSuspended}, Dst: v1.ApprovalStateGranted}, + {Action: v1.ApprovalActionExpire, Src: []v1.ApprovalState{v1.ApprovalStateGranted, v1.ApprovalStateSuspended}, Dst: v1.ApprovalStateExpired}, } var fourEyes = fsm.Transitions{ {Action: v1.ApprovalActionAllow, Src: []v1.ApprovalState{v1.ApprovalStatePending, v1.ApprovalStateRejected}, Dst: v1.ApprovalStateSemigranted}, - {Action: v1.ApprovalActionDeny, Src: []v1.ApprovalState{v1.ApprovalStatePending, v1.ApprovalStateGranted, v1.ApprovalStateSemigranted}, Dst: v1.ApprovalStateRejected}, {Action: v1.ApprovalActionAllow, Src: []v1.ApprovalState{v1.ApprovalStateSemigranted}, Dst: v1.ApprovalStateGranted}, + {Action: v1.ApprovalActionAllow, Src: []v1.ApprovalState{v1.ApprovalStateExpired}, Dst: v1.ApprovalStateGranted}, + {Action: v1.ApprovalActionDeny, Src: []v1.ApprovalState{v1.ApprovalStatePending, v1.ApprovalStateGranted, v1.ApprovalStateSemigranted, v1.ApprovalStateExpired}, Dst: v1.ApprovalStateRejected}, {Action: v1.ApprovalActionSuspend, Src: []v1.ApprovalState{v1.ApprovalStateGranted}, Dst: v1.ApprovalStateSuspended}, {Action: v1.ApprovalActionResume, Src: []v1.ApprovalState{v1.ApprovalStateSuspended}, Dst: v1.ApprovalStateGranted}, + {Action: v1.ApprovalActionExpire, Src: []v1.ApprovalState{v1.ApprovalStateGranted, v1.ApprovalStateSuspended}, Dst: v1.ApprovalStateExpired}, } var transitionMap = map[v1.ApprovalStrategy]fsm.Transitions{ diff --git a/approval/internal/handler/approval/handler.go b/approval/internal/handler/approval/handler.go index 46fd4f951..73099c92b 100644 --- a/approval/internal/handler/approval/handler.go +++ b/approval/internal/handler/approval/handler.go @@ -6,15 +6,22 @@ package approval import ( "context" + "fmt" + "time" "github.com/pkg/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" approvalv1 "github.com/telekom/controlplane/approval/api/v1" approval_condition "github.com/telekom/controlplane/approval/internal/condition" "github.com/telekom/controlplane/approval/internal/handler/util" + commonclient "github.com/telekom/controlplane/common/pkg/client" "github.com/telekom/controlplane/common/pkg/condition" "github.com/telekom/controlplane/common/pkg/handler" + "github.com/telekom/controlplane/common/pkg/types" "github.com/telekom/controlplane/common/pkg/util/contextutil" ) @@ -31,7 +38,11 @@ func (h *ApprovalHandler) CreateOrUpdate(ctx context.Context, approval *approval } fsm := ApprovalStrategyFSM[approval.Spec.Strategy] - approval.Status.AvailableTransitions = fsm.AvailableTransitions(approval.Spec.State) + // Filter out system-only actions (Expire) from available transitions + approval.Status.AvailableTransitions = filterSystemActions(fsm.AvailableTransitions(approval.Spec.State)) + + // Capture state change BEFORE updating LastState (needed for expiration logic) + stateChanged := approval.Spec.State != approval.Status.LastState approval.Status.LastState = approval.Spec.State switch approval.Spec.State { @@ -60,6 +71,16 @@ func (h *ApprovalHandler) CreateOrUpdate(ctx context.Context, approval *approval approval.SetCondition(condition.NewProcessingCondition("Suspended", "Approval is suspended")) approval.SetCondition(condition.NewReadyCondition("Suspended", "Approval is suspended")) + case approvalv1.ApprovalStateExpired: + approval.SetCondition(approval_condition.NewExpiredCondition()) + approval.SetCondition(condition.NewProcessingCondition("Expired", "Approval has expired")) + approval.SetCondition(condition.NewReadyCondition("Expired", "Approval has expired but can be re-approved")) + + } + + // Handle ApprovalExpiration lifecycle (do this after conditions are set) + if err := h.handleExpiration(ctx, approval, stateChanged); err != nil { + return errors.Wrap(err, "failed to handle expiration") } return nil @@ -125,3 +146,121 @@ func (h *ApprovalHandler) Delete(ctx context.Context, approval *approvalv1.Appro logger.Info("Approval deleted") return nil } + +// filterSystemActions removes system-only actions (Expire) from the available transitions +func filterSystemActions(transitions approvalv1.AvailableTransitions) approvalv1.AvailableTransitions { + filtered := make(approvalv1.AvailableTransitions, 0, len(transitions)) + for _, t := range transitions { + if t.Action != approvalv1.ApprovalActionExpire { + filtered = append(filtered, t) + } + } + return filtered +} + +// handleExpiration manages the lifecycle of ApprovalExpiration resources +func (h *ApprovalHandler) handleExpiration(ctx context.Context, approval *approvalv1.Approval, stateChanged bool) error { + // Only for non-Auto strategies + if approval.Spec.Strategy == approvalv1.ApprovalStrategyAuto { + return nil + } + + c := commonclient.ClientFromContextOrDie(ctx) + + switch approval.Spec.State { + case approvalv1.ApprovalStateGranted: + if stateChanged { + // Create or update ApprovalExpiration with fresh dates + // (covers initial GRANTED and REALLOW from EXPIRED) + return createOrUpdateApprovalExpiration(ctx, c, approval) + } + // State unchanged, leave ApprovalExpiration alone + + case approvalv1.ApprovalStateSuspended: + // Clock keeps ticking, leave ApprovalExpiration alone + + case approvalv1.ApprovalStateExpired: + // Already expired, leave ApprovalExpiration alone + + case approvalv1.ApprovalStateRejected: + if stateChanged { + // Delete ApprovalExpiration + return deleteApprovalExpiration(ctx, c, approval) + } + + case approvalv1.ApprovalStatePending, approvalv1.ApprovalStateSemigranted: + // No ApprovalExpiration should exist in these states + // (safety: delete if exists, but shouldn't happen) + return deleteApprovalExpiration(ctx, c, approval) + } + + return nil +} + +// createOrUpdateApprovalExpiration creates or updates an ApprovalExpiration with fresh dates +func createOrUpdateApprovalExpiration(ctx context.Context, c commonclient.JanitorClient, approval *approvalv1.Approval) error { + // TODO: Load expiration config from environment or use defaults + // For now, hardcode defaults: 12 months expiration, 2 months weekly, 2 weeks daily + expirationPeriodMonths := 12 + weeklyReminderMonths := 2 + dailyReminderWeeks := 2 + + now := time.Now() + expirationDate := now.AddDate(0, expirationPeriodMonths, 0) + weeklyReminderDate := expirationDate.AddDate(0, -weeklyReminderMonths, 0) + dailyReminderDate := expirationDate.AddDate(0, 0, -dailyReminderWeeks*7) + + ae := &approvalv1.ApprovalExpiration{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s--expiration", approval.Name), + Namespace: approval.Namespace, + }, + } + + mutate := func() error { + if err := controllerutil.SetControllerReference(approval, ae, c.Scheme()); err != nil { + return errors.Wrap(err, "failed to set controller reference") + } + ae.Spec = approvalv1.ApprovalExpirationSpec{ + Approval: types.ObjectRef{ + Name: approval.Name, + Namespace: approval.Namespace, + }, + Expiration: metav1.Time{Time: expirationDate}, + WeeklyReminder: metav1.Time{Time: weeklyReminderDate}, + DailyReminder: metav1.Time{Time: dailyReminderDate}, + } + return nil + } + + _, err := c.CreateOrUpdate(ctx, ae, mutate) + if err != nil { + return errors.Wrap(err, "failed to create or update ApprovalExpiration") + } + + log.FromContext(ctx).Info("Created or updated ApprovalExpiration", "name", ae.Name, "expiration", expirationDate) + return nil +} + +// deleteApprovalExpiration deletes the ApprovalExpiration if it exists +func deleteApprovalExpiration(ctx context.Context, c commonclient.JanitorClient, approval *approvalv1.Approval) error { + ae := &approvalv1.ApprovalExpiration{} + key := client.ObjectKey{ + Name: fmt.Sprintf("%s--expiration", approval.Name), + Namespace: approval.Namespace, + } + + err := c.Get(ctx, key, ae) + if err != nil { + // Doesn't exist, nothing to delete + return client.IgnoreNotFound(err) + } + + // Exists, delete it + if err := c.Delete(ctx, ae); err != nil { + return errors.Wrap(err, "failed to delete ApprovalExpiration") + } + + log.FromContext(ctx).Info("Deleted ApprovalExpiration", "name", ae.Name) + return nil +} diff --git a/approval/internal/handler/approvalexpiration/handler.go b/approval/internal/handler/approvalexpiration/handler.go new file mode 100644 index 000000000..dc351f5a6 --- /dev/null +++ b/approval/internal/handler/approvalexpiration/handler.go @@ -0,0 +1,247 @@ +// Copyright 2025 Deutsche Telekom IT GmbH +// +// SPDX-License-Identifier: Apache-2.0 + +package approvalexpiration + +import ( + "context" + "fmt" + "time" + + "github.com/pkg/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" + + v1 "github.com/telekom/controlplane/approval/api/v1" + "github.com/telekom/controlplane/approval/internal/config" + "github.com/telekom/controlplane/approval/internal/handler/util" + ctrlerrors "github.com/telekom/controlplane/common/pkg/errors/ctrlerrors" +) + +// Handler handles ApprovalExpiration resources +type Handler struct { + client client.Client + config *config.ExpirationConfig +} + +// NewHandler creates a new ApprovalExpiration handler +func NewHandler(c client.Client, cfg *config.ExpirationConfig) *Handler { + return &Handler{ + client: c, + config: cfg, + } +} + +// CreateOrUpdate processes an ApprovalExpiration resource +func (h *Handler) CreateOrUpdate(ctx context.Context, ae *v1.ApprovalExpiration) error { + logger := log.FromContext(ctx) + + // Load parent Approval + approval, err := h.getParentApproval(ctx, ae) + if err != nil { + return errors.Wrap(err, "failed to get parent approval") + } + + // Guard: parent must be in active state (GRANTED or SUSPENDED) + if approval.Spec.State != v1.ApprovalStateGranted && approval.Spec.State != v1.ApprovalStateSuspended { + logger.V(1).Info("Parent approval not in expirable state, skipping", + "approvalState", approval.Spec.State) + return nil + } + + now := time.Now() + + // Check if expired + if now.After(ae.Spec.Expiration.Time) || now.Equal(ae.Spec.Expiration.Time) { + logger.Info("Approval expired, transitioning to EXPIRED state", + "expiration", ae.Spec.Expiration.Time) + return h.ensureApprovalExpired(ctx, approval) + } + + // Check if reminder should be sent + if h.shouldRemind(ae, now) { + if err := h.sendReminder(ctx, ae, approval, now); err != nil { + return errors.Wrap(err, "failed to send reminder") + } + ae.Status.LastReminder = &metav1.Time{Time: now} + logger.V(1).Info("Reminder sent", "lastReminder", now) + } + + // Schedule next reconciliation + return h.requeueAtNextEvent(ae, now) +} + +// Delete handles cleanup when an ApprovalExpiration is deleted +func (h *Handler) Delete(ctx context.Context, ae *v1.ApprovalExpiration) error { + // Notifications are owned by ApprovalExpiration, auto-deleted by GC + return nil +} + +// getParentApproval fetches the parent Approval resource +func (h *Handler) getParentApproval(ctx context.Context, ae *v1.ApprovalExpiration) (*v1.Approval, error) { + approval := &v1.Approval{} + key := client.ObjectKey{ + Name: ae.Spec.Approval.Name, + Namespace: ae.Spec.Approval.Namespace, + } + if err := h.client.Get(ctx, key, approval); err != nil { + return nil, err + } + return approval, nil +} + +// shouldRemind determines if a reminder should be sent based on the current time and last reminder +func (h *Handler) shouldRemind(ae *v1.ApprovalExpiration, now time.Time) bool { + lastReminder := ae.Status.LastReminder + + expired := now.After(ae.Spec.Expiration.Time) || now.Equal(ae.Spec.Expiration.Time) + + if expired || now.After(ae.Spec.DailyReminder.Time) { + // Daily: send if never reminded OR last reminder was >= 1 day ago + return lastReminder == nil || now.After(lastReminder.Add(24*time.Hour)) + } + + if now.After(ae.Spec.WeeklyReminder.Time) { + // Weekly: send if never reminded OR last reminder was >= 1 week ago + return lastReminder == nil || now.After(lastReminder.Add(7*24*time.Hour)) + } + + // Too early + return false +} + +// ensureApprovalExpired transitions the parent Approval to EXPIRED state +func (h *Handler) ensureApprovalExpired(ctx context.Context, approval *v1.Approval) error { + if approval.Spec.State == v1.ApprovalStateExpired { + return nil // Already expired, no-op + } + + // Add system Decision (matches auto-approval pattern) + approval.Spec.Decisions = append(approval.Spec.Decisions, v1.Decision{ + Name: "System", + Comment: "Automatically expired - expiration date reached. Available actions: Allow (re-approve), Deny (reject).", + Timestamp: &metav1.Time{Time: time.Now()}, + ResultingState: v1.ApprovalStateExpired, + }) + + // Set state + approval.Spec.State = v1.ApprovalStateExpired + + // Update via API server (goes through webhook) + if err := h.client.Update(ctx, approval); err != nil { + return errors.Wrap(err, "failed to expire approval") + } + + return nil +} + +// sendReminder sends a reminder notification for the approaching or reached expiration +func (h *Handler) sendReminder(ctx context.Context, ae *v1.ApprovalExpiration, approval *v1.Approval, now time.Time) error { + // Determine reminder type + var reminderType string + var daysRemaining int64 + + expired := now.After(ae.Spec.Expiration.Time) || now.Equal(ae.Spec.Expiration.Time) + switch { + case expired: + reminderType = "expired" + daysRemaining = 0 + case now.After(ae.Spec.DailyReminder.Time): + reminderType = "daily" + daysRemaining = int64(ae.Spec.Expiration.Time.Sub(now).Hours() / 24) + default: + reminderType = "weekly" + daysRemaining = int64(ae.Spec.Expiration.Time.Sub(now).Hours() / 24) + } + + // Build notification data with template properties + notificationData := util.NotificationData{ + Owner: ae, + SendToChannelNamespace: approval.Namespace, + StateNew: string(approval.Spec.State), + StateOld: string(approval.Spec.State), // State hasn't changed yet for reminders + Target: &approval.Spec.Target, + Requester: &approval.Spec.Requester, + Decider: &approval.Spec.Decider, + Scenario: util.NotificationScenarioUpdated, + Action: approval.Spec.Action, + ExpirationDate: ae.Spec.Expiration.Format("2006-01-02"), + DaysRemaining: fmt.Sprintf("%d", daysRemaining), + ReminderType: reminderType, + } + + // Send to decider + notificationData.Actor = util.ActorDecider + deciderRef, err := util.SendReminderNotification(ctx, h.client, ¬ificationData) + if err != nil { + return errors.Wrap(err, "failed to send decider reminder") + } + + // Send to requester + notificationData.Actor = util.ActorRequester + requesterRef, err := util.SendReminderNotification(ctx, h.client, ¬ificationData) + if err != nil { + return errors.Wrap(err, "failed to send requester reminder") + } + + // Store last notification ref (we'll keep the requester one) + ae.Status.LastNotificationRef = requesterRef + + _ = deciderRef // Used for sending, but we only store one ref + + return nil +} + +// requeueAtNextEvent schedules the next reconciliation at the appropriate time +func (h *Handler) requeueAtNextEvent(ae *v1.ApprovalExpiration, now time.Time) error { + var nextEvent time.Time + + switch { + case now.Before(ae.Spec.WeeklyReminder.Time): + // Not yet in reminder period + nextEvent = ae.Spec.WeeklyReminder.Time + + case now.Before(ae.Spec.DailyReminder.Time): + // In weekly reminder period + if ae.Status.LastReminder != nil { + nextWeekly := ae.Status.LastReminder.Add(7 * 24 * time.Hour) + if nextWeekly.Before(ae.Spec.DailyReminder.Time) { + nextEvent = nextWeekly + } else { + nextEvent = ae.Spec.DailyReminder.Time + } + } else { + nextEvent = now // Should remind now + } + + case now.Before(ae.Spec.Expiration.Time): + // In daily reminder period + if ae.Status.LastReminder != nil { + nextDaily := ae.Status.LastReminder.Add(24 * time.Hour) + if nextDaily.Before(ae.Spec.Expiration.Time) { + nextEvent = nextDaily + } else { + nextEvent = ae.Spec.Expiration.Time + } + } else { + nextEvent = now + } + + default: + // Already expired - requeue in 1 day for continued daily reminders + if ae.Status.LastReminder != nil { + nextEvent = ae.Status.LastReminder.Add(24 * time.Hour) + } else { + nextEvent = now + } + } + + delay := nextEvent.Sub(now) + if delay <= 0 { + delay = time.Second // Immediate retry + } + + return ctrlerrors.RetryableWithDelayErrorf(delay, "next expiration event at %s", nextEvent.Format(time.RFC3339)) +} diff --git a/approval/internal/handler/approvalexpiration/handler_test.go b/approval/internal/handler/approvalexpiration/handler_test.go new file mode 100644 index 000000000..2d8bad007 --- /dev/null +++ b/approval/internal/handler/approvalexpiration/handler_test.go @@ -0,0 +1,237 @@ +// Copyright 2025 Deutsche Telekom IT GmbH +// +// SPDX-License-Identifier: Apache-2.0 + +package approvalexpiration + +import ( + "testing" + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + v1 "github.com/telekom/controlplane/approval/api/v1" + "github.com/telekom/controlplane/approval/internal/config" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestApprovalExpirationHandler(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "ApprovalExpiration Handler Suite") +} + +var _ = Describe("ApprovalExpiration Handler", func() { + var handler *Handler + var cfg *config.ExpirationConfig + + BeforeEach(func() { + cfg = &config.ExpirationConfig{ + ExpirationPeriodMonths: 12, + LastMonthsWithWeeklyReminder: 2, + LastWeeksWithDailyReminder: 2, + } + handler = &Handler{ + client: nil, // Not needed for unit tests + config: cfg, + } + }) + + Describe("shouldRemind", func() { + var ae *v1.ApprovalExpiration + var now time.Time + + BeforeEach(func() { + now = time.Date(2026, 5, 1, 12, 0, 0, 0, time.UTC) + // Realistic timeline: expiration in 3 months + expirationDate := now.AddDate(0, 3, 0) // 3 months from now (Aug 1) + weeklyReminderDate := now.AddDate(0, 1, 0) // 1 month from now (Jun 1) - last 2 months + dailyReminderDate := now.AddDate(0, 2, 17) // 2 months 17 days from now (Jul 18) - last 2 weeks + + ae = &v1.ApprovalExpiration{ + Spec: v1.ApprovalExpirationSpec{ + Expiration: metav1.Time{Time: expirationDate}, + WeeklyReminder: metav1.Time{Time: weeklyReminderDate}, + DailyReminder: metav1.Time{Time: dailyReminderDate}, + }, + Status: v1.ApprovalExpirationStatus{}, + } + }) + + Context("when before weekly reminder period", func() { + It("should not send reminder", func() { + testNow := ae.Spec.WeeklyReminder.Add(-24 * time.Hour) // 1 day before weekly starts + result := handler.shouldRemind(ae, testNow) + Expect(result).To(BeFalse()) + }) + }) + + Context("when in weekly reminder period", func() { + It("should send reminder if never reminded", func() { + testNow := ae.Spec.WeeklyReminder.Add(1 * time.Hour) + ae.Status.LastReminder = nil + result := handler.shouldRemind(ae, testNow) + Expect(result).To(BeTrue()) + }) + + It("should send reminder if last reminded over a week ago", func() { + testNow := ae.Spec.WeeklyReminder.Add(8 * 24 * time.Hour) // 8 days after weekly start + lastReminder := testNow.Add(-8 * 24 * time.Hour) // 8 days ago + ae.Status.LastReminder = &metav1.Time{Time: lastReminder} + result := handler.shouldRemind(ae, testNow) + Expect(result).To(BeTrue()) + }) + + It("should not send reminder if last reminded less than a week ago", func() { + // Still in weekly period, not yet in daily period + testNow := ae.Spec.WeeklyReminder.Add(10 * 24 * time.Hour) // 10 days after weekly start, still before daily + lastReminder := testNow.Add(-5 * 24 * time.Hour) // 5 days ago + ae.Status.LastReminder = &metav1.Time{Time: lastReminder} + result := handler.shouldRemind(ae, testNow) + Expect(result).To(BeFalse()) + }) + }) + + Context("when in daily reminder period", func() { + It("should send reminder if never reminded", func() { + testNow := ae.Spec.DailyReminder.Add(1 * time.Hour) + ae.Status.LastReminder = nil + result := handler.shouldRemind(ae, testNow) + Expect(result).To(BeTrue()) + }) + + It("should send reminder if last reminded over a day ago", func() { + testNow := ae.Spec.DailyReminder.Add(2 * 24 * time.Hour) // 2 days after daily start + lastReminder := testNow.Add(-25 * time.Hour) // 25 hours ago + ae.Status.LastReminder = &metav1.Time{Time: lastReminder} + result := handler.shouldRemind(ae, testNow) + Expect(result).To(BeTrue()) + }) + + It("should not send reminder if last reminded less than a day ago", func() { + testNow := ae.Spec.DailyReminder.Add(2 * 24 * time.Hour) // 2 days after daily start + lastReminder := testNow.Add(-20 * time.Hour) // 20 hours ago + ae.Status.LastReminder = &metav1.Time{Time: lastReminder} + result := handler.shouldRemind(ae, testNow) + Expect(result).To(BeFalse()) + }) + }) + + Context("when expired", func() { + It("should send reminder if never reminded", func() { + testNow := ae.Spec.Expiration.Add(1 * time.Hour) // 1 hour after expiration + ae.Status.LastReminder = nil + result := handler.shouldRemind(ae, testNow) + Expect(result).To(BeTrue()) + }) + + It("should send reminder if last reminded over a day ago", func() { + testNow := ae.Spec.Expiration.Add(3 * 24 * time.Hour) // 3 days after expiration + lastReminder := testNow.Add(-25 * time.Hour) // 25 hours ago + ae.Status.LastReminder = &metav1.Time{Time: lastReminder} + result := handler.shouldRemind(ae, testNow) + Expect(result).To(BeTrue()) + }) + + It("should not send reminder if last reminded less than a day ago", func() { + testNow := ae.Spec.Expiration.Add(3 * 24 * time.Hour) // 3 days after expiration + lastReminder := testNow.Add(-20 * time.Hour) // 20 hours ago + ae.Status.LastReminder = &metav1.Time{Time: lastReminder} + result := handler.shouldRemind(ae, testNow) + Expect(result).To(BeFalse()) + }) + }) + + Context("when exactly at expiration time", func() { + It("should send reminder", func() { + testNow := ae.Spec.Expiration.Time + ae.Status.LastReminder = nil + result := handler.shouldRemind(ae, testNow) + Expect(result).To(BeTrue()) + }) + }) + }) + + Describe("requeueAtNextEvent", func() { + var ae *v1.ApprovalExpiration + var now time.Time + + BeforeEach(func() { + now = time.Date(2026, 5, 1, 12, 0, 0, 0, time.UTC) + // Realistic timeline: expiration in 3 months + expirationDate := now.AddDate(0, 3, 0) // 3 months from now (Aug 1) + weeklyReminderDate := now.AddDate(0, 1, 0) // 1 month from now (Jun 1) - last 2 months + dailyReminderDate := now.AddDate(0, 2, 17) // 2 months 17 days from now (Jul 18) - last 2 weeks + + ae = &v1.ApprovalExpiration{ + Spec: v1.ApprovalExpirationSpec{ + Expiration: metav1.Time{Time: expirationDate}, + WeeklyReminder: metav1.Time{Time: weeklyReminderDate}, + DailyReminder: metav1.Time{Time: dailyReminderDate}, + }, + Status: v1.ApprovalExpirationStatus{}, + } + }) + + Context("when before weekly reminder period", func() { + It("should requeue at weekly reminder time", func() { + testNow := ae.Spec.WeeklyReminder.Add(-1 * time.Hour) + err := handler.requeueAtNextEvent(ae, testNow) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("next expiration event")) + }) + }) + + Context("when in weekly reminder period", func() { + It("should requeue at next weekly interval if reminded", func() { + testNow := ae.Spec.WeeklyReminder.Add(8 * 24 * time.Hour) + lastReminder := testNow.Add(-1 * time.Hour) + ae.Status.LastReminder = &metav1.Time{Time: lastReminder} + err := handler.requeueAtNextEvent(ae, testNow) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("next expiration event")) + }) + + It("should requeue at daily reminder time if next weekly is after daily", func() { + testNow := ae.Spec.WeeklyReminder.Add(1 * 24 * time.Hour) + lastReminder := testNow.Add(-1 * time.Hour) + ae.Status.LastReminder = &metav1.Time{Time: lastReminder} + err := handler.requeueAtNextEvent(ae, testNow) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("next expiration event")) + }) + }) + + Context("when in daily reminder period", func() { + It("should requeue at next daily interval if reminded", func() { + testNow := ae.Spec.DailyReminder.Add(25 * time.Hour) + lastReminder := testNow.Add(-1 * time.Hour) + ae.Status.LastReminder = &metav1.Time{Time: lastReminder} + err := handler.requeueAtNextEvent(ae, testNow) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("next expiration event")) + }) + + It("should requeue at expiration time if next daily is after expiration", func() { + testNow := ae.Spec.Expiration.Add(-1 * time.Hour) + lastReminder := testNow.Add(-1 * time.Hour) + ae.Status.LastReminder = &metav1.Time{Time: lastReminder} + err := handler.requeueAtNextEvent(ae, testNow) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("next expiration event")) + }) + }) + + Context("when expired", func() { + It("should requeue at next daily interval for continued reminders", func() { + testNow := ae.Spec.Expiration.Add(3 * 24 * time.Hour) + lastReminder := testNow.Add(-1 * time.Hour) + ae.Status.LastReminder = &metav1.Time{Time: lastReminder} + err := handler.requeueAtNextEvent(ae, testNow) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("next expiration event")) + }) + }) + }) +}) diff --git a/approval/internal/handler/util/notification.go b/approval/internal/handler/util/notification.go index 33a146293..b1f02a36a 100644 --- a/approval/internal/handler/util/notification.go +++ b/approval/internal/handler/util/notification.go @@ -45,9 +45,12 @@ const ( // TemplatePlaceholderResourceType can be api/event TemplatePlaceholderResourceType = "resource_type" - TemplatePlaceholderStateOld = "state_old" - TemplatePlaceholderStateNew = "state_new" - TemplatePlaceholderScopes = "scopes" + TemplatePlaceholderStateOld = "state_old" + TemplatePlaceholderStateNew = "state_new" + TemplatePlaceholderScopes = "scopes" + TemplatePlaceholderExpirationDate = "expiration_date" + TemplatePlaceholderDaysRemaining = "days_remaining" + TemplatePlaceholderReminderType = "reminder_type" ) const ( @@ -78,6 +81,10 @@ type NotificationData struct { Scenario NotificationScenario Actor Actor Action string + // Expiration-specific fields + ExpirationDate string + DaysRemaining string + ReminderType string } func extractDecider(decider *approvalv1.Decider) (map[string]any, error) { @@ -226,6 +233,69 @@ func initializeProperties() map[string]any { properties[TemplatePlaceholderStateOld] = defaultValue properties[TemplatePlaceholderStateNew] = defaultValue properties[TemplatePlaceholderScopes] = defaultValue + // Note: Expiration fields (ExpirationDate, DaysRemaining, ReminderType) are only + // initialized in SendReminderNotification, not for regular notifications return properties } + +// SendReminderNotification sends an expiration reminder notification +func SendReminderNotification(ctx context.Context, c client.Client, data *NotificationData) (*types.ObjectRef, error) { + properties := initializeProperties() + + properties[TemplatePlaceholderEnvironment] = contextutil.EnvFromContextOrDie(ctx) + properties[TemplatePlaceholderStateNew] = data.StateNew + properties[TemplatePlaceholderStateOld] = data.StateOld + properties[TemplatePlaceholderExpirationDate] = data.ExpirationDate + properties[TemplatePlaceholderDaysRemaining] = data.DaysRemaining + properties[TemplatePlaceholderReminderType] = data.ReminderType + + requesterMap, err := extractRequester(data.Requester) + if err != nil { + return nil, errors.Wrap(err, "failed to extract requester data") + } + for k, v := range requesterMap { + properties[strings.ToLower(k)] = v + } + + deciderMap, err := extractDecider(data.Decider) + if err != nil { + return nil, errors.Wrap(err, "failed to extract decider data") + } + for k, v := range deciderMap { + properties[strings.ToLower(k)] = v + } + + // Build purpose: ----reminder-- + // Example: approvalexpiration--subscribe--reminder--decider + purposeStringBuilder := strings.Builder{} + purposeStringBuilder.WriteString(strings.ToLower(data.Owner.GetObjectKind().GroupVersionKind().Kind)) + purposeStringBuilder.WriteString(DELIMITER) + purposeStringBuilder.WriteString(data.Action) + purposeStringBuilder.WriteString(DELIMITER) + purposeStringBuilder.WriteString("reminder") + purposeStringBuilder.WriteString(DELIMITER) + purposeStringBuilder.WriteString(string(data.Actor)) + purpose := purposeStringBuilder.String() + + // Build notification name: ---- + nameStringBuilder := strings.Builder{} + nameStringBuilder.WriteString(purpose) + nameStringBuilder.WriteString(DELIMITER) + nameStringBuilder.WriteString(data.Target.GetName()) + name := nameStringBuilder.String() + + notificationBuilder := builder.New(). + WithOwner(data.Owner). + WithSender(notificationv1.SenderTypeSystem, "ApprovalService"). + WithDefaultChannels(ctx, data.SendToChannelNamespace). + WithPurpose(purpose). + WithName(labelutil.NormalizeNameValue(name)). + WithProperties(properties) + + notification, err := notificationBuilder.Send(ctx) + if err != nil { + return nil, err + } + return types.ObjectRefFromObject(notification), nil +} diff --git a/approval/internal/webhook/v1/approval_webhook.go b/approval/internal/webhook/v1/approval_webhook.go index 00cea17ef..5b85e3a37 100644 --- a/approval/internal/webhook/v1/approval_webhook.go +++ b/approval/internal/webhook/v1/approval_webhook.go @@ -86,6 +86,10 @@ func (a *ApprovalCustomValidator) ValidateUpdate(_ context.Context, oldObj, newO // instead of Status.AvailableTransitions (which may be stale or nil before // the controller has reconciled). Auto strategy uses its own FSM. if stateChanged { + if validationErr := validateExpireTransition(newObj); validationErr != nil { + return warnings, validationErr + } + fsmDef, ok := approvalhandler.ApprovalStrategyFSM[newObj.Spec.Strategy] if !ok { err = apierrors.NewBadRequest("Unknown approval strategy") @@ -124,3 +128,19 @@ func (a *ApprovalCustomValidator) ValidateDelete(_ context.Context, obj *approva return nil, nil } + +// 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") +} diff --git a/approval/internal/webhook/v1/approval_webhook_test.go b/approval/internal/webhook/v1/approval_webhook_test.go index 8d3776dea..5458e92d4 100644 --- a/approval/internal/webhook/v1/approval_webhook_test.go +++ b/approval/internal/webhook/v1/approval_webhook_test.go @@ -473,4 +473,80 @@ var _ = Describe("Approval Webhook", func() { Expect(err).NotTo(HaveOccurred()) }) }) + + Context("Expire action validation (system-only)", func() { + var validator ApprovalCustomValidator + + makeApproval := func(specState approvalv1.ApprovalState, decisions []approvalv1.Decision) *approvalv1.Approval { + return &approvalv1.Approval{ + Spec: approvalv1.ApprovalSpec{ + Strategy: approvalv1.ApprovalStrategySimple, + State: specState, + Decisions: decisions, + }, + } + } + + It("should reject manual transition to EXPIRED without System decision", func() { + oldObj := makeApproval(approvalv1.ApprovalStateGranted, nil) + newObj := makeApproval(approvalv1.ApprovalStateExpired, []approvalv1.Decision{ + {Name: "Alice", Email: "alice@telekom.de", Comment: "Manual expire", ResultingState: approvalv1.ApprovalStateExpired}, + }) + _, err := validator.ValidateUpdate(context.Background(), oldObj, newObj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("Expire action is system-only")) + }) + + It("should reject manual transition to EXPIRED from SUSPENDED without System decision", func() { + oldObj := makeApproval(approvalv1.ApprovalStateSuspended, nil) + newObj := makeApproval(approvalv1.ApprovalStateExpired, []approvalv1.Decision{ + {Name: "Bob", Email: "bob@telekom.de", Comment: "Manual expire", ResultingState: approvalv1.ApprovalStateExpired}, + }) + _, err := validator.ValidateUpdate(context.Background(), oldObj, newObj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("Expire action is system-only")) + }) + + It("should allow System transition to EXPIRED", func() { + oldObj := makeApproval(approvalv1.ApprovalStateGranted, nil) + newObj := makeApproval(approvalv1.ApprovalStateExpired, []approvalv1.Decision{ + {Name: "System", Email: "", Comment: "Automatically expired", ResultingState: approvalv1.ApprovalStateExpired}, + }) + _, err := validator.ValidateUpdate(context.Background(), oldObj, newObj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should allow System transition to EXPIRED from SUSPENDED", func() { + oldObj := makeApproval(approvalv1.ApprovalStateSuspended, nil) + newObj := makeApproval(approvalv1.ApprovalStateExpired, []approvalv1.Decision{ + {Name: "System", Email: "", Comment: "Automatically expired", ResultingState: approvalv1.ApprovalStateExpired}, + }) + _, err := validator.ValidateUpdate(context.Background(), oldObj, newObj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should allow transition from EXPIRED to GRANTED (re-approval)", func() { + oldObj := makeApproval(approvalv1.ApprovalStateExpired, []approvalv1.Decision{ + {Name: "System", Email: "", Comment: "Automatically expired", ResultingState: approvalv1.ApprovalStateExpired}, + }) + newObj := makeApproval(approvalv1.ApprovalStateGranted, []approvalv1.Decision{ + {Name: "System", Email: "", Comment: "Automatically expired", ResultingState: approvalv1.ApprovalStateExpired}, + {Name: "Charlie", Email: "charlie@telekom.de", Comment: "Re-approved", ResultingState: approvalv1.ApprovalStateGranted}, + }) + _, err := validator.ValidateUpdate(context.Background(), oldObj, newObj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should allow transition from EXPIRED to REJECTED", func() { + oldObj := makeApproval(approvalv1.ApprovalStateExpired, []approvalv1.Decision{ + {Name: "System", Email: "", Comment: "Automatically expired", ResultingState: approvalv1.ApprovalStateExpired}, + }) + newObj := makeApproval(approvalv1.ApprovalStateRejected, []approvalv1.Decision{ + {Name: "System", Email: "", Comment: "Automatically expired", ResultingState: approvalv1.ApprovalStateExpired}, + {Name: "Dave", Email: "dave@telekom.de", Comment: "Denied", ResultingState: approvalv1.ApprovalStateRejected}, + }) + _, err := validator.ValidateUpdate(context.Background(), oldObj, newObj) + Expect(err).NotTo(HaveOccurred()) + }) + }) }) From d979b386708080073b400c54365d34337bb6361c Mon Sep 17 00:00:00 2001 From: Julius Malcovsky Date: Mon, 4 May 2026 16:01:17 +0200 Subject: [PATCH 2/5] fix(approval-expiry): add new CR to kustomization --- approval/config/crd/kustomization.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/approval/config/crd/kustomization.yaml b/approval/config/crd/kustomization.yaml index 29bce0dd3..56dd3fa3d 100644 --- a/approval/config/crd/kustomization.yaml +++ b/approval/config/crd/kustomization.yaml @@ -8,6 +8,7 @@ resources: - bases/approval.cp.ei.telekom.de_approvals.yaml - bases/approval.cp.ei.telekom.de_approvalrequests.yaml +- bases/approval.cp.ei.telekom.de_approvalexpirations.yaml # +kubebuilder:scaffold:crdkustomizeresource patches: From 6e9661261973279fe5243980f42017f30a7a466f Mon Sep 17 00:00:00 2001 From: Julius Malcovsky Date: Thu, 7 May 2026 11:38:10 +0200 Subject: [PATCH 3/5] chore(approval-expiration): code improvements and refactoring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Björn Kottner Co-authored-by: Ismael Garba Co-authored-by: Stefan Siber Co-authored-by: Ron Gummich --- approval/api/v1/approvalexpiration_types.go | 19 +- approval/api/v1/builder/builder.go | 2 +- approval/api/v1/common_types.go | 3 + approval/api/v1/zz_generated.deepcopy.go | 23 +- approval/cmd/main.go | 15 +- ....cp.ei.telekom.de_approvalexpirations.yaml | 101 +++++--- approval/config/rbac/role.yaml | 1 + approval/internal/condition/condition.go | 2 +- approval/internal/condition/condition_test.go | 115 +++++++++ approval/internal/config/expiration.go | 63 +++-- .../controller/approval_controller.go | 9 +- .../approvalexpiration_controller.go | 37 ++- .../approvalexpiration_controller_test.go | 9 +- approval/internal/controller/suite_test.go | 34 ++- approval/internal/handler/approval/handler.go | 33 +-- .../handler/approvalexpiration/handler.go | 198 ++++++--------- .../approvalexpiration/handler_test.go | 233 +++--------------- .../internal/handler/util/notification.go | 4 +- .../internal/webhook/v1/approval_webhook.go | 29 ++- .../webhook/v1/approval_webhook_test.go | 23 +- .../webhook/v1/approvalrequest_webhook.go | 2 +- 21 files changed, 489 insertions(+), 466 deletions(-) create mode 100644 approval/internal/condition/condition_test.go diff --git a/approval/api/v1/approvalexpiration_types.go b/approval/api/v1/approvalexpiration_types.go index 3a6a83ceb..a5ed79244 100644 --- a/approval/api/v1/approvalexpiration_types.go +++ b/approval/api/v1/approvalexpiration_types.go @@ -5,6 +5,7 @@ package v1 import ( + "github.com/telekom/controlplane/common/pkg/reminder" "github.com/telekom/controlplane/common/pkg/types" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -18,11 +19,11 @@ type ApprovalExpirationSpec struct { // Expiration is the absolute date when the approval expires Expiration metav1.Time `json:"expiration"` - // WeeklyReminder is the date from which weekly reminders start - WeeklyReminder metav1.Time `json:"weeklyReminder"` - - // DailyReminder is the date from which daily reminders start - DailyReminder metav1.Time `json:"dailyReminder"` + // Thresholds defines when reminders should be sent relative to the expiration deadline. + // For example: [{Before: "720h"}, {Before: "168h", Repeat: "24h"}] sends one reminder + // 30 days before expiration, then daily reminders starting 7 days before. + // +optional + Thresholds []reminder.Threshold `json:"thresholds,omitempty"` } // ApprovalExpirationStatus defines the observed state of ApprovalExpiration @@ -34,13 +35,9 @@ type ApprovalExpirationStatus struct { // +optional Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"` - // LastReminder is the timestamp when the last reminder was sent - // +optional - LastReminder *metav1.Time `json:"lastReminder,omitempty"` - - // LastNotificationRef is a reference to the last sent reminder notification + // SentReminders tracks which reminder thresholds have been triggered and when // +optional - LastNotificationRef *types.ObjectRef `json:"lastNotificationRef,omitempty"` + SentReminders []reminder.SentReminder `json:"sentReminders,omitempty"` } // +kubebuilder:object:root=true diff --git a/approval/api/v1/builder/builder.go b/approval/api/v1/builder/builder.go index a6e303fa6..6fc3b4bdd 100644 --- a/approval/api/v1/builder/builder.go +++ b/approval/api/v1/builder/builder.go @@ -192,7 +192,7 @@ func (b *approvalBuilder) Build(ctx context.Context) (finalResult ApprovalResult approvalReq.Spec.State = v1.ApprovalStateGranted if len(approvalReq.Spec.Decisions) == 0 { approvalReq.Spec.Decisions = append(approvalReq.Spec.Decisions, v1.Decision{ - Name: "System", + Name: v1.SystemDecisionName, Comment: v1.AutoApprovedComment, ResultingState: v1.ApprovalStateGranted, }) diff --git a/approval/api/v1/common_types.go b/approval/api/v1/common_types.go index 004744493..c375175c7 100644 --- a/approval/api/v1/common_types.go +++ b/approval/api/v1/common_types.go @@ -22,6 +22,9 @@ const ( ApprovalStrategyFourEyes ApprovalStrategy = "FourEyes" ) +// SystemDecisionName is the decision name used for system-generated decisions (auto-approval, expiration). +const SystemDecisionName = "System" + // AutoApprovedComment is the comment added to auto-approved ApprovalRequests. const AutoApprovedComment = "Auto-approved: The approval strategy does not require manual review." diff --git a/approval/api/v1/zz_generated.deepcopy.go b/approval/api/v1/zz_generated.deepcopy.go index 692db05c5..44707158a 100644 --- a/approval/api/v1/zz_generated.deepcopy.go +++ b/approval/api/v1/zz_generated.deepcopy.go @@ -9,6 +9,7 @@ package v1 import ( + "github.com/telekom/controlplane/common/pkg/reminder" "github.com/telekom/controlplane/common/pkg/types" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -105,8 +106,13 @@ func (in *ApprovalExpirationSpec) DeepCopyInto(out *ApprovalExpirationSpec) { *out = *in in.Approval.DeepCopyInto(&out.Approval) in.Expiration.DeepCopyInto(&out.Expiration) - in.WeeklyReminder.DeepCopyInto(&out.WeeklyReminder) - in.DailyReminder.DeepCopyInto(&out.DailyReminder) + if in.Thresholds != nil { + in, out := &in.Thresholds, &out.Thresholds + *out = make([]reminder.Threshold, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ApprovalExpirationSpec. @@ -129,13 +135,12 @@ func (in *ApprovalExpirationStatus) DeepCopyInto(out *ApprovalExpirationStatus) (*in)[i].DeepCopyInto(&(*out)[i]) } } - if in.LastReminder != nil { - in, out := &in.LastReminder, &out.LastReminder - *out = (*in).DeepCopy() - } - if in.LastNotificationRef != nil { - in, out := &in.LastNotificationRef, &out.LastNotificationRef - *out = (*in).DeepCopy() + if in.SentReminders != nil { + in, out := &in.SentReminders, &out.SentReminders + *out = make([]reminder.SentReminder, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } } diff --git a/approval/cmd/main.go b/approval/cmd/main.go index 59f7cb6a3..386c07171 100644 --- a/approval/cmd/main.go +++ b/approval/cmd/main.go @@ -87,9 +87,8 @@ func main() { os.Exit(1) } setupLog.Info("loaded expiration config", - "expirationPeriodMonths", expirationConfig.ExpirationPeriodMonths, - "weeklyReminderMonths", expirationConfig.LastMonthsWithWeeklyReminder, - "dailyReminderWeeks", expirationConfig.LastWeeksWithDailyReminder) + "expirationDuration", expirationConfig.ExpirationDuration, + "thresholds", len(expirationConfig.DefaultThresholds)) // if the enable-http2 flag is false (the default), http/2 should be disabled // due to its vulnerabilities. More specifically, disabling http/2 will @@ -134,8 +133,9 @@ func main() { } if err = (&controller.ApprovalReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + ExpirationConfig: expirationConfig, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Approval") os.Exit(1) @@ -148,9 +148,8 @@ func main() { os.Exit(1) } if err = (&controller.ApprovalExpirationReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - ExpirationConfig: expirationConfig, + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "ApprovalExpiration") os.Exit(1) diff --git a/approval/config/crd/bases/approval.cp.ei.telekom.de_approvalexpirations.yaml b/approval/config/crd/bases/approval.cp.ei.telekom.de_approvalexpirations.yaml index 8dd42d055..d80807210 100644 --- a/approval/config/crd/bases/approval.cp.ei.telekom.de_approvalexpirations.yaml +++ b/approval/config/crd/bases/approval.cp.ei.telekom.de_approvalexpirations.yaml @@ -69,25 +69,38 @@ spec: - name - namespace type: object - dailyReminder: - description: DailyReminder is the date from which daily reminders - start - format: date-time - type: string expiration: description: Expiration is the absolute date when the approval expires format: date-time type: string - weeklyReminder: - description: WeeklyReminder is the date from which weekly reminders - start - format: date-time - type: string + thresholds: + description: |- + Thresholds defines when reminders should be sent relative to the expiration deadline. + For example: [{Before: "720h"}, {Before: "168h", Repeat: "24h"}] sends one reminder + 30 days before expiration, then daily reminders starting 7 days before. + items: + description: Threshold defines when a reminder should fire relative + to a deadline. + properties: + before: + description: |- + Before is how long before the deadline this reminder fires. + For example, "720h" means 30 days before the deadline. + type: string + repeat: + description: |- + Repeat optionally re-sends the reminder at this interval once the + "Before" window is entered. For example, Before=168h + Repeat=24h + sends a reminder at 7d, 6d, 5d, … , 1d before the deadline. + If omitted, the reminder fires exactly once for this threshold. + type: string + required: + - before + type: object + type: array required: - approval - - dailyReminder - expiration - - weeklyReminder type: object status: description: ApprovalExpirationStatus defines the observed state of ApprovalExpiration @@ -151,29 +164,47 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map - lastNotificationRef: - description: LastNotificationRef is a reference to the last sent reminder - notification - properties: - name: - type: string - namespace: - type: string - uid: - description: |- - UID is a type that holds unique ID values, including UUIDs. Because we - don't ONLY use UUIDs, this is an alias to string. Being a type captures - intent and helps make sure that UIDs and names do not get conflated. - type: string - required: - - name - - namespace - type: object - lastReminder: - description: LastReminder is the timestamp when the last reminder - was sent - format: date-time - type: string + sentReminders: + description: SentReminders tracks which reminder thresholds have been + triggered and when + items: + description: SentReminder tracks a reminder that was sent for a + specific threshold. + properties: + ref: + description: Ref is the reference to the created notification + or other resource. + properties: + name: + type: string + namespace: + type: string + uid: + description: |- + UID is a type that holds unique ID values, including UUIDs. Because we + don't ONLY use UUIDs, this is an alias to string. Being a type captures + intent and helps make sure that UIDs and names do not get conflated. + type: string + required: + - name + - namespace + type: object + sentAt: + description: SentAt is when this reminder was last sent for + this threshold. + format: date-time + type: string + threshold: + description: |- + Threshold is the key identifying which threshold triggered this reminder. + It is the string representation of the Threshold.Before duration (e.g. "720h0m0s"). + type: string + required: + - ref + - sentAt + - threshold + type: object + type: array type: object type: object served: true diff --git a/approval/config/rbac/role.yaml b/approval/config/rbac/role.yaml index 4063e8718..30f52f15d 100644 --- a/approval/config/rbac/role.yaml +++ b/approval/config/rbac/role.yaml @@ -9,6 +9,7 @@ metadata: rules: - apiGroups: - "" + - events.k8s.io resources: - events verbs: diff --git a/approval/internal/condition/condition.go b/approval/internal/condition/condition.go index d87b61ff8..4940bfe3e 100644 --- a/approval/internal/condition/condition.go +++ b/approval/internal/condition/condition.go @@ -58,7 +58,7 @@ func NewSemigrantedCondition() metav1.Condition { func NewExpiredCondition() metav1.Condition { return metav1.Condition{ Type: "Approved", - Status: metav1.ConditionTrue, + Status: metav1.ConditionFalse, Reason: "Expired", Message: "Request has expired and requires re-approval", } diff --git a/approval/internal/condition/condition_test.go b/approval/internal/condition/condition_test.go new file mode 100644 index 000000000..ced427f9e --- /dev/null +++ b/approval/internal/condition/condition_test.go @@ -0,0 +1,115 @@ +// Copyright 2025 Deutsche Telekom IT GmbH +// +// SPDX-License-Identifier: Apache-2.0 + +package condition + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestConditions(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Condition Suite") +} + +var _ = Describe("Approval Conditions", func() { + // The "Approved" condition type should indicate whether the approval grants access + // Status=True means approved and access is granted + // Status=False means not approved or access is revoked + + Context("NewApprovedCondition", func() { + It("should return Approved=True for Granted state", func() { + cond := NewApprovedCondition() + Expect(cond.Type).To(Equal("Approved")) + Expect(cond.Status).To(Equal(metav1.ConditionTrue)) + Expect(cond.Reason).To(Equal("Granted")) + Expect(cond.Message).To(ContainSubstring("granted")) + }) + }) + + Context("NewRejectedCondition", func() { + It("should return Approved=False for Rejected state", func() { + cond := NewRejectedCondition() + Expect(cond.Type).To(Equal("Approved")) + Expect(cond.Status).To(Equal(metav1.ConditionFalse)) + Expect(cond.Reason).To(Equal("Rejected")) + Expect(cond.Message).To(ContainSubstring("rejected")) + }) + }) + + Context("NewPendingCondition", func() { + It("should return Approved=False for Pending state", func() { + cond := NewPendingCondition() + Expect(cond.Type).To(Equal("Approved")) + Expect(cond.Status).To(Equal(metav1.ConditionFalse)) + Expect(cond.Reason).To(Equal("Pending")) + Expect(cond.Message).To(ContainSubstring("pending")) + }) + }) + + Context("NewSemigrantedCondition", func() { + It("should return Approved=False for Semigranted state", func() { + cond := NewSemigrantedCondition() + Expect(cond.Type).To(Equal("Approved")) + Expect(cond.Status).To(Equal(metav1.ConditionFalse)) + Expect(cond.Reason).To(Equal("Semigranted")) + Expect(cond.Message).To(ContainSubstring("partially")) + }) + }) + + Context("NewSuspendedCondition", func() { + It("should return Approved=True for Suspended state", func() { + cond := NewSuspendedCondition() + Expect(cond.Type).To(Equal("Approved")) + Expect(cond.Status).To(Equal(metav1.ConditionTrue)) + Expect(cond.Reason).To(Equal("Suspended")) + Expect(cond.Message).To(ContainSubstring("suspended")) + }) + }) + + Context("NewExpiredCondition", func() { + It("should return Approved=False for Expired state", func() { + cond := NewExpiredCondition() + Expect(cond.Type).To(Equal("Approved")) + Expect(cond.Status).To(Equal(metav1.ConditionFalse), "Expired approvals should not be considered approved") + Expect(cond.Reason).To(Equal("Expired")) + Expect(cond.Message).To(ContainSubstring("expired")) + }) + }) + + // Semantic validation: states that grant access vs states that don't + Describe("Semantic Validation", func() { + It("should have Approved=True only for states that grant access", func() { + // States that GRANT access + grantingStates := []metav1.Condition{ + NewApprovedCondition(), // Granted + NewSuspendedCondition(), // Suspended (debatable, but currently True) + } + + for _, cond := range grantingStates { + Expect(cond.Status).To(Equal(metav1.ConditionTrue), + "State %s should have Approved=True", cond.Reason) + } + }) + + It("should have Approved=False for states that deny access", func() { + // States that DENY or DON'T YET grant access + denyingStates := []metav1.Condition{ + NewPendingCondition(), // Not yet approved + NewSemigrantedCondition(), // Partially approved (not enough) + NewRejectedCondition(), // Explicitly denied + NewExpiredCondition(), // No longer approved + } + + for _, cond := range denyingStates { + Expect(cond.Status).To(Equal(metav1.ConditionFalse), + "State %s should have Approved=False", cond.Reason) + } + }) + }) +}) diff --git a/approval/internal/config/expiration.go b/approval/internal/config/expiration.go index 266eec1d2..190796fd0 100644 --- a/approval/internal/config/expiration.go +++ b/approval/internal/config/expiration.go @@ -5,20 +5,22 @@ package config import ( + "time" + "github.com/pkg/errors" "github.com/spf13/viper" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/telekom/controlplane/common/pkg/reminder" ) // ExpirationConfig holds the configuration for approval expiration type ExpirationConfig struct { - // ExpirationPeriodMonths is the number of months until an approval expires - ExpirationPeriodMonths int - - // LastMonthsWithWeeklyReminder is the number of months before expiration when weekly reminders start - LastMonthsWithWeeklyReminder int + // ExpirationDuration is how long until an approval expires (from creation) + ExpirationDuration time.Duration - // LastWeeksWithDailyReminder is the number of weeks before expiration when daily reminders start - LastWeeksWithDailyReminder int + // DefaultThresholds defines the reminder schedule for approvals + DefaultThresholds []reminder.Threshold } // LoadExpirationConfig loads the expiration configuration from environment variables @@ -28,27 +30,52 @@ func LoadExpirationConfig() (*ExpirationConfig, error) { viper.AutomaticEnv() viper.SetEnvPrefix("APPROVAL") - config := &ExpirationConfig{ - ExpirationPeriodMonths: viper.GetInt("EXPIRATION_PERIOD_MONTHS"), - LastMonthsWithWeeklyReminder: viper.GetInt("EXPIRATION_WEEKLY_REMINDER_MONTHS"), - LastWeeksWithDailyReminder: viper.GetInt("EXPIRATION_DAILY_REMINDER_WEEKS"), - } + expirationMonths := viper.GetInt("EXPIRATION_PERIOD_MONTHS") + weeklyReminderMonths := viper.GetInt("EXPIRATION_WEEKLY_REMINDER_MONTHS") + dailyReminderWeeks := viper.GetInt("EXPIRATION_DAILY_REMINDER_WEEKS") - if config.ExpirationPeriodMonths <= 0 { + if expirationMonths <= 0 { return nil, errors.New("APPROVAL_EXPIRATION_PERIOD_MONTHS must be greater than 0") } - if config.LastMonthsWithWeeklyReminder < 0 { + if weeklyReminderMonths < 0 { return nil, errors.New("APPROVAL_EXPIRATION_WEEKLY_REMINDER_MONTHS must be >= 0") } - if config.LastWeeksWithDailyReminder < 0 { + if dailyReminderWeeks < 0 { return nil, errors.New("APPROVAL_EXPIRATION_DAILY_REMINDER_WEEKS must be >= 0") } - return config, nil + // Convert to durations + expirationDuration := time.Duration(expirationMonths) * 30 * 24 * time.Hour + + // Build default thresholds + var thresholds []reminder.Threshold + + // Weekly reminder threshold (if configured) + if weeklyReminderMonths > 0 { + weeklyBefore := time.Duration(weeklyReminderMonths) * 30 * 24 * time.Hour + thresholds = append(thresholds, reminder.Threshold{ + Before: metav1.Duration{Duration: weeklyBefore}, + }) + } + + // Daily reminder threshold (if configured) + if dailyReminderWeeks > 0 { + dailyBefore := time.Duration(dailyReminderWeeks) * 7 * 24 * time.Hour + dailyRepeat := metav1.Duration{Duration: 24 * time.Hour} + thresholds = append(thresholds, reminder.Threshold{ + Before: metav1.Duration{Duration: dailyBefore}, + Repeat: &dailyRepeat, + }) + } + + return &ExpirationConfig{ + ExpirationDuration: expirationDuration, + DefaultThresholds: thresholds, + }, nil } func setExpirationConfigDefaults() { viper.SetDefault("EXPIRATION_PERIOD_MONTHS", 12) - viper.SetDefault("EXPIRATION_WEEKLY_REMINDER_MONTHS", 2) - viper.SetDefault("EXPIRATION_DAILY_REMINDER_WEEKS", 2) + viper.SetDefault("EXPIRATION_WEEKLY_REMINDER_MONTHS", 1) + viper.SetDefault("EXPIRATION_DAILY_REMINDER_WEEKS", 1) } diff --git a/approval/internal/controller/approval_controller.go b/approval/internal/controller/approval_controller.go index 36b306878..86a5d5a70 100644 --- a/approval/internal/controller/approval_controller.go +++ b/approval/internal/controller/approval_controller.go @@ -14,6 +14,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller" approvalv1 "github.com/telekom/controlplane/approval/api/v1" + "github.com/telekom/controlplane/approval/internal/config" approval_handler "github.com/telekom/controlplane/approval/internal/handler/approval" cconfig "github.com/telekom/controlplane/common/pkg/config" cc "github.com/telekom/controlplane/common/pkg/controller" @@ -22,8 +23,9 @@ import ( // ApprovalReconciler reconciles a Approval object type ApprovalReconciler struct { client.Client - Scheme *runtime.Scheme - Recorder record.EventRecorder + Scheme *runtime.Scheme + Recorder record.EventRecorder + ExpirationConfig *config.ExpirationConfig cc.Controller[*approvalv1.Approval] } @@ -42,7 +44,8 @@ func (r *ApprovalReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c // SetupWithManager sets up the controller with the Manager. func (r *ApprovalReconciler) SetupWithManager(mgr ctrl.Manager) error { r.Recorder = mgr.GetEventRecorderFor("approval-controller") - r.Controller = cc.NewController(&approval_handler.ApprovalHandler{}, r.Client, r.Recorder) + handler := approval_handler.NewHandler(r.Client, r.ExpirationConfig) + r.Controller = cc.NewController(handler, r.Client, r.Recorder) return ctrl.NewControllerManagedBy(mgr). For(&approvalv1.Approval{}). diff --git a/approval/internal/controller/approvalexpiration_controller.go b/approval/internal/controller/approvalexpiration_controller.go index 6eb565a29..31e2af559 100644 --- a/approval/internal/controller/approvalexpiration_controller.go +++ b/approval/internal/controller/approvalexpiration_controller.go @@ -8,13 +8,13 @@ import ( "context" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/events" "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" approvalv1 "github.com/telekom/controlplane/approval/api/v1" - "github.com/telekom/controlplane/approval/internal/config" "github.com/telekom/controlplane/approval/internal/handler/approvalexpiration" cconfig "github.com/telekom/controlplane/common/pkg/config" cc "github.com/telekom/controlplane/common/pkg/controller" @@ -23,9 +23,9 @@ import ( // ApprovalExpirationReconciler reconciles an ApprovalExpiration object type ApprovalExpirationReconciler struct { client.Client - Scheme *runtime.Scheme - Recorder record.EventRecorder - ExpirationConfig *config.ExpirationConfig + Scheme *runtime.Scheme + Recorder record.EventRecorder // Deprecated: kept for common controller compatibility + EventRecorder events.EventRecorder // New events API cc.Controller[*approvalv1.ApprovalExpiration] } @@ -36,6 +36,8 @@ type ApprovalExpirationReconciler struct { // +kubebuilder:rbac:groups=approval.cp.ei.telekom.de,resources=approvals,verbs=get;list;watch;update;patch // +kubebuilder:rbac:groups=notification.cp.ei.telekom.de,resources=notifications,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=notification.cp.ei.telekom.de,resources=notificationchannels,verbs=get;list;watch +// +kubebuilder:rbac:groups=events.k8s.io,resources=events,verbs=create;patch +// +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // Reconcile is part of the main kubernetes reconciliation loop func (r *ApprovalExpirationReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { @@ -44,8 +46,12 @@ func (r *ApprovalExpirationReconciler) Reconcile(ctx context.Context, req ctrl.R // SetupWithManager sets up the controller with the Manager. func (r *ApprovalExpirationReconciler) SetupWithManager(mgr ctrl.Manager) error { - r.Recorder = mgr.GetEventRecorderFor("approvalexpiration-controller") - handler := approvalexpiration.NewHandler(r.Client, r.ExpirationConfig) + // Use new events API (k8s.io/client-go/tools/events) + r.EventRecorder = mgr.GetEventRecorder("approvalexpiration-controller") + // TODO: Migrate common controller to use events.EventRecorder, then remove this adapter + r.Recorder = &eventsRecorderAdapter{events: r.EventRecorder} + + handler := approvalexpiration.NewHandler(r.Client) r.Controller = cc.NewController(handler, r.Client, r.Recorder) return ctrl.NewControllerManagedBy(mgr). @@ -56,3 +62,22 @@ func (r *ApprovalExpirationReconciler) SetupWithManager(mgr ctrl.Manager) error }). Complete(r) } + +// 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 { + events events.EventRecorder +} + +func (a *eventsRecorderAdapter) Event(object runtime.Object, eventtype, reason, message string) { + a.events.Eventf(object, nil, eventtype, reason, "Event", message) +} + +func (a *eventsRecorderAdapter) Eventf(object runtime.Object, eventtype, reason, messageFmt string, args ...interface{}) { + a.events.Eventf(object, nil, eventtype, reason, "Event", messageFmt, args...) +} + +func (a *eventsRecorderAdapter) AnnotatedEventf(object runtime.Object, annotations map[string]string, eventtype, reason, messageFmt string, args ...interface{}) { + // events.EventRecorder doesn't support annotations in the same way, just forward to Eventf + a.events.Eventf(object, nil, eventtype, reason, "Event", messageFmt, args...) +} diff --git a/approval/internal/controller/approvalexpiration_controller_test.go b/approval/internal/controller/approvalexpiration_controller_test.go index 5e1070f82..fd47ef204 100644 --- a/approval/internal/controller/approvalexpiration_controller_test.go +++ b/approval/internal/controller/approvalexpiration_controller_test.go @@ -144,11 +144,12 @@ var _ = Describe("ApprovalExpiration Controller", Ordered, func() { g.Expect(ae.OwnerReferences[0].Kind).To(Equal("Approval")) g.Expect(*ae.OwnerReferences[0].Controller).To(BeTrue()) - By("checking ApprovalExpiration has expiration dates set") + By("checking ApprovalExpiration has expiration and thresholds set") g.Expect(ae.Spec.Expiration.Time).To(BeTemporally(">", time.Now())) - g.Expect(ae.Spec.WeeklyReminder.Time).To(BeTemporally("<", ae.Spec.Expiration.Time)) - g.Expect(ae.Spec.DailyReminder.Time).To(BeTemporally("<", ae.Spec.Expiration.Time)) - g.Expect(ae.Spec.DailyReminder.Time).To(BeTemporally(">", ae.Spec.WeeklyReminder.Time)) + g.Expect(ae.Spec.Thresholds).To(HaveLen(2)) + g.Expect(ae.Spec.Thresholds[0].Before.Duration).To(BeNumerically(">", 0)) + g.Expect(ae.Spec.Thresholds[1].Before.Duration).To(BeNumerically(">", 0)) + g.Expect(ae.Spec.Thresholds[1].Repeat).NotTo(BeNil()) By("checking ApprovalExpiration references the Approval") g.Expect(ae.Spec.Approval.Name).To(Equal(resourceName)) diff --git a/approval/internal/controller/suite_test.go b/approval/internal/controller/suite_test.go index 7753c886f..75bae8b24 100644 --- a/approval/internal/controller/suite_test.go +++ b/approval/internal/controller/suite_test.go @@ -25,6 +25,7 @@ import ( approvalv1 "github.com/telekom/controlplane/approval/api/v1" "github.com/telekom/controlplane/approval/internal/config" + "github.com/telekom/controlplane/common/pkg/reminder" "github.com/telekom/controlplane/common/pkg/test" "github.com/telekom/controlplane/common/pkg/test/mock" "github.com/telekom/controlplane/common/pkg/test/testutil" @@ -121,10 +122,24 @@ var _ = BeforeSuite(func() { }) Expect(err).ToNot(HaveOccurred()) + // Create test expiration config (needed by ApprovalReconciler and ApprovalExpirationReconciler) + weeklyBefore := 30 * 24 * time.Hour + dailyBefore := 7 * 24 * time.Hour + dailyRepeat := 24 * time.Hour + + expirationConfig := &config.ExpirationConfig{ + ExpirationDuration: 12 * 30 * 24 * time.Hour, // 12 months + DefaultThresholds: []reminder.Threshold{ + {Before: metav1.Duration{Duration: weeklyBefore}}, + {Before: metav1.Duration{Duration: dailyBefore}, Repeat: &metav1.Duration{Duration: dailyRepeat}}, + }, + } + err = (&ApprovalReconciler{ - Client: k8sManager.GetClient(), - Scheme: k8sManager.GetScheme(), - Recorder: &mock.EventRecorder{}, + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + Recorder: &mock.EventRecorder{}, + ExpirationConfig: expirationConfig, }).SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) @@ -136,17 +151,10 @@ var _ = BeforeSuite(func() { }).SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) - expirationConfig := &config.ExpirationConfig{ - ExpirationPeriodMonths: 12, - LastMonthsWithWeeklyReminder: 2, - LastWeeksWithDailyReminder: 2, - } - err = (&ApprovalExpirationReconciler{ - Client: k8sManager.GetClient(), - Scheme: k8sManager.GetScheme(), - Recorder: &mock.EventRecorder{}, - ExpirationConfig: expirationConfig, + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + Recorder: &mock.EventRecorder{}, }).SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) diff --git a/approval/internal/handler/approval/handler.go b/approval/internal/handler/approval/handler.go index 73099c92b..3f0f61855 100644 --- a/approval/internal/handler/approval/handler.go +++ b/approval/internal/handler/approval/handler.go @@ -17,6 +17,7 @@ import ( approvalv1 "github.com/telekom/controlplane/approval/api/v1" approval_condition "github.com/telekom/controlplane/approval/internal/condition" + "github.com/telekom/controlplane/approval/internal/config" "github.com/telekom/controlplane/approval/internal/handler/util" commonclient "github.com/telekom/controlplane/common/pkg/client" "github.com/telekom/controlplane/common/pkg/condition" @@ -27,7 +28,16 @@ import ( var _ handler.Handler[*approvalv1.Approval] = &ApprovalHandler{} -type ApprovalHandler struct{} +type ApprovalHandler struct { + cfg *config.ExpirationConfig +} + +// NewHandler creates a new ApprovalHandler with the provided configuration +func NewHandler(client client.Client, cfg *config.ExpirationConfig) *ApprovalHandler { + return &ApprovalHandler{ + cfg: cfg, + } +} func (h *ApprovalHandler) CreateOrUpdate(ctx context.Context, approval *approvalv1.Approval) error { // handle the notifications first @@ -149,7 +159,7 @@ func (h *ApprovalHandler) Delete(ctx context.Context, approval *approvalv1.Appro // filterSystemActions removes system-only actions (Expire) from the available transitions func filterSystemActions(transitions approvalv1.AvailableTransitions) approvalv1.AvailableTransitions { - filtered := make(approvalv1.AvailableTransitions, 0, len(transitions)) + var filtered approvalv1.AvailableTransitions for _, t := range transitions { if t.Action != approvalv1.ApprovalActionExpire { filtered = append(filtered, t) @@ -172,7 +182,7 @@ func (h *ApprovalHandler) handleExpiration(ctx context.Context, approval *approv if stateChanged { // Create or update ApprovalExpiration with fresh dates // (covers initial GRANTED and REALLOW from EXPIRED) - return createOrUpdateApprovalExpiration(ctx, c, approval) + return createOrUpdateApprovalExpiration(ctx, c, approval, h.cfg) } // State unchanged, leave ApprovalExpiration alone @@ -198,17 +208,9 @@ func (h *ApprovalHandler) handleExpiration(ctx context.Context, approval *approv } // createOrUpdateApprovalExpiration creates or updates an ApprovalExpiration with fresh dates -func createOrUpdateApprovalExpiration(ctx context.Context, c commonclient.JanitorClient, approval *approvalv1.Approval) error { - // TODO: Load expiration config from environment or use defaults - // For now, hardcode defaults: 12 months expiration, 2 months weekly, 2 weeks daily - expirationPeriodMonths := 12 - weeklyReminderMonths := 2 - dailyReminderWeeks := 2 - +func createOrUpdateApprovalExpiration(ctx context.Context, c commonclient.JanitorClient, approval *approvalv1.Approval, cfg *config.ExpirationConfig) error { now := time.Now() - expirationDate := now.AddDate(0, expirationPeriodMonths, 0) - weeklyReminderDate := expirationDate.AddDate(0, -weeklyReminderMonths, 0) - dailyReminderDate := expirationDate.AddDate(0, 0, -dailyReminderWeeks*7) + expirationDate := now.Add(cfg.ExpirationDuration) ae := &approvalv1.ApprovalExpiration{ ObjectMeta: metav1.ObjectMeta{ @@ -226,9 +228,8 @@ func createOrUpdateApprovalExpiration(ctx context.Context, c commonclient.Janito Name: approval.Name, Namespace: approval.Namespace, }, - Expiration: metav1.Time{Time: expirationDate}, - WeeklyReminder: metav1.Time{Time: weeklyReminderDate}, - DailyReminder: metav1.Time{Time: dailyReminderDate}, + Expiration: metav1.Time{Time: expirationDate}, + Thresholds: cfg.DefaultThresholds, } return nil } diff --git a/approval/internal/handler/approvalexpiration/handler.go b/approval/internal/handler/approvalexpiration/handler.go index dc351f5a6..813e455e7 100644 --- a/approval/internal/handler/approvalexpiration/handler.go +++ b/approval/internal/handler/approvalexpiration/handler.go @@ -15,22 +15,21 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" v1 "github.com/telekom/controlplane/approval/api/v1" - "github.com/telekom/controlplane/approval/internal/config" "github.com/telekom/controlplane/approval/internal/handler/util" - ctrlerrors "github.com/telekom/controlplane/common/pkg/errors/ctrlerrors" + "github.com/telekom/controlplane/common/pkg/reminder" + "github.com/telekom/controlplane/common/pkg/types" + "github.com/telekom/controlplane/common/pkg/util/contextutil" ) // Handler handles ApprovalExpiration resources type Handler struct { client client.Client - config *config.ExpirationConfig } // NewHandler creates a new ApprovalExpiration handler -func NewHandler(c client.Client, cfg *config.ExpirationConfig) *Handler { +func NewHandler(c client.Client) *Handler { return &Handler{ client: c, - config: cfg, } } @@ -53,24 +52,45 @@ func (h *Handler) CreateOrUpdate(ctx context.Context, ae *v1.ApprovalExpiration) now := time.Now() - // Check if expired + // Check if expired — transition to EXPIRED and return immediately. + // No notification is sent here because the Approval handler's handleNotifications + // already sends state-change notifications when it processes the GRANTED→EXPIRED transition. if now.After(ae.Spec.Expiration.Time) || now.Equal(ae.Spec.Expiration.Time) { logger.Info("Approval expired, transitioning to EXPIRED state", "expiration", ae.Spec.Expiration.Time) return h.ensureApprovalExpired(ctx, approval) } - // Check if reminder should be sent - if h.shouldRemind(ae, now) { - if err := h.sendReminder(ctx, ae, approval, now); err != nil { + // Evaluate which reminder (if any) should fire now + pending := reminder.Evaluate(ae.Spec.Expiration.Time, ae.Spec.Thresholds, ae.Status.SentReminders, now) + if len(pending) > 0 { + p := pending[0] + logger.V(1).Info("Sending reminder", "threshold", p.Key) + + // Send notification + notificationRef, err := h.sendReminder(ctx, ae, approval, now, p.Threshold) + if err != nil { return errors.Wrap(err, "failed to send reminder") } - ae.Status.LastReminder = &metav1.Time{Time: now} - logger.V(1).Info("Reminder sent", "lastReminder", now) + + // Track that this reminder was sent + ae.Status.SentReminders = reminder.UpsertSent(ae.Status.SentReminders, &reminder.SentReminder{ + Threshold: p.Key, + Ref: *notificationRef, + SentAt: metav1.NewTime(now), + }) } - // Schedule next reconciliation - return h.requeueAtNextEvent(ae, now) + // Schedule next reconciliation via the happy-path requeue mechanism. + // We use SetRequeueAfter instead of returning an error to avoid the common + // controller marking the object as NotReady and emitting Warning events. + delay := reminder.NextRequeue(ae.Spec.Expiration.Time, ae.Spec.Thresholds, ae.Status.SentReminders, now) + if delay <= 0 { + delay = time.Second // Immediate retry + } + logger.V(1).Info("Scheduling next expiration check", "requeueAfter", delay) + contextutil.SetRequeueAfter(ctx, delay) + return nil } // Delete handles cleanup when an ApprovalExpiration is deleted @@ -92,26 +112,6 @@ func (h *Handler) getParentApproval(ctx context.Context, ae *v1.ApprovalExpirati return approval, nil } -// shouldRemind determines if a reminder should be sent based on the current time and last reminder -func (h *Handler) shouldRemind(ae *v1.ApprovalExpiration, now time.Time) bool { - lastReminder := ae.Status.LastReminder - - expired := now.After(ae.Spec.Expiration.Time) || now.Equal(ae.Spec.Expiration.Time) - - if expired || now.After(ae.Spec.DailyReminder.Time) { - // Daily: send if never reminded OR last reminder was >= 1 day ago - return lastReminder == nil || now.After(lastReminder.Add(24*time.Hour)) - } - - if now.After(ae.Spec.WeeklyReminder.Time) { - // Weekly: send if never reminded OR last reminder was >= 1 week ago - return lastReminder == nil || now.After(lastReminder.Add(7*24*time.Hour)) - } - - // Too early - return false -} - // ensureApprovalExpired transitions the parent Approval to EXPIRED state func (h *Handler) ensureApprovalExpired(ctx context.Context, approval *v1.Approval) error { if approval.Spec.State == v1.ApprovalStateExpired { @@ -120,7 +120,7 @@ func (h *Handler) ensureApprovalExpired(ctx context.Context, approval *v1.Approv // Add system Decision (matches auto-approval pattern) approval.Spec.Decisions = append(approval.Spec.Decisions, v1.Decision{ - Name: "System", + Name: v1.SystemDecisionName, Comment: "Automatically expired - expiration date reached. Available actions: Allow (re-approve), Deny (reject).", Timestamp: &metav1.Time{Time: time.Now()}, ResultingState: v1.ApprovalStateExpired, @@ -138,110 +138,52 @@ func (h *Handler) ensureApprovalExpired(ctx context.Context, approval *v1.Approv } // sendReminder sends a reminder notification for the approaching or reached expiration -func (h *Handler) sendReminder(ctx context.Context, ae *v1.ApprovalExpiration, approval *v1.Approval, now time.Time) error { - // Determine reminder type - var reminderType string - var daysRemaining int64 - - expired := now.After(ae.Spec.Expiration.Time) || now.Equal(ae.Spec.Expiration.Time) - switch { - case expired: - reminderType = "expired" +func (h *Handler) sendReminder(ctx context.Context, ae *v1.ApprovalExpiration, approval *v1.Approval, now time.Time, threshold reminder.Threshold) (*types.ObjectRef, error) { + // Calculate days remaining + daysRemaining := int64(ae.Spec.Expiration.Time.Sub(now).Hours() / 24) + if daysRemaining < 0 { daysRemaining = 0 - case now.After(ae.Spec.DailyReminder.Time): - reminderType = "daily" - daysRemaining = int64(ae.Spec.Expiration.Time.Sub(now).Hours() / 24) - default: - reminderType = "weekly" - daysRemaining = int64(ae.Spec.Expiration.Time.Sub(now).Hours() / 24) } - // Build notification data with template properties - notificationData := util.NotificationData{ - Owner: ae, - SendToChannelNamespace: approval.Namespace, - StateNew: string(approval.Spec.State), - StateOld: string(approval.Spec.State), // State hasn't changed yet for reminders - Target: &approval.Spec.Target, - Requester: &approval.Spec.Requester, - Decider: &approval.Spec.Decider, - Scenario: util.NotificationScenarioUpdated, - Action: approval.Spec.Action, - ExpirationDate: ae.Spec.Expiration.Format("2006-01-02"), - DaysRemaining: fmt.Sprintf("%d", daysRemaining), - ReminderType: reminderType, + // Determine reminder type based on threshold + var reminderType string + if threshold.Repeat != nil { + reminderType = "daily" // Repeating thresholds are daily + } else if daysRemaining == 0 { + reminderType = "expired" + } else { + reminderType = "weekly" // One-shot thresholds are weekly } - // Send to decider + // Build base notification data with template properties + notificationData := util.NotificationData{ + Owner: ae, + StateNew: string(approval.Spec.State), + StateOld: string(approval.Spec.State), // State hasn't changed yet for reminders + Target: &approval.Spec.Target, + Requester: &approval.Spec.Requester, + Decider: &approval.Spec.Decider, + Scenario: util.NotificationScenarioUpdated, + Action: approval.Spec.Action, + ExpirationDate: ae.Spec.Expiration.Format("2006-01-02"), + DaysRemaining: fmt.Sprintf("%d", daysRemaining), + ReminderType: reminderType, + } + + // Send to decider (in decider's namespace) notificationData.Actor = util.ActorDecider - deciderRef, err := util.SendReminderNotification(ctx, h.client, ¬ificationData) - if err != nil { - return errors.Wrap(err, "failed to send decider reminder") + notificationData.SendToChannelNamespace = approval.Spec.Decider.ApplicationRef.Namespace + if _, err := util.SendReminderNotification(ctx, ¬ificationData); err != nil { + return nil, errors.Wrap(err, "failed to send decider reminder") } - // Send to requester + // Send to requester (in requester's namespace) notificationData.Actor = util.ActorRequester - requesterRef, err := util.SendReminderNotification(ctx, h.client, ¬ificationData) + notificationData.SendToChannelNamespace = approval.Spec.Requester.ApplicationRef.Namespace + requesterRef, err := util.SendReminderNotification(ctx, ¬ificationData) if err != nil { - return errors.Wrap(err, "failed to send requester reminder") - } - - // Store last notification ref (we'll keep the requester one) - ae.Status.LastNotificationRef = requesterRef - - _ = deciderRef // Used for sending, but we only store one ref - - return nil -} - -// requeueAtNextEvent schedules the next reconciliation at the appropriate time -func (h *Handler) requeueAtNextEvent(ae *v1.ApprovalExpiration, now time.Time) error { - var nextEvent time.Time - - switch { - case now.Before(ae.Spec.WeeklyReminder.Time): - // Not yet in reminder period - nextEvent = ae.Spec.WeeklyReminder.Time - - case now.Before(ae.Spec.DailyReminder.Time): - // In weekly reminder period - if ae.Status.LastReminder != nil { - nextWeekly := ae.Status.LastReminder.Add(7 * 24 * time.Hour) - if nextWeekly.Before(ae.Spec.DailyReminder.Time) { - nextEvent = nextWeekly - } else { - nextEvent = ae.Spec.DailyReminder.Time - } - } else { - nextEvent = now // Should remind now - } - - case now.Before(ae.Spec.Expiration.Time): - // In daily reminder period - if ae.Status.LastReminder != nil { - nextDaily := ae.Status.LastReminder.Add(24 * time.Hour) - if nextDaily.Before(ae.Spec.Expiration.Time) { - nextEvent = nextDaily - } else { - nextEvent = ae.Spec.Expiration.Time - } - } else { - nextEvent = now - } - - default: - // Already expired - requeue in 1 day for continued daily reminders - if ae.Status.LastReminder != nil { - nextEvent = ae.Status.LastReminder.Add(24 * time.Hour) - } else { - nextEvent = now - } - } - - delay := nextEvent.Sub(now) - if delay <= 0 { - delay = time.Second // Immediate retry + return nil, errors.Wrap(err, "failed to send requester reminder") } - return ctrlerrors.RetryableWithDelayErrorf(delay, "next expiration event at %s", nextEvent.Format(time.RFC3339)) + return requesterRef, nil } diff --git a/approval/internal/handler/approvalexpiration/handler_test.go b/approval/internal/handler/approvalexpiration/handler_test.go index 2d8bad007..301d235dd 100644 --- a/approval/internal/handler/approvalexpiration/handler_test.go +++ b/approval/internal/handler/approvalexpiration/handler_test.go @@ -8,13 +8,13 @@ import ( "testing" "time" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" v1 "github.com/telekom/controlplane/approval/api/v1" "github.com/telekom/controlplane/approval/internal/config" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" + "github.com/telekom/controlplane/common/pkg/reminder" ) func TestApprovalExpirationHandler(t *testing.T) { @@ -23,215 +23,54 @@ func TestApprovalExpirationHandler(t *testing.T) { } var _ = Describe("ApprovalExpiration Handler", func() { - var handler *Handler var cfg *config.ExpirationConfig BeforeEach(func() { + // Create config with default thresholds + weeklyBefore := 30 * 24 * time.Hour + dailyBefore := 7 * 24 * time.Hour + dailyRepeat := 24 * time.Hour + cfg = &config.ExpirationConfig{ - ExpirationPeriodMonths: 12, - LastMonthsWithWeeklyReminder: 2, - LastWeeksWithDailyReminder: 2, - } - handler = &Handler{ - client: nil, // Not needed for unit tests - config: cfg, + ExpirationDuration: 12 * 30 * 24 * time.Hour, // 12 months + DefaultThresholds: []reminder.Threshold{ + {Before: metav1.Duration{Duration: weeklyBefore}}, + {Before: metav1.Duration{Duration: dailyBefore}, Repeat: &metav1.Duration{Duration: dailyRepeat}}, + }, } }) - Describe("shouldRemind", func() { - var ae *v1.ApprovalExpiration - var now time.Time - - BeforeEach(func() { - now = time.Date(2026, 5, 1, 12, 0, 0, 0, time.UTC) - // Realistic timeline: expiration in 3 months - expirationDate := now.AddDate(0, 3, 0) // 3 months from now (Aug 1) - weeklyReminderDate := now.AddDate(0, 1, 0) // 1 month from now (Jun 1) - last 2 months - dailyReminderDate := now.AddDate(0, 2, 17) // 2 months 17 days from now (Jul 18) - last 2 weeks - - ae = &v1.ApprovalExpiration{ - Spec: v1.ApprovalExpirationSpec{ - Expiration: metav1.Time{Time: expirationDate}, - WeeklyReminder: metav1.Time{Time: weeklyReminderDate}, - DailyReminder: metav1.Time{Time: dailyReminderDate}, - }, - Status: v1.ApprovalExpirationStatus{}, - } - }) - - Context("when before weekly reminder period", func() { - It("should not send reminder", func() { - testNow := ae.Spec.WeeklyReminder.Add(-24 * time.Hour) // 1 day before weekly starts - result := handler.shouldRemind(ae, testNow) - Expect(result).To(BeFalse()) - }) - }) - - Context("when in weekly reminder period", func() { - It("should send reminder if never reminded", func() { - testNow := ae.Spec.WeeklyReminder.Add(1 * time.Hour) - ae.Status.LastReminder = nil - result := handler.shouldRemind(ae, testNow) - Expect(result).To(BeTrue()) - }) - - It("should send reminder if last reminded over a week ago", func() { - testNow := ae.Spec.WeeklyReminder.Add(8 * 24 * time.Hour) // 8 days after weekly start - lastReminder := testNow.Add(-8 * 24 * time.Hour) // 8 days ago - ae.Status.LastReminder = &metav1.Time{Time: lastReminder} - result := handler.shouldRemind(ae, testNow) - Expect(result).To(BeTrue()) - }) - - It("should not send reminder if last reminded less than a week ago", func() { - // Still in weekly period, not yet in daily period - testNow := ae.Spec.WeeklyReminder.Add(10 * 24 * time.Hour) // 10 days after weekly start, still before daily - lastReminder := testNow.Add(-5 * 24 * time.Hour) // 5 days ago - ae.Status.LastReminder = &metav1.Time{Time: lastReminder} - result := handler.shouldRemind(ae, testNow) - Expect(result).To(BeFalse()) - }) - }) - - Context("when in daily reminder period", func() { - It("should send reminder if never reminded", func() { - testNow := ae.Spec.DailyReminder.Add(1 * time.Hour) - ae.Status.LastReminder = nil - result := handler.shouldRemind(ae, testNow) - Expect(result).To(BeTrue()) - }) - - It("should send reminder if last reminded over a day ago", func() { - testNow := ae.Spec.DailyReminder.Add(2 * 24 * time.Hour) // 2 days after daily start - lastReminder := testNow.Add(-25 * time.Hour) // 25 hours ago - ae.Status.LastReminder = &metav1.Time{Time: lastReminder} - result := handler.shouldRemind(ae, testNow) - Expect(result).To(BeTrue()) - }) - - It("should not send reminder if last reminded less than a day ago", func() { - testNow := ae.Spec.DailyReminder.Add(2 * 24 * time.Hour) // 2 days after daily start - lastReminder := testNow.Add(-20 * time.Hour) // 20 hours ago - ae.Status.LastReminder = &metav1.Time{Time: lastReminder} - result := handler.shouldRemind(ae, testNow) - Expect(result).To(BeFalse()) - }) - }) - - Context("when expired", func() { - It("should send reminder if never reminded", func() { - testNow := ae.Spec.Expiration.Add(1 * time.Hour) // 1 hour after expiration - ae.Status.LastReminder = nil - result := handler.shouldRemind(ae, testNow) - Expect(result).To(BeTrue()) - }) - - It("should send reminder if last reminded over a day ago", func() { - testNow := ae.Spec.Expiration.Add(3 * 24 * time.Hour) // 3 days after expiration - lastReminder := testNow.Add(-25 * time.Hour) // 25 hours ago - ae.Status.LastReminder = &metav1.Time{Time: lastReminder} - result := handler.shouldRemind(ae, testNow) - Expect(result).To(BeTrue()) - }) - - It("should not send reminder if last reminded less than a day ago", func() { - testNow := ae.Spec.Expiration.Add(3 * 24 * time.Hour) // 3 days after expiration - lastReminder := testNow.Add(-20 * time.Hour) // 20 hours ago - ae.Status.LastReminder = &metav1.Time{Time: lastReminder} - result := handler.shouldRemind(ae, testNow) - Expect(result).To(BeFalse()) - }) - }) - - Context("when exactly at expiration time", func() { - It("should send reminder", func() { - testNow := ae.Spec.Expiration.Time - ae.Status.LastReminder = nil - result := handler.shouldRemind(ae, testNow) - Expect(result).To(BeTrue()) - }) + Describe("config integration", func() { + It("should have valid default thresholds", func() { + Expect(cfg.DefaultThresholds).To(HaveLen(2)) + Expect(cfg.DefaultThresholds[0].Before.Duration).To(Equal(30 * 24 * time.Hour)) + Expect(cfg.DefaultThresholds[0].Repeat).To(BeNil()) + Expect(cfg.DefaultThresholds[1].Before.Duration).To(Equal(7 * 24 * time.Hour)) + Expect(cfg.DefaultThresholds[1].Repeat).NotTo(BeNil()) + Expect(cfg.DefaultThresholds[1].Repeat.Duration).To(Equal(24 * time.Hour)) }) }) - Describe("requeueAtNextEvent", func() { - var ae *v1.ApprovalExpiration - var now time.Time + Describe("reminder evaluation", func() { + It("should use common/pkg/reminder for scheduling", func() { + now := time.Date(2026, 5, 1, 12, 0, 0, 0, time.UTC) + expiration := now.Add(10 * 24 * time.Hour) // 10 days out - BeforeEach(func() { - now = time.Date(2026, 5, 1, 12, 0, 0, 0, time.UTC) - // Realistic timeline: expiration in 3 months - expirationDate := now.AddDate(0, 3, 0) // 3 months from now (Aug 1) - weeklyReminderDate := now.AddDate(0, 1, 0) // 1 month from now (Jun 1) - last 2 months - dailyReminderDate := now.AddDate(0, 2, 17) // 2 months 17 days from now (Jul 18) - last 2 weeks - - ae = &v1.ApprovalExpiration{ + ae := &v1.ApprovalExpiration{ Spec: v1.ApprovalExpirationSpec{ - Expiration: metav1.Time{Time: expirationDate}, - WeeklyReminder: metav1.Time{Time: weeklyReminderDate}, - DailyReminder: metav1.Time{Time: dailyReminderDate}, + Expiration: metav1.Time{Time: expiration}, + Thresholds: cfg.DefaultThresholds, + }, + Status: v1.ApprovalExpirationStatus{ + SentReminders: []reminder.SentReminder{}, }, - Status: v1.ApprovalExpirationStatus{}, } - }) - - Context("when before weekly reminder period", func() { - It("should requeue at weekly reminder time", func() { - testNow := ae.Spec.WeeklyReminder.Add(-1 * time.Hour) - err := handler.requeueAtNextEvent(ae, testNow) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("next expiration event")) - }) - }) - - Context("when in weekly reminder period", func() { - It("should requeue at next weekly interval if reminded", func() { - testNow := ae.Spec.WeeklyReminder.Add(8 * 24 * time.Hour) - lastReminder := testNow.Add(-1 * time.Hour) - ae.Status.LastReminder = &metav1.Time{Time: lastReminder} - err := handler.requeueAtNextEvent(ae, testNow) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("next expiration event")) - }) - - It("should requeue at daily reminder time if next weekly is after daily", func() { - testNow := ae.Spec.WeeklyReminder.Add(1 * 24 * time.Hour) - lastReminder := testNow.Add(-1 * time.Hour) - ae.Status.LastReminder = &metav1.Time{Time: lastReminder} - err := handler.requeueAtNextEvent(ae, testNow) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("next expiration event")) - }) - }) - - Context("when in daily reminder period", func() { - It("should requeue at next daily interval if reminded", func() { - testNow := ae.Spec.DailyReminder.Add(25 * time.Hour) - lastReminder := testNow.Add(-1 * time.Hour) - ae.Status.LastReminder = &metav1.Time{Time: lastReminder} - err := handler.requeueAtNextEvent(ae, testNow) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("next expiration event")) - }) - - It("should requeue at expiration time if next daily is after expiration", func() { - testNow := ae.Spec.Expiration.Add(-1 * time.Hour) - lastReminder := testNow.Add(-1 * time.Hour) - ae.Status.LastReminder = &metav1.Time{Time: lastReminder} - err := handler.requeueAtNextEvent(ae, testNow) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("next expiration event")) - }) - }) - Context("when expired", func() { - It("should requeue at next daily interval for continued reminders", func() { - testNow := ae.Spec.Expiration.Add(3 * 24 * time.Hour) - lastReminder := testNow.Add(-1 * time.Hour) - ae.Status.LastReminder = &metav1.Time{Time: lastReminder} - err := handler.requeueAtNextEvent(ae, testNow) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("next expiration event")) - }) + // 10 days out: we're in the weekly window (30 days before) but not yet in daily window (7 days before) + // reminder.Evaluate returns the tightest matching threshold + pending := reminder.Evaluate(ae.Spec.Expiration.Time, ae.Spec.Thresholds, ae.Status.SentReminders, now) + Expect(pending).To(HaveLen(1)) + Expect(pending[0].Key).To(Equal("720h0m0s")) // 30 days (weekly threshold) }) }) }) diff --git a/approval/internal/handler/util/notification.go b/approval/internal/handler/util/notification.go index b1f02a36a..a99be4b03 100644 --- a/approval/internal/handler/util/notification.go +++ b/approval/internal/handler/util/notification.go @@ -240,7 +240,7 @@ func initializeProperties() map[string]any { } // SendReminderNotification sends an expiration reminder notification -func SendReminderNotification(ctx context.Context, c client.Client, data *NotificationData) (*types.ObjectRef, error) { +func SendReminderNotification(ctx context.Context, data *NotificationData) (*types.ObjectRef, error) { properties := initializeProperties() properties[TemplatePlaceholderEnvironment] = contextutil.EnvFromContextOrDie(ctx) @@ -289,7 +289,7 @@ func SendReminderNotification(ctx context.Context, c client.Client, data *Notifi WithOwner(data.Owner). WithSender(notificationv1.SenderTypeSystem, "ApprovalService"). WithDefaultChannels(ctx, data.SendToChannelNamespace). - WithPurpose(purpose). + WithPurpose(strings.ToLower(purpose)). WithName(labelutil.NormalizeNameValue(name)). WithProperties(properties) diff --git a/approval/internal/webhook/v1/approval_webhook.go b/approval/internal/webhook/v1/approval_webhook.go index 5b85e3a37..7fca42266 100644 --- a/approval/internal/webhook/v1/approval_webhook.go +++ b/approval/internal/webhook/v1/approval_webhook.go @@ -6,6 +6,7 @@ package v1 import ( "context" + "strings" apierrors "k8s.io/apimachinery/pkg/api/errors" ctrl "sigs.k8s.io/controller-runtime" @@ -73,7 +74,7 @@ func (a *ApprovalCustomValidator) ValidateCreate(_ context.Context, obj *approva } // 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) { approvallog.Info("validate update", "name", newObj.Name) if newObj.Spec.Strategy == approvalv1.ApprovalStrategyAuto && newObj.Spec.State != approvalv1.ApprovalStateGranted { @@ -86,7 +87,7 @@ func (a *ApprovalCustomValidator) ValidateUpdate(_ context.Context, oldObj, newO // instead of Status.AvailableTransitions (which may be stale or nil before // the controller has reconciled). Auto strategy uses its own FSM. if stateChanged { - if validationErr := validateExpireTransition(newObj); validationErr != nil { + if validationErr := validateExpireTransition(ctx, newObj); validationErr != nil { return warnings, validationErr } @@ -129,17 +130,27 @@ func (a *ApprovalCustomValidator) ValidateDelete(_ context.Context, obj *approva return nil, nil } -// validateExpireTransition blocks manual transitions to EXPIRED state (system-only action) -func validateExpireTransition(newObj *approvalv1.Approval) error { +// controllerServiceAccountSuffix is the suffix of the service account used by the approval controller. +// The full username is "system:serviceaccount::controller-manager". +const controllerServiceAccountSuffix = "controller-manager" + +// validateExpireTransition blocks manual transitions to EXPIRED state. +// Only the controller service account is allowed to perform this transition. +func validateExpireTransition(ctx context.Context, 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 - } + // Verify the caller is the controller service account + req, err := admission.RequestFromContext(ctx) + if err != nil { + return apierrors.NewBadRequest("Expire action is system-only and cannot be triggered manually") + } + + username := req.UserInfo.Username + // Expected format: system:serviceaccount::controller-manager + if strings.HasPrefix(username, "system:serviceaccount:") && strings.HasSuffix(username, controllerServiceAccountSuffix) { + return nil } return apierrors.NewBadRequest("Expire action is system-only and cannot be triggered manually") diff --git a/approval/internal/webhook/v1/approval_webhook_test.go b/approval/internal/webhook/v1/approval_webhook_test.go index 5458e92d4..9cb98d0c0 100644 --- a/approval/internal/webhook/v1/approval_webhook_test.go +++ b/approval/internal/webhook/v1/approval_webhook_test.go @@ -8,7 +8,10 @@ import ( "context" "time" + admissionv1 "k8s.io/api/admission/v1" + authenticationv1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" approvalv1 "github.com/telekom/controlplane/approval/api/v1" @@ -16,6 +19,18 @@ import ( . "github.com/onsi/gomega" ) +// controllerContext returns a context that simulates a request from the controller service account. +func controllerContext() context.Context { + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + UserInfo: authenticationv1.UserInfo{ + Username: "system:serviceaccount:controlplane-system:approval-controller-manager", + }, + }, + } + return admission.NewContextWithRequest(context.Background(), req) +} + var _ = Describe("Approval Webhook", func() { Context("When creating Approval under Defaulting Webhook", func() { It("Should not modify non-Auto strategy approvals", func() { @@ -507,21 +522,21 @@ var _ = Describe("Approval Webhook", func() { Expect(err.Error()).To(ContainSubstring("Expire action is system-only")) }) - It("should allow System transition to EXPIRED", func() { + It("should allow controller service account to transition to EXPIRED", func() { oldObj := makeApproval(approvalv1.ApprovalStateGranted, nil) newObj := makeApproval(approvalv1.ApprovalStateExpired, []approvalv1.Decision{ {Name: "System", Email: "", Comment: "Automatically expired", ResultingState: approvalv1.ApprovalStateExpired}, }) - _, err := validator.ValidateUpdate(context.Background(), oldObj, newObj) + _, err := validator.ValidateUpdate(controllerContext(), oldObj, newObj) Expect(err).NotTo(HaveOccurred()) }) - It("should allow System transition to EXPIRED from SUSPENDED", func() { + It("should allow controller service account to transition to EXPIRED from SUSPENDED", func() { oldObj := makeApproval(approvalv1.ApprovalStateSuspended, nil) newObj := makeApproval(approvalv1.ApprovalStateExpired, []approvalv1.Decision{ {Name: "System", Email: "", Comment: "Automatically expired", ResultingState: approvalv1.ApprovalStateExpired}, }) - _, err := validator.ValidateUpdate(context.Background(), oldObj, newObj) + _, err := validator.ValidateUpdate(controllerContext(), oldObj, newObj) Expect(err).NotTo(HaveOccurred()) }) diff --git a/approval/internal/webhook/v1/approvalrequest_webhook.go b/approval/internal/webhook/v1/approvalrequest_webhook.go index 5704125cb..cbc1c4773 100644 --- a/approval/internal/webhook/v1/approvalrequest_webhook.go +++ b/approval/internal/webhook/v1/approvalrequest_webhook.go @@ -52,7 +52,7 @@ func (ar *ApprovalRequestCustomDefaulter) Default(_ context.Context, obj *approv obj.Spec.State = approvalv1.ApprovalStateGranted if len(obj.Spec.Decisions) == 0 { obj.Spec.Decisions = append(obj.Spec.Decisions, approvalv1.Decision{ - Name: "System", + Name: approvalv1.SystemDecisionName, Comment: approvalv1.AutoApprovedComment, ResultingState: approvalv1.ApprovalStateGranted, }) From de68a0df4ecb8d109f6239f1fa716396cee80f10 Mon Sep 17 00:00:00 2001 From: Julius Malcovsky Date: Wed, 20 May 2026 08:26:53 +0200 Subject: [PATCH 4/5] chore(approval-expiration): fix linting issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Björn Kottner Co-authored-by: Ismael Garba Co-authored-by: Stefan Siber Co-authored-by: Ron Gummich --- approval/go.mod | 2 +- approval/internal/condition/condition_test.go | 3 ++- approval/internal/controller/approval_controller.go | 4 ++-- approval/internal/controller/approvalrequest_controller.go | 2 +- approval/internal/handler/approval/handler.go | 2 +- approval/internal/handler/approvalexpiration/handler.go | 7 ++++--- .../internal/handler/approvalexpiration/handler_test.go | 5 +++-- 7 files changed, 14 insertions(+), 11 deletions(-) diff --git a/approval/go.mod b/approval/go.mod index 47135c29e..2c22b0f4d 100644 --- a/approval/go.mod +++ b/approval/go.mod @@ -16,6 +16,7 @@ require ( github.com/onsi/ginkgo/v2 v2.28.3 github.com/onsi/gomega v1.40.0 github.com/pkg/errors v0.9.1 + github.com/spf13/viper v1.21.0 k8s.io/api v0.36.0 k8s.io/apimachinery v0.36.0 k8s.io/client-go v0.36.0 @@ -75,7 +76,6 @@ require ( github.com/spf13/cast v1.10.0 // indirect github.com/spf13/cobra v1.10.2 // indirect github.com/spf13/pflag v1.0.10 // indirect - github.com/spf13/viper v1.21.0 // indirect github.com/stoewer/go-strcase v1.3.0 // indirect github.com/subosito/gotenv v1.6.0 // indirect github.com/x448/float16 v0.8.4 // indirect diff --git a/approval/internal/condition/condition_test.go b/approval/internal/condition/condition_test.go index ced427f9e..72285e8ef 100644 --- a/approval/internal/condition/condition_test.go +++ b/approval/internal/condition/condition_test.go @@ -7,9 +7,10 @@ package condition import ( "testing" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestConditions(t *testing.T) { diff --git a/approval/internal/controller/approval_controller.go b/approval/internal/controller/approval_controller.go index 86a5d5a70..4da7f1ea2 100644 --- a/approval/internal/controller/approval_controller.go +++ b/approval/internal/controller/approval_controller.go @@ -2,7 +2,7 @@ // // SPDX-License-Identifier: Apache-2.0 -package controller //nolint:dupl // approval and approvalrequest controllers are intentionally similar +package controller import ( "context" @@ -44,7 +44,7 @@ func (r *ApprovalReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c // SetupWithManager sets up the controller with the Manager. func (r *ApprovalReconciler) SetupWithManager(mgr ctrl.Manager) error { r.Recorder = mgr.GetEventRecorderFor("approval-controller") - handler := approval_handler.NewHandler(r.Client, r.ExpirationConfig) + handler := approval_handler.NewHandler(r.ExpirationConfig) r.Controller = cc.NewController(handler, r.Client, r.Recorder) return ctrl.NewControllerManagedBy(mgr). diff --git a/approval/internal/controller/approvalrequest_controller.go b/approval/internal/controller/approvalrequest_controller.go index b7b8af335..4cac95365 100644 --- a/approval/internal/controller/approvalrequest_controller.go +++ b/approval/internal/controller/approvalrequest_controller.go @@ -2,7 +2,7 @@ // // SPDX-License-Identifier: Apache-2.0 -package controller //nolint:dupl // approval and approvalrequest controllers are intentionally similar +package controller import ( "context" diff --git a/approval/internal/handler/approval/handler.go b/approval/internal/handler/approval/handler.go index 3f0f61855..2fa28f643 100644 --- a/approval/internal/handler/approval/handler.go +++ b/approval/internal/handler/approval/handler.go @@ -33,7 +33,7 @@ type ApprovalHandler struct { } // NewHandler creates a new ApprovalHandler with the provided configuration -func NewHandler(client client.Client, cfg *config.ExpirationConfig) *ApprovalHandler { +func NewHandler(cfg *config.ExpirationConfig) *ApprovalHandler { return &ApprovalHandler{ cfg: cfg, } diff --git a/approval/internal/handler/approvalexpiration/handler.go b/approval/internal/handler/approvalexpiration/handler.go index 813e455e7..b7d93a93c 100644 --- a/approval/internal/handler/approvalexpiration/handler.go +++ b/approval/internal/handler/approvalexpiration/handler.go @@ -147,11 +147,12 @@ func (h *Handler) sendReminder(ctx context.Context, ae *v1.ApprovalExpiration, a // Determine reminder type based on threshold var reminderType string - if threshold.Repeat != nil { + switch { + case threshold.Repeat != nil: reminderType = "daily" // Repeating thresholds are daily - } else if daysRemaining == 0 { + case daysRemaining == 0: reminderType = "expired" - } else { + default: reminderType = "weekly" // One-shot thresholds are weekly } diff --git a/approval/internal/handler/approvalexpiration/handler_test.go b/approval/internal/handler/approvalexpiration/handler_test.go index 301d235dd..b3337adc7 100644 --- a/approval/internal/handler/approvalexpiration/handler_test.go +++ b/approval/internal/handler/approvalexpiration/handler_test.go @@ -8,13 +8,14 @@ import ( "testing" "time" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" v1 "github.com/telekom/controlplane/approval/api/v1" "github.com/telekom/controlplane/approval/internal/config" "github.com/telekom/controlplane/common/pkg/reminder" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" ) func TestApprovalExpirationHandler(t *testing.T) { From 3cebfa35c85cad8ae225172b2fbdd3a086bd66ac Mon Sep 17 00:00:00 2001 From: Julius Malcovsky Date: Wed, 20 May 2026 23:01:53 +0200 Subject: [PATCH 5/5] feat(approval-expiration): expired approvals are no longer treated as denied MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Björn Kottner Co-authored-by: Ismael Garba Co-authored-by: Stefan Siber Co-authored-by: Ron Gummich --- approval/api/v1/approval_types.go | 4 +- approval/api/v1/builder/builder.go | 7 +- approval/api/v1/common_types.go | 4 +- ...val.cp.ei.telekom.de_approvalrequests.yaml | 1 - .../approval.cp.ei.telekom.de_approvals.yaml | 3 - approval/internal/condition/condition.go | 6 +- approval/internal/condition/condition_test.go | 6 +- approval/internal/handler/approval/fsm.go | 14 +-- approval/internal/handler/approval/handler.go | 22 +---- .../handler/approvalexpiration/handler.go | 28 +++--- .../internal/webhook/v1/approval_webhook.go | 31 ------- .../webhook/v1/approval_webhook_test.go | 91 ------------------- 12 files changed, 35 insertions(+), 182 deletions(-) diff --git a/approval/api/v1/approval_types.go b/approval/api/v1/approval_types.go index 76b26297d..d1aacf373 100644 --- a/approval/api/v1/approval_types.go +++ b/approval/api/v1/approval_types.go @@ -37,7 +37,7 @@ type ApprovalSpec struct { Strategy ApprovalStrategy `json:"strategy"` // State defines the state of the approval - // +kubebuilder:validation:Enum=Pending;Semigranted;Granted;Rejected;Suspended;Expired + // +kubebuilder:validation:Enum=Pending;Semigranted;Granted;Rejected;Suspended // +kubebuilder:default=Pending State ApprovalState `json:"state"` @@ -57,7 +57,7 @@ type ApprovalStatus struct { AvailableTransitions AvailableTransitions `json:"availableTransitions,omitempty"` // LastState defines the last state of the approval - // +kubebuilder:validation:Enum=Pending;Semigranted;Granted;Rejected;Suspended;Expired + // +kubebuilder:validation:Enum=Pending;Semigranted;Granted;Rejected;Suspended // +kubebuilder:default=Pending LastState ApprovalState `json:"lastState,omitempty"` diff --git a/approval/api/v1/builder/builder.go b/approval/api/v1/builder/builder.go index 6fc3b4bdd..5ae4d6d78 100644 --- a/approval/api/v1/builder/builder.go +++ b/approval/api/v1/builder/builder.go @@ -242,12 +242,11 @@ func (b *approvalBuilder) Build(ctx context.Context) (finalResult ApprovalResult if approvalExists { log.V(2).Info("Approval exists") isDenied := b.Approval.Spec.State == v1.ApprovalStateRejected || - b.Approval.Spec.State == v1.ApprovalStateSuspended || - b.Approval.Spec.State == v1.ApprovalStateExpired + b.Approval.Spec.State == v1.ApprovalStateSuspended if isDenied { - log.V(1).Info("Approval is rejected, suspended, or expired and must not be provisioned") - b.Owner.SetCondition(newApprovalGrantedCondition(b.Approval.Spec.State, "Approval has been rejected, suspended, or expired")) + log.V(1).Info("Approval is rejected or suspended and must not be provisioned") + b.Owner.SetCondition(newApprovalGrantedCondition(b.Approval.Spec.State, "Approval has been rejected or suspended")) return ApprovalResultDenied, nil } } diff --git a/approval/api/v1/common_types.go b/approval/api/v1/common_types.go index c375175c7..7422da56a 100644 --- a/approval/api/v1/common_types.go +++ b/approval/api/v1/common_types.go @@ -35,7 +35,6 @@ const ( ApprovalActionDeny ApprovalAction = "Deny" ApprovalActionSuspend ApprovalAction = "Suspend" ApprovalActionResume ApprovalAction = "Resume" - ApprovalActionExpire ApprovalAction = "Expire" ) func (a ApprovalAction) String() string { @@ -50,7 +49,6 @@ const ( ApprovalStateGranted ApprovalState = "Granted" ApprovalStateRejected ApprovalState = "Rejected" ApprovalStateSuspended ApprovalState = "Suspended" - ApprovalStateExpired ApprovalState = "Expired" ) func (s ApprovalState) String() string { @@ -140,6 +138,6 @@ type Decision struct { // ResultingState is the state the resource transitioned to as a result of this decision. // Automatically set by the defaulting webhook to match Spec.State when not provided. // +kubebuilder:validation:Required - // +kubebuilder:validation:Enum=Pending;Semigranted;Granted;Rejected;Suspended;Expired + // +kubebuilder:validation:Enum=Pending;Semigranted;Granted;Rejected;Suspended ResultingState ApprovalState `json:"resultingState"` } diff --git a/approval/config/crd/bases/approval.cp.ei.telekom.de_approvalrequests.yaml b/approval/config/crd/bases/approval.cp.ei.telekom.de_approvalrequests.yaml index 0b9a83dae..bb396b03d 100644 --- a/approval/config/crd/bases/approval.cp.ei.telekom.de_approvalrequests.yaml +++ b/approval/config/crd/bases/approval.cp.ei.telekom.de_approvalrequests.yaml @@ -127,7 +127,6 @@ spec: - Granted - Rejected - Suspended - - Expired type: string timestamp: description: Timestamp of when the decision was made diff --git a/approval/config/crd/bases/approval.cp.ei.telekom.de_approvals.yaml b/approval/config/crd/bases/approval.cp.ei.telekom.de_approvals.yaml index 5db4cabe7..34744acb0 100644 --- a/approval/config/crd/bases/approval.cp.ei.telekom.de_approvals.yaml +++ b/approval/config/crd/bases/approval.cp.ei.telekom.de_approvals.yaml @@ -145,7 +145,6 @@ spec: - Granted - Rejected - Suspended - - Expired type: string timestamp: description: Timestamp of when the decision was made @@ -222,7 +221,6 @@ spec: - Granted - Rejected - Suspended - - Expired type: string strategy: default: Auto @@ -357,7 +355,6 @@ spec: - Granted - Rejected - Suspended - - Expired type: string notificationRefs: description: NotificationRefs is a reference to the notifications diff --git a/approval/internal/condition/condition.go b/approval/internal/condition/condition.go index 4940bfe3e..3c548a959 100644 --- a/approval/internal/condition/condition.go +++ b/approval/internal/condition/condition.go @@ -57,9 +57,9 @@ func NewSemigrantedCondition() metav1.Condition { func NewExpiredCondition() metav1.Condition { return metav1.Condition{ - Type: "Approved", - Status: metav1.ConditionFalse, + Type: "Expired", + Status: metav1.ConditionTrue, Reason: "Expired", - Message: "Request has expired and requires re-approval", + Message: "Approval has expired and requires re-approval", } } diff --git a/approval/internal/condition/condition_test.go b/approval/internal/condition/condition_test.go index 72285e8ef..bcb6a17a6 100644 --- a/approval/internal/condition/condition_test.go +++ b/approval/internal/condition/condition_test.go @@ -76,8 +76,8 @@ var _ = Describe("Approval Conditions", func() { Context("NewExpiredCondition", func() { It("should return Approved=False for Expired state", func() { cond := NewExpiredCondition() - Expect(cond.Type).To(Equal("Approved")) - Expect(cond.Status).To(Equal(metav1.ConditionFalse), "Expired approvals should not be considered approved") + Expect(cond.Type).To(Equal("Expired")) + Expect(cond.Status).To(Equal(metav1.ConditionTrue)) Expect(cond.Reason).To(Equal("Expired")) Expect(cond.Message).To(ContainSubstring("expired")) }) @@ -90,6 +90,7 @@ var _ = Describe("Approval Conditions", func() { grantingStates := []metav1.Condition{ NewApprovedCondition(), // Granted NewSuspendedCondition(), // Suspended (debatable, but currently True) + NewExpiredCondition(), // Expiry is informative only, approval itself is still approved } for _, cond := range grantingStates { @@ -104,7 +105,6 @@ var _ = Describe("Approval Conditions", func() { NewPendingCondition(), // Not yet approved NewSemigrantedCondition(), // Partially approved (not enough) NewRejectedCondition(), // Explicitly denied - NewExpiredCondition(), // No longer approved } for _, cond := range denyingStates { diff --git a/approval/internal/handler/approval/fsm.go b/approval/internal/handler/approval/fsm.go index 8d879fbea..3733d56a6 100644 --- a/approval/internal/handler/approval/fsm.go +++ b/approval/internal/handler/approval/fsm.go @@ -10,29 +10,25 @@ import ( ) var auto = fsm.Transitions{ - {Action: v1.ApprovalActionAllow, Src: []v1.ApprovalState{v1.ApprovalStateRejected, v1.ApprovalStateExpired}, Dst: v1.ApprovalStateGranted}, - {Action: v1.ApprovalActionDeny, Src: []v1.ApprovalState{v1.ApprovalStateGranted, v1.ApprovalStateExpired}, Dst: v1.ApprovalStateRejected}, + {Action: v1.ApprovalActionAllow, Src: []v1.ApprovalState{v1.ApprovalStateRejected}, Dst: v1.ApprovalStateGranted}, + {Action: v1.ApprovalActionDeny, Src: []v1.ApprovalState{v1.ApprovalStateGranted}, Dst: v1.ApprovalStateRejected}, {Action: v1.ApprovalActionSuspend, Src: []v1.ApprovalState{v1.ApprovalStateGranted}, Dst: v1.ApprovalStateSuspended}, {Action: v1.ApprovalActionResume, Src: []v1.ApprovalState{v1.ApprovalStateSuspended}, Dst: v1.ApprovalStateGranted}, - {Action: v1.ApprovalActionExpire, Src: []v1.ApprovalState{v1.ApprovalStateGranted, v1.ApprovalStateSuspended}, Dst: v1.ApprovalStateExpired}, } var simple = fsm.Transitions{ - {Action: v1.ApprovalActionAllow, Src: []v1.ApprovalState{v1.ApprovalStatePending, v1.ApprovalStateRejected, v1.ApprovalStateExpired}, Dst: v1.ApprovalStateGranted}, - {Action: v1.ApprovalActionDeny, Src: []v1.ApprovalState{v1.ApprovalStatePending, v1.ApprovalStateGranted, v1.ApprovalStateExpired}, Dst: v1.ApprovalStateRejected}, + {Action: v1.ApprovalActionAllow, Src: []v1.ApprovalState{v1.ApprovalStatePending, v1.ApprovalStateRejected}, Dst: v1.ApprovalStateGranted}, + {Action: v1.ApprovalActionDeny, Src: []v1.ApprovalState{v1.ApprovalStatePending, v1.ApprovalStateGranted}, Dst: v1.ApprovalStateRejected}, {Action: v1.ApprovalActionSuspend, Src: []v1.ApprovalState{v1.ApprovalStateGranted}, Dst: v1.ApprovalStateSuspended}, {Action: v1.ApprovalActionResume, Src: []v1.ApprovalState{v1.ApprovalStateSuspended}, Dst: v1.ApprovalStateGranted}, - {Action: v1.ApprovalActionExpire, Src: []v1.ApprovalState{v1.ApprovalStateGranted, v1.ApprovalStateSuspended}, Dst: v1.ApprovalStateExpired}, } var fourEyes = fsm.Transitions{ {Action: v1.ApprovalActionAllow, Src: []v1.ApprovalState{v1.ApprovalStatePending, v1.ApprovalStateRejected}, Dst: v1.ApprovalStateSemigranted}, {Action: v1.ApprovalActionAllow, Src: []v1.ApprovalState{v1.ApprovalStateSemigranted}, Dst: v1.ApprovalStateGranted}, - {Action: v1.ApprovalActionAllow, Src: []v1.ApprovalState{v1.ApprovalStateExpired}, Dst: v1.ApprovalStateGranted}, - {Action: v1.ApprovalActionDeny, Src: []v1.ApprovalState{v1.ApprovalStatePending, v1.ApprovalStateGranted, v1.ApprovalStateSemigranted, v1.ApprovalStateExpired}, Dst: v1.ApprovalStateRejected}, + {Action: v1.ApprovalActionDeny, Src: []v1.ApprovalState{v1.ApprovalStatePending, v1.ApprovalStateGranted, v1.ApprovalStateSemigranted}, Dst: v1.ApprovalStateRejected}, {Action: v1.ApprovalActionSuspend, Src: []v1.ApprovalState{v1.ApprovalStateGranted}, Dst: v1.ApprovalStateSuspended}, {Action: v1.ApprovalActionResume, Src: []v1.ApprovalState{v1.ApprovalStateSuspended}, Dst: v1.ApprovalStateGranted}, - {Action: v1.ApprovalActionExpire, Src: []v1.ApprovalState{v1.ApprovalStateGranted, v1.ApprovalStateSuspended}, Dst: v1.ApprovalStateExpired}, } var transitionMap = map[v1.ApprovalStrategy]fsm.Transitions{ diff --git a/approval/internal/handler/approval/handler.go b/approval/internal/handler/approval/handler.go index 2fa28f643..a648d8d29 100644 --- a/approval/internal/handler/approval/handler.go +++ b/approval/internal/handler/approval/handler.go @@ -48,8 +48,7 @@ func (h *ApprovalHandler) CreateOrUpdate(ctx context.Context, approval *approval } fsm := ApprovalStrategyFSM[approval.Spec.Strategy] - // Filter out system-only actions (Expire) from available transitions - approval.Status.AvailableTransitions = filterSystemActions(fsm.AvailableTransitions(approval.Spec.State)) + approval.Status.AvailableTransitions = fsm.AvailableTransitions(approval.Spec.State) // Capture state change BEFORE updating LastState (needed for expiration logic) stateChanged := approval.Spec.State != approval.Status.LastState @@ -81,11 +80,6 @@ func (h *ApprovalHandler) CreateOrUpdate(ctx context.Context, approval *approval approval.SetCondition(condition.NewProcessingCondition("Suspended", "Approval is suspended")) approval.SetCondition(condition.NewReadyCondition("Suspended", "Approval is suspended")) - case approvalv1.ApprovalStateExpired: - approval.SetCondition(approval_condition.NewExpiredCondition()) - approval.SetCondition(condition.NewProcessingCondition("Expired", "Approval has expired")) - approval.SetCondition(condition.NewReadyCondition("Expired", "Approval has expired but can be re-approved")) - } // Handle ApprovalExpiration lifecycle (do this after conditions are set) @@ -157,17 +151,6 @@ func (h *ApprovalHandler) Delete(ctx context.Context, approval *approvalv1.Appro return nil } -// filterSystemActions removes system-only actions (Expire) from the available transitions -func filterSystemActions(transitions approvalv1.AvailableTransitions) approvalv1.AvailableTransitions { - var filtered approvalv1.AvailableTransitions - for _, t := range transitions { - if t.Action != approvalv1.ApprovalActionExpire { - filtered = append(filtered, t) - } - } - return filtered -} - // handleExpiration manages the lifecycle of ApprovalExpiration resources func (h *ApprovalHandler) handleExpiration(ctx context.Context, approval *approvalv1.Approval, stateChanged bool) error { // Only for non-Auto strategies @@ -189,9 +172,6 @@ func (h *ApprovalHandler) handleExpiration(ctx context.Context, approval *approv case approvalv1.ApprovalStateSuspended: // Clock keeps ticking, leave ApprovalExpiration alone - case approvalv1.ApprovalStateExpired: - // Already expired, leave ApprovalExpiration alone - case approvalv1.ApprovalStateRejected: if stateChanged { // Delete ApprovalExpiration diff --git a/approval/internal/handler/approvalexpiration/handler.go b/approval/internal/handler/approvalexpiration/handler.go index b7d93a93c..9e538e6c1 100644 --- a/approval/internal/handler/approvalexpiration/handler.go +++ b/approval/internal/handler/approvalexpiration/handler.go @@ -10,11 +10,13 @@ import ( "time" "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" v1 "github.com/telekom/controlplane/approval/api/v1" + "github.com/telekom/controlplane/approval/internal/condition" "github.com/telekom/controlplane/approval/internal/handler/util" "github.com/telekom/controlplane/common/pkg/reminder" "github.com/telekom/controlplane/common/pkg/types" @@ -58,7 +60,7 @@ func (h *Handler) CreateOrUpdate(ctx context.Context, ae *v1.ApprovalExpiration) if now.After(ae.Spec.Expiration.Time) || now.Equal(ae.Spec.Expiration.Time) { logger.Info("Approval expired, transitioning to EXPIRED state", "expiration", ae.Spec.Expiration.Time) - return h.ensureApprovalExpired(ctx, approval) + return h.addExpiredCondition(ctx, approval) } // Evaluate which reminder (if any) should fire now @@ -95,7 +97,16 @@ func (h *Handler) CreateOrUpdate(ctx context.Context, ae *v1.ApprovalExpiration) // Delete handles cleanup when an ApprovalExpiration is deleted func (h *Handler) Delete(ctx context.Context, ae *v1.ApprovalExpiration) error { - // Notifications are owned by ApprovalExpiration, auto-deleted by GC + logger := log.FromContext(ctx) + + approval, err := h.getParentApproval(ctx, ae) + if err != nil { + return errors.Wrap(err, "failed to get parent approval") + } + + removed := meta.RemoveStatusCondition(&approval.Status.Conditions, "Expired") + logger.V(1).Info("Removed 'Expired' status condition", "removed", removed) + return nil } @@ -113,21 +124,16 @@ func (h *Handler) getParentApproval(ctx context.Context, ae *v1.ApprovalExpirati } // ensureApprovalExpired transitions the parent Approval to EXPIRED state -func (h *Handler) ensureApprovalExpired(ctx context.Context, approval *v1.Approval) error { - if approval.Spec.State == v1.ApprovalStateExpired { - return nil // Already expired, no-op - } - +func (h *Handler) addExpiredCondition(ctx context.Context, approval *v1.Approval) error { // Add system Decision (matches auto-approval pattern) approval.Spec.Decisions = append(approval.Spec.Decisions, v1.Decision{ Name: v1.SystemDecisionName, - Comment: "Automatically expired - expiration date reached. Available actions: Allow (re-approve), Deny (reject).", + Comment: "Expiration date reached. Approval state remains, but expired condition added.", Timestamp: &metav1.Time{Time: time.Now()}, - ResultingState: v1.ApprovalStateExpired, + ResultingState: approval.Spec.State, }) - // Set state - approval.Spec.State = v1.ApprovalStateExpired + approval.Status.Conditions = append(approval.Status.Conditions, condition.NewExpiredCondition()) // Update via API server (goes through webhook) if err := h.client.Update(ctx, approval); err != nil { diff --git a/approval/internal/webhook/v1/approval_webhook.go b/approval/internal/webhook/v1/approval_webhook.go index 20ab50a2d..72f577d2b 100644 --- a/approval/internal/webhook/v1/approval_webhook.go +++ b/approval/internal/webhook/v1/approval_webhook.go @@ -6,7 +6,6 @@ package v1 import ( "context" - "strings" apierrors "k8s.io/apimachinery/pkg/api/errors" ctrl "sigs.k8s.io/controller-runtime" @@ -83,10 +82,6 @@ func (a *ApprovalCustomValidator) ValidateUpdate(ctx context.Context, oldObj, ne // instead of Status.AvailableTransitions (which may be stale or nil before // the controller has reconciled). Auto strategy uses its own FSM. if stateChanged { - if validationErr := validateExpireTransition(ctx, newObj); validationErr != nil { - return warnings, validationErr - } - fsmDef, ok := approvalhandler.ApprovalStrategyFSM[newObj.Spec.Strategy] if !ok { err = apierrors.NewBadRequest("Unknown approval strategy") @@ -125,29 +120,3 @@ func (a *ApprovalCustomValidator) ValidateDelete(_ context.Context, obj *approva return nil, nil } - -// controllerServiceAccountSuffix is the suffix of the service account used by the approval controller. -// The full username is "system:serviceaccount::controller-manager". -const controllerServiceAccountSuffix = "controller-manager" - -// validateExpireTransition blocks manual transitions to EXPIRED state. -// Only the controller service account is allowed to perform this transition. -func validateExpireTransition(ctx context.Context, newObj *approvalv1.Approval) error { - if newObj.Spec.State != approvalv1.ApprovalStateExpired { - return nil - } - - // Verify the caller is the controller service account - req, err := admission.RequestFromContext(ctx) - if err != nil { - return apierrors.NewBadRequest("Expire action is system-only and cannot be triggered manually") - } - - username := req.UserInfo.Username - // Expected format: system:serviceaccount::controller-manager - if strings.HasPrefix(username, "system:serviceaccount:") && strings.HasSuffix(username, controllerServiceAccountSuffix) { - return nil - } - - return apierrors.NewBadRequest("Expire action is system-only and cannot be triggered manually") -} diff --git a/approval/internal/webhook/v1/approval_webhook_test.go b/approval/internal/webhook/v1/approval_webhook_test.go index f6583311a..361491dc8 100644 --- a/approval/internal/webhook/v1/approval_webhook_test.go +++ b/approval/internal/webhook/v1/approval_webhook_test.go @@ -8,10 +8,7 @@ import ( "context" "time" - admissionv1 "k8s.io/api/admission/v1" - authenticationv1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" approvalv1 "github.com/telekom/controlplane/approval/api/v1" @@ -19,18 +16,6 @@ import ( . "github.com/onsi/gomega" ) -// controllerContext returns a context that simulates a request from the controller service account. -func controllerContext() context.Context { - req := admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - UserInfo: authenticationv1.UserInfo{ - Username: "system:serviceaccount:controlplane-system:approval-controller-manager", - }, - }, - } - return admission.NewContextWithRequest(context.Background(), req) -} - var _ = Describe("Approval Webhook", func() { Context("When creating Approval under Defaulting Webhook", func() { It("Should not modify non-Auto strategy approvals", func() { @@ -509,80 +494,4 @@ var _ = Describe("Approval Webhook", func() { Expect(err).NotTo(HaveOccurred()) }) }) - - Context("Expire action validation (system-only)", func() { - var validator ApprovalCustomValidator - - makeApproval := func(specState approvalv1.ApprovalState, decisions []approvalv1.Decision) *approvalv1.Approval { - return &approvalv1.Approval{ - Spec: approvalv1.ApprovalSpec{ - Strategy: approvalv1.ApprovalStrategySimple, - State: specState, - Decisions: decisions, - }, - } - } - - It("should reject manual transition to EXPIRED without System decision", func() { - oldObj := makeApproval(approvalv1.ApprovalStateGranted, nil) - newObj := makeApproval(approvalv1.ApprovalStateExpired, []approvalv1.Decision{ - {Name: "Alice", Email: "alice@telekom.de", Comment: "Manual expire", ResultingState: approvalv1.ApprovalStateExpired}, - }) - _, err := validator.ValidateUpdate(context.Background(), oldObj, newObj) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("Expire action is system-only")) - }) - - It("should reject manual transition to EXPIRED from SUSPENDED without System decision", func() { - oldObj := makeApproval(approvalv1.ApprovalStateSuspended, nil) - newObj := makeApproval(approvalv1.ApprovalStateExpired, []approvalv1.Decision{ - {Name: "Bob", Email: "bob@telekom.de", Comment: "Manual expire", ResultingState: approvalv1.ApprovalStateExpired}, - }) - _, err := validator.ValidateUpdate(context.Background(), oldObj, newObj) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("Expire action is system-only")) - }) - - It("should allow controller service account to transition to EXPIRED", func() { - oldObj := makeApproval(approvalv1.ApprovalStateGranted, nil) - newObj := makeApproval(approvalv1.ApprovalStateExpired, []approvalv1.Decision{ - {Name: "System", Email: "", Comment: "Automatically expired", ResultingState: approvalv1.ApprovalStateExpired}, - }) - _, err := validator.ValidateUpdate(controllerContext(), oldObj, newObj) - Expect(err).NotTo(HaveOccurred()) - }) - - It("should allow controller service account to transition to EXPIRED from SUSPENDED", func() { - oldObj := makeApproval(approvalv1.ApprovalStateSuspended, nil) - newObj := makeApproval(approvalv1.ApprovalStateExpired, []approvalv1.Decision{ - {Name: "System", Email: "", Comment: "Automatically expired", ResultingState: approvalv1.ApprovalStateExpired}, - }) - _, err := validator.ValidateUpdate(controllerContext(), oldObj, newObj) - Expect(err).NotTo(HaveOccurred()) - }) - - It("should allow transition from EXPIRED to GRANTED (re-approval)", func() { - oldObj := makeApproval(approvalv1.ApprovalStateExpired, []approvalv1.Decision{ - {Name: "System", Email: "", Comment: "Automatically expired", ResultingState: approvalv1.ApprovalStateExpired}, - }) - newObj := makeApproval(approvalv1.ApprovalStateGranted, []approvalv1.Decision{ - {Name: "System", Email: "", Comment: "Automatically expired", ResultingState: approvalv1.ApprovalStateExpired}, - {Name: "Charlie", Email: "charlie@telekom.de", Comment: "Re-approved", ResultingState: approvalv1.ApprovalStateGranted}, - }) - _, err := validator.ValidateUpdate(context.Background(), oldObj, newObj) - Expect(err).NotTo(HaveOccurred()) - }) - - It("should allow transition from EXPIRED to REJECTED", func() { - oldObj := makeApproval(approvalv1.ApprovalStateExpired, []approvalv1.Decision{ - {Name: "System", Email: "", Comment: "Automatically expired", ResultingState: approvalv1.ApprovalStateExpired}, - }) - newObj := makeApproval(approvalv1.ApprovalStateRejected, []approvalv1.Decision{ - {Name: "System", Email: "", Comment: "Automatically expired", ResultingState: approvalv1.ApprovalStateExpired}, - {Name: "Dave", Email: "dave@telekom.de", Comment: "Denied", ResultingState: approvalv1.ApprovalStateRejected}, - }) - _, err := validator.ValidateUpdate(context.Background(), oldObj, newObj) - Expect(err).NotTo(HaveOccurred()) - }) - }) })