Handle/mitigate crash and context issues (#11)

* Use  new context (not a child)
* Remove calls to a function that exits the process.
* Add more "context"(info, like for humans) to errors
* Address another err with no context issue
* Respond the webhook only based on payload parsing.
Actual Event processing is move to background 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
* Cancel whole drift work  on context deadline
* Move error function return value to the standard position
Handle cases where GetContents returns nil HTTP response (like in Context
cancellation)

Author: Oded-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 {

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)

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)

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 {

cmd/telefonistka/server.go

@@ -1,7 +1,6 @@
 package telefonistka
 
 import (
-	"context"
 	"net/http"
 	"os"
 	"time"
@@ -39,9 +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) {
-		ctx, cancel := context.WithTimeout(r.Context(), 30*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)
 	}
 }
 

internal/pkg/argocd/argocd.go

@@ -51,20 +51,26 @@ 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)
 	var unstructureds []*unstructured.Unstructured
 	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
 }

internal/pkg/argocd/argocd_copied_from_upstream.go

@@ -2,14 +2,14 @@ package argocd
 
 import (
 	"encoding/json"
+	"fmt"
 
 	"github.com/argoproj/argo-cd/v2/controller"
 	"github.com/argoproj/argo-cd/v2/pkg/apiclient/application"
 	"github.com/argoproj/argo-cd/v2/pkg/apiclient/settings"
 	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 +42,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,25 +51,29 @@ func groupObjsByKey(localObs []*unstructured.Unstructured, liveObjs []*unstructu
 		}
 	}
 	localObs, _, err := controller.DeduplicateTargetObjects(appNamespace, localObs, &resourceInfoProvider{namespacedByGk: namespacedByGk})
-	errors.CheckError(err)
+	if err != nil {
+		return nil, fmt.Errorf("Failed to DeDuplicate target objects: %v", err)
+	}
 	objByKey := make(map[kube.ResourceKey]*unstructured.Unstructured)
 	for i := range localObs {
 		obj := localObs[i]
 		if !(hook.IsHook(obj) || ignore.Ignore(obj)) {
 			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, 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}
 		if key.Kind == kube.SecretKind && key.Group == "" {
@@ -80,7 +84,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, fmt.Errorf("Failed to set app instance label: %v", err)
+				}
 			}
 
 			items = append(items, objKeyLiveTarget{key, live, local})
@@ -95,5 +101,5 @@ func groupObjsForDiff(resources *application.ManagedResourcesResponse, objs map[
 		}
 		items = append(items, objKeyLiveTarget{key, nil, local})
 	}
-	return items
+	return items, nil
 }

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,22 +188,56 @@ 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) {
+// 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)
+
+	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
+	}
+	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)
 
 	eventPayloadInterface, err := github.ParseWebHook(eventType, payload)
 	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
 
@@ -971,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
@@ -983,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
 }

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)