Repository: argoproj/argo
Pull request #3636 (2020-07-29 18:30:11)

fix: Fix 'malformed request: field selector' error
Fixes https://github.com/argoproj/argo/issues/3625


Patch:
diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go
index 2aaa20c0c..5df28cac3 100644
--- a/server/workflow/workflow_server.go
+++ b/server/workflow/workflow_server.go
@@ -142,11 +142,10 @@ func (s *workflowServer) WatchWorkflows(req *workflowpkg.WatchWorkflowsRequest,
 	opts := &metav1.ListOptions{}
 	if req.ListOptions != nil {
 		opts = req.ListOptions
-		wfName, err := argoutil.RecoverWorkflowNameFromSelectorString(opts.FieldSelector)
-		if err != nil {
-			return fmt.Errorf("malformed request: workflows must be specified with a 'metadata.name=...' field selector; unable to parse: %w", err)
-		}
-		if len(wfName) > 0 {
+		wfName := argoutil.RecoverWorkflowNameFromSelectorStringIfAny(opts.FieldSelector)
+		if wfName != "" {
+			// If we are using an alias (such as `@latest`) we need to dereference it.
+			// s.getWorkflow does that for us
 			wf, err := s.getWorkflow(wfClient, req.Namespace, wfName, metav1.GetOptions{})
 			if err != nil {
 				return err
diff --git a/util/util.go b/util/util.go
index dc18f8bde..8ab63c6c6 100644
--- a/util/util.go
+++ b/util/util.go
@@ -96,22 +96,21 @@ func RecoverIndexFromNodeName(name string) int {
 
 func GenerateFieldSelectorFromWorkflowName(wfName string) string {
 	result := fields.ParseSelectorOrDie(fmt.Sprintf("metadata.name=%s", wfName)).String()
-	compare, err := RecoverWorkflowNameFromSelectorString(result)
-	if err != nil || wfName != compare {
-		log.WithFields(log.Fields{"wfName": wfName}).Error(err)
+	compare := RecoverWorkflowNameFromSelectorStringIfAny(result)
+	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, error) {
-	nameIndex := strings.Index(selector, "=")
-	if nameIndex < 0 {
-		return "", fmt.Errorf("incorrect prefix: selector is not formatted correctly")
-	}
-	prefix := selector[:nameIndex]
-	if prefix != "metadata.name" {
-		return "", fmt.Errorf("incorrect prefix: expected 'metadata.name' but got '%s'", prefix)
+func RecoverWorkflowNameFromSelectorStringIfAny(selector string) string {
+	const tag = "metadata.name="
+	if starts := strings.Index(selector, tag); starts > -1 {
+		suffix := selector[starts+len(tag):]
+		if ends := strings.Index(suffix, ","); ends > -1 {
+			return strings.TrimSpace(suffix[:ends])
+		}
+		return strings.TrimSpace(suffix)
 	}
-	return selector[nameIndex+1:], nil
+	return ""
 }


Test patch:
diff --git a/util/util_test.go b/util/util_test.go
index ae32030a4..a4b9b7bf3 100644
--- a/util/util_test.go
+++ b/util/util_test.go
@@ -37,23 +37,30 @@ func TestRecoverWorkflowNameFromSelectorString(t *testing.T) {
 	}{
 		{"TestRecoverWorkflowNameFromSelectorString", args{"metadata.name=whalesay"}, "whalesay"},
 		{"TestRecoverWorkflowNameFromSelectorStringEmptyWf", args{"metadata.name="}, ""},
+		{"TestRecoverWorkflowNameFromSelectorStringEmptyWf", args{"metadata.name=whalesay,other=hello"}, "whalesay"},
+		{"TestRecoverWorkflowNameFromSelectorStringEmptyWf", args{"foo=bar,metadata.name=whalesay,other=hello"}, "whalesay"},
+		{"TestRecoverWorkflowNameFromSelectorStringEmptyWf", args{"foo=bar,metadata.name=whalesay"}, "whalesay"},
+		{"TestRecoverWorkflowNameFromSelectorStringEmptyWf", args{"foo=bar,metadata.name= whalesay ,other=hello"}, "whalesay"},
+		{"TestRecoverWorkflowNameFromSelectorStringEmptyWf", args{"foo=bar,other=hello"}, ""},
+		{"TestRecoverWorkflowNameFromSelectorString", args{"metadata.name=@latest"}, "@latest"},
+		{"TestRecoverWorkflowNameFromSelectorStringEmptyWf", args{"metadata.name="}, ""},
+		{"TestRecoverWorkflowNameFromSelectorStringEmptyWf", args{"metadata.name=@latest,other=hello"}, "@latest"},
+		{"TestRecoverWorkflowNameFromSelectorStringEmptyWf", args{"foo=bar,metadata.name=@latest,other=hello"}, "@latest"},
+		{"TestRecoverWorkflowNameFromSelectorStringEmptyWf", args{"foo=bar,metadata.name=@latest"}, "@latest"},
+		{"TestRecoverWorkflowNameFromSelectorStringEmptyWf", args{"foo=bar,metadata.name= @latest ,other=hello"}, "@latest"},
+		{"TestRecoverWorkflowNameFromSelectorStringEmptyWf", args{"foo=bar,other=hello"}, ""},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := RecoverWorkflowNameFromSelectorString(tt.args.selector)
-			assert.NoError(t, err)
+			got := RecoverWorkflowNameFromSelectorStringIfAny(tt.args.selector)
 			if got != tt.want {
-				t.Errorf("RecoverWorkflowNameFromSelectorString() = %v, want %v", got, tt.want)
+				t.Errorf("RecoverWorkflowNameFromSelectorStringIfAny() = %v, want %v", got, tt.want)
 			}
 		})
 	}
-}
-
-func TestRecoverWorkflowNameFromSelectorStringError(t *testing.T) {
-	name, err := RecoverWorkflowNameFromSelectorString("whatever=whalesay")
-	assert.NotNil(t, err)
+	name := RecoverWorkflowNameFromSelectorStringIfAny("whatever=whalesay")
 	assert.Equal(t, name, "")
 	assert.NotPanics(t, func() {
-		_, _ = RecoverWorkflowNameFromSelectorString("whatever")
+		_ = RecoverWorkflowNameFromSelectorStringIfAny("whatever")
 	})
 }