Skip to content

Commit

Permalink
Fix pvc vs bucket detection case 🌵
Browse files Browse the repository at this point in the history 
Depending on the `config-artifact-bucket` configMap, `build-pipeline`
will create either a PVC or GCS Bucket for sharing output between
task(run)s in the pipeline. There was a bug (or missing case) in the
code that checks if it needs to create PVC.

We want to create a PVC in the following cases:
- the `config-artifact-bucket` configMap is missing
- the `config-artifact-bucket` configMap data is empty
- the `config-artifact-bucket` configMap has no
`BucketLocationKey` (aka `location`) *or* it's value is empty.

The 2nd case wasn't correctly handled between
`InitializedArtifactStorage` and `GetArtifactStorage` functions.

Signed-off-by: Vincent Demeester <[email protected]>
  • Loading branch information
@vdemeester
vdemeester committed Feb 6, 2019
1 parent 6cdf87e commit c8f5430
Show file tree
Hide file tree
Showing 5 changed files with 131 additions and 43 deletions.
118 changes: 93 additions & 25 deletions pkg/artifacts/artifact_storage_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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{
Expand All @@ -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)
}
Expand All @@ -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{
Expand All @@ -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)
}
Expand All @@ -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,
Expand All @@ -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)
}
Expand Down
50 changes: 35 additions & 15 deletions pkg/artifacts/artifacts_storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down

0 comments on commit c8f5430

Please sign in to comment.