From e0fec1811d0d1bdb3f773af1966c7e41976123db Mon Sep 17 00:00:00 2001 From: Chitrang Patel Date: Mon, 8 Apr 2024 09:11:10 -0400 Subject: [PATCH] TEP0154 - Enable concise resolver syntax This PR enables concise resolver syntax interface. --- config/config-feature-flags.yaml | 2 + docs/how-to-write-a-resolver.md | 22 ++- docs/pipeline-api.md | 32 ++++ docs/resolver-template/cmd/resolver/main.go | 9 ++ .../cmd/resolver/main_test.go | 142 +++++++++++++++++- pkg/apis/config/feature_flags.go | 39 +++-- pkg/apis/config/feature_flags_test.go | 5 + .../testdata/feature-flags-all-flags-set.yaml | 1 + ...nvalid-enable-concise-resolver-syntax.yaml | 21 +++ pkg/apis/pipeline/v1/container_validation.go | 78 +++++++--- .../pipeline/v1/container_validation_test.go | 50 ++++-- pkg/apis/pipeline/v1/pipeline_types_test.go | 51 ++++++- .../pipeline/v1/pipelineref_validation.go | 26 +--- .../v1/pipelineref_validation_test.go | 51 +++++-- pkg/apis/pipeline/v1/taskref_validation.go | 32 +--- .../pipeline/v1/taskref_validation_test.go | 42 ++++-- .../pipeline/v1beta1/container_validation.go | 78 +++++++--- .../v1beta1/container_validation_test.go | 52 +++++-- .../pipeline/v1beta1/openapi_generated.go | 7 + .../pipeline/v1beta1/pipeline_types_test.go | 51 ++++++- .../v1beta1/pipelineref_validation.go | 61 ++++++-- .../v1beta1/pipelineref_validation_test.go | 55 ++++--- pkg/apis/pipeline/v1beta1/swagger.json | 4 + .../pipeline/v1beta1/taskref_validation.go | 56 ++++--- .../v1beta1/taskref_validation_test.go | 48 +++--- .../v1beta1/resolution_request_types.go | 7 + .../pipelinerun/pipelinerun_test.go | 2 +- pkg/reconciler/pipelinerun/resources/apply.go | 28 +++- .../pipelinerun/resources/pipelineref.go | 10 +- .../pipelinerun/resources/pipelineref_test.go | 2 + pkg/reconciler/taskrun/resources/taskref.go | 9 ++ .../taskrun/resources/taskref_test.go | 2 + pkg/reconciler/taskrun/taskrun_test.go | 2 +- .../resolver/bundle/resolver.go | 15 +- .../resolver/cluster/resolver.go | 15 +- .../resolver/framework/fakeresolver.go | 20 ++- .../resolver/framework/reconciler_test.go | 136 +++++++++++++++++ pkg/remoteresolution/resolver/git/resolver.go | 34 +++-- .../resolver/http/resolver.go | 30 ++-- .../resolver/http/resolver_test.go | 12 +- pkg/remoteresolution/resolver/hub/resolver.go | 13 +- pkg/remoteresolution/resource/crd_resource.go | 1 + .../resource/crd_resource_test.go | 2 + pkg/remoteresolution/resource/request_test.go | 1 + pkg/resolution/resource/name.go | 5 + pkg/resolution/resource/name_test.go | 105 +++++++++++++ test/e2e-tests-kind-prow-alpha.env | 1 + test/e2e-tests.sh | 14 ++ test/featureflags.go | 15 +- test/remoteresolution/resolution.go | 7 +- 50 files changed, 1196 insertions(+), 307 deletions(-) create mode 100644 pkg/apis/config/testdata/feature-flags-invalid-enable-concise-resolver-syntax.yaml diff --git a/config/config-feature-flags.yaml b/config/config-feature-flags.yaml index bea6994cc99..26a97d523b7 100644 --- a/config/config-feature-flags.yaml +++ b/config/config-feature-flags.yaml @@ -137,3 +137,5 @@ data: # "pipelinerun" for Pipelinerun and "taskrun" for Taskrun. Or a combination of # these. disable-inline-spec: "" + # Setting this flag to "true" will enable the use of concise resolver syntax + enable-concise-resolver-syntax: "false" diff --git a/docs/how-to-write-a-resolver.md b/docs/how-to-write-a-resolver.md index efa9ee61654..cf3f30d9ee1 100644 --- a/docs/how-to-write-a-resolver.md +++ b/docs/how-to-write-a-resolver.md @@ -255,6 +255,7 @@ import ( The `Validate` method checks that the resolution-spec submitted as part of a resolution request are valid. Our example resolver doesn't expect any params in the spec so we'll simply ensure that the there are no params. +Our example resolver also expects format for the `url` to be `demoscheme://` so we'll validate this format. In the previous version, this was instead called `ValidateParams` method. See below for the differences. @@ -268,8 +269,24 @@ func (r *resolver) Validate(ctx context.Context, req *v1beta1.ResolutionRequestS if len(req.Params) > 0 { return errors.New("no params allowed") } + url := req.URL + u, err := neturl.ParseRequestURI(url) + if err != nil { + return err + } + if u.Scheme != "demoscheme" { + return fmt.Errorf("Invalid Scheme. Want %s, Got %s", "demoscheme", u.Scheme) + } + if u.Path == "" { + return errors.New("Empty path.") + } return nil } +``` + +You'll also need to add the `net/url` as `neturl` and `"errors"` package to your list of imports at +the top of the file. + ``` {{% /tab %}} @@ -295,8 +312,8 @@ the top of the file. ## The `Resolve` method We implement the `Resolve` method to do the heavy lifting of fetching -the contents of a file and returning them. For this example we're just -going to return a hard-coded string of YAML. Since Tekton Pipelines +the contents of a file and returning them. It takes in the resolution request spec as input. +For this example we're just going to return a hard-coded string of YAML. Since Tekton Pipelines currently only supports fetching Pipeline resources via remote resolution that's what we'll return. @@ -465,6 +482,7 @@ func (*myResolvedResource) RefSource() *pipelinev1.RefSource { } ``` + ## The deployment configuration Finally, our resolver needs some deployment configuration so that it can diff --git a/docs/pipeline-api.md b/docs/pipeline-api.md index c8e57271de6..ee4a505f849 100644 --- a/docs/pipeline-api.md +++ b/docs/pipeline-api.md @@ -304,6 +304,22 @@ resource being requested. For example: repo URL, commit SHA, path to file, the kind of authentication to leverage, etc.

+ + +url
+ +string + + + +(Optional) +

URL is the runtime url passed to the resolver +to help it figure out how to resolver the resource being +requested. +This is currently at an ALPHA stability level and subject to +alpha API compatibility policies.

+ + @@ -358,6 +374,22 @@ resource being requested. For example: repo URL, commit SHA, path to file, the kind of authentication to leverage, etc.

+ + +url
+ +string + + + +(Optional) +

URL is the runtime url passed to the resolver +to help it figure out how to resolver the resource being +requested. +This is currently at an ALPHA stability level and subject to +alpha API compatibility policies.

+ +

