chore(webhook): move maxConcurrentAppRefresh from secret to configmap

Signed-off-by: phanama <[email protected]>
argoproj · Sep 3, 2023 · 507530c · 507530c
1 parent c56e642
commit 507530c
Show file tree

Hide file tree

Showing 3 changed files with 98 additions and 9 deletions.
diff --git a/util/settings/settings.go b/util/settings/settings.go
@@ -493,7 +493,7 @@ const (
 	ResourceDeepLinks = "resource.links"
 	extensionConfig   = "extension.config"
 	// settingsWebhookMaxConcurrentAppRefresh is the key for max concurrent app refresh
-	settingsWebhookMaxConcurrentAppRefresh = "webhook.maxConcurrentAppRefresh"
+	settingsWebhookMaxConcurrentAppRefreshKey = "webhook.maxConcurrentAppRefresh"
 	// defaultSettingsWebhookMaxConcurrentAppRefresh is the default value for the number of max concurrent app refresh
 	defaultWebhookMaxConcurrentAppRefresh = 10
 )
@@ -1430,6 +1430,19 @@ func updateSettingsFromConfigMap(settings *ArgoCDSettings, argoCDCM *apiv1.Confi
 	settings.TrackingMethod = argoCDCM.Data[settingsResourceTrackingMethodKey]
 	settings.OIDCTLSInsecureSkipVerify = argoCDCM.Data[oidcTLSInsecureSkipVerifyKey] == "true"
 	settings.ExtensionConfig = argoCDCM.Data[extensionConfig]
+	if webhookMaxConcurrentAppRefresh := argoCDCM.Data[settingsWebhookMaxConcurrentAppRefreshKey]; len(webhookMaxConcurrentAppRefresh) > 0 {
+		i, err := strconv.Atoi(string(webhookMaxConcurrentAppRefresh))
+		if err != nil {
+			log.Warnf("invalid input for %s: %s. Using the default value %d.", settingsWebhookMaxConcurrentAppRefreshKey, err.Error(), defaultWebhookMaxConcurrentAppRefresh)
+		}
+		if i < 1 {
+			i = defaultWebhookMaxConcurrentAppRefresh
+			log.Warnf("%s is less than 1. Using the default value %d.", settingsWebhookMaxConcurrentAppRefreshKey, defaultWebhookMaxConcurrentAppRefresh)
+		}
+		settings.WebhookMaxConcurrentAppRefresh = i
+	} else {
+		settings.WebhookMaxConcurrentAppRefresh = defaultWebhookMaxConcurrentAppRefresh
+	}
 }
 
 // validateExternalURL ensures the external URL that is set on the configmap is valid
