From ace664050e243a5f48c138fa01444d30eefda5b1 Mon Sep 17 00:00:00 2001 From: Andrea Frittoli Date: Tue, 14 Jul 2020 15:38:43 +0100 Subject: [PATCH] Add pipeline artifact config to the share store The pipeline controller loads the artifact storage config maps via the client on every reconcile. Refactor the artifact storage code to extract the configuration aspects and move them into the shared storage. Update the artifact storage to use config from the store. Since the artifact storage interface changed as a consequence, update all the consumers accordingly: input and out resources and the task run controller. --- config/config-artifact-bucket.yaml | 3 + pkg/apis/config/artifact_bucket.go | 112 +++++ pkg/apis/config/artifact_bucket_test.go | 107 +++++ pkg/apis/config/artifact_pvc.go | 86 ++++ pkg/apis/config/artifact_pvc_test.go | 105 +++++ pkg/apis/config/default.go | 2 +- pkg/apis/config/store.go | 36 +- pkg/apis/config/store_test.go | 12 +- .../config-artifact-bucket-all-set.yaml | 24 + .../config-artifact-bucket-empty.yaml | 26 ++ .../testdata/config-artifact-bucket.yaml | 21 + .../testdata/config-artifact-pvc-all-set.yaml | 22 + .../testdata/config-artifact-pvc-empty.yaml | 26 ++ .../config/testdata/config-artifact-pvc.yaml | 21 + pkg/apis/config/zz_generated.deepcopy.go | 32 ++ pkg/artifacts/artifact_storage_test.go | 431 +++++++----------- pkg/artifacts/artifacts_storage.go | 192 ++------ pkg/reconciler/pipelinerun/pipelinerun.go | 4 +- .../pipelinerun/pipelinerun_test.go | 20 +- .../taskrun/resources/input_resource_test.go | 39 +- .../taskrun/resources/input_resources.go | 9 +- .../taskrun/resources/output_resource.go | 10 +- .../taskrun/resources/output_resource_test.go | 64 ++- pkg/reconciler/taskrun/taskrun.go | 4 +- pkg/reconciler/taskrun/taskrun_test.go | 20 +- test/artifact_bucket_test.go | 18 +- test/v1alpha1/artifact_bucket_test.go | 18 +- 27 files changed, 942 insertions(+), 522 deletions(-) create mode 100644 pkg/apis/config/artifact_bucket.go create mode 100644 pkg/apis/config/artifact_bucket_test.go create mode 100644 pkg/apis/config/artifact_pvc.go create mode 100644 pkg/apis/config/artifact_pvc_test.go create mode 100644 pkg/apis/config/testdata/config-artifact-bucket-all-set.yaml create mode 100644 pkg/apis/config/testdata/config-artifact-bucket-empty.yaml create mode 100644 pkg/apis/config/testdata/config-artifact-bucket.yaml create mode 100644 pkg/apis/config/testdata/config-artifact-pvc-all-set.yaml create mode 100644 pkg/apis/config/testdata/config-artifact-pvc-empty.yaml create mode 100644 pkg/apis/config/testdata/config-artifact-pvc.yaml diff --git a/config/config-artifact-bucket.yaml b/config/config-artifact-bucket.yaml index 048d589b829..381b0a12cf0 100644 --- a/config/config-artifact-bucket.yaml +++ b/config/config-artifact-bucket.yaml @@ -28,3 +28,6 @@ metadata: # bucket.service.account.secret.name: # # The key in the secret with the required service account json # bucket.service.account.secret.key: +# # The field name that should be used for the service account +# # Valid values: GOOGLE_APPLICATION_CREDENTIALS, BOTO_CONFIG. +# bucket.service.account.field.name: GOOGLE_APPLICATION_CREDENTIALS \ No newline at end of file diff --git a/pkg/apis/config/artifact_bucket.go b/pkg/apis/config/artifact_bucket.go new file mode 100644 index 00000000000..30553e0bcf5 --- /dev/null +++ b/pkg/apis/config/artifact_bucket.go @@ -0,0 +1,112 @@ +/* +Copyright 2019 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config + +import ( + "os" + + corev1 "k8s.io/api/core/v1" +) + +const ( + // bucketLocationKey is the name of the configmap entry that specifies + // loction of the bucket. + BucketLocationKey = "location" + + // BucketServiceAccountSecretNameKey is the name of the configmap entry that specifies + // the name of the secret that will provide the servie account with bucket access. + // This secret must have a key called serviceaccount that will have a value with + // the service account with access to the bucket + BucketServiceAccountSecretNameKey = "bucket.service.account.secret.name" + + // BucketServiceAccountSecretKeyKey is the name of the configmap entry that specifies + // the secret key that will have a value with the service account json with access + // to the bucket + BucketServiceAccountSecretKeyKey = "bucket.service.account.secret.key" + + // DefaultBucketServiceAccountSecretKey defaults to a gcs bucket + DefaultBucketServiceFieldName = "GOOGLE_APPLICATION_CREDENTIALS" + + // bucketServiceAccountFieldName is the name of the configmap entry that specifies + // the field name that should be used for the service account. + // Valid values: GOOGLE_APPLICATION_CREDENTIALS, BOTO_CONFIG. + BucketServiceAccountFieldNameKey = "bucket.service.account.field.name" +) + +// ArtifactPVC holds the configurations for the artifacts PVC +// +k8s:deepcopy-gen=true +type ArtifactBucket struct { + Location string + ServiceAccountSecretName string + ServiceAccountSecretKey string + ServiceAccountFieldName string +} + +// GetArtifactBucketConfigName returns the name of the configmap containing all +// customizations for the storage bucket. +func GetArtifactBucketConfigName() string { + if e := os.Getenv("CONFIG_ARTIFACT_BUCKET_NAME"); e != "" { + return e + } + return "config-artifact-bucket" +} + +// Equals returns true if two Configs are identical +func (cfg *ArtifactBucket) Equals(other *ArtifactBucket) bool { + if cfg == nil && other == nil { + return true + } + + if cfg == nil || other == nil { + return false + } + + return other.Location == cfg.Location && + other.ServiceAccountSecretName == cfg.ServiceAccountSecretName && + other.ServiceAccountSecretKey == cfg.ServiceAccountSecretKey && + other.ServiceAccountFieldName == cfg.ServiceAccountFieldName +} + +// NewArtifactBucketFromMap returns a Config given a map corresponding to a ConfigMap +func NewArtifactBucketFromMap(cfgMap map[string]string) (*ArtifactBucket, error) { + tc := ArtifactBucket{ + ServiceAccountFieldName: DefaultBucketServiceFieldName, + } + + if location, ok := cfgMap[BucketLocationKey]; ok { + tc.Location = location + } + + if serviceAccountSecretName, ok := cfgMap[BucketServiceAccountSecretNameKey]; ok { + tc.ServiceAccountSecretName = serviceAccountSecretName + } + + if serviceAccountSecretKey, ok := cfgMap[BucketServiceAccountSecretKeyKey]; ok { + tc.ServiceAccountSecretKey = serviceAccountSecretKey + } + + if serviceAccountFieldName, ok := cfgMap[BucketServiceAccountFieldNameKey]; ok { + tc.ServiceAccountFieldName = serviceAccountFieldName + } + + return &tc, nil +} + +// NewArtifactBucketFromConfigMap returns a Config for the given configmap +func NewArtifactBucketFromConfigMap(config *corev1.ConfigMap) (*ArtifactBucket, error) { + return NewArtifactBucketFromMap(config.Data) +} diff --git a/pkg/apis/config/artifact_bucket_test.go b/pkg/apis/config/artifact_bucket_test.go new file mode 100644 index 00000000000..81ca0e4043a --- /dev/null +++ b/pkg/apis/config/artifact_bucket_test.go @@ -0,0 +1,107 @@ +/* +Copyright 2020 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config_test + +import ( + "os" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/tektoncd/pipeline/pkg/apis/config" + test "github.com/tektoncd/pipeline/pkg/reconciler/testing" + "github.com/tektoncd/pipeline/test/diff" +) + +func TestNewArtifactBucketFromConfigMap(t *testing.T) { + type testCase struct { + expectedConfig *config.ArtifactBucket + fileName string + } + + testCases := []testCase{ + { + expectedConfig: &config.ArtifactBucket{ + Location: "gs://my-bucket", + ServiceAccountFieldName: "GOOGLE_APPLICATION_CREDENTIALS", + }, + fileName: config.GetArtifactBucketConfigName(), + }, + { + expectedConfig: &config.ArtifactBucket{ + Location: "gs://test-bucket", + ServiceAccountSecretName: "test-secret", + ServiceAccountSecretKey: "key", + ServiceAccountFieldName: "some-field", + }, + fileName: "config-artifact-bucket-all-set", + }, + } + + for _, tc := range testCases { + verifyConfigFileWithExpectedArtifactBucketConfig(t, tc.fileName, tc.expectedConfig) + } +} + +func TestNewArtifactBucketFromEmptyConfigMap(t *testing.T) { + ArtifactBucketConfigEmptyName := "config-artifact-bucket-empty" + expectedConfig := &config.ArtifactBucket{ + ServiceAccountFieldName: "GOOGLE_APPLICATION_CREDENTIALS", + } + verifyConfigFileWithExpectedArtifactBucketConfig(t, ArtifactBucketConfigEmptyName, expectedConfig) +} + +func TestGetArtifactBucketConfigName(t *testing.T) { + for _, tc := range []struct { + description string + artifactsBucketEnvValue string + expected string + }{{ + description: "Artifact Bucket config value not set", + artifactsBucketEnvValue: "", + expected: "config-artifact-bucket", + }, { + description: "Artifact Bucket config value set", + artifactsBucketEnvValue: "config-artifact-bucket-test", + expected: "config-artifact-bucket-test", + }} { + t.Run(tc.description, func(t *testing.T) { + original := os.Getenv("CONFIG_ARTIFACT_BUCKET_NAME") + defer t.Cleanup(func() { + os.Setenv("CONFIG_ARTIFACT_BUCKET_NAME", original) + }) + if tc.artifactsBucketEnvValue != "" { + os.Setenv("CONFIG_ARTIFACT_BUCKET_NAME", tc.artifactsBucketEnvValue) + } + got := config.GetArtifactBucketConfigName() + want := tc.expected + if got != want { + t.Errorf("GetArtifactBucketConfigName() = %s, want %s", got, want) + } + }) + } +} + +func verifyConfigFileWithExpectedArtifactBucketConfig(t *testing.T, fileName string, expectedConfig *config.ArtifactBucket) { + cm := test.ConfigMapFromTestFile(t, fileName) + if ab, err := config.NewArtifactBucketFromConfigMap(cm); err == nil { + if d := cmp.Diff(ab, expectedConfig); d != "" { + t.Errorf("Diff:\n%s", diff.PrintWantGot(d)) + } + } else { + t.Errorf("NewArtifactBucketFromConfigMap(actual) = %v", err) + } +} diff --git a/pkg/apis/config/artifact_pvc.go b/pkg/apis/config/artifact_pvc.go new file mode 100644 index 00000000000..630ab1a7f41 --- /dev/null +++ b/pkg/apis/config/artifact_pvc.go @@ -0,0 +1,86 @@ +/* +Copyright 2019 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config + +import ( + "os" + + corev1 "k8s.io/api/core/v1" +) + +const ( + // DefaultPVCSize is the default size of the PVC to create + DefaultPVCSize = "5Gi" + + // PVCSizeKey is the name of the configmap entry that specifies the size of the PVC to create + PVCSizeKey = "size" + + // PVCStorageClassNameKey is the name of the configmap entry that specifies the storage class of the PVC to create + PVCStorageClassNameKey = "storageClassName" +) + +// ArtifactPVC holds the configurations for the artifacts PVC +// +k8s:deepcopy-gen=true +type ArtifactPVC struct { + Size string + StorageClassName string +} + +// GetArtifactPVCConfigName returns the name of the configmap containing all +// customizations for the storage PVC. +func GetArtifactPVCConfigName() string { + if e := os.Getenv("CONFIG_ARTIFACT_PVC_NAME"); e != "" { + return e + } + return "config-artifact-pvc" +} + +// Equals returns true if two Configs are identical +func (cfg *ArtifactPVC) Equals(other *ArtifactPVC) bool { + if cfg == nil && other == nil { + return true + } + + if cfg == nil || other == nil { + return false + } + + return other.Size == cfg.Size && + other.StorageClassName == cfg.StorageClassName +} + +// NewDefaultsFromMap returns a Config given a map corresponding to a ConfigMap +func NewArtifactPVCFromMap(cfgMap map[string]string) (*ArtifactPVC, error) { + tc := ArtifactPVC{ + Size: DefaultPVCSize, + } + + if size, ok := cfgMap[PVCSizeKey]; ok { + tc.Size = size + } + + if storageClassName, ok := cfgMap[PVCStorageClassNameKey]; ok { + tc.StorageClassName = storageClassName + } + + return &tc, nil +} + +// NewDefaultsFromConfigMap returns a Config for the given configmap +func NewArtifactPVCFromConfigMap(config *corev1.ConfigMap) (*ArtifactPVC, error) { + return NewArtifactPVCFromMap(config.Data) +} diff --git a/pkg/apis/config/artifact_pvc_test.go b/pkg/apis/config/artifact_pvc_test.go new file mode 100644 index 00000000000..ec515f7ab4a --- /dev/null +++ b/pkg/apis/config/artifact_pvc_test.go @@ -0,0 +1,105 @@ +/* +Copyright 2020 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config_test + +import ( + "os" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/tektoncd/pipeline/pkg/apis/config" + test "github.com/tektoncd/pipeline/pkg/reconciler/testing" + "github.com/tektoncd/pipeline/test/diff" +) + +func TestNewArtifactPVCFromConfigMap(t *testing.T) { + type testCase struct { + expectedConfig *config.ArtifactPVC + fileName string + } + + testCases := []testCase{ + { + expectedConfig: &config.ArtifactPVC{ + Size: "5Gi", + StorageClassName: "my-class", + }, + fileName: config.GetArtifactPVCConfigName(), + }, + { + expectedConfig: &config.ArtifactPVC{ + Size: "10Gi", + StorageClassName: "test-class", + }, + fileName: "config-artifact-pvc-all-set", + }, + } + + for _, tc := range testCases { + verifyConfigFileWithExpectedArtifactPVCConfig(t, tc.fileName, tc.expectedConfig) + } +} + +func TestNewArtifactPVCFromEmptyConfigMap(t *testing.T) { + ArtifactPVCConfigEmptyName := "config-artifact-pvc-empty" + expectedConfig := &config.ArtifactPVC{ + Size: "5Gi", + } + verifyConfigFileWithExpectedArtifactPVCConfig(t, ArtifactPVCConfigEmptyName, expectedConfig) +} + +func TestGetArtifactPVCConfigName(t *testing.T) { + for _, tc := range []struct { + description string + artifactsPVCEnvValue string + expected string + }{{ + description: "Artifact PVC config value not set", + artifactsPVCEnvValue: "", + expected: "config-artifact-pvc", + }, { + description: "Artifact PVC config value set", + artifactsPVCEnvValue: "config-artifact-pvc-test", + expected: "config-artifact-pvc-test", + }} { + t.Run(tc.description, func(t *testing.T) { + original := os.Getenv("CONFIG_ARTIFACT_PVC_NAME") + defer t.Cleanup(func() { + os.Setenv("CONFIG_ARTIFACT_PVC_NAME", original) + }) + if tc.artifactsPVCEnvValue != "" { + os.Setenv("CONFIG_ARTIFACT_PVC_NAME", tc.artifactsPVCEnvValue) + } + got := config.GetArtifactPVCConfigName() + want := tc.expected + if got != want { + t.Errorf("GetArtifactPVCConfigName() = %s, want %s", got, want) + } + }) + } +} + +func verifyConfigFileWithExpectedArtifactPVCConfig(t *testing.T, fileName string, expectedConfig *config.ArtifactPVC) { + cm := test.ConfigMapFromTestFile(t, fileName) + if ab, err := config.NewArtifactPVCFromConfigMap(cm); err == nil { + if d := cmp.Diff(ab, expectedConfig); d != "" { + t.Errorf("Diff:\n%s", diff.PrintWantGot(d)) + } + } else { + t.Errorf("NewArtifactPVCFromConfigMap(actual) = %v", err) + } +} diff --git a/pkg/apis/config/default.go b/pkg/apis/config/default.go index 04b30a10a82..309ca0fb561 100644 --- a/pkg/apis/config/default.go +++ b/pkg/apis/config/default.go @@ -49,7 +49,7 @@ type Defaults struct { DefaultCloudEventsSink string } -// GetBucketConfigName returns the name of the configmap containing all +// GetDefaultsConfigName returns the name of the configmap containing all // customizations for the storage bucket. func GetDefaultsConfigName() string { if e := os.Getenv("CONFIG_DEFAULTS_NAME"); e != "" { diff --git a/pkg/apis/config/store.go b/pkg/apis/config/store.go index 9b5c87331d5..e84a16bcdf4 100644 --- a/pkg/apis/config/store.go +++ b/pkg/apis/config/store.go @@ -27,8 +27,10 @@ type cfgKey struct{} // Config holds the collection of configurations that we attach to contexts. // +k8s:deepcopy-gen=false type Config struct { - Defaults *Defaults - FeatureFlags *FeatureFlags + Defaults *Defaults + FeatureFlags *FeatureFlags + ArtifactBucket *ArtifactBucket + ArtifactPVC *ArtifactPVC } // FromContext extracts a Config from the provided context. @@ -48,9 +50,13 @@ func FromContextOrDefaults(ctx context.Context) *Config { } defaults, _ := NewDefaultsFromMap(map[string]string{}) featureFlags, _ := NewFeatureFlagsFromMap(map[string]string{}) + artifactBucket, _ := NewArtifactBucketFromMap(map[string]string{}) + artifactPVC, _ := NewArtifactPVCFromMap(map[string]string{}) return &Config{ - Defaults: defaults, - FeatureFlags: featureFlags, + Defaults: defaults, + FeatureFlags: featureFlags, + ArtifactBucket: artifactBucket, + ArtifactPVC: artifactPVC, } } @@ -70,11 +76,13 @@ type Store struct { func NewStore(logger configmap.Logger, onAfterStore ...func(name string, value interface{})) *Store { store := &Store{ UntypedStore: configmap.NewUntypedStore( - "defaults/features", + "defaults/features/artifacts", logger, configmap.Constructors{ - GetDefaultsConfigName(): NewDefaultsFromConfigMap, - GetFeatureFlagsConfigName(): NewFeatureFlagsFromConfigMap, + GetDefaultsConfigName(): NewDefaultsFromConfigMap, + GetFeatureFlagsConfigName(): NewFeatureFlagsFromConfigMap, + GetArtifactBucketConfigName(): NewArtifactBucketFromConfigMap, + GetArtifactPVCConfigName(): NewArtifactPVCFromConfigMap, }, onAfterStore..., ), @@ -98,9 +106,19 @@ func (s *Store) Load() *Config { if featureFlags == nil { featureFlags, _ = NewFeatureFlagsFromMap(map[string]string{}) } + artifactBucket := s.UntypedLoad(GetArtifactBucketConfigName()) + if artifactBucket == nil { + artifactBucket, _ = NewArtifactBucketFromMap(map[string]string{}) + } + artifactPVC := s.UntypedLoad(GetArtifactPVCConfigName()) + if artifactPVC == nil { + artifactPVC, _ = NewArtifactPVCFromMap(map[string]string{}) + } return &Config{ - Defaults: defaults.(*Defaults).DeepCopy(), - FeatureFlags: featureFlags.(*FeatureFlags).DeepCopy(), + Defaults: defaults.(*Defaults).DeepCopy(), + FeatureFlags: featureFlags.(*FeatureFlags).DeepCopy(), + ArtifactBucket: artifactBucket.(*ArtifactBucket).DeepCopy(), + ArtifactPVC: artifactPVC.(*ArtifactPVC).DeepCopy(), } } diff --git a/pkg/apis/config/store_test.go b/pkg/apis/config/store_test.go index 4f1f3629be2..866e944e766 100644 --- a/pkg/apis/config/store_test.go +++ b/pkg/apis/config/store_test.go @@ -30,18 +30,26 @@ import ( func TestStoreLoadWithContext(t *testing.T) { defaultConfig := test.ConfigMapFromTestFile(t, "config-defaults") featuresConfig := test.ConfigMapFromTestFile(t, "feature-flags-all-flags-set") + artifactBucketConfig := test.ConfigMapFromTestFile(t, "config-artifact-bucket") + artifactPVCConfig := test.ConfigMapFromTestFile(t, "config-artifact-pvc") expectedDefaults, _ := config.NewDefaultsFromConfigMap(defaultConfig) expectedFeatures, _ := config.NewFeatureFlagsFromConfigMap(featuresConfig) + expectedArtifactBucket, _ := config.NewArtifactBucketFromConfigMap(artifactBucketConfig) + expectedArtifactPVC, _ := config.NewArtifactPVCFromConfigMap(artifactPVCConfig) expected := &config.Config{ - Defaults: expectedDefaults, - FeatureFlags: expectedFeatures, + Defaults: expectedDefaults, + FeatureFlags: expectedFeatures, + ArtifactBucket: expectedArtifactBucket, + ArtifactPVC: expectedArtifactPVC, } store := config.NewStore(logtesting.TestLogger(t)) store.OnConfigChanged(defaultConfig) store.OnConfigChanged(featuresConfig) + store.OnConfigChanged(artifactBucketConfig) + store.OnConfigChanged(artifactPVCConfig) cfg := config.FromContext(store.ToContext(context.Background())) diff --git a/pkg/apis/config/testdata/config-artifact-bucket-all-set.yaml b/pkg/apis/config/testdata/config-artifact-bucket-all-set.yaml new file mode 100644 index 00000000000..236715851ff --- /dev/null +++ b/pkg/apis/config/testdata/config-artifact-bucket-all-set.yaml @@ -0,0 +1,24 @@ +# Copyright 2020 The Tekton Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: v1 +kind: ConfigMap +metadata: + name: config-artifact-bucket + namespace: tekton-pipelines +data: + location: "gs://test-bucket" + bucket.service.account.secret.name: "test-secret" + bucket.service.account.secret.key: "key" + bucket.service.account.field.name: "some-field" diff --git a/pkg/apis/config/testdata/config-artifact-bucket-empty.yaml b/pkg/apis/config/testdata/config-artifact-bucket-empty.yaml new file mode 100644 index 00000000000..d0bfa806daa --- /dev/null +++ b/pkg/apis/config/testdata/config-artifact-bucket-empty.yaml @@ -0,0 +1,26 @@ +# Copyright 2020 The Tekton Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: v1 +kind: ConfigMap +metadata: + name: config-artifact-bucket + namespace: tekton-pipelines +data: + _example: | + ################################ + # # + # EXAMPLE CONFIGURATION # + # # + ################################ diff --git a/pkg/apis/config/testdata/config-artifact-bucket.yaml b/pkg/apis/config/testdata/config-artifact-bucket.yaml new file mode 100644 index 00000000000..a4a7f062f5e --- /dev/null +++ b/pkg/apis/config/testdata/config-artifact-bucket.yaml @@ -0,0 +1,21 @@ +# Copyright 2020 The Tekton Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: v1 +kind: ConfigMap +metadata: + name: config-artifact-bucket + namespace: tekton-pipelines +data: + location: "gs://my-bucket" \ No newline at end of file diff --git a/pkg/apis/config/testdata/config-artifact-pvc-all-set.yaml b/pkg/apis/config/testdata/config-artifact-pvc-all-set.yaml new file mode 100644 index 00000000000..80785ff8f3e --- /dev/null +++ b/pkg/apis/config/testdata/config-artifact-pvc-all-set.yaml @@ -0,0 +1,22 @@ +# Copyright 2020 The Tekton Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: v1 +kind: ConfigMap +metadata: + name: config-artifact-pvc + namespace: tekton-pipelines +data: + size: "10Gi" + storageClassName: "test-class" diff --git a/pkg/apis/config/testdata/config-artifact-pvc-empty.yaml b/pkg/apis/config/testdata/config-artifact-pvc-empty.yaml new file mode 100644 index 00000000000..0b5556004ea --- /dev/null +++ b/pkg/apis/config/testdata/config-artifact-pvc-empty.yaml @@ -0,0 +1,26 @@ +# Copyright 2020 The Tekton Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: v1 +kind: ConfigMap +metadata: + name: config-artifact-pvc + namespace: tekton-pipelines +data: + _example: | + ################################ + # # + # EXAMPLE CONFIGURATION # + # # + ################################ diff --git a/pkg/apis/config/testdata/config-artifact-pvc.yaml b/pkg/apis/config/testdata/config-artifact-pvc.yaml new file mode 100644 index 00000000000..26cecb75a2c --- /dev/null +++ b/pkg/apis/config/testdata/config-artifact-pvc.yaml @@ -0,0 +1,21 @@ +# Copyright 2020 The Tekton Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: v1 +kind: ConfigMap +metadata: + name: config-artifact-pvc + namespace: tekton-pipelines +data: + storageClassName: "my-class" \ No newline at end of file diff --git a/pkg/apis/config/zz_generated.deepcopy.go b/pkg/apis/config/zz_generated.deepcopy.go index f17d31c89df..33596b2e155 100644 --- a/pkg/apis/config/zz_generated.deepcopy.go +++ b/pkg/apis/config/zz_generated.deepcopy.go @@ -24,6 +24,38 @@ import ( pod "github.com/tektoncd/pipeline/pkg/apis/pipeline/pod" ) +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ArtifactBucket) DeepCopyInto(out *ArtifactBucket) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ArtifactBucket. +func (in *ArtifactBucket) DeepCopy() *ArtifactBucket { + if in == nil { + return nil + } + out := new(ArtifactBucket) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ArtifactPVC) DeepCopyInto(out *ArtifactPVC) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ArtifactPVC. +func (in *ArtifactPVC) DeepCopy() *ArtifactPVC { + if in == nil { + return nil + } + out := new(ArtifactPVC) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Defaults) DeepCopyInto(out *Defaults) { *out = *in diff --git a/pkg/artifacts/artifact_storage_test.go b/pkg/artifacts/artifact_storage_test.go index 99dd9099af4..87a76ff6d71 100644 --- a/pkg/artifacts/artifact_storage_test.go +++ b/pkg/artifacts/artifact_storage_test.go @@ -17,22 +17,22 @@ limitations under the License. package artifacts import ( + "context" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" resourcev1alpha1 "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1" "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1/storage" - "github.com/tektoncd/pipeline/pkg/system" "github.com/tektoncd/pipeline/test/diff" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" fakek8s "k8s.io/client-go/kubernetes/fake" - logtesting "knative.dev/pkg/logging/testing" ) var ( @@ -57,7 +57,7 @@ var ( } defaultStorageClass *string customStorageClass = "custom-storage-class" - persistentVolumeClaim = GetPersistentVolumeClaim(DefaultPVCSize, defaultStorageClass) + persistentVolumeClaim = GetPersistentVolumeClaim(config.DefaultPVCSize, defaultStorageClass) quantityComparer = cmp.Comparer(func(x, y resource.Quantity) bool { return x.Cmp(y) == 0 }) @@ -114,80 +114,52 @@ func GetPersistentVolumeClaim(size string, storageClassName *string) *corev1.Per return pvc } -func TestConfigMapNeedsPVC(t *testing.T) { - logger := logtesting.TestLogger(t) +func TestNeedsPVC(t *testing.T) { for _, c := range []struct { - desc string - configMap *corev1.ConfigMap - pvcNeeded bool + desc string + bucketConfig map[string]string + pvcNeeded bool }{{ desc: "valid bucket", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketLocationKey: "gs://fake-bucket", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", - }, + bucketConfig: map[string]string{ + config.BucketLocationKey: "gs://fake-bucket", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, pvcNeeded: false, }, { desc: "location empty", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketLocationKey: "", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", - }, + bucketConfig: map[string]string{ + config.BucketLocationKey: "", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, pvcNeeded: true, }, { desc: "missing location", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", - }, + bucketConfig: map[string]string{ + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, pvcNeeded: true, }, { - desc: "no config map data", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - }, - pvcNeeded: true, + desc: "no config map data", + bucketConfig: map[string]string{}, + pvcNeeded: true, }, { desc: "no secret", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketLocationKey: "gs://fake-bucket", - }, + bucketConfig: map[string]string{ + config.BucketLocationKey: "gs://fake-bucket", }, pvcNeeded: false, }} { t.Run(c.desc, func(t *testing.T) { - needed, err := ConfigMapNeedsPVC(c.configMap, nil, logger) - if err != nil { - t.Fatalf("Somehow had error checking if PVC was needed run: %s", err) + artifactBucket, _ := config.NewArtifactBucketFromMap(c.bucketConfig) + configs := config.Config{ + ArtifactBucket: artifactBucket, } + ctx := config.ToContext(context.Background(), &configs) + needed := NeedsPVC(ctx) if needed != c.pvcNeeded { t.Fatalf("Expected that ConfigMapNeedsPVC would be %t, but was %t", c.pvcNeeded, needed) } @@ -196,60 +168,42 @@ func TestConfigMapNeedsPVC(t *testing.T) { } -func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { - logger := logtesting.TestLogger(t) +func TestInitializeArtifactStorage(t *testing.T) { for _, c := range []struct { desc string - configMap *corev1.ConfigMap - expectedArtifactStorage ArtifactStorageInterface + storageConfig map[string]string storagetype string + expectedArtifactStorage ArtifactStorageInterface }{{ desc: "pvc configmap size", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetPVCConfigName(), - }, - Data: map[string]string{ - PVCSizeKey: "10Gi", - }, + storageConfig: map[string]string{ + config.PVCSizeKey: "10Gi", }, + storagetype: "pvc", expectedArtifactStorage: &storage.ArtifactPVC{ Name: "pipelineruntest", PersistentVolumeClaim: GetPersistentVolumeClaim("10Gi", defaultStorageClass), ShellImage: "busybox", }, - storagetype: "pvc", }, { desc: "pvc configmap storageclass", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetPVCConfigName(), - }, - Data: map[string]string{ - PVCStorageClassNameKey: customStorageClass, - }, + storageConfig: map[string]string{ + config.PVCStorageClassNameKey: customStorageClass, }, + storagetype: "pvc", expectedArtifactStorage: &storage.ArtifactPVC{ Name: "pipelineruntest", PersistentVolumeClaim: GetPersistentVolumeClaim("5Gi", &customStorageClass), ShellImage: "busybox", }, - storagetype: "pvc", }, { desc: "valid bucket", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketLocationKey: "gs://fake-bucket", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", - }, + storageConfig: map[string]string{ + config.BucketLocationKey: "gs://fake-bucket", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, + storagetype: "bucket", expectedArtifactStorage: &storage.ArtifactBucket{ Location: "gs://fake-bucket", Secrets: []resourcev1alpha1.SecretParam{{ @@ -260,89 +214,60 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { ShellImage: "busybox", GsutilImage: "google/cloud-sdk", }, - storagetype: "bucket", }, { desc: "location empty", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketLocationKey: "", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", - }, + storageConfig: map[string]string{ + config.BucketLocationKey: "", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, + storagetype: "pvc", expectedArtifactStorage: &storage.ArtifactPVC{ Name: "pipelineruntest", PersistentVolumeClaim: persistentVolumeClaim, ShellImage: "busybox", }, - storagetype: "pvc", }, { desc: "missing location", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", - }, + storageConfig: map[string]string{ + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, + storagetype: "pvc", expectedArtifactStorage: &storage.ArtifactPVC{ Name: "pipelineruntest", PersistentVolumeClaim: persistentVolumeClaim, ShellImage: "busybox", }, - storagetype: "pvc", }, { - desc: "no config map data", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - }, + desc: "no config map data", + storageConfig: map[string]string{}, + storagetype: "pvc", expectedArtifactStorage: &storage.ArtifactPVC{ Name: "pipelineruntest", PersistentVolumeClaim: persistentVolumeClaim, ShellImage: "busybox", }, - storagetype: "pvc", }, { desc: "no secret", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketLocationKey: "gs://fake-bucket", - }, + storageConfig: map[string]string{ + config.BucketLocationKey: "gs://fake-bucket", }, + storagetype: "bucket", expectedArtifactStorage: &storage.ArtifactBucket{ Location: "gs://fake-bucket", ShellImage: "busybox", GsutilImage: "google/cloud-sdk", }, - storagetype: "bucket", }, { desc: "valid bucket with boto config", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketLocationKey: "s3://fake-bucket", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", - BucketServiceAccountFieldName: "BOTO_CONFIG", - }, + storageConfig: map[string]string{ + config.BucketLocationKey: "s3://fake-bucket", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", + config.BucketServiceAccountFieldNameKey: "BOTO_CONFIG", }, + storagetype: "bucket", expectedArtifactStorage: &storage.ArtifactBucket{ Location: "s3://fake-bucket", ShellImage: "busybox", @@ -353,11 +278,17 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { SecretName: "secret1", }}, }, - storagetype: "bucket", }} { t.Run(c.desc, func(t *testing.T) { - fakekubeclient := fakek8s.NewSimpleClientset(c.configMap) - artifactStorage, err := InitializeArtifactStorage(images, pipelinerun, &pipelineWithTasksWithFrom.Spec, fakekubeclient, logger) + fakekubeclient := fakek8s.NewSimpleClientset() + configs := config.Config{} + if c.storagetype == "bucket" { + configs.ArtifactBucket, _ = config.NewArtifactBucketFromMap(c.storageConfig) + } else { + configs.ArtifactPVC, _ = config.NewArtifactPVCFromMap(c.storageConfig) + } + ctx := config.ToContext(context.Background(), &configs) + artifactStorage, err := InitializeArtifactStorage(ctx, images, pipelinerun, &pipelineWithTasksWithFrom.Spec, fakekubeclient) if err != nil { t.Fatalf("Somehow had error initializing artifact storage run out of fake client: %s", err) } @@ -373,7 +304,7 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { } // Make sure we don't get any errors running CleanupArtifactStorage against the resulting storage, whether it's // a bucket or a PVC. - if err := CleanupArtifactStorage(pipelinerun, fakekubeclient, logger); err != nil { + if err := CleanupArtifactStorage(ctx, pipelinerun, fakekubeclient); err != nil { t.Fatalf("Error cleaning up artifact storage: %s", err) } if d := cmp.Diff(artifactStorage.GetType(), c.storagetype); d != "" { @@ -387,7 +318,6 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { } func TestInitializeArtifactStorageNoStorageNeeded(t *testing.T) { - logger := logtesting.TestLogger(t) // This Pipeline has Tasks that use both inputs and outputs, but there is // no link between the inputs and outputs, so no storage is needed pipeline := &v1beta1.Pipeline{ @@ -444,49 +374,42 @@ func TestInitializeArtifactStorageNoStorageNeeded(t *testing.T) { }, } for _, c := range []struct { - desc string - configMap *corev1.ConfigMap + desc string + storageConfig map[string]string + storagetype string }{{ desc: "has pvc configured", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetPVCConfigName(), - }, - Data: map[string]string{ - PVCSizeKey: "10Gi", - }, + storageConfig: map[string]string{ + config.PVCSizeKey: "10Gi", }, + storagetype: "pvc", }, { desc: "has bucket configured", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketLocationKey: "gs://fake-bucket", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", - }, + storageConfig: map[string]string{ + config.BucketLocationKey: "gs://fake-bucket", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, + storagetype: "bucket", }, { desc: "no configmap", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketLocationKey: "", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", - }, + storageConfig: map[string]string{ + config.BucketLocationKey: "", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, + storagetype: "bucket", }} { t.Run(c.desc, func(t *testing.T) { - fakekubeclient := fakek8s.NewSimpleClientset(c.configMap) - artifactStorage, err := InitializeArtifactStorage(images, pipelinerun, &pipeline.Spec, fakekubeclient, logger) + fakekubeclient := fakek8s.NewSimpleClientset() + configs := config.Config{} + if c.storagetype == "bucket" { + configs.ArtifactBucket, _ = config.NewArtifactBucketFromMap(c.storageConfig) + } else { + configs.ArtifactPVC, _ = config.NewArtifactPVCFromMap(c.storageConfig) + } + ctx := config.ToContext(context.Background(), &configs) + artifactStorage, err := InitializeArtifactStorage(ctx, images, pipelinerun, &pipeline.Spec, fakekubeclient) if err != nil { t.Fatalf("Somehow had error initializing artifact storage run out of fake client: %s", err) } @@ -504,51 +427,41 @@ func TestCleanupArtifactStorage(t *testing.T) { Name: "pipelineruntest", }, } - logger := logtesting.TestLogger(t) for _, c := range []struct { - desc string - configMap *corev1.ConfigMap + desc string + storageConfig map[string]string }{{ desc: "location empty", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketLocationKey: "", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", - }, + storageConfig: map[string]string{ + config.BucketLocationKey: "", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, }, { desc: "missing location", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", - }, + storageConfig: map[string]string{ + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, }, { - desc: "no config map data", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - }, + desc: "no config map data", + storageConfig: map[string]string{}, }} { t.Run(c.desc, func(t *testing.T) { - fakekubeclient := fakek8s.NewSimpleClientset(c.configMap, GetPVCSpec(pipelinerun, persistentVolumeClaim.Spec.Resources.Requests["storage"], defaultStorageClass)) - _, err := fakekubeclient.CoreV1().PersistentVolumeClaims(pipelinerun.Namespace).Get(GetPVCName(pipelinerun), metav1.GetOptions{}) + fakekubeclient := fakek8s.NewSimpleClientset(GetPVCSpec(pipelinerun, persistentVolumeClaim.Spec.Resources.Requests["storage"], defaultStorageClass)) + ab, err := config.NewArtifactBucketFromMap(c.storageConfig) + if err != nil { + t.Fatalf("Error getting an ArtifactBucket from data %s, %s", c.storageConfig, err) + } + configs := config.Config{ + ArtifactBucket: ab, + } + ctx := config.ToContext(context.Background(), &configs) + _, err = fakekubeclient.CoreV1().PersistentVolumeClaims(pipelinerun.Namespace).Get(GetPVCName(pipelinerun), metav1.GetOptions{}) if err != nil { t.Fatalf("Error getting expected PVC %s for PipelineRun %s: %s", GetPVCName(pipelinerun), pipelinerun.Name, err) } - if err := CleanupArtifactStorage(pipelinerun, fakekubeclient, logger); err != nil { + if err := CleanupArtifactStorage(ctx, pipelinerun, fakekubeclient); err != nil { t.Fatalf("Error cleaning up artifact storage: %s", err) } _, err = fakekubeclient.CoreV1().PersistentVolumeClaims(pipelinerun.Namespace).Get(GetPVCName(pipelinerun), metav1.GetOptions{}) @@ -568,10 +481,9 @@ func TestInitializeArtifactStorageWithoutConfigMap(t *testing.T) { Namespace: "foo", }, } - logger := logtesting.TestLogger(t) fakekubeclient := fakek8s.NewSimpleClientset() - pvc, err := InitializeArtifactStorage(images, pipelinerun, &pipelineWithTasksWithFrom.Spec, fakekubeclient, logger) + pvc, err := InitializeArtifactStorage(context.Background(), images, pipelinerun, &pipelineWithTasksWithFrom.Spec, fakekubeclient) if err != nil { t.Fatalf("Somehow had error initializing artifact storage run out of fake client: %s", err) } @@ -587,30 +499,23 @@ func TestInitializeArtifactStorageWithoutConfigMap(t *testing.T) { } } -func TestGetArtifactStorageWithConfigMap(t *testing.T) { +func TestGetArtifactStorageWithConfig(t *testing.T) { pipelinerun := &v1beta1.PipelineRun{ ObjectMeta: metav1.ObjectMeta{ Namespace: "foo", Name: "pipelineruntest", }, } - logger := logtesting.TestLogger(t) for _, c := range []struct { desc string - configMap *corev1.ConfigMap + storageConfig map[string]string expectedArtifactStorage ArtifactStorageInterface }{{ desc: "valid bucket", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketLocationKey: "gs://fake-bucket", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", - }, + storageConfig: map[string]string{ + config.BucketLocationKey: "gs://fake-bucket", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, expectedArtifactStorage: &storage.ArtifactBucket{ Location: "gs://fake-bucket", @@ -624,16 +529,10 @@ func TestGetArtifactStorageWithConfigMap(t *testing.T) { }, }, { desc: "location empty", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketLocationKey: "", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", - }, + storageConfig: map[string]string{ + config.BucketLocationKey: "", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, expectedArtifactStorage: &storage.ArtifactPVC{ Name: pipelinerun.Name, @@ -641,40 +540,33 @@ func TestGetArtifactStorageWithConfigMap(t *testing.T) { }, }, { desc: "missing location", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - Data: map[string]string{ - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", - }, + storageConfig: map[string]string{ + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, expectedArtifactStorage: &storage.ArtifactPVC{ Name: pipelinerun.Name, ShellImage: "busybox", }, }, { - desc: "no config map data", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), - }, - }, + desc: "no config map data", + storageConfig: map[string]string{}, expectedArtifactStorage: &storage.ArtifactPVC{ Name: pipelinerun.Name, ShellImage: "busybox", }, }} { t.Run(c.desc, func(t *testing.T) { - fakekubeclient := fakek8s.NewSimpleClientset(c.configMap) - - artifactStorage, err := GetArtifactStorage(images, pipelinerun.Name, fakekubeclient, logger) + fakekubeclient := fakek8s.NewSimpleClientset() + ab, err := config.NewArtifactBucketFromMap(c.storageConfig) if err != nil { - t.Fatalf("Somehow had error initializing artifact storage run out of fake client: %s", err) + t.Fatalf("Error getting an ArtifactBucket from data %s, %s", c.storageConfig, err) + } + configs := config.Config{ + ArtifactBucket: ab, } + ctx := config.ToContext(context.Background(), &configs) + artifactStorage := GetArtifactStorage(ctx, images, pipelinerun.Name, fakekubeclient) if d := cmp.Diff(artifactStorage, c.expectedArtifactStorage); d != "" { t.Fatalf(diff.PrintWantGot(d)) @@ -683,13 +575,9 @@ func TestGetArtifactStorageWithConfigMap(t *testing.T) { } } -func TestGetArtifactStorageWithoutConfigMap(t *testing.T) { - logger := logtesting.TestLogger(t) +func TestGetArtifactStorageWithoutConfig(t *testing.T) { fakekubeclient := fakek8s.NewSimpleClientset() - pvc, err := GetArtifactStorage(images, "pipelineruntest", fakekubeclient, logger) - if err != nil { - t.Fatalf("Somehow had error initializing artifact storage run out of fake client: %s", err) - } + pvc := GetArtifactStorage(context.Background(), images, "pipelineruntest", fakekubeclient) expectedArtifactPVC := &storage.ArtifactPVC{ Name: "pipelineruntest", @@ -701,23 +589,16 @@ func TestGetArtifactStorageWithoutConfigMap(t *testing.T) { } } -func TestGetArtifactStorageWithPVCConfigMap(t *testing.T) { - logger := logtesting.TestLogger(t) +func TestGetArtifactStorageWithPVCConfig(t *testing.T) { prName := "pipelineruntest" for _, c := range []struct { desc string - configMap *corev1.ConfigMap + storageConfig map[string]string expectedArtifactStorage ArtifactStorageInterface }{{ desc: "valid pvc", - configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.GetNamespace(), - Name: GetPVCConfigName(), - }, - Data: map[string]string{ - PVCSizeKey: "10Gi", - }, + storageConfig: map[string]string{ + config.PVCSizeKey: "10Gi", }, expectedArtifactStorage: &storage.ArtifactPVC{ Name: "pipelineruntest", @@ -725,12 +606,16 @@ func TestGetArtifactStorageWithPVCConfigMap(t *testing.T) { }, }} { t.Run(c.desc, func(t *testing.T) { - fakekubeclient := fakek8s.NewSimpleClientset(c.configMap) - - artifactStorage, err := GetArtifactStorage(images, prName, fakekubeclient, logger) + fakekubeclient := fakek8s.NewSimpleClientset() + ap, err := config.NewArtifactPVCFromMap(c.storageConfig) if err != nil { - t.Fatalf("Somehow had error initializing artifact storage run out of fake client: %s", err) + t.Fatalf("Error getting an ArtifactPVC from data %s, %s", c.storageConfig, err) + } + configs := config.Config{ + ArtifactPVC: ap, } + ctx := config.ToContext(context.Background(), &configs) + artifactStorage := GetArtifactStorage(ctx, images, prName, fakekubeclient) if d := cmp.Diff(artifactStorage, c.expectedArtifactStorage); d != "" { t.Fatalf(diff.PrintWantGot(d)) diff --git a/pkg/artifacts/artifacts_storage.go b/pkg/artifacts/artifacts_storage.go index 67f0d7305cf..1b1223ad80a 100644 --- a/pkg/artifacts/artifacts_storage.go +++ b/pkg/artifacts/artifacts_storage.go @@ -17,73 +17,24 @@ limitations under the License. package artifacts import ( + "context" "fmt" - "os" "strings" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" resourcev1alpha1 "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1" "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1/storage" - "github.com/tektoncd/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" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/kubernetes" + "knative.dev/pkg/logging" ) -const ( - // PVCSizeKey is the name of the configmap entry that specifies the size of the PVC to create - PVCSizeKey = "size" - - // DefaultPVCSize is the default size of the PVC to create - DefaultPVCSize = "5Gi" - - // PVCStorageClassNameKey is the name of the configmap entry that specifies the storage class of the PVC to create - PVCStorageClassNameKey = "storageClassName" - - // BucketLocationKey is the name of the configmap entry that specifies - // loction of the bucket. - BucketLocationKey = "location" - - // BucketServiceAccountSecretName is the name of the configmap entry that specifies - // the name of the secret that will provide the servie account with bucket access. - // This secret must have a key called serviceaccount that will have a value with - // the service account with access to the bucket - BucketServiceAccountSecretName = "bucket.service.account.secret.name" - - // BucketServiceAccountSecretKey is the name of the configmap entry that specifies - // the secret key that will have a value with the service account json with access - // to the bucket - BucketServiceAccountSecretKey = "bucket.service.account.secret.key" - - // BucketServiceAccountFieldName is the name of the configmap entry that specifies - // the field name that should be used for the service account. - // Valid values: GOOGLE_APPLICATION_CREDENTIALS, BOTO_CONFIG. Defaults to GOOGLE_APPLICATION_CREDENTIALS. - BucketServiceAccountFieldName = "bucket.service.account.field.name" -) - -// GetBucketConfigName returns the name of the configmap containing all -// customizations for the storage bucket. -func GetBucketConfigName() string { - if e := os.Getenv("CONFIG_ARTIFACT_BUCKET_NAME"); e != "" { - return e - } - return "config-artifact-bucket" -} - -// GetPVCConfigName returns the name of the configmap containing all -// customizations for the storage PVC. -func GetPVCConfigName() string { - if e := os.Getenv("CONFIG_ARTIFACT_PVC_NAME"); e != "" { - return e - } - return "config-artifact-pvc" -} - // ArtifactStorageInterface is an interface to define the steps to copy // an pipeline artifact to/from temporary storage type ArtifactStorageInterface interface { @@ -125,7 +76,7 @@ func (a *ArtifactStorageNone) StorageBasePath(pr *v1beta1.PipelineRun) string { // InitializeArtifactStorage will check if there is there is a // bucket configured, create a PVC or return nil if no storage is required. -func InitializeArtifactStorage(images pipeline.Images, pr *v1beta1.PipelineRun, ps *v1beta1.PipelineSpec, c kubernetes.Interface, logger *zap.SugaredLogger) (ArtifactStorageInterface, error) { +func InitializeArtifactStorage(ctx context.Context, images pipeline.Images, pr *v1beta1.PipelineRun, ps *v1beta1.PipelineSpec, c kubernetes.Interface) (ArtifactStorageInterface, error) { // Artifact storage is needed under the following condition: // Any Task in the pipeline contains an Output resource // AND that Output resource is one of the AllowedOutputResource types. @@ -153,32 +104,23 @@ func InitializeArtifactStorage(images pipeline.Images, pr *v1beta1.PipelineRun, return &ArtifactStorageNone{}, nil } - configMap, err := c.CoreV1().ConfigMaps(system.GetNamespace()).Get(GetBucketConfigName(), metav1.GetOptions{}) - shouldCreatePVC, err := ConfigMapNeedsPVC(configMap, err, logger) - if err != nil { - return nil, err - } - if shouldCreatePVC { - pvc, err := createPVC(pr, c) + if NeedsPVC(ctx) { + pvc, err := createPVC(ctx, pr, c) if err != nil { return nil, err } return &storage.ArtifactPVC{Name: pr.Name, PersistentVolumeClaim: pvc, ShellImage: images.ShellImage}, nil } - return NewArtifactBucketConfigFromConfigMap(images)(configMap) + return NewArtifactBucketFromConfig(ctx, images), nil } // CleanupArtifactStorage will delete the PipelineRun's artifact storage PVC if it exists. The PVC is created for using // an output workspace or artifacts from one Task to another Task. No other PVCs will be impacted by this cleanup. -func CleanupArtifactStorage(pr *v1beta1.PipelineRun, c kubernetes.Interface, logger *zap.SugaredLogger) error { - configMap, err := c.CoreV1().ConfigMaps(system.GetNamespace()).Get(GetBucketConfigName(), metav1.GetOptions{}) - shouldCreatePVC, err := ConfigMapNeedsPVC(configMap, err, logger) - if err != nil { - return err - } - if shouldCreatePVC { - err = deletePVC(pr, c) +func CleanupArtifactStorage(ctx context.Context, pr *v1beta1.PipelineRun, c kubernetes.Interface) error { + + if NeedsPVC(ctx) { + err := deletePVC(pr, c) if err != nil { return err } @@ -186,105 +128,61 @@ func CleanupArtifactStorage(pr *v1beta1.PipelineRun, c kubernetes.Interface, log return nil } -// ConfigMapNeedsPVC checks if the possibly-nil config map passed to it is configured to use a bucket for artifact storage, +// NeedsPVC checks if the Tekton is is configured to use a bucket for artifact storage, // returning true if instead a PVC is needed. -func ConfigMapNeedsPVC(configMap *corev1.ConfigMap, err error, logger *zap.SugaredLogger) (bool, error) { - if err != nil { - if errors.IsNotFound(err) { - return true, nil - } - return false, fmt.Errorf("couldn't determine if PVC was needed from config map: %w", err) - } - if configMap == nil { - return true, nil - } - if configMap.Data == nil { - logger.Warn("the configmap has no data") - return true, nil - } - location, ok := configMap.Data[BucketLocationKey] - if !ok { - return true, nil +func NeedsPVC(ctx context.Context) bool { + bucketConfig := config.FromContextOrDefaults(ctx).ArtifactBucket + if bucketConfig == nil { + return true } - if strings.TrimSpace(location) == "" { - logger.Warnf("the configmap key %q is empty", BucketLocationKey) - return true, nil + if strings.TrimSpace(bucketConfig.Location) == "" { + logging.FromContext(ctx).Warnf("the configmap key %q is empty", config.BucketLocationKey) + return true } - return false, nil + return false } // GetArtifactStorage returns the storage interface to enable // consumer code to get a container step for copy to/from storage -func GetArtifactStorage(images pipeline.Images, prName string, c kubernetes.Interface, logger *zap.SugaredLogger) (ArtifactStorageInterface, error) { - configMap, err := c.CoreV1().ConfigMaps(system.GetNamespace()).Get(GetBucketConfigName(), metav1.GetOptions{}) - pvc, err := ConfigMapNeedsPVC(configMap, err, logger) - if err != nil { - return nil, fmt.Errorf("couldn't determine if PVC was needed from config map: %w", err) +func GetArtifactStorage(ctx context.Context, images pipeline.Images, prName string, c kubernetes.Interface) ArtifactStorageInterface { + if NeedsPVC(ctx) { + return &storage.ArtifactPVC{Name: prName, ShellImage: images.ShellImage} } - if pvc { - return &storage.ArtifactPVC{Name: prName, ShellImage: images.ShellImage}, nil - } - return NewArtifactBucketConfigFromConfigMap(images)(configMap) + return NewArtifactBucketFromConfig(ctx, images) } -// NewArtifactBucketConfigFromConfigMap creates a Bucket from the supplied ConfigMap -func NewArtifactBucketConfigFromConfigMap(images pipeline.Images) func(configMap *corev1.ConfigMap) (*storage.ArtifactBucket, error) { - return func(configMap *corev1.ConfigMap) (*storage.ArtifactBucket, error) { - c := &storage.ArtifactBucket{ - ShellImage: images.ShellImage, - GsutilImage: images.GsutilImage, - } +// NewArtifactBucketFromConfig creates a Bucket from the supplied ConfigMap +func NewArtifactBucketFromConfig(ctx context.Context, images pipeline.Images) *storage.ArtifactBucket { + c := &storage.ArtifactBucket{ + ShellImage: images.ShellImage, + GsutilImage: images.GsutilImage, + } - if configMap.Data == nil { - return c, nil - } - if location, ok := configMap.Data[BucketLocationKey]; !ok { - c.Location = "" - } else { - c.Location = location - } - sp := resourcev1alpha1.SecretParam{} - if secretName, ok := configMap.Data[BucketServiceAccountSecretName]; ok { - if secretKey, ok := configMap.Data[BucketServiceAccountSecretKey]; ok { - sp.FieldName = "GOOGLE_APPLICATION_CREDENTIALS" - if fieldName, ok := configMap.Data[BucketServiceAccountFieldName]; ok { - sp.FieldName = fieldName - } - sp.SecretName = secretName - sp.SecretKey = secretKey - c.Secrets = append(c.Secrets, sp) - } - } - return c, nil + bucketConfig := config.FromContextOrDefaults(ctx).ArtifactBucket + c.Location = bucketConfig.Location + sp := resourcev1alpha1.SecretParam{} + if bucketConfig.ServiceAccountSecretName != "" && bucketConfig.ServiceAccountSecretKey != "" { + sp.SecretName = bucketConfig.ServiceAccountSecretName + sp.SecretKey = bucketConfig.ServiceAccountSecretKey + sp.FieldName = bucketConfig.ServiceAccountFieldName + c.Secrets = append(c.Secrets, sp) } + return c } -func createPVC(pr *v1beta1.PipelineRun, c kubernetes.Interface) (*corev1.PersistentVolumeClaim, error) { +func createPVC(ctx context.Context, pr *v1beta1.PipelineRun, c kubernetes.Interface) (*corev1.PersistentVolumeClaim, error) { if _, err := c.CoreV1().PersistentVolumeClaims(pr.Namespace).Get(GetPVCName(pr), metav1.GetOptions{}); err != nil { if errors.IsNotFound(err) { - - configMap, err := c.CoreV1().ConfigMaps(system.GetNamespace()).Get(GetPVCConfigName(), metav1.GetOptions{}) - if err != nil && !errors.IsNotFound(err) { - return nil, fmt.Errorf("failed to get PVC ConfigMap %s for %q due to error: %w", GetPVCConfigName(), pr.Name, err) - } - var pvcSizeStr string - var pvcStorageClassNameStr string - if configMap != nil { - pvcSizeStr = configMap.Data[PVCSizeKey] - pvcStorageClassNameStr = configMap.Data[PVCStorageClassNameKey] - } - if pvcSizeStr == "" { - pvcSizeStr = DefaultPVCSize - } - pvcSize, err := resource.ParseQuantity(pvcSizeStr) + pvcConfig := config.FromContextOrDefaults(ctx).ArtifactPVC + pvcSize, err := resource.ParseQuantity(pvcConfig.Size) if err != nil { - return nil, fmt.Errorf("failed to create Persistent Volume spec for %q due to error: %w", pr.Name, err) + return nil, err } var pvcStorageClassName *string - if pvcStorageClassNameStr == "" { + if pvcConfig.StorageClassName == "" { pvcStorageClassName = nil } else { - pvcStorageClassName = &pvcStorageClassNameStr + pvcStorageClassName = &pvcConfig.StorageClassName } pvcSpec := GetPVCSpec(pr, pvcSize, pvcStorageClassName) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index b30eb273955..1ff6c2a9b8e 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -163,7 +163,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1beta1.PipelineRun) pr.SetDefaults(contexts.WithUpgradeViaDefaulting(ctx)) c.updatePipelineResults(ctx, pr) - if err := artifacts.CleanupArtifactStorage(pr, c.KubeClientSet, logger); err != nil { + if err := artifacts.CleanupArtifactStorage(ctx, pr, c.KubeClientSet); err != nil { logger.Errorf("Failed to delete PVC for PipelineRun %s: %v", pr.Name, err) return c.finishReconcileUpdateEmitEvents(ctx, pr, before, err) } @@ -450,7 +450,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err } } - as, err := artifacts.InitializeArtifactStorage(c.Images, pr, pipelineSpec, c.KubeClientSet, logger) + as, err := artifacts.InitializeArtifactStorage(ctx, c.Images, pr, pipelineSpec, c.KubeClientSet) if err != nil { logger.Infof("PipelineRun failed to initialize artifact storage %s", pr.Name) return controller.NewPermanentError(err) diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 611b592bddb..4894bc3c544 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -83,7 +83,7 @@ func getRunName(pr *v1beta1.PipelineRun) string { } func ensureConfigurationConfigMapsExist(d *test.Data) { - var defaultsExists, featureFlagsExists bool + var defaultsExists, featureFlagsExists, artifactBucketExists, artifactPVCExists bool for _, cm := range d.ConfigMaps { if cm.Name == config.GetDefaultsConfigName() { defaultsExists = true @@ -91,6 +91,12 @@ func ensureConfigurationConfigMapsExist(d *test.Data) { if cm.Name == config.GetFeatureFlagsConfigName() { featureFlagsExists = true } + if cm.Name == config.GetArtifactBucketConfigName() { + artifactBucketExists = true + } + if cm.Name == config.GetArtifactPVCConfigName() { + artifactPVCExists = true + } } if !defaultsExists { d.ConfigMaps = append(d.ConfigMaps, &corev1.ConfigMap{ @@ -104,6 +110,18 @@ func ensureConfigurationConfigMapsExist(d *test.Data) { Data: map[string]string{}, }) } + if !artifactBucketExists { + d.ConfigMaps = append(d.ConfigMaps, &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: config.GetArtifactBucketConfigName(), Namespace: system.GetNamespace()}, + Data: map[string]string{}, + }) + } + if !artifactPVCExists { + d.ConfigMaps = append(d.ConfigMaps, &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: config.GetArtifactPVCConfigName(), Namespace: system.GetNamespace()}, + Data: map[string]string{}, + }) + } } // getPipelineRunController returns an instance of the PipelineRun controller/reconciler that has been seeded with diff --git a/pkg/reconciler/taskrun/resources/input_resource_test.go b/pkg/reconciler/taskrun/resources/input_resource_test.go index 962979d0acb..f88ee86c130 100644 --- a/pkg/reconciler/taskrun/resources/input_resource_test.go +++ b/pkg/reconciler/taskrun/resources/input_resource_test.go @@ -17,18 +17,17 @@ limitations under the License. package resources import ( + "context" "testing" "github.com/google/go-cmp/cmp" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/apis/resource" resourcev1alpha1 "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1" - "github.com/tektoncd/pipeline/pkg/artifacts" - "github.com/tektoncd/pipeline/pkg/logging" "github.com/tektoncd/pipeline/test/diff" "github.com/tektoncd/pipeline/test/names" - "go.uber.org/zap" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" fakek8s "k8s.io/client-go/kubernetes/fake" @@ -49,7 +48,6 @@ var ( ImageDigestExporterImage: "override-with-imagedigest-exporter-image:latest", } inputResourceInterfaces map[string]v1beta1.PipelineResourceInterface - logger *zap.SugaredLogger gitInputs = []v1beta1.TaskResource{{ ResourceDeclaration: v1beta1.ResourceDeclaration{ @@ -108,7 +106,6 @@ var ( ) func setUp() { - logger, _ = logging.NewLogger("", "") rs := []*resourcev1alpha1.PipelineResource{{ ObjectMeta: metav1.ObjectMeta{ @@ -966,7 +963,7 @@ gsutil cp gs://fake-bucket/rules.zip /workspace/gcs-dir setUp() names.TestingSeed() fakekubeclient := fakek8s.NewSimpleClientset() - got, err := AddInputResource(fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, mockResolveTaskResources(c.taskRun), logger) + got, err := AddInputResource(context.Background(), fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, mockResolveTaskResources(c.taskRun)) if (err != nil) != c.wantErr { t.Errorf("Test: %q; AddInputResource() error = %v, WantErr %v", c.desc, err, c.wantErr) } @@ -1202,7 +1199,7 @@ gsutil rsync -d -r gs://fake-bucket/rules.zip /workspace/gcs-input-resource names.TestingSeed() setUp() fakekubeclient := fakek8s.NewSimpleClientset() - got, err := AddInputResource(fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, mockResolveTaskResources(c.taskRun), logger) + got, err := AddInputResource(context.Background(), fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, mockResolveTaskResources(c.taskRun)) if (err != nil) != c.wantErr { t.Errorf("Test: %q; AddInputResource() error = %v, WantErr %v", c.desc, err, c.wantErr) } @@ -1425,20 +1422,20 @@ func TestAddStepsToTaskWithBucketFromConfigMap(t *testing.T) { }} { t.Run(c.desc, func(t *testing.T) { setUp() - fakekubeclient := fakek8s.NewSimpleClientset( - &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "tekton-pipelines", - Name: artifacts.GetBucketConfigName(), - }, - Data: map[string]string{ - artifacts.BucketLocationKey: "gs://fake-bucket", - artifacts.BucketServiceAccountSecretName: "gcs-config", - artifacts.BucketServiceAccountSecretKey: "my-key", - }, - }, - ) - got, err := AddInputResource(fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, mockResolveTaskResources(c.taskRun), logger) + fakekubeclient := fakek8s.NewSimpleClientset() + bucketConfig, err := config.NewArtifactBucketFromMap(map[string]string{ + config.BucketLocationKey: "gs://fake-bucket", + config.BucketServiceAccountSecretNameKey: "gcs-config", + config.BucketServiceAccountSecretKeyKey: "my-key", + }) + if err != nil { + t.Fatalf("Test: %q; Error setting up bucket config = %v", c.desc, err) + } + configs := config.Config{ + ArtifactBucket: bucketConfig, + } + ctx := config.ToContext(context.Background(), &configs) + got, err := AddInputResource(ctx, fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, mockResolveTaskResources(c.taskRun)) if err != nil { t.Errorf("Test: %q; AddInputResource() error = %v", c.desc, err) } diff --git a/pkg/reconciler/taskrun/resources/input_resources.go b/pkg/reconciler/taskrun/resources/input_resources.go index 083f5f28f4a..0ba8e501cad 100644 --- a/pkg/reconciler/taskrun/resources/input_resources.go +++ b/pkg/reconciler/taskrun/resources/input_resources.go @@ -17,6 +17,7 @@ limitations under the License. package resources import ( + "context" "fmt" "path/filepath" @@ -24,7 +25,6 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1/storage" "github.com/tektoncd/pipeline/pkg/artifacts" - "go.uber.org/zap" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/kubernetes" ) @@ -45,13 +45,13 @@ func getBoundResource(resourceName string, boundResources []v1beta1.TaskResource // from previous task // 3. If resource has paths declared then fresh copy of resource is not fetched func AddInputResource( + ctx context.Context, kubeclient kubernetes.Interface, images pipeline.Images, taskName string, taskSpec *v1beta1.TaskSpec, taskRun *v1beta1.TaskRun, inputResources map[string]v1beta1.PipelineResourceInterface, - logger *zap.SugaredLogger, ) (*v1beta1.TaskSpec, error) { if taskSpec == nil || taskSpec.Resources == nil || taskSpec.Resources.Inputs == nil { return taskSpec, nil @@ -66,10 +66,7 @@ func AddInputResource( if prNameFromLabel == "" { prNameFromLabel = pvcName } - as, err := artifacts.GetArtifactStorage(images, prNameFromLabel, kubeclient, logger) - if err != nil { - return nil, err - } + as := artifacts.GetArtifactStorage(ctx, images, prNameFromLabel, kubeclient) // Iterate in reverse through the list, each element prepends but we want the first one to remain first. for i := len(taskSpec.Resources.Inputs) - 1; i >= 0; i-- { diff --git a/pkg/reconciler/taskrun/resources/output_resource.go b/pkg/reconciler/taskrun/resources/output_resource.go index 3fcc5dc748a..b06f2628a3f 100644 --- a/pkg/reconciler/taskrun/resources/output_resource.go +++ b/pkg/reconciler/taskrun/resources/output_resource.go @@ -17,6 +17,7 @@ limitations under the License. package resources import ( + "context" "fmt" "path/filepath" @@ -24,7 +25,6 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1/storage" "github.com/tektoncd/pipeline/pkg/artifacts" - "go.uber.org/zap" "k8s.io/client-go/kubernetes" ) @@ -45,13 +45,13 @@ var ( // 1. If resource has a targetpath that is used. Otherwise: // 2. If resource is declared in outputs only then the default is /output/resource_name func AddOutputResources( + ctx context.Context, kubeclient kubernetes.Interface, images pipeline.Images, taskName string, taskSpec *v1beta1.TaskSpec, taskRun *v1beta1.TaskRun, outputResources map[string]v1beta1.PipelineResourceInterface, - logger *zap.SugaredLogger, ) (*v1beta1.TaskSpec, error) { if taskSpec == nil || taskSpec.Resources == nil || taskSpec.Resources.Outputs == nil { return taskSpec, nil @@ -60,10 +60,8 @@ func AddOutputResources( taskSpec = taskSpec.DeepCopy() pvcName := taskRun.GetPipelineRunPVCName() - as, err := artifacts.GetArtifactStorage(images, pvcName, kubeclient, logger) - if err != nil { - return nil, err - } + as := artifacts.GetArtifactStorage(ctx, images, pvcName, kubeclient) + needsPvc := false for _, output := range taskSpec.Resources.Outputs { if taskRun.Spec.Resources == nil { diff --git a/pkg/reconciler/taskrun/resources/output_resource_test.go b/pkg/reconciler/taskrun/resources/output_resource_test.go index 7e112f0ef99..40bb83f6fa2 100644 --- a/pkg/reconciler/taskrun/resources/output_resource_test.go +++ b/pkg/reconciler/taskrun/resources/output_resource_test.go @@ -17,14 +17,14 @@ limitations under the License. package resources import ( + "context" "testing" "github.com/google/go-cmp/cmp" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/apis/resource" resourcev1alpha1 "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1" - "github.com/tektoncd/pipeline/pkg/artifacts" - "github.com/tektoncd/pipeline/pkg/logging" "github.com/tektoncd/pipeline/test/diff" "github.com/tektoncd/pipeline/test/names" corev1 "k8s.io/api/core/v1" @@ -37,7 +37,6 @@ var ( ) func outputTestResourceSetup() { - logger, _ = logging.NewLogger("", "") rs := []*resourcev1alpha1.PipelineResource{{ ObjectMeta: metav1.ObjectMeta{ @@ -917,7 +916,7 @@ func TestValidOutputResources(t *testing.T) { names.TestingSeed() outputTestResourceSetup() fakekubeclient := fakek8s.NewSimpleClientset() - got, err := AddOutputResources(fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, resolveOutputResources(c.taskRun), logger) + got, err := AddOutputResources(context.Background(), fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, resolveOutputResources(c.taskRun)) if err != nil { t.Fatalf("Failed to declare output resources for test name %q ; test description %q: error %v", c.name, c.desc, err) } @@ -1101,18 +1100,18 @@ func TestValidOutputResourcesWithBucketStorage(t *testing.T) { t.Run(c.name, func(t *testing.T) { outputTestResourceSetup() names.TestingSeed() - fakekubeclient := fakek8s.NewSimpleClientset( - &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "tekton-pipelines", - Name: artifacts.GetBucketConfigName(), - }, - Data: map[string]string{ - artifacts.BucketLocationKey: "gs://fake-bucket", - }, - }, - ) - got, err := AddOutputResources(fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, resolveOutputResources(c.taskRun), logger) + fakekubeclient := fakek8s.NewSimpleClientset() + bucketConfig, err := config.NewArtifactBucketFromMap(map[string]string{ + config.BucketLocationKey: "gs://fake-bucket", + }) + if err != nil { + t.Fatalf("Test: %q; Error setting up bucket config = %v", c.desc, err) + } + configs := config.Config{ + ArtifactBucket: bucketConfig, + } + ctx := config.ToContext(context.Background(), &configs) + got, err := AddOutputResources(ctx, fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, resolveOutputResources(c.taskRun)) if err != nil { t.Fatalf("Failed to declare output resources for test name %q ; test description %q: error %v", c.name, c.desc, err) } @@ -1316,7 +1315,7 @@ func TestInvalidOutputResources(t *testing.T) { t.Run(c.desc, func(t *testing.T) { outputTestResourceSetup() fakekubeclient := fakek8s.NewSimpleClientset() - _, err := AddOutputResources(fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, resolveOutputResources(c.taskRun), logger) + _, err := AddOutputResources(context.Background(), fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, resolveOutputResources(c.taskRun)) if (err != nil) != c.wantErr { t.Fatalf("Test AddOutputResourceSteps %v : error%v", c.desc, err) } @@ -1707,27 +1706,26 @@ func TestInputOutputBucketResources(t *testing.T) { t.Run(c.name, func(t *testing.T) { names.TestingSeed() outputTestResourceSetup() - fakekubeclient := fakek8s.NewSimpleClientset( - &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "tekton-pipelines", - Name: artifacts.GetBucketConfigName(), - }, - Data: map[string]string{ - artifacts.BucketLocationKey: "gs://fake-bucket", - artifacts.BucketServiceAccountSecretName: "sname", - artifacts.BucketServiceAccountSecretKey: "key.json", - }, - }, - ) - + fakekubeclient := fakek8s.NewSimpleClientset() + bucketConfig, err := config.NewArtifactBucketFromMap(map[string]string{ + config.BucketLocationKey: "gs://fake-bucket", + config.BucketServiceAccountSecretNameKey: "sname", + config.BucketServiceAccountSecretKeyKey: "key.json", + }) + if err != nil { + t.Fatalf("Test: %q; Error setting up bucket config = %v", c.desc, err) + } + configs := config.Config{ + ArtifactBucket: bucketConfig, + } + ctx := config.ToContext(context.Background(), &configs) inputs := resolveInputResources(c.taskRun) - ts, err := AddInputResource(fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, inputs, logger) + ts, err := AddInputResource(ctx, fakekubeclient, images, c.task.Name, &c.task.Spec, c.taskRun, inputs) if err != nil { t.Fatalf("Failed to declare input resources for test name %q ; test description %q: error %v", c.name, c.desc, err) } - got, err := AddOutputResources(fakekubeclient, images, c.task.Name, ts, c.taskRun, resolveOutputResources(c.taskRun), logger) + got, err := AddOutputResources(ctx, fakekubeclient, images, c.task.Name, ts, c.taskRun, resolveOutputResources(c.taskRun)) if err != nil { t.Fatalf("Failed to declare output resources for test name %q ; test description %q: error %v", c.name, c.desc, err) } diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 948599da346..cf98a402471 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -494,13 +494,13 @@ func (c *Reconciler) createPod(ctx context.Context, tr *v1beta1.TaskRun, rtr *re return nil, err } - ts, err = resources.AddInputResource(c.KubeClientSet, c.Images, rtr.TaskName, ts, tr, inputResources, logger) + ts, err = resources.AddInputResource(ctx, c.KubeClientSet, c.Images, rtr.TaskName, ts, tr, inputResources) if err != nil { logger.Errorf("Failed to create a pod for taskrun: %s due to input resource error %v", tr.Name, err) return nil, err } - ts, err = resources.AddOutputResources(c.KubeClientSet, c.Images, rtr.TaskName, ts, tr, outputResources, logger) + ts, err = resources.AddOutputResources(ctx, c.KubeClientSet, c.Images, rtr.TaskName, ts, tr, outputResources) if err != nil { logger.Errorf("Failed to create a pod for taskrun: %s due to output resource error %v", tr.Name, err) return nil, err diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index 1f8171bfe0e..edf6f094844 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -235,7 +235,7 @@ func getRunName(tr *v1beta1.TaskRun) string { } func ensureConfigurationConfigMapsExist(d *test.Data) { - var defaultsExists, featureFlagsExists bool + var defaultsExists, featureFlagsExists, artifactBucketExists, artifactPVCExists bool for _, cm := range d.ConfigMaps { if cm.Name == config.GetDefaultsConfigName() { defaultsExists = true @@ -243,6 +243,12 @@ func ensureConfigurationConfigMapsExist(d *test.Data) { if cm.Name == config.GetFeatureFlagsConfigName() { featureFlagsExists = true } + if cm.Name == config.GetArtifactBucketConfigName() { + artifactBucketExists = true + } + if cm.Name == config.GetArtifactPVCConfigName() { + artifactPVCExists = true + } } if !defaultsExists { d.ConfigMaps = append(d.ConfigMaps, &corev1.ConfigMap{ @@ -256,6 +262,18 @@ func ensureConfigurationConfigMapsExist(d *test.Data) { Data: map[string]string{}, }) } + if !artifactBucketExists { + d.ConfigMaps = append(d.ConfigMaps, &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: config.GetArtifactBucketConfigName(), Namespace: system.GetNamespace()}, + Data: map[string]string{}, + }) + } + if !artifactPVCExists { + d.ConfigMaps = append(d.ConfigMaps, &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: config.GetArtifactPVCConfigName(), Namespace: system.GetNamespace()}, + Data: map[string]string{}, + }) + } } // getTaskRunController returns an instance of the TaskRun controller/reconciler that has been seeded with diff --git a/test/artifact_bucket_test.go b/test/artifact_bucket_test.go index eaaed92b222..09e6716b591 100644 --- a/test/artifact_bucket_test.go +++ b/test/artifact_bucket_test.go @@ -26,10 +26,10 @@ import ( "time" tb "github.com/tektoncd/pipeline/internal/builder/v1beta1" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" resourcev1alpha1 "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1" - "github.com/tektoncd/pipeline/pkg/artifacts" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" knativetest "knative.dev/pkg/test" @@ -118,22 +118,22 @@ func TestStorageBucketPipelineRun(t *testing.T) { defer runTaskToDeleteBucket(c, t, namespace, bucketName, bucketSecretName, bucketSecretKey) - originalConfigMap, err := c.KubeClient.Kube.CoreV1().ConfigMaps(systemNamespace).Get(artifacts.GetBucketConfigName(), metav1.GetOptions{}) + originalConfigMap, err := c.KubeClient.Kube.CoreV1().ConfigMaps(systemNamespace).Get(config.GetArtifactBucketConfigName(), metav1.GetOptions{}) if err != nil { - t.Fatalf("Failed to get ConfigMap `%s`: %s", artifacts.GetBucketConfigName(), err) + t.Fatalf("Failed to get ConfigMap `%s`: %s", config.GetArtifactBucketConfigName(), err) } originalConfigMapData := originalConfigMap.Data - t.Logf("Creating ConfigMap %s", artifacts.GetBucketConfigName()) + t.Logf("Creating ConfigMap %s", config.GetArtifactBucketConfigName()) configMapData := map[string]string{ - artifacts.BucketLocationKey: fmt.Sprintf("gs://%s", bucketName), - artifacts.BucketServiceAccountSecretName: bucketSecretName, - artifacts.BucketServiceAccountSecretKey: bucketSecretKey, + config.BucketLocationKey: fmt.Sprintf("gs://%s", bucketName), + config.BucketServiceAccountSecretNameKey: bucketSecretName, + config.BucketServiceAccountSecretKeyKey: bucketSecretKey, } - if err := updateConfigMap(c.KubeClient, systemNamespace, artifacts.GetBucketConfigName(), configMapData); err != nil { + if err := updateConfigMap(c.KubeClient, systemNamespace, config.GetArtifactBucketConfigName(), configMapData); err != nil { t.Fatal(err) } - defer resetConfigMap(t, c, systemNamespace, artifacts.GetBucketConfigName(), originalConfigMapData) + defer resetConfigMap(t, c, systemNamespace, config.GetArtifactBucketConfigName(), originalConfigMapData) t.Logf("Creating Git PipelineResource %s", helloworldResourceName) helloworldResource := tb.PipelineResource(helloworldResourceName, tb.PipelineResourceSpec( diff --git a/test/v1alpha1/artifact_bucket_test.go b/test/v1alpha1/artifact_bucket_test.go index 2df4258d564..28d4ec62266 100644 --- a/test/v1alpha1/artifact_bucket_test.go +++ b/test/v1alpha1/artifact_bucket_test.go @@ -26,8 +26,8 @@ import ( "time" tb "github.com/tektoncd/pipeline/internal/builder/v1alpha1" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" - "github.com/tektoncd/pipeline/pkg/artifacts" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" knativetest "knative.dev/pkg/test" @@ -100,22 +100,22 @@ func TestStorageBucketPipelineRun(t *testing.T) { defer runTaskToDeleteBucket(c, t, namespace, bucketName, bucketSecretName, bucketSecretKey) - originalConfigMap, err := c.KubeClient.Kube.CoreV1().ConfigMaps(systemNamespace).Get(artifacts.GetBucketConfigName(), metav1.GetOptions{}) + originalConfigMap, err := c.KubeClient.Kube.CoreV1().ConfigMaps(systemNamespace).Get(config.GetArtifactBucketConfigName(), metav1.GetOptions{}) if err != nil { - t.Fatalf("Failed to get ConfigMap `%s`: %s", artifacts.GetBucketConfigName(), err) + t.Fatalf("Failed to get ConfigMap `%s`: %s", config.GetArtifactBucketConfigName(), err) } originalConfigMapData := originalConfigMap.Data - t.Logf("Creating ConfigMap %s", artifacts.GetBucketConfigName()) + t.Logf("Creating ConfigMap %s", config.GetArtifactBucketConfigName()) configMapData := map[string]string{ - artifacts.BucketLocationKey: fmt.Sprintf("gs://%s", bucketName), - artifacts.BucketServiceAccountSecretName: bucketSecretName, - artifacts.BucketServiceAccountSecretKey: bucketSecretKey, + config.BucketLocationKey: fmt.Sprintf("gs://%s", bucketName), + config.BucketServiceAccountSecretNameKey: bucketSecretName, + config.BucketServiceAccountSecretKeyKey: bucketSecretKey, } - if err := updateConfigMap(c.KubeClient, systemNamespace, artifacts.GetBucketConfigName(), configMapData); err != nil { + if err := updateConfigMap(c.KubeClient, systemNamespace, config.GetArtifactBucketConfigName(), configMapData); err != nil { t.Fatal(err) } - defer resetConfigMap(t, c, systemNamespace, artifacts.GetBucketConfigName(), originalConfigMapData) + defer resetConfigMap(t, c, systemNamespace, config.GetArtifactBucketConfigName(), originalConfigMapData) t.Logf("Creating Git PipelineResource %s", helloworldResourceName) helloworldResource := tb.PipelineResource(helloworldResourceName, tb.PipelineResourceSpec(