From f21298b69bec7985b274a084bb14fee2cdbc6709 Mon Sep 17 00:00:00 2001 From: Zaki Shaikh Date: Thu, 26 Feb 2026 12:59:42 +0530 Subject: [PATCH] refactor: move secret creation to reconciler Move git-auth secret creation from startPR (webhook handler) to the reconciler via a new `secret-created` annotation. PipelineRuns are now created with secret-created=false, and the reconciler creates the secret and patches the annotation to true before the PipelineRun proceeds. This decouples secret lifecycle management from the webhook path, enabling the reconciler to handle retries and error recovery. RBAC is updated accordingly: secret create/update permissions move from the controller role to the watcher role. https://issues.redhat.com/browse/SRVKP-10877 Signed-off-by: Zaki Shaikh Assisted-by: Claude Opus 4.6 (via Claude Code) --- config/201-controller-role.yaml | 2 +- config/202-watcher-role.yaml | 2 +- pkg/apis/pipelinesascode/keys/keys.go | 2 + pkg/kubeinteraction/labels.go | 5 + pkg/kubeinteraction/labels_test.go | 2 + pkg/pipelineascode/pipelineascode.go | 52 --- .../pipelineascode_startpr_test.go | 374 +----------------- pkg/reconciler/event.go | 8 + pkg/reconciler/event_test.go | 3 + pkg/reconciler/reconciler.go | 136 +++++-- pkg/reconciler/reconciler_test.go | 195 +++++++++ test/pkg/gitea/test.go | 5 +- 12 files changed, 339 insertions(+), 447 deletions(-) diff --git a/config/201-controller-role.yaml b/config/201-controller-role.yaml index 8e789182f1..d2dfa48cbd 100644 --- a/config/201-controller-role.yaml +++ b/config/201-controller-role.yaml @@ -67,7 +67,7 @@ rules: verbs: ["create"] - apiGroups: [""] resources: ["secrets"] - verbs: ["get", "create", "update", "delete"] + verbs: ["get", "delete"] - apiGroups: ["pipelinesascode.tekton.dev"] resources: ["repositories"] verbs: ["get", "create", "list"] diff --git a/config/202-watcher-role.yaml b/config/202-watcher-role.yaml index 13a8b59a12..f677f08336 100644 --- a/config/202-watcher-role.yaml +++ b/config/202-watcher-role.yaml @@ -67,7 +67,7 @@ metadata: rules: - apiGroups: [""] resources: ["secrets"] - verbs: ["get", "delete"] + verbs: ["get", "create", "update", "delete"] - apiGroups: ["pipelinesascode.tekton.dev"] resources: ["repositories"] verbs: ["get", "list", "update", "watch"] diff --git a/pkg/apis/pipelinesascode/keys/keys.go b/pkg/apis/pipelinesascode/keys/keys.go index 10febd501f..ef25cce505 100644 --- a/pkg/apis/pipelinesascode/keys/keys.go +++ b/pkg/apis/pipelinesascode/keys/keys.go @@ -61,6 +61,8 @@ const ( LogURL = pipelinesascode.GroupName + "/log-url" ExecutionOrder = pipelinesascode.GroupName + "/execution-order" SCMReportingPLRStarted = pipelinesascode.GroupName + "/scm-reporting-plr-started" + SecretCreated = pipelinesascode.GroupName + "/secret-created" + CloneURL = pipelinesascode.GroupName + "/clone-url" // PublicGithubAPIURL default is "https://api.github.com" but it can be overridden by X-GitHub-Enterprise-Host header. PublicGithubAPIURL = "https://api.github.com" GithubApplicationID = "github-application-id" diff --git a/pkg/kubeinteraction/labels.go b/pkg/kubeinteraction/labels.go index 965cc05f7e..8abcceba50 100644 --- a/pkg/kubeinteraction/labels.go +++ b/pkg/kubeinteraction/labels.go @@ -55,6 +55,7 @@ func AddLabelsAndAnnotations(event *info.Event, pipelineRun *tektonv1.PipelineRu keys.SourceBranch: event.HeadBranch, keys.Repository: repo.GetName(), keys.GitProvider: providerConfig.Name, + keys.SecretCreated: "false", keys.ControllerInfo: fmt.Sprintf(`{"name":"%s","configmap":"%s","secret":"%s", "gRepo": "%s"}`, paramsinfo.Controller.Name, paramsinfo.Controller.Configmap, paramsinfo.Controller.Secret, paramsinfo.Controller.GlobalRepository), } @@ -82,6 +83,10 @@ func AddLabelsAndAnnotations(event *info.Event, pipelineRun *tektonv1.PipelineRu annotations[keys.TargetProjectID] = strconv.Itoa(int(event.TargetProjectID)) } + if event.CloneURL != "" { + annotations[keys.CloneURL] = event.CloneURL + } + if value, ok := pipelineRun.GetObjectMeta().GetAnnotations()[keys.CancelInProgress]; ok { labels[keys.CancelInProgress] = value } diff --git a/pkg/kubeinteraction/labels_test.go b/pkg/kubeinteraction/labels_test.go index cca0208fde..9ee91973cf 100644 --- a/pkg/kubeinteraction/labels_test.go +++ b/pkg/kubeinteraction/labels_test.go @@ -24,6 +24,7 @@ func TestAddLabelsAndAnnotations(t *testing.T) { event.SHAURL = "https://url/sha" event.HeadBranch = "pr_branch" event.HeadURL = "https://url/pr" + event.CloneURL = "https://url/clone" type args struct { event *info.Event @@ -81,6 +82,7 @@ func TestAddLabelsAndAnnotations(t *testing.T) { assert.Equal(t, tt.args.pipelineRun.Annotations[keys.SourceRepoURL], tt.args.event.HeadURL) assert.Equal(t, tt.args.pipelineRun.Annotations[keys.ControllerInfo], fmt.Sprintf(`{"name":"%s","configmap":"%s","secret":"%s", "gRepo": "%s"}`, tt.args.controllerInfo.Name, tt.args.controllerInfo.Configmap, tt.args.controllerInfo.Secret, tt.args.controllerInfo.GlobalRepository)) + assert.Equal(t, tt.args.pipelineRun.Annotations[keys.CloneURL], tt.args.event.CloneURL) }) } } diff --git a/pkg/pipelineascode/pipelineascode.go b/pkg/pipelineascode/pipelineascode.go index 30eda6c8a7..923f34e64b 100644 --- a/pkg/pipelineascode/pipelineascode.go +++ b/pkg/pipelineascode/pipelineascode.go @@ -20,10 +20,8 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" providerstatus "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/status" - "github.com/openshift-pipelines/pipelines-as-code/pkg/secrets" tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "go.uber.org/zap" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -206,7 +204,6 @@ func (p *PacRun) Run(ctx context.Context) error { } func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.PipelineRun, error) { - var gitAuthSecretName string prName := match.PipelineRun.GetName() if prName == "" { prName = match.PipelineRun.GetGenerateName() @@ -218,37 +215,6 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi p.event.BaseBranch, ) - // Automatically create a secret with the token to be reused by git-clone task - if p.pacInfo.SecretAutoCreation { - if annotation, ok := match.PipelineRun.GetAnnotations()[keys.GitAuthSecret]; ok { - gitAuthSecretName = annotation - p.debugf("startPR: using git auth secret from annotation=%s", gitAuthSecretName) - } else { - return nil, fmt.Errorf("cannot get annotation %s as set on PR", keys.GitAuthSecret) - } - - authSecret, err := secrets.MakeBasicAuthSecret(p.event, gitAuthSecretName) - if err != nil { - return nil, fmt.Errorf("making basic auth secret: %s has failed: %w ", gitAuthSecretName, err) - } - - if err = p.k8int.CreateSecret(ctx, match.Repo.GetNamespace(), authSecret); err != nil { - // NOTE: Handle AlreadyExists errors due to etcd/API server timing issues. - // Investigation found: slow etcd response causes API server retry, resulting in - // duplicate secret creation attempts for the same PR. This is a workaround, not - // designed behavior - reuse existing secret to prevent PipelineRun failure. - if errors.IsAlreadyExists(err) { - msg := fmt.Sprintf("Secret %s already exists in namespace %s, reusing existing secret", - authSecret.GetName(), match.Repo.GetNamespace()) - p.eventEmitter.EmitMessage(match.Repo, zap.WarnLevel, "RepositorySecretReused", msg) - } else { - return nil, fmt.Errorf("creating basic auth secret: %s has failed: %w ", authSecret.GetName(), err) - } - } else { - p.debugf("startPR: created git auth secret %s in namespace %s", authSecret.GetName(), match.Repo.GetNamespace()) - } - } - // Add labels and annotations to pipelinerun err := kubeinteraction.AddLabelsAndAnnotations(p.event, match.PipelineRun, match.Repo, p.vcx.GetConfig(), p.run) if err != nil { @@ -269,29 +235,11 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi pr, err := p.run.Clients.Tekton.TektonV1().PipelineRuns(match.Repo.GetNamespace()).Create(ctx, match.PipelineRun, metav1.CreateOptions{}) if err != nil { - // cleanup the gitauth secret because ownerRef isn't set when the pipelineRun creation failed - if p.pacInfo.SecretAutoCreation { - if errDelSec := p.k8int.DeleteSecret(ctx, p.logger, match.Repo.GetNamespace(), gitAuthSecretName); errDelSec != nil { - // don't overshadow the pipelineRun creation error, just log - p.logger.Errorf("removing auto created secret: %s in namespace %s has failed: %w ", gitAuthSecretName, match.Repo.GetNamespace(), errDelSec) - } - } // we need to make difference between markdown error and normal error that goes to namespace/controller stream return nil, fmt.Errorf("creating pipelinerun %s in namespace %s has failed.\n\nTekton Controller has reported this error: ```%w``` ", match.PipelineRun.GetGenerateName(), match.Repo.GetNamespace(), err) } - // update ownerRef of secret with pipelineRun, so that it gets cleanedUp with pipelineRun - if p.pacInfo.SecretAutoCreation { - err := p.k8int.UpdateSecretWithOwnerRef(ctx, p.logger, pr.Namespace, gitAuthSecretName, pr) - if err != nil { - // we still return the created PR with error, and allow caller to decide what to do with the PR, and avoid - // unneeded SIGSEGV's - return pr, fmt.Errorf("cannot update pipelinerun %s with ownerRef: %w", pr.GetGenerateName(), err) - } - p.debugf("startPR: updated secret ownerRef for pipelinerun=%s secret=%s", pr.GetName(), gitAuthSecretName) - } - // Create status with the log url p.logger.Infof("PipelineRun %s has been created in namespace %s with status %s for SHA: %s Target Branch: %s", pr.GetName(), match.Repo.GetNamespace(), pr.Spec.Status, p.event.SHA, p.event.BaseBranch) diff --git a/pkg/pipelineascode/pipelineascode_startpr_test.go b/pkg/pipelineascode/pipelineascode_startpr_test.go index d80718f50f..b2bebfcc76 100644 --- a/pkg/pipelineascode/pipelineascode_startpr_test.go +++ b/pkg/pipelineascode/pipelineascode_startpr_test.go @@ -218,15 +218,13 @@ func setupProviderForTest(cs *params.Run, logger *zap.SugaredLogger, fakeclient } // createTestMatch creates a Match object for testing startPR. -func createTestMatch(withSecret bool, concurrencyLimit *int) matcher.Match { +func createTestMatch(concurrencyLimit *int) matcher.Match { namespace := "test-namespace" prName := "test-pr-" annotations := make(map[string]string) labels := make(map[string]string) - if withSecret { - annotations[keys.GitAuthSecret] = "test-git-secret" - } + annotations[keys.GitAuthSecret] = "test-git-secret" pr := &pipelinev1.PipelineRun{ ObjectMeta: metav1.ObjectMeta{ @@ -263,7 +261,7 @@ func TestStartPR(t *testing.T) { fixture := setupStartPRTestDefault(t) defer fixture.teardown() - match := createTestMatch(true, nil) + match := createTestMatch(nil) pr, err := fixture.pac.startPR(fixture.ctx, match) @@ -280,77 +278,6 @@ func TestStartPR(t *testing.T) { assert.Assert(t, hasLogURL, "LogURL annotation should be set") } -func TestStartPR_MissingSecretAnnotation(t *testing.T) { - fixture := setupStartPRTestDefault(t) - defer fixture.teardown() - - match := createTestMatch(false, nil) // no secret annotation - - pr, err := fixture.pac.startPR(fixture.ctx, match) - - assert.Assert(t, pr == nil) - assert.ErrorContains(t, err, "cannot get annotation") - assert.ErrorContains(t, err, keys.GitAuthSecret) -} - -func TestStartPR_SecretCreationScenarios(t *testing.T) { - tests := []struct { - name string - createSecretError error - secretAutoCreate bool - expectSuccess bool - expectErrorMsg []string - }{ - { - name: "secret already exists - should succeed with warning", - createSecretError: errors.NewAlreadyExists(schema.GroupResource{Group: "", Resource: "secrets"}, "test-git-secret"), - secretAutoCreate: true, - expectSuccess: true, - expectErrorMsg: nil, - }, - { - name: "secret creation failure - should fail", - createSecretError: fmt.Errorf("connection timeout"), - secretAutoCreate: true, - expectSuccess: false, - expectErrorMsg: []string{"creating basic auth secret", "has failed"}, - }, - { - name: "secret auto-creation disabled - should succeed without creating secret", - createSecretError: nil, - secretAutoCreate: false, - expectSuccess: true, - expectErrorMsg: nil, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - config := defaultStartPRTestConfig() - config.createSecretError = tt.createSecretError - config.secretAutoCreation = tt.secretAutoCreate - fixture := setupStartPRTestWithConfig(t, config) - defer fixture.teardown() - - match := createTestMatch(true, nil) - - pr, err := fixture.pac.startPR(fixture.ctx, match) - - if tt.expectSuccess { - assert.NilError(t, err) - assert.Assert(t, pr != nil, "PipelineRun should be created") - assert.Equal(t, pr.GetNamespace(), "test-namespace") - } else { - assert.Assert(t, pr == nil, "PipelineRun should be nil on error") - assert.Assert(t, err != nil, "Error should be returned") - for _, errMsg := range tt.expectErrorMsg { - assert.ErrorContains(t, err, errMsg) - } - } - }) - } -} - func TestStartPR_AnnotationAndLabelPropagation(t *testing.T) { tests := []struct { name string @@ -389,7 +316,7 @@ func TestStartPR_AnnotationAndLabelPropagation(t *testing.T) { fixture := setupStartPRTestDefault(t) defer fixture.teardown() - match := createTestMatch(true, nil) + match := createTestMatch(nil) if tt.setupMatch != nil { tt.setupMatch(&match) @@ -478,7 +405,7 @@ func TestStartPR_ConcurrencyLimitBehavior(t *testing.T) { fixture := setupStartPRTestDefault(t) defer fixture.teardown() - match := createTestMatch(true, tt.concurrencyLimit) + match := createTestMatch(tt.concurrencyLimit) pr, err := fixture.pac.startPR(fixture.ctx, match) @@ -502,227 +429,13 @@ func TestStartPR_ConcurrencyLimitBehavior(t *testing.T) { } } -func TestStartPR_CreationFailureWithSecretCleanup(t *testing.T) { - ctx, _ := rtesting.SetupFakeContext(t) - observer, _ := zapobserver.New(zap.InfoLevel) - logger := zap.New(observer).Sugar() - - stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{ - Namespaces: []*corev1.Namespace{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "test-namespace", - }, - }, - }, - }) - - creationFailed := false - stdata.Pipeline.PrependReactor("create", "pipelineruns", func(_ k8stesting.Action) (handled bool, ret runtime.Object, err error) { - if !creationFailed { - creationFailed = true - - return true, nil, fmt.Errorf("namespace quota exceeded") - } - - return false, nil, nil - }) - - cs := ¶ms.Run{ - Clients: clients.Clients{ - PipelineAsCode: stdata.PipelineAsCode, - Log: logger, - Kube: stdata.Kube, - Tekton: stdata.Pipeline, - }, - Info: info.Info{ - Controller: &info.ControllerInfo{ - Name: "default", - Configmap: "pipelines-as-code", - Secret: "pipelines-as-code-secret", - }, - }, - } - cs.Clients.SetConsoleUI(consoleui.FallBackConsole{}) - - event := &info.Event{ - SHA: "test-sha", - Organization: "test-org", - Repository: "test-repo", - URL: "https://test.com/repo", - HeadBranch: "test-branch", - BaseBranch: "main", - Sender: "test-user", - EventType: "pull_request", - TriggerTarget: "pull_request", - PullRequestNumber: 123, - Provider: &info.Provider{ - Token: "test-token", - User: "git", - URL: "https://api.github.com", - }, - } - - match := createTestMatch(true, nil) - - kint := &kitesthelper.KinterfaceTest{ - ConsoleURL: "https://console.test", - } - - pacInfo := &info.PacOpts{ - Settings: settings.Settings{ - SecretAutoCreation: true, - }, - } - - fakeclient, _, _, teardown := ghtesthelper.SetupGH() - defer teardown() - - vcx := setupProviderForTest(cs, logger, fakeclient, pacInfo) - p := NewPacs(event, vcx, cs, pacInfo, kint, logger, nil) - - pr, err := p.startPR(ctx, match) - - assert.Assert(t, pr == nil, "PipelineRun should be nil on creation failure") - assert.ErrorContains(t, err, "creating pipelinerun") - assert.ErrorContains(t, err, "has failed") - assert.Assert(t, kint.SecretDeleted, "Secret should have been deleted on PR creation failure") -} - -func TestStartPR_SecretCleanupError(t *testing.T) { - ctx, _ := rtesting.SetupFakeContext(t) - observer, log := zapobserver.New(zap.InfoLevel) - logger := zap.New(observer).Sugar() - - stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{ - Namespaces: []*corev1.Namespace{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "test-namespace", - }, - }, - }, - }) - - stdata.Pipeline.PrependReactor("create", "pipelineruns", func(_ k8stesting.Action) (handled bool, ret runtime.Object, err error) { - return true, nil, fmt.Errorf("api server unavailable") - }) - - cs := ¶ms.Run{ - Clients: clients.Clients{ - PipelineAsCode: stdata.PipelineAsCode, - Log: logger, - Kube: stdata.Kube, - Tekton: stdata.Pipeline, - }, - Info: info.Info{ - Controller: &info.ControllerInfo{ - Name: "default", - Configmap: "pipelines-as-code", - Secret: "pipelines-as-code-secret", - }, - }, - } - cs.Clients.SetConsoleUI(consoleui.FallBackConsole{}) - - event := &info.Event{ - SHA: "test-sha", - Organization: "test-org", - Repository: "test-repo", - URL: "https://test.com/repo", - HeadBranch: "test-branch", - BaseBranch: "main", - Sender: "test-user", - EventType: "pull_request", - TriggerTarget: "pull_request", - PullRequestNumber: 123, - Provider: &info.Provider{ - Token: "test-token", - User: "git", - URL: "https://api.github.com", - }, - } - - match := createTestMatch(true, nil) - - // Mock that fails both PR creation AND secret deletion - kint := &kitesthelper.KinterfaceTest{ - ConsoleURL: "https://console.test", - DeleteSecretError: fmt.Errorf("failed to delete secret"), - } - - pacInfo := &info.PacOpts{ - Settings: settings.Settings{ - SecretAutoCreation: true, - }, - } - - fakeclient, _, _, teardown := ghtesthelper.SetupGH() - defer teardown() - - vcx := setupProviderForTest(cs, logger, fakeclient, pacInfo) - p := NewPacs(event, vcx, cs, pacInfo, kint, logger, nil) - - pr, err := p.startPR(ctx, match) - - assert.Assert(t, pr == nil, "PipelineRun should be nil") - assert.ErrorContains(t, err, "creating pipelinerun") - assert.ErrorContains(t, err, "has failed") - assert.Assert(t, kint.SecretDeleted, "Secret deletion should have been attempted") - - logEntries := log.FilterMessageSnippet("removing auto created secret").TakeAll() - assert.Assert(t, len(logEntries) > 0, "Should have logged secret deletion error") -} - -func TestStartPR_SecretOwnerRefUpdateErrors(t *testing.T) { - tests := []struct { - name string - updateSecretError error - expectErrorMsg []string - }{ - { - name: "generic update failure - should return PR with error", - updateSecretError: fmt.Errorf("failed to update ownerRef"), - expectErrorMsg: []string{"cannot update pipelinerun", "with ownerRef"}, - }, - { - name: "conflict error - should return PR with error", - updateSecretError: errors.NewConflict(schema.GroupResource{Group: "", Resource: "secrets"}, "test-git-secret", fmt.Errorf("resource version mismatch")), - expectErrorMsg: []string{"cannot update pipelinerun", "with ownerRef"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - config := defaultStartPRTestConfig() - config.updateSecretError = tt.updateSecretError - fixture := setupStartPRTestWithConfig(t, config) - defer fixture.teardown() - - match := createTestMatch(true, nil) - - pr, err := fixture.pac.startPR(fixture.ctx, match) - - assert.Assert(t, pr != nil, "PipelineRun should still be returned despite ownerRef update failure") - assert.Assert(t, err != nil, "Error should be returned") - - for _, errMsg := range tt.expectErrorMsg { - assert.ErrorContains(t, err, errMsg) - } - - assert.Equal(t, pr.GetNamespace(), "test-namespace") - assert.Assert(t, pr.GetName() != "" || pr.GetGenerateName() != "", "PR should have a name") - }) - } -} - func TestStartPR_StatusCreationFailure(t *testing.T) { config := defaultStartPRTestConfig() config.createStatusErorring = true fixture := setupStartPRTestWithConfig(t, config) defer fixture.teardown() - match := createTestMatch(true, nil) + match := createTestMatch(nil) pr, err := fixture.pac.startPR(fixture.ctx, match) @@ -737,7 +450,7 @@ func TestStartPR_GitHubAppLogURLHandling(t *testing.T) { fixture := setupStartPRTestDefault(t) defer fixture.teardown() - match := createTestMatch(true, nil) + match := createTestMatch(nil) // Add InstallationID annotation to simulate GitHub App match.PipelineRun.Annotations[keys.InstallationID] = "12345" @@ -865,7 +578,7 @@ func TestStartPR_PatchBehavior(t *testing.T) { vcx := setupProviderForTest(cs, logger, fakeclient, pacInfo) p := NewPacs(event, vcx, cs, pacInfo, kint, logger, nil) - match := createTestMatch(true, nil) + match := createTestMatch(nil) pr, err := p.startPR(ctx, match) assert.Assert(t, pr != nil, "PipelineRun should be returned even when patch fails") @@ -881,7 +594,7 @@ func TestStartPR_PatchBehavior(t *testing.T) { fixture := setupStartPRTestDefault(t) defer fixture.teardown() - match := createTestMatch(true, nil) + match := createTestMatch(nil) pr, err := fixture.pac.startPR(fixture.ctx, match) assert.NilError(t, err) @@ -965,7 +678,7 @@ func TestStartPR_ConcurrentCreation(t *testing.T) { numConcurrent := 5 matches := make([]matcher.Match, numConcurrent) for i := range numConcurrent { - matches[i] = createTestMatch(true, nil) + matches[i] = createTestMatch(nil) // Use actual names instead of GenerateName for fake client compatibility matches[i].PipelineRun.Name = fmt.Sprintf("test-pr-%d", i) matches[i].PipelineRun.GenerateName = "" @@ -1002,70 +715,3 @@ func TestStartPR_ConcurrentCreation(t *testing.T) { assert.Equal(t, successCount, numConcurrent, "All concurrent PipelineRuns should succeed with proper isolation, got %d/%d (failures: %d)", successCount, numConcurrent, failureCount) t.Logf("Successfully created %d/%d concurrent PipelineRuns", successCount, numConcurrent) } - -func TestStartPR_ConcurrentWithSameSecret(t *testing.T) { - cs, event, logger, ctx, fakeclient, teardown := setupStartPRTest(t) - defer teardown() - - // Track secret creation attempts with a mock that simulates AlreadyExists after first creation - kintWithTracking := &KinterfaceTestWithSecretTracking{ - KinterfaceTest: kitesthelper.KinterfaceTest{ - ConsoleURL: "https://console.test", - }, - } - - pacInfo := &info.PacOpts{ - Settings: settings.Settings{ - SecretAutoCreation: true, - }, - } - - vcx := setupProviderForTest(cs, logger, fakeclient, pacInfo) - - // Create multiple matches that will try to create the same secret - // This simulates the real-world scenario where multiple PRs might trigger simultaneously - numConcurrent := 3 - matches := make([]matcher.Match, numConcurrent) - for i := range numConcurrent { - matches[i] = createTestMatch(true, nil) - // Use the same secret name for all to test race condition handling - matches[i].PipelineRun.Annotations[keys.GitAuthSecret] = "shared-git-secret" - // Use actual names instead of GenerateName for fake client compatibility - matches[i].PipelineRun.Name = fmt.Sprintf("test-pr-shared-%d", i) - matches[i].PipelineRun.GenerateName = "" - } - - results := runConcurrentStartPR(t, numConcurrent, func(idx int) (*pipelinev1.PipelineRun, error) { - p := NewPacs(event, vcx, cs, pacInfo, kintWithTracking, logger, nil) - return p.startPR(ctx, matches[idx]) - }) - - successCount := 0 - failureCount := 0 - for range numConcurrent { - res := <-results - // All should succeed because AlreadyExists errors are handled gracefully - if res.err == nil && res.pr != nil { - successCount++ - assert.Equal(t, res.pr.GetNamespace(), "test-namespace") - - secretName, ok := res.pr.GetAnnotations()[keys.GitAuthSecret] - assert.Assert(t, ok, "GitAuthSecret annotation should be present") - assert.Equal(t, secretName, "shared-git-secret") - - _, hasState := res.pr.GetAnnotations()[keys.State] - assert.Assert(t, hasState, fmt.Sprintf("State should be set for PR %d", res.idx)) - } else { - failureCount++ - t.Logf("PipelineRun %d failed: %v", res.idx, res.err) - } - } - - // With the AlreadyExists handling, all should succeed - assert.Equal(t, successCount, numConcurrent, "All concurrent PipelineRuns should succeed with AlreadyExists handling, got %d/%d (failures: %d)", successCount, numConcurrent, failureCount) - - // Verify that secret creation was attempted multiple times (indicating race condition) - attempts := kintWithTracking.secretCreationCount.Load() - assert.Assert(t, attempts >= 1, "Secret creation should have been attempted at least once, got %d attempts", attempts) - t.Logf("Successfully created %d/%d concurrent PipelineRuns with shared secret (%d creation attempts)", successCount, numConcurrent, attempts) -} diff --git a/pkg/reconciler/event.go b/pkg/reconciler/event.go index 196289b4d7..c75536a904 100644 --- a/pkg/reconciler/event.go +++ b/pkg/reconciler/event.go @@ -82,6 +82,10 @@ func buildEventFromPipelineRun(pr *tektonv1.PipelineRun) *info.Event { event.TriggerTarget = triggertype.PullRequest } + if cloneURL, ok := prAnno[keys.CloneURL]; ok { + event.CloneURL = cloneURL + } + // GitHub if installationID, ok := prAnno[keys.InstallationID]; ok { id, _ := strconv.Atoi(installationID) @@ -89,6 +93,10 @@ func buildEventFromPipelineRun(pr *tektonv1.PipelineRun) *info.Event { } if gheURL, ok := prAnno[keys.GHEURL]; ok { event.GHEURL = gheURL + if event.Provider == nil { + event.Provider = &info.Provider{} + } + event.Provider.URL = gheURL } // GitLab diff --git a/pkg/reconciler/event_test.go b/pkg/reconciler/event_test.go index 72eb3fc6a2..ba8cd7f52a 100644 --- a/pkg/reconciler/event_test.go +++ b/pkg/reconciler/event_test.go @@ -28,6 +28,7 @@ func TestBuildEventFromPipelineRun(t *testing.T) { GHEURL: "http://ghe", SourceProjectID: 1234, TargetProjectID: 2345, + CloneURL: "https://url/clone", } tests := []struct { name string @@ -50,6 +51,7 @@ func TestBuildEventFromPipelineRun(t *testing.T) { Annotations: map[string]string{ keys.ShaTitle: "sha-title", keys.ShaURL: "sha-url", + keys.CloneURL: "https://url/clone", // github keys.InstallationID: "12345678", @@ -80,6 +82,7 @@ func TestBuildEventFromPipelineRun(t *testing.T) { assert.Equal(t, event.SourceProjectID, tt.event.SourceProjectID) assert.Equal(t, event.TargetProjectID, tt.event.TargetProjectID) assert.Equal(t, event.PullRequestNumber, tt.event.PullRequestNumber) + assert.Equal(t, event.CloneURL, tt.event.CloneURL) }) } } diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 9314a85d4a..c64fcfb6b1 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -11,6 +11,7 @@ import ( tektonv1lister "github.com/tektoncd/pipeline/pkg/client/listers/pipeline/v1" "go.uber.org/zap" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "knative.dev/pkg/logging" pkgreconciler "knative.dev/pkg/reconciler" @@ -33,6 +34,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" providerstatus "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/status" queuepkg "github.com/openshift-pipelines/pipelines-as-code/pkg/queue" + "github.com/openshift-pipelines/pipelines-as-code/pkg/secrets" ) // Reconciler implements controller.Reconciler for PipelineRun resources. @@ -77,6 +79,46 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun return nil } + repoName := pr.GetAnnotations()[keys.Repository] + repo, err := r.repoLister.Repositories(pr.Namespace).Get(repoName) + if err != nil { + return fmt.Errorf("failed to get repository CR: %w", err) + } + + // use same pac opts across the reconciliation + pacInfo := r.run.Info.GetPacOpts() + + // If we have a controllerInfo annotation, then we need to get the + // configmap configuration for it + // + // The annotation is a json string with a label, the pac controller + // configmap and the GitHub app secret . + // + // We always assume the controller is in the same namespace as the original + // controller but that may changes + if controllerInfo, ok := pr.GetAnnotations()[keys.ControllerInfo]; ok { + var parsedControllerInfo *info.ControllerInfo + if err := json.Unmarshal([]byte(controllerInfo), &parsedControllerInfo); err != nil { + return fmt.Errorf("failed to parse controllerInfo: %w", err) + } + r.run.Info.Controller = parsedControllerInfo + } else { + r.run.Info.Controller = info.GetControllerInfoFromEnvOrDefault() + } + + if secretCreated, ok := pr.GetAnnotations()[keys.SecretCreated]; ok && secretCreated == "false" && pacInfo.SecretAutoCreation { + // if secret creation is true then return anyway from createSecretForPipelineRun function + // because it patches the PipelineRun with the secretCreated annotation so after the + // patch success we will get another reconciliation call for the same pipelineRun. + // Note: only return error if the error is not related to secret creation otherwise we would interrupt other operations. + err := r.createSecretForPipelineRun(ctx, logger, pr, repo) + if err != nil && strings.Contains(err.Error(), "creating basic auth secret") { + return fmt.Errorf("failed to create secret for pipelineRun %s/%s: %w", pr.GetNamespace(), pr.GetName(), err) + } + + logger.Errorf("failed to create secret for pipelineRun %s/%s: %v", pr.GetNamespace(), pr.GetName(), err) + } + reason := "" if len(pr.Status.GetConditions()) > 0 { reason = pr.Status.GetConditions()[0].GetReason() @@ -92,11 +134,6 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun if reason == string(tektonv1.PipelineRunReasonRunning) && !startReported { logger.Infof("pipelineRun %s/%s is running but not yet reported to provider, updating status", pr.GetNamespace(), pr.GetName()) - repoName := pr.GetAnnotations()[keys.Repository] - repo, err := r.repoLister.Repositories(pr.Namespace).Get(repoName) - if err != nil { - return fmt.Errorf("failed to get repository CR: %w", err) - } return r.updatePipelineRunToInProgress(ctx, logger, repo, pr) } logger.Debugf("pipelineRun %s/%s condition not met: reason='%s', startReported=%v", pr.GetNamespace(), pr.GetName(), reason, startReported) @@ -118,24 +155,6 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun return nil } - // If we have a controllerInfo annotation, then we need to get the - // configmap configuration for it - // - // The annotation is a json string with a label, the pac controller - // configmap and the GitHub app secret . - // - // We always assume the controller is in the same namespace as the original - // controller but that may changes - if controllerInfo, ok := pr.GetAnnotations()[keys.ControllerInfo]; ok { - var parsedControllerInfo *info.ControllerInfo - if err := json.Unmarshal([]byte(controllerInfo), &parsedControllerInfo); err != nil { - return fmt.Errorf("failed to parse controllerInfo: %w", err) - } - r.run.Info.Controller = parsedControllerInfo - } else { - r.run.Info.Controller = info.GetControllerInfoFromEnvOrDefault() - } - ctx = info.StoreCurrentControllerName(ctx, r.run.Info.Controller.Name) logFields := []interface{}{ @@ -165,9 +184,6 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun logger.Infof("pipelineRun %v/%v is done, reconciling to report status! ", pr.GetNamespace(), pr.GetName()) r.eventEmitter.SetLogger(logger) - // use same pac opts across the reconciliation - pacInfo := r.run.Info.GetPacOpts() - detectedProvider, event, err := r.detectProvider(ctx, logger, pr) if err != nil { msg := fmt.Sprintf("detectProvider: %v", err) @@ -184,6 +200,67 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun return nil } +func (r *Reconciler) createSecretForPipelineRun(ctx context.Context, logger *zap.SugaredLogger, pr *tektonv1.PipelineRun, repo *v1alpha1.Repository) error { + var gitAuthSecretName string + // as GitAuthSecret annotation is added to the PipelineRun in getPipelineRunsFromRepo function + // we expect the name here otherwise error out + if annotation, ok := pr.GetAnnotations()[keys.GitAuthSecret]; ok { + gitAuthSecretName = annotation + logger.Debugf("using git auth secret from annotation=%s for pipelineRun %s/%s", gitAuthSecretName, pr.GetNamespace(), pr.GetName()) + } else { + return fmt.Errorf("cannot get annotation %s as set on pipelineRun %s/%s", keys.GitAuthSecret, pr.GetNamespace(), pr.GetName()) + } + + // here we don't need provider but we need to call initGitProviderClient because we need event + // built with user and token so secret can be built upon user and token + _, event, err := r.initGitProviderClient(ctx, logger, repo, pr) + if err != nil { + return fmt.Errorf("cannot initialize git provider client: %w", err) + } + + authSecret, err := secrets.MakeBasicAuthSecret(event, gitAuthSecretName) + if err != nil { + return fmt.Errorf("making basic auth secret: %s has failed: %w ", gitAuthSecretName, err) + } + + if err = r.kinteract.CreateSecret(ctx, repo.GetNamespace(), authSecret); err != nil { + // NOTE: Handle AlreadyExists errors due to etcd/API server timing issues. + // Investigation found: slow etcd response causes API server retry, resulting in + // duplicate secret creation attempts for the same PR. This is a workaround, not + // designed behavior - reuse existing secret to prevent PipelineRun failure. + if errors.IsAlreadyExists(err) { + msg := fmt.Sprintf("Secret %s already exists in namespace %s, reusing existing secret", + authSecret.GetName(), repo.GetNamespace()) + r.eventEmitter.EmitMessage(nil, zap.WarnLevel, "RepositorySecretReused", msg) + } else { + return fmt.Errorf("creating basic auth secret: %s has failed: %w ", authSecret.GetName(), err) + } + } else { + logger.Debugf("created git auth secret %s in namespace %s for pipelineRun %s/%s", authSecret.GetName(), pr.GetNamespace(), pr.GetName()) + } + + if err = r.kinteract.UpdateSecretWithOwnerRef(ctx, logger, pr.Namespace, gitAuthSecretName, pr); err != nil { + return fmt.Errorf("cannot update secret %s with ownerRef to pipelinerun %s: %w", gitAuthSecretName, pr.GetName(), err) + } + logger.Debugf("updated secret ownerRef for pipelinerun=%s secret=%s", pr.GetName(), gitAuthSecretName) + + patchAnnotations := map[string]any{ + "metadata": map[string]any{ + "annotations": map[string]string{ + keys.SecretCreated: "true", + }, + }, + } + + _, err = action.PatchPipelineRun(ctx, logger, "patching annotations.secretCreated", r.run.Clients.Tekton, pr, patchAnnotations) + if err != nil { + return fmt.Errorf("failed to patch pipelinerun %s annotations.secretCreated: %w", pr.GetName(), err) + } + + logger.Debugf("patched annotations.secretCreated for pipelinerun=%s", pr.GetName()) + return nil +} + func (r *Reconciler) reportFinalStatus(ctx context.Context, logger *zap.SugaredLogger, pacInfo *info.PacOpts, event *info.Event, pr *tektonv1.PipelineRun, provider provider.Interface) (*v1alpha1.Repository, error) { repoName := pr.GetAnnotations()[keys.Repository] repo, err := r.repoLister.Repositories(pr.Namespace).Get(repoName) @@ -349,8 +426,11 @@ func (r *Reconciler) initGitProviderClient(ctx context.Context, logger *zap.Suga } else { // secretNS is needed when git provider is other than Github App. secretNS := repo.GetNamespace() - if repo.Spec.GitProvider != nil && repo.Spec.GitProvider.Secret == nil && r.globalRepo != nil && r.globalRepo.Spec.GitProvider != nil && r.globalRepo.Spec.GitProvider.Secret != nil { - secretNS = r.globalRepo.GetNamespace() + if r.globalRepo, err = r.repoLister.Repositories(r.run.Info.Kube.Namespace).Get(r.run.Info.Controller.GlobalRepository); err == nil && r.globalRepo != nil { + if repo.Spec.GitProvider != nil && repo.Spec.GitProvider.Secret == nil && r.globalRepo.Spec.GitProvider != nil && r.globalRepo.Spec.GitProvider.Secret != nil { + r.secretNS = r.globalRepo.GetNamespace() + } + repo.Spec.Merge(r.globalRepo.Spec) } secretFromRepo := pac.SecretFromRepository{ diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 8d5e3abf9c..7375747330 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "io" + "maps" "net/http" "strings" "testing" @@ -13,6 +14,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" "github.com/openshift-pipelines/pipelines-as-code/pkg/consoleui" + "github.com/openshift-pipelines/pipelines-as-code/pkg/events" "github.com/openshift-pipelines/pipelines-as-code/pkg/kubeinteraction" "github.com/openshift-pipelines/pipelines-as-code/pkg/params" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients" @@ -32,8 +34,11 @@ import ( "gotest.tools/v3/assert" "gotest.tools/v3/golden" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + k8stesting "k8s.io/client-go/testing" knativeapi "knative.dev/pkg/apis" knativeduckv1 "knative.dev/pkg/apis/duck/v1" rtesting "knative.dev/pkg/reconciler/testing" @@ -345,6 +350,7 @@ func TestReconcileKind_SCMReportingLogic(t *testing.T) { keys.SHA: "123afc", keys.URLOrg: "random", keys.URLRepository: "app", + keys.SecretCreated: "true", }, }, Spec: tektonv1.PipelineRunSpec{}, @@ -377,6 +383,7 @@ func TestReconcileKind_SCMReportingLogic(t *testing.T) { keys.SHA: "123afc", keys.URLOrg: "random", keys.URLRepository: "app", + keys.SecretCreated: "true", }, }, Spec: tektonv1.PipelineRunSpec{}, @@ -408,6 +415,7 @@ func TestReconcileKind_SCMReportingLogic(t *testing.T) { keys.SHA: "123afc", keys.URLOrg: "random", keys.URLRepository: "app", + keys.SecretCreated: "true", }, }, Spec: tektonv1.PipelineRunSpec{ @@ -474,6 +482,9 @@ func TestReconcileKind_SCMReportingLogic(t *testing.T) { Pac: &info.PacOpts{ Settings: settings.Settings{}, }, + Kube: &info.KubeOpts{ + Namespace: "global", + }, }, } cs.Clients.SetConsoleUI(consoleui.FallBackConsole{}) @@ -523,3 +534,187 @@ func TestReconcileKind_SCMReportingLogic(t *testing.T) { }) } } + +func TestCreateSecretForPipelineRun(t *testing.T) { + // Base annotations required by initGitProviderClient (detectProvider + buildEventFromPipelineRun) + baseAnnotations := map[string]string{ + keys.GitProvider: "github", + keys.RepoURL: "https://github.com/org/repo", + keys.URLOrg: "org", + keys.URLRepository: "repo", + keys.SHA: "abc123", + } + + providerSecretName := "pac-git-basic-auth-owner-repo" + + tests := []struct { + name string + prAnnotations map[string]string + repoUser string + createSecretError error + updateSecretError error + simulatePatchErr bool + wantErr string + wantLogSnippet string + verifyPatched bool + }{ + { + name: "missing git-auth-secret annotation", + prAnnotations: map[string]string{}, + wantErr: "cannot get annotation", + }, + { + name: "MakeBasicAuthSecret failure with malformed URL", + prAnnotations: map[string]string{ + keys.GitAuthSecret: "test-secret", + keys.RepoURL: "http://[invalid", + }, + wantErr: "making basic auth secret", + }, + { + name: "CreateSecret generic failure", + prAnnotations: map[string]string{ + keys.GitAuthSecret: "test-secret", + }, + createSecretError: fmt.Errorf("connection timeout"), + wantErr: "creating basic auth secret", + }, + { + name: "CreateSecret AlreadyExists succeeds with warning", + prAnnotations: map[string]string{ + keys.GitAuthSecret: "test-secret", + }, + createSecretError: errors.NewAlreadyExists(schema.GroupResource{Group: "", Resource: "secrets"}, "test-secret"), + wantLogSnippet: "already exists", + verifyPatched: true, + }, + { + name: "UpdateSecretWithOwnerRef failure", + prAnnotations: map[string]string{ + keys.GitAuthSecret: "test-secret", + }, + updateSecretError: fmt.Errorf("failed to update owner ref"), + wantErr: "cannot update secret", + }, + { + name: "PatchPipelineRun failure returns error", + prAnnotations: map[string]string{ + keys.GitAuthSecret: "test-secret", + }, + simulatePatchErr: true, + wantErr: "failed to patch pipelinerun", + }, + { + name: "happy path with default git user", + prAnnotations: map[string]string{ + keys.GitAuthSecret: "test-secret", + }, + verifyPatched: true, + }, + { + name: "happy path with custom git user", + prAnnotations: map[string]string{ + keys.GitAuthSecret: "test-secret", + }, + repoUser: "custom-user", + verifyPatched: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + observer, log := zapobserver.New(zap.InfoLevel) + logger := zap.New(observer).Sugar() + + // Merge base annotations with test-specific annotations (test-specific overrides base) + annotations := maps.Clone(baseAnnotations) + maps.Copy(annotations, tt.prAnnotations) + + pr := &tektonv1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pr", + Namespace: "test-ns", + Annotations: annotations, + }, + } + + repo := &v1alpha1.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-repo", + Namespace: "test-ns", + }, + Spec: v1alpha1.RepositorySpec{ + GitProvider: &v1alpha1.GitProvider{ + Secret: &v1alpha1.Secret{ + Name: providerSecretName, + }, + User: tt.repoUser, + }, + }, + } + + testData := testclient.Data{ + PipelineRuns: []*tektonv1.PipelineRun{pr}, + Repositories: []*v1alpha1.Repository{repo}, + } + stdata, informers := testclient.SeedTestData(t, ctx, testData) + + if tt.simulatePatchErr { + stdata.Pipeline.PrependReactor("patch", "pipelineruns", func(_ k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("etcd unavailable") + }) + } + + kint := &testkubernetestint.KinterfaceTest{ + GetSecretResult: map[string]string{ + providerSecretName: "test-token", + }, + CreateSecretError: tt.createSecretError, + UpdateSecretError: tt.updateSecretError, + } + + r := &Reconciler{ + run: ¶ms.Run{ + Clients: clients.Clients{ + Tekton: stdata.Pipeline, + Log: logger, + }, + Info: info.Info{ + Pac: &info.PacOpts{ + Settings: settings.Settings{}, + }, + Kube: &info.KubeOpts{ + Namespace: "global", + }, + Controller: &info.ControllerInfo{ + GlobalRepository: "global-repo", + }, + }, + }, + repoLister: informers.Repository.Lister(), + kinteract: kint, + eventEmitter: events.NewEventEmitter(stdata.Kube, logger), + } + + err := r.createSecretForPipelineRun(ctx, logger, pr, repo) + + if tt.wantErr != "" { + assert.Assert(t, err != nil, "expected error containing: %s", tt.wantErr) + assert.ErrorContains(t, err, tt.wantErr) + return + } + assert.NilError(t, err) + + if tt.wantLogSnippet != "" { + logEntries := log.FilterMessageSnippet(tt.wantLogSnippet).TakeAll() + assert.Assert(t, len(logEntries) > 0, "expected log snippet %q not found", tt.wantLogSnippet) + } + + if tt.verifyPatched { + updatedPR, getErr := stdata.Pipeline.TektonV1().PipelineRuns(pr.Namespace).Get(ctx, pr.Name, metav1.GetOptions{}) + assert.NilError(t, getErr) + assert.Equal(t, updatedPR.Annotations[keys.SecretCreated], "true") + } + }) + } +} diff --git a/test/pkg/gitea/test.go b/test/pkg/gitea/test.go index 36606a3980..2fdaddc13a 100644 --- a/test/pkg/gitea/test.go +++ b/test/pkg/gitea/test.go @@ -199,7 +199,10 @@ func TestPR(t *testing.T, topts *TestOpts) (context.Context, func()) { if topts.GlobalRepoCRParams == nil { spec.GitProvider = gp } else { - spec.GitProvider = &v1alpha1.GitProvider{Type: providerType} + spec.GitProvider = &v1alpha1.GitProvider{ + Type: providerType, + Secret: &v1alpha1.Secret{Name: topts.TargetNS, Key: "token"}, + } } assert.NilError(t, CreateCRD(ctx, topts, spec, false))