@@ -1477,14 +1490,6 @@ func (mgr *SettingsManager) updateSettingsFromSecret(settings *ArgoCDSettings, a
 	if azureDevOpsPassword := argoCDSecret.Data[settingsWebhookAzureDevOpsPasswordKey]; len(azureDevOpsPassword) > 0 {
 		settings.WebhookAzureDevOpsPassword = string(azureDevOpsPassword)
 	}
-	if webhookMaxConcurrentAppRefresh := argoCDSecret.Data[settingsWebhookMaxConcurrentAppRefresh]; len(webhookMaxConcurrentAppRefresh) > 0 {
-		i, err := strconv.Atoi(string(webhookMaxConcurrentAppRefresh))
-		if err != nil {
-			i = defaultWebhookMaxConcurrentAppRefresh
-			log.Warnf("invalid input for %s: %s. Using the default value.", settingsWebhookMaxConcurrentAppRefresh, err.Error())
-		}
-		settings.WebhookMaxConcurrentAppRefresh = i
-	}
 
 	// The TLS certificate may be externally managed. We try to load it from an
 	// external secret first. If the external secret doesn't exist, we either

diff --git a/util/settings/settings_test.go b/util/settings/settings_test.go
@@ -169,6 +169,88 @@ func TestInClusterServerAddressEnabledByDefault(t *testing.T) {
 	assert.Equal(t, true, settings.InClusterEnabled)
 }
 
+func TestValidWebhookConfiguration(t *testing.T) {
+	secret := []byte("randomly-strong-secret")
+	uid := []byte("random-uid")
+	kubeClient := fake.NewSimpleClientset(
+		&v1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      common.ArgoCDConfigMapName,
+				Namespace: "default",
+				Labels: map[string]string{
+					"app.kubernetes.io/part-of": "argocd",
+				},
+			},
+			Data: map[string]string{
+				"webhook.maxConcurrentAppRefresh": "25",
+			},
+		},
+		&v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      common.ArgoCDSecretName,
+				Namespace: "default",
+				Labels: map[string]string{
+					"app.kubernetes.io/part-of": "argocd",
+				},
+			},
+			Data: map[string][]byte{
+				"server.secretkey":               nil,
+				"webhook.github.secret":          secret,
+				"webhook.gitlab.secret":          secret,
+				"webhook.bitbucket.uuid":         uid,
+				"webhook.bitbucketserver.secret": secret,
+				"webhook.gogs.secret":            secret,
+				"webhook.azuredevops.username":   uid,
+				"webhook.azuredevops.password":   secret,
+			},
+		},
+	)
+	settingsManager := NewSettingsManager(context.Background(), kubeClient, "default")
+	settings, err := settingsManager.GetSettings()
+	assert.NoError(t, err)
+	assert.Equal(t, 25, settings.WebhookMaxConcurrentAppRefresh)
+	assert.Equal(t, string(secret), settings.WebhookGitHubSecret)
+	assert.Equal(t, string(secret), settings.WebhookGitLabSecret)
+	assert.Equal(t, string(uid), settings.WebhookBitbucketUUID)
+	assert.Equal(t, string(secret), settings.WebhookBitbucketServerSecret)
+	assert.Equal(t, string(secret), settings.WebhookGogsSecret)
+	assert.Equal(t, string(uid), settings.WebhookAzureDevOpsUsername)
+	assert.Equal(t, string(secret), settings.WebhookAzureDevOpsPassword)
+}
+
+func TestInvalidWebhookConfiguration(t *testing.T) {
+	kubeClient := fake.NewSimpleClientset(
+		&v1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      common.ArgoCDConfigMapName,
+				Namespace: "default",
+				Labels: map[string]string{
+					"app.kubernetes.io/part-of": "argocd",
+				},
+			},
+			Data: map[string]string{
+				"webhook.maxConcurrentAppRefresh": "0",
+			},
+		},
+		&v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      common.ArgoCDSecretName,
+				Namespace: "default",
+				Labels: map[string]string{
+					"app.kubernetes.io/part-of": "argocd",
+				},
+			},
+			Data: map[string][]byte{
+				"server.secretkey": nil,
+			},
+		},
+	)
+	settingsManager := NewSettingsManager(context.Background(), kubeClient, "default")
+	settings, err := settingsManager.GetSettings()
+	assert.NoError(t, err)
+	assert.Equal(t, 10, settings.WebhookMaxConcurrentAppRefresh)
+}
+
 func TestGetAppInstanceLabelKey(t *testing.T) {
 	_, settingsManager := fixtures(map[string]string{
 		"application.instanceLabelKey": "testLabel",

diff --git a/util/webhook/webhook.go b/util/webhook/webhook.go
@@ -105,8 +105,10 @@ func NewHandler(namespace string, applicationNamespaces []string, appClientset a
 		return nil
 	}
 
+	// though it seems redundant in util/settings, this serves as a safeguard for the handler
 	maxConcurrentAppRefresh := defaultMaxConcurrentAppRefresh
 	if set.WebhookMaxConcurrentAppRefresh > 0 {
+		log.Warnf("webhook setting for max concurrent app refresh is not set, using the default value %d", defaultMaxConcurrentAppRefresh)
 		maxConcurrentAppRefresh = set.WebhookMaxConcurrentAppRefresh
 	}