feat(controller)!: Key-only artifacts. Fixes #3184 (#4618)

Signed-off-by: Alex Collins <alex_collins@intuit.com>
Co-authored-by: Saravanan Balasubramanian <33908564+sarabala1979@users.noreply.github.com>

api/jsonschema/schema.json

@@ -3340,7 +3340,6 @@
         }
       },
       "required": [
-        "bucket",
         "key"
       ],
       "type": "object"
@@ -3478,7 +3477,6 @@
         }
       },
       "required": [
-        "addresses",
         "path"
       ],
       "type": "object"
@@ -3936,10 +3934,6 @@
         }
       },
       "required": [
-        "endpoint",
-        "bucket",
-        "accessKeySecret",
-        "secretKeySecret",
         "key"
       ],
       "type": "object"
@@ -4202,10 +4196,6 @@
         }
       },
       "required": [
-        "endpoint",
-        "bucket",
-        "accessKeySecret",
-        "secretKeySecret",
         "key"
       ],
       "type": "object"

api/openapi-spec/swagger.json

@@ -6741,7 +6741,6 @@
       "description": "GCSArtifact is the location of a GCS artifact",
       "type": "object",
       "required": [
-        "bucket",
         "key"
       ],
       "properties": {
@@ -6848,7 +6847,6 @@
       "description": "HDFSArtifact is the location of an HDFS artifact",
       "type": "object",
       "required": [
-        "addresses",
         "path"
       ],
       "properties": {
@@ -7329,10 +7327,6 @@
       "description": "OSSArtifact is the location of an Alibaba Cloud OSS artifact",
       "type": "object",
       "required": [
-        "endpoint",
-        "bucket",
-        "accessKeySecret",
-        "secretKeySecret",
         "key"
       ],
       "properties": {
@@ -7575,10 +7569,6 @@
       "description": "S3Artifact is the location of an S3 artifact",
       "type": "object",
       "required": [
-        "endpoint",
-        "bucket",
-        "accessKeySecret",
-        "secretKeySecret",
         "key"
       ],
       "properties": {

config/config.go

@@ -2,12 +2,14 @@ package config
 
 import (
 	"fmt"
+	"path"
 
 	apiv1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 
 	wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
+	"github.com/argoproj/argo/workflow/common"
 )
 
 var EmptyConfigFunc = func() interface{} { return &Config{} }
@@ -138,6 +140,41 @@ func (a *ArtifactRepository) IsArchiveLogs() bool {
 	return a != nil && a.ArchiveLogs != nil && *a.ArchiveLogs
 }
 
+type ArtifactRepositoryType interface {
+	IntoArtifactLocation(l *wfv1.ArtifactLocation)
+}
+
+func (a *ArtifactRepository) Get() ArtifactRepositoryType {
+	if a == nil {
+		return nil
+	} else if a.Artifactory != nil {
+		return a.Artifactory
+	} else if a.GCS != nil {
+		return a.GCS
+	} else if a.HDFS != nil {
+		return a.HDFS
+	} else if a.OSS != nil {
+		return a.OSS
+	} else if a.S3 != nil {
+		return a.S3
+	}
+	return nil
+}
+
+// ToArtifactLocation returns the artifact location set with default template key:
+// key = `{{workflow.name}}/{{pod.name}}`
+func (a *ArtifactRepository) ToArtifactLocation() *wfv1.ArtifactLocation {
+	if a == nil {
+		return nil
+	}
+	l := &wfv1.ArtifactLocation{ArchiveLogs: a.ArchiveLogs}
+	v := a.Get()
+	if v != nil {
+		v.IntoArtifactLocation(l)
+	}
+	return l
+}
+
 type PersistConfig struct {
 	NodeStatusOffload bool `json:"nodeStatusOffLoad,omitempty"`
 	// Archive workflows to persistence.
@@ -212,6 +249,14 @@ type S3ArtifactRepository struct {
 	KeyPrefix string `json:"keyPrefix,omitempty"`
 }
 
+func (r *S3ArtifactRepository) IntoArtifactLocation(l *wfv1.ArtifactLocation) {
+	k := r.KeyFormat
+	if k == "" {
+		k = path.Join(r.KeyPrefix, common.DefaultArchivePattern)
+	}
+	l.S3 = &wfv1.S3Artifact{S3Bucket: r.S3Bucket, Key: k}
+}
+
 // OSSArtifactRepository defines the controller configuration for an OSS artifact repository
 type OSSArtifactRepository struct {
 	wfv1.OSSBucket `json:",inline"`
@@ -220,6 +265,14 @@ type OSSArtifactRepository struct {
 	KeyFormat string `json:"keyFormat,omitempty"`
 }
 
+func (r *OSSArtifactRepository) IntoArtifactLocation(l *wfv1.ArtifactLocation) {
+	k := r.KeyFormat
+	if k == "" {
+		k = common.DefaultArchivePattern
+	}
+	l.OSS = &wfv1.OSSArtifact{OSSBucket: r.OSSBucket, Key: k}
+}
+
 // GCSArtifactRepository defines the controller configuration for a GCS artifact repository
 type GCSArtifactRepository struct {
 	wfv1.GCSBucket `json:",inline"`
@@ -228,13 +281,30 @@ type GCSArtifactRepository struct {
 	KeyFormat string `json:"keyFormat,omitempty"`
 }
 
+func (r *GCSArtifactRepository) IntoArtifactLocation(l *wfv1.ArtifactLocation) {
+	k := r.KeyFormat
+	if k == "" {
+		k = common.DefaultArchivePattern
+	}
+	l.GCS = &wfv1.GCSArtifact{GCSBucket: r.GCSBucket, Key: k}
+}
+
 // ArtifactoryArtifactRepository defines the controller configuration for an artifactory artifact repository
 type ArtifactoryArtifactRepository struct {
 	wfv1.ArtifactoryAuth `json:",inline"`
 	// RepoURL is the url for artifactory repo.
 	RepoURL string `json:"repoURL,omitempty"`
 }
 
+func (r *ArtifactoryArtifactRepository) IntoArtifactLocation(l *wfv1.ArtifactLocation) {
+	u := ""
+	if r.RepoURL != "" {
+		u = r.RepoURL + "/"
+	}
+	u = fmt.Sprintf("%s%s", u, common.DefaultArchivePattern)
+	l.Artifactory = &wfv1.ArtifactoryArtifact{ArtifactoryAuth: r.ArtifactoryAuth, URL: u}
+}
+
 // HDFSArtifactRepository defines the controller configuration for an HDFS artifact repository
 type HDFSArtifactRepository struct {
 	wfv1.HDFSConfig `json:",inline"`
@@ -246,6 +316,14 @@ type HDFSArtifactRepository struct {
 	Force bool `json:"force,omitempty"`
 }
 
+func (r *HDFSArtifactRepository) IntoArtifactLocation(l *wfv1.ArtifactLocation) {
+	p := r.PathFormat
+	if p == "" {
+		p = common.DefaultArchivePattern
+	}
+	l.HDFS = &wfv1.HDFSArtifact{HDFSConfig: r.HDFSConfig, Path: p, Force: r.Force}
+}
+
 // MetricsConfig defines a config for a metrics server
 type MetricsConfig struct {
 	// Enabled controls metric emission. Default is true, set "enabled: false" to turn off

config/config_test.go

@@ -7,6 +7,60 @@ import (
 	"k8s.io/utils/pointer"
 )
 
+func TestArtifactRepository(t *testing.T) {
+	t.Run("Nil", func(t *testing.T) {
+		var r *ArtifactRepository
+		assert.Nil(t, r.Get())
+		l := r.ToArtifactLocation()
+		assert.Nil(t, l)
+	})
+	t.Run("ArchiveLogs", func(t *testing.T) {
+		r := &ArtifactRepository{Artifactory: &ArtifactoryArtifactRepository{}, ArchiveLogs: pointer.BoolPtr(true)}
+		l := r.ToArtifactLocation()
+		assert.Equal(t, pointer.BoolPtr(true), l.ArchiveLogs)
+	})
+	t.Run("Artifactory", func(t *testing.T) {
+		r := &ArtifactRepository{Artifactory: &ArtifactoryArtifactRepository{RepoURL: "http://my-repo"}}
+		assert.IsType(t, &ArtifactoryArtifactRepository{}, r.Get())
+		l := r.ToArtifactLocation()
+		if assert.NotNil(t, l.Artifactory) {
+			assert.Equal(t, "http://my-repo/{{workflow.name}}/{{pod.name}}", l.Artifactory.URL)
+		}
+	})
+	t.Run("GCS", func(t *testing.T) {
+		r := &ArtifactRepository{GCS: &GCSArtifactRepository{}}
+		assert.IsType(t, &GCSArtifactRepository{}, r.Get())
+		l := r.ToArtifactLocation()
+		if assert.NotNil(t, l.GCS) {
+			assert.Equal(t, "{{workflow.name}}/{{pod.name}}", l.GCS.Key)
+		}
+	})
+	t.Run("HDFS", func(t *testing.T) {
+		r := &ArtifactRepository{HDFS: &HDFSArtifactRepository{}}
+		assert.IsType(t, &HDFSArtifactRepository{}, r.Get())
+		l := r.ToArtifactLocation()
+		if assert.NotNil(t, l.HDFS) {
+			assert.Equal(t, "{{workflow.name}}/{{pod.name}}", l.HDFS.Path)
+		}
+	})
+	t.Run("OSS", func(t *testing.T) {
+		r := &ArtifactRepository{OSS: &OSSArtifactRepository{}}
+		assert.IsType(t, &OSSArtifactRepository{}, r.Get())
+		l := r.ToArtifactLocation()
+		if assert.NotNil(t, l.OSS) {
+			assert.Equal(t, "{{workflow.name}}/{{pod.name}}", l.OSS.Key)
+		}
+	})
+	t.Run("S3", func(t *testing.T) {
+		r := &ArtifactRepository{S3: &S3ArtifactRepository{KeyPrefix: "my-key-prefix"}}
+		assert.IsType(t, &S3ArtifactRepository{}, r.Get())
+		l := r.ToArtifactLocation()
+		if assert.NotNil(t, l.S3) {
+			assert.Equal(t, "my-key-prefix/{{workflow.name}}/{{pod.name}}", l.S3.Key)
+		}
+	})
+}
+
 func TestArtifactRepository_IsArchiveLogs(t *testing.T) {
 	assert.False(t, (&ArtifactRepository{}).IsArchiveLogs())
 	assert.False(t, (&ArtifactRepository{ArchiveLogs: pointer.BoolPtr(false)}).IsArchiveLogs())

docs/artifact-repository-ref.md

@@ -15,7 +15,7 @@ metadata:
     # if you want to use this config map by default - name it "artifact-repositories" 
   name: artifact-repositories
   annotations:
-    # if you want to use a specific key, put that's key into this annotation 
+    # v3.0 and after - if you want to use a specific key, put that's key into this annotation 
     workflows.argoproj.io/default-artifact-repository: default-v1
 data:
   default-v1: |
@@ -40,4 +40,6 @@ spec:
     key: my-key # default can be set by the annotation
 ```
 
+This feature gives maximum benefit when used with [key-only artifacts](key-only-artifacts.md).
+
 Reference: [fields.md#artifactrepositoryref](fields.md#artifactrepositoryref).