From 8e9874c57f0d4e32044e57939f2b1b9623afc00f 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 InitializeArtifactStorage to use config from the store. --- 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/store.go | 20 +- .../config-artifact-bucket-all-set.yaml | 24 +++ .../config-artifact-bucket-empty.yaml | 26 +++ .../testdata/config-artifact-bucket.yaml | 4 +- .../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 | 191 ++++++++--------- pkg/artifacts/artifacts_storage.go | 193 ++++-------------- pkg/reconciler/pipelinerun/config/store.go | 83 -------- .../pipelinerun/config/store_test.go | 57 ------ pkg/reconciler/pipelinerun/pipelinerun.go | 2 +- .../taskrun/resources/input_resources.go | 8 +- .../taskrun/resources/output_resource.go | 9 +- pkg/reconciler/taskrun/taskrun.go | 4 +- 21 files changed, 722 insertions(+), 413 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 rename pkg/{reconciler/pipelinerun => apis}/config/testdata/config-artifact-bucket.yaml (88%) 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 delete mode 100644 pkg/reconciler/pipelinerun/config/store.go delete mode 100644 pkg/reconciler/pipelinerun/config/store_test.go 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/store.go b/pkg/apis/config/store.go index 9b5c87331d5..08d9b509c53 100644 --- a/pkg/apis/config/store.go +++ b/pkg/apis/config/store.go @@ -29,6 +29,8 @@ type cfgKey struct{} type Config struct { 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, + 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, + GetArtifactBucketConfigName(): NewArtifactBucketFromMap, + GetArtifactPVCConfigName(): NewArtifactPVCFromMap, }, 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(), + ArtifactBucket: artifactBucket.(*ArtifactBucket).DeepCopy(), + ArtifactPVC: artifactPVC.(*ArtifactPVC).DeepCopy(), } } 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/reconciler/pipelinerun/config/testdata/config-artifact-bucket.yaml b/pkg/apis/config/testdata/config-artifact-bucket.yaml similarity index 88% rename from pkg/reconciler/pipelinerun/config/testdata/config-artifact-bucket.yaml rename to pkg/apis/config/testdata/config-artifact-bucket.yaml index 1838e28dfcb..a4a7f062f5e 100644 --- a/pkg/reconciler/pipelinerun/config/testdata/config-artifact-bucket.yaml +++ b/pkg/apis/config/testdata/config-artifact-bucket.yaml @@ -1,4 +1,4 @@ -# Copyright 2019 The Tekton Authors +# 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. @@ -18,4 +18,4 @@ metadata: name: config-artifact-bucket namespace: tekton-pipelines data: - location: "gs://build-pipeline-fake-bucket" + 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..1d31c2b002c 100644 --- a/pkg/artifacts/artifact_storage_test.go +++ b/pkg/artifacts/artifact_storage_test.go @@ -17,10 +17,12 @@ 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" @@ -57,7 +59,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 +116,53 @@ func GetPersistentVolumeClaim(size string, storageClassName *string) *corev1.Per return pvc } -func TestConfigMapNeedsPVC(t *testing.T) { +func TestNeedsPVC(t *testing.T) { logger := logtesting.TestLogger(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) } @@ -208,10 +183,10 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetPVCConfigName(), + Name: config.GetArtifactPVCConfigName(), }, Data: map[string]string{ - PVCSizeKey: "10Gi", + config.PVCSizeKey: "10Gi", }, }, expectedArtifactStorage: &storage.ArtifactPVC{ @@ -225,10 +200,10 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetPVCConfigName(), + Name: config.GetArtifactPVCConfigName(), }, Data: map[string]string{ - PVCStorageClassNameKey: customStorageClass, + config.PVCStorageClassNameKey: customStorageClass, }, }, expectedArtifactStorage: &storage.ArtifactPVC{ @@ -242,12 +217,12 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), + Name: config.GetArtifactBucketConfigName(), }, Data: map[string]string{ - BucketLocationKey: "gs://fake-bucket", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", + config.BucketLocationKey: "gs://fake-bucket", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, }, expectedArtifactStorage: &storage.ArtifactBucket{ @@ -266,12 +241,12 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), + Name: config.GetArtifactBucketConfigName(), }, Data: map[string]string{ - BucketLocationKey: "", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", + config.BucketLocationKey: "", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, }, expectedArtifactStorage: &storage.ArtifactPVC{ @@ -285,11 +260,11 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), + Name: config.GetArtifactBucketConfigName(), }, Data: map[string]string{ - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, }, expectedArtifactStorage: &storage.ArtifactPVC{ @@ -303,7 +278,7 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), + Name: config.GetArtifactBucketConfigName(), }, }, expectedArtifactStorage: &storage.ArtifactPVC{ @@ -317,10 +292,10 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), + Name: config.GetArtifactBucketConfigName(), }, Data: map[string]string{ - BucketLocationKey: "gs://fake-bucket", + config.BucketLocationKey: "gs://fake-bucket", }, }, expectedArtifactStorage: &storage.ArtifactBucket{ @@ -334,13 +309,13 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), + Name: config.GetArtifactBucketConfigName(), }, Data: map[string]string{ - BucketLocationKey: "s3://fake-bucket", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", - BucketServiceAccountFieldName: "BOTO_CONFIG", + config.BucketLocationKey: "s3://fake-bucket", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", + config.BucketServiceAccountFieldNameKey: "BOTO_CONFIG", }, }, expectedArtifactStorage: &storage.ArtifactBucket{ @@ -451,10 +426,10 @@ func TestInitializeArtifactStorageNoStorageNeeded(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetPVCConfigName(), + Name: config.GetArtifactPVCConfigName(), }, Data: map[string]string{ - PVCSizeKey: "10Gi", + config.PVCSizeKey: "10Gi", }, }, }, { @@ -462,12 +437,12 @@ func TestInitializeArtifactStorageNoStorageNeeded(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), + Name: config.GetArtifactBucketConfigName(), }, Data: map[string]string{ - BucketLocationKey: "gs://fake-bucket", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", + config.BucketLocationKey: "gs://fake-bucket", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, }, }, { @@ -475,12 +450,12 @@ func TestInitializeArtifactStorageNoStorageNeeded(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), + Name: config.GetArtifactBucketConfigName(), }, Data: map[string]string{ - BucketLocationKey: "", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", + config.BucketLocationKey: "", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, }, }} { @@ -513,12 +488,12 @@ func TestCleanupArtifactStorage(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), + Name: config.GetArtifactBucketConfigName(), }, Data: map[string]string{ - BucketLocationKey: "", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", + config.BucketLocationKey: "", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, }, }, { @@ -526,11 +501,11 @@ func TestCleanupArtifactStorage(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), + Name: config.GetArtifactBucketConfigName(), }, Data: map[string]string{ - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, }, }, { @@ -538,7 +513,7 @@ func TestCleanupArtifactStorage(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), + Name: config.GetArtifactBucketConfigName(), }, }, }} { @@ -604,12 +579,12 @@ func TestGetArtifactStorageWithConfigMap(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), + Name: config.GetArtifactBucketConfigName(), }, Data: map[string]string{ - BucketLocationKey: "gs://fake-bucket", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", + config.BucketLocationKey: "gs://fake-bucket", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, }, expectedArtifactStorage: &storage.ArtifactBucket{ @@ -627,12 +602,12 @@ func TestGetArtifactStorageWithConfigMap(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), + Name: config.GetArtifactBucketConfigName(), }, Data: map[string]string{ - BucketLocationKey: "", - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", + config.BucketLocationKey: "", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, }, expectedArtifactStorage: &storage.ArtifactPVC{ @@ -644,11 +619,11 @@ func TestGetArtifactStorageWithConfigMap(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), + Name: config.GetArtifactBucketConfigName(), }, Data: map[string]string{ - BucketServiceAccountSecretName: "secret1", - BucketServiceAccountSecretKey: "sakey", + config.BucketServiceAccountSecretNameKey: "secret1", + config.BucketServiceAccountSecretKeyKey: "sakey", }, }, expectedArtifactStorage: &storage.ArtifactPVC{ @@ -660,7 +635,7 @@ func TestGetArtifactStorageWithConfigMap(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetBucketConfigName(), + Name: config.GetArtifactBucketConfigName(), }, }, expectedArtifactStorage: &storage.ArtifactPVC{ @@ -713,10 +688,10 @@ func TestGetArtifactStorageWithPVCConfigMap(t *testing.T) { configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: GetPVCConfigName(), + Name: config.GetArtifactPVCConfigName(), }, Data: map[string]string{ - PVCSizeKey: "10Gi", + config.PVCSizeKey: "10Gi", }, }, expectedArtifactStorage: &storage.ArtifactPVC{ diff --git a/pkg/artifacts/artifacts_storage.go b/pkg/artifacts/artifacts_storage.go index 67f0d7305cf..b96d54ea45c 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,108 +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 - } - if strings.TrimSpace(location) == "" { - logger.Warnf("the configmap key %q is empty", BucketLocationKey) - return true, nil +func NeedsPVC(ctx context.Context) bool { + bucketConfig := config.FromContextOrDefaults(ctx).ArtifactBucket + 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 + 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) + pvcSpec := GetPVCSpec(pr, pvcSize, &pvcConfig.StorageClassName) pvc, err := c.CoreV1().PersistentVolumeClaims(pr.Namespace).Create(pvcSpec) if err != nil { return nil, fmt.Errorf("failed to claim Persistent Volume %q due to error: %w", pr.Name, err) diff --git a/pkg/reconciler/pipelinerun/config/store.go b/pkg/reconciler/pipelinerun/config/store.go deleted file mode 100644 index 7f46dd41e72..00000000000 --- a/pkg/reconciler/pipelinerun/config/store.go +++ /dev/null @@ -1,83 +0,0 @@ -/* -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 ( - "context" - - "github.com/tektoncd/pipeline/pkg/apis/pipeline" - "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1/storage" - "github.com/tektoncd/pipeline/pkg/artifacts" - "knative.dev/pkg/configmap" -) - -type cfgKey struct{} - -// +k8s:deepcopy-gen=false -type Config struct { - ArtifactBucket *storage.ArtifactBucket -} - -func FromContext(ctx context.Context) *Config { - return ctx.Value(cfgKey{}).(*Config) -} - -func ToContext(ctx context.Context, c *Config) context.Context { - return context.WithValue(ctx, cfgKey{}, c) -} - -// +k8s:deepcopy-gen=false -type Store struct { - *configmap.UntypedStore - - images pipeline.Images -} - -func NewStore(images pipeline.Images, logger configmap.Logger) *Store { - return &Store{ - UntypedStore: configmap.NewUntypedStore( - "pipelinerun", - logger, - configmap.Constructors{ - artifacts.GetBucketConfigName(): artifacts.NewArtifactBucketConfigFromConfigMap(images), - }, - ), - images: images, - } -} - -func (s *Store) ToContext(ctx context.Context) context.Context { - return ToContext(ctx, s.Load()) -} - -func (s *Store) Load() *Config { - ep := s.UntypedLoad(artifacts.GetBucketConfigName()) - if ep == nil { - return &Config{ - ArtifactBucket: &storage.ArtifactBucket{ - Location: "", - ShellImage: s.images.ShellImage, - GsutilImage: s.images.GsutilImage, - }, - } - } - - return &Config{ - ArtifactBucket: ep.(*storage.ArtifactBucket).DeepCopy(), - } - -} diff --git a/pkg/reconciler/pipelinerun/config/store_test.go b/pkg/reconciler/pipelinerun/config/store_test.go deleted file mode 100644 index 101e3b2991b..00000000000 --- a/pkg/reconciler/pipelinerun/config/store_test.go +++ /dev/null @@ -1,57 +0,0 @@ -/* -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 ( - "context" - "testing" - - "github.com/google/go-cmp/cmp" - "github.com/tektoncd/pipeline/pkg/apis/pipeline" - "github.com/tektoncd/pipeline/pkg/artifacts" - ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing" - "github.com/tektoncd/pipeline/test/diff" - - test "github.com/tektoncd/pipeline/pkg/reconciler/testing" -) - -func TestStoreLoadWithContext(t *testing.T) { - store := NewStore(pipeline.Images{}, ttesting.TestLogger(t)) - bucketConfig := test.ConfigMapFromTestFile(t, "config-artifact-bucket") - store.OnConfigChanged(bucketConfig) - - config := FromContext(store.ToContext(context.Background())) - - expected, _ := artifacts.NewArtifactBucketConfigFromConfigMap(pipeline.Images{})(bucketConfig) - if d := cmp.Diff(expected, config.ArtifactBucket); d != "" { - t.Errorf("Unexpected controller config %s", diff.PrintWantGot(d)) - } -} -func TestStoreImmutableConfig(t *testing.T) { - store := NewStore(pipeline.Images{}, ttesting.TestLogger(t)) - store.OnConfigChanged(test.ConfigMapFromTestFile(t, "config-artifact-bucket")) - - config := store.Load() - - config.ArtifactBucket.Location = "mutated" - - newConfig := store.Load() - - if newConfig.ArtifactBucket.Location == "mutated" { - t.Error("Controller config is not immutable") - } -} diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 2244aa23668..55736d41fb5 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -449,7 +449,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/taskrun/resources/input_resources.go b/pkg/reconciler/taskrun/resources/input_resources.go index 083f5f28f4a..4600fdb5bf7 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" @@ -45,13 +46,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 +67,7 @@ func AddInputResource( if prNameFromLabel == "" { prNameFromLabel = pvcName } - as, err := artifacts.GetArtifactStorage(images, prNameFromLabel, kubeclient, logger) - if err != nil { - return nil, err - } + as := artifacts.GetArtifactStorage(images, prNameFromLabel, kubeclient, logger) // 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..2a0ef4cca7f 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" @@ -45,13 +46,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 +61,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/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index ee5b0c0b116..63a46f3159f 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -499,13 +499,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