diff --git a/pkg/artifacts/artifact_storage_test.go b/pkg/artifacts/artifact_storage_test.go index 15184735f85..69f0bcd2930 100644 --- a/pkg/artifacts/artifact_storage_test.go +++ b/pkg/artifacts/artifact_storage_test.go @@ -22,12 +22,14 @@ import ( "github.com/google/go-cmp/cmp" "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1" "github.com/knative/build-pipeline/pkg/system" + logtesting "github.com/knative/pkg/logging/testing" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" fakek8s "k8s.io/client-go/kubernetes/fake" ) -func TestInitializeArtifactStorage_WithConfigMap(t *testing.T) { +func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { + logger := logtesting.TestLogger(t) for _, c := range []struct { desc string configMap *corev1.ConfigMap @@ -107,6 +109,24 @@ func TestInitializeArtifactStorage_WithConfigMap(t *testing.T) { Name: "pipelineruntest", }, storagetype: "pvc", + }, { + desc: "no config map data", + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace, + Name: v1alpha1.BucketConfigName, + }, + }, + pipelinerun: &v1alpha1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "foo", + Name: "pipelineruntest", + }, + }, + expectedArtifactStorage: &v1alpha1.ArtifactPVC{ + Name: "pipelineruntest", + }, + storagetype: "pvc", }, { desc: "no secret", configMap: &corev1.ConfigMap{ @@ -131,7 +151,7 @@ func TestInitializeArtifactStorage_WithConfigMap(t *testing.T) { }} { t.Run(c.desc, func(t *testing.T) { fakekubeclient := fakek8s.NewSimpleClientset(c.configMap) - bucket, err := InitializeArtifactStorage(c.pipelinerun, fakekubeclient) + bucket, err := InitializeArtifactStorage(c.pipelinerun, fakekubeclient, logger) if err != nil { t.Fatalf("Somehow had error initializing artifact storage run out of fake client: %s", err) } @@ -145,7 +165,8 @@ func TestInitializeArtifactStorage_WithConfigMap(t *testing.T) { } } -func TestInitializeArtifactStorage_WithoutConfigMap(t *testing.T) { +func TestInitializeArtifactStorageWithoutConfigMap(t *testing.T) { + logger := logtesting.TestLogger(t) fakekubeclient := fakek8s.NewSimpleClientset() pipelinerun := &v1alpha1.PipelineRun{ ObjectMeta: metav1.ObjectMeta{ @@ -154,7 +175,7 @@ func TestInitializeArtifactStorage_WithoutConfigMap(t *testing.T) { }, } - pvc, err := InitializeArtifactStorage(pipelinerun, fakekubeclient) + pvc, err := InitializeArtifactStorage(pipelinerun, fakekubeclient, logger) if err != nil { t.Fatalf("Somehow had error initializing artifact storage run out of fake client: %s", err) } @@ -168,9 +189,16 @@ func TestInitializeArtifactStorage_WithoutConfigMap(t *testing.T) { } } -func TestGetArtifactStorage_WithConfigMap(t *testing.T) { - fakekubeclient := fakek8s.NewSimpleClientset( - &corev1.ConfigMap{ +func TestGetArtifactStorageWithConfigMap(t *testing.T) { + logger := logtesting.TestLogger(t) + prName := "pipelineruntest" + for _, c := range []struct { + desc string + configMap *corev1.ConfigMap + expectedArtifactStorage ArtifactStorageInterface + }{{ + desc: "valid bucket", + configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.Namespace, Name: v1alpha1.BucketConfigName, @@ -181,30 +209,70 @@ func TestGetArtifactStorage_WithConfigMap(t *testing.T) { v1alpha1.BucketServiceAccountSecretKey: "sakey", }, }, - ) - - bucket, err := GetArtifactStorage("pipelineruntest", fakekubeclient) - if err != nil { - t.Fatalf("Somehow had error initializing artifact storage run out of fake client: %s", err) - } + expectedArtifactStorage: &v1alpha1.ArtifactBucket{ + Location: "gs://fake-bucket", + Secrets: []v1alpha1.SecretParam{{ + FieldName: "GOOGLE_APPLICATION_CREDENTIALS", + SecretKey: "sakey", + SecretName: "secret1", + }}, + }, + }, { + desc: "location empty", + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace, + Name: v1alpha1.BucketConfigName, + }, + Data: map[string]string{ + v1alpha1.BucketLocationKey: "", + v1alpha1.BucketServiceAccountSecretName: "secret1", + v1alpha1.BucketServiceAccountSecretKey: "sakey", + }, + }, + expectedArtifactStorage: &v1alpha1.ArtifactPVC{Name: prName}, + }, { + desc: "missing location", + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace, + Name: v1alpha1.BucketConfigName, + }, + Data: map[string]string{ + v1alpha1.BucketServiceAccountSecretName: "secret1", + v1alpha1.BucketServiceAccountSecretKey: "sakey", + }, + }, + expectedArtifactStorage: &v1alpha1.ArtifactPVC{Name: prName}, + }, { + desc: "no config map data", + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace, + Name: v1alpha1.BucketConfigName, + }, + }, + expectedArtifactStorage: &v1alpha1.ArtifactPVC{Name: prName}, + }} { + t.Run(c.desc, func(t *testing.T) { + fakekubeclient := fakek8s.NewSimpleClientset(c.configMap) - expectedArtifactBucket := &v1alpha1.ArtifactBucket{ - Location: "gs://fake-bucket", - Secrets: []v1alpha1.SecretParam{{ - FieldName: "GOOGLE_APPLICATION_CREDENTIALS", - SecretKey: "sakey", - SecretName: "secret1", - }}, - } + bucket, err := GetArtifactStorage(prName, fakekubeclient, logger) + if err != nil { + t.Fatalf("Somehow had error initializing artifact storage run out of fake client: %s", err) + } - if diff := cmp.Diff(bucket, expectedArtifactBucket); diff != "" { - t.Fatalf("want %v, but got %v", expectedArtifactBucket, bucket) + if diff := cmp.Diff(bucket, c.expectedArtifactStorage); diff != "" { + t.Fatalf("want %v, but got %v", c.expectedArtifactStorage, bucket) + } + }) } } -func TestGetArtifactStorage_WithoutConfigMap(t *testing.T) { +func TestGetArtifactStorageWithoutConfigMap(t *testing.T) { + logger := logtesting.TestLogger(t) fakekubeclient := fakek8s.NewSimpleClientset() - pvc, err := GetArtifactStorage("pipelineruntest", fakekubeclient) + pvc, err := GetArtifactStorage("pipelineruntest", fakekubeclient, logger) if err != nil { t.Fatalf("Somehow had error initializing artifact storage run out of fake client: %s", err) } diff --git a/pkg/artifacts/artifacts_storage.go b/pkg/artifacts/artifacts_storage.go index dc7946c6ae5..1dfe562f3f8 100644 --- a/pkg/artifacts/artifacts_storage.go +++ b/pkg/artifacts/artifacts_storage.go @@ -22,6 +22,7 @@ import ( "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1" "github.com/knative/build-pipeline/pkg/system" + "go.uber.org/zap" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" @@ -41,23 +42,13 @@ type ArtifactStorageInterface interface { // InitializeArtifactStorage will check if there is there is a // bucket configured or create a PVC -func InitializeArtifactStorage(pr *v1alpha1.PipelineRun, c kubernetes.Interface) (ArtifactStorageInterface, error) { - needPVC := false +func InitializeArtifactStorage(pr *v1alpha1.PipelineRun, c kubernetes.Interface, logger *zap.SugaredLogger) (ArtifactStorageInterface, error) { configMap, err := c.CoreV1().ConfigMaps(system.Namespace).Get(v1alpha1.BucketConfigName, metav1.GetOptions{}) + shouldCreatePVC, err := needsPVC(configMap, err, logger) if err != nil { - needPVC = true + return nil, err } - if configMap != nil && configMap.Data != nil { - if _, ok := configMap.Data[v1alpha1.BucketLocationKey]; !ok { - needPVC = true - } else { - location, _ := configMap.Data[v1alpha1.BucketLocationKey] - if strings.TrimSpace(location) == "" { - needPVC = true - } - } - } - if needPVC { + if shouldCreatePVC { err = createPVC(pr, c) if err != nil { return nil, err @@ -68,11 +59,40 @@ func InitializeArtifactStorage(pr *v1alpha1.PipelineRun, c kubernetes.Interface) return NewArtifactBucketConfigFromConfigMap(configMap) } +func needsPVC(configMap *corev1.ConfigMap, err error, logger *zap.SugaredLogger) (bool, error) { + if err != nil { + if errors.IsNotFound(err) { + return true, nil + } + return false, err + } + if configMap == nil { + return true, nil + } + if configMap.Data == nil { + logger.Warn("the configmap has no data") + return true, nil + } + if location, ok := configMap.Data[v1alpha1.BucketLocationKey]; !ok { + return true, nil + } else { + logger.Warnf("the configmap key %q is empty", v1alpha1.BucketLocationKey) + if strings.TrimSpace(location) == "" { + return true, nil + } + } + return false, nil +} + // GetArtifactStorage returns the storage interface to enable // consumer code to get a container step for copy to/from storage -func GetArtifactStorage(prName string, c kubernetes.Interface) (ArtifactStorageInterface, error) { +func GetArtifactStorage(prName string, c kubernetes.Interface, logger *zap.SugaredLogger) (ArtifactStorageInterface, error) { configMap, err := c.CoreV1().ConfigMaps(system.Namespace).Get(v1alpha1.BucketConfigName, metav1.GetOptions{}) + pvc, err := needsPVC(configMap, err, logger) if err != nil { + return nil, err + } + if pvc { return &v1alpha1.ArtifactPVC{Name: prName}, nil } return NewArtifactBucketConfigFromConfigMap(configMap) diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go index f6932e00bdf..d1a9edf9910 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go @@ -302,7 +302,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er rprt := resources.GetNextTask(pr.Name, pipelineState, c.Logger) var as artifacts.ArtifactStorageInterface - if as, err = artifacts.InitializeArtifactStorage(pr, c.KubeClientSet); err != nil { + if as, err = artifacts.InitializeArtifactStorage(pr, c.KubeClientSet, c.Logger); err != nil { c.Logger.Infof("PipelineRun failed to initialize artifact storage %s", pr.Name) return err } diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go b/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go index e39bad79ba1..fe9eb00b8d6 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go @@ -72,7 +72,7 @@ func AddInputResource( if prNameFromLabel == "" { prNameFromLabel = pvcName } - as, err := artifacts.GetArtifactStorage(prNameFromLabel, kubeclient) + as, err := artifacts.GetArtifactStorage(prNameFromLabel, kubeclient, logger) if err != nil { return nil, err } diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go b/pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go index cc05c223ec8..b2bdf169b2a 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go @@ -67,7 +67,7 @@ func AddOutputResources( } pvcName := taskRun.GetPipelineRunPVCName() - as, err := artifacts.GetArtifactStorage(pvcName, kubeclient) + as, err := artifacts.GetArtifactStorage(pvcName, kubeclient, logger) if err != nil { return err }