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]>
enarha · Mar 12, 2024 · f32eb1e · f32eb1e
1 parent 226781a
commit f32eb1e
Show file tree

Hide file tree

Showing 28 changed files with 71 additions and 193 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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,

diff --git a/pkg/cmd/tknpac/info/install_test.go b/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()

diff --git a/pkg/kubeinteraction/labels.go b/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")
 	}
+	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
@@ -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)

diff --git 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
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
 	}