From e1f35ea7aa914b9696d232dad7dd5234446b891b Mon Sep 17 00:00:00 2001 From: pashakostohrys Date: Sat, 20 Jul 2024 19:25:49 +0300 Subject: [PATCH 1/2] wh improvement --- server/server.go | 2 +- util/settings/settings.go | 26 ++++++++++++++++++++++++++ util/webhook/webhook.go | 15 +++++++++++++-- util/webhook/webhook_test.go | 27 +++++++++++++++++++++++++-- 4 files changed, 65 insertions(+), 5 deletions(-) diff --git a/server/server.go b/server/server.go index 2ce70a933742b..46d7b422320cd 100644 --- a/server/server.go +++ b/server/server.go @@ -1048,7 +1048,7 @@ func (a *ArgoCDServer) newHTTPServer(ctx context.Context, port int, grpcWebHandl // Webhook handler for git events (Note: cache timeouts are hardcoded because API server does not write to cache and not really using them) argoDB := db.NewDB(a.Namespace, a.settingsMgr, a.KubeClientset) - acdWebhookHandler := webhook.NewHandler(a.Namespace, a.ArgoCDServerOpts.ApplicationNamespaces, a.AppClientset, a.settings, a.settingsMgr, a.RepoServerCache, a.Cache, argoDB) + acdWebhookHandler := webhook.NewHandler(a.Namespace, a.ArgoCDServerOpts.ApplicationNamespaces, a.ArgoCDServerOpts.WebhookParallelism, a.AppClientset, a.settings, a.settingsMgr, a.RepoServerCache, a.Cache, argoDB, a.settingsMgr.GetMaxWebhookPayloadSize()) mux.HandleFunc("/api/webhook", acdWebhookHandler.Handler) diff --git a/util/settings/settings.go b/util/settings/settings.go index 8cf7f8bec1dcc..316f78fc4af2c 100644 --- a/util/settings/settings.go +++ b/util/settings/settings.go @@ -442,6 +442,8 @@ const ( settingsWebhookAzureDevOpsUsernameKey = "webhook.azuredevops.username" // settingsWebhookAzureDevOpsPasswordKey is the key for Azure DevOps webhook password settingsWebhookAzureDevOpsPasswordKey = "webhook.azuredevops.password" + // settingsWebhookMaxPayloadSize is the key for the maximum payload size for webhooks in MB + settingsWebhookMaxPayloadSizeMB = "webhook.maxPayloadSizeMB" // settingsApplicationInstanceLabelKey is the key to configure injected app instance label key settingsApplicationInstanceLabelKey = "application.instanceLabelKey" // settingsCodefreshReporterVersion is the key to configure injected app instance label key @@ -525,6 +527,11 @@ const ( kustomizeSetNamespaceEnabledKey = "kustomize.setNamespace.enabled" ) +const ( + // default max webhook payload size is 1GB + defaultMaxWebhookPayloadSize = int64(1) * 1024 * 1024 * 1024 +) + var ( sourceTypeToEnableGenerationKey = map[v1alpha1.ApplicationSourceType]string{ v1alpha1.ApplicationSourceTypeKustomize: "kustomize.enable", @@ -2262,3 +2269,22 @@ func (mgr *SettingsManager) GetResourceCustomLabels() ([]string, error) { } return []string{}, nil } + +func (mgr *SettingsManager) GetMaxWebhookPayloadSize() int64 { + argoCDCM, err := mgr.getConfigMap() + if err != nil { + return defaultMaxWebhookPayloadSize + } + + if argoCDCM.Data[settingsWebhookMaxPayloadSizeMB] == "" { + return defaultMaxWebhookPayloadSize + } + + maxPayloadSizeMB, err := strconv.ParseInt(argoCDCM.Data[settingsWebhookMaxPayloadSizeMB], 10, 64) + if err != nil { + log.Warnf("Failed to parse '%s' key: %v", settingsWebhookMaxPayloadSizeMB, err) + return defaultMaxWebhookPayloadSize + } + + return maxPayloadSizeMB * 1024 * 1024 +} diff --git a/util/webhook/webhook.go b/util/webhook/webhook.go index dab69d7b131b7..1da0a0efa2d9c 100644 --- a/util/webhook/webhook.go +++ b/util/webhook/webhook.go @@ -61,9 +61,10 @@ type ArgoCDWebhookHandler struct { azuredevopsAuthHandler func(r *http.Request) error gogs *gogs.Webhook settingsSrc settingsSource + maxWebhookPayloadSizeB int64 } -func NewHandler(namespace string, applicationNamespaces []string, appClientset appclientset.Interface, set *settings.ArgoCDSettings, settingsSrc settingsSource, repoCache *cache.Cache, serverCache *servercache.Cache, argoDB db.ArgoDB) *ArgoCDWebhookHandler { +func NewHandler(namespace string, applicationNamespaces []string, appClientset appclientset.Interface, set *settings.ArgoCDSettings, settingsSrc settingsSource, repoCache *cache.Cache, serverCache *servercache.Cache, argoDB db.ArgoDB, maxWebhookPayloadSizeB int64) *ArgoCDWebhookHandler { githubWebhook, err := github.New(github.Options.Secret(set.WebhookGitHubSecret)) if err != nil { log.Warnf("Unable to init the GitHub webhook") @@ -113,6 +114,7 @@ func NewHandler(namespace string, applicationNamespaces []string, appClientset a repoCache: repoCache, serverCache: serverCache, db: argoDB, + maxWebhookPayloadSizeB: maxWebhookPayloadSizeB, } return &acdWebhook @@ -384,10 +386,11 @@ func sourceUsesURL(source v1alpha1.ApplicationSource, webURL string, repoRegexp } func (a *ArgoCDWebhookHandler) Handler(w http.ResponseWriter, r *http.Request) { - var payload interface{} var err error + r.Body = http.MaxBytesReader(w, r.Body, a.maxWebhookPayloadSizeB) + switch { case r.Header.Get("X-Vss-Activityid") != "": if err = a.azuredevopsAuthHandler(r); err != nil { @@ -430,6 +433,14 @@ func (a *ArgoCDWebhookHandler) Handler(w http.ResponseWriter, r *http.Request) { } if err != nil { + // If the error is due to a large payload, return a more user-friendly error message + if err.Error() == "error parsing payload" { + msg := fmt.Sprintf("Webhook processing failed: The payload is either too large or corrupted. Please check the payload size (must be under %v MB) and ensure it is valid JSON", a.maxWebhookPayloadSizeB/1024/1024) + log.WithField(common.SecurityField, common.SecurityHigh).Warn(msg) + http.Error(w, msg, http.StatusBadRequest) + return + } + log.Infof("Webhook processing failed: %s", err) status := http.StatusBadRequest if r.Method != http.MethodPost { diff --git a/util/webhook/webhook_test.go b/util/webhook/webhook_test.go index a32d7205bcd07..a96a59831b4ac 100644 --- a/util/webhook/webhook_test.go +++ b/util/webhook/webhook_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "fmt" + "github.com/stretchr/testify/require" "io" "net/http" "net/http/httptest" @@ -56,6 +57,11 @@ type reactorDef struct { } func NewMockHandler(reactor *reactorDef, applicationNamespaces []string, objects ...runtime.Object) *ArgoCDWebhookHandler { + defaultMaxPayloadSize := int64(1) * 1024 * 1024 * 1024 + return NewMockHandlerWithPayloadLimit(reactor, applicationNamespaces, defaultMaxPayloadSize, objects...) +} + +func NewMockHandlerWithPayloadLimit(reactor *reactorDef, applicationNamespaces []string, maxPayloadSize int64, objects ...runtime.Object) *ArgoCDWebhookHandler { appClientset := appclientset.NewSimpleClientset(objects...) if reactor != nil { defaultReactor := appClientset.ReactionChain[0] @@ -73,7 +79,7 @@ func NewMockHandler(reactor *reactorDef, applicationNamespaces []string, objects 1*time.Minute, 10*time.Second, 1*time.Minute, - ), servercache.NewCache(appstate.NewCache(cacheClient, time.Minute), time.Minute, time.Minute, time.Minute), &mocks.ArgoDB{}) + ), servercache.NewCache(appstate.NewCache(cacheClient, time.Minute), time.Minute, time.Minute, time.Minute), &mocks.ArgoDB{}, maxPayloadSize) } func TestGitHubCommitEvent(t *testing.T) { @@ -394,7 +400,7 @@ func TestInvalidEvent(t *testing.T) { w := httptest.NewRecorder() h.Handler(w, req) assert.Equal(t, w.Code, http.StatusBadRequest) - expectedLogResult := "Webhook processing failed: error parsing payload" + expectedLogResult := "Webhook processing failed: The payload is either too large or corrupted. Please check the payload size (must be under 1024 MB) and ensure it is valid JSON" assert.Equal(t, expectedLogResult, hook.LastEntry().Message) assert.Equal(t, expectedLogResult+"\n", w.Body.String()) hook.Reset() @@ -605,3 +611,20 @@ func Test_getWebUrlRegex(t *testing.T) { }) } } + +func TestGitHubCommitEventMaxPayloadSize(t *testing.T) { + hook := test.NewGlobal() + maxPayloadSize := int64(100) + h := NewMockHandlerWithPayloadLimit(nil, []string{}, maxPayloadSize) + req := httptest.NewRequest(http.MethodPost, "/api/webhook", nil) + req.Header.Set("X-GitHub-Event", "push") + eventJSON, err := os.ReadFile("testdata/github-commit-event.json") + require.NoError(t, err) + req.Body = io.NopCloser(bytes.NewReader(eventJSON)) + w := httptest.NewRecorder() + h.Handler(w, req) + assert.Equal(t, http.StatusBadRequest, w.Code) + expectedLogResult := "Webhook processing failed: The payload is either too large or corrupted. Please check the payload size (must be under 0 MB) and ensure it is valid JSON" + assert.Equal(t, expectedLogResult, hook.LastEntry().Message) + hook.Reset() +} From 5ddf5dd7b7eeedc7221395f6c3aa19c4c2f1ebc9 Mon Sep 17 00:00:00 2001 From: pashakostohrys Date: Sat, 20 Jul 2024 19:31:51 +0300 Subject: [PATCH 2/2] fix build err --- server/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/server.go b/server/server.go index 46d7b422320cd..b8f0fc1a76dee 100644 --- a/server/server.go +++ b/server/server.go @@ -1048,7 +1048,7 @@ func (a *ArgoCDServer) newHTTPServer(ctx context.Context, port int, grpcWebHandl // Webhook handler for git events (Note: cache timeouts are hardcoded because API server does not write to cache and not really using them) argoDB := db.NewDB(a.Namespace, a.settingsMgr, a.KubeClientset) - acdWebhookHandler := webhook.NewHandler(a.Namespace, a.ArgoCDServerOpts.ApplicationNamespaces, a.ArgoCDServerOpts.WebhookParallelism, a.AppClientset, a.settings, a.settingsMgr, a.RepoServerCache, a.Cache, argoDB, a.settingsMgr.GetMaxWebhookPayloadSize()) + acdWebhookHandler := webhook.NewHandler(a.Namespace, a.ArgoCDServerOpts.ApplicationNamespaces, a.AppClientset, a.settings, a.settingsMgr, a.RepoServerCache, a.Cache, argoDB, a.settingsMgr.GetMaxWebhookPayloadSize()) mux.HandleFunc("/api/webhook", acdWebhookHandler.Handler)