diff --git a/pkg/provider/github/github.go b/pkg/provider/github/github.go index 09af833e6d..2a1dc3b121 100644 --- a/pkg/provider/github/github.go +++ b/pkg/provider/github/github.go @@ -363,6 +363,17 @@ func (v *Provider) SetClient(ctx context.Context, run *params.Run, event *info.E // If Global and Repo level configurations are not provided then lets not override the provider token. if token != "" { event.Provider.Token = token + } else if len(v.RepositoryIDs) > 0 { + // We need to keep the token unscoped until ScopeTokenToListOfRepos so that CreateToken can + // look up the extra repos from the configmap. + // Token is scoped to only the calling repo if no additional scoping repos are configured + // so that no unwanted remote tasks are executed. + ns := info.GetNS(ctx) + scopedToken, err := v.GetAppToken(ctx, run.Clients.Kube, event.Provider.URL, event.InstallationID, ns) + if err != nil { + return fmt.Errorf("failed to scope token to triggering repository: %w", err) + } + event.Provider.Token = scopedToken } } diff --git a/pkg/provider/github/parse_payload.go b/pkg/provider/github/parse_payload.go index b1011cdac1..1523420c95 100644 --- a/pkg/provider/github/parse_payload.go +++ b/pkg/provider/github/parse_payload.go @@ -131,18 +131,29 @@ type Payload struct { Installation struct { ID *int64 `json:"id"` } `json:"installation"` + Repository struct { + ID *int64 `json:"id"` + } `json:"repository"` } -func getInstallationIDFromPayload(payload string) (int64, error) { +func getInstallationAndRepoIDFromPayload(payload string) (int64, int64, error) { var data Payload err := json.Unmarshal([]byte(payload), &data) if err != nil { - return -1, err + return -1, -1, err } + + var installationID int64 = -1 + var repoID int64 = -1 if data.Installation.ID != nil { - return *data.Installation.ID, nil + installationID = *data.Installation.ID + } + + if data.Repository.ID != nil { + repoID = *data.Repository.ID } - return -1, nil + + return installationID, repoID, nil } // ParsePayload will parse the payload and return the event @@ -177,7 +188,7 @@ func (v *Provider) ParsePayload(ctx context.Context, run *params.Run, request *h return nil, err } - installationIDFrompayload, err := getInstallationIDFromPayload(payload) + installationIDFrompayload, repoIDFromPayload, err := getInstallationAndRepoIDFromPayload(payload) if err != nil { return nil, err } @@ -189,6 +200,10 @@ func (v *Provider) ParsePayload(ctx context.Context, run *params.Run, request *h } } + if repoIDFromPayload > 0 { + v.RepositoryIDs = []int64{repoIDFromPayload} + } + eventInt, err := github.ParseWebHook(event.EventType, []byte(payload)) if err != nil { return nil, err @@ -537,6 +552,7 @@ func (v *Provider) handleReRequestEvent(ctx context.Context, event *github.Check // fine because you can't do a rereq without being a github owner? runevent.Sender = event.GetSender().GetLogin() v.userType = event.GetSender().GetType() + v.RepositoryIDs = []int64{event.GetRepo().GetID()} return runevent, nil } runevent.PullRequestNumber = event.GetCheckRun().GetCheckSuite().PullRequests[0].GetNumber() @@ -584,6 +600,7 @@ func (v *Provider) handleCheckSuites(ctx context.Context, event *github.CheckSui // fine because you can't do a rereq without being a github owner? runevent.Sender = event.GetSender().GetLogin() v.userType = event.GetSender().GetType() + v.RepositoryIDs = []int64{event.GetRepo().GetID()} return runevent, nil } runevent.PullRequestNumber = event.GetCheckSuite().PullRequests[0].GetNumber() @@ -699,6 +716,7 @@ func (v *Provider) handleCommitCommentEvent(ctx context.Context, event *github.C runevent.BaseURL = runevent.HeadURL runevent.TriggerTarget = triggertype.Push v.userType = event.GetSender().GetType() + v.RepositoryIDs = []int64{event.GetRepo().GetID()} repo, err := MatchEventURLRepo(ctx, v.Run, runevent, "") if err != nil { diff --git a/pkg/resolve/remote.go b/pkg/resolve/remote.go index 6986a53d24..521cfb0b93 100644 --- a/pkg/resolve/remote.go +++ b/pkg/resolve/remote.go @@ -122,8 +122,8 @@ func resolveRemoteResources(ctx context.Context, rt *matcher.RemoteTasks, types // making sure that the pipeline with same annotation name is not fetched if alreadyFetchedResource(fetchedResourcesForEvent.Pipelines, remotePipeline) { rt.Logger.Debugf("skipping already fetched pipeline %s in annotations on pipelinerun %s", remotePipeline, pipelinerun.GetName()) - // already fetched, then just get the pipeline to add to run specific Resources - pipeline = fetchedResourcesForEvent.Pipelines[remotePipeline] + // already fetched, deep-copy so inlining for this run doesn't mutate the cached original + pipeline = fetchedResourcesForEvent.Pipelines[remotePipeline].DeepCopy() } else { // seems like a new pipeline, fetch it based on name in annotation pipeline, err = rt.GetPipelineFromAnnotationName(ctx, remotePipeline) @@ -181,7 +181,7 @@ func resolveRemoteResources(ctx context.Context, rt *matcher.RemoteTasks, types // if task is already fetched in the event, then just copy the task if alreadyFetchedResource(fetchedResourcesForEvent.Tasks, remoteTask) { rt.Logger.Debugf("skipping already fetched task %s in annotations on pipelinerun %s", remoteTask, pipelinerun.GetName()) - task = fetchedResourcesForEvent.Tasks[remoteTask] + task = fetchedResourcesForEvent.Tasks[remoteTask].DeepCopy() } else { // get the task from annotation name task, err = rt.GetTaskFromAnnotationName(ctx, remoteTask) @@ -213,7 +213,7 @@ func resolveRemoteResources(ctx context.Context, rt *matcher.RemoteTasks, types // if PipelineRef is used then, first resolve pipeline and replace all taskRef{Finally/Task} of Pipeline, then put inlinePipeline in PipelineRun if pipelinerun.Spec.PipelineRef != nil && pipelinerun.Spec.PipelineRef.Resolver == "" { - pipelineResolved := fetchedResourcesForPipelineRun.Pipeline + pipelineResolved := fetchedResourcesForPipelineRun.Pipeline.DeepCopy() turns, err := inlineTasks(pipelineResolved.Spec.Tasks, ropt, fetchedResourcesForPipelineRun) if err != nil { return nil, err diff --git a/pkg/resolve/remote_test.go b/pkg/resolve/remote_test.go index c14f039e76..cd5a08eb2c 100644 --- a/pkg/resolve/remote_test.go +++ b/pkg/resolve/remote_test.go @@ -512,6 +512,84 @@ func TestRemote(t *testing.T) { } } +// Verifies that cached remote pipelines are deep-copied before modification. +// Without deep copy, when the first PipelineRun applies its task annotation, +// it would mutate the cached pipeline and leak that task into the second run. +func TestSharedRemotePipelineCacheNotMutated(t *testing.T) { + taskName := "shared-task" + + pipelineTask := tektonv1.TaskSpec{ + Steps: []tektonv1.Step{ + {Name: "from-pipeline", Image: "alpine", Command: []string{"true"}}, + }, + } + pipelinerunTask := tektonv1.TaskSpec{ + Steps: []tektonv1.Step{ + {Name: "from-pipelinerun", Image: "busybox", Command: []string{"false"}}, + }, + } + + // remote pipeline with a single TaskRef + pipeline := ttkn.MakePipeline("shared-pipeline", []tektonv1.PipelineTask{ + {Name: taskName, TaskRef: &tektonv1.TaskRef{Name: taskName}}, + }, map[string]string{ + apipac.Task: "http://remote/shared-task", + }) + pipelineB, err := yaml.Marshal(pipeline) + assert.NilError(t, err) + + pipelineTaskB, err := ttkn.MakeTaskB(taskName, pipelineTask) + assert.NilError(t, err) + pipelinerunTaskB, err := ttkn.MakeTaskB(taskName, pipelinerunTask) + assert.NilError(t, err) + + remoteURLS := map[string]map[string]string{ + "http://remote/shared-pipeline": {"body": string(pipelineB), "code": "200"}, + "http://remote/shared-task": {"body": string(pipelineTaskB), "code": "200"}, + "http://remote/pr-task": {"body": string(pipelinerunTaskB), "code": "200"}, + } + + // first run: overrides the task via pipelinerun annotation + // second run: same pipeline, no override — should get the original task + pipelineruns := []*tektonv1.PipelineRun{ + ttkn.MakePR("first-run", map[string]string{ + apipac.Pipeline: "http://remote/shared-pipeline", + apipac.Task: "http://remote/pr-task", + }, tektonv1.PipelineRunSpec{ + PipelineRef: &tektonv1.PipelineRef{Name: "shared-pipeline"}, + }), + ttkn.MakePR("second-run", map[string]string{ + apipac.Pipeline: "http://remote/shared-pipeline", + }, tektonv1.PipelineRunSpec{ + PipelineRef: &tektonv1.PipelineRef{Name: "shared-pipeline"}, + }), + } + + observer, _ := zapobserver.New(zap.InfoLevel) + logger := zap.New(observer).Sugar() + ctx, _ := rtesting.SetupFakeContext(t) + httpTestClient := httptesthelper.MakeHTTPTestClient(remoteURLS) + rt := &matcher.RemoteTasks{ + ProviderInterface: &testprovider.TestProviderImp{}, + Logger: logger, + Run: ¶ms.Run{ + Clients: clients.Clients{HTTP: *httpTestClient}, + }, + } + + ret, err := resolveRemoteResources(ctx, rt, TektonTypes{PipelineRuns: pipelineruns}, &Opts{RemoteTasks: true, GenerateName: true}) + assert.NilError(t, err) + assert.Equal(t, len(ret), 2) + + firstStep := ret[0].Spec.PipelineSpec.Tasks[0].TaskSpec.Steps[0] + assert.Equal(t, firstStep.Name, "from-pipelinerun") + assert.Equal(t, firstStep.Image, "busybox") + + secondStep := ret[1].Spec.PipelineSpec.Tasks[0].TaskSpec.Steps[0] + assert.Equal(t, secondStep.Name, "from-pipeline") + assert.Equal(t, secondStep.Image, "alpine") +} + func TestAssembleTaskFQDNs(t *testing.T) { tests := []struct { name string