From 4d9003316560090b422e6e27c136a1cb02f74687 Mon Sep 17 00:00:00 2001 From: Chitrang Patel Date: Wed, 27 Mar 2024 16:01:44 -0400 Subject: [PATCH] TEP-0154: Enable concise resolver syntax This PR enables concise resolver syntax as per TEP 0154. It allows users to pass a url like string to Taskref/name field. In turn, the Tekton controller passes the string to the Resolution Request under the name `resolutionName` which the resolvers can parse and resolve. --- config/config-feature-flags.yaml | 2 + docs/how-to-write-a-resolver.md | 30 ++++++++++- docs/pipeline-api.md | 4 ++ .../cmd/demoresolver/main.go | 19 ++++++- .../cmd/demoresolver/main_test.go | 50 ++++++++++++++++++- go.sum | 25 ++++++++++ pkg/apis/config/feature_flags.go | 36 ++++++++----- 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 | 38 ++++++++++---- .../pipeline/v1/container_validation_test.go | 28 ++++++----- pkg/apis/pipeline/v1/pipeline_types_test.go | 24 ++++++++- .../pipeline/v1/pipelineref_validation.go | 36 ++++++++++--- .../v1/pipelineref_validation_test.go | 28 ++++++----- pkg/apis/pipeline/v1/taskref_validation.go | 30 +++++++---- .../pipeline/v1/taskref_validation_test.go | 26 +++++----- .../v1beta1/resolution_request_types.go | 3 +- .../pipelinerun/pipelinerun_test.go | 2 +- pkg/reconciler/pipelinerun/resources/apply.go | 28 ++++++++--- .../pipelinerun/resources/pipelineref.go | 11 +++- .../pipelinerun/resources/pipelineref_test.go | 2 + pkg/reconciler/taskrun/resources/taskref.go | 11 +++- .../taskrun/resources/taskref_test.go | 2 + pkg/reconciler/taskrun/taskrun_test.go | 2 +- pkg/remote/resolution/resolver.go | 12 +++-- pkg/remote/resolution/resolver_test.go | 8 +-- pkg/resolution/common/interface.go | 1 + pkg/resolution/resolver/bundle/resolver.go | 7 ++- .../resolver/bundle/resolver_test.go | 2 +- pkg/resolution/resolver/cluster/resolver.go | 7 ++- .../resolver/framework/fakeresolver.go | 12 ++++- .../resolver/framework/interface.go | 8 ++- .../resolver/framework/reconciler.go | 27 +++++++--- pkg/resolution/resolver/git/resolver.go | 7 ++- pkg/resolution/resolver/git/resolver_test.go | 2 +- pkg/resolution/resolver/http/resolver.go | 7 ++- pkg/resolution/resolver/http/resolver_test.go | 4 +- pkg/resolution/resolver/hub/resolver.go | 7 ++- pkg/resolution/resolver/hub/resolver_test.go | 6 +-- pkg/resolution/resource/crd_resource.go | 3 +- pkg/resolution/resource/crd_resource_test.go | 2 + pkg/resolution/resource/name.go | 5 +- pkg/resolution/resource/name_test.go | 19 +++++-- pkg/resolution/resource/request.go | 15 ++++-- pkg/resolution/resource/request_test.go | 13 +++-- test/e2e-tests-kind-prow-alpha.env | 1 + test/e2e-tests.sh | 14 ++++++ test/featureflags.go | 15 +++--- test/resolution.go | 25 +++++++--- 50 files changed, 536 insertions(+), 157 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 6cb5073da37..8d8358551ae 100644 --- a/config/config-feature-flags.yaml +++ b/config/config-feature-flags.yaml @@ -131,3 +131,5 @@ data: enable-artifacts: "false" # Setting this flag to "true" will enable the built-in param input validation via param enum. enable-param-enum: "false" + # 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 0237fa11daf..4a74086ed33 100644 --- a/docs/how-to-write-a-resolver.md +++ b/docs/how-to-write-a-resolver.md @@ -220,6 +220,32 @@ func (r *resolver) ValidateParams(ctx context.Context, params map[string]string) You'll also need to add the `"errors"` package to your list of imports at the top of the file. +## The `ValidateResolutionName` method + +The `ValidateResolutionName` method checks that the `resolutionName` submitted as part of +a resolution request is valid and can be parsed by the resolver. Our example resolver expects +format for the `resolutionName` to be `demoscheme://` so we'll validate this format. + +```go +// ValidateResolutionName ensures resolutionName from a request is as expected. +func (r *resolver) ValidateResolutionName(ctx context.Context, resolutionName string) error { + u, err := url.ParseRequestURI(resolutionName) + if err != nil { + return err + } + if u.Scheme != "demoscheme" { + return errors.New("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` and `"errors"` package to your list of imports at +the top of the file. + ## The `Resolve` method We implement the `Resolve` method to do the heavy lifting of fetching @@ -233,8 +259,8 @@ The method signature we're implementing here has a is another type we have to implement but it has a small footprint: ```go -// Resolve uses the given params to resolve the requested file or resource. -func (r *resolver) Resolve(ctx context.Context, params map[string]string) (framework.ResolvedResource, error) { +// Resolve uses the given resolutionName or params to resolve the requested file or resource. +func (r *resolver) Resolve(ctx context.Context, resolutionName string, params map[string]string) (framework.ResolvedResource, error) { return &myResolvedResource{}, nil } diff --git a/docs/pipeline-api.md b/docs/pipeline-api.md index 5a07522b52b..4430b4d95ee 100644 --- a/docs/pipeline-api.md +++ b/docs/pipeline-api.md @@ -3494,6 +3494,10 @@ Example: “task/git-clone/0.8/git-clone.yaml”

+

RemoteResolutionUrl +(string alias)

+
+

ResolverName (string alias)

