From f32eb1e466b5e1ce071d3c045a569b12c5dac7d4 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Fri, 8 Mar 2024 10:40:02 +0100 Subject: [PATCH] fix race conditions on pr concurrency when storing a pointer in the context, the pointer get nil sometime on high load which causes some of the pipelinerun issue we fix this by passing the value around instead of passing the pointer in context. There is more going on in this bug that meet the eyes but that seems to fix it. remove all the StoreInfo and GetInfo functions and use the value directly. Signed-off-by: Chmouel Boudjnah --- cmd/pipelines-as-code-controller/main.go | 1 - pkg/adapter/adapter.go | 3 +- pkg/adapter/adapter_test.go | 6 --- pkg/adapter/incoming.go | 1 + pkg/adapter/incoming_test.go | 12 ++--- pkg/cmd/tknpac/info/install_test.go | 10 ++--- pkg/kubeinteraction/labels.go | 10 ++--- pkg/kubeinteraction/labels_test.go | 14 +++--- pkg/params/info/runinfo.go | 54 ----------------------- pkg/params/info/runinfo_test.go | 53 ---------------------- pkg/pipelineascode/match.go | 2 +- pkg/pipelineascode/pipelineascode.go | 2 +- pkg/pipelineascode/pipelineascode_test.go | 10 ++--- pkg/pipelineascode/secret.go | 5 +-- pkg/provider/github/app/token.go | 4 +- pkg/provider/github/app/token_test.go | 21 ++++----- pkg/provider/github/github_test.go | 10 ++--- pkg/provider/github/parse_payload.go | 10 +++-- pkg/provider/github/parse_payload_test.go | 4 +- pkg/provider/github/scope_test.go | 8 ++-- pkg/reconciler/controller.go | 1 - pkg/reconciler/event.go | 2 + pkg/reconciler/reconciler.go | 7 ++- pkg/reconciler/reconciler_test.go | 8 ++-- test/github_config_maxkeepruns_test.go | 2 +- test/github_incoming_test.go | 2 +- test/pkg/cctx/ctx.go | 1 - test/pkg/github/setup.go | 1 + 28 files changed, 71 insertions(+), 193 deletions(-) delete mode 100644 pkg/params/info/runinfo_test.go diff --git a/cmd/pipelines-as-code-controller/main.go b/cmd/pipelines-as-code-controller/main.go index 07f066d98..f3b493b4d 100644 --- a/cmd/pipelines-as-code-controller/main.go +++ b/cmd/pipelines-as-code-controller/main.go @@ -42,7 +42,6 @@ func main() { ctx = evadapter.WithConfiguratorOptions(ctx, []evadapter.ConfiguratorOption{copt}) ctx = info.StoreNS(ctx, ns) - ctx = info.StoreInfo(ctx, rinfo.Controller.Name, rinfo) ctx = info.StoreCurrentControllerName(ctx, rinfo.Controller.Name) ctx = context.WithValue(ctx, client.Key{}, run.Clients.Kube) diff --git a/pkg/adapter/adapter.go b/pkg/adapter/adapter.go index 3e3709e9e..2ae6e42ca 100644 --- a/pkg/adapter/adapter.go +++ b/pkg/adapter/adapter.go @@ -101,14 +101,12 @@ func (l *listener) Start(ctx context.Context) error { func (l listener) handleEvent(ctx context.Context) http.HandlerFunc { return func(response http.ResponseWriter, request *http.Request) { // we should fix this, this basically reads configmap on every request, is that supposed to be okay? - currentControllerName := info.GetCurrentControllerName(ctx) if err := l.run.UpdatePACInfo(ctx); err != nil { log.Fatalf("error getting config and setting from configmaps: %v", err) } ninfo := &info.Info{} l.run.Info.DeepCopy(ninfo) - ctx = info.StoreInfo(ctx, currentControllerName, ninfo) if request.Method != http.MethodPost { l.writeResponse(response, http.StatusOK, "ok") @@ -223,6 +221,7 @@ func (l listener) detectProvider(req *http.Request, reqBody string) (provider.In } gitHub := github.New() + gitHub.Run = l.run isGH, processReq, logger, reason, err := gitHub.Detect(req, reqBody, &log) if isGH { return l.processRes(processReq, gitHub, logger, reason, err) diff --git a/pkg/adapter/adapter_test.go b/pkg/adapter/adapter_test.go index e6c4e41f9..82585b42f 100644 --- a/pkg/adapter/adapter_test.go +++ b/pkg/adapter/adapter_test.go @@ -41,12 +41,6 @@ func TestHandleEvent(t *testing.T) { logger, _ := logger.GetLogger() ctx = info.StoreCurrentControllerName(ctx, "default") - ctx = info.StoreInfo(ctx, "default", &info.Info{ - Controller: &info.ControllerInfo{ - Secret: info.DefaultPipelinesAscodeSecretName, - Configmap: info.DefaultPipelinesAscodeConfigmapName, - }, - }) ctx = info.StoreNS(ctx, "default") emptys := &unstructured.Unstructured{} diff --git a/pkg/adapter/incoming.go b/pkg/adapter/incoming.go index 91ce86bc4..ed1e86012 100644 --- a/pkg/adapter/incoming.go +++ b/pkg/adapter/incoming.go @@ -104,6 +104,7 @@ func (l *listener) detectIncoming(ctx context.Context, req *http.Request, payloa if repo.Spec.GitProvider == nil || repo.Spec.GitProvider.Type == "" { gh := github.New() + gh.Run = l.run ns := info.GetNS(ctx) enterpriseURL, token, installationID, err := app.GetAndUpdateInstallationID(ctx, req, l.run, repo, gh, ns) if err != nil { diff --git a/pkg/adapter/incoming_test.go b/pkg/adapter/incoming_test.go index f1f713cd6..6536ac43f 100644 --- a/pkg/adapter/incoming_test.go +++ b/pkg/adapter/incoming_test.go @@ -655,11 +655,6 @@ func Test_listener_detectIncoming(t *testing.T) { t.Run(tt.name, func(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) ctx = info.StoreCurrentControllerName(ctx, "default") - ctx = info.StoreInfo(ctx, "default", &info.Info{ - Controller: &info.ControllerInfo{ - Secret: info.DefaultPipelinesAscodeSecretName, - }, - }) ctx = info.StoreNS(ctx, testNamespace.GetName()) cs, _ := testclient.SeedTestData(t, ctx, tt.args.data) client := ¶ms.Run{ @@ -667,7 +662,11 @@ func Test_listener_detectIncoming(t *testing.T) { PipelineAsCode: cs.PipelineAsCode, Kube: cs.Kube, }, - Info: info.Info{}, + Info: info.Info{ + Controller: &info.ControllerInfo{ + Secret: info.DefaultPipelinesAscodeSecretName, + }, + }, } observer, _ := zapobserver.New(zap.InfoLevel) logger := zap.New(observer).Sugar() @@ -681,6 +680,7 @@ func Test_listener_detectIncoming(t *testing.T) { kint: kint, event: info.NewEvent(), } + // make a new request req := httptest.NewRequest(tt.args.method, fmt.Sprintf("http://localhost%s?repository=%s&secret=%s&pipelinerun=%s&branch=%s", tt.args.queryURL, diff --git a/pkg/cmd/tknpac/info/install_test.go b/pkg/cmd/tknpac/info/install_test.go index 457fed9be..cab045637 100644 --- a/pkg/cmd/tknpac/info/install_test.go +++ b/pkg/cmd/tknpac/info/install_test.go @@ -160,11 +160,6 @@ func TestInfo(t *testing.T) { if tt.secret != nil { name = tt.secret.GetName() } - ctx = info.StoreInfo(ctx, "default", &info.Info{ - Controller: &info.ControllerInfo{ - Secret: name, - }, - }) stdata, _ := testclient.SeedTestData(t, ctx, tdata) cs := ¶ms.Run{ Clients: clients.Clients{ @@ -172,6 +167,11 @@ func TestInfo(t *testing.T) { Kube: stdata.Kube, HTTP: *httpTestClient, }, + Info: info.Info{ + Controller: &info.ControllerInfo{ + Secret: name, + }, + }, } io, out := tcli.NewIOStream() diff --git a/pkg/kubeinteraction/labels.go b/pkg/kubeinteraction/labels.go index e3bb940a3..ee5d55f7c 100644 --- a/pkg/kubeinteraction/labels.go +++ b/pkg/kubeinteraction/labels.go @@ -1,7 +1,6 @@ package kubeinteraction import ( - "context" "fmt" "strconv" @@ -9,6 +8,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" "github.com/openshift-pipelines/pipelines-as-code/pkg/formatting" + "github.com/openshift-pipelines/pipelines-as-code/pkg/params" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/version" tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" @@ -21,13 +21,13 @@ const ( StateFailed = "failed" ) -func AddLabelsAndAnnotations(ctx context.Context, event *info.Event, pipelineRun *tektonv1.PipelineRun, repo *apipac.Repository, providerinfo *info.ProviderConfig) error { +func AddLabelsAndAnnotations(event *info.Event, pipelineRun *tektonv1.PipelineRun, repo *apipac.Repository, providerConfig *info.ProviderConfig, paramsRun *params.Run) error { if event == nil { return fmt.Errorf("event should not be nil") } + paramsinfo := paramsRun.Info // Add labels on the soon-to-be created pipelinerun so UI/CLI can easily // query them. - paramsinfo := info.GetInfo(ctx, info.GetCurrentControllerName(ctx)) labels := map[string]string{ // These keys are used in LabelSelector query, so we are keeping in Labels as it is. // But adding same keys to Annotations so UI/CLI can fetch the actual value instead of modified value @@ -52,7 +52,7 @@ func AddLabelsAndAnnotations(ctx context.Context, event *info.Event, pipelineRun keys.EventType: event.EventType, keys.Branch: event.BaseBranch, keys.Repository: repo.GetName(), - keys.GitProvider: providerinfo.Name, + keys.GitProvider: providerConfig.Name, keys.State: StateStarted, keys.ControllerInfo: fmt.Sprintf(`{"name":"%s","configmap":"%s","secret":"%s"}`, paramsinfo.Controller.Name, paramsinfo.Controller.Configmap, paramsinfo.Controller.Secret), } @@ -63,7 +63,7 @@ func AddLabelsAndAnnotations(ctx context.Context, event *info.Event, pipelineRun } // TODO: move to provider specific function - if providerinfo.Name == "github" || providerinfo.Name == "github-enterprise" { + if providerConfig.Name == "github" || providerConfig.Name == "github-enterprise" { if event.InstallationID != -1 { annotations[keys.InstallationID] = strconv.FormatInt(event.InstallationID, 10) } diff --git a/pkg/kubeinteraction/labels_test.go b/pkg/kubeinteraction/labels_test.go index d279e6adc..9ac878977 100644 --- a/pkg/kubeinteraction/labels_test.go +++ b/pkg/kubeinteraction/labels_test.go @@ -6,11 +6,11 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" + "github.com/openshift-pipelines/pipelines-as-code/pkg/params" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "gotest.tools/v3/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - rtesting "knative.dev/pkg/reconciler/testing" ) func TestAddLabelsAndAnnotations(t *testing.T) { @@ -58,12 +58,12 @@ func TestAddLabelsAndAnnotations(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx, _ := rtesting.SetupFakeContext(t) - ctx = info.StoreCurrentControllerName(ctx, "default") - ctx = info.StoreInfo(ctx, "default", &info.Info{ - Controller: tt.args.controllerInfo, - }) - err := AddLabelsAndAnnotations(ctx, tt.args.event, tt.args.pipelineRun, tt.args.repo, &info.ProviderConfig{}) + paramsRun := ¶ms.Run{ + Info: info.Info{ + Controller: tt.args.controllerInfo, + }, + } + err := AddLabelsAndAnnotations(tt.args.event, tt.args.pipelineRun, tt.args.repo, &info.ProviderConfig{}, paramsRun) assert.NilError(t, err) assert.Equal(t, tt.args.pipelineRun.Labels[keys.URLOrg], tt.args.event.Organization, "'%s' != %s", tt.args.pipelineRun.Labels[keys.URLOrg], tt.args.event.Organization) diff --git a/pkg/params/info/runinfo.go b/pkg/params/info/runinfo.go index 780a704a1..aa264e5b4 100644 --- a/pkg/params/info/runinfo.go +++ b/pkg/params/info/runinfo.go @@ -1,9 +1,5 @@ package info -import ( - "context" -) - type Info struct { Pac *PacOpts Kube *KubeOpts @@ -17,53 +13,3 @@ func (i *Info) DeepCopy(out *Info) { type ( contextKey string ) - -type CtxInfo struct { - Pac *PacOpts - Kube *KubeOpts - Controller *ControllerInfo -} - -// GetInfo Pac Settings for that label. -func GetInfo(ctx context.Context, label string) *Info { - labelContextKey := contextKey(label) - if val := ctx.Value(labelContextKey); val != nil { - if ctxInfo, ok := val.(CtxInfo); ok { - return &Info{ - Pac: ctxInfo.Pac, - Kube: ctxInfo.Kube, - Controller: ctxInfo.Controller, - } - } - } - return nil -} - -// StoreInfo Pac Settings for a label. -func StoreInfo(ctx context.Context, label string, info *Info) context.Context { - labelContextKey := contextKey(label) - if val := ctx.Value(labelContextKey); val != nil { - if ctxInfo, ok := val.(CtxInfo); ok { - if ctxInfo.Pac == nil { - ctxInfo.Pac = &PacOpts{} - } - if ctxInfo.Kube == nil { - ctxInfo.Kube = &KubeOpts{ - Namespace: GetNS(ctx), - } - } - if ctxInfo.Controller == nil { - ctxInfo.Controller = &ControllerInfo{} - } - ctxInfo.Pac = info.Pac - ctxInfo.Kube = info.Kube - ctxInfo.Controller = info.Controller - return context.WithValue(ctx, labelContextKey, ctxInfo) - } - } - return context.WithValue(ctx, labelContextKey, CtxInfo{ - Pac: info.Pac, - Kube: info.Kube, - Controller: info.Controller, - }) -} diff --git a/pkg/params/info/runinfo_test.go b/pkg/params/info/runinfo_test.go deleted file mode 100644 index f9cafe4cd..000000000 --- a/pkg/params/info/runinfo_test.go +++ /dev/null @@ -1,53 +0,0 @@ -package info - -import ( - "context" - "testing" - - "github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings" - "gotest.tools/v3/assert" -) - -func TestRunInfoContext(t *testing.T) { - label1 := "label1" - info := &Info{ - Pac: &PacOpts{ - Settings: &settings.Settings{ - ApplicationName: "App for " + label1, - }, - }, - Kube: &KubeOpts{ - Namespace: label1, - }, - } - ctx := context.TODO() - ctx = StoreInfo(ctx, label1, info) - - label2 := "label2" - info2 := &Info{ - Pac: &PacOpts{ - Settings: &settings.Settings{ - ApplicationName: "App for " + label2, - }, - }, - Kube: &KubeOpts{ - Namespace: label2, - }, - } - ctx = StoreInfo(ctx, label2, info2) - - t.Run("Get", func(t *testing.T) { - rinfo1 := GetInfo(ctx, label1) - assert.Assert(t, rinfo1 != nil) - assert.Assert(t, rinfo1.Pac.Settings.ApplicationName == "App for "+label1) - assert.Assert(t, rinfo1.Kube.Namespace == label1) - rinfo2 := GetInfo(ctx, label2) - assert.Assert(t, rinfo2 != nil) - assert.Assert(t, rinfo2.Pac.Settings.ApplicationName == "App for "+label2) - assert.Assert(t, rinfo2.Kube.Namespace == label2) - }) - t.Run("Get no context", func(t *testing.T) { - nctx := context.TODO() - assert.Assert(t, GetInfo(nctx, label1) == nil) - }) -} diff --git a/pkg/pipelineascode/match.go b/pkg/pipelineascode/match.go index cc73ebd52..f3f6355b5 100644 --- a/pkg/pipelineascode/match.go +++ b/pkg/pipelineascode/match.go @@ -68,7 +68,7 @@ func (p *PacRun) verifyRepoAndUser(ctx context.Context) (*v1alpha1.Repository, e // so instead of having to specify their in Repo each time, they use a // shared one from pac. if p.event.InstallationID > 0 { - p.event.Provider.WebhookSecret, _ = GetCurrentNSWebhookSecret(ctx, p.k8int) + p.event.Provider.WebhookSecret, _ = GetCurrentNSWebhookSecret(ctx, p.k8int, p.run) } else { err := SecretFromRepository(ctx, p.run, p.k8int, p.vcx.GetConfig(), p.event, repo, p.logger) if err != nil { diff --git a/pkg/pipelineascode/pipelineascode.go b/pkg/pipelineascode/pipelineascode.go index 1b83f7441..72f1e85d0 100644 --- a/pkg/pipelineascode/pipelineascode.go +++ b/pkg/pipelineascode/pipelineascode.go @@ -145,7 +145,7 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi } // Add labels and annotations to pipelinerun - err := kubeinteraction.AddLabelsAndAnnotations(ctx, p.event, match.PipelineRun, match.Repo, p.vcx.GetConfig()) + err := kubeinteraction.AddLabelsAndAnnotations(p.event, match.PipelineRun, match.Repo, p.vcx.GetConfig(), p.run) if err != nil { p.logger.Errorf("Error adding labels/annotations to PipelineRun '%s' in namespace '%s': %v", match.PipelineRun.GetName(), match.Repo.GetNamespace(), err) } diff --git a/pkg/pipelineascode/pipelineascode_test.go b/pkg/pipelineascode/pipelineascode_test.go index 08a5f1ffd..44eb39fa5 100644 --- a/pkg/pipelineascode/pipelineascode_test.go +++ b/pkg/pipelineascode/pipelineascode_test.go @@ -502,11 +502,6 @@ func TestRun(t *testing.T) { secrets[info.DefaultPipelinesAscodeSecretName] = webhookSecret } - ctx = info.StoreInfo(ctx, "default", &info.Info{ - Controller: &info.ControllerInfo{ - Secret: info.DefaultPipelinesAscodeSecretName, - }, - }) if tt.repositories == nil { tt.repositories = []*v1alpha1.Repository{ testnewrepo.NewRepo(repo), @@ -552,6 +547,9 @@ func TestRun(t *testing.T) { HubCatalogs: &hubCatalogs, }, }, + Controller: &info.ControllerInfo{ + Secret: info.DefaultPipelinesAscodeSecretName, + }, }, } mac := hmac.New(sha256.New, []byte(payloadEncodedSecret)) @@ -586,7 +584,7 @@ func TestRun(t *testing.T) { vcx := &ghprovider.Provider{ Client: fakeclient, - Run: params.New(), + Run: cs, Token: github.String("None"), Logger: logger, } diff --git a/pkg/pipelineascode/secret.go b/pkg/pipelineascode/secret.go index 489d2754d..5ed9cf6fc 100644 --- a/pkg/pipelineascode/secret.go +++ b/pkg/pipelineascode/secret.go @@ -90,12 +90,11 @@ func SecretFromRepository(ctx context.Context, cs *params.Run, k8int kubeinterac } // GetCurrentNSWebhookSecret get secret from namespace as stored on context. -func GetCurrentNSWebhookSecret(ctx context.Context, k8int kubeinteraction.Interface) (string, error) { +func GetCurrentNSWebhookSecret(ctx context.Context, k8int kubeinteraction.Interface, run *params.Run) (string, error) { ns := info.GetNS(ctx) - paramsinfo := info.GetInfo(ctx, info.GetCurrentControllerName(ctx)) s, err := k8int.GetSecret(ctx, ktypes.GetSecretOpt{ Namespace: ns, - Name: paramsinfo.Controller.Secret, + Name: run.Info.Controller.Secret, Key: defaultPipelinesAscodeSecretWebhookSecretKey, }) // a lot of people have problem with this secret, when encoding it to base64 which add a \n when we do : diff --git a/pkg/provider/github/app/token.go b/pkg/provider/github/app/token.go index 88ca83d86..61fb22b7b 100644 --- a/pkg/provider/github/app/token.go +++ b/pkg/provider/github/app/token.go @@ -100,7 +100,9 @@ type JWTClaim struct { func GenerateJWT(ctx context.Context, ns string, run *params.Run) (string, error) { // TODO: move this out of here - applicationID, privateKey, err := github.GetAppIDAndPrivateKey(ctx, ns, run.Clients.Kube) + gh := github.New() + gh.Run = run + applicationID, privateKey, err := gh.GetAppIDAndPrivateKey(ctx, ns, run.Clients.Kube) if err != nil { return "", err } diff --git a/pkg/provider/github/app/token_test.go b/pkg/provider/github/app/token_test.go index 357110078..ca9fbddad 100644 --- a/pkg/provider/github/app/token_test.go +++ b/pkg/provider/github/app/token_test.go @@ -126,15 +126,10 @@ func Test_GenerateJWT(t *testing.T) { } ctx, _ := rtesting.SetupFakeContext(t) ctx = info.StoreCurrentControllerName(ctx, "default") - name := "" + secretName := "" if tt.secret != nil { - name = tt.secret.GetName() + secretName = tt.secret.GetName() } - ctx = info.StoreInfo(ctx, "default", &info.Info{ - Controller: &info.ControllerInfo{ - Secret: name, - }, - }) stdata, _ := testclient.SeedTestData(t, ctx, tdata) run := ¶ms.Run{ @@ -143,6 +138,11 @@ func Test_GenerateJWT(t *testing.T) { PipelineAsCode: stdata.PipelineAsCode, Kube: stdata.Kube, }, + Info: info.Info{ + Controller: &info.ControllerInfo{ + Secret: secretName, + }, + }, } token, err := GenerateJWT(ctx, tt.namespace.GetName(), run) @@ -193,12 +193,10 @@ func Test_GetAndUpdateInstallationID(t *testing.T) { Pac: &info.PacOpts{ Settings: &settings.Settings{}, }, + Controller: &info.ControllerInfo{Secret: validSecret.GetName()}, }, } ctx = info.StoreCurrentControllerName(ctx, "default") - ctx = info.StoreInfo(ctx, "default", &info.Info{ - Controller: &info.ControllerInfo{Secret: validSecret.GetName()}, - }) ctx = info.StoreNS(ctx, testNamespace.GetName()) jwtToken, err := GenerateJWT(ctx, testNamespace.GetName(), run) @@ -221,8 +219,7 @@ func Test_GetAndUpdateInstallationID(t *testing.T) { }, } - gprovider := &github.Provider{Client: fakeghclient, APIURL: &serverURL} - + gprovider := &github.Provider{Client: fakeghclient, APIURL: &serverURL, Run: run} mux.HandleFunc(fmt.Sprintf("/app/installations/%d/access_tokens", wantID), func(w http.ResponseWriter, r *http.Request) { testMethod(t, r, "POST") w.Header().Set("Authorization", "Bearer "+jwtToken) diff --git a/pkg/provider/github/github_test.go b/pkg/provider/github/github_test.go index 60de6202f..96397d1f3 100644 --- a/pkg/provider/github/github_test.go +++ b/pkg/provider/github/github_test.go @@ -1030,11 +1030,6 @@ func TestCreateToken(t *testing.T) { } ctx = info.StoreNS(ctx, testNamespace.GetName()) ctx = info.StoreCurrentControllerName(ctx, "default") - ctx = info.StoreInfo(ctx, "default", &info.Info{ - Controller: &info.ControllerInfo{ - Secret: validSecret.GetName(), - }, - }) tdata := testclient.Data{ Namespaces: []*corev1.Namespace{testNamespace}, @@ -1049,6 +1044,11 @@ func TestCreateToken(t *testing.T) { PipelineAsCode: stdata.PipelineAsCode, Kube: stdata.Kube, }, + Info: info.Info{ + Controller: &info.ControllerInfo{ + Secret: validSecret.GetName(), + }, + }, } info := &info.Event{ diff --git a/pkg/provider/github/parse_payload.go b/pkg/provider/github/parse_payload.go index cfc749265..102253915 100644 --- a/pkg/provider/github/parse_payload.go +++ b/pkg/provider/github/parse_payload.go @@ -24,9 +24,11 @@ import ( "k8s.io/client-go/kubernetes" ) -func GetAppIDAndPrivateKey(ctx context.Context, ns string, kube kubernetes.Interface) (int64, []byte, error) { - controllerName := info.GetCurrentControllerName(ctx) - paramsinfo := info.GetInfo(ctx, controllerName) +// GetAppIDAndPrivateKey retrieves the GitHub application ID and private key from a secret in the specified namespace. +// It takes a context, namespace, and Kubernetes client as input parameters. +// It returns the application ID (int64), private key ([]byte), and an error if any. +func (v *Provider) GetAppIDAndPrivateKey(ctx context.Context, ns string, kube kubernetes.Interface) (int64, []byte, error) { + paramsinfo := &v.Run.Info secret, err := kube.CoreV1().Secrets(ns).Get(ctx, paramsinfo.Controller.Secret, v1.GetOptions{}) if err != nil { return 0, []byte{}, fmt.Errorf("could not get the secret %s in ns %s: %w", paramsinfo.Controller.Secret, ns, err) @@ -43,7 +45,7 @@ func GetAppIDAndPrivateKey(ctx context.Context, ns string, kube kubernetes.Inter } func (v *Provider) GetAppToken(ctx context.Context, kube kubernetes.Interface, gheURL string, installationID int64, ns string) (string, error) { - applicationID, privateKey, err := GetAppIDAndPrivateKey(ctx, ns, kube) + applicationID, privateKey, err := v.GetAppIDAndPrivateKey(ctx, ns, kube) if err != nil { return "", err } diff --git a/pkg/provider/github/parse_payload_test.go b/pkg/provider/github/parse_payload_test.go index 0860665fb..d876b4346 100644 --- a/pkg/provider/github/parse_payload_test.go +++ b/pkg/provider/github/parse_payload_test.go @@ -730,6 +730,7 @@ func TestAppTokenGeneration(t *testing.T) { Pac: &info.PacOpts{ Settings: &settings.Settings{}, }, + Controller: &info.ControllerInfo{Secret: secretName}, }, } @@ -754,9 +755,6 @@ func TestAppTokenGeneration(t *testing.T) { } tt.ctx = info.StoreCurrentControllerName(tt.ctx, "default") - tt.ctx = info.StoreInfo(tt.ctx, "default", &info.Info{ - Controller: &info.ControllerInfo{Secret: secretName}, - }) tt.ctx = info.StoreNS(tt.ctx, tt.ctxNS) _, err := gprovider.ParsePayload(tt.ctx, run, request, string(jeez)) diff --git a/pkg/provider/github/scope_test.go b/pkg/provider/github/scope_test.go index 768aeb31b..071bb6f35 100644 --- a/pkg/provider/github/scope_test.go +++ b/pkg/provider/github/scope_test.go @@ -197,15 +197,13 @@ func TestScopeTokenToListOfRepos(t *testing.T) { SecretGHAppRepoScoped: tt.secretGHAppRepoScopedKey, }, }, + Controller: &info.ControllerInfo{ + Secret: info.DefaultPipelinesAscodeSecretName, + }, }, } ctx = info.StoreCurrentControllerName(ctx, "default") - ctx = info.StoreInfo(ctx, "default", &info.Info{ - Controller: &info.ControllerInfo{ - Secret: info.DefaultPipelinesAscodeSecretName, - }, - }) ctx = info.StoreNS(ctx, testNamespace.GetName()) fakeghclient, mux, serverURL, teardown := ghtesthelper.SetupGH() diff --git a/pkg/reconciler/controller.go b/pkg/reconciler/controller.go index 55a0c21f2..c3215b1d9 100644 --- a/pkg/reconciler/controller.go +++ b/pkg/reconciler/controller.go @@ -36,7 +36,6 @@ func NewController() func(context.Context, configmap.Watcher) *controller.Impl { if err != nil { log.Fatal("failed to init clients : ", err) } - ctx = info.StoreInfo(ctx, ns, &run.Info) kinteract, err := kubeinteraction.NewKubernetesInteraction(run) if err != nil { diff --git a/pkg/reconciler/event.go b/pkg/reconciler/event.go index 4ce973718..cf69c816e 100644 --- a/pkg/reconciler/event.go +++ b/pkg/reconciler/event.go @@ -31,6 +31,8 @@ func (r *Reconciler) detectProvider(ctx context.Context, logger *zap.SugaredLogg switch gitProvider { case "github", "github-enterprise": gh := github.New() + gh.Logger = logger + gh.Run = r.run if event.InstallationID != 0 { if err := gh.InitAppClient(ctx, r.run.Clients.Kube, event); err != nil { return nil, nil, err diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index b5b520f09..88c2b5a5d 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -105,7 +105,6 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun if err := r.run.UpdatePACInfo(ctx); err != nil { return fmt.Errorf("failed to get information for controller config %v: %w", r.run.Info.Controller, err) } - ctx = info.StoreInfo(ctx, r.run.Info.Controller.Name, &r.run.Info) ctx = info.StoreCurrentControllerName(ctx, r.run.Info.Controller.Name) logger = logger.With( @@ -146,7 +145,7 @@ func (r *Reconciler) reportFinalStatus(ctx context.Context, logger *zap.SugaredL r.run.Clients.ConsoleUI.SetParams(maptemplate) if event.InstallationID > 0 { - event.Provider.WebhookSecret, _ = pipelineascode.GetCurrentNSWebhookSecret(ctx, r.kinteract) + event.Provider.WebhookSecret, _ = pipelineascode.GetCurrentNSWebhookSecret(ctx, r.kinteract, r.run) } else { if err := pipelineascode.SecretFromRepository(ctx, r.run, r.kinteract, provider.GetConfig(), event, repo, logger); err != nil { return repo, fmt.Errorf("cannot get secret from repository: %w", err) @@ -189,6 +188,7 @@ func (r *Reconciler) reportFinalStatus(ctx context.Context, logger *zap.SugaredL if err != nil { return repo, fmt.Errorf("cannot get pipeline: %w", err) } + if err := r.updatePipelineRunToInProgress(ctx, logger, repo, pr); err != nil { return repo, fmt.Errorf("failed to update status: %w", err) } @@ -203,7 +203,6 @@ func (r *Reconciler) updatePipelineRunToInProgress(ctx context.Context, logger * if err != nil { return fmt.Errorf("cannot update state: %w", err) } - p, event, err := r.detectProvider(ctx, logger, pr) if err != nil { logger.Error(err) @@ -211,7 +210,7 @@ func (r *Reconciler) updatePipelineRunToInProgress(ctx context.Context, logger * } if event.InstallationID > 0 { - event.Provider.WebhookSecret, _ = pipelineascode.GetCurrentNSWebhookSecret(ctx, r.kinteract) + event.Provider.WebhookSecret, _ = pipelineascode.GetCurrentNSWebhookSecret(ctx, r.kinteract, r.run) } else { if err := pipelineascode.SecretFromRepository(ctx, r.run, r.kinteract, p.GetConfig(), event, repo, logger); err != nil { return fmt.Errorf("cannot get secret from repo: %w", err) diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index f1b91c2dd..2cb53fb59 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -125,11 +125,6 @@ func TestReconciler_ReconcileKind(t *testing.T) { secretName := secrets.GenerateBasicAuthSecretName() ctx = info.StoreCurrentControllerName(ctx, "default") - ctx = info.StoreInfo(ctx, "default", &info.Info{ - Controller: &info.ControllerInfo{ - Secret: secretName, - }, - }) pr.Annotations = map[string]string{ keys.GitAuthSecret: secretName, @@ -205,6 +200,9 @@ func TestReconciler_ReconcileKind(t *testing.T) { SecretAutoCreation: true, }, }, + Controller: &info.ControllerInfo{ + Secret: secretName, + }, }, }, pipelineRunLister: stdata.PipelineLister, diff --git a/test/github_config_maxkeepruns_test.go b/test/github_config_maxkeepruns_test.go index 19be77c45..c566d8310 100644 --- a/test/github_config_maxkeepruns_test.go +++ b/test/github_config_maxkeepruns_test.go @@ -18,7 +18,7 @@ import ( ) func TestGithubMaxKeepRuns(t *testing.T) { - ctx := context.TODO() + ctx := context.Background() g := &tgithub.PRTest{ Label: "Github MaxKeepRun config", YamlFiles: []string{"testdata/pipelinerun-max-keep-run-1.yaml"}, diff --git a/test/github_incoming_test.go b/test/github_incoming_test.go index e1ddcd919..74df55c20 100644 --- a/test/github_incoming_test.go +++ b/test/github_incoming_test.go @@ -134,7 +134,7 @@ func verifyIncomingWebhook(t *testing.T, randomedString string, entries map[stri assert.NilError(t, err) defer httpResp.Body.Close() runcnx.Clients.Log.Infof("Kicked off on incoming URL: %s", incomingURL) - assert.Assert(t, httpResp.StatusCode >= 200 && httpResp.StatusCode < 300) + assert.Assert(t, httpResp.StatusCode >= 200 && httpResp.StatusCode < 300, "http status Code should be %d", httpResp.StatusCode) // to re enable after debugging... g := tgithub.PRTest{ Cnx: runcnx, diff --git a/test/pkg/cctx/ctx.go b/test/pkg/cctx/ctx.go index 92dca3694..a023d39a6 100644 --- a/test/pkg/cctx/ctx.go +++ b/test/pkg/cctx/ctx.go @@ -20,7 +20,6 @@ func GetControllerCtxInfo(ctx context.Context, run *params.Run) (context.Context ctx = info.StoreNS(ctx, ns) run.Info.Controller = info.GetControllerInfoFromEnvOrDefault() run.Clients.Log.Infof("Pipelines as Code Controller: %+v", run.Info.Controller) - ctx = info.StoreInfo(ctx, run.Info.Controller.Name, &run.Info) ctx = info.StoreCurrentControllerName(ctx, run.Info.Controller.Name) return ctx, nil } diff --git a/test/pkg/github/setup.go b/test/pkg/github/setup.go index d19b57e9e..c418b4faf 100644 --- a/test/pkg/github/setup.go +++ b/test/pkg/github/setup.go @@ -78,6 +78,7 @@ func Setup(ctx context.Context, onSecondController, viaDirectWebhook bool) (cont run.Info.Controller = info.GetControllerInfoFromEnvOrDefault() e2eoptions := options.E2E{Organization: split[0], Repo: split[1], DirectWebhook: viaDirectWebhook, ControllerURL: controllerURL} gprovider := github.New() + gprovider.Run = run event := info.NewEvent() if githubToken == "" && !viaDirectWebhook {