Skip to content

Commit bca9c3d

Browse files
committed
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 <zashaikh@redhat.com> Assisted-by: Claude Opus 4.6 (via Claude Code)
1 parent c6397d3 commit bca9c3d

File tree

12 files changed

+339
-447
lines changed

12 files changed

+339
-447
lines changed

config/201-controller-role.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ rules:
6767
verbs: ["create"]
6868
- apiGroups: [""]
6969
resources: ["secrets"]
70-
verbs: ["get", "create", "update", "delete"]
70+
verbs: ["get", "delete"]
7171
- apiGroups: ["pipelinesascode.tekton.dev"]
7272
resources: ["repositories"]
7373
verbs: ["get", "create", "list"]

config/202-watcher-role.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ metadata:
6767
rules:
6868
- apiGroups: [""]
6969
resources: ["secrets"]
70-
verbs: ["get", "delete"]
70+
verbs: ["get", "create", "update", "delete"]
7171
- apiGroups: ["pipelinesascode.tekton.dev"]
7272
resources: ["repositories"]
7373
verbs: ["get", "list", "update", "watch"]

pkg/apis/pipelinesascode/keys/keys.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,8 @@ const (
6161
LogURL = pipelinesascode.GroupName + "/log-url"
6262
ExecutionOrder = pipelinesascode.GroupName + "/execution-order"
6363
SCMReportingPLRStarted = pipelinesascode.GroupName + "/scm-reporting-plr-started"
64+
SecretCreated = pipelinesascode.GroupName + "/secret-created"
65+
CloneURL = pipelinesascode.GroupName + "/clone-url"
6466
// PublicGithubAPIURL default is "https://api.github.com" but it can be overridden by X-GitHub-Enterprise-Host header.
6567
PublicGithubAPIURL = "https://api.github.com"
6668
GithubApplicationID = "github-application-id"

pkg/kubeinteraction/labels.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ func AddLabelsAndAnnotations(event *info.Event, pipelineRun *tektonv1.PipelineRu
5555
keys.SourceBranch: event.HeadBranch,
5656
keys.Repository: repo.GetName(),
5757
keys.GitProvider: providerConfig.Name,
58+
keys.SecretCreated: "false",
5859
keys.ControllerInfo: fmt.Sprintf(`{"name":"%s","configmap":"%s","secret":"%s", "gRepo": "%s"}`,
5960
paramsinfo.Controller.Name, paramsinfo.Controller.Configmap, paramsinfo.Controller.Secret, paramsinfo.Controller.GlobalRepository),
6061
}
@@ -82,6 +83,10 @@ func AddLabelsAndAnnotations(event *info.Event, pipelineRun *tektonv1.PipelineRu
8283
annotations[keys.TargetProjectID] = strconv.Itoa(int(event.TargetProjectID))
8384
}
8485

86+
if event.CloneURL != "" {
87+
annotations[keys.CloneURL] = event.CloneURL
88+
}
89+
8590
if value, ok := pipelineRun.GetObjectMeta().GetAnnotations()[keys.CancelInProgress]; ok {
8691
labels[keys.CancelInProgress] = value
8792
}

pkg/kubeinteraction/labels_test.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ func TestAddLabelsAndAnnotations(t *testing.T) {
2424
event.SHAURL = "https://url/sha"
2525
event.HeadBranch = "pr_branch"
2626
event.HeadURL = "https://url/pr"
27+
event.CloneURL = "https://url/clone"
2728

2829
type args struct {
2930
event *info.Event
@@ -81,6 +82,7 @@ func TestAddLabelsAndAnnotations(t *testing.T) {
8182
assert.Equal(t, tt.args.pipelineRun.Annotations[keys.SourceRepoURL], tt.args.event.HeadURL)
8283
assert.Equal(t, tt.args.pipelineRun.Annotations[keys.ControllerInfo],
8384
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))
85+
assert.Equal(t, tt.args.pipelineRun.Annotations[keys.CloneURL], tt.args.event.CloneURL)
8486
})
8587
}
8688
}

pkg/pipelineascode/pipelineascode.go

Lines changed: 0 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,8 @@ import (
2020
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
2121
"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
2222
providerstatus "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/status"
23-
"github.com/openshift-pipelines/pipelines-as-code/pkg/secrets"
2423
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
2524
"go.uber.org/zap"
26-
"k8s.io/apimachinery/pkg/api/errors"
2725
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2826
)
2927

@@ -206,7 +204,6 @@ func (p *PacRun) Run(ctx context.Context) error {
206204
}
207205

