fix: URL encode poolName in azure-pipelines (kedacore#5120)

Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
zroubalik · Nov 27, 2023 · 4f84471 · 4f84471
1 parent 27bf91e
commit 4f84471
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -64,6 +64,7 @@ Here is an overview of all new **experimental** features:
 ### Fixes
 
 - **General**: Prevented stuck status due to timeouts during scalers generation ([#5083](https://github.com/kedacore/keda/issues/5083))
+- **Azure Pipelines**: No more HTTP 400 errors produced by poolName with spaces ([#5107](https://github.com/kedacore/keda/issues/5107))
 
 ### Deprecations
 

diff --git a/pkg/scalers/azure_pipelines_scaler.go b/pkg/scalers/azure_pipelines_scaler.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"strconv"
 	"strings"
 	"time"
@@ -265,8 +266,8 @@ func parseAzurePipelinesMetadata(ctx context.Context, config *ScalerConfig, http
 }
 
 func getPoolIDFromName(ctx context.Context, poolName string, metadata *azurePipelinesMetadata, httpClient *http.Client) (int, error) {
-	url := fmt.Sprintf("%s/_apis/distributedtask/pools?poolName=%s", metadata.organizationURL, poolName)
-	body, err := getAzurePipelineRequest(ctx, url, metadata, httpClient)
+	urlString := fmt.Sprintf("%s/_apis/distributedtask/pools?poolName=%s", metadata.organizationURL, url.QueryEscape(poolName))
+	body, err := getAzurePipelineRequest(ctx, urlString, metadata, httpClient)
 	if err != nil {
 		return -1, err
 	}
@@ -290,8 +291,8 @@ func getPoolIDFromName(ctx context.Context, poolName string, metadata *azurePipe
 }
 
 func validatePoolID(ctx context.Context, poolID string, metadata *azurePipelinesMetadata, httpClient *http.Client) (int, error) {
-	url := fmt.Sprintf("%s/_apis/distributedtask/pools?poolID=%s", metadata.organizationURL, poolID)
-	body, err := getAzurePipelineRequest(ctx, url, metadata, httpClient)
+	urlString := fmt.Sprintf("%s/_apis/distributedtask/pools?poolID=%s", metadata.organizationURL, poolID)
+	body, err := getAzurePipelineRequest(ctx, urlString, metadata, httpClient)
 	if err != nil {
 		return -1, fmt.Errorf("agent pool with id `%s` not found: %w", poolID, err)
 	}
@@ -305,8 +306,8 @@ func validatePoolID(ctx context.Context, poolID string, metadata *azurePipelines
 	return result.ID, nil
 }
 
-func getAzurePipelineRequest(ctx context.Context, url string, metadata *azurePipelinesMetadata, httpClient *http.Client) ([]byte, error) {
-	req, err := http.NewRequestWithContext(ctx, "GET", url, nil)
+func getAzurePipelineRequest(ctx context.Context, urlString string, metadata *azurePipelinesMetadata, httpClient *http.Client) ([]byte, error) {
+	req, err := http.NewRequestWithContext(ctx, "GET", urlString, nil)
 	if err != nil {
 		return []byte{}, err
 	}
@@ -325,21 +326,21 @@ func getAzurePipelineRequest(ctx context.Context, url string, metadata *azurePip
 	r.Body.Close()
 
 	if !(r.StatusCode >= 200 && r.StatusCode <= 299) {
-		return []byte{}, fmt.Errorf("the Azure DevOps REST API returned error. url: %s status: %d response: %s", url, r.StatusCode, string(b))
+		return []byte{}, fmt.Errorf("the Azure DevOps REST API returned error. urlString: %s status: %d response: %s", urlString, r.StatusCode, string(b))
 	}
 
 	return b, nil
 }
 
 func (s *azurePipelinesScaler) GetAzurePipelinesQueueLength(ctx context.Context) (int64, error) {
 	// HotFix Issue (#4387), $top changes the format of the returned JSON
-	var url string
+	var urlString string
 	if s.metadata.parent != "" {
-		url = fmt.Sprintf("%s/_apis/distributedtask/pools/%d/jobrequests", s.metadata.organizationURL, s.metadata.poolID)
+		urlString = fmt.Sprintf("%s/_apis/distributedtask/pools/%d/jobrequests", s.metadata.organizationURL, s.metadata.poolID)
 	} else {
-		url = fmt.Sprintf("%s/_apis/distributedtask/pools/%d/jobrequests?$top=%d", s.metadata.organizationURL, s.metadata.poolID, s.metadata.jobsToFetch)
+		urlString = fmt.Sprintf("%s/_apis/distributedtask/pools/%d/jobrequests?$top=%d", s.metadata.organizationURL, s.metadata.poolID, s.metadata.jobsToFetch)
 	}
-	body, err := getAzurePipelineRequest(ctx, url, s.metadata, s.httpClient)
+	body, err := getAzurePipelineRequest(ctx, urlString, s.metadata, s.httpClient)
 	if err != nil {
 		return -1, err
 	}

diff --git a/pkg/scalers/azure_pipelines_scaler_test.go b/pkg/scalers/azure_pipelines_scaler_test.go
@@ -101,6 +101,8 @@ var testValidateAzurePipelinesPoolData = []validateAzurePipelinesPoolTestData{
 	{"poolName doesn't exist", map[string]string{"poolName": "sample"}, true, "poolName", http.StatusOK, `{"count":0,"value":[]}`},
 	// poolName is used if poolName and poolID are defined
 	{"poolName is used if poolName and poolID are defined", map[string]string{"poolName": "sample", "poolID": "1"}, false, "poolName", http.StatusOK, `{"count":1,"value":[{"id":1}]}`},
+	// poolName can have a space in it
+	{"poolName can have a space in it", map[string]string{"poolName": "sample pool name"}, false, "poolName", http.StatusOK, `{"count":1,"value":[{"id":1}]}`},
 }
 
 func TestValidateAzurePipelinesPool(t *testing.T) {
@@ -109,7 +111,7 @@ func TestValidateAzurePipelinesPool(t *testing.T) {
 			var apiStub = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 				_, ok := r.URL.Query()[testData.queryParam]
 				if !ok {
-					t.Error("Worng QueryParam")
+					t.Error("Wrong QueryParam")
 				}
 				w.WriteHeader(testData.httpCode)
 				_, _ = w.Write([]byte(testData.response))