Repository: argoproj/argo
Pull request #3596 (2020-07-24 23:24:14)

fix(controller): Fix bug in util/RecoverWorkflowNameFromSelectorString. Add error handling
Checklist:

* [x] Either (a) I've created an [enhancement proposal](https://github.com/argoproj/argo/issues/new/choose) and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
* [x] The title of the PR is (a) [conventional](https://www.conventionalcommits.org/en/v1.0.0/), (b) states what changed, and (c) suffixes the related issues number. E.g. `"fix(controller): Updates such and such. Fixes #1234"`.  
* [x] I've signed the CLA.
* [x] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
* [ ] My builds are green. Try syncing with master if they are not. 
* [x] My organization is added to [USERS.md](https://github.com/argoproj/argo/blob/master/USERS.md).



Patch:
diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go
index 7dbfab22b..0263d7200 100644
--- a/server/workflow/workflow_server.go
+++ b/server/workflow/workflow_server.go
@@ -143,7 +143,10 @@ func (s *workflowServer) WatchWorkflows(req *workflowpkg.WatchWorkflowsRequest,
 	opts := &metav1.ListOptions{}
 	if req.ListOptions != nil {
 		opts = req.ListOptions
-		wfName := argoutil.RecoverWorkflowNameFromSelectorString(opts.FieldSelector)
+		wfName, err := argoutil.RecoverWorkflowNameFromSelectorString(opts.FieldSelector)
+		if err != nil {
+			return err
+		}
 		if len(wfName) > 0 {
 			wf, err := s.getWorkflow(wfClient, req.Namespace, wfName, metav1.GetOptions{})
 			if err != nil {
diff --git a/util/util.go b/util/util.go
index 49169db5e..a524b9190 100644
--- a/util/util.go
+++ b/util/util.go
@@ -96,14 +96,22 @@ func RecoverIndexFromNodeName(name string) int {
 
 func GenerateFieldSelectorFromWorkflowName(wfName string) string {
 	result := fields.ParseSelectorOrDie(fmt.Sprintf("metadata.name=%s", wfName)).String()
-	if compare := RecoverWorkflowNameFromSelectorString(result); wfName != compare {
+	compare, err := RecoverWorkflowNameFromSelectorString(result)
+	if err != nil {
+		log.WithFields(log.Fields{"wfName": wfName}).Error(err)
+	}
+	if wfName != compare {
 		panic(fmt.Sprintf("Could not recover field selector from workflow name. Expected '%s' but got '%s'\n", wfName, compare))
 	}
 	return result
 }
 
-func RecoverWorkflowNameFromSelectorString(selector string) string {
+func RecoverWorkflowNameFromSelectorString(selector string) (string, error) {
 	nameIndex := strings.Index(selector, "=")
+	prefix := selector[:nameIndex]
+	if prefix != "metadata.name" {
+		return "", fmt.Errorf("Incorrect prefix. Expected 'metadata.name' but got '%s'", prefix)
+	}
 	name := selector[nameIndex+1:]
-	return name
+	return name, nil
 }


Test patch:
diff --git a/util/util_test.go b/util/util_test.go
index 378548f97..a3542928c 100644
--- a/util/util_test.go
+++ b/util/util_test.go
@@ -2,6 +2,8 @@ package util
 
 import (
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestGenerateFieldSelectorFromWorkflowName(t *testing.T) {
@@ -34,12 +36,21 @@ func TestRecoverWorkflowNameFromSelectorString(t *testing.T) {
 		want string
 	}{
 		{"TestRecoverWorkflowNameFromSelectorString", args{"metadata.name=whalesay"}, "whalesay"},
+		{"TestRecoverWorkflowNameFromSelectorStringEmptyWf", args{"metadata.name="}, ""},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := RecoverWorkflowNameFromSelectorString(tt.args.selector); got != tt.want {
+			got, err := RecoverWorkflowNameFromSelectorString(tt.args.selector)
+			assert.NoError(t, err)
+			if got != tt.want {
 				t.Errorf("RecoverWorkflowNameFromSelectorString() = %v, want %v", got, tt.want)
 			}
 		})
 	}
 }
+
+func TestRecoverWorkflowNameFromSelectorStringError(t *testing.T) {
+	name, err := RecoverWorkflowNameFromSelectorString("whatever=whalesay")
+	assert.NotNil(t, err)
+	assert.Equal(t, name, "")
+}