208206
func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.PipelineRun, error) {
209-
var gitAuthSecretName string
210207
prName := match.PipelineRun.GetName()
211208
if prName == "" {
212209
prName = match.PipelineRun.GetGenerateName()
@@ -218,37 +215,6 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi
218215
p.event.BaseBranch,
219216
)
220217

221-
// Automatically create a secret with the token to be reused by git-clone task
222-
if p.pacInfo.SecretAutoCreation {
223-
if annotation, ok := match.PipelineRun.GetAnnotations()[keys.GitAuthSecret]; ok {
224-
gitAuthSecretName = annotation
225-
p.debugf("startPR: using git auth secret from annotation=%s", gitAuthSecretName)
226-
} else {
227-
return nil, fmt.Errorf("cannot get annotation %s as set on PR", keys.GitAuthSecret)
228-
}
229-
230-
authSecret, err := secrets.MakeBasicAuthSecret(p.event, gitAuthSecretName)
231-
if err != nil {
232-
return nil, fmt.Errorf("making basic auth secret: %s has failed: %w ", gitAuthSecretName, err)
233-
}
234-
235-
if err = p.k8int.CreateSecret(ctx, match.Repo.GetNamespace(), authSecret); err != nil {
236-
// NOTE: Handle AlreadyExists errors due to etcd/API server timing issues.
237-
// Investigation found: slow etcd response causes API server retry, resulting in
238-
// duplicate secret creation attempts for the same PR. This is a workaround, not
239-
// designed behavior - reuse existing secret to prevent PipelineRun failure.
240-
if errors.IsAlreadyExists(err) {
241-
msg := fmt.Sprintf("Secret %s already exists in namespace %s, reusing existing secret",
242-
authSecret.GetName(), match.Repo.GetNamespace())
243-
p.eventEmitter.EmitMessage(match.Repo, zap.WarnLevel, "RepositorySecretReused", msg)
244-
} else {
245-
return nil, fmt.Errorf("creating basic auth secret: %s has failed: %w ", authSecret.GetName(), err)
246-
}
247-
} else {
248-
p.debugf("startPR: created git auth secret %s in namespace %s", authSecret.GetName(), match.Repo.GetNamespace())
249-
}
250-
}
251-
252218
// Add labels and annotations to pipelinerun
253219
err := kubeinteraction.AddLabelsAndAnnotations(p.event, match.PipelineRun, match.Repo, p.vcx.GetConfig(), p.run)
254220
if err != nil {
@@ -269,29 +235,11 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi
269235
pr, err := p.run.Clients.Tekton.TektonV1().PipelineRuns(match.Repo.GetNamespace()).Create(ctx,
270236
match.PipelineRun, metav1.CreateOptions{})
271237
if err != nil {
272-
// cleanup the gitauth secret because ownerRef isn't set when the pipelineRun creation failed
273-
if p.pacInfo.SecretAutoCreation {
274-
if errDelSec := p.k8int.DeleteSecret(ctx, p.logger, match.Repo.GetNamespace(), gitAuthSecretName); errDelSec != nil {
275-
// don't overshadow the pipelineRun creation error, just log
276-
p.logger.Errorf("removing auto created secret: %s in namespace %s has failed: %w ", gitAuthSecretName, match.Repo.GetNamespace(), errDelSec)
277-
}
278-
}
279238
// we need to make difference between markdown error and normal error that goes to namespace/controller stream
280239
return nil, fmt.Errorf("creating pipelinerun %s in namespace %s has failed.\n\nTekton Controller has reported this error: ```%w``` ", match.PipelineRun.GetGenerateName(),
281240
match.Repo.GetNamespace(), err)
282241
}
283242

284-
// update ownerRef of secret with pipelineRun, so that it gets cleanedUp with pipelineRun
285-
if p.pacInfo.SecretAutoCreation {
286-
err := p.k8int.UpdateSecretWithOwnerRef(ctx, p.logger, pr.Namespace, gitAuthSecretName, pr)
287-
if err != nil {
288-
// we still return the created PR with error, and allow caller to decide what to do with the PR, and avoid
289-
// unneeded SIGSEGV's
290-
return pr, fmt.Errorf("cannot update pipelinerun %s with ownerRef: %w", pr.GetGenerateName(), err)
291-
}
292-
p.debugf("startPR: updated secret ownerRef for pipelinerun=%s secret=%s", pr.GetName(), gitAuthSecretName)
293-
}
294-
295243
// Create status with the log url
296244
p.logger.Infof("PipelineRun %s has been created in namespace %s with status %s for SHA: %s Target Branch: %s",
297245
pr.GetName(), match.Repo.GetNamespace(), pr.Spec.Status, p.event.SHA, p.event.BaseBranch)

0 commit comments

Comments
 (0)