From bed39ade5650c4b607e770462c9ef5249a4a3080 Mon Sep 17 00:00:00 2001 From: Jeremy Kaplan Date: Sat, 5 Oct 2024 21:54:14 -0400 Subject: [PATCH] refactor: Extract AppEngine Cron source check into middleware This de-duplicates the code we have to check the AppEngine Cron request header and pushes it into the HTTP router and out of the app logic. As a convenient side effect, this also ensures that any cron job failure produces an HTTP 500 response (indicating failure to AppEngine) and an ERROR-severity log line (which we're starting to use for alerts). --- main.go | 12 +++---- middleware.go | 30 +++++++++++++++++ middleware_test.go | 76 +++++++++++++++++++++++++++++++++++++++++++ pairing_bot.go | 80 +++++++++++++++------------------------------- 4 files changed, 138 insertions(+), 60 deletions(-) create mode 100644 middleware.go create mode 100644 middleware_test.go diff --git a/main.go b/main.go index 41be497..73ee1ed 100644 --- a/main.go +++ b/main.go @@ -127,12 +127,12 @@ func main() { welcomeStream: welcomeStream, } - http.HandleFunc("/", http.NotFound) // will this handle anything that's not defined? - http.HandleFunc("/webhooks", pl.handle) // from zulip - http.HandleFunc("/match", pl.match) // from GCP- daily - http.HandleFunc("/endofbatch", pl.endofbatch) // from GCP- weekly - http.HandleFunc("/welcome", pl.welcome) // from GCP- weekly - http.HandleFunc("/checkin", pl.checkin) // from GCP- weekly + http.HandleFunc("/", http.NotFound) // will this handle anything that's not defined? + http.HandleFunc("/webhooks", pl.handle) // from zulip + http.HandleFunc("/match", cron(pl.Match)) // from GCP- daily + http.HandleFunc("/endofbatch", cron(pl.EndOfBatch)) // from GCP- weekly + http.HandleFunc("/welcome", cron(pl.Welcome)) // from GCP- weekly + http.HandleFunc("/checkin", cron(pl.Checkin)) // from GCP- weekly port := os.Getenv("PORT") if port == "" { diff --git a/middleware.go b/middleware.go new file mode 100644 index 0000000..bbb108f --- /dev/null +++ b/middleware.go @@ -0,0 +1,30 @@ +package main + +import ( + "context" + "log/slog" + "net/http" +) + +// JobFunc is the type of function that can run as a cron job. +type JobFunc func(context.Context) error + +// cron wraps a job function to make it an HTTP handler. The handler enforces +// that requests originate from App Engine's Cron scheduler. +func cron(job JobFunc) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + // Check that the request is originating from within app engine + // https://cloud.google.com/appengine/docs/standard/go/scheduling-jobs-with-cron-yaml#validating_cron_requests + if r.Header.Get("X-Appengine-Cron") != "true" { + http.NotFound(w, r) + return + } + + err := job(r.Context()) + if err != nil { + slog.Error("Job failed", slog.Any("error", err)) + w.WriteHeader(http.StatusInternalServerError) + return + } + } +} diff --git a/middleware_test.go b/middleware_test.go new file mode 100644 index 0000000..2db1477 --- /dev/null +++ b/middleware_test.go @@ -0,0 +1,76 @@ +package main + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/recursecenter/pairing-bot/internal/assert" +) + +func Test_cron(t *testing.T) { + t.Run("run job for AppEngine", func(t *testing.T) { + // Arrange a cron job that tells us whether it ran. + ran := false + handler := cron(func(context.Context) error { + ran = true + return nil + }) + + // Prepare an AppEngine-sourced request. + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("X-Appengine-Cron", "true") + + // Run it! + w := httptest.NewRecorder() + handler(w, req) + + resp := w.Result() + defer resp.Body.Close() + + assert.Equal(t, ran, true) + assert.Equal(t, resp.StatusCode, 200) + }) + + t.Run("deny request outside of cron", func(t *testing.T) { + // Arrange a cron job that fails the test if it runs. + handler := cron(func(context.Context) error { + t.Error("handler should not have run") + return nil + }) + + // Prepare a request from outside of AppEngine (no custom header). + req := httptest.NewRequest(http.MethodGet, "/", nil) + + // Run it! + w := httptest.NewRecorder() + handler(w, req) + + resp := w.Result() + defer resp.Body.Close() + + assert.Equal(t, resp.StatusCode, 404) + }) + + t.Run("report job failure", func(t *testing.T) { + // Arrange a cron job that errors. + handler := cron(func(context.Context) error { + return errors.New("test error") + }) + + // Prepare an AppEngine-sourced request. + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("X-Appengine-Cron", "true") + + // Run it! + w := httptest.NewRecorder() + handler(w, req) + + resp := w.Result() + defer resp.Body.Close() + + assert.Equal(t, resp.StatusCode, 500) + }) +} diff --git a/pairing_bot.go b/pairing_bot.go index aba957a..926d033 100644 --- a/pairing_bot.go +++ b/pairing_bot.go @@ -1,10 +1,10 @@ package main import ( + "context" "encoding/json" "fmt" "log" - "log/slog" "math/rand" "net/http" "slices" @@ -142,27 +142,17 @@ func (pl *PairingLogic) handle(w http.ResponseWriter, r *http.Request) { } } -// "match" makes matches for pairing, and messages those people to notify them of their match -// it runs once per day (it's triggered with app engine's cron service) -func (pl *PairingLogic) match(w http.ResponseWriter, r *http.Request) { - // Check that the request is originating from within app engine - // https://cloud.google.com/appengine/docs/flexible/go/scheduling-jobs-with-cron-yaml#validating_cron_requests - if r.Header.Get("X-Appengine-Cron") != "true" { - http.NotFound(w, r) - return - } - - ctx := r.Context() - +// Match generates new pairs for today and sends notifications for them. +func (pl *PairingLogic) Match(ctx context.Context) error { recursersList, err := pl.rdb.ListPairingTomorrow(ctx) log.Println(recursersList) if err != nil { - log.Printf("Could not get list of recursers from DB: %s\n", err) + return fmt.Errorf("get today's recursers from DB: %w", err) } skippersList, err := pl.rdb.ListSkippingTomorrow(ctx) if err != nil { - log.Printf("Could not get list of skippers from DB: %s\n", err) + return fmt.Errorf("get today's skippers from DB: %w", err) } // get everyone who was set to skip today and set them back to isSkippingTomorrow = false @@ -187,7 +177,7 @@ func (pl *PairingLogic) match(w http.ResponseWriter, r *http.Request) { // if for some reason there's no matches today, we're done if len(recursersList) == 0 { log.Println("No one was signed up to pair today -- so there were no matches") - return + return nil } // message the peeps! @@ -229,19 +219,13 @@ func (pl *PairingLogic) match(w http.ResponseWriter, r *http.Request) { if err := pl.pdb.SetNumPairings(ctx, pairing); err != nil { log.Printf("Failed to record today's pairings: %s", err) } -} -// Unsubscribe people from Pairing Bot when their batch is over. They're always welcome to re-subscribe manually! -func (pl *PairingLogic) endofbatch(w http.ResponseWriter, r *http.Request) { - // Check that the request is originating from within app engine - // https://cloud.google.com/appengine/docs/flexible/go/scheduling-jobs-with-cron-yaml#validating_cron_requests - if r.Header.Get("X-Appengine-Cron") != "true" { - http.NotFound(w, r) - return - } + return nil +} +// EndOfBatch unsubscribes everyone who just never-graduated with this batch. +func (pl *PairingLogic) EndOfBatch(ctx context.Context) error { // getting all the recursers - ctx := r.Context() recursersList, err := pl.rdb.GetAllUsers(ctx) if err != nil { log.Println("Could not get list of recursers from DB: ", err) @@ -249,10 +233,7 @@ func (pl *PairingLogic) endofbatch(w http.ResponseWriter, r *http.Request) { profiles, err := pl.recurse.ActiveRecursers(ctx) if err != nil { - log.Println("Encountered error while getting currently-active Recursers: ", err) - slog.Error("Aborting end-of-batch processing!") - w.WriteHeader(http.StatusInternalServerError) - return + return fmt.Errorf("get active Recursers: %w", err) } var idsOfPeopleAtRc []int64 @@ -297,13 +278,13 @@ func (pl *PairingLogic) endofbatch(w http.ResponseWriter, r *http.Request) { } } } -} -func (pl *PairingLogic) checkin(w http.ResponseWriter, r *http.Request) { - ctx := r.Context() + return nil +} +// Checkin posts a message to Pairing Bot's checkin topic. +func (pl *PairingLogic) Checkin(ctx context.Context) error { numPairings, err := pl.pdb.GetTotalPairingsDuringLastWeek(ctx) - if err != nil { log.Println("Unable to get the total number of pairings during the last week: : ", err) } @@ -320,36 +301,26 @@ func (pl *PairingLogic) checkin(w http.ResponseWriter, r *http.Request) { checkinMessage, err := renderCheckin(time.Now(), numPairings, len(recursersList), review.Content) if err != nil { - log.Printf("Error when trying to render Pairing Bot checkin: %s", err) - return + return fmt.Errorf("render checkin: %w", err) } if err := pl.zulip.PostToTopic(ctx, "checkins", "Pairing Bot", checkinMessage); err != nil { - log.Printf("Error when trying to submit Pairing Bot checkins stream message: %s\n", err) + return fmt.Errorf("send checkin: %w", err) } + + return nil } -// welcome sends a "Welcome to Pairing Bot" message to introduce the new batch +// Welcome sends a "Welcome to Pairing Bot" message to introduce the new batch // to Pairing Bot. // // We send this message during the second week of batch. The first week is a // bit overwhelming with all of the orientation meetings and messages, and // people haven't had time to think too much about their projects. -func (pl *PairingLogic) welcome(w http.ResponseWriter, r *http.Request) { - // Check that the request is originating from within app engine - // https://cloud.google.com/appengine/docs/flexible/go/scheduling-jobs-with-cron-yaml#validating_cron_requests - if r.Header.Get("X-Appengine-Cron") != "true" { - http.NotFound(w, r) - return - } - - ctx := r.Context() - +func (pl *PairingLogic) Welcome(ctx context.Context) error { batches, err := pl.recurse.AllBatches(ctx) if err != nil { - log.Printf("Error when fetching batches: %s", err) - w.WriteHeader(http.StatusInternalServerError) - return + return fmt.Errorf("get list of batches: %w", err) } // Loop through the batches until we find the first non-mini batch. Mini @@ -369,12 +340,13 @@ func (pl *PairingLogic) welcome(w http.ResponseWriter, r *http.Request) { if currentBatch.IsSecondWeek(now) { msg, err := renderWelcome(now) if err != nil { - log.Printf("Error when trying to send welcome message about Pairing Bot %s\n", err) - return + return fmt.Errorf("render welcome message: %w", err) } if err := pl.zulip.PostToTopic(ctx, pl.welcomeStream, "🍐🤖", msg); err != nil { - log.Printf("Error when trying to send welcome message about Pairing Bot %s\n", err) + return fmt.Errorf("send welcome message: %w", err) } } + + return nil }