RequestedAt can be used as ID of the request (#280)

* RequestedAt can be used as ID of the request

* validate request on every use case

* fixed test

* fixed req time compare bug

* fix error

api/v1beta1/module_types.go

@@ -298,25 +298,22 @@ func (m *Module) IsPlanOnly() bool {
 func (m *Module) NewRunRequest(reqType string) *Request {
 	req := Request{
 		RequestedAt: &metav1.Time{Time: time.Now()},
-		ID:          NewRequestID(),
 		Type:        reqType,
 	}
 
 	return &req
 }
 
 // PendingRunRequest returns pending requests if any from module's annotation.
-func (m *Module) PendingRunRequest() (*Request, bool) {
+// function will return err if req is not json or not valid
+func (m *Module) PendingRunRequest() (*Request, error) {
 	valueString, exists := m.ObjectMeta.Annotations[RunRequestAnnotationKey]
 	if !exists {
-		return nil, false
+		return nil, nil
 	}
 	value := Request{}
 	if err := json.Unmarshal([]byte(valueString), &value); err != nil {
-		// unmarshal errors are ignored as it should not happen and if it does
-		// it can be treated as no request pending and module can override it
-		// with new valid request
-		return nil, false
+		return nil, err
 	}
-	return &value, true
+	return &value, value.Validate()
 }

api/v1beta1/run.go

@@ -1,8 +1,6 @@
 package v1beta1
 
 import (
-	"crypto/rand"
-	"encoding/base64"
 	"fmt"
 	"time"
 
@@ -50,7 +48,6 @@ func NewRun(module *Module, req *Request) Run {
 
 // Request represents terraform run request
 type Request struct {
-	ID          string       `json:"id,omitempty"`
 	RequestedAt *metav1.Time `json:"reqAt,omitempty"`
 	Type        string       `json:"type,omitempty"`
 	PR          *PullRequest `json:"pr,omitempty"`
@@ -65,7 +62,7 @@ type PullRequest struct {
 func (req *Request) Validate() error {
 
 	if req.RequestedAt.IsZero() {
-		return fmt.Errorf("valid timestamp is required for 'RequestedAt'")
+		return fmt.Errorf("'reqAt' is required and must be in the '2006-01-02T15:04:05Z' format")
 	}
 
 	switch req.Type {
@@ -119,10 +116,3 @@ func (req *Request) RepoRef(module *Module) string {
 
 	return module.Spec.RepoRef
 }
-
-// NewRequestID generates random string as ID
-func NewRequestID() string {
-	b := make([]byte, 6)
-	rand.Read(b)
-	return base64.StdEncoding.EncodeToString(b)
-}

controllers/module_controller.go

@@ -113,13 +113,17 @@ func (r *ModuleReconciler) Reconcile(ctx context.Context, req reconcile.Request)
 	// case 1:
 	// check for run triggers
 	//
-	runReq, ok := module.PendingRunRequest()
-	if ok {
+	runReq, err := module.PendingRunRequest()
+	if err != nil {
+		log.Error("removing invalid run request", "err", err)
+		sysutil.RemoveCurrentRequest(ctx, r.Client, module.NamespacedName())
+		return ctrl.Result{RequeueAfter: pollIntervalDuration}, nil
+	}
+	if runReq != nil {
 		log.Debug("processing pending run request", "req", runReq, "delay", time.Since(runReq.RequestedAt.Time))
 		// use next poll internal as minimum queue duration as status change will not trigger Reconcile
 		r.triggerRun(ctx, module, runReq)
 		return ctrl.Result{RequeueAfter: pollIntervalDuration}, nil
-
 	}
 
 	// case 2:

prplanner/requests_test.go

@@ -18,7 +18,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 )
 
-var cmpIgnoreRandFields = cmpopts.IgnoreFields(tfaplv1beta1.Request{}, "ID", "RequestedAt")
+var cmpIgnoreRandFields = cmpopts.IgnoreFields(tfaplv1beta1.Request{}, "RequestedAt")
 
 func generateMockPR(num int, ref string, comments []string) *pr {
 	p := &pr{

sysutil/k8s_req_api.go

@@ -23,10 +23,10 @@ func EnsureRequest(ctx context.Context, client client.Client, key types.Namespac
 			return err
 		}
 
-		existingReq, ok := module.PendingRunRequest()
-		if ok {
-			// if annotated request ID is matching then nothing to do
-			if req.ID == existingReq.ID {
+		existingReq, err := module.PendingRunRequest()
+		// if valid req found then verify if its matching given req
+		if err == nil && existingReq != nil {
+			if req.RequestedAt.Equal(existingReq.RequestedAt) {
 				return nil
 			} else {
 				return tfaplv1beta1.ErrRunRequestExist
@@ -65,13 +65,43 @@ func RemoveRequest(ctx context.Context, client client.Client, key types.Namespac
 			return err
 		}
 
-		existingReq, ok := module.PendingRunRequest()
-		if !ok {
-			return tfaplv1beta1.ErrNoRunRequestFound
+		existingReq, err := module.PendingRunRequest()
+		if err == nil {
+			if existingReq == nil {
+				return tfaplv1beta1.ErrNoRunRequestFound
+			}
+
+			if !req.RequestedAt.Equal(existingReq.RequestedAt) {
+				return tfaplv1beta1.ErrRunRequestMismatch
+			}
+		}
+
+		delete(module.ObjectMeta.Annotations, tfaplv1beta1.RunRequestAnnotationKey)
+
+		// return err itself here (not wrapped inside another error)
+		// so that ExponentialBackoffWithContext can identify it correctly.
+		return client.Update(ctx, module)
+	}
+
+	err := CallWithBackOff(ctx, tryUpdate)
+	if err != nil {
+		return fmt.Errorf("unable to remove pending run request err:%w", err)
+	}
+
+	return nil
+}
+
+// RemoveCurrentRequest will try to remove current request without validation
+func RemoveCurrentRequest(ctx context.Context, client client.Client, key types.NamespacedName) error {
+	tryUpdate := func(ctx context.Context) error {
+		// refetch module on every try
+		module, err := GetModule(ctx, client, key)
+		if err != nil {
+			return err
 		}
 
-		if req.ID != existingReq.ID {
-			return tfaplv1beta1.ErrRunRequestMismatch
+		if _, exists := module.ObjectMeta.Annotations[tfaplv1beta1.RunRequestAnnotationKey]; !exists {
+			return nil
 		}
 
 		delete(module.ObjectMeta.Annotations, tfaplv1beta1.RunRequestAnnotationKey)

webserver/modules.go

@@ -47,8 +47,7 @@ func createNamespaceMap(ctx context.Context, modules []tfaplv1beta1.Module, redi
 		module.Runs = slices.CompactFunc(module.Runs, func(a *tfaplv1beta1.Run, b *tfaplv1beta1.Run) bool {
 			if a != nil && b != nil &&
 				a.StartedAt != nil && b.StartedAt != nil {
-				return a.Request.ID == b.Request.ID &&
-					a.StartedAt.Compare(b.StartedAt.Time) == 0
+				return a.StartedAt.Compare(b.StartedAt.Time) == 0
 			}
 			return false
 		})