From e6aa7757a91ee3b9880181627236e089a1a4cdcf Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Fri, 14 Jun 2024 13:57:27 +0200 Subject: [PATCH 01/13] Test new timeout values --- cmd/telefonistka/server.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/telefonistka/server.go b/cmd/telefonistka/server.go index 6f53483e..64d148fe 100644 --- a/cmd/telefonistka/server.go +++ b/cmd/telefonistka/server.go @@ -39,7 +39,7 @@ func init() { //nolint:gochecknoinits func handleWebhook(githubWebhookSecret []byte, mainGhClientCache *lru.Cache[string, githubapi.GhClientPair], prApproverGhClientCache *lru.Cache[string, githubapi.GhClientPair]) func(http.ResponseWriter, *http.Request) { return func(w http.ResponseWriter, r *http.Request) { - ctx, cancel := context.WithTimeout(r.Context(), 30*time.Second) + ctx, cancel := context.WithTimeout(r.Context(), 120*time.Second) defer cancel() githubapi.HandleEvent(r, ctx, mainGhClientCache, prApproverGhClientCache, githubWebhookSecret) } @@ -63,8 +63,8 @@ func serve() { srv := &http.Server{ Handler: mux, Addr: ":8080", - ReadTimeout: 10 * time.Second, - WriteTimeout: 10 * time.Second, + ReadTimeout: 120 * time.Second, + WriteTimeout: 120 * time.Second, } log.Infoln("server started") From 35956eef8317e82f233baa43a8eb235c650e3d07 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Mon, 17 Jun 2024 10:56:42 +0200 Subject: [PATCH 02/13] Another try at debuging context issue --- cmd/telefonistka/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/telefonistka/server.go b/cmd/telefonistka/server.go index 64d148fe..1275f844 100644 --- a/cmd/telefonistka/server.go +++ b/cmd/telefonistka/server.go @@ -40,7 +40,7 @@ func init() { //nolint:gochecknoinits func handleWebhook(githubWebhookSecret []byte, mainGhClientCache *lru.Cache[string, githubapi.GhClientPair], prApproverGhClientCache *lru.Cache[string, githubapi.GhClientPair]) func(http.ResponseWriter, *http.Request) { return func(w http.ResponseWriter, r *http.Request) { ctx, cancel := context.WithTimeout(r.Context(), 120*time.Second) - defer cancel() + // defer cancel() githubapi.HandleEvent(r, ctx, mainGhClientCache, prApproverGhClientCache, githubWebhookSecret) } } From 8ccc1316716bea29a9306964b701637030f1b666 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Mon, 17 Jun 2024 11:37:36 +0200 Subject: [PATCH 03/13] Ignore context cancelation for now --- cmd/telefonistka/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/telefonistka/server.go b/cmd/telefonistka/server.go index 1275f844..1e7f2de6 100644 --- a/cmd/telefonistka/server.go +++ b/cmd/telefonistka/server.go @@ -39,7 +39,7 @@ func init() { //nolint:gochecknoinits func handleWebhook(githubWebhookSecret []byte, mainGhClientCache *lru.Cache[string, githubapi.GhClientPair], prApproverGhClientCache *lru.Cache[string, githubapi.GhClientPair]) func(http.ResponseWriter, *http.Request) { return func(w http.ResponseWriter, r *http.Request) { - ctx, cancel := context.WithTimeout(r.Context(), 120*time.Second) + ctx, _ := context.WithTimeout(r.Context(), 120*time.Second) // defer cancel() githubapi.HandleEvent(r, ctx, mainGhClientCache, prApproverGhClientCache, githubWebhookSecret) } From a0c98444a28b5590b5b42b9454f4f43002467574 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Mon, 17 Jun 2024 12:44:30 +0200 Subject: [PATCH 04/13] Test a new context (not a child) --- cmd/telefonistka/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/telefonistka/server.go b/cmd/telefonistka/server.go index 1e7f2de6..57dade4b 100644 --- a/cmd/telefonistka/server.go +++ b/cmd/telefonistka/server.go @@ -39,7 +39,7 @@ func init() { //nolint:gochecknoinits func handleWebhook(githubWebhookSecret []byte, mainGhClientCache *lru.Cache[string, githubapi.GhClientPair], prApproverGhClientCache *lru.Cache[string, githubapi.GhClientPair]) func(http.ResponseWriter, *http.Request) { return func(w http.ResponseWriter, r *http.Request) { - ctx, _ := context.WithTimeout(r.Context(), 120*time.Second) + ctx, _ := context.WithTimeout(context.Background(), 120*time.Second) // defer cancel() githubapi.HandleEvent(r, ctx, mainGhClientCache, prApproverGhClientCache, githubWebhookSecret) } From afb6128d6783ea5841572590e760bcf53e21e5b1 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Wed, 19 Jun 2024 13:15:15 +0200 Subject: [PATCH 05/13] Remove calls to a function that exits the process. Add conext to error messages --- internal/pkg/argocd/argocd.go | 24 ++++++++++++------- .../pkg/argocd/argocd_copied_from_upstream.go | 19 +++++++++------ 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index 14ebc50b..789dd372 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -51,7 +51,7 @@ type DiffResult struct { func generateArgocdAppDiff(ctx context.Context, app *argoappv1.Application, proj *argoappv1.AppProject, resources *application.ManagedResourcesResponse, argoSettings *settings.Settings, diffOptions *DifferenceOption) (foundDiffs bool, diffElements []DiffElement, err error) { liveObjs, err := cmdutil.LiveObjects(resources.Items) if err != nil { - return false, nil, err + return false, nil, fmt.Errorf("Failed to get live objects: %v", err) } items := make([]objKeyLiveTarget, 0) @@ -59,12 +59,18 @@ func generateArgocdAppDiff(ctx context.Context, app *argoappv1.Application, proj for _, mfst := range diffOptions.res.Manifests { obj, err := argoappv1.UnmarshalToUnstructured(mfst) if err != nil { - return false, nil, err + return false, nil, fmt.Errorf("Failed to unmarshal manifest: %v", err) } unstructureds = append(unstructureds, obj) } - groupedObjs := groupObjsByKey(unstructureds, liveObjs, app.Spec.Destination.Namespace) - items = groupObjsForDiff(resources, groupedObjs, items, argoSettings, app.InstanceName(argoSettings.ControllerNamespace), app.Spec.Destination.Namespace) + groupedObjs, err := groupObjsByKey(unstructureds, liveObjs, app.Spec.Destination.Namespace) + if err != nil { + return false, nil, fmt.Errorf("Failed to group objects by key: %v", err) + } + items, err := groupObjsForDiff(resources, groupedObjs, items, argoSettings, app.InstanceName(argoSettings.ControllerNamespace), app.Spec.Destination.Namespace) + if err != nil { + return false, nil, fmt.Errorf("Failed to group objects for diff: %v", err) + } for _, item := range items { var diffElement DiffElement @@ -85,11 +91,11 @@ func generateArgocdAppDiff(ctx context.Context, app *argoappv1.Application, proj WithNoCache(). Build() if err != nil { - return false, nil, err + return false, nil, fmt.Errorf("Failed to build diff config: %v", err) } diffRes, err := argodiff.StateDiff(item.live, item.target, diffConfig) if err != nil { - return false, nil, err + return false, nil, fmt.Errorf("Failed to diff objects: %v", err) } if diffRes.Modified || item.target == nil || item.live == nil { @@ -105,7 +111,7 @@ func generateArgocdAppDiff(ctx context.Context, app *argoappv1.Application, proj live = item.live err = json.Unmarshal(diffRes.PredictedLive, target) if err != nil { - return false, nil, err + return false, nil, fmt.Errorf("Failed to unmarshal predicted live object: %v", err) } } else { live = item.live @@ -117,7 +123,7 @@ func generateArgocdAppDiff(ctx context.Context, app *argoappv1.Application, proj diffElement.Diff, err = diffLiveVsTargetObject(live, target) if err != nil { - return false, nil, err + return false, nil, fmt.Errorf("Failed to diff live objects: %v", err) } } diffElements = append(diffElements, diffElement) @@ -151,7 +157,7 @@ func createArgoCdClient() (apiclient.Client, error) { clientset, err := apiclient.NewClient(opts) if err != nil { - return nil, err + return nil, fmt.Errorf("Error creating ArgoCD API client: %v", err) } return clientset, nil } diff --git a/internal/pkg/argocd/argocd_copied_from_upstream.go b/internal/pkg/argocd/argocd_copied_from_upstream.go index 303479e1..6fe58430 100644 --- a/internal/pkg/argocd/argocd_copied_from_upstream.go +++ b/internal/pkg/argocd/argocd_copied_from_upstream.go @@ -9,7 +9,6 @@ import ( argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" repoapiclient "github.com/argoproj/argo-cd/v2/reposerver/apiclient" "github.com/argoproj/argo-cd/v2/util/argo" - "github.com/argoproj/argo-cd/v2/util/errors" "github.com/argoproj/gitops-engine/pkg/sync/hook" "github.com/argoproj/gitops-engine/pkg/sync/ignore" "github.com/argoproj/gitops-engine/pkg/utils/kube" @@ -42,7 +41,7 @@ func (p *resourceInfoProvider) IsNamespaced(gk schema.GroupKind) (bool, error) { // This function creates a map of objects by key(object name/kind/ns) from the rendered manifests. // That map is used to compare the objects in the application with the objects in the cluster. // copied from https://github.com/argoproj/argo-cd/blob/4f6a8dce80f0accef7ed3b5510e178a6b398b331/cmd/argocd/commands/app.go#L1091-L1109 -func groupObjsByKey(localObs []*unstructured.Unstructured, liveObjs []*unstructured.Unstructured, appNamespace string) map[kube.ResourceKey]*unstructured.Unstructured { +func groupObjsByKey(localObs []*unstructured.Unstructured, liveObjs []*unstructured.Unstructured, appNamespace string) (map[kube.ResourceKey]*unstructured.Unstructured, error) { namespacedByGk := make(map[schema.GroupKind]bool) for i := range liveObjs { if liveObjs[i] != nil { @@ -51,7 +50,9 @@ func groupObjsByKey(localObs []*unstructured.Unstructured, liveObjs []*unstructu } } localObs, _, err := controller.DeduplicateTargetObjects(appNamespace, localObs, &resourceInfoProvider{namespacedByGk: namespacedByGk}) - errors.CheckError(err) + if err != nil { + return nil, err + } objByKey := make(map[kube.ResourceKey]*unstructured.Unstructured) for i := range localObs { obj := localObs[i] @@ -59,17 +60,19 @@ func groupObjsByKey(localObs []*unstructured.Unstructured, liveObjs []*unstructu objByKey[kube.GetResourceKey(obj)] = obj } } - return objByKey + return objByKey, nil } // This function create a slice of objects to be "diff'ed", each element contains the key, live(in-cluster API state) and target(rended manifest from git) object. // Copied from https://github.com/argoproj/argo-cd/blob/4f6a8dce80f0accef7ed3b5510e178a6b398b331/cmd/argocd/commands/app.go#L1341-L1372 -func groupObjsForDiff(resources *application.ManagedResourcesResponse, objs map[kube.ResourceKey]*unstructured.Unstructured, items []objKeyLiveTarget, argoSettings *settings.Settings, appName, namespace string) []objKeyLiveTarget { +func groupObjsForDiff(resources *application.ManagedResourcesResponse, objs map[kube.ResourceKey]*unstructured.Unstructured, items []objKeyLiveTarget, argoSettings *settings.Settings, appName, namespace string) ([]objKeyLiveTarget, error) { resourceTracking := argo.NewResourceTracking() for _, res := range resources.Items { live := &unstructured.Unstructured{} err := json.Unmarshal([]byte(res.NormalizedLiveState), &live) - errors.CheckError(err) + if err != nil { + return nil, err + } key := kube.ResourceKey{Name: res.Name, Namespace: res.Namespace, Group: res.Group, Kind: res.Kind} if key.Kind == kube.SecretKind && key.Group == "" { @@ -80,7 +83,9 @@ func groupObjsForDiff(resources *application.ManagedResourcesResponse, objs map[ if local, ok := objs[key]; ok || live != nil { if local != nil && !kube.IsCRD(local) { err = resourceTracking.SetAppInstance(local, argoSettings.AppLabelKey, appName, namespace, argoappv1.TrackingMethod(argoSettings.GetTrackingMethod())) - errors.CheckError(err) + if err != nil { + return nil, err + } } items = append(items, objKeyLiveTarget{key, live, local}) From 4c9de2a518f15727a799ba9ffe70fafc4a7f9b58 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Wed, 19 Jun 2024 13:21:13 +0200 Subject: [PATCH 06/13] Re-add the context cancelation defera + some comment Fix some function return issues --- cmd/telefonistka/server.go | 6 ++++-- internal/pkg/argocd/argocd.go | 2 +- internal/pkg/argocd/argocd_copied_from_upstream.go | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/cmd/telefonistka/server.go b/cmd/telefonistka/server.go index 57dade4b..1b3e8f96 100644 --- a/cmd/telefonistka/server.go +++ b/cmd/telefonistka/server.go @@ -39,8 +39,10 @@ func init() { //nolint:gochecknoinits func handleWebhook(githubWebhookSecret []byte, mainGhClientCache *lru.Cache[string, githubapi.GhClientPair], prApproverGhClientCache *lru.Cache[string, githubapi.GhClientPair]) func(http.ResponseWriter, *http.Request) { return func(w http.ResponseWriter, r *http.Request) { - ctx, _ := context.WithTimeout(context.Background(), 120*time.Second) - // defer cancel() + // We don't use the request context as it might have a short deadline and we don't want to stop event handling based on that + // But we do want to stop the event handling after a certain point, so: + ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) + defer cancel() githubapi.HandleEvent(r, ctx, mainGhClientCache, prApproverGhClientCache, githubWebhookSecret) } } diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index 789dd372..dc746f74 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -67,7 +67,7 @@ func generateArgocdAppDiff(ctx context.Context, app *argoappv1.Application, proj if err != nil { return false, nil, fmt.Errorf("Failed to group objects by key: %v", err) } - items, err := groupObjsForDiff(resources, groupedObjs, items, argoSettings, app.InstanceName(argoSettings.ControllerNamespace), app.Spec.Destination.Namespace) + items, err = groupObjsForDiff(resources, groupedObjs, items, argoSettings, app.InstanceName(argoSettings.ControllerNamespace), app.Spec.Destination.Namespace) if err != nil { return false, nil, fmt.Errorf("Failed to group objects for diff: %v", err) } diff --git a/internal/pkg/argocd/argocd_copied_from_upstream.go b/internal/pkg/argocd/argocd_copied_from_upstream.go index 6fe58430..f0b4c95f 100644 --- a/internal/pkg/argocd/argocd_copied_from_upstream.go +++ b/internal/pkg/argocd/argocd_copied_from_upstream.go @@ -100,5 +100,5 @@ func groupObjsForDiff(resources *application.ManagedResourcesResponse, objs map[ } items = append(items, objKeyLiveTarget{key, nil, local}) } - return items + return items, nil } From f34c9b06e901aa0a673e074a0a3b83fd65305c4d Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Wed, 19 Jun 2024 15:56:25 +0200 Subject: [PATCH 07/13] Update server.go --- cmd/telefonistka/server.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/telefonistka/server.go b/cmd/telefonistka/server.go index 1b3e8f96..18223573 100644 --- a/cmd/telefonistka/server.go +++ b/cmd/telefonistka/server.go @@ -65,8 +65,8 @@ func serve() { srv := &http.Server{ Handler: mux, Addr: ":8080", - ReadTimeout: 120 * time.Second, - WriteTimeout: 120 * time.Second, + ReadTimeout: 10 * time.Second, + WriteTimeout: 10 * time.Second, } log.Infoln("server started") From 3c3e214e73ffa4927ef0600c51febb8babc3b9f8 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Thu, 20 Jun 2024 13:40:27 +0200 Subject: [PATCH 08/13] Add more "context"(info, like for humans) to errors --- internal/pkg/argocd/argocd_copied_from_upstream.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/pkg/argocd/argocd_copied_from_upstream.go b/internal/pkg/argocd/argocd_copied_from_upstream.go index f0b4c95f..a8a50a21 100644 --- a/internal/pkg/argocd/argocd_copied_from_upstream.go +++ b/internal/pkg/argocd/argocd_copied_from_upstream.go @@ -2,6 +2,7 @@ package argocd import ( "encoding/json" + "fmt" "github.com/argoproj/argo-cd/v2/controller" "github.com/argoproj/argo-cd/v2/pkg/apiclient/application" @@ -51,7 +52,7 @@ func groupObjsByKey(localObs []*unstructured.Unstructured, liveObjs []*unstructu } localObs, _, err := controller.DeduplicateTargetObjects(appNamespace, localObs, &resourceInfoProvider{namespacedByGk: namespacedByGk}) if err != nil { - return nil, err + return nil, fmt.Errorf("Failed to DeDuplicate target objects: %v", err) } objByKey := make(map[kube.ResourceKey]*unstructured.Unstructured) for i := range localObs { @@ -71,7 +72,7 @@ func groupObjsForDiff(resources *application.ManagedResourcesResponse, objs map[ live := &unstructured.Unstructured{} err := json.Unmarshal([]byte(res.NormalizedLiveState), &live) if err != nil { - return nil, err + return nil, fmt.Errorf("Failed to unmarshal live object(%v): %v", res.Name, err) } key := kube.ResourceKey{Name: res.Name, Namespace: res.Namespace, Group: res.Group, Kind: res.Kind} From 863b98b23197ee1ae70981e2c80c29891bfe345f Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Thu, 20 Jun 2024 13:42:14 +0200 Subject: [PATCH 09/13] Address another err with no context issue --- internal/pkg/argocd/argocd_copied_from_upstream.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/argocd/argocd_copied_from_upstream.go b/internal/pkg/argocd/argocd_copied_from_upstream.go index a8a50a21..d78b6b86 100644 --- a/internal/pkg/argocd/argocd_copied_from_upstream.go +++ b/internal/pkg/argocd/argocd_copied_from_upstream.go @@ -85,7 +85,7 @@ func groupObjsForDiff(resources *application.ManagedResourcesResponse, objs map[ if local != nil && !kube.IsCRD(local) { err = resourceTracking.SetAppInstance(local, argoSettings.AppLabelKey, appName, namespace, argoappv1.TrackingMethod(argoSettings.GetTrackingMethod())) if err != nil { - return nil, err + return nil, fmt.Errorf("Failed to set app instance label: %v", err) } } From 43f34604161fdbef0522d0a8c32cbd3737631916 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Fri, 21 Jun 2024 09:06:59 +0200 Subject: [PATCH 10/13] Respond the webhook only based on payload parsing. Actual Event proccessing is move to backround thread. This require some refactoring, created ReciveWebhook and ReciveEventFile functions to represent the different behavior in Web Server VS CLI triggering while keeping to the GH stuff in the GH package --- cmd/telefonistka/event.go | 25 +------------------ cmd/telefonistka/server.go | 13 +++++----- internal/pkg/githubapi/github.go | 43 +++++++++++++++++++++++++++++--- 3 files changed, 48 insertions(+), 33 deletions(-) diff --git a/cmd/telefonistka/event.go b/cmd/telefonistka/event.go index 885fb425..3ff94caf 100644 --- a/cmd/telefonistka/event.go +++ b/cmd/telefonistka/event.go @@ -1,14 +1,9 @@ package telefonistka import ( - "bytes" - "context" - "io" - "net/http" "os" lru "github.com/hashicorp/golang-lru/v2" - log "github.com/sirupsen/logrus" "github.com/spf13/cobra" "github.com/wayfair-incubator/telefonistka/internal/pkg/githubapi" ) @@ -32,27 +27,9 @@ func init() { //nolint:gochecknoinits } func event(eventType string, eventFilePath string) { - ctx := context.Background() - - log.Infof("Event type: %s", eventType) - log.Infof("Proccesing file: %s", eventFilePath) - - payload, err := os.ReadFile(eventFilePath) - if err != nil { - panic(err) - } - - // To use the same code path as for Webhook I'm creating an http request with the payload from the file. - // This might not be very smart. - - h, _ := http.NewRequest("POST", "", nil) //nolint:noctx - h.Body = io.NopCloser(bytes.NewReader(payload)) - h.Header.Set("Content-Type", "application/json") - h.Header.Set("X-GitHub-Event", eventType) - mainGhClientCache, _ := lru.New[string, githubapi.GhClientPair](128) prApproverGhClientCache, _ := lru.New[string, githubapi.GhClientPair](128) - githubapi.HandleEvent(h, ctx, mainGhClientCache, prApproverGhClientCache, nil) + githubapi.ReciveEventFile(eventFilePath, eventType, mainGhClientCache, prApproverGhClientCache) } func getEnv(key, fallback string) string { diff --git a/cmd/telefonistka/server.go b/cmd/telefonistka/server.go index 18223573..076b28f8 100644 --- a/cmd/telefonistka/server.go +++ b/cmd/telefonistka/server.go @@ -1,7 +1,6 @@ package telefonistka import ( - "context" "net/http" "os" "time" @@ -39,11 +38,13 @@ func init() { //nolint:gochecknoinits func handleWebhook(githubWebhookSecret []byte, mainGhClientCache *lru.Cache[string, githubapi.GhClientPair], prApproverGhClientCache *lru.Cache[string, githubapi.GhClientPair]) func(http.ResponseWriter, *http.Request) { return func(w http.ResponseWriter, r *http.Request) { - // We don't use the request context as it might have a short deadline and we don't want to stop event handling based on that - // But we do want to stop the event handling after a certain point, so: - ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) - defer cancel() - githubapi.HandleEvent(r, ctx, mainGhClientCache, prApproverGhClientCache, githubWebhookSecret) + err := githubapi.ReciveWebhook(r, mainGhClientCache, prApproverGhClientCache, githubWebhookSecret) + if err != nil { + log.Errorf("error handling webhook: %v", err) + http.Error(w, "Internal server error", http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) } } diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index e5136b61..5f884ccc 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -8,12 +8,15 @@ import ( "encoding/hex" "encoding/json" "fmt" + "io" "net/http" + "os" "path" "regexp" "sort" "strings" "text/template" + "time" "github.com/cenkalti/backoff/v4" "github.com/google/go-github/v62/github" @@ -185,12 +188,36 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr } } -func HandleEvent(r *http.Request, ctx context.Context, mainGhClientCache *lru.Cache[string, GhClientPair], prApproverGhClientCache *lru.Cache[string, GhClientPair], githubWebhookSecret []byte) { +func ReciveEventFile(eventType string, eventFilePath string, mainGhClientCache *lru.Cache[string, GhClientPair], prApproverGhClientCache *lru.Cache[string, GhClientPair]) { + log.Infof("Event type: %s", eventType) + log.Infof("Proccesing file: %s", eventFilePath) + + payload, err := os.ReadFile(eventFilePath) + if err != nil { + panic(err) + } + eventPayloadInterface, err := github.ParseWebHook(eventType, payload) + if err != nil { + log.Errorf("could not parse webhook: err=%s\n", err) + prom.InstrumentWebhookHit("parsing_failed") + return + } + // This is a hack to simulate a webhook event so we can use the same code path + r, _ := http.NewRequest("POST", "", nil) //nolint:noctx + r.Body = io.NopCloser(bytes.NewReader(payload)) + r.Header.Set("Content-Type", "application/json") + r.Header.Set("X-GitHub-Event", eventType) + + HandleEvent(eventPayloadInterface, mainGhClientCache, prApproverGhClientCache, r, payload, eventType) +} + +// ReciveWebhook is the main entry point for the webhook handling it starts parases the webhook payload and start a thread to handle the event success/failure are dependant on the payload parsing only +func ReciveWebhook(r *http.Request, mainGhClientCache *lru.Cache[string, GhClientPair], prApproverGhClientCache *lru.Cache[string, GhClientPair], githubWebhookSecret []byte) error { payload, err := github.ValidatePayload(r, githubWebhookSecret) if err != nil { log.Errorf("error reading request body: err=%s\n", err) prom.InstrumentWebhookHit("validation_failed") - return + return err } eventType := github.WebHookType(r) @@ -198,9 +225,19 @@ func HandleEvent(r *http.Request, ctx context.Context, mainGhClientCache *lru.Ca if err != nil { log.Errorf("could not parse webhook: err=%s\n", err) prom.InstrumentWebhookHit("parsing_failed") - return + return err } prom.InstrumentWebhookHit("successful") + + go HandleEvent(eventPayloadInterface, mainGhClientCache, prApproverGhClientCache, r, payload, eventType) + return nil +} + +func HandleEvent(eventPayloadInterface interface{}, mainGhClientCache *lru.Cache[string, GhClientPair], prApproverGhClientCache *lru.Cache[string, GhClientPair], r *http.Request, payload []byte, eventType string) { + // We don't use the request context as it might have a short deadline and we don't want to stop event handling based on that + // But we do want to stop the event handling after a certain point, so: + ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) + defer cancel() var mainGithubClientPair GhClientPair var approverGithubClientPair GhClientPair From 791420cecc5f9d24f81beee62abe2912698aecc8 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Fri, 21 Jun 2024 09:15:28 +0200 Subject: [PATCH 11/13] Change handleEvent to a private function. Minor comment change --- internal/pkg/githubapi/github.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 5f884ccc..4a0929bc 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -188,6 +188,7 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr } } +// ReciveEventFile this one is similar to ReciveWebhook but it's used for CLI triggering, i simulates a webhook event to use the same code path as the webhook handler. func ReciveEventFile(eventType string, eventFilePath string, mainGhClientCache *lru.Cache[string, GhClientPair], prApproverGhClientCache *lru.Cache[string, GhClientPair]) { log.Infof("Event type: %s", eventType) log.Infof("Proccesing file: %s", eventFilePath) @@ -202,13 +203,12 @@ func ReciveEventFile(eventType string, eventFilePath string, mainGhClientCache * prom.InstrumentWebhookHit("parsing_failed") return } - // This is a hack to simulate a webhook event so we can use the same code path r, _ := http.NewRequest("POST", "", nil) //nolint:noctx r.Body = io.NopCloser(bytes.NewReader(payload)) r.Header.Set("Content-Type", "application/json") r.Header.Set("X-GitHub-Event", eventType) - HandleEvent(eventPayloadInterface, mainGhClientCache, prApproverGhClientCache, r, payload, eventType) + handleEvent(eventPayloadInterface, mainGhClientCache, prApproverGhClientCache, r, payload, eventType) } // ReciveWebhook is the main entry point for the webhook handling it starts parases the webhook payload and start a thread to handle the event success/failure are dependant on the payload parsing only @@ -229,11 +229,11 @@ func ReciveWebhook(r *http.Request, mainGhClientCache *lru.Cache[string, GhClien } prom.InstrumentWebhookHit("successful") - go HandleEvent(eventPayloadInterface, mainGhClientCache, prApproverGhClientCache, r, payload, eventType) + go handleEvent(eventPayloadInterface, mainGhClientCache, prApproverGhClientCache, r, payload, eventType) return nil } -func HandleEvent(eventPayloadInterface interface{}, mainGhClientCache *lru.Cache[string, GhClientPair], prApproverGhClientCache *lru.Cache[string, GhClientPair], r *http.Request, payload []byte, eventType string) { +func handleEvent(eventPayloadInterface interface{}, mainGhClientCache *lru.Cache[string, GhClientPair], prApproverGhClientCache *lru.Cache[string, GhClientPair], r *http.Request, payload []byte, eventType string) { // We don't use the request context as it might have a short deadline and we don't want to stop event handling based on that // But we do want to stop the event handling after a certain point, so: ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) From 4d55de352604e1cff517a545882559bcc7de269b Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Fri, 21 Jun 2024 12:19:04 +0200 Subject: [PATCH 12/13] Cancel whole drift on context deadline --- internal/pkg/githubapi/promotion.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/pkg/githubapi/promotion.go b/internal/pkg/githubapi/promotion.go index 44ebd07e..b9b013fd 100644 --- a/internal/pkg/githubapi/promotion.go +++ b/internal/pkg/githubapi/promotion.go @@ -51,6 +51,9 @@ func contains(s []string, str string) bool { } func DetectDrift(ghPrClientDetails GhPrClientDetails) error { + if ghPrClientDetails.Ctx.Err() != nil { + return ghPrClientDetails.Ctx.Err() + } diffOutputMap := make(map[string]string) defaultBranch, _ := ghPrClientDetails.GetDefaultBranch() config, err := GetInRepoConfig(ghPrClientDetails, defaultBranch) From 16ea0287eb429218fc1d88a033b7a880c9b73751 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Fri, 21 Jun 2024 15:40:09 +0200 Subject: [PATCH 13/13] Move error function return value to the standard position Handle cases where GetContents returns nil HTTP respons (like in Context cancellation) --- cmd/telefonistka/bump-version-overwrite.go | 2 +- cmd/telefonistka/bump-version-regex.go | 2 +- cmd/telefonistka/bump-version-yaml.go | 2 +- internal/pkg/githubapi/github.go | 17 +++++++++++------ 4 files changed, 14 insertions(+), 9 deletions(-) diff --git a/cmd/telefonistka/bump-version-overwrite.go b/cmd/telefonistka/bump-version-overwrite.go index fa6b4f59..e2df86df 100644 --- a/cmd/telefonistka/bump-version-overwrite.go +++ b/cmd/telefonistka/bump-version-overwrite.go @@ -73,7 +73,7 @@ func bumpVersionOverwrite(targetRepo string, targetFile string, file string, git ghPrClientDetails.PrLogger = log.WithFields(log.Fields{}) // TODO what fields should be here? defaultBranch, _ := ghPrClientDetails.GetDefaultBranch() - initialFileContent, err, statusCode := githubapi.GetFileContent(ghPrClientDetails, defaultBranch, targetFile) + initialFileContent, statusCode, err := githubapi.GetFileContent(ghPrClientDetails, defaultBranch, targetFile) if statusCode == 404 { ghPrClientDetails.PrLogger.Infof("File %s was not found\n", targetFile) } else if err != nil { diff --git a/cmd/telefonistka/bump-version-regex.go b/cmd/telefonistka/bump-version-regex.go index 0e9fa25e..fc3d50f1 100644 --- a/cmd/telefonistka/bump-version-regex.go +++ b/cmd/telefonistka/bump-version-regex.go @@ -71,7 +71,7 @@ func bumpVersionRegex(targetRepo string, targetFile string, regex string, replac r := regexp.MustCompile(regex) defaultBranch, _ := ghPrClientDetails.GetDefaultBranch() - initialFileContent, err, _ := githubapi.GetFileContent(ghPrClientDetails, defaultBranch, targetFile) + initialFileContent, _, err := githubapi.GetFileContent(ghPrClientDetails, defaultBranch, targetFile) if err != nil { ghPrClientDetails.PrLogger.Errorf("Fail to fetch file content:%s\n", err) os.Exit(1) diff --git a/cmd/telefonistka/bump-version-yaml.go b/cmd/telefonistka/bump-version-yaml.go index 7642662c..c130b775 100644 --- a/cmd/telefonistka/bump-version-yaml.go +++ b/cmd/telefonistka/bump-version-yaml.go @@ -74,7 +74,7 @@ func bumpVersionYaml(targetRepo string, targetFile string, address string, value defaultBranch, _ := ghPrClientDetails.GetDefaultBranch() - initialFileContent, err, _ := githubapi.GetFileContent(ghPrClientDetails, defaultBranch, targetFile) + initialFileContent, _, err := githubapi.GetFileContent(ghPrClientDetails, defaultBranch, targetFile) if err != nil { ghPrClientDetails.PrLogger.Errorf("Fail to fetch file content:%s\n", err) os.Exit(1) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 4a0929bc..bf75c41c 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -1008,7 +1008,7 @@ func ApprovePr(approverClient *github.Client, ghPrClientDetails GhPrClientDetail } func GetInRepoConfig(ghPrClientDetails GhPrClientDetails, defaultBranch string) (*cfg.Config, error) { - inRepoConfigFileContentString, err, _ := GetFileContent(ghPrClientDetails, defaultBranch, "telefonistka.yaml") + inRepoConfigFileContentString, _, err := GetFileContent(ghPrClientDetails, defaultBranch, "telefonistka.yaml") if err != nil { ghPrClientDetails.PrLogger.Errorf("Could not get in-repo configuration: err=%s\n", err) return nil, err @@ -1020,18 +1020,23 @@ func GetInRepoConfig(ghPrClientDetails GhPrClientDetails, defaultBranch string) return c, err } -func GetFileContent(ghPrClientDetails GhPrClientDetails, branch string, filePath string) (string, error, int) { +func GetFileContent(ghPrClientDetails GhPrClientDetails, branch string, filePath string) (string, int, error) { rGetContentOps := github.RepositoryContentGetOptions{Ref: branch} fileContent, _, resp, err := ghPrClientDetails.GhClientPair.v3Client.Repositories.GetContents(ghPrClientDetails.Ctx, ghPrClientDetails.Owner, ghPrClientDetails.Repo, filePath, &rGetContentOps) - prom.InstrumentGhCall(resp) if err != nil { ghPrClientDetails.PrLogger.Errorf("Fail to get file:%s\n%v\n", err, resp) - return "", err, resp.StatusCode + if resp == nil { + return "", 0, err + } + prom.InstrumentGhCall(resp) + return "", resp.StatusCode, err + } else { + prom.InstrumentGhCall(resp) } fileContentString, err := fileContent.GetContent() if err != nil { ghPrClientDetails.PrLogger.Errorf("Fail to serlize file:%s\n", err) - return "", err, resp.StatusCode + return "", resp.StatusCode, err } - return fileContentString, nil, resp.StatusCode + return fileContentString, resp.StatusCode, nil }