ResolutionRequestStatus diff --git a/docs/resolver-template/cmd/resolver/main.go b/docs/resolver-template/cmd/resolver/main.go index 8bbb01f958c..1484b62a0d7 100644 --- a/docs/resolver-template/cmd/resolver/main.go +++ b/docs/resolver-template/cmd/resolver/main.go @@ -16,6 +16,8 @@ package main import ( "context" "errors" + "fmt" + neturl "net/url" pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "github.com/tektoncd/pipeline/pkg/apis/resolution/v1beta1" @@ -57,6 +59,13 @@ func (r *resolver) Validate(ctx context.Context, req *v1beta1.ResolutionRequestS if len(req.Params) > 0 { return errors.New("no params allowed") } + u, err := neturl.ParseRequestURI(req.URL) + if err != nil { + return err + } + if u.Scheme != "demoscheme" { + return fmt.Errorf("Invalid Scheme. Want %s, Got %s", "demoscheme", u.Scheme) + } return nil } diff --git a/docs/resolver-template/cmd/resolver/main_test.go b/docs/resolver-template/cmd/resolver/main_test.go index 23b9dcf610b..46c6eedb160 100644 --- a/docs/resolver-template/cmd/resolver/main_test.go +++ b/docs/resolver-template/cmd/resolver/main_test.go @@ -18,15 +18,18 @@ package main import ( "encoding/base64" + "errors" "testing" "time" + pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "github.com/tektoncd/pipeline/pkg/apis/resolution/v1beta1" ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing" frtesting "github.com/tektoncd/pipeline/pkg/remoteresolution/resolver/framework/testing" resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common" "github.com/tektoncd/pipeline/test" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + v1 "knative.dev/pkg/apis/duck/v1" _ "knative.dev/pkg/system/testing" ) @@ -48,7 +51,9 @@ func TestResolver(t *testing.T) { resolutioncommon.LabelKeyResolverType: "demo", }, }, - Spec: v1beta1.ResolutionRequestSpec{}, + Spec: v1beta1.ResolutionRequestSpec{ + URL: "demoscheme://foo/bar", + }, } d := test.Data{ ResolutionRequests: []*v1beta1.ResolutionRequest{request}, @@ -65,3 +70,138 @@ func TestResolver(t *testing.T) { frtesting.RunResolverReconcileTest(ctx, t, d, r, request, expectedStatus, expectedErr) } + +func TestResolver_Failure_Wrong_Scheme(t *testing.T) { + ctx, _ := ttesting.SetupFakeContext(t) + + r := &resolver{} + + request := &v1beta1.ResolutionRequest{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "resolution.tekton.dev/v1beta1", + Kind: "ResolutionRequest", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "rr", + Namespace: "foo", + CreationTimestamp: metav1.Time{Time: time.Now()}, + Labels: map[string]string{ + resolutioncommon.LabelKeyResolverType: "demo", + }, + }, + Spec: v1beta1.ResolutionRequestSpec{ + URL: "wrongscheme://foo/bar", + }, + } + d := test.Data{ + ResolutionRequests: []*v1beta1.ResolutionRequest{request}, + } + + expectedStatus := &v1beta1.ResolutionRequestStatus{ + Status: v1.Status{ + Conditions: v1.Conditions{ + { + Type: "Succeeded", + Status: "False", + Reason: "ResolutionFailed", + Message: `invalid resource request "foo/rr": Invalid Scheme. Want demoscheme, Got wrongscheme`, + }, + }, + }, + } + + // If you want to test scenarios where an error should occur, pass a non-nil error to RunResolverReconcileTest + expectedErr := errors.New(`invalid resource request "foo/rr": Invalid Scheme. Want demoscheme, Got wrongscheme`) + frtesting.RunResolverReconcileTest(ctx, t, d, r, request, expectedStatus, expectedErr) +} + +func TestResolver_Failure_InvalidUrl(t *testing.T) { + ctx, _ := ttesting.SetupFakeContext(t) + + r := &resolver{} + + request := &v1beta1.ResolutionRequest{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "resolution.tekton.dev/v1beta1", + Kind: "ResolutionRequest", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "rr", + Namespace: "foo", + CreationTimestamp: metav1.Time{Time: time.Now()}, + Labels: map[string]string{ + resolutioncommon.LabelKeyResolverType: "demo", + }, + }, + Spec: v1beta1.ResolutionRequestSpec{ + URL: "foo/bar", + }, + } + d := test.Data{ + ResolutionRequests: []*v1beta1.ResolutionRequest{request}, + } + + expectedStatus := &v1beta1.ResolutionRequestStatus{ + Status: v1.Status{ + Conditions: v1.Conditions{ + { + Type: "Succeeded", + Status: "False", + Reason: "ResolutionFailed", + Message: `invalid resource request "foo/rr": parse "foo/bar": invalid URI for request`, + }, + }, + }, + } + + // If you want to test scenarios where an error should occur, pass a non-nil error to RunResolverReconcileTest + expectedErr := errors.New(`invalid resource request "foo/rr": parse "foo/bar": invalid URI for request`) + frtesting.RunResolverReconcileTest(ctx, t, d, r, request, expectedStatus, expectedErr) +} + +func TestResolver_Failure_InvalidParams(t *testing.T) { + ctx, _ := ttesting.SetupFakeContext(t) + + r := &resolver{} + + request := &v1beta1.ResolutionRequest{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "resolution.tekton.dev/v1beta1", + Kind: "ResolutionRequest", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "rr", + Namespace: "foo", + CreationTimestamp: metav1.Time{Time: time.Now()}, + Labels: map[string]string{ + resolutioncommon.LabelKeyResolverType: "demo", + }, + }, + Spec: v1beta1.ResolutionRequestSpec{ + Params: []pipelinev1.Param{{ + Name: "foo", + Value: *pipelinev1.NewStructuredValues("bar"), + }}, + }, + } + d := test.Data{ + ResolutionRequests: []*v1beta1.ResolutionRequest{request}, + } + + expectedStatus := &v1beta1.ResolutionRequestStatus{ + Status: v1.Status{ + Conditions: v1.Conditions{ + { + Type: "Succeeded", + Status: "False", + Reason: "ResolutionFailed", + Message: `invalid resource request "foo/rr": no params allowed`, + }, + }, + }, + } + + // If you want to test scenarios where an error should occur, pass a non-nil error to RunResolverReconcileTest + expectedErr := errors.New(`invalid resource request "foo/rr": no params allowed`) + frtesting.RunResolverReconcileTest(ctx, t, d, r, request, expectedStatus, expectedErr) +} diff --git a/pkg/apis/config/feature_flags.go b/pkg/apis/config/feature_flags.go index 66fdeb5c7ad..5108c43caa3 100644 --- a/pkg/apis/config/feature_flags.go +++ b/pkg/apis/config/feature_flags.go @@ -102,12 +102,12 @@ const ( EnableCELInWhenExpression = "enable-cel-in-whenexpression" // EnableStepActions is the flag to enable the use of StepActions in Steps EnableStepActions = "enable-step-actions" - // EnableArtifacts is the flag to enable the use of Artifacts in Steps EnableArtifacts = "enable-artifacts" - // EnableParamEnum is the flag to enabled enum in params EnableParamEnum = "enable-param-enum" + // EnableConciseResolverSyntax is the flag to enable concise resolver syntax + EnableConciseResolverSyntax = "enable-concise-resolver-syntax" // DisableInlineSpec is the flag to disable embedded spec // in Taskrun or Pipelinerun @@ -168,6 +168,13 @@ var ( Stability: AlphaAPIFields, Enabled: DefaultAlphaFeatureEnabled, } + + // DefaultEnableConciseResolverSyntax is the default PerFeatureFlag value for EnableConciseResolverSyntax + DefaultEnableConciseResolverSyntax = PerFeatureFlag{ + Name: EnableConciseResolverSyntax, + Stability: AlphaAPIFields, + Enabled: DefaultAlphaFeatureEnabled, + } ) // FeatureFlags holds the features configurations @@ -189,17 +196,18 @@ type FeatureFlags struct { // ignore: skip trusted resources verification when no matching verification policies found // warn: skip trusted resources verification when no matching verification policies found and log a warning // fail: fail the taskrun or pipelines run if no matching verification policies found - VerificationNoMatchPolicy string - EnableProvenanceInStatus bool - ResultExtractionMethod string - MaxResultSize int - SetSecurityContext bool - Coschedule string - EnableCELInWhenExpression bool - EnableStepActions bool - EnableParamEnum bool - EnableArtifacts bool - DisableInlineSpec string + VerificationNoMatchPolicy string + EnableProvenanceInStatus bool + ResultExtractionMethod string + MaxResultSize int + SetSecurityContext bool + Coschedule string + EnableCELInWhenExpression bool + EnableStepActions bool + EnableParamEnum bool + EnableArtifacts bool + DisableInlineSpec string + EnableConciseResolverSyntax bool } // GetFeatureFlagsConfigName returns the name of the configmap containing all @@ -294,14 +302,15 @@ func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) { if err := setPerFeatureFlag(EnableParamEnum, DefaultEnableParamEnum, &tc.EnableParamEnum); err != nil { return nil, err } - if err := setPerFeatureFlag(EnableArtifacts, DefaultEnableArtifacts, &tc.EnableArtifacts); err != nil { return nil, err } if err := setFeatureInlineSpec(cfgMap, DisableInlineSpec, DefaultDisableInlineSpec, &tc.DisableInlineSpec); err != nil { return nil, err } - + if err := setPerFeatureFlag(EnableConciseResolverSyntax, DefaultEnableConciseResolverSyntax, &tc.EnableConciseResolverSyntax); err != nil { + return nil, err + } // Given that they are alpha features, Tekton Bundles and Custom Tasks should be switched on if // enable-api-fields is "alpha". If enable-api-fields is not "alpha" then fall back to the value of // each feature's individual flag. diff --git a/pkg/apis/config/feature_flags_test.go b/pkg/apis/config/feature_flags_test.go index cded62e7d44..08d33e12caa 100644 --- a/pkg/apis/config/feature_flags_test.go +++ b/pkg/apis/config/feature_flags_test.go @@ -58,6 +58,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { EnableStepActions: config.DefaultEnableStepActions.Enabled, EnableParamEnum: config.DefaultEnableParamEnum.Enabled, DisableInlineSpec: config.DefaultDisableInlineSpec, + EnableConciseResolverSyntax: config.DefaultEnableConciseResolverSyntax.Enabled, }, fileName: config.GetFeatureFlagsConfigName(), }, @@ -83,6 +84,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { EnableArtifacts: true, EnableParamEnum: true, DisableInlineSpec: "pipeline,pipelinerun,taskrun", + EnableConciseResolverSyntax: true, }, fileName: "feature-flags-all-flags-set", }, @@ -316,6 +318,9 @@ func TestNewFeatureFlagsConfigMapErrors(t *testing.T) { }, { fileName: "feature-flags-invalid-enable-artifacts", want: `failed parsing feature flags config "invalid": strconv.ParseBool: parsing "invalid": invalid syntax for feature enable-artifacts`, + }, { + fileName: "feature-flags-invalid-enable-concise-resolver-syntax", + want: `failed parsing feature flags config "invalid": strconv.ParseBool: parsing "invalid": invalid syntax for feature enable-concise-resolver-syntax`, }} { t.Run(tc.fileName, func(t *testing.T) { cm := test.ConfigMapFromTestFile(t, tc.fileName) diff --git a/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml b/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml index 6b539bc16da..3a798646664 100644 --- a/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml +++ b/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml @@ -37,3 +37,4 @@ data: enable-param-enum: "true" enable-artifacts: "true" disable-inline-spec: "pipeline,pipelinerun,taskrun" + enable-concise-resolver-syntax: "true" diff --git a/pkg/apis/config/testdata/feature-flags-invalid-enable-concise-resolver-syntax.yaml b/pkg/apis/config/testdata/feature-flags-invalid-enable-concise-resolver-syntax.yaml new file mode 100644 index 00000000000..4945e2f6f76 --- /dev/null +++ b/pkg/apis/config/testdata/feature-flags-invalid-enable-concise-resolver-syntax.yaml @@ -0,0 +1,21 @@ +# Copyright 2024 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: feature-flags + namespace: tekton-pipelines +data: + enable-concise-resolver-syntax: "invalid" diff --git a/pkg/apis/pipeline/v1/container_validation.go b/pkg/apis/pipeline/v1/container_validation.go index bfee6884b0b..a145da01d9a 100644 --- a/pkg/apis/pipeline/v1/container_validation.go +++ b/pkg/apis/pipeline/v1/container_validation.go @@ -18,45 +18,79 @@ package v1 import ( "context" + "fmt" "strings" + "net/url" + "github.com/tektoncd/pipeline/pkg/apis/config" "k8s.io/apimachinery/pkg/util/validation" "knative.dev/pkg/apis" ) -// Validate ensures that a supplied Ref field is populated -// correctly. No errors are returned for a nil Ref. -func (ref *Ref) Validate(ctx context.Context) (errs *apis.FieldError) { - if ref == nil { - return errs - } - +func validateRef(ctx context.Context, refName string, refResolver ResolverName, refParams Params) (errs *apis.FieldError) { switch { - case ref.Resolver != "" || ref.Params != nil: - if ref.Resolver != "" { - errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver")) - if ref.Name != "" { - errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver")) - } - } - if ref.Params != nil { + case refResolver != "" || refParams != nil: + if refParams != nil { errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver params", config.BetaAPIFields).ViaField("params")) - if ref.Name != "" { + if refName != "" { errs = errs.Also(apis.ErrMultipleOneOf("name", "params")) } - if ref.Resolver == "" { + if refResolver == "" { errs = errs.Also(apis.ErrMissingField("resolver")) } - errs = errs.Also(ValidateParameters(ctx, ref.Params)) + errs = errs.Also(ValidateParameters(ctx, refParams)) + } + if refResolver != "" { + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver")) + if refName != "" { + // make sure that the name is url-like. + err := RefNameLikeUrl(refName) + if err == nil && !config.FromContextOrDefaults(ctx).FeatureFlags.EnableConciseResolverSyntax { + // If name is url-like then concise resolver syntax must be enabled + errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("feature flag %s should be set to true to use concise resolver syntax", config.EnableConciseResolverSyntax), "")) + } + if err != nil { + errs = errs.Also(apis.ErrInvalidValue(err, "name")) + } + } } - case ref.Name != "": - // ref name must be a valid k8s name - if errSlice := validation.IsQualifiedName(ref.Name); len(errSlice) != 0 { - errs = errs.Also(apis.ErrInvalidValue(strings.Join(errSlice, ","), "name")) + case refName != "": + // ref name can be a Url-like format. + if err := RefNameLikeUrl(refName); err == nil { + // If name is url-like then concise resolver syntax must be enabled + if !config.FromContextOrDefaults(ctx).FeatureFlags.EnableConciseResolverSyntax { + errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("feature flag %s should be set to true to use concise resolver syntax", config.EnableConciseResolverSyntax), "")) + } + // In stage1 of concise remote resolvers syntax, this is a required field. + // TODO: remove this check when implementing stage 2 where this is optional. + if refResolver == "" { + errs = errs.Also(apis.ErrMissingField("resolver")) + } + // Or, it must be a valid k8s name + } else { + // ref name must be a valid k8s name + if errSlice := validation.IsQualifiedName(refName); len(errSlice) != 0 { + errs = errs.Also(apis.ErrInvalidValue(strings.Join(errSlice, ","), "name")) + } } default: errs = errs.Also(apis.ErrMissingField("name")) } return errs } + +// Validate ensures that a supplied Ref field is populated +// correctly. No errors are returned for a nil Ref. +func (ref *Ref) Validate(ctx context.Context) (errs *apis.FieldError) { + if ref == nil { + return errs + } + return validateRef(ctx, ref.Name, ref.Resolver, ref.Params) +} + +// RefNameLikeUrl checks if the name is url parsable and returns an error if it isn't. +func RefNameLikeUrl(name string) error { + _, err := url.ParseRequestURI(name) + return err +} diff --git a/pkg/apis/pipeline/v1/container_validation_test.go b/pkg/apis/pipeline/v1/container_validation_test.go index 60c95f88120..7bdd0e36bfa 100644 --- a/pkg/apis/pipeline/v1/container_validation_test.go +++ b/pkg/apis/pipeline/v1/container_validation_test.go @@ -21,12 +21,22 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/tektoncd/pipeline/pkg/apis/config" cfgtesting "github.com/tektoncd/pipeline/pkg/apis/config/testing" v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "github.com/tektoncd/pipeline/test/diff" "knative.dev/pkg/apis" ) +func enableConciseResolverSyntax(ctx context.Context) context.Context { + return config.ToContext(ctx, &config.Config{ + FeatureFlags: &config.FeatureFlags{ + EnableConciseResolverSyntax: true, + EnableAPIFields: config.BetaAPIFields, + }, + }) +} + func TestRef_Valid(t *testing.T) { tests := []struct { name string @@ -93,29 +103,45 @@ func TestRef_Invalid(t *testing.T) { }, wantErr: apis.ErrMissingField("resolver"), }, { - name: "ref resolver disallowed in conjunction with ref name", + name: "ref with resolver and k8s style name", ref: &v1.Ref{ Name: "foo", ResolverRef: v1.ResolverRef{ Resolver: "git", }, }, - wantErr: apis.ErrMultipleOneOf("name", "resolver"), + wantErr: apis.ErrInvalidValue(`parse "foo": invalid URI for request`, "name"), + wc: enableConciseResolverSyntax, + }, { + name: "ref with url-like name without resolver", + ref: &v1.Ref{ + Name: "https://foo.com/bar", + }, + wantErr: apis.ErrMissingField("resolver"), + wc: enableConciseResolverSyntax, }, { - name: "ref params disallowed in conjunction with ref name", + name: "ref params disallowed in conjunction with pipelineref name", ref: &v1.Ref{ - Name: "bar", + Name: "https://foo/bar", ResolverRef: v1.ResolverRef{ - Params: v1.Params{{ - Name: "foo", - Value: v1.ParamValue{ - Type: v1.ParamTypeString, - StringVal: "bar", - }, - }}, + Resolver: "git", + Params: v1.Params{{Name: "foo", Value: v1.ParamValue{StringVal: "bar"}}}, }, }, - wantErr: apis.ErrMultipleOneOf("name", "params").Also(apis.ErrMissingField("resolver")), + wantErr: apis.ErrMultipleOneOf("name", "params"), + wc: enableConciseResolverSyntax, + }, { + name: "ref with url-like name without enable-concise-resolver-syntax", + ref: &v1.Ref{Name: "https://foo.com/bar"}, + wantErr: apis.ErrMissingField("resolver").Also(&apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + }), + }, { + name: "ref without enable-concise-resolver-syntax", + ref: &v1.Ref{Name: "https://foo.com/bar", ResolverRef: v1.ResolverRef{Resolver: "git"}}, + wantErr: &apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + }, }, { name: "invalid ref name", ref: &v1.Ref{Name: "_foo"}, diff --git a/pkg/apis/pipeline/v1/pipeline_types_test.go b/pkg/apis/pipeline/v1/pipeline_types_test.go index 61771b90f2a..12eb23477de 100644 --- a/pkg/apis/pipeline/v1/pipeline_types_test.go +++ b/pkg/apis/pipeline/v1/pipeline_types_test.go @@ -581,6 +581,7 @@ func TestPipelineTask_ValidateRegularTask_Failure(t *testing.T) { name string task PipelineTask expectedError apis.FieldError + configMap map[string]string }{{ name: "pipeline task - invalid taskSpec", task: PipelineTask{ @@ -612,15 +613,58 @@ func TestPipelineTask_ValidateRegularTask_Failure(t *testing.T) { Paths: []string{"taskRef.name"}, }, }, { - name: "pipeline task - taskRef with resolver and name", + name: "pipeline task - taskRef with resolver and k8s style name", task: PipelineTask{ Name: "foo", TaskRef: &TaskRef{Name: "foo", ResolverRef: ResolverRef{Resolver: "git"}}, }, + expectedError: apis.FieldError{ + Message: `invalid value: parse "foo": invalid URI for request`, + Paths: []string{"taskRef.name"}, + }, + configMap: map[string]string{"enable-concise-resolver-syntax": "true"}, + }, { + name: "pipeline task - taskRef with url-like name without enable-concise-resolver-syntax", + task: PipelineTask{ + Name: "foo", + TaskRef: &TaskRef{Name: "https://foo.com/bar"}, + }, + expectedError: *apis.ErrMissingField("taskRef.resolver").Also(&apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + Paths: []string{"taskRef"}, + }), + }, { + name: "pipeline task - taskRef without enable-concise-resolver-syntax", + task: PipelineTask{ + Name: "foo", + TaskRef: &TaskRef{Name: "https://foo.com/bar", ResolverRef: ResolverRef{Resolver: "git"}}, + }, + expectedError: apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + Paths: []string{"taskRef"}, + }, + }, { + name: "pipeline task - taskRef with url-like name without resolver", + task: PipelineTask{ + Name: "foo", + TaskRef: &TaskRef{Name: "https://foo.com/bar"}, + }, + expectedError: apis.FieldError{ + Message: `missing field(s)`, + Paths: []string{"taskRef.resolver"}, + }, + configMap: map[string]string{"enable-concise-resolver-syntax": "true"}, + }, { + name: "pipeline task - taskRef with name and params", + task: PipelineTask{ + Name: "foo", + TaskRef: &TaskRef{Name: "https://foo/bar", ResolverRef: ResolverRef{Resolver: "git", Params: Params{{Name: "foo", Value: ParamValue{StringVal: "bar"}}}}}, + }, expectedError: apis.FieldError{ Message: `expected exactly one, got both`, - Paths: []string{"taskRef.name", "taskRef.resolver"}, + Paths: []string{"taskRef.name", "taskRef.params"}, }, + configMap: map[string]string{"enable-concise-resolver-syntax": "true"}, }, { name: "pipeline task - taskRef with resolver params but no resolver", task: PipelineTask{ @@ -634,7 +678,8 @@ func TestPipelineTask_ValidateRegularTask_Failure(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := tt.task.validateTask(context.Background()) + ctx := cfgtesting.SetFeatureFlags(context.Background(), t, tt.configMap) + err := tt.task.validateTask(ctx) if err == nil { t.Error("PipelineTask.validateTask() did not return error for invalid pipeline task") } diff --git a/pkg/apis/pipeline/v1/pipelineref_validation.go b/pkg/apis/pipeline/v1/pipelineref_validation.go index 9fa7c9894d0..c23db32a50a 100644 --- a/pkg/apis/pipeline/v1/pipelineref_validation.go +++ b/pkg/apis/pipeline/v1/pipelineref_validation.go @@ -19,7 +19,6 @@ package v1 import ( "context" - "github.com/tektoncd/pipeline/pkg/apis/config" "knative.dev/pkg/apis" ) @@ -27,28 +26,7 @@ import ( // correctly. No errors are returned for a nil PipelineRef. func (ref *PipelineRef) Validate(ctx context.Context) (errs *apis.FieldError) { if ref == nil { - return + return errs } - - if ref.Resolver != "" || ref.Params != nil { - if ref.Resolver != "" { - errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver")) - if ref.Name != "" { - errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver")) - } - } - if ref.Params != nil { - errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver params", config.BetaAPIFields).ViaField("params")) - if ref.Name != "" { - errs = errs.Also(apis.ErrMultipleOneOf("name", "params")) - } - if ref.Resolver == "" { - errs = errs.Also(apis.ErrMissingField("resolver")) - } - errs = errs.Also(ValidateParameters(ctx, ref.Params)) - } - } else if ref.Name == "" { - errs = errs.Also(apis.ErrMissingField("name")) - } - return + return validateRef(ctx, ref.Name, ref.Resolver, ref.Params) } diff --git a/pkg/apis/pipeline/v1/pipelineref_validation_test.go b/pkg/apis/pipeline/v1/pipelineref_validation_test.go index 114eb6a0f10..62baa5df3ff 100644 --- a/pkg/apis/pipeline/v1/pipelineref_validation_test.go +++ b/pkg/apis/pipeline/v1/pipelineref_validation_test.go @@ -37,6 +37,13 @@ func TestPipelineRef_Invalid(t *testing.T) { name: "pipelineRef without Pipeline Name", ref: &v1.PipelineRef{}, wantErr: apis.ErrMissingField("name"), + }, { + name: "invalid pipelineref name", + ref: &v1.PipelineRef{Name: "_foo"}, + wantErr: &apis.FieldError{ + Message: `invalid value: name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`, + Paths: []string{"name"}, + }, }, { name: "pipelineref resolver disallowed without beta feature gate", ref: &v1.PipelineRef{ @@ -65,31 +72,45 @@ func TestPipelineRef_Invalid(t *testing.T) { wantErr: apis.ErrMissingField("resolver"), withContext: cfgtesting.EnableBetaAPIFields, }, { - name: "pipelineref resolver disallowed in conjunction with pipelineref name", + name: "pipelineRef with resolver and k8s style name", ref: &v1.PipelineRef{ Name: "foo", ResolverRef: v1.ResolverRef{ - Resolver: "bar", + Resolver: "git", }, }, - wantErr: apis.ErrMultipleOneOf("name", "resolver"), - withContext: cfgtesting.EnableBetaAPIFields, + wantErr: apis.ErrInvalidValue(`parse "foo": invalid URI for request`, "name"), + withContext: enableConciseResolverSyntax, }, { - name: "pipelineref params disallowed in conjunction with pipelineref name", + name: "pipelineRef with url-like name without resolver", ref: &v1.PipelineRef{ - Name: "bar", + Name: "https://foo.com/bar", + }, + wantErr: apis.ErrMissingField("resolver"), + withContext: enableConciseResolverSyntax, + }, { + name: "pipelineRef params disallowed in conjunction with pipelineref name", + ref: &v1.PipelineRef{ + Name: "https://foo/bar", ResolverRef: v1.ResolverRef{ - Params: v1.Params{{ - Name: "foo", - Value: v1.ParamValue{ - Type: v1.ParamTypeString, - StringVal: "bar", - }, - }}, + Resolver: "git", + Params: v1.Params{{Name: "foo", Value: v1.ParamValue{StringVal: "bar"}}}, }, }, - wantErr: apis.ErrMultipleOneOf("name", "params").Also(apis.ErrMissingField("resolver")), - withContext: cfgtesting.EnableBetaAPIFields, + wantErr: apis.ErrMultipleOneOf("name", "params"), + withContext: enableConciseResolverSyntax, + }, { + name: "pipelineRef with url-like name without enable-concise-resolver-syntax", + ref: &v1.PipelineRef{Name: "https://foo.com/bar"}, + wantErr: apis.ErrMissingField("resolver").Also(&apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + }), + }, { + name: "pipelineRef without enable-concise-resolver-syntax", + ref: &v1.PipelineRef{Name: "https://foo.com/bar", ResolverRef: v1.ResolverRef{Resolver: "git"}}, + wantErr: &apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + }, }} for _, tc := range tests { diff --git a/pkg/apis/pipeline/v1/taskref_validation.go b/pkg/apis/pipeline/v1/taskref_validation.go index 4f4e0330350..bbc5fbbfc48 100644 --- a/pkg/apis/pipeline/v1/taskref_validation.go +++ b/pkg/apis/pipeline/v1/taskref_validation.go @@ -18,10 +18,7 @@ package v1 import ( "context" - "strings" - "github.com/tektoncd/pipeline/pkg/apis/config" - "k8s.io/apimachinery/pkg/util/validation" "knative.dev/pkg/apis" ) @@ -31,32 +28,5 @@ func (ref *TaskRef) Validate(ctx context.Context) (errs *apis.FieldError) { if ref == nil { return errs } - - switch { - case ref.Resolver != "" || ref.Params != nil: - if ref.Resolver != "" { - errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver")) - if ref.Name != "" { - errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver")) - } - } - if ref.Params != nil { - errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver params", config.BetaAPIFields).ViaField("params")) - if ref.Name != "" { - errs = errs.Also(apis.ErrMultipleOneOf("name", "params")) - } - if ref.Resolver == "" { - errs = errs.Also(apis.ErrMissingField("resolver")) - } - errs = errs.Also(ValidateParameters(ctx, ref.Params)) - } - case ref.Name != "": - // TaskRef name must be a valid k8s name - if errSlice := validation.IsQualifiedName(ref.Name); len(errSlice) != 0 { - errs = errs.Also(apis.ErrInvalidValue(strings.Join(errSlice, ","), "name")) - } - default: - errs = errs.Also(apis.ErrMissingField("name")) - } - return errs + return validateRef(ctx, ref.Name, ref.Resolver, ref.Params) } diff --git a/pkg/apis/pipeline/v1/taskref_validation_test.go b/pkg/apis/pipeline/v1/taskref_validation_test.go index d2c721bc6a1..8640045e566 100644 --- a/pkg/apis/pipeline/v1/taskref_validation_test.go +++ b/pkg/apis/pipeline/v1/taskref_validation_test.go @@ -120,31 +120,45 @@ func TestTaskRef_Invalid(t *testing.T) { wantErr: apis.ErrMissingField("resolver"), wc: cfgtesting.EnableBetaAPIFields, }, { - name: "taskref resolver disallowed in conjunction with taskref name", + name: "taskRef with resolver and k8s style name", taskRef: &v1.TaskRef{ Name: "foo", ResolverRef: v1.ResolverRef{ Resolver: "git", }, }, - wantErr: apis.ErrMultipleOneOf("name", "resolver"), - wc: cfgtesting.EnableBetaAPIFields, + wantErr: apis.ErrInvalidValue(`parse "foo": invalid URI for request`, "name"), + wc: enableConciseResolverSyntax, + }, { + name: "taskRef with url-like name without resolver", + taskRef: &v1.TaskRef{ + Name: "https://foo.com/bar", + }, + wantErr: apis.ErrMissingField("resolver"), + wc: enableConciseResolverSyntax, }, { - name: "taskref params disallowed in conjunction with taskref name", + name: "taskRef params disallowed in conjunction with pipelineref name", taskRef: &v1.TaskRef{ - Name: "bar", + Name: "https://foo/bar", ResolverRef: v1.ResolverRef{ - Params: v1.Params{{ - Name: "foo", - Value: v1.ParamValue{ - Type: v1.ParamTypeString, - StringVal: "bar", - }, - }}, + Resolver: "git", + Params: v1.Params{{Name: "foo", Value: v1.ParamValue{StringVal: "bar"}}}, }, }, - wantErr: apis.ErrMultipleOneOf("name", "params").Also(apis.ErrMissingField("resolver")), - wc: cfgtesting.EnableBetaAPIFields, + wantErr: apis.ErrMultipleOneOf("name", "params"), + wc: enableConciseResolverSyntax, + }, { + name: "taskRef with url-like name without enable-concise-resolver-syntax", + taskRef: &v1.TaskRef{Name: "https://foo.com/bar"}, + wantErr: apis.ErrMissingField("resolver").Also(&apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + }), + }, { + name: "taskRef without enable-concise-resolver-syntax", + taskRef: &v1.TaskRef{Name: "https://foo.com/bar", ResolverRef: v1.ResolverRef{Resolver: "git"}}, + wantErr: &apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + }, }} for _, ts := range tests { t.Run(ts.name, func(t *testing.T) { diff --git a/pkg/apis/pipeline/v1beta1/container_validation.go b/pkg/apis/pipeline/v1beta1/container_validation.go index bab6f8bc4d5..b9f66375d10 100644 --- a/pkg/apis/pipeline/v1beta1/container_validation.go +++ b/pkg/apis/pipeline/v1beta1/container_validation.go @@ -18,45 +18,79 @@ package v1beta1 import ( "context" + "fmt" "strings" + "net/url" + "github.com/tektoncd/pipeline/pkg/apis/config" "k8s.io/apimachinery/pkg/util/validation" "knative.dev/pkg/apis" ) -// Validate ensures that a supplied Ref field is populated -// correctly. No errors are returned for a nil Ref. -func (ref *Ref) Validate(ctx context.Context) (errs *apis.FieldError) { - if ref == nil { - return errs - } - +func validateRef(ctx context.Context, refName string, refResolver ResolverName, refParams Params) (errs *apis.FieldError) { switch { - case ref.Resolver != "" || ref.Params != nil: - if ref.Resolver != "" { - errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver")) - if ref.Name != "" { - errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver")) - } - } - if ref.Params != nil { + case refResolver != "" || refParams != nil: + if refParams != nil { errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver params", config.BetaAPIFields).ViaField("params")) - if ref.Name != "" { + if refName != "" { errs = errs.Also(apis.ErrMultipleOneOf("name", "params")) } - if ref.Resolver == "" { + if refResolver == "" { errs = errs.Also(apis.ErrMissingField("resolver")) } - errs = errs.Also(ValidateParameters(ctx, ref.Params)) + errs = errs.Also(ValidateParameters(ctx, refParams)) + } + if refResolver != "" { + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver")) + if refName != "" { + // make sure that the name is url-like. + err := RefNameLikeUrl(refName) + if err == nil && !config.FromContextOrDefaults(ctx).FeatureFlags.EnableConciseResolverSyntax { + // If name is url-like then concise resolver syntax must be enabled + errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("feature flag %s should be set to true to use concise resolver syntax", config.EnableConciseResolverSyntax), "")) + } + if err != nil { + errs = errs.Also(apis.ErrInvalidValue(err, "name")) + } + } } - case ref.Name != "": - // Ref name must be a valid k8s name - if errSlice := validation.IsQualifiedName(ref.Name); len(errSlice) != 0 { - errs = errs.Also(apis.ErrInvalidValue(strings.Join(errSlice, ","), "name")) + case refName != "": + // ref name can be a Url-like format. + if err := RefNameLikeUrl(refName); err == nil { + // If name is url-like then concise resolver syntax must be enabled + if !config.FromContextOrDefaults(ctx).FeatureFlags.EnableConciseResolverSyntax { + errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("feature flag %s should be set to true to use concise resolver syntax", config.EnableConciseResolverSyntax), "")) + } + // In stage1 of concise remote resolvers syntax, this is a required field. + // TODO: remove this check when implementing stage 2 where this is optional. + if refResolver == "" { + errs = errs.Also(apis.ErrMissingField("resolver")) + } + // Or, it must be a valid k8s name + } else { + // ref name must be a valid k8s name + if errSlice := validation.IsQualifiedName(refName); len(errSlice) != 0 { + errs = errs.Also(apis.ErrInvalidValue(strings.Join(errSlice, ","), "name")) + } } default: errs = errs.Also(apis.ErrMissingField("name")) } return errs } + +// Validate ensures that a supplied Ref field is populated +// correctly. No errors are returned for a nil Ref. +func (ref *Ref) Validate(ctx context.Context) (errs *apis.FieldError) { + if ref == nil { + return errs + } + return validateRef(ctx, ref.Name, ref.Resolver, ref.Params) +} + +// RefNameLikeUrl checks if the name is url parsable and returns an error if it isn't. +func RefNameLikeUrl(name string) error { + _, err := url.ParseRequestURI(name) + return err +} diff --git a/pkg/apis/pipeline/v1beta1/container_validation_test.go b/pkg/apis/pipeline/v1beta1/container_validation_test.go index 95e7e4a28c0..79b23fa6f25 100644 --- a/pkg/apis/pipeline/v1beta1/container_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/container_validation_test.go @@ -21,12 +21,22 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/tektoncd/pipeline/pkg/apis/config" cfgtesting "github.com/tektoncd/pipeline/pkg/apis/config/testing" - "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + v1beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/test/diff" "knative.dev/pkg/apis" ) +func enableConciseResolverSyntax(ctx context.Context) context.Context { + return config.ToContext(ctx, &config.Config{ + FeatureFlags: &config.FeatureFlags{ + EnableConciseResolverSyntax: true, + EnableAPIFields: config.BetaAPIFields, + }, + }) +} + func TestRef_Valid(t *testing.T) { tests := []struct { name string @@ -93,29 +103,45 @@ func TestRef_Invalid(t *testing.T) { }, wantErr: apis.ErrMissingField("resolver"), }, { - name: "ref resolver disallowed in conjunction with ref name", + name: "ref with resolver and k8s style name", ref: &v1beta1.Ref{ Name: "foo", ResolverRef: v1beta1.ResolverRef{ Resolver: "git", }, }, - wantErr: apis.ErrMultipleOneOf("name", "resolver"), + wantErr: apis.ErrInvalidValue(`parse "foo": invalid URI for request`, "name"), + wc: enableConciseResolverSyntax, + }, { + name: "ref with url-like name without resolver", + ref: &v1beta1.Ref{ + Name: "https://foo.com/bar", + }, + wantErr: apis.ErrMissingField("resolver"), + wc: enableConciseResolverSyntax, }, { - name: "ref params disallowed in conjunction with ref name", + name: "ref params disallowed in conjunction with pipelineref name", ref: &v1beta1.Ref{ - Name: "bar", + Name: "https://foo/bar", ResolverRef: v1beta1.ResolverRef{ - Params: v1beta1.Params{{ - Name: "foo", - Value: v1beta1.ParamValue{ - Type: v1beta1.ParamTypeString, - StringVal: "bar", - }, - }}, + Resolver: "git", + Params: v1beta1.Params{{Name: "foo", Value: v1beta1.ParamValue{StringVal: "bar"}}}, }, }, - wantErr: apis.ErrMultipleOneOf("name", "params").Also(apis.ErrMissingField("resolver")), + wantErr: apis.ErrMultipleOneOf("name", "params"), + wc: enableConciseResolverSyntax, + }, { + name: "ref with url-like name without enable-concise-resolver-syntax", + ref: &v1beta1.Ref{Name: "https://foo.com/bar"}, + wantErr: apis.ErrMissingField("resolver").Also(&apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + }), + }, { + name: "ref without enable-concise-resolver-syntax", + ref: &v1beta1.Ref{Name: "https://foo.com/bar", ResolverRef: v1beta1.ResolverRef{Resolver: "git"}}, + wantErr: &apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + }, }, { name: "invalid ref name", ref: &v1beta1.Ref{Name: "_foo"}, diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go index d938565b86d..f12dd96c516 100644 --- a/pkg/apis/pipeline/v1beta1/openapi_generated.go +++ b/pkg/apis/pipeline/v1beta1/openapi_generated.go @@ -6297,6 +6297,13 @@ func schema_pkg_apis_resolution_v1beta1_ResolutionRequestSpec(ref common.Referen }, }, }, + "url": { + SchemaProps: spec.SchemaProps{ + Description: "URL is the runtime url passed to the resolver to help it figure out how to resolver the resource being requested. This is currently at an ALPHA stability level and subject to alpha API compatibility policies.", + Type: []string{"string"}, + Format: "", + }, + }, }, }, }, diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go index d0dc19b4cdf..28fdcf2382d 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go @@ -617,6 +617,7 @@ func TestPipelineTask_ValidateRegularTask_Failure(t *testing.T) { name string task PipelineTask expectedError apis.FieldError + configMap map[string]string }{{ name: "pipeline task - invalid taskSpec", task: PipelineTask{ @@ -655,15 +656,58 @@ func TestPipelineTask_ValidateRegularTask_Failure(t *testing.T) { }, expectedError: *apis.ErrGeneric("bundle requires \"enable-tekton-oci-bundles\" feature gate to be true but it is false"), }, { - name: "pipeline task - taskRef with resolver and name", + name: "pipeline task - taskRef with resolver and k8s style name", task: PipelineTask{ Name: "foo", TaskRef: &TaskRef{Name: "foo", ResolverRef: ResolverRef{Resolver: "git"}}, }, + expectedError: apis.FieldError{ + Message: `invalid value: parse "foo": invalid URI for request`, + Paths: []string{"taskRef.name"}, + }, + configMap: map[string]string{"enable-concise-resolver-syntax": "true"}, + }, { + name: "pipeline task - taskRef with url-like name without enable-concise-resolver-syntax", + task: PipelineTask{ + Name: "foo", + TaskRef: &TaskRef{Name: "https://foo.com/bar"}, + }, + expectedError: *apis.ErrMissingField("taskRef.resolver").Also(&apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + Paths: []string{"taskRef"}, + }), + }, { + name: "pipeline task - taskRef without enable-concise-resolver-syntax", + task: PipelineTask{ + Name: "foo", + TaskRef: &TaskRef{Name: "https://foo.com/bar", ResolverRef: ResolverRef{Resolver: "git"}}, + }, + expectedError: apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + Paths: []string{"taskRef"}, + }, + }, { + name: "pipeline task - taskRef with url-like name without resolver", + task: PipelineTask{ + Name: "foo", + TaskRef: &TaskRef{Name: "https://foo.com/bar"}, + }, + expectedError: apis.FieldError{ + Message: `missing field(s)`, + Paths: []string{"taskRef.resolver"}, + }, + configMap: map[string]string{"enable-concise-resolver-syntax": "true"}, + }, { + name: "pipeline task - taskRef with name and params", + task: PipelineTask{ + Name: "foo", + TaskRef: &TaskRef{Name: "https://foo/bar", ResolverRef: ResolverRef{Resolver: "git", Params: Params{{Name: "foo", Value: ParamValue{StringVal: "bar"}}}}}, + }, expectedError: apis.FieldError{ Message: `expected exactly one, got both`, - Paths: []string{"taskRef.name", "taskRef.resolver"}, + Paths: []string{"taskRef.name", "taskRef.params"}, }, + configMap: map[string]string{"enable-concise-resolver-syntax": "true"}, }, { name: "pipeline task - taskRef with resolver params but no resolver", task: PipelineTask{ @@ -677,7 +721,8 @@ func TestPipelineTask_ValidateRegularTask_Failure(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := tt.task.validateTask(context.Background()) + ctx := cfgtesting.SetFeatureFlags(context.Background(), t, tt.configMap) + err := tt.task.validateTask(ctx) if err == nil { t.Error("PipelineTask.validateTask() did not return error for invalid pipeline task") } diff --git a/pkg/apis/pipeline/v1beta1/pipelineref_validation.go b/pkg/apis/pipeline/v1beta1/pipelineref_validation.go index a0b7e02f18b..b5780112560 100644 --- a/pkg/apis/pipeline/v1beta1/pipelineref_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipelineref_validation.go @@ -19,9 +19,11 @@ package v1beta1 import ( "context" "fmt" + "strings" "github.com/google/go-containerregistry/pkg/name" "github.com/tektoncd/pipeline/pkg/apis/config" + "k8s.io/apimachinery/pkg/util/validation" "knative.dev/pkg/apis" ) @@ -31,17 +33,8 @@ func (ref *PipelineRef) Validate(ctx context.Context) (errs *apis.FieldError) { if ref == nil { return errs } - - if ref.Resolver != "" || ref.Params != nil { - if ref.Resolver != "" { - errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver")) - if ref.Name != "" { - errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver")) - } - if ref.Bundle != "" { - errs = errs.Also(apis.ErrMultipleOneOf("bundle", "resolver")) - } - } + switch { + case ref.Resolver != "" || ref.Params != nil: if ref.Params != nil { errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver params", config.BetaAPIFields).ViaField("params")) if ref.Name != "" { @@ -55,16 +48,52 @@ func (ref *PipelineRef) Validate(ctx context.Context) (errs *apis.FieldError) { } errs = errs.Also(ValidateParameters(ctx, ref.Params)) } - } else { + if ref.Resolver != "" { + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver")) + if ref.Name != "" { + // make sure that the name is url-like. + err := RefNameLikeUrl(ref.Name) + if err == nil && !config.FromContextOrDefaults(ctx).FeatureFlags.EnableConciseResolverSyntax { + // If name is url-like then concise resolver syntax must be enabled + errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("feature flag %s should be set to true to use concise resolver syntax", config.EnableConciseResolverSyntax), "")) + } + if err != nil { + errs = errs.Also(apis.ErrInvalidValue(err, "name")) + } + } + if ref.Bundle != "" { + errs = errs.Also(apis.ErrMultipleOneOf("bundle", "resolver")) + } + } + case ref.Bundle != "": if ref.Name == "" { errs = errs.Also(apis.ErrMissingField("name")) } - if ref.Bundle != "" { - errs = errs.Also(validateBundleFeatureFlag(ctx, "bundle", true).ViaField("bundle")) - if _, err := name.ParseReference(ref.Bundle); err != nil { - errs = errs.Also(apis.ErrInvalidValue("invalid bundle reference", "bundle", err.Error())) + errs = errs.Also(validateBundleFeatureFlag(ctx, "bundle", true).ViaField("bundle")) + if _, err := name.ParseReference(ref.Bundle); err != nil { + errs = errs.Also(apis.ErrInvalidValue("invalid bundle reference", "bundle", err.Error())) + } + case ref.Name != "": + // ref name can be a Url-like format. + if err := RefNameLikeUrl(ref.Name); err == nil { + // If name is url-like then concise resolver syntax must be enabled + if !config.FromContextOrDefaults(ctx).FeatureFlags.EnableConciseResolverSyntax { + errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("feature flag %s should be set to true to use concise resolver syntax", config.EnableConciseResolverSyntax), "")) + } + // In stage1 of concise remote resolvers syntax, this is a required field. + // TODO: remove this check when implementing stage 2 where this is optional. + if ref.Resolver == "" { + errs = errs.Also(apis.ErrMissingField("resolver")) + } + // Or, it must be a valid k8s name + } else { + // ref name must be a valid k8s name + if errSlice := validation.IsQualifiedName(ref.Name); len(errSlice) != 0 { + errs = errs.Also(apis.ErrInvalidValue(strings.Join(errSlice, ","), "name")) } } + default: + errs = errs.Also(apis.ErrMissingField("name")) } return //nolint:nakedret } diff --git a/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go b/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go index 82549d20ca9..a5a00617dc1 100644 --- a/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go @@ -63,6 +63,13 @@ func TestPipelineRef_Invalid(t *testing.T) { name: "pipelineRef without Pipeline Name", ref: &v1beta1.PipelineRef{}, wantErr: apis.ErrMissingField("name"), + }, { + name: "invalid pipelineref name", + ref: &v1beta1.PipelineRef{Name: "_foo"}, + wantErr: &apis.FieldError{ + Message: `invalid value: name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`, + Paths: []string{"name"}, + }, }, { name: "pipelineref resolver disallowed without beta feature gate", ref: &v1beta1.PipelineRef{ @@ -91,41 +98,45 @@ func TestPipelineRef_Invalid(t *testing.T) { wantErr: apis.ErrMissingField("resolver"), withContext: cfgtesting.EnableBetaAPIFields, }, { - name: "pipelineref resolver disallowed in conjunction with pipelineref name", + name: "pipelineRef with resolver and k8s style name", ref: &v1beta1.PipelineRef{ Name: "foo", ResolverRef: v1beta1.ResolverRef{ - Resolver: "bar", + Resolver: "git", }, }, - wantErr: apis.ErrMultipleOneOf("name", "resolver"), - withContext: cfgtesting.EnableBetaAPIFields, + wantErr: apis.ErrInvalidValue(`parse "foo": invalid URI for request`, "name"), + withContext: enableConciseResolverSyntax, }, { - name: "pipelineref resolver disallowed in conjunction with pipelineref bundle", + name: "pipelineRef with url-like name without resolver", ref: &v1beta1.PipelineRef{ - Bundle: "foo", - ResolverRef: v1beta1.ResolverRef{ - Resolver: "baz", - }, + Name: "https://foo.com/bar", }, - wantErr: apis.ErrMultipleOneOf("bundle", "resolver"), - withContext: enableTektonOCIBundles(t), + wantErr: apis.ErrMissingField("resolver"), + withContext: enableConciseResolverSyntax, }, { - name: "pipelineref params disallowed in conjunction with pipelineref name", + name: "pipelineRef params disallowed in conjunction with pipelineref name", ref: &v1beta1.PipelineRef{ - Name: "bar", + Name: "https://foo/bar", ResolverRef: v1beta1.ResolverRef{ - Params: v1beta1.Params{{ - Name: "foo", - Value: v1beta1.ParamValue{ - Type: v1beta1.ParamTypeString, - StringVal: "bar", - }, - }}, + Resolver: "git", + Params: []v1beta1.Param{{Name: "foo", Value: v1beta1.ParamValue{StringVal: "bar"}}}, }, }, - wantErr: apis.ErrMultipleOneOf("name", "params").Also(apis.ErrMissingField("resolver")), - withContext: cfgtesting.EnableBetaAPIFields, + wantErr: apis.ErrMultipleOneOf("name", "params"), + withContext: enableConciseResolverSyntax, + }, { + name: "pipelineRef with url-like name without enable-concise-resolver-syntax", + ref: &v1beta1.PipelineRef{Name: "https://foo.com/bar"}, + wantErr: apis.ErrMissingField("resolver").Also(&apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + }), + }, { + name: "pipelineRef without enable-concise-resolver-syntax", + ref: &v1beta1.PipelineRef{Name: "https://foo.com/bar", ResolverRef: v1beta1.ResolverRef{Resolver: "git"}}, + wantErr: &apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + }, }, { name: "pipelineref params disallowed in conjunction with pipelineref bundle", ref: &v1beta1.PipelineRef{ diff --git a/pkg/apis/pipeline/v1beta1/swagger.json b/pkg/apis/pipeline/v1beta1/swagger.json index 622b1d680c3..62121c79b69 100644 --- a/pkg/apis/pipeline/v1beta1/swagger.json +++ b/pkg/apis/pipeline/v1beta1/swagger.json @@ -1722,6 +1722,10 @@ "$ref": "#/definitions/v1.Param" }, "x-kubernetes-list-type": "atomic" + }, + "url": { + "description": "URL is the runtime url passed to the resolver to help it figure out how to resolver the resource being requested. This is currently at an ALPHA stability level and subject to alpha API compatibility policies.", + "type": "string" } } }, diff --git a/pkg/apis/pipeline/v1beta1/taskref_validation.go b/pkg/apis/pipeline/v1beta1/taskref_validation.go index a3e2bb036c3..7db46c0d9c5 100644 --- a/pkg/apis/pipeline/v1beta1/taskref_validation.go +++ b/pkg/apis/pipeline/v1beta1/taskref_validation.go @@ -18,6 +18,7 @@ package v1beta1 import ( "context" + "fmt" "strings" "github.com/google/go-containerregistry/pkg/name" @@ -32,18 +33,8 @@ func (ref *TaskRef) Validate(ctx context.Context) (errs *apis.FieldError) { if ref == nil { return errs } - switch { case ref.Resolver != "" || ref.Params != nil: - if ref.Resolver != "" { - errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver")) - if ref.Name != "" { - errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver")) - } - if ref.Bundle != "" { - errs = errs.Also(apis.ErrMultipleOneOf("bundle", "resolver")) - } - } if ref.Params != nil { errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver params", config.BetaAPIFields).ViaField("params")) if ref.Name != "" { @@ -57,6 +48,23 @@ func (ref *TaskRef) Validate(ctx context.Context) (errs *apis.FieldError) { } errs = errs.Also(ValidateParameters(ctx, ref.Params)) } + if ref.Resolver != "" { + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver")) + if ref.Name != "" { + // make sure that the name is url-like. + err := RefNameLikeUrl(ref.Name) + if err == nil && !config.FromContextOrDefaults(ctx).FeatureFlags.EnableConciseResolverSyntax { + // If name is url-like then concise resolver syntax must be enabled + errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("feature flag %s should be set to true to use concise resolver syntax", config.EnableConciseResolverSyntax), "")) + } + if err != nil { + errs = errs.Also(apis.ErrInvalidValue(err, "name")) + } + } + if ref.Bundle != "" { + errs = errs.Also(apis.ErrMultipleOneOf("bundle", "resolver")) + } + } case ref.Bundle != "": if ref.Name == "" { errs = errs.Also(apis.ErrMissingField("name")) @@ -65,13 +73,27 @@ func (ref *TaskRef) Validate(ctx context.Context) (errs *apis.FieldError) { if _, err := name.ParseReference(ref.Bundle); err != nil { errs = errs.Also(apis.ErrInvalidValue("invalid bundle reference", "bundle", err.Error())) } - default: - if ref.Name == "" { - errs = errs.Also(apis.ErrMissingField("name")) - } else if errSlice := validation.IsQualifiedName(ref.Name); len(errSlice) != 0 { - // TaskRef name must be a valid k8s name - errs = errs.Also(apis.ErrInvalidValue(strings.Join(errSlice, ","), "name")) + case ref.Name != "": + // ref name can be a Url-like format. + if err := RefNameLikeUrl(ref.Name); err == nil { + // If name is url-like then concise resolver syntax must be enabled + if !config.FromContextOrDefaults(ctx).FeatureFlags.EnableConciseResolverSyntax { + errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("feature flag %s should be set to true to use concise resolver syntax", config.EnableConciseResolverSyntax), "")) + } + // In stage1 of concise remote resolvers syntax, this is a required field. + // TODO: remove this check when implementing stage 2 where this is optional. + if ref.Resolver == "" { + errs = errs.Also(apis.ErrMissingField("resolver")) + } + // Or, it must be a valid k8s name + } else { + // ref name must be a valid k8s name + if errSlice := validation.IsQualifiedName(ref.Name); len(errSlice) != 0 { + errs = errs.Also(apis.ErrInvalidValue(strings.Join(errSlice, ","), "name")) + } } + default: + errs = errs.Also(apis.ErrMissingField("name")) } - return errs + return //nolint:nakedret } diff --git a/pkg/apis/pipeline/v1beta1/taskref_validation_test.go b/pkg/apis/pipeline/v1beta1/taskref_validation_test.go index 7d1f4f488ff..09136f812c3 100644 --- a/pkg/apis/pipeline/v1beta1/taskref_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/taskref_validation_test.go @@ -113,22 +113,45 @@ func TestTaskRef_Invalid(t *testing.T) { wantErr: apis.ErrInvalidValue("invalid bundle reference", "bundle", "could not parse reference: invalid reference"), wc: enableTektonOCIBundles(t), }, { - name: "taskref params disallowed without resolver", + name: "taskRef with resolver and k8s style name", taskRef: &v1beta1.TaskRef{ + Name: "foo", ResolverRef: v1beta1.ResolverRef{ - Params: v1beta1.Params{}, + Resolver: "git", }, }, + wantErr: apis.ErrInvalidValue(`parse "foo": invalid URI for request`, "name"), + wc: enableConciseResolverSyntax, + }, { + name: "taskRef with url-like name without resolver", + taskRef: &v1beta1.TaskRef{ + Name: "https://foo.com/bar", + }, wantErr: apis.ErrMissingField("resolver"), + wc: enableConciseResolverSyntax, }, { - name: "taskref resolver disallowed in conjunction with taskref name", + name: "taskRef params disallowed in conjunction with pipelineref name", taskRef: &v1beta1.TaskRef{ - Name: "foo", + Name: "https://foo/bar", ResolverRef: v1beta1.ResolverRef{ Resolver: "git", + Params: v1beta1.Params{{Name: "foo", Value: v1beta1.ParamValue{StringVal: "bar"}}}, }, }, - wantErr: apis.ErrMultipleOneOf("name", "resolver"), + wantErr: apis.ErrMultipleOneOf("name", "params"), + wc: enableConciseResolverSyntax, + }, { + name: "taskRef with url-like name without enable-concise-resolver-syntax", + taskRef: &v1beta1.TaskRef{Name: "https://foo.com/bar"}, + wantErr: apis.ErrMissingField("resolver").Also(&apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + }), + }, { + name: "taskRef without enable-concise-resolver-syntax", + taskRef: &v1beta1.TaskRef{Name: "https://foo.com/bar", ResolverRef: v1beta1.ResolverRef{Resolver: "git"}}, + wantErr: &apis.FieldError{ + Message: `feature flag enable-concise-resolver-syntax should be set to true to use concise resolver syntax`, + }, }, { name: "taskref resolver disallowed in conjunction with taskref bundle", taskRef: &v1beta1.TaskRef{ @@ -139,21 +162,6 @@ func TestTaskRef_Invalid(t *testing.T) { }, wantErr: apis.ErrMultipleOneOf("bundle", "resolver"), wc: enableTektonOCIBundles(t), - }, { - name: "taskref params disallowed in conjunction with taskref name", - taskRef: &v1beta1.TaskRef{ - Name: "bar", - ResolverRef: v1beta1.ResolverRef{ - Params: v1beta1.Params{{ - Name: "foo", - Value: v1beta1.ParamValue{ - Type: v1beta1.ParamTypeString, - StringVal: "bar", - }, - }}, - }, - }, - wantErr: apis.ErrMultipleOneOf("name", "params").Also(apis.ErrMissingField("resolver")), }, { name: "taskref params disallowed in conjunction with taskref bundle", taskRef: &v1beta1.TaskRef{ diff --git a/pkg/apis/resolution/v1beta1/resolution_request_types.go b/pkg/apis/resolution/v1beta1/resolution_request_types.go index 60b51fa0498..f78a4a493c3 100644 --- a/pkg/apis/resolution/v1beta1/resolution_request_types.go +++ b/pkg/apis/resolution/v1beta1/resolution_request_types.go @@ -64,6 +64,13 @@ type ResolutionRequestSpec struct { // +optional // +listType=atomic Params []pipelinev1.Param `json:"params,omitempty"` + // URL is the runtime url passed to the resolver + // to help it figure out how to resolver the resource being + // requested. + // This is currently at an ALPHA stability level and subject to + // alpha API compatibility policies. + // +optional + URL string `json:"url,omitempty"` } // ResolutionRequestStatus are all the fields in a ResolutionRequest's diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index b1f9904ccf3..ee053617c36 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -16723,7 +16723,7 @@ spec: // the ResolutionRequest's name is generated by resolverName, namespace and runName. func getResolvedResolutionRequest(t *testing.T, resolverName string, resourceBytes []byte, namespace string, runName string) resolutionv1beta1.ResolutionRequest { t.Helper() - name, err := remoteresource.GenerateDeterministicName(resolverName, namespace+"/"+runName, nil) + name, err := remoteresource.GenerateDeterministicNameFromSpec(resolverName, namespace+"/"+runName, &resolutionv1beta1.ResolutionRequestSpec{}) if err != nil { t.Errorf("error generating name for %s/%s/%s: %v", resolverName, namespace, runName, err) } diff --git a/pkg/reconciler/pipelinerun/resources/apply.go b/pkg/reconciler/pipelinerun/resources/apply.go index 3aeb0accebd..b971d94d4f8 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/pkg/reconciler/pipelinerun/resources/apply.go @@ -249,8 +249,11 @@ func ApplyTaskResults(targets PipelineRunState, resolvedResultRefs ResolvedResul } } pipelineTask.When = pipelineTask.When.ReplaceVariables(stringReplacements, arrayReplacements) - if pipelineTask.TaskRef != nil && pipelineTask.TaskRef.Params != nil { - pipelineTask.TaskRef.Params = pipelineTask.TaskRef.Params.ReplaceVariables(stringReplacements, arrayReplacements, objectReplacements) + if pipelineTask.TaskRef != nil { + if pipelineTask.TaskRef.Params != nil { + pipelineTask.TaskRef.Params = pipelineTask.TaskRef.Params.ReplaceVariables(stringReplacements, arrayReplacements, objectReplacements) + } + pipelineTask.TaskRef.Name = substitution.ApplyReplacements(pipelineTask.TaskRef.Name, stringReplacements) } pipelineTask.DisplayName = substitution.ApplyReplacements(pipelineTask.DisplayName, stringReplacements) for i, workspace := range pipelineTask.Workspaces { @@ -268,8 +271,11 @@ func ApplyPipelineTaskStateContext(state PipelineRunState, replacements map[stri pipelineTask := resolvedPipelineRunTask.PipelineTask.DeepCopy() pipelineTask.Params = pipelineTask.Params.ReplaceVariables(replacements, nil, nil) pipelineTask.When = pipelineTask.When.ReplaceVariables(replacements, nil) - if pipelineTask.TaskRef != nil && pipelineTask.TaskRef.Params != nil { - pipelineTask.TaskRef.Params = pipelineTask.TaskRef.Params.ReplaceVariables(replacements, nil, nil) + if pipelineTask.TaskRef != nil { + if pipelineTask.TaskRef.Params != nil { + pipelineTask.TaskRef.Params = pipelineTask.TaskRef.Params.ReplaceVariables(replacements, nil, nil) + } + pipelineTask.TaskRef.Name = substitution.ApplyReplacements(pipelineTask.TaskRef.Name, replacements) } pipelineTask.DisplayName = substitution.ApplyReplacements(pipelineTask.DisplayName, replacements) resolvedPipelineRunTask.PipelineTask = pipelineTask @@ -311,8 +317,11 @@ func ApplyReplacements(p *v1.PipelineSpec, replacements map[string]string, array p.Tasks[i].Workspaces[j].SubPath = substitution.ApplyReplacements(p.Tasks[i].Workspaces[j].SubPath, replacements) } p.Tasks[i].When = p.Tasks[i].When.ReplaceVariables(replacements, arrayReplacements) - if p.Tasks[i].TaskRef != nil && p.Tasks[i].TaskRef.Params != nil { - p.Tasks[i].TaskRef.Params = p.Tasks[i].TaskRef.Params.ReplaceVariables(replacements, arrayReplacements, objectReplacements) + if p.Tasks[i].TaskRef != nil { + if p.Tasks[i].TaskRef.Params != nil { + p.Tasks[i].TaskRef.Params = p.Tasks[i].TaskRef.Params.ReplaceVariables(replacements, arrayReplacements, objectReplacements) + } + p.Tasks[i].TaskRef.Name = substitution.ApplyReplacements(p.Tasks[i].TaskRef.Name, replacements) } p.Tasks[i] = propagateParams(p.Tasks[i], replacements, arrayReplacements, objectReplacements) } @@ -331,8 +340,11 @@ func ApplyReplacements(p *v1.PipelineSpec, replacements map[string]string, array p.Finally[i].Workspaces[j].SubPath = substitution.ApplyReplacements(p.Finally[i].Workspaces[j].SubPath, replacements) } p.Finally[i].When = p.Finally[i].When.ReplaceVariables(replacements, arrayReplacements) - if p.Finally[i].TaskRef != nil && p.Finally[i].TaskRef.Params != nil { - p.Finally[i].TaskRef.Params = p.Finally[i].TaskRef.Params.ReplaceVariables(replacements, arrayReplacements, objectReplacements) + if p.Finally[i].TaskRef != nil { + if p.Finally[i].TaskRef.Params != nil { + p.Finally[i].TaskRef.Params = p.Finally[i].TaskRef.Params.ReplaceVariables(replacements, arrayReplacements, objectReplacements) + } + p.Finally[i].TaskRef.Name = substitution.ApplyReplacements(p.Finally[i].TaskRef.Name, replacements) } p.Finally[i] = propagateParams(p.Finally[i], replacements, arrayReplacements, objectReplacements) } diff --git a/pkg/reconciler/pipelinerun/resources/pipelineref.go b/pkg/reconciler/pipelinerun/resources/pipelineref.go index 226ca35c201..16844d895bd 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelineref.go +++ b/pkg/reconciler/pipelinerun/resources/pipelineref.go @@ -31,6 +31,7 @@ import ( "github.com/tektoncd/pipeline/pkg/remote" "github.com/tektoncd/pipeline/pkg/remoteresolution/remote/resolution" remoteresource "github.com/tektoncd/pipeline/pkg/remoteresolution/resource" + "github.com/tektoncd/pipeline/pkg/substitution" "github.com/tektoncd/pipeline/pkg/trustedresources" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -70,10 +71,17 @@ func GetPipelineFunc(ctx context.Context, k8s kubernetes.Interface, tekton clien stringReplacements[k] = v } replacedParams := pr.Params.ReplaceVariables(stringReplacements, arrayReplacements, objectReplacements) - + var url string + // The name is url-like so its not a local reference. + if err := v1.RefNameLikeUrl(pr.Name); err == nil { + // apply variable replacements in the name. + pr.Name = substitution.ApplyReplacements(pr.Name, stringReplacements) + url = pr.Name + } resolverPayload := remoteresource.ResolverPayload{ ResolutionSpec: &resolutionV1beta1.ResolutionRequestSpec{ Params: replacedParams, + URL: url, }, } resolver := resolution.NewResolver(requester, pipelineRun, string(pr.Resolver), resolverPayload) diff --git a/pkg/reconciler/pipelinerun/resources/pipelineref_test.go b/pkg/reconciler/pipelinerun/resources/pipelineref_test.go index 1f94e784bd3..87d48306cf5 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelineref_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelineref_test.go @@ -438,6 +438,7 @@ func TestGetPipelineFunc_RemoteResolution_ReplacedParams(t *testing.T) { ctx = config.ToContext(ctx, cfg) pipeline := parse.MustParseV1PipelineAndSetDefaults(t, pipelineYAMLString) pipelineRef := &v1.PipelineRef{ + Name: "https://foo/bar", ResolverRef: v1.ResolverRef{ Resolver: "git", Params: []v1.Param{{ @@ -467,6 +468,7 @@ func TestGetPipelineFunc_RemoteResolution_ReplacedParams(t *testing.T) { Name: "bar", Value: *v1.NewStructuredValues("test-pipeline"), }}, + URL: "https://foo/bar", }, }, } diff --git a/pkg/reconciler/taskrun/resources/taskref.go b/pkg/reconciler/taskrun/resources/taskref.go index 872af8a787a..0407deb3dc5 100644 --- a/pkg/reconciler/taskrun/resources/taskref.go +++ b/pkg/reconciler/taskrun/resources/taskref.go @@ -31,6 +31,7 @@ import ( "github.com/tektoncd/pipeline/pkg/remote" "github.com/tektoncd/pipeline/pkg/remoteresolution/remote/resolution" remoteresource "github.com/tektoncd/pipeline/pkg/remoteresolution/resource" + "github.com/tektoncd/pipeline/pkg/substitution" "github.com/tektoncd/pipeline/pkg/trustedresources" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -97,6 +98,7 @@ func GetTaskFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset // casting it to a TaskObject. return func(ctx context.Context, name string) (*v1.Task, *v1.RefSource, *trustedresources.VerificationResult, error) { var replacedParams v1.Params + var url string if ownerAsTR, ok := owner.(*v1.TaskRun); ok { stringReplacements, arrayReplacements, _ := replacementsFromParams(ownerAsTR.Spec.Params) for k, v := range getContextReplacements("", ownerAsTR) { @@ -106,6 +108,11 @@ func GetTaskFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset p.Value.ApplyReplacements(stringReplacements, arrayReplacements, nil) replacedParams = append(replacedParams, p) } + if err := v1.RefNameLikeUrl(tr.Name); err == nil { + // The name is url-like so its not a local reference. + tr.Name = substitution.ApplyReplacements(tr.Name, stringReplacements) + url = tr.Name + } } else { replacedParams = append(replacedParams, tr.Params...) } @@ -114,6 +121,7 @@ func GetTaskFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset Namespace: namespace, ResolutionSpec: &resolutionV1beta1.ResolutionRequestSpec{ Params: replacedParams, + URL: url, }, } resolver := resolution.NewResolver(requester, owner, string(tr.Resolver), resolverPayload) @@ -149,6 +157,7 @@ func GetStepActionFunc(tekton clientset.Interface, k8s kubernetes.Interface, req Namespace: namespace, ResolutionSpec: &resolutionV1beta1.ResolutionRequestSpec{ Params: step.Ref.Params, + URL: step.Ref.Name, }, } resolver := resolution.NewResolver(requester, tr, string(step.Ref.Resolver), resolverPayload) diff --git a/pkg/reconciler/taskrun/resources/taskref_test.go b/pkg/reconciler/taskrun/resources/taskref_test.go index 9bf6c18ca38..e619fe6a4b8 100644 --- a/pkg/reconciler/taskrun/resources/taskref_test.go +++ b/pkg/reconciler/taskrun/resources/taskref_test.go @@ -1194,6 +1194,7 @@ func TestGetTaskFunc_RemoteResolution_ReplacedParams(t *testing.T) { ctx = config.ToContext(ctx, cfg) task := parse.MustParseV1TaskAndSetDefaults(t, taskYAMLString) taskRef := &v1.TaskRef{ + Name: "https://foo/bar", ResolverRef: v1.ResolverRef{ Resolver: "git", Params: []v1.Param{{ @@ -1223,6 +1224,7 @@ func TestGetTaskFunc_RemoteResolution_ReplacedParams(t *testing.T) { Name: "bar", Value: *v1.NewStructuredValues("test-task"), }}, + URL: "https://foo/bar", }, }, } diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index ce23ebfa491..bc5a8354655 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -7039,7 +7039,7 @@ func TestIsConcurrentModificationError(t *testing.T) { // the ResolutionRequest's name is generated by resolverName, namespace and runName. func getResolvedResolutionRequest(t *testing.T, resolverName string, resourceBytes []byte, namespace string, runName string) resolutionv1beta1.ResolutionRequest { t.Helper() - name, err := remoteresource.GenerateDeterministicName(resolverName, namespace+"/"+runName, nil) + name, err := remoteresource.GenerateDeterministicNameFromSpec(resolverName, namespace+"/"+runName, &resolutionv1beta1.ResolutionRequestSpec{}) if err != nil { t.Errorf("error generating name for %s/%s/%s: %v", resolverName, namespace, runName, err) } diff --git a/pkg/remoteresolution/resolver/bundle/resolver.go b/pkg/remoteresolution/resolver/bundle/resolver.go index 85abd28672f..4f8612931a0 100644 --- a/pkg/remoteresolution/resolver/bundle/resolver.go +++ b/pkg/remoteresolution/resolver/bundle/resolver.go @@ -18,6 +18,7 @@ package bundle import ( "context" + "errors" "github.com/tektoncd/pipeline/pkg/apis/resolution/v1beta1" "github.com/tektoncd/pipeline/pkg/remoteresolution/resolver/framework" @@ -69,10 +70,18 @@ func (r *Resolver) GetSelector(context.Context) map[string]string { // Validate ensures reqolution request spec from a request are as expected. func (r *Resolver) Validate(ctx context.Context, req *v1beta1.ResolutionRequestSpec) error { - return bundle.ValidateParams(ctx, req.Params) + if len(req.Params) > 0 { + return bundle.ValidateParams(ctx, req.Params) + } + // Remove this error once validate url has been implemented. + return errors.New("cannot validate request. the Validate method has not been implemented.") } -// Resolve uses the given params to resolve the requested file or resource. +// Resolve uses the given request spec resolve the requested file or resource. func (r *Resolver) Resolve(ctx context.Context, req *v1beta1.ResolutionRequestSpec) (resolutionframework.ResolvedResource, error) { - return bundle.ResolveRequest(ctx, r.kubeClientSet, req) + if len(req.Params) > 0 { + return bundle.ResolveRequest(ctx, r.kubeClientSet, req) + } + // Remove this error once resolution of url has been implemented. + return nil, errors.New("the Resolve method has not been implemented.") } diff --git a/pkg/remoteresolution/resolver/cluster/resolver.go b/pkg/remoteresolution/resolver/cluster/resolver.go index 7c1e9072334..c08f8a18bd3 100644 --- a/pkg/remoteresolution/resolver/cluster/resolver.go +++ b/pkg/remoteresolution/resolver/cluster/resolver.go @@ -18,6 +18,7 @@ package cluster import ( "context" + "errors" "github.com/tektoncd/pipeline/pkg/apis/resolution/v1beta1" clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned" @@ -70,13 +71,21 @@ func (r *Resolver) GetSelector(_ context.Context) map[string]string { // Validate returns an error if the given parameter map is not // valid for a resource request targeting the cluster resolver. func (r *Resolver) Validate(ctx context.Context, req *v1beta1.ResolutionRequestSpec) error { - return cluster.ValidateParams(ctx, req.Params) + if len(req.Params) > 0 { + return cluster.ValidateParams(ctx, req.Params) + } + // Remove this error once validate url has been implemented. + return errors.New("cannot validate request. the Validate method has not been implemented.") } // Resolve performs the work of fetching a resource from a namespace with the given -// parameters. +// resolution spec. func (r *Resolver) Resolve(ctx context.Context, req *v1beta1.ResolutionRequestSpec) (resolutionframework.ResolvedResource, error) { - return cluster.ResolveFromParams(ctx, req.Params, r.pipelineClientSet) + if len(req.Params) > 0 { + return cluster.ResolveFromParams(ctx, req.Params, r.pipelineClientSet) + } + // Remove this error once resolution of url has been implemented. + return nil, errors.New("the Resolve method has not been implemented.") } var _ resolutionframework.ConfigWatcher = &Resolver{} diff --git a/pkg/remoteresolution/resolver/framework/fakeresolver.go b/pkg/remoteresolution/resolver/framework/fakeresolver.go index 995ead2e19e..ad7da0e6b85 100644 --- a/pkg/remoteresolution/resolver/framework/fakeresolver.go +++ b/pkg/remoteresolution/resolver/framework/fakeresolver.go @@ -18,6 +18,7 @@ package framework import ( "context" + "fmt" "time" "github.com/tektoncd/pipeline/pkg/apis/resolution/v1beta1" @@ -25,6 +26,8 @@ import ( "github.com/tektoncd/pipeline/pkg/resolution/resolver/framework" ) +const FakeUrl string = "fake://url" + var _ Resolver = &FakeResolver{} // FakeResolver implements a framework.Resolver that can fetch pre-configured strings based on a parameter value, or return @@ -56,13 +59,26 @@ func (r *FakeResolver) GetSelector(_ context.Context) map[string]string { // Validate returns an error if the given parameter map is not // valid for a resource request targeting the fake resolver. func (r *FakeResolver) Validate(_ context.Context, req *v1beta1.ResolutionRequestSpec) error { - return framework.ValidateParams(req.Params) + if len(req.Params) > 0 { + return framework.ValidateParams(req.Params) + } + if req.URL != FakeUrl { + return fmt.Errorf("Wrong url. Expected: %s, Got: %s", FakeUrl, req.URL) + } + return nil } // Resolve performs the work of fetching a file from the fake resolver given a map of // parameters. func (r *FakeResolver) Resolve(_ context.Context, req *v1beta1.ResolutionRequestSpec) (framework.ResolvedResource, error) { - return framework.Resolve(req.Params, r.ForParam) + if len(req.Params) > 0 { + return framework.Resolve(req.Params, r.ForParam) + } + frr, ok := r.ForParam[req.URL] + if !ok { + return nil, fmt.Errorf("couldn't find resource for url %s", req.URL) + } + return frr, nil } var _ framework.TimedResolution = &FakeResolver{} diff --git a/pkg/remoteresolution/resolver/framework/reconciler_test.go b/pkg/remoteresolution/resolver/framework/reconciler_test.go index e72582ca471..6c11ecc8a0f 100644 --- a/pkg/remoteresolution/resolver/framework/reconciler_test.go +++ b/pkg/remoteresolution/resolver/framework/reconciler_test.go @@ -148,6 +148,142 @@ func TestReconcile(t *testing.T) { }, }, }, + }, { + name: "unknown url", + inputRequest: &v1beta1.ResolutionRequest{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "resolution.tekton.dev/v1beta1", + Kind: "ResolutionRequest", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "rr", + Namespace: "foo", + CreationTimestamp: metav1.Time{Time: time.Now()}, + Labels: map[string]string{ + resolutioncommon.LabelKeyResolverType: resolutionframework.LabelValueFakeResolverType, + }, + }, + Spec: v1beta1.ResolutionRequestSpec{ + URL: "dne://does-not-exist", + }, + Status: v1beta1.ResolutionRequestStatus{}, + }, + expectedErr: errors.New("invalid resource request \"foo/rr\": Wrong url. Expected: fake://url, Got: dne://does-not-exist"), + }, { + name: "valid url", + inputRequest: &v1beta1.ResolutionRequest{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "resolution.tekton.dev/v1beta1", + Kind: "ResolutionRequest", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "rr", + Namespace: "foo", + CreationTimestamp: metav1.Time{Time: time.Now()}, + Labels: map[string]string{ + resolutioncommon.LabelKeyResolverType: resolutionframework.LabelValueFakeResolverType, + }, + }, + Spec: v1beta1.ResolutionRequestSpec{ + URL: framework.FakeUrl, + }, + Status: v1beta1.ResolutionRequestStatus{}, + }, + paramMap: map[string]*resolutionframework.FakeResolvedResource{ + framework.FakeUrl: { + Content: "some content", + AnnotationMap: map[string]string{"foo": "bar"}, + ContentSource: &pipelinev1.RefSource{ + URI: "https://abc.com", + Digest: map[string]string{ + "sha1": "xyz", + }, + EntryPoint: "foo/bar", + }, + }, + }, + expectedStatus: &v1beta1.ResolutionRequestStatus{ + Status: duckv1.Status{ + Annotations: map[string]string{ + "foo": "bar", + }, + }, + ResolutionRequestStatusFields: v1beta1.ResolutionRequestStatusFields{ + Data: base64.StdEncoding.Strict().EncodeToString([]byte("some content")), + RefSource: &pipelinev1.RefSource{ + URI: "https://abc.com", + Digest: map[string]string{ + "sha1": "xyz", + }, + EntryPoint: "foo/bar", + }, + Source: &pipelinev1.RefSource{ + URI: "https://abc.com", + Digest: map[string]string{ + "sha1": "xyz", + }, + EntryPoint: "foo/bar", + }, + }, + }, + }, { + name: "resource not found for url", + inputRequest: &v1beta1.ResolutionRequest{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "resolution.tekton.dev/v1beta1", + Kind: "ResolutionRequest", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "rr", + Namespace: "foo", + CreationTimestamp: metav1.Time{Time: time.Now()}, + Labels: map[string]string{ + resolutioncommon.LabelKeyResolverType: resolutionframework.LabelValueFakeResolverType, + }, + }, + Spec: v1beta1.ResolutionRequestSpec{ + URL: framework.FakeUrl, + }, + Status: v1beta1.ResolutionRequestStatus{}, + }, + paramMap: map[string]*resolutionframework.FakeResolvedResource{ + "other://resource": { + Content: "some content", + AnnotationMap: map[string]string{"foo": "bar"}, + ContentSource: &pipelinev1.RefSource{ + URI: "https://abc.com", + Digest: map[string]string{ + "sha1": "xyz", + }, + EntryPoint: "foo/bar", + }, + }, + }, + expectedErr: errors.New("error getting \"Fake\" \"foo/rr\": couldn't find resource for url fake://url"), + }, { + name: "invalid params", + inputRequest: &v1beta1.ResolutionRequest{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "resolution.tekton.dev/v1beta1", + Kind: "ResolutionRequest", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "rr", + Namespace: "foo", + CreationTimestamp: metav1.Time{Time: time.Now()}, + Labels: map[string]string{ + resolutioncommon.LabelKeyResolverType: resolutionframework.LabelValueFakeResolverType, + }, + }, + Spec: v1beta1.ResolutionRequestSpec{ + Params: []pipelinev1.Param{{ + Name: "not-a-fake-param", + Value: *pipelinev1.NewStructuredValues("bar"), + }}, + }, + Status: v1beta1.ResolutionRequestStatus{}, + }, + expectedErr: errors.New(`invalid resource request "foo/rr": missing fake-key`), }, { name: "error resolving", inputRequest: &v1beta1.ResolutionRequest{ diff --git a/pkg/remoteresolution/resolver/git/resolver.go b/pkg/remoteresolution/resolver/git/resolver.go index 8aa15b65bd0..7231f4b007c 100644 --- a/pkg/remoteresolution/resolver/git/resolver.go +++ b/pkg/remoteresolution/resolver/git/resolver.go @@ -97,28 +97,36 @@ func (r *Resolver) GetSelector(_ context.Context) map[string]string { // ValidateParams returns an error if the given parameter map is not // valid for a resource request targeting the gitresolver. func (r *Resolver) Validate(ctx context.Context, req *v1beta1.ResolutionRequestSpec) error { - return git.ValidateParams(ctx, req.Params) + if len(req.Params) > 0 { + return git.ValidateParams(ctx, req.Params) + } + // Remove this error once validate url has been implemented. + return errors.New("cannot validate request. the Validate method has not been implemented.") } // Resolve performs the work of fetching a file from git given a map of // parameters. func (r *Resolver) Resolve(ctx context.Context, req *v1beta1.ResolutionRequestSpec) (resolutionframework.ResolvedResource, error) { - origParams := req.Params + if len(req.Params) > 0 { + origParams := req.Params - if git.IsDisabled(ctx) { - return nil, errors.New(disabledError) - } + if git.IsDisabled(ctx) { + return nil, errors.New(disabledError) + } - params, err := git.PopulateDefaultParams(ctx, origParams) - if err != nil { - return nil, err - } + params, err := git.PopulateDefaultParams(ctx, origParams) + if err != nil { + return nil, err + } - if params[git.UrlParam] != "" { - return git.ResolveAnonymousGit(ctx, params) - } + if params[git.UrlParam] != "" { + return git.ResolveAnonymousGit(ctx, params) + } - return git.ResolveAPIGit(ctx, params, r.kubeClient, r.logger, r.cache, r.ttl, r.clientFunc) + return git.ResolveAPIGit(ctx, params, r.kubeClient, r.logger, r.cache, r.ttl, r.clientFunc) + } + // Remove this error once resolution of url has been implemented. + return nil, errors.New("the Resolve method has not been implemented.") } var _ resolutionframework.ConfigWatcher = &Resolver{} diff --git a/pkg/remoteresolution/resolver/http/resolver.go b/pkg/remoteresolution/resolver/http/resolver.go index 3a0a5d48f0c..ec106586d9c 100644 --- a/pkg/remoteresolution/resolver/http/resolver.go +++ b/pkg/remoteresolution/resolver/http/resolver.go @@ -81,20 +81,28 @@ func (r *Resolver) GetSelector(context.Context) map[string]string { // Validate ensures parameters from a request are as expected. func (r *Resolver) Validate(ctx context.Context, req *v1beta1.ResolutionRequestSpec) error { - return http.ValidateParams(ctx, req.Params) + if len(req.Params) > 0 { + return http.ValidateParams(ctx, req.Params) + } + // Remove this error once validate url has been implemented. + return errors.New("cannot validate request. the Validate method has not been implemented.") } // Resolve uses the given params to resolve the requested file or resource. func (r *Resolver) Resolve(ctx context.Context, req *v1beta1.ResolutionRequestSpec) (resolutionframework.ResolvedResource, error) { - oParams := req.Params - if http.IsDisabled(ctx) { - return nil, errors.New(disabledError) - } - - params, err := http.PopulateDefaultParams(ctx, oParams) - if err != nil { - return nil, err + if len(req.Params) > 0 { + oParams := req.Params + if http.IsDisabled(ctx) { + return nil, errors.New(disabledError) + } + + params, err := http.PopulateDefaultParams(ctx, oParams) + if err != nil { + return nil, err + } + + return http.FetchHttpResource(ctx, params, r.kubeClient, r.logger) } - - return http.FetchHttpResource(ctx, params, r.kubeClient, r.logger) + // Remove this error once resolution of url has been implemented. + return nil, errors.New("the Resolve method has not been implemented.") } diff --git a/pkg/remoteresolution/resolver/http/resolver_test.go b/pkg/remoteresolution/resolver/http/resolver_test.go index f6d4634822e..0c9a3fd2ec5 100644 --- a/pkg/remoteresolution/resolver/http/resolver_test.go +++ b/pkg/remoteresolution/resolver/http/resolver_test.go @@ -108,6 +108,9 @@ func TestValidate(t *testing.T) { params := map[string]string{} if tc.url != "nourl" { params[httpresolution.UrlParam] = tc.url + } else { + // inject a fake param so that it can validate that the url is actually missing. + params["foo"] = "bar" } req := v1beta1.ResolutionRequestSpec{ Params: toParams(params), @@ -160,6 +163,11 @@ func TestResolve(t *testing.T) { Name: httpresolution.UrlParam, Value: *pipelinev1.NewStructuredValues(svr.URL), }) + } else { + params = append(params, pipelinev1.Param{ + Name: "foo", + Value: *pipelinev1.NewStructuredValues("bar"), + }) } resolver := Resolver{} req := v1beta1.ResolutionRequestSpec{ @@ -199,7 +207,7 @@ func TestResolve(t *testing.T) { func TestResolveNotEnabled(t *testing.T) { var err error resolver := Resolver{} - someParams := map[string]string{} + someParams := map[string]string{"foo": "bar"} req := v1beta1.ResolutionRequestSpec{ Params: toParams(someParams), } @@ -210,7 +218,7 @@ func TestResolveNotEnabled(t *testing.T) { if d := cmp.Diff(disabledError, err.Error()); d != "" { t.Errorf("unexpected error: %s", diff.PrintWantGot(d)) } - err = resolver.Validate(resolverDisabledContext(), &v1beta1.ResolutionRequestSpec{Params: toParams(map[string]string{})}) + err = resolver.Validate(resolverDisabledContext(), &v1beta1.ResolutionRequestSpec{Params: toParams(someParams)}) if err == nil { t.Fatalf("expected disabled err") } diff --git a/pkg/remoteresolution/resolver/hub/resolver.go b/pkg/remoteresolution/resolver/hub/resolver.go index fbea8b32709..8c29b23e50d 100644 --- a/pkg/remoteresolution/resolver/hub/resolver.go +++ b/pkg/remoteresolution/resolver/hub/resolver.go @@ -15,6 +15,7 @@ package hub import ( "context" + "errors" "github.com/tektoncd/pipeline/pkg/apis/resolution/v1beta1" "github.com/tektoncd/pipeline/pkg/remoteresolution/resolver/framework" @@ -69,10 +70,18 @@ func (r *Resolver) GetSelector(context.Context) map[string]string { // Validate ensures parameters from a request are as expected. func (r *Resolver) Validate(ctx context.Context, req *v1beta1.ResolutionRequestSpec) error { - return hub.ValidateParams(ctx, req.Params, r.TektonHubURL) + if len(req.Params) > 0 { + return hub.ValidateParams(ctx, req.Params, r.TektonHubURL) + } + // Remove this error once validate url has been implemented. + return errors.New("cannot validate request. the Validate method has not been implemented.") } // Resolve uses the given params to resolve the requested file or resource. func (r *Resolver) Resolve(ctx context.Context, req *v1beta1.ResolutionRequestSpec) (resolutionframework.ResolvedResource, error) { - return hub.Resolve(ctx, req.Params, r.TektonHubURL, r.ArtifactHubURL) + if len(req.Params) > 0 { + return hub.Resolve(ctx, req.Params, r.TektonHubURL, r.ArtifactHubURL) + } + // Remove this error once resolution of url has been implemented. + return nil, errors.New("the Resolve method has not been implemented.") } diff --git a/pkg/remoteresolution/resource/crd_resource.go b/pkg/remoteresolution/resource/crd_resource.go index 017654ca005..55c619f7d48 100644 --- a/pkg/remoteresolution/resource/crd_resource.go +++ b/pkg/remoteresolution/resource/crd_resource.go @@ -87,6 +87,7 @@ func (r *CRDRequester) createResolutionRequest(ctx context.Context, resolver Res owner = ownedReq.OwnerRef() } rr := resolutionresource.CreateResolutionRequest(ctx, resolver, req.ResolverPayload().Name, req.ResolverPayload().Namespace, req.ResolverPayload().ResolutionSpec.Params, owner) + rr.Spec.URL = req.ResolverPayload().ResolutionSpec.URL _, err := r.clientset.ResolutionV1beta1().ResolutionRequests(rr.Namespace).Create(ctx, rr, metav1.CreateOptions{}) return err } diff --git a/pkg/remoteresolution/resource/crd_resource_test.go b/pkg/remoteresolution/resource/crd_resource_test.go index 6bc4150e67e..728a52b73b7 100644 --- a/pkg/remoteresolution/resource/crd_resource_test.go +++ b/pkg/remoteresolution/resource/crd_resource_test.go @@ -78,6 +78,7 @@ resolverPayload: value: main - name: pathInRepo value: task/git-clone/0.6/git-clone.yaml + url: "https://foo/bar" `) baseRR := mustParseResolutionRequest(t, ` kind: "ResolutionRequest" @@ -102,6 +103,7 @@ spec: value: "main" - name: "pathInRepo" value: "task/git-clone/0.6/git-clone.yaml" + url: "https://foo/bar" `) createdRR := baseRR.DeepCopy() // diff --git a/pkg/remoteresolution/resource/request_test.go b/pkg/remoteresolution/resource/request_test.go index e387070345b..664f66cc423 100644 --- a/pkg/remoteresolution/resource/request_test.go +++ b/pkg/remoteresolution/resource/request_test.go @@ -40,6 +40,7 @@ func TestNewRequest(t *testing.T) { {Name: "param1", Value: v1.ParamValue{Type: v1.ParamTypeString, StringVal: "value1"}}, {Name: "param2", Value: v1.ParamValue{Type: v1.ParamTypeString, StringVal: "value2"}}, }, + URL: "https://foo/bar", }, }, } diff --git a/pkg/resolution/resource/name.go b/pkg/resolution/resource/name.go index eef3cb88afb..f93e0814e2e 100644 --- a/pkg/resolution/resource/name.go +++ b/pkg/resolution/resource/name.go @@ -105,6 +105,11 @@ func GenerateDeterministicNameFromSpec(prefix, base string, resolutionSpec *v1be } } } + if len(resolutionSpec.URL) > 0 { + if _, err := hasher.Write([]byte(resolutionSpec.URL)); err != nil { + return "", err + } + } name := fmt.Sprintf("%s-%x", prefix, hasher.Sum(nil)) if maxLength > len(name) { return name, nil diff --git a/pkg/resolution/resource/name_test.go b/pkg/resolution/resource/name_test.go index 1a90a0aaf18..95bb8c3afe1 100644 --- a/pkg/resolution/resource/name_test.go +++ b/pkg/resolution/resource/name_test.go @@ -20,6 +20,7 @@ import ( "testing" v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" + "github.com/tektoncd/pipeline/pkg/apis/resolution/v1beta1" "github.com/tektoncd/pipeline/pkg/resolution/resource" ) @@ -113,3 +114,107 @@ func TestGenerateDeterministicName(t *testing.T) { }) } } + +func TestGenerateDeterministicNameFromSpec(t *testing.T) { + type args struct { + prefix string + base string + params []v1.Param + url string + } + golden := args{ + prefix: "prefix", + base: "base", + params: []v1.Param{ + { + Name: "string-param", + Value: v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "value1", + }, + }, + { + Name: "array-param", + Value: v1.ParamValue{ + Type: v1.ParamTypeArray, + ArrayVal: []string{"value1", "value2"}, + }, + }, + { + Name: "object-param", + Value: v1.ParamValue{ + Type: v1.ParamTypeObject, + ObjectVal: map[string]string{"key": "value"}, + }, + }, + }, + url: "https://foo/bar", + } + tests := []struct { + name string + args args + want string + wantErr bool + }{ + { + name: "only contains prefix", + args: args{ + prefix: golden.prefix, + }, + want: "prefix-6c62272e07bb014262b821756295c58d", + }, + { + name: "only contains base", + args: args{ + base: golden.base, + }, + want: "-6989337ae0757277b806e97e86444ef0", + }, + { + name: "only contains url", + args: args{ + url: golden.url, + }, + want: "-dcfaf53735f4a84a3e319e17352940b4", + }, + { + name: "only contains params", + args: args{ + params: golden.params, + }, + want: "-52921b17d3c2930a34419c618d6af0e9", + }, + { + name: "params with different order should generate same hash", + args: args{ + params: []v1.Param{ + golden.params[2], + golden.params[1], + golden.params[0], + }, + }, + want: "-52921b17d3c2930a34419c618d6af0e9", + }, + { + name: "contain all fields", + args: golden, + want: "prefix-ff25bd24688ab610bdc530a5ab3aabbd", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resolutionSpec := &v1beta1.ResolutionRequestSpec{ + Params: tt.args.params, + URL: tt.args.url, + } + got, err := resource.GenerateDeterministicNameFromSpec(tt.args.prefix, tt.args.base, resolutionSpec) + if (err != nil) != tt.wantErr { + t.Errorf("GenerateDeterministicNameFromSpec() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("GenerateDeterministicNameFromSpec() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/test/e2e-tests-kind-prow-alpha.env b/test/e2e-tests-kind-prow-alpha.env index 255bdf63579..dfcc2e76444 100644 --- a/test/e2e-tests-kind-prow-alpha.env +++ b/test/e2e-tests-kind-prow-alpha.env @@ -8,3 +8,4 @@ ENABLE_STEP_ACTIONS=true ENABLE_CEL_IN_WHENEXPRESSION=true ENABLE_PARAM_ENUM=true ENABLE_ARTIFACTS=true +ENABLE_CONCISE_RESOLVER_SYNTAX=true diff --git a/test/e2e-tests.sh b/test/e2e-tests.sh index a291f27e4ba..8a8b63a48eb 100755 --- a/test/e2e-tests.sh +++ b/test/e2e-tests.sh @@ -32,6 +32,7 @@ ENABLE_STEP_ACTIONS=${ENABLE_STEP_ACTIONS:="false"} ENABLE_CEL_IN_WHENEXPRESSION=${ENABLE_CEL_IN_WHENEXPRESSION:="false"} ENABLE_PARAM_ENUM=${ENABLE_PARAM_ENUM:="false"} ENABLE_ARTIFACTS=${ENABLE_ARTIFACTS:="false"} +ENABLE_CONCISE_RESOLVER_SYNTAX=${ENABLE_CONCISE_RESOLVER_SYNTAX:="false"} failed=0 # Script entry point. @@ -130,6 +131,18 @@ function set_enable_artifacts() { kubectl patch configmap feature-flags -n tekton-pipelines -p "$jsonpatch" } +function set_enable_concise_resolver_syntax() { + local method="$1" + if [ "$method" != "false" ] && [ "$method" != "true" ]; then + printf "Invalid value for enable-concise-resolver-syntax %s\n" ${method} + exit 255 + fi + printf "Setting enable-concise-resolver-syntax to %s\n", ${method} + jsonpatch=$(printf "{\"data\": {\"enable-concise-resolver-syntax\": \"%s\"}}" $1) + echo "feature-flags ConfigMap patch: ${jsonpatch}" + kubectl patch configmap feature-flags -n tekton-pipelines -p "$jsonpatch" +} + function run_e2e() { # Run the integration tests header "Running Go e2e tests" @@ -157,6 +170,7 @@ set_enable_step_actions "$ENABLE_STEP_ACTIONS" set_cel_in_whenexpression "$ENABLE_CEL_IN_WHENEXPRESSION" set_enable_param_enum "$ENABLE_PARAM_ENUM" set_enable_artifacts "$ENABLE_ARTIFACTS" +set_enable_concise_resolver_syntax "$ENABLE_CONCISE_RESOLVER_SYNTAX" run_e2e (( failed )) && fail_test diff --git a/test/featureflags.go b/test/featureflags.go index 673d82cb75f..a8551be5401 100644 --- a/test/featureflags.go +++ b/test/featureflags.go @@ -111,13 +111,14 @@ func requireAllGates(gates map[string]string) func(context.Context, *testing.T, func getFeatureFlagsBaseOnAPIFlag(t *testing.T) *config.FeatureFlags { t.Helper() alphaFeatureFlags, err := config.NewFeatureFlagsFromMap(map[string]string{ - "enable-api-fields": "alpha", - "results-from": "sidecar-logs", - "enable-tekton-oci-bundles": "true", - "enable-step-actions": "true", - "enable-cel-in-whenexpression": "true", - "enable-param-enum": "true", - "enable-artifacts": "true", + "enable-api-fields": "alpha", + "results-from": "sidecar-logs", + "enable-tekton-oci-bundles": "true", + "enable-step-actions": "true", + "enable-cel-in-whenexpression": "true", + "enable-param-enum": "true", + "enable-artifacts": "true", + "enable-concise-resolver-syntax": "true", }) if err != nil { t.Fatalf("error creating alpha feature flags configmap: %v", err) diff --git a/test/remoteresolution/resolution.go b/test/remoteresolution/resolution.go index daefaed94ea..05274a7a182 100644 --- a/test/remoteresolution/resolution.go +++ b/test/remoteresolution/resolution.go @@ -73,7 +73,9 @@ func (r *Requester) Submit(ctx context.Context, resolverName resolution.Resolver if (r.ResolverPayload == resource.ResolverPayload{} || r.ResolverPayload.ResolutionSpec == nil || len(r.ResolverPayload.ResolutionSpec.Params) == 0) { return r.ResolvedResource, r.SubmitErr } - + if r.ResolverPayload.ResolutionSpec.URL == "" { + return r.ResolvedResource, r.SubmitErr + } reqParams := make(map[string]pipelinev1.ParamValue) for _, p := range req.ResolverPayload().ResolutionSpec.Params { reqParams[p.Name] = p.Value @@ -90,6 +92,9 @@ func (r *Requester) Submit(ctx context.Context, resolverName resolution.Resolver if len(wrongParams) > 0 { return nil, errors.New(strings.Join(wrongParams, "; ")) } + if r.ResolverPayload.ResolutionSpec.URL != req.ResolverPayload().ResolutionSpec.URL { + return nil, fmt.Errorf("Resolution name did not match. Got %s; Want %s", req.ResolverPayload().ResolutionSpec.URL, r.ResolverPayload.ResolutionSpec.URL) + } return r.ResolvedResource, r.SubmitErr }