diff --git a/api/v1beta1/module_types.go b/api/v1beta1/module_types.go index 5938e53..e7801a2 100644 --- a/api/v1beta1/module_types.go +++ b/api/v1beta1/module_types.go @@ -298,7 +298,6 @@ func (m *Module) IsPlanOnly() bool { func (m *Module) NewRunRequest(reqType string) *Request { req := Request{ RequestedAt: &metav1.Time{Time: time.Now()}, - ID: NewRequestID(), Type: reqType, } @@ -306,17 +305,15 @@ func (m *Module) NewRunRequest(reqType string) *Request { } // 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() } diff --git a/api/v1beta1/run.go b/api/v1beta1/run.go index b0db158..76adc0b 100644 --- a/api/v1beta1/run.go +++ b/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) -} diff --git a/controllers/module_controller.go b/controllers/module_controller.go index f00985c..1001013 100644 --- a/controllers/module_controller.go +++ 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: diff --git a/prplanner/requests_test.go b/prplanner/requests_test.go index 9d86218..1b5e62f 100644 --- a/prplanner/requests_test.go +++ b/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{ diff --git a/sysutil/k8s_req_api.go b/sysutil/k8s_req_api.go index 048b942..6a5e6da 100644 --- a/sysutil/k8s_req_api.go +++ b/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) diff --git a/webserver/modules.go b/webserver/modules.go index 73c768e..cdefbcf 100644 --- a/webserver/modules.go +++ b/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 })