fix: Fixed artifact retrieval when templateRef in use. Fixes #9631, #…

…9644. (#9648) * fix: Fixed artifact retrieval when templateRef in use. Fixes #9631. Signed-off-by: Brian Loss <[email protected]> * chore: Address review feedback - use util method * Expose getTemplateFromNode in workflow/util/util.go * Update uses of getTemplateFromNode to GetTemplateFromNode * Call GetTemplateFromNode in artifact_server.go Signed-off-by: Brian Loss <[email protected]> Signed-off-by: Brian Loss <[email protected]>
argoproj · Sep 21, 2022 · e556fe3 · e556fe3
1 parent ecdeedd
commit e556fe3
Show file tree

Hide file tree

Showing 4 changed files with 80 additions and 7 deletions.
diff --git a/server/artifacts/artifact_server.go b/server/artifacts/artifact_server.go
@@ -28,6 +28,7 @@ import (
 	artifact "github.com/argoproj/argo-workflows/v3/workflow/artifacts"
 	"github.com/argoproj/argo-workflows/v3/workflow/artifacts/common"
 	"github.com/argoproj/argo-workflows/v3/workflow/hydrator"
+	"github.com/argoproj/argo-workflows/v3/workflow/util"
 )
 
 type ArtifactServer struct {
@@ -390,7 +391,7 @@ func (a *ArtifactServer) getArtifactAndDriver(ctx context.Context, nodeId, artif
 	// 3. Workflow spec defines artifactRepositoryRef which is a ConfigMap which defines the location
 	// 4. Template defines ArchiveLocation
 
-	templateName := wf.Status.Nodes[nodeId].TemplateName
+	templateName := util.GetTemplateFromNode(wf.Status.Nodes[nodeId])
 	template := wf.GetTemplateByName(templateName)
 	if template == nil {
 		return nil, nil, fmt.Errorf("no template found by the name of '%s' (which is the template associated with nodeId '%s'??", templateName, nodeId)

diff --git a/server/artifacts/artifact_server_test.go b/server/artifacts/artifact_server_test.go
@@ -54,7 +54,7 @@ func (a *fakeArtifactDriver) Load(_ *wfv1.Artifact, path string) error {
 }
 
 var bucketsOfKeys = map[string][]string{
-	"my-bucket": []string{
+	"my-bucket": {
 		"my-wf/my-node-1/my-s3-input-artifact.tgz",
 		"my-wf/my-node-1/my-s3-artifact-directory",
 		"my-wf/my-node-1/my-s3-artifact-directory/a.txt",
@@ -64,12 +64,15 @@ var bucketsOfKeys = map[string][]string{
 		"my-wf/my-node-1/my-oss-artifact.zip",
 		"my-wf/my-node-1/my-s3-artifact.tgz",
 	},
-	"my-bucket-2": []string{
+	"my-bucket-2": {
 		"my-wf/my-node-2/my-s3-artifact-bucket-2",
 	},
-	"my-bucket-3": []string{
+	"my-bucket-3": {
 		"my-wf/my-node-2/my-s3-artifact-bucket-3",
 	},
+	"my-bucket-4": {
+		"my-wf/my-node-3/my-s3-artifact.tgz",
+	},
 }
 
 func (a *fakeArtifactDriver) OpenStream(artifact *wfv1.Artifact) (io.ReadCloser, error) {
@@ -278,9 +281,46 @@ func newServer() *ArtifactServer {
 						},
 					},
 				},
+
+				"my-node-3": wfv1.NodeStatus{
+					TemplateRef: &wfv1.TemplateRef{
+						Name:     "my-template",
+						Template: "template-3",
+					},
+					Outputs: &wfv1.Outputs{
+						Artifacts: wfv1.Artifacts{
+							{
+								Name: "my-s3-artifact",
+								ArtifactLocation: wfv1.ArtifactLocation{
+									S3: &wfv1.S3Artifact{
+										// S3 is a configured artifact repo, so does not need key
+										Key: "my-wf/my-node-3/my-s3-artifact.tgz",
+										S3Bucket: wfv1.S3Bucket{
+											Bucket:   "my-bucket-4",
+											Endpoint: "minio:9000",
+										},
+									},
+								},
+							},
+						},
+					},
+				},
 				// a node without input/output artifacts
 				"my-node-no-artifacts": wfv1.NodeStatus{},
 			},
+			StoredTemplates: map[string]wfv1.Template{
+				"namespaced/my-template/template-3": {
+					Name: "template-3",
+					Outputs: wfv1.Outputs{
+						Artifacts: wfv1.Artifacts{
+							{
+								Name: "my-s3-artifact",
+								Path: "my-s3-artifact.tgz",
+							},
+						},
+					},
+				},
+			},
 		},
 	}
 	argo := fakewfv1.NewSimpleClientset(wf, &wfv1.Workflow{
@@ -448,6 +488,38 @@ func TestArtifactServer_GetOutputArtifact(t *testing.T) {
 	}
 }
 
+func TestArtifactServer_GetOutputArtifactWithTemplate(t *testing.T) {
+	s := newServer()
+
+	tests := []struct {
+		fileName     string
+		artifactName string
+	}{
+		{
+			fileName:     "my-s3-artifact.tgz",
+			artifactName: "my-s3-artifact",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.artifactName, func(t *testing.T) {
+			r := &http.Request{}
+			r.URL = mustParse(fmt.Sprintf("/artifacts/my-ns/my-wf/my-node-3/%s", tt.artifactName))
+			recorder := httptest.NewRecorder()
+
+			s.GetOutputArtifact(recorder, r)
+			if assert.Equal(t, 200, recorder.Result().StatusCode) {
+				assert.Equal(t, fmt.Sprintf(`filename="%s"`, tt.fileName), recorder.Header().Get("Content-Disposition"))
+				all, err := io.ReadAll(recorder.Result().Body)
+				if err != nil {
+					panic(fmt.Sprintf("failed to read http body: %v", err))
+				}
+				assert.Equal(t, "my-data", string(all))
+			}
+		})
+	}
+}
+
 func TestArtifactServer_GetInputArtifact(t *testing.T) {
 	s := newServer()
 

diff --git a/workflow/util/util.go b/workflow/util/util.go
@@ -755,7 +755,7 @@ func getDescendantNodeIDs(wf *wfv1.Workflow, node wfv1.NodeStatus) []string {
 }
 
 func deletePodNodeDuringRetryWorkflow(wf *wfv1.Workflow, node wfv1.NodeStatus, deletedPods map[string]bool, podsToDelete []string) (map[string]bool, []string) {
-	templateName := getTemplateFromNode(node)
+	templateName := GetTemplateFromNode(node)
 	version := GetWorkflowPodNameVersion(wf)
 	podName := PodName(wf.Name, node.Name, templateName, node.ID, version)
 	if _, ok := deletedPods[podName]; !ok {
@@ -999,7 +999,7 @@ func resetNode(node wfv1.NodeStatus) wfv1.NodeStatus {
 	return node
 }
 
-func getTemplateFromNode(node wfv1.NodeStatus) string {
+func GetTemplateFromNode(node wfv1.NodeStatus) string {
 	if node.TemplateRef != nil {
 		return node.TemplateRef.Template
 	}

diff --git a/workflow/util/util_test.go b/workflow/util/util_test.go
@@ -1083,7 +1083,7 @@ func TestGetTemplateFromNode(t *testing.T) {
 	}
 
 	for _, tc := range cases {
-		actual := getTemplateFromNode(tc.inputNode)
+		actual := GetTemplateFromNode(tc.inputNode)
 		assert.Equal(t, tc.expectedTemplateName, actual)
 	}
 }