Try to address some complexity code smells

pkg/provider/webhook/webhook.go

@@ -100,6 +100,7 @@ func (w *WebHook) GetSecret(ctx context.Context, ref esv1alpha1.ExternalSecretDa
 	if err != nil {
 		return nil, err
 	}
+	// Only parse as json if we have a jsonpath set
 	if provider.Result.JSONPath != "" {
 		jsondata := interface{}(nil)
 		if err := yaml.Unmarshal(result, &jsondata); err != nil {
@@ -128,60 +129,55 @@ func (w *WebHook) GetSecretMap(ctx context.Context, ref esv1alpha1.ExternalSecre
 	if err != nil {
 		return nil, err
 	}
+
+	// We always want json here, so just parse it out
 	jsondata := interface{}(nil)
-	var jsonvalue map[string]interface{}
-	var ok bool
+	if err := yaml.Unmarshal(result, &jsondata); err != nil {
+		return nil, fmt.Errorf("failed to parse response json: %w", err)
+	}
+	// Get subdata via jsonpath, if given
 	if provider.Result.JSONPath != "" {
-		if err := yaml.Unmarshal(result, &jsondata); err != nil {
-			return nil, fmt.Errorf("failed to parse response json: %w", err)
-		}
 		jsondata, err = jsonpath.Get(provider.Result.JSONPath, jsondata)
 		if err != nil {
 			return nil, fmt.Errorf("failed to get response path %s: %w", provider.Result.JSONPath, err)
 		}
-		jsonvalue, ok = jsondata.(map[string]interface{})
-		if !ok {
-			jsonstring, ok := jsondata.(string)
-			if !ok {
-				return nil, fmt.Errorf("failed to get response (wrong type: %T)", jsondata)
-			}
-			if err := yaml.Unmarshal([]byte(jsonstring), &jsondata); err != nil {
-				return nil, fmt.Errorf("failed to parse data json: %w", err)
-			}
-			jsonvalue, ok = jsondata.(map[string]interface{})
-			if !ok {
-				return nil, fmt.Errorf("failed to get response (wrong type in data: %T)", jsondata)
-			}
-		}
-	} else {
-		if err := yaml.Unmarshal(result, &jsondata); err != nil {
-			return nil, fmt.Errorf("failed to parse data json: %w", err)
-		}
-		jsonvalue, ok = jsondata.(map[string]interface{})
-		if !ok {
-			return nil, fmt.Errorf("failed to get response (wrong type in body: %T)", jsondata)
+	}
+	// If the value is a string, try to parse it as json
+	jsonstring, ok := jsondata.(string)
+	if ok {
+		// This could also happen if the response was a single json-encoded string
+		// but that is an extremely unlikely scenario
+		if err := yaml.Unmarshal([]byte(jsonstring), &jsondata); err != nil {
+			return nil, fmt.Errorf("failed to parse response json from jsonpath: %w", err)
 		}
 	}
+	// Use the data as a key-value map
+	jsonvalue, ok := jsondata.(map[string]interface{})
+	if !ok {
+		return nil, fmt.Errorf("failed to get response (wrong type: %T)", jsondata)
+	}
+
+	// Change the map of generic objects to a map of byte arrays
 	values := make(map[string][]byte)
 	for rKey, rValue := range jsonvalue {
 		jVal, ok := rValue.(string)
 		if !ok {
-			return nil, fmt.Errorf("failed to get response (wrong type: %T)", rValue)
+			return nil, fmt.Errorf("failed to get response (wrong type in key '%s': %T)", rKey, rValue)
 		}
 		values[rKey] = []byte(jVal)
 	}
 	return values, nil
 }
 
-func (w *WebHook) getWebhookData(ctx context.Context, provider *esv1alpha1.WebhookProvider, ref esv1alpha1.ExternalSecretDataRemoteRef) ([]byte, error) {
+func (w *WebHook) getTemplateData(ctx context.Context, ref esv1alpha1.ExternalSecretDataRemoteRef, secrets []esv1alpha1.WebhookSecret) (map[string]map[string]string, error) {
 	data := map[string]map[string]string{
 		"remoteRef": {
 			"key":     url.QueryEscape(ref.Key),
 			"version": url.QueryEscape(ref.Version),
 		},
 	}
-	if provider.Secrets != nil {
-		for _, secref := range provider.Secrets {
+	if secrets != nil {
+		for _, secref := range secrets {
 			if _, ok := data[secref.Name]; !ok {
 				data[secref.Name] = make(map[string]string)
 			}
@@ -194,6 +190,14 @@ func (w *WebHook) getWebhookData(ctx context.Context, provider *esv1alpha1.Webho
 			}
 		}
 	}
+	return data, nil
+}
+
+func (w *WebHook) getWebhookData(ctx context.Context, provider *esv1alpha1.WebhookProvider, ref esv1alpha1.ExternalSecretDataRemoteRef) ([]byte, error) {
+	data, err := w.getTemplateData(ctx, ref, provider.Secrets)
+	if err != nil {
+		return nil, err
+	}
 	method := provider.Method
 	if method == "" {
 		method = "GET"
@@ -221,7 +225,7 @@ func (w *WebHook) getWebhookData(ctx context.Context, provider *esv1alpha1.Webho
 		}
 	}
 
-	client, err := w.getHTTPClient(ctx, provider)
+	client, err := w.getHTTPClient(provider)
 	if err != nil {
 		return nil, fmt.Errorf("failed to call endpoint: %w", err)
 	}
@@ -236,54 +240,64 @@ func (w *WebHook) getWebhookData(ctx context.Context, provider *esv1alpha1.Webho
 	return io.ReadAll(resp.Body)
 }
 
-func (w *WebHook) getHTTPClient(_ context.Context, provider *esv1alpha1.WebhookProvider) (*http.Client, error) {
+func (w *WebHook) getHTTPClient(provider *esv1alpha1.WebhookProvider) (*http.Client, error) {
 	client := &http.Client{}
 	if provider.Timeout != nil {
 		client.Timeout = provider.Timeout.Duration
 	}
-	if len(provider.CABundle) != 0 || provider.CAProvider != nil {
-		caCertPool := x509.NewCertPool()
-		if len(provider.CABundle) > 0 {
-			ok := caCertPool.AppendCertsFromPEM(provider.CABundle)
-			if !ok {
-				return nil, fmt.Errorf("failed to append cabundle")
-			}
-		}
+	if len(provider.CABundle) == 0 && provider.CAProvider == nil {
+		// No need to process ca stuff if it is not there
+		return client, nil
+	}
+	caCertPool, err := w.getCACertPool(provider)
+	if err != nil {
+		return nil, err
+	}
 
-		if provider.CAProvider != nil && w.storeKind == esv1alpha1.ClusterSecretStoreKind && provider.CAProvider.Namespace == nil {
-			return nil, fmt.Errorf("missing namespace on CAProvider secret")
+	tlsConf := &tls.Config{
+		RootCAs:    caCertPool,
+		MinVersion: tls.VersionTLS12,
+	}
+	client.Transport = &http.Transport{TLSClientConfig: tlsConf}
+	return client, nil
+}
+
+func (w *WebHook) getCACertPool(provider *esv1alpha1.WebhookProvider) (*x509.CertPool, error) {
+	caCertPool := x509.NewCertPool()
+	if len(provider.CABundle) > 0 {
+		ok := caCertPool.AppendCertsFromPEM(provider.CABundle)
+		if !ok {
+			return nil, fmt.Errorf("failed to append cabundle")
 		}
+	}
 
-		if provider.CAProvider != nil {
-			var cert []byte
-			var err error
-
-			switch provider.CAProvider.Type {
-			case esv1alpha1.WebhookCAProviderTypeSecret:
-				cert, err = w.getCertFromSecret(provider)
-			case esv1alpha1.WebhookCAProviderTypeConfigMap:
-				cert, err = w.getCertFromConfigMap(provider)
-			default:
-				return nil, fmt.Errorf("unknown caprovider type: %s", provider.CAProvider.Type)
-			}
+	if provider.CAProvider != nil && w.storeKind == esv1alpha1.ClusterSecretStoreKind && provider.CAProvider.Namespace == nil {
+		return nil, fmt.Errorf("missing namespace on CAProvider secret")
+	}
 
-			if err != nil {
-				return nil, err
-			}
+	if provider.CAProvider != nil {
+		var cert []byte
+		var err error
 
-			ok := caCertPool.AppendCertsFromPEM(cert)
-			if !ok {
-				return nil, fmt.Errorf("failed to append cabundle")
-			}
+		switch provider.CAProvider.Type {
+		case esv1alpha1.WebhookCAProviderTypeSecret:
+			cert, err = w.getCertFromSecret(provider)
+		case esv1alpha1.WebhookCAProviderTypeConfigMap:
+			cert, err = w.getCertFromConfigMap(provider)
+		default:
+			err = fmt.Errorf("unknown caprovider type: %s", provider.CAProvider.Type)
 		}
 
-		tlsConf := &tls.Config{
-			RootCAs:    caCertPool,
-			MinVersion: tls.VersionTLS12,
+		if err != nil {
+			return nil, err
+		}
+
+		ok := caCertPool.AppendCertsFromPEM(cert)
+		if !ok {
+			return nil, fmt.Errorf("failed to append cabundle")
 		}
-		client.Transport = &http.Transport{TLSClientConfig: tlsConf}
 	}
-	return client, nil
+	return caCertPool, nil
 }
 
 func (w *WebHook) getCertFromSecret(provider *esv1alpha1.WebhookProvider) ([]byte, error) {

pkg/provider/webhook/webhook_test.go

@@ -28,6 +28,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	esv1alpha1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1alpha1"
+	"github.com/external-secrets/external-secrets/pkg/provider"
 )
 
 type testCase struct {
@@ -183,7 +184,7 @@ args:
   response: 'some simple string'
 want:
   path: /api/getsecret?id=testkey&version=1
-  err: failed to get response (wrong type in body
+  err: failed to get response (wrong type
   resultmap:
     thesecret: secret-value
     alsosecret: another-value
@@ -197,7 +198,7 @@ args:
   response: '{"result":{"thesecret":"secret-value","alsosecret":"another-value"}}'
 want:
   path: /api/getsecret?id=testkey&version=1
-  err: failed to get response (wrong type in data
+  err: failed to get response (wrong type
   resultmap:
     thesecret: secret-value
     alsosecret: another-value
@@ -217,9 +218,9 @@ func TestWebhookGetSecret(t *testing.T) {
 	}
 }
 
-func runTestCase(tc testCase, t *testing.T) {
+func testCaseServer(tc testCase, t *testing.T) *httptest.Server {
 	// Start a new server for every test case because the server wants to check the expected api path
-	ts := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+	return httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
 		if tc.Want.Path != "" && req.URL.String() != tc.Want.Path {
 			t.Errorf("%s: unexpected api path: %s, expected %s", tc.Case, req.URL.String(), tc.Want.Path)
 		}
@@ -228,17 +229,31 @@ func runTestCase(tc testCase, t *testing.T) {
 		}
 		rw.Write([]byte(tc.Args.Response))
 	}))
+}
+
+func parseTimeout(timeout string) (*metav1.Duration, error) {
+	if timeout == "" {
+		return nil, nil
+	}
+	dur, err := time.ParseDuration(timeout)
+	if err != nil {
+		return nil, err
+	}
+	return &metav1.Duration{Duration: dur}, nil
+}
+
+func runTestCase(tc testCase, t *testing.T) {
+	ts := testCaseServer(tc, t)
 	defer ts.Close()
 
 	testStore := makeClusterSecretStore(ts.URL, tc.Args)
-	if tc.Args.Timeout != "" {
-		dur, err := time.ParseDuration(tc.Args.Timeout)
-		if err != nil {
-			t.Errorf("%s: error parsing timeout '%s': %s", tc.Case, tc.Args.Timeout, err.Error())
-			return
-		}
-		testStore.Spec.Provider.Webhook.Timeout = &metav1.Duration{Duration: dur}
+	var err error
+	timeout, err := parseTimeout(tc.Args.Timeout)
+	if err != nil {
+		t.Errorf("%s: error parsing timeout '%s': %s", tc.Case, tc.Args.Timeout, err.Error())
+		return
 	}
+	testStore.Spec.Provider.Webhook.Timeout = timeout
 	testProv := &Provider{}
 	client, err := testProv.NewClient(context.Background(), testStore, nil, "testnamespace")
 	if err != nil {
@@ -246,41 +261,53 @@ func runTestCase(tc testCase, t *testing.T) {
 		return
 	}
 
+	if tc.Want.ResultMap != nil {
+		testGetSecretMap(tc, t, client)
+	} else {
+		testGetSecret(tc, t, client)
+	}
+}
+
+func testGetSecretMap(tc testCase, t *testing.T, client provider.SecretsClient) {
 	testRef := esv1alpha1.ExternalSecretDataRemoteRef{
 		Key:     tc.Args.Key,
 		Version: tc.Args.Version,
 	}
-	if tc.Want.ResultMap != nil {
-		secretmap, err := client.GetSecretMap(context.Background(), testRef)
-		errStr := ""
-		if err != nil {
-			errStr = err.Error()
-		}
-		if (tc.Want.Err == "") != (errStr == "") || !strings.Contains(errStr, tc.Want.Err) {
-			t.Errorf("%s: unexpected error: '%s' (expected '%s')", tc.Case, errStr, tc.Want.Err)
-		}
-		if err == nil {
-			for wantkey, wantval := range tc.Want.ResultMap {
-				gotval, ok := secretmap[wantkey]
-				if !ok {
-					t.Errorf("%s: unexpected response: wanted key '%s' not found", tc.Case, wantkey)
-				} else if string(gotval) != wantval {
-					t.Errorf("%s: unexpected response: key '%s' = '%s' (expected '%s')", tc.Case, wantkey, wantval, gotval)
-				}
+	secretmap, err := client.GetSecretMap(context.Background(), testRef)
+	errStr := ""
+	if err != nil {
+		errStr = err.Error()
+	}
+	if (tc.Want.Err == "") != (errStr == "") || !strings.Contains(errStr, tc.Want.Err) {
+		t.Errorf("%s: unexpected error: '%s' (expected '%s')", tc.Case, errStr, tc.Want.Err)
+	}
+	if err == nil {
+		for wantkey, wantval := range tc.Want.ResultMap {
+			gotval, ok := secretmap[wantkey]
+			if !ok {
+				t.Errorf("%s: unexpected response: wanted key '%s' not found", tc.Case, wantkey)
+			} else if string(gotval) != wantval {
+				t.Errorf("%s: unexpected response: key '%s' = '%s' (expected '%s')", tc.Case, wantkey, wantval, gotval)
 			}
 		}
-	} else {
-		secret, err := client.GetSecret(context.Background(), testRef)
-		errStr := ""
-		if err != nil {
-			errStr = err.Error()
-		}
-		if !strings.Contains(errStr, tc.Want.Err) {
-			t.Errorf("%s: unexpected error: '%s' (expected '%s')", tc.Case, errStr, tc.Want.Err)
-		}
-		if err == nil && string(secret) != tc.Want.Result {
-			t.Errorf("%s: unexpected response: '%s' (expected '%s')", tc.Case, secret, tc.Want.Result)
-		}
+	}
+}
+
+func testGetSecret(tc testCase, t *testing.T, client provider.SecretsClient) {
+	testRef := esv1alpha1.ExternalSecretDataRemoteRef{
+		Key:     tc.Args.Key,
+		Version: tc.Args.Version,
+	}
+	secret, err := client.GetSecret(context.Background(), testRef)
+	errStr := ""
+	if err != nil {
+		errStr = err.Error()
+	}
+	if !strings.Contains(errStr, tc.Want.Err) {
+		t.Errorf("%s: unexpected error: '%s' (expected '%s')", tc.Case, errStr, tc.Want.Err)
+	}
+	if err == nil && string(secret) != tc.Want.Result {
+		t.Errorf("%s: unexpected response: '%s' (expected '%s')", tc.Case, secret, tc.Want.Result)
 	}
 }