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 <[email protected]>

Author: chmouel

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)

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)

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{}

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 {

pkg/adapter/incoming_test.go

@@ -655,19 +655,18 @@ 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 := &params.Run{
 				Clients: clients.Clients{
 					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,

pkg/cmd/tknpac/info/install_test.go

@@ -160,18 +160,18 @@ 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 := &params.Run{
 				Clients: clients.Clients{
 					PipelineAsCode: stdata.PipelineAsCode,
 					Kube:           stdata.Kube,
 					HTTP:           *httpTestClient,
 				},
+				Info: info.Info{
+					Controller: &info.ControllerInfo{
+						Secret: name,
+					},
+				},
 			}
 
 			io, out := tcli.NewIOStream()

pkg/kubeinteraction/labels.go

@@ -1,14 +1,14 @@
 package kubeinteraction
 
 import (
-	"context"
 	"fmt"
 	"strconv"
 
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode"
 	"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")
 	}
-	// Add labels on the soon to be created pipelinerun so UI/CLI can easily
+	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
@@ -44,7 +44,7 @@ func AddLabelsAndAnnotations(ctx context.Context, event *info.Event, pipelineRun
 		// In PAC v0.20.x releases we will remove these keys from Labels
 		keys.Sender:      formatting.CleanValueKubernetes(event.Sender),
 		keys.Branch:      formatting.CleanValueKubernetes(event.BaseBranch),
-		keys.GitProvider: providerinfo.Name,
+		keys.GitProvider: providerConfig.Name,
 	}
 
 	annotations := map[string]string{
@@ -58,7 +58,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),
 	}
@@ -69,7 +69,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)
 		}

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 := &params.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)

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,
-	})
-}