diff --git a/docs/resolver-template/cmd/demoresolver/main.go b/docs/resolver-template/cmd/demoresolver/main.go index 4c11d7164c2..de98984f72e 100644 --- a/docs/resolver-template/cmd/demoresolver/main.go +++ b/docs/resolver-template/cmd/demoresolver/main.go @@ -16,6 +16,8 @@ package main import ( "context" "errors" + "fmt" + "net/url" pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "github.com/tektoncd/pipeline/pkg/apis/resolution/v1beta1" @@ -51,6 +53,21 @@ func (r *resolver) GetSelector(context.Context) map[string]string { } } +// ValidateResolutionName ensures resolutionName from a request is as expected. +func (r *resolver) ValidateResolutionName(ctx context.Context, resolutionName string) error { + u, err := url.ParseRequestURI(resolutionName) + if err != nil { + return err + } + if u.Scheme != "demoscheme" { + return errors.New(fmt.Sprintf("Invalid Scheme. Want %s, Got %s", "demoscheme", u.Scheme)) + } + if u.Path == "" { + return errors.New("Empty path.") + } + return nil +} + // ValidateParams ensures parameters from a request are as expected. func (r *resolver) ValidateParams(ctx context.Context, params []pipelinev1.Param) error { if len(params) > 0 { @@ -60,7 +77,7 @@ func (r *resolver) ValidateParams(ctx context.Context, params []pipelinev1.Param } // Resolve uses the given params to resolve the requested file or resource. -func (r *resolver) Resolve(ctx context.Context, params []pipelinev1.Param) (framework.ResolvedResource, error) { +func (r *resolver) Resolve(ctx context.Context, resolutionName string, params []pipelinev1.Param) (framework.ResolvedResource, error) { return &myResolvedResource{}, nil } diff --git a/docs/resolver-template/cmd/demoresolver/main_test.go b/docs/resolver-template/cmd/demoresolver/main_test.go index 137aed1b96b..976f0423bcf 100644 --- a/docs/resolver-template/cmd/demoresolver/main_test.go +++ b/docs/resolver-template/cmd/demoresolver/main_test.go @@ -18,6 +18,7 @@ package main import ( "encoding/base64" + "errors" "testing" "time" @@ -27,6 +28,7 @@ import ( frtesting "github.com/tektoncd/pipeline/pkg/resolution/resolver/framework/testing" "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 +50,9 @@ func TestResolver(t *testing.T) { resolutioncommon.LabelKeyResolverType: "demo", }, }, - Spec: v1beta1.ResolutionRequestSpec{}, + Spec: v1beta1.ResolutionRequestSpec{ + ResolutionName: "demoscheme://foo/bar", + }, } d := test.Data{ ResolutionRequests: []*v1beta1.ResolutionRequest{request}, @@ -65,3 +69,47 @@ func TestResolver(t *testing.T) { frtesting.RunResolverReconcileTest(ctx, t, d, r, request, expectedStatus, expectedErr) } + +func TestResolver_Failure(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{ + ResolutionName: "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) +} diff --git a/go.sum b/go.sum index f5955f70c16..490c6092d0f 100644 --- a/go.sum +++ b/go.sum @@ -259,6 +259,11 @@ github.com/cncf/udpa/go v0.0.0-20210930031921-04548b0d99d4/go.mod h1:6pvJx4me5XP github.com/cncf/xds/go v0.0.0-20210922020428-25de7278fc84/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs= github.com/cncf/xds/go v0.0.0-20211001041855-01bcc9b48dfe/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs= github.com/cncf/xds/go v0.0.0-20211011173535-cb28da3451f1/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs= +<<<<<<< HEAD +======= +github.com/cncf/xds/go v0.0.0-20231128003011-0fa0005c9caa h1:jQCWAUqqlij9Pgj2i/PB79y4KOPYVyFYdROxgaCwdTQ= +github.com/cncf/xds/go v0.0.0-20231128003011-0fa0005c9caa/go.mod h1:x/1Gn8zydmfq8dk6e9PdstVsDgu9RuyIIJqAaF//0IM= +>>>>>>> ecfa77738 (TEP-0154: Enable concise resolver syntax) github.com/cockroachdb/datadriven v0.0.0-20190809214429-80d97fb3cbaa/go.mod h1:zn76sxSg3SzpJ0PPJaLDCu+Bu0Lg3sKTORVIj19EIF8= github.com/containerd/aufs v0.0.0-20200908144142-dab0cbea06f4/go.mod h1:nukgQABAEopAHvB6j7cnP5zJ+/3aVcE7hCYqvIwAHyE= github.com/containerd/aufs v0.0.0-20201003224125-76a6863f2989/go.mod h1:AkGGQs9NM2vtYHaUen+NljV0/baGCAPELGm2q9ZXpWU= @@ -435,6 +440,11 @@ github.com/envoyproxy/go-control-plane v0.9.9-0.20201210154907-fd9021fe5dad/go.m github.com/envoyproxy/go-control-plane v0.9.9-0.20210217033140-668b12f5399d/go.mod h1:cXg6YxExXjJnVBQHBLXeUAgxn2UodCpnH306RInaBQk= github.com/envoyproxy/go-control-plane v0.10.2-0.20220325020618-49ff273808a1/go.mod h1:KJwIaB5Mv44NWtYuAOFCVOjcI94vtpEz2JU/D2v6IjE= github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c= +<<<<<<< HEAD +======= +github.com/envoyproxy/protoc-gen-validate v1.0.4 h1:gVPz/FMfvh57HdSJQyvBtF00j8JU4zdyUgIUNhlgg0A= +github.com/envoyproxy/protoc-gen-validate v1.0.4/go.mod h1:qys6tmnRsYrQqIhm2bvKZH4Blx/1gTIZ2UKVY1M+Yew= +>>>>>>> ecfa77738 (TEP-0154: Enable concise resolver syntax) github.com/evanphx/json-patch v4.9.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= github.com/evanphx/json-patch v4.12.0+incompatible h1:4onqiflcdA9EOZ4RxV643DvftH5pOlLGNtQ5lPWQu84= github.com/evanphx/json-patch v4.12.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= @@ -454,8 +464,13 @@ github.com/garyburd/redigo v0.0.0-20150301180006-535138d7bcd7/go.mod h1:NR3MbYis github.com/ghodss/yaml v0.0.0-20150909031657-73d445a93680/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeMEF04= github.com/ghodss/yaml v1.0.0/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeMEF04= github.com/gliderlabs/ssh v0.2.2/go.mod h1:U7qILu1NlMHj9FlMhZLlkCdDnU1DBEAqr0aevW3Awn0= +<<<<<<< HEAD github.com/gliderlabs/ssh v0.3.7 h1:iV3Bqi942d9huXnzEF2Mt+CY9gLu8DNM4Obd+8bODRE= github.com/gliderlabs/ssh v0.3.7/go.mod h1:zpHEXBstFnQYtGnB8k8kQLol82umzn/2/snG7alWVD8= +======= +github.com/gliderlabs/ssh v0.3.5 h1:OcaySEmAQJgyYcArR+gGGTHCyE7nvhEMTlYY+Dp8CpY= +github.com/gliderlabs/ssh v0.3.5/go.mod h1:8XB4KraRrX39qHhT6yxPsHedjA08I/uBVwj4xC+/+z4= +>>>>>>> ecfa77738 (TEP-0154: Enable concise resolver syntax) github.com/go-fed/httpsig v1.1.0 h1:9M+hb0jkEICD8/cAiNqEB66R87tTINszBRTjwjQzWcI= github.com/go-fed/httpsig v1.1.0/go.mod h1:RCMrTZvN1bJYtofsG4rd5NaO5obxQ5xBkdiS7xsT7bM= github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 h1:+zs/tPmkDkHx3U66DAb0lQFJrpS6731Oaa12ikc+DiI= @@ -464,8 +479,13 @@ github.com/go-git/go-billy/v5 v5.5.0 h1:yEY4yhzCDuMGSv83oGxiBotRzhwhNr8VZyphhiu+ github.com/go-git/go-billy/v5 v5.5.0/go.mod h1:hmexnoNsr2SJU1Ju67OaNz5ASJY3+sHgFRpCtpDCKow= github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399 h1:eMje31YglSBqCdIqdhKBW8lokaMrL3uTkpGYlE2OOT4= github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399/go.mod h1:1OCfN199q1Jm3HZlxleg+Dw/mwps2Wbk9frAWm+4FII= +<<<<<<< HEAD github.com/go-git/go-git/v5 v5.12.0 h1:7Md+ndsjrzZxbddRDZjF14qK+NN56sy6wkqaVrjZtys= github.com/go-git/go-git/v5 v5.12.0/go.mod h1:FTM9VKtnI2m65hNI/TenDDDnUf2Q9FHnXYjuz9i5OEY= +======= +github.com/go-git/go-git/v5 v5.11.0 h1:XIZc1p+8YzypNr34itUfSvYJcv+eYdTnTvOZ2vD3cA4= +github.com/go-git/go-git/v5 v5.11.0/go.mod h1:6GFcX2P3NM7FPBfpePbpLd21XxsgdAt+lKqXmCUiUCY= +>>>>>>> ecfa77738 (TEP-0154: Enable concise resolver syntax) github.com/go-gl/glfw v0.0.0-20190409004039-e6da0acd62b1/go.mod h1:vR7hzQXu2zJy9AVAgeJqvqgH9Q5CA+iKCZ2gyEVpxRU= github.com/go-gl/glfw/v3.3/glfw v0.0.0-20191125211704-12ad95a8df72/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8= github.com/go-gl/glfw/v3.3/glfw v0.0.0-20200222043503-6f7a984d4dc4/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8= @@ -515,8 +535,13 @@ github.com/go-task/slim-sprig v0.0.0-20230315185526-52ccab3ef572 h1:tfuBGBXKqDEe github.com/go-task/slim-sprig v0.0.0-20230315185526-52ccab3ef572/go.mod h1:9Pwr4B2jHnOSGXyyzV8ROjYa2ojvAY6HCGYYfMoC3Ls= github.com/go-test/deep v1.1.0 h1:WOcxcdHcvdgThNXjw0t76K42FXTU7HpNQWHpA2HHNlg= github.com/go-test/deep v1.1.0/go.mod h1:5C2ZWiW0ErCdrYzpqxLbTX7MG14M9iiw8DgHncVwcsE= +<<<<<<< HEAD github.com/gobuffalo/flect v1.0.2 h1:eqjPGSo2WmjgY2XlpGwo2NXgL3RucAKo4k4qQMNA5sA= github.com/gobuffalo/flect v1.0.2/go.mod h1:A5msMlrHtLqh9umBSnvabjsMrCcCpAyzglnDvkbYKHs= +======= +github.com/gobuffalo/flect v0.2.4 h1:BSYA8+T60cdyq+vynaSUjqSVI9mDEg9ZfQUXKmfjo4I= +github.com/gobuffalo/flect v0.2.4/go.mod h1:1ZyCLIbg0YD7sDkzvFdPoOydPtD8y9JQnrOROolUcM8= +>>>>>>> ecfa77738 (TEP-0154: Enable concise resolver syntax) github.com/goccy/kpoward v0.1.0 h1:UcrLMG9rq7NwrMiUc0h+qUyIlvqPzqLiPb+zQEqH8cE= github.com/goccy/kpoward v0.1.0/go.mod h1:m13lkcWSvNXtYC9yrXzguwrt/YTDAGioPusndMdQ+eA= github.com/godbus/dbus v0.0.0-20151105175453-c7fdd8b5cd55/go.mod h1:/YcGZj5zSblfDWMMoOzV4fas9FZnQYTkDnsGvmh2Grw= diff --git a/pkg/apis/config/feature_flags.go b/pkg/apis/config/feature_flags.go index d4be3c024ee..a77dd4dc5a7 100644 --- a/pkg/apis/config/feature_flags.go +++ b/pkg/apis/config/feature_flags.go @@ -100,12 +100,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" disableAffinityAssistantKey = "disable-affinity-assistant" disableCredsInitKey = "disable-creds-init" @@ -160,6 +160,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 @@ -183,16 +190,17 @@ 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 + VerificationNoMatchPolicy string + EnableProvenanceInStatus bool + ResultExtractionMethod string + MaxResultSize int + SetSecurityContext bool + Coschedule string + EnableCELInWhenExpression bool + EnableStepActions bool + EnableParamEnum bool + EnableArtifacts bool + EnableConciseResolverSyntax bool } // GetFeatureFlagsConfigName returns the name of the configmap containing all @@ -287,10 +295,12 @@ 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 := 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 5cf208ba594..9b0309a32ed 100644 --- a/pkg/apis/config/feature_flags_test.go +++ b/pkg/apis/config/feature_flags_test.go @@ -57,6 +57,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { EnableCELInWhenExpression: config.DefaultEnableCELInWhenExpression.Enabled, EnableStepActions: config.DefaultEnableStepActions.Enabled, EnableParamEnum: config.DefaultEnableParamEnum.Enabled, + EnableConciseResolverSyntax: config.DefaultEnableConciseResolverSyntax.Enabled, }, fileName: config.GetFeatureFlagsConfigName(), }, @@ -81,6 +82,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { EnableStepActions: true, EnableArtifacts: true, EnableParamEnum: true, + EnableConciseResolverSyntax: true, }, fileName: "feature-flags-all-flags-set", }, @@ -307,6 +309,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 51f312c52b0..4c208639953 100644 --- a/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml +++ b/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml @@ -36,3 +36,4 @@ data: enable-step-actions: "true" enable-param-enum: "true" enable-artifacts: "true" + 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 56108b63db8..a97ca95b423 100644 --- a/pkg/apis/pipeline/v1/container_validation.go +++ b/pkg/apis/pipeline/v1/container_validation.go @@ -20,6 +20,8 @@ import ( "context" "strings" + "net/url" + "github.com/tektoncd/pipeline/pkg/apis/config" "k8s.io/apimachinery/pkg/util/validation" "knative.dev/pkg/apis" @@ -34,12 +36,6 @@ func (ref *Ref) Validate(ctx context.Context) (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 { errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver params", config.BetaAPIFields).ViaField("params")) if ref.Name != "" { @@ -50,13 +46,37 @@ func (ref *Ref) 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. + if err := RefNameLikeUrl(ref.Name); 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")) + // ref name can be a Url-like format. + if err := RefNameLikeUrl(ref.Name); err == nil { + // 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 } + +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..50348fff7d0 100644 --- a/pkg/apis/pipeline/v1/container_validation_test.go +++ b/pkg/apis/pipeline/v1/container_validation_test.go @@ -93,29 +93,33 @@ 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: cfgtesting.EnableBetaAPIFields, }, { - name: "ref params disallowed in conjunction with ref name", + name: "ref with url-like name without resolver", ref: &v1.Ref{ - Name: "bar", + Name: "https://foo.com/bar", + }, + wantErr: apis.ErrMissingField("resolver"), + wc: cfgtesting.EnableBetaAPIFields, + }, { + name: "ref params disallowed in conjucntion with pipelineref name", + ref: &v1.Ref{ + 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: cfgtesting.EnableBetaAPIFields, }, { 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..821e20ad340 100644 --- a/pkg/apis/pipeline/v1/pipeline_types_test.go +++ b/pkg/apis/pipeline/v1/pipeline_types_test.go @@ -612,14 +612,34 @@ 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"}, + }, + }, { + 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"}, + }, + }, { + 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"}, }, }, { name: "pipeline task - taskRef with resolver params but no resolver", diff --git a/pkg/apis/pipeline/v1/pipelineref_validation.go b/pkg/apis/pipeline/v1/pipelineref_validation.go index 9fa7c9894d0..7da3e7954f4 100644 --- a/pkg/apis/pipeline/v1/pipelineref_validation.go +++ b/pkg/apis/pipeline/v1/pipelineref_validation.go @@ -18,8 +18,10 @@ package v1 import ( "context" + "strings" "github.com/tektoncd/pipeline/pkg/apis/config" + "k8s.io/apimachinery/pkg/util/validation" "knative.dev/pkg/apis" ) @@ -30,13 +32,8 @@ func (ref *PipelineRef) Validate(ctx context.Context) (errs *apis.FieldError) { return } - 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")) - } - } + 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 != "" { @@ -47,7 +44,30 @@ func (ref *PipelineRef) Validate(ctx context.Context) (errs *apis.FieldError) { } errs = errs.Also(ValidateParameters(ctx, ref.Params)) } - } else if ref.Name == "" { + 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. + if err := RefNameLikeUrl(ref.Name); err != nil { + errs = errs.Also(apis.ErrInvalidValue(err, "name")) + } + } + } + case ref.Name != "": + // Pipelineref name can be a Url-like format. + if err := RefNameLikeUrl(ref.Name); err == nil { + // 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")) + } + } else { + // Or, it 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 diff --git a/pkg/apis/pipeline/v1/pipelineref_validation_test.go b/pkg/apis/pipeline/v1/pipelineref_validation_test.go index 114eb6a0f10..6733b86fef0 100644 --- a/pkg/apis/pipeline/v1/pipelineref_validation_test.go +++ b/pkg/apis/pipeline/v1/pipelineref_validation_test.go @@ -65,30 +65,32 @@ 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"), + wantErr: apis.ErrInvalidValue(`parse "foo": invalid URI for request`, "name"), withContext: cfgtesting.EnableBetaAPIFields, }, { - 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: cfgtesting.EnableBetaAPIFields, + }, { + name: "pipelineRef params disallowed in conjucntion 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")), + wantErr: apis.ErrMultipleOneOf("name", "params"), withContext: cfgtesting.EnableBetaAPIFields, }} diff --git a/pkg/apis/pipeline/v1/taskref_validation.go b/pkg/apis/pipeline/v1/taskref_validation.go index 1a743e6dac6..c16c52cad38 100644 --- a/pkg/apis/pipeline/v1/taskref_validation.go +++ b/pkg/apis/pipeline/v1/taskref_validation.go @@ -34,12 +34,6 @@ func (ref *TaskRef) Validate(ctx context.Context) (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 { errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "resolver params", config.BetaAPIFields).ViaField("params")) if ref.Name != "" { @@ -50,10 +44,28 @@ 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. + if err := RefNameLikeUrl(ref.Name); err != nil { + errs = errs.Also(apis.ErrInvalidValue(err, "name")) + } + } + } 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")) + // TaskRef name can be a Url-like format. + if err := RefNameLikeUrl(ref.Name); err == nil { + // 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")) + } + } else { + // Or, it 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")) diff --git a/pkg/apis/pipeline/v1/taskref_validation_test.go b/pkg/apis/pipeline/v1/taskref_validation_test.go index d2c721bc6a1..860dcc93378 100644 --- a/pkg/apis/pipeline/v1/taskref_validation_test.go +++ b/pkg/apis/pipeline/v1/taskref_validation_test.go @@ -120,30 +120,32 @@ 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"), + wantErr: apis.ErrInvalidValue(`parse "foo": invalid URI for request`, "name"), wc: cfgtesting.EnableBetaAPIFields, }, { - name: "taskref params disallowed in conjunction with taskref name", + name: "taskRef with url-like name without resolver", taskRef: &v1.TaskRef{ - Name: "bar", + Name: "https://foo.com/bar", + }, + wantErr: apis.ErrMissingField("resolver"), + wc: cfgtesting.EnableBetaAPIFields, + }, { + name: "taskRef params disallowed in conjucntion with pipelineref name", + taskRef: &v1.TaskRef{ + 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: cfgtesting.EnableBetaAPIFields, }} for _, ts := range tests { diff --git a/pkg/apis/resolution/v1beta1/resolution_request_types.go b/pkg/apis/resolution/v1beta1/resolution_request_types.go index 60b51fa0498..619fff927fc 100644 --- a/pkg/apis/resolution/v1beta1/resolution_request_types.go +++ b/pkg/apis/resolution/v1beta1/resolution_request_types.go @@ -63,7 +63,8 @@ type ResolutionRequestSpec struct { // path to file, the kind of authentication to leverage, etc. // +optional // +listType=atomic - Params []pipelinev1.Param `json:"params,omitempty"` + Params []pipelinev1.Param `json:"params,omitempty"` + ResolutionName string `json:"resolutionName,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 71d1e776f1c..38631133c60 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -16588,7 +16588,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.GenerateDeterministicName(resolverName, namespace+"/"+runName, nil, "") 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 a6674f15483..a0e9f93e86c 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelineref.go +++ b/pkg/reconciler/pipelinerun/resources/pipelineref.go @@ -30,6 +30,7 @@ import ( "github.com/tektoncd/pipeline/pkg/remote" "github.com/tektoncd/pipeline/pkg/remote/resolution" remoteresource "github.com/tektoncd/pipeline/pkg/resolution/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" @@ -69,8 +70,14 @@ func GetPipelineFunc(ctx context.Context, k8s kubernetes.Interface, tekton clien stringReplacements[k] = v } replacedParams := pr.Params.ReplaceVariables(stringReplacements, arrayReplacements, objectReplacements) - - resolver := resolution.NewResolver(requester, pipelineRun, string(pr.Resolver), "", "", replacedParams) + var resolutionName 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) + resolutionName = pr.Name + } + resolver := resolution.NewResolver(requester, pipelineRun, string(pr.Resolver), "", "", resolutionName, replacedParams) return resolvePipeline(ctx, resolver, name, namespace, k8s, tekton, verificationPolicies) } default: diff --git a/pkg/reconciler/pipelinerun/resources/pipelineref_test.go b/pkg/reconciler/pipelinerun/resources/pipelineref_test.go index 564a5160996..db6a9693ac0 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelineref_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelineref_test.go @@ -434,6 +434,7 @@ func TestGetPipelineFunc_RemoteResolution_ReplacedParams(t *testing.T) { ctx = config.ToContext(ctx, cfg) pipeline := parse.MustParseV1Pipeline(t, pipelineYAMLString) pipelineRef := &v1.PipelineRef{ + Name: "https://foo/bar", ResolverRef: v1.ResolverRef{ Resolver: "git", Params: []v1.Param{{ @@ -461,6 +462,7 @@ func TestGetPipelineFunc_RemoteResolution_ReplacedParams(t *testing.T) { Name: "bar", Value: *v1.NewStructuredValues("test-pipeline"), }}, + ResolutionName: "https://foo/bar", } fn := resources.GetPipelineFunc(ctx, nil, clients, requester, &v1.PipelineRun{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/reconciler/taskrun/resources/taskref.go b/pkg/reconciler/taskrun/resources/taskref.go index 14317583240..e6182376e44 100644 --- a/pkg/reconciler/taskrun/resources/taskref.go +++ b/pkg/reconciler/taskrun/resources/taskref.go @@ -30,6 +30,7 @@ import ( "github.com/tektoncd/pipeline/pkg/remote" "github.com/tektoncd/pipeline/pkg/remote/resolution" remoteresource "github.com/tektoncd/pipeline/pkg/resolution/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" @@ -96,6 +97,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 resolutionName string if ownerAsTR, ok := owner.(*v1.TaskRun); ok { stringReplacements, arrayReplacements, _ := replacementsFromParams(ownerAsTR.Spec.Params) for k, v := range getContextReplacements("", ownerAsTR) { @@ -105,10 +107,15 @@ 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) + resolutionName = tr.Name + } } else { replacedParams = append(replacedParams, tr.Params...) } - resolver := resolution.NewResolver(requester, owner, string(tr.Resolver), trName, namespace, replacedParams) + resolver := resolution.NewResolver(requester, owner, string(tr.Resolver), trName, namespace, resolutionName, replacedParams) return resolveTask(ctx, resolver, name, namespace, kind, k8s, tekton, verificationPolicies) } @@ -135,7 +142,7 @@ func GetStepActionFunc(tekton clientset.Interface, k8s kubernetes.Interface, req // casting it to a StepAction. return func(ctx context.Context, name string) (*v1alpha1.StepAction, *v1.RefSource, error) { // TODO(#7259): support params replacements for resolver params - resolver := resolution.NewResolver(requester, tr, string(step.Ref.Resolver), trName, namespace, step.Ref.Params) + resolver := resolution.NewResolver(requester, tr, string(step.Ref.Resolver), trName, namespace, step.Ref.Name, step.Ref.Params) return resolveStepAction(ctx, resolver, name, namespace, k8s, tekton) } } diff --git a/pkg/reconciler/taskrun/resources/taskref_test.go b/pkg/reconciler/taskrun/resources/taskref_test.go index 27379a2f94b..39f5c78f2dc 100644 --- a/pkg/reconciler/taskrun/resources/taskref_test.go +++ b/pkg/reconciler/taskrun/resources/taskref_test.go @@ -936,6 +936,7 @@ func TestGetTaskFunc_RemoteResolution_ReplacedParams(t *testing.T) { ctx = config.ToContext(ctx, cfg) task := parse.MustParseV1Task(t, taskYAMLString) taskRef := &v1.TaskRef{ + Name: "https://foo/bar", ResolverRef: v1.ResolverRef{ Resolver: "git", Params: []v1.Param{{ @@ -963,6 +964,7 @@ func TestGetTaskFunc_RemoteResolution_ReplacedParams(t *testing.T) { Name: "bar", Value: *v1.NewStructuredValues("test-task"), }}, + ResolutionName: "https://foo/bar", } tr := &v1.TaskRun{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index 256364c2333..e5395b18e85 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -6981,7 +6981,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.GenerateDeterministicName(resolverName, namespace+"/"+runName, nil, "") if err != nil { t.Errorf("error generating name for %s/%s/%s: %v", resolverName, namespace, runName, err) } diff --git a/pkg/remote/resolution/resolver.go b/pkg/remote/resolution/resolver.go index 772b39e416a..2bc4932d34b 100644 --- a/pkg/remote/resolution/resolver.go +++ b/pkg/remote/resolution/resolver.go @@ -36,6 +36,7 @@ type Resolver struct { owner kmeta.OwnerRefable resolverName string params v1.Params + resolutionName string targetName string targetNamespace string } @@ -44,11 +45,12 @@ var _ remote.Resolver = &Resolver{} // NewResolver returns an implementation of remote.Resolver capable // of performing asynchronous remote resolution. -func NewResolver(requester remoteresource.Requester, owner kmeta.OwnerRefable, resolverName string, targetName string, targetNamespace string, params v1.Params) remote.Resolver { +func NewResolver(requester remoteresource.Requester, owner kmeta.OwnerRefable, resolverName string, targetName string, targetNamespace string, resolutionName string, params v1.Params) remote.Resolver { return &Resolver{ requester: requester, owner: owner, resolverName: resolverName, + resolutionName: resolutionName, params: params, targetName: targetName, targetNamespace: targetNamespace, @@ -58,7 +60,7 @@ func NewResolver(requester remoteresource.Requester, owner kmeta.OwnerRefable, r // Get implements remote.Resolver. func (resolver *Resolver) Get(ctx context.Context, _, _ string) (runtime.Object, *v1.RefSource, error) { resolverName := remoteresource.ResolverName(resolver.resolverName) - req, err := buildRequest(resolver.resolverName, resolver.owner, resolver.targetName, resolver.targetNamespace, resolver.params) + req, err := buildRequest(resolver.resolverName, resolver.owner, resolver.targetName, resolver.targetNamespace, resolver.resolutionName, resolver.params) if err != nil { return nil, nil, fmt.Errorf("error building request for remote resource: %w", err) } @@ -88,7 +90,7 @@ func (resolver *Resolver) List(_ context.Context) ([]remote.ResolvedObject, erro return nil, nil } -func buildRequest(resolverName string, owner kmeta.OwnerRefable, name string, namespace string, params v1.Params) (*resolutionRequest, error) { +func buildRequest(resolverName string, owner kmeta.OwnerRefable, name string, namespace string, resolutionName string, params v1.Params) (*resolutionRequest, error) { if name == "" { name = owner.GetObjectMeta().GetName() namespace = owner.GetObjectMeta().GetNamespace() @@ -100,12 +102,12 @@ func buildRequest(resolverName string, owner kmeta.OwnerRefable, name string, na // prevents multiple requests being issued for the same // pipelinerun's pipelineRef or taskrun's taskRef. remoteResourceBaseName := namespace + "/" + name - name, err := remoteresource.GenerateDeterministicName(resolverName, remoteResourceBaseName, params) + name, err := remoteresource.GenerateDeterministicName(resolverName, remoteResourceBaseName, params, resolutionName) if err != nil { return nil, fmt.Errorf("error generating name for taskrun %s/%s: %w", namespace, name, err) } req := &resolutionRequest{ - Request: remoteresource.NewRequest(name, namespace, params), + Request: remoteresource.NewRequest(name, namespace, params, resolutionName), owner: owner, } return req, nil diff --git a/pkg/remote/resolution/resolver_test.go b/pkg/remote/resolution/resolver_test.go index 8e900ba50ea..84adab9f6e6 100644 --- a/pkg/remote/resolution/resolver_test.go +++ b/pkg/remote/resolution/resolver_test.go @@ -68,7 +68,7 @@ func TestGet_Successful(t *testing.T) { SubmitErr: nil, ResolvedResource: resolved, } - resolver := NewResolver(requester, owner, "git", "", "", nil) + resolver := NewResolver(requester, owner, "git", "", "", "", nil) if _, _, err := resolver.Get(ctx, "foo", "bar"); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -121,7 +121,7 @@ func TestGet_Errors(t *testing.T) { SubmitErr: tc.submitErr, ResolvedResource: tc.resolvedResource, } - resolver := NewResolver(requester, owner, "git", "", "", nil) + resolver := NewResolver(requester, owner, "git", "", "", "", nil) obj, refSource, err := resolver.Get(ctx, "foo", "bar") if obj != nil { t.Errorf("received unexpected resolved resource") @@ -155,7 +155,7 @@ func TestBuildRequest(t *testing.T) { }, } - req, err := buildRequest("git", owner, tc.targetName, tc.targetNamespace, nil) + req, err := buildRequest("git", owner, tc.targetName, tc.targetNamespace, "", nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -166,7 +166,7 @@ func TestBuildRequest(t *testing.T) { if tc.targetName != "" { reqNameBase = tc.targetNamespace + "/" + tc.targetName } - expectedReqName, err := remoteresource.GenerateDeterministicName("git", reqNameBase, nil) + expectedReqName, err := remoteresource.GenerateDeterministicName("git", reqNameBase, nil, "") if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/pkg/resolution/common/interface.go b/pkg/resolution/common/interface.go index 3d2941c8aed..c6f8ac5ac5a 100644 --- a/pkg/resolution/common/interface.go +++ b/pkg/resolution/common/interface.go @@ -45,6 +45,7 @@ type Request interface { Name() string Namespace() string Params() pipelinev1.Params + ResolutionName() string } // OwnedRequest is implemented by any type implementing Request that also needs diff --git a/pkg/resolution/resolver/bundle/resolver.go b/pkg/resolution/resolver/bundle/resolver.go index a5cd07ac5f3..292ba9c0777 100644 --- a/pkg/resolution/resolver/bundle/resolver.go +++ b/pkg/resolution/resolver/bundle/resolver.go @@ -74,6 +74,11 @@ func (r *Resolver) GetSelector(context.Context) map[string]string { } } +// ValidateResolutionName ensures that the resolutionName from the request can be parsed. +func (r *Resolver) ValidateResolutionName(ctx context.Context, resolutionName string) error { + return nil +} + // ValidateParams ensures parameters from a request are as expected. func (r *Resolver) ValidateParams(ctx context.Context, params []pipelinev1.Param) error { if r.isDisabled(ctx) { @@ -86,7 +91,7 @@ func (r *Resolver) ValidateParams(ctx context.Context, params []pipelinev1.Param } // Resolve uses the given params to resolve the requested file or resource. -func (r *Resolver) Resolve(ctx context.Context, params []pipelinev1.Param) (framework.ResolvedResource, error) { +func (r *Resolver) Resolve(ctx context.Context, resolutionName string, params []pipelinev1.Param) (framework.ResolvedResource, error) { if r.isDisabled(ctx) { return nil, errors.New(disabledError) } diff --git a/pkg/resolution/resolver/bundle/resolver_test.go b/pkg/resolution/resolver/bundle/resolver_test.go index 4f575d00b8f..598dadd3ba5 100644 --- a/pkg/resolution/resolver/bundle/resolver_test.go +++ b/pkg/resolution/resolver/bundle/resolver_test.go @@ -184,7 +184,7 @@ func TestResolveDisabled(t *testing.T) { Name: bundle.ParamImagePullSecret, Value: *pipelinev1.NewStructuredValues("baz"), }} - _, err = resolver.Resolve(resolverDisabledContext(), params) + _, err = resolver.Resolve(resolverDisabledContext(), "", params) if err == nil { t.Fatalf("expected disabled err") } diff --git a/pkg/resolution/resolver/cluster/resolver.go b/pkg/resolution/resolver/cluster/resolver.go index 44c5c800b9e..eb42b0b43e5 100644 --- a/pkg/resolution/resolver/cluster/resolver.go +++ b/pkg/resolution/resolver/cluster/resolver.go @@ -86,9 +86,14 @@ func (r *Resolver) ValidateParams(ctx context.Context, params []pipelinev1.Param return err } +// ValidateResolutionName ensures that the resolutionName from the request can be parsed. +func (r *Resolver) ValidateResolutionName(ctx context.Context, resolutionName string) error { + return nil +} + // Resolve performs the work of fetching a resource from a namespace with the given // parameters. -func (r *Resolver) Resolve(ctx context.Context, origParams []pipelinev1.Param) (framework.ResolvedResource, error) { +func (r *Resolver) Resolve(ctx context.Context, resolutionName string, origParams []pipelinev1.Param) (framework.ResolvedResource, error) { if r.isDisabled(ctx) { return nil, errors.New(disabledError) } diff --git a/pkg/resolution/resolver/framework/fakeresolver.go b/pkg/resolution/resolver/framework/fakeresolver.go index 0943199601b..fc03b06b9a7 100644 --- a/pkg/resolution/resolver/framework/fakeresolver.go +++ b/pkg/resolution/resolver/framework/fakeresolver.go @@ -38,6 +38,8 @@ const ( // FakeParamName is the name used for the fake resolver's single parameter. FakeParamName string = "fake-key" + + FakeResolutionName string = "fake://url" ) var _ Resolver = &FakeResolver{} @@ -92,6 +94,10 @@ func (r *FakeResolver) GetName(_ context.Context) string { return FakeResolverName } +func (r *FakeResolver) GetResolutionName(_ context.Context) string { + return FakeResolutionName +} + // GetSelector returns the labels that resource requests are required to have for // the fake resolver to process them. func (r *FakeResolver) GetSelector(_ context.Context) map[string]string { @@ -100,6 +106,10 @@ func (r *FakeResolver) GetSelector(_ context.Context) map[string]string { } } +func (r *FakeResolver) ValidateResolutionName(_ context.Context, resolutionName string) error { + return nil +} + // ValidateParams returns an error if the given parameter map is not // valid for a resource request targeting the fake resolver. func (r *FakeResolver) ValidateParams(_ context.Context, params []pipelinev1.Param) error { @@ -131,7 +141,7 @@ func (r *FakeResolver) ValidateParams(_ context.Context, params []pipelinev1.Par // Resolve performs the work of fetching a file from the fake resolver given a map of // parameters. -func (r *FakeResolver) Resolve(_ context.Context, params []pipelinev1.Param) (ResolvedResource, error) { +func (r *FakeResolver) Resolve(_ context.Context, resolutionName string, params []pipelinev1.Param) (ResolvedResource, error) { paramsMap := make(map[string]pipelinev1.ParamValue) for _, p := range params { paramsMap[p.Name] = p.Value diff --git a/pkg/resolution/resolver/framework/interface.go b/pkg/resolution/resolver/framework/interface.go index 0a0118c28da..7288f779209 100644 --- a/pkg/resolution/resolver/framework/interface.go +++ b/pkg/resolution/resolver/framework/interface.go @@ -43,13 +43,17 @@ type Resolver interface { // request and should return an error if any are missing or invalid. ValidateParams(context.Context, []pipelinev1.Param) error - // Resolve receives the parameters passed via a resource request + // ValidateResolutionName is given the resolutionName from a resource + // request and should return an error if the resolver cannot resolve it. + ValidateResolutionName(context.Context, string) error + + // Resolve receives the url and parameters passed via a resource request // and returns the resolved data along with any annotations // to include in the response. If resolution fails then an error // should be returned instead. If a resolution.Error // is returned then its Reason and Message are used as part of the // response to the request. - Resolve(context.Context, []pipelinev1.Param) (ResolvedResource, error) + Resolve(context.Context, string, []pipelinev1.Param) (ResolvedResource, error) } // ConfigWatcher is the interface to implement if your resolver accepts diff --git a/pkg/resolution/resolver/framework/reconciler.go b/pkg/resolution/resolver/framework/reconciler.go index b981ea6e02a..c9ff32106f2 100644 --- a/pkg/resolution/resolver/framework/reconciler.go +++ b/pkg/resolution/resolver/framework/reconciler.go @@ -119,15 +119,28 @@ func (r *Reconciler) resolve(ctx context.Context, key string, rr *v1beta1.Resolu defer cancelFn() go func() { - validationError := r.resolver.ValidateParams(resolutionCtx, rr.Spec.Params) - if validationError != nil { - errChan <- &resolutioncommon.InvalidRequestError{ - ResolutionRequestKey: key, - Message: validationError.Error(), + // if params are provided, use them instead of the resolutionName to resolve the request. + // The webhook will thwor a validation error if both are provided anyway. + if len(rr.Spec.Params) > 0 { + validationError := r.resolver.ValidateParams(resolutionCtx, rr.Spec.Params) + if validationError != nil { + errChan <- &resolutioncommon.InvalidRequestError{ + ResolutionRequestKey: key, + Message: validationError.Error(), + } + return + } + } else { + validationError := r.resolver.ValidateResolutionName(resolutionCtx, rr.Spec.ResolutionName) + if validationError != nil { + errChan <- &resolutioncommon.InvalidRequestError{ + ResolutionRequestKey: key, + Message: validationError.Error(), + } + return } - return } - resource, resolveErr := r.resolver.Resolve(resolutionCtx, rr.Spec.Params) + resource, resolveErr := r.resolver.Resolve(resolutionCtx, rr.Spec.ResolutionName, rr.Spec.Params) if resolveErr != nil { errChan <- &resolutioncommon.GetResourceError{ ResolverName: r.resolver.GetName(resolutionCtx), diff --git a/pkg/resolution/resolver/git/resolver.go b/pkg/resolution/resolver/git/resolver.go index 3405eff6a78..e69665c409c 100644 --- a/pkg/resolution/resolver/git/resolver.go +++ b/pkg/resolution/resolver/git/resolver.go @@ -124,9 +124,14 @@ func (r *Resolver) ValidateParams(ctx context.Context, params []pipelinev1.Param return nil } +// ValidateResolutionName ensures that the resolutionName from the request can be parsed. +func (r *Resolver) ValidateResolutionName(ctx context.Context, resolutionName string) error { + return nil +} + // Resolve performs the work of fetching a file from git given a map of // parameters. -func (r *Resolver) Resolve(ctx context.Context, origParams []pipelinev1.Param) (framework.ResolvedResource, error) { +func (r *Resolver) Resolve(ctx context.Context, resolutionName string, origParams []pipelinev1.Param) (framework.ResolvedResource, error) { if r.isDisabled(ctx) { return nil, errors.New(disabledError) } diff --git a/pkg/resolution/resolver/git/resolver_test.go b/pkg/resolution/resolver/git/resolver_test.go index a35ec7646a2..28ff38f40c8 100644 --- a/pkg/resolution/resolver/git/resolver_test.go +++ b/pkg/resolution/resolver/git/resolver_test.go @@ -245,7 +245,7 @@ func TestResolveNotEnabled(t *testing.T) { pathParam: "bar", revisionParam: "baz", } - _, err = resolver.Resolve(resolverDisabledContext(), toParams(someParams)) + _, err = resolver.Resolve(resolverDisabledContext(), "", toParams(someParams)) if err == nil { t.Fatalf("expected disabled err") } diff --git a/pkg/resolution/resolver/http/resolver.go b/pkg/resolution/resolver/http/resolver.go index 9e920f360f8..00bb1e2f605 100644 --- a/pkg/resolution/resolver/http/resolver.go +++ b/pkg/resolution/resolver/http/resolver.go @@ -87,6 +87,11 @@ func (r *Resolver) GetSelector(context.Context) map[string]string { } } +// ValidateResolutionName ensures that the resolutionName from the request can be parsed. +func (r *Resolver) ValidateResolutionName(ctx context.Context, resolutionName string) error { + return nil +} + // ValidateParams ensures parameters from a request are as expected. func (r *Resolver) ValidateParams(ctx context.Context, params []pipelinev1.Param) error { if r.isDisabled(ctx) { @@ -100,7 +105,7 @@ func (r *Resolver) ValidateParams(ctx context.Context, params []pipelinev1.Param } // Resolve uses the given params to resolve the requested file or resource. -func (r *Resolver) Resolve(ctx context.Context, oParams []pipelinev1.Param) (framework.ResolvedResource, error) { +func (r *Resolver) Resolve(ctx context.Context, resolutionName string, oParams []pipelinev1.Param) (framework.ResolvedResource, error) { if r.isDisabled(ctx) { return nil, errors.New(disabledError) } diff --git a/pkg/resolution/resolver/http/resolver_test.go b/pkg/resolution/resolver/http/resolver_test.go index e2e8e7758be..da2dd2cdf1e 100644 --- a/pkg/resolution/resolver/http/resolver_test.go +++ b/pkg/resolution/resolver/http/resolver_test.go @@ -191,7 +191,7 @@ func TestResolve(t *testing.T) { }) } resolver := Resolver{} - output, err := resolver.Resolve(contextWithConfig(defaultHttpTimeoutValue), params) + output, err := resolver.Resolve(contextWithConfig(defaultHttpTimeoutValue), "", params) if tc.expectedErr != "" { re := regexp.MustCompile(tc.expectedErr) if !re.MatchString(err.Error()) { @@ -226,7 +226,7 @@ func TestResolveNotEnabled(t *testing.T) { var err error resolver := Resolver{} someParams := map[string]string{} - _, err = resolver.Resolve(resolverDisabledContext(), toParams(someParams)) + _, err = resolver.Resolve(resolverDisabledContext(), "", toParams(someParams)) if err == nil { t.Fatalf("expected disabled err") } diff --git a/pkg/resolution/resolver/hub/resolver.go b/pkg/resolution/resolver/hub/resolver.go index abbc19c1985..20d313c8327 100644 --- a/pkg/resolution/resolver/hub/resolver.go +++ b/pkg/resolution/resolver/hub/resolver.go @@ -92,6 +92,11 @@ func (r *Resolver) ValidateParams(ctx context.Context, params []pipelinev1.Param return nil } +// ValidateResolutionName ensures that the resolutionName from the request can be parsed. +func (r *Resolver) ValidateResolutionName(ctx context.Context, resolutionName string) error { + return nil +} + type tektonHubDataResponse struct { YAML string `json:"yaml"` } @@ -109,7 +114,7 @@ type artifactHubResponse struct { } // Resolve uses the given params to resolve the requested file or resource. -func (r *Resolver) Resolve(ctx context.Context, params []pipelinev1.Param) (framework.ResolvedResource, error) { +func (r *Resolver) Resolve(ctx context.Context, resolutionName string, params []pipelinev1.Param) (framework.ResolvedResource, error) { if r.isDisabled(ctx) { return nil, errors.New(disabledError) } diff --git a/pkg/resolution/resolver/hub/resolver_test.go b/pkg/resolution/resolver/hub/resolver_test.go index e6ba327c969..d2ea4d716fe 100644 --- a/pkg/resolution/resolver/hub/resolver_test.go +++ b/pkg/resolution/resolver/hub/resolver_test.go @@ -359,7 +359,7 @@ func TestResolveConstraint(t *testing.T) { ParamCatalog: tt.catalog, ParamType: tt.hubType, } - output, err := resolver.Resolve(contextWithConfig(), toParams(params)) + output, err := resolver.Resolve(contextWithConfig(), "", toParams(params)) if tt.expectedErr != nil { checkExpectedErr(t, tt.expectedErr, err) } else { @@ -496,7 +496,7 @@ func TestResolveDisabled(t *testing.T) { ParamVersion: "bar", ParamCatalog: "baz", } - _, err = resolver.Resolve(resolverDisabledContext(), toParams(params)) + _, err = resolver.Resolve(resolverDisabledContext(), "", toParams(params)) if err == nil { t.Fatalf("expected missing name err") } @@ -597,7 +597,7 @@ func TestResolve(t *testing.T) { ParamType: tc.hubType, } - output, err := resolver.Resolve(contextWithConfig(), toParams(params)) + output, err := resolver.Resolve(contextWithConfig(), "", toParams(params)) if tc.expectedErr != nil { checkExpectedErr(t, tc.expectedErr, err) } else { diff --git a/pkg/resolution/resource/crd_resource.go b/pkg/resolution/resource/crd_resource.go index 90fd7653303..b640b9018e5 100644 --- a/pkg/resolution/resource/crd_resource.go +++ b/pkg/resolution/resource/crd_resource.go @@ -98,7 +98,8 @@ func (r *CRDRequester) createResolutionRequest(ctx context.Context, resolver Res }, }, Spec: v1beta1.ResolutionRequestSpec{ - Params: req.Params(), + Params: req.Params(), + ResolutionName: req.ResolutionName(), }, } appendOwnerReference(rr, req) diff --git a/pkg/resolution/resource/crd_resource_test.go b/pkg/resolution/resource/crd_resource_test.go index da5a06fac38..68716bc94ea 100644 --- a/pkg/resolution/resource/crd_resource_test.go +++ b/pkg/resolution/resource/crd_resource_test.go @@ -75,6 +75,7 @@ params: value: main - name: pathInRepo value: task/git-clone/0.6/git-clone.yaml +resolutionName: "https://foo/bar" `) baseRR := mustParseResolutionRequest(t, ` kind: "ResolutionRequest" @@ -99,6 +100,7 @@ spec: value: "main" - name: "pathInRepo" value: "task/git-clone/0.6/git-clone.yaml" + resolutionName: "https://foo/bar" `) createdRR := baseRR.DeepCopy() // diff --git a/pkg/resolution/resource/name.go b/pkg/resolution/resource/name.go index 051eabc89d0..4f2d23720ea 100644 --- a/pkg/resolution/resource/name.go +++ b/pkg/resolution/resource/name.go @@ -35,7 +35,7 @@ func nameHasher() hash.Hash { // will have the format {prefix}-{hash} where {prefix} is // given and {hash} is nameHasher(base) + nameHasher(param1) + // nameHasher(param2) + ... -func GenerateDeterministicName(prefix, base string, params v1.Params) (string, error) { +func GenerateDeterministicName(prefix, base string, params v1.Params, resolutionName string) (string, error) { hasher := nameHasher() if _, err := hasher.Write([]byte(base)); err != nil { return "", err @@ -67,5 +67,8 @@ func GenerateDeterministicName(prefix, base string, params v1.Params) (string, e } } } + if _, err := hasher.Write([]byte(resolutionName)); err != nil { + return "", err + } return fmt.Sprintf("%s-%x", prefix, hasher.Sum(nil)), nil } diff --git a/pkg/resolution/resource/name_test.go b/pkg/resolution/resource/name_test.go index 1a90a0aaf18..e24711d298d 100644 --- a/pkg/resolution/resource/name_test.go +++ b/pkg/resolution/resource/name_test.go @@ -25,9 +25,10 @@ import ( func TestGenerateDeterministicName(t *testing.T) { type args struct { - prefix string - base string - params []v1.Param + prefix string + base string + params []v1.Param + resolutionName string } golden := args{ prefix: "prefix", @@ -55,6 +56,7 @@ func TestGenerateDeterministicName(t *testing.T) { }, }, }, + resolutionName: "https://foo/bar", } tests := []struct { name string @@ -76,6 +78,13 @@ func TestGenerateDeterministicName(t *testing.T) { }, want: "-6989337ae0757277b806e97e86444ef0", }, + { + name: "only contains resolutionName", + args: args{ + resolutionName: golden.resolutionName, + }, + want: "-dcfaf53735f4a84a3e319e17352940b4", + }, { name: "only contains params", args: args{ @@ -97,12 +106,12 @@ func TestGenerateDeterministicName(t *testing.T) { { name: "contain all fields", args: golden, - want: "prefix-ba2f256f318de7f4154da577c283cb9e", + want: "prefix-ff25bd24688ab610bdc530a5ab3aabbd", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := resource.GenerateDeterministicName(tt.args.prefix, tt.args.base, tt.args.params) + got, err := resource.GenerateDeterministicName(tt.args.prefix, tt.args.base, tt.args.params, tt.args.resolutionName) if (err != nil) != tt.wantErr { t.Errorf("GenerateDeterministicName() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/pkg/resolution/resource/request.go b/pkg/resolution/resource/request.go index 9e0f3e194e7..4ba73a282ab 100644 --- a/pkg/resolution/resource/request.go +++ b/pkg/resolution/resource/request.go @@ -22,15 +22,16 @@ var _ Request = &BasicRequest{} // BasicRequest holds the fields needed to submit a new resource request. type BasicRequest struct { - name string - namespace string - params v1.Params + name string + namespace string + params v1.Params + resolutionName string } // NewRequest returns an instance of a BasicRequest with the given name, // namespace and params. -func NewRequest(name, namespace string, params v1.Params) Request { - return &BasicRequest{name, namespace, params} +func NewRequest(name, namespace string, params v1.Params, resolutionName string) Request { + return &BasicRequest{name, namespace, params, resolutionName} } var _ Request = &BasicRequest{} @@ -49,3 +50,7 @@ func (req *BasicRequest) Namespace() string { func (req *BasicRequest) Params() v1.Params { return req.params } + +func (req *BasicRequest) ResolutionName() string { + return req.resolutionName +} diff --git a/pkg/resolution/resource/request_test.go b/pkg/resolution/resource/request_test.go index e91b1c799b5..9d4d956cab1 100644 --- a/pkg/resolution/resource/request_test.go +++ b/pkg/resolution/resource/request_test.go @@ -27,9 +27,10 @@ import ( func TestNewRequest(t *testing.T) { type args struct { - name string - namespace string - params v1.Params + name string + namespace string + params v1.Params + resolutionName string } type want = args golden := args{ @@ -39,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"}}, }, + resolutionName: "https://foo/bar", } tests := []struct { name string @@ -58,7 +60,7 @@ func TestNewRequest(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - request := resource.NewRequest(tt.args.name, tt.args.namespace, tt.args.params) + request := resource.NewRequest(tt.args.name, tt.args.namespace, tt.args.params, tt.args.resolutionName) if request == nil { t.Errorf("NewRequest() return nil") } @@ -68,6 +70,9 @@ func TestNewRequest(t *testing.T) { if request.Namespace() != tt.want.namespace { t.Errorf("NewRequest().Namespace() = %v, want %v", request.Namespace(), tt.want.namespace) } + if request.ResolutionName() != tt.want.resolutionName { + t.Errorf("NewRequest().ResolutionName() = %v, want %v", request.ResolutionName(), tt.want.resolutionName) + } if d := cmp.Diff(tt.want.params, request.Params()); d != "" { t.Errorf("expected params to match %s", diff.PrintWantGot(d)) } 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/resolution.go b/test/resolution.go index 514988427f2..36d9ebf6875 100644 --- a/test/resolution.go +++ b/test/resolution.go @@ -61,7 +61,8 @@ type Requester struct { // An error to return when a request is submitted. SubmitErr error // Params that should match those on the request in order to return the resolved resource - Params []pipelinev1.Param + Params []pipelinev1.Param + ResolutionName string } // Submit implements resolution.Requester, accepting the name of a @@ -71,6 +72,9 @@ func (r *Requester) Submit(ctx context.Context, resolverName resolution.Resolver if len(r.Params) == 0 { return r.ResolvedResource, r.SubmitErr } + if r.ResolutionName == "" { + return r.ResolvedResource, r.SubmitErr + } reqParams := make(map[string]pipelinev1.ParamValue) for _, p := range req.Params() { reqParams[p.Name] = p.Value @@ -87,6 +91,9 @@ func (r *Requester) Submit(ctx context.Context, resolverName resolution.Resolver if len(wrongParams) > 0 { return nil, errors.New(strings.Join(wrongParams, "; ")) } + if r.ResolutionName != req.ResolutionName() { + return nil, errors.New(fmt.Sprintf("Resolution name did not match. Got %s; Want %s", req.ResolutionName(), r.ResolutionName)) + } return r.ResolvedResource, r.SubmitErr } @@ -130,7 +137,8 @@ type RawRequest struct { // the request namespace Namespace string // the params for the request - Params []pipelinev1.Param + Params []pipelinev1.Param + ResolutionName string } // Request returns a Request interface based on the RawRequest. @@ -152,12 +160,13 @@ type Request struct { var _ resolution.Request = &Request{} // NewRequest creates a mock request that is populated with the given name namespace and params -func NewRequest(name, namespace string, params []pipelinev1.Param) *Request { +func NewRequest(name, namespace string, resolutionName string, params []pipelinev1.Param) *Request { return &Request{ RawRequest: RawRequest{ - Name: name, - Namespace: namespace, - Params: params, + Name: name, + Namespace: namespace, + Params: params, + ResolutionName: resolutionName, }, } } @@ -176,3 +185,7 @@ func (r *Request) Namespace() string { func (r *Request) Params() pipelinev1.Params { return r.RawRequest.Params } + +func (r *Request) ResolutionName() string { + return r.RawRequest.ResolutionName +}