Fix empty secret namespace issue in reconciler #1753

zakisk · 2024-08-20T09:44:51Z

Fixed empty secret namespace issue in reconcile when git proivder is not github. added E2E tests for gitea as well to confirm this behavior.

What's happening?

Let's take a scenario that I got 5 PipelineRun and I set concurrency_limit to 3 so when PRs are acquired in queue_pipelinerun from semaphore (I got 3 PRs in acquired list) and looped, for first PipelineRun r.secretNS value is empty in the loop at this point when calling updatePipelineRunToInProgress and updatePipelineRunToInProgress returning error from this line saying "an empty namespace may not be set when a resource name is provided" and loop is ending on first PR leaving other two PRs (concurrency_limit - 1 PRs) in queued state and as they are removed from semaphore they aren't no more in acquired list when next time ReconcileKind is called. This PR addresses that bug.

https://issues.redhat.com/browse/SRVKP-5897

codecov · 2024-08-20T09:49:45Z

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 65.07%. Comparing base (5173774) to head (1f35118).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/reconciler/reconciler.go	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1753      +/-   ##
==========================================
- Coverage   65.11%   65.07%   -0.04%     
==========================================
  Files         174      174              
  Lines       13137    13142       +5     
==========================================
- Hits         8554     8552       -2     
- Misses       4018     4025       +7     
  Partials      565      565

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

test/gitea_test.go

enarha · 2024-08-20T16:31:18Z

pkg/reconciler/reconciler.go

+		// in ReconcileKind this r.secretNS is not set and it is needed when git provider
+		// is not github.
+		r.secretNS = repo.GetNamespace()
+		if repo.Spec.GitProvider != nil && repo.Spec.GitProvider.Secret == nil && r.globalRepo.Spec.GitProvider != nil && r.globalRepo.Spec.GitProvider.Secret != nil {


We have the same code in reportFinalStatus. Can we have it in a single place? I mean if we have it in updatePipelineRunToInProgress, is it really needed in reportFinalStatus as well? Is it possible that reportFinalStatus is called without updatePipelineRunToInProgress being called earlier?

Is it possible that reportFinalStatus is called without updatePipelineRunToInProgress being called earlier?

@enarha yes it is possible that reportFinalStatus will be called without updatePipelineRunToInProgress if concurrency_limit is not set and all PipelineRuns are provisioned in STARTED state by controller.

Understood. Isn't there a common path (always executed) where we can move that code?

Understood. Isn't there a common path (always executed) where we can move that code?

🤔 Yes, at the beginning of ReconcileKind it can be.

But we don't have repo reference object there in ReconcileKind, should we fetch repo object there?

should we fetch repo object there?

@enarha if yes then I will need to fetch repo in ReconcileKind and change queue_pipelineruns func signature to pass repo.

piyush-garg · 2024-08-21T12:49:47Z

pkg/reconciler/reconciler.go

@@ -234,6 +234,13 @@ func (r *Reconciler) updatePipelineRunToInProgress(ctx context.Context, logger *
 	if event.InstallationID > 0 {
 		event.Provider.WebhookSecret, _ = pac.GetCurrentNSWebhookSecret(ctx, r.kinteract, r.run)
 	} else {
+		// in ReconcileKind this r.secretNS is not set and it is needed when git provider
+		// is not github.
+		r.secretNS = repo.GetNamespace()


Do we need to set this in reconciler field? can't we just pass the value while getting the secret, instead of setting it on reconciler?

@piyush-garg I think r.secretNS is the field which is meant for fetching secret for PipelineRun, if you see its uses it is used only two times once in reportFinalStatus and in updatePipelineRunToInProgress.

but in case of global repo and all, setting this param can may be have adverse affect on next event etc, i would rather skip it for now like setting in reconciler

I m not sure about the effect now but when i went through the code i see even r.secretNS is set either by repo.GetNamespace() or r.secretNS = r.globalRepo.GetNamespace() for globalRepo

@piyush-garg do you think it cause some issue 🤔

I see the point of @piyush-garg even if i don't think it would cause problem but i think @zakisk you can remove that variable being on the struct and instead be scoped to the function that would be at least a saner code.

chmouel · 2024-09-09T12:28:54Z

can you explain a bit more the problem statement of what you are solving, i am guessing myself and other reviewers can guess it from your PR but neither the SRVKP or the PR description describe how this bugs occurring and how to replicate it.

chmouel

pr is fine, just a few cleaning comment that would be needed....

test/gitea_test.go

chmouel · 2024-09-09T12:37:08Z

pkg/reconciler/reconciler.go

@@ -234,6 +234,13 @@ func (r *Reconciler) updatePipelineRunToInProgress(ctx context.Context, logger *
 	if event.InstallationID > 0 {
 		event.Provider.WebhookSecret, _ = pac.GetCurrentNSWebhookSecret(ctx, r.kinteract, r.run)
 	} else {
+		// in ReconcileKind this r.secretNS is not set and it is needed when git provider
+		// is not github.
+		r.secretNS = repo.GetNamespace()


I see the point of @piyush-garg even if i don't think it would cause problem but i think @zakisk you can remove that variable being on the struct and instead be scoped to the function that would be at least a saner code.

zakisk · 2024-09-09T14:23:21Z

how this bugs occurring?

@chmouel when PRs are acquired in queue_pipelinerun from semaphore and looped, (let assume that I got 5 PR and concurrency_limit is 3 so I got 3 PRs in acquired list), for first PipelineRun r.secretNS value is empty in the loop at this point when calling updatePipelineRunToInProgress and updatePipelineRunToInProgress returning error from this line saying "an empty namespace may not be set when a resource name is provided" and loop is ending on first PR leaving other two PRs (concurrency_limit - 1 PRs) in queued state and as they are removed from semaphore they aren't no more in acquired list when next time ReconcileKind is called.

zakisk · 2024-09-09T14:34:45Z

how to reproduce it?

I had encounter this while I was writing gitea tests for SRVKP-5560 (PR link), so if you run tests of this PR without that r.secretNS assigment code, or any other existing concurrency test with concurrency_limit greater than 1, you will get that bug e.g. TestGiteaConcurrencyExclusivenessMultiplePipelines.

fixed empty secret namespace issue in reconcile when git proivder is not github e.g. gitea, gitlab etc. added E2E tests as well to confirm behaviour. https://issues.redhat.com/browse/SRVKP-5897 Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>

chmouel · 2024-09-09T17:26:25Z

how this bugs occurring?

@chmouel when PRs are acquired in queue_pipelinerun from semaphore and looped, (let assume that I got 5 PR and concurrency_limit is 3 so I got 3 PRs in acquired list), for first PipelineRun r.secretNS value is empty in the loop at this point when calling updatePipelineRunToInProgress and updatePipelineRunToInProgress returning error from this line saying "an empty namespace may not be set when a resource name is provided" and loop is ending on first PR leaving other two PRs (concurrency_limit - 1 PRs) in queued state and as they are removed from semaphore they aren't no more in acquired list when next time ReconcileKind is called.

great but can you add this to the PR description, the git commit and the SRVKP ? that will makes easier for everyone to know what the code was doing..

chmouel · 2024-09-09T17:27:19Z

how to reproduce it?

I had encounter this while I was writing gitea tests for SRVKP-5560 (PR link), so if you run tests of this PR without that r.secretNS assigment code, or any other existing concurrency test with concurrency_limit greater than 1, you will get that bug e.g. TestGiteaConcurrencyExclusivenessMultiplePipelines.

yeah just wanted you add this to the PR description to explain to the reviewer like me (or qe engineers) when looking at the PR how to test your PR easily without having to read the code.....

zakisk · 2024-09-09T19:59:55Z

great but can you add this to the PR description, the git commit and the SRVKP ? that will makes easier for everyone to know what the code was doing..

@chmouel oh, ok I will, I missed that.

zakisk · 2024-09-10T04:09:37Z

yeah just wanted you add this to the PR description to explain to the reviewer like me (or qe engineers) when looking at the PR how to test your PR easily without having to read the code.....

Added reproduction steps to SRVKP.

chmouel · 2024-09-10T07:51:33Z

LGTM, good work @zakisk

zakisk requested review from savitaashture, piyush-garg and enarha August 20, 2024 09:48

savitaashture reviewed Aug 20, 2024

View reviewed changes

test/gitea_test.go Outdated Show resolved Hide resolved

zakisk force-pushed the SRVKP-5897 branch from 3ed5af5 to ce88d46 Compare August 20, 2024 14:34

enarha reviewed Aug 20, 2024

View reviewed changes

piyush-garg reviewed Aug 21, 2024

View reviewed changes

zakisk force-pushed the SRVKP-5897 branch 2 times, most recently from 076fd82 to e99eb87 Compare September 2, 2024 09:33

chmouel reviewed Sep 9, 2024

View reviewed changes

zakisk force-pushed the SRVKP-5897 branch from e99eb87 to 1f35118 Compare September 9, 2024 15:27

chmouel merged commit 77e61ea into openshift-pipelines:main Sep 10, 2024
4 checks passed

github-actions bot mentioned this pull request Sep 20, 2024

[openshift-pipelines/pipelines-as-code] v0.28.0 vdemeester/chick-group#89

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix empty secret namespace issue in reconciler #1753

Fix empty secret namespace issue in reconciler #1753

zakisk commented Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 20, 2024 •

edited

Loading

enarha Aug 20, 2024

zakisk Aug 20, 2024

enarha Aug 20, 2024

zakisk Aug 20, 2024

zakisk Aug 21, 2024

zakisk Aug 21, 2024

piyush-garg Aug 21, 2024

zakisk Aug 22, 2024

piyush-garg Aug 26, 2024

savitaashture Aug 28, 2024

chmouel Sep 9, 2024

chmouel commented Sep 9, 2024

chmouel left a comment

chmouel Sep 9, 2024

zakisk commented Sep 9, 2024

zakisk commented Sep 9, 2024

chmouel commented Sep 9, 2024

chmouel commented Sep 9, 2024

zakisk commented Sep 9, 2024

zakisk commented Sep 10, 2024

chmouel commented Sep 10, 2024

Fix empty secret namespace issue in reconciler #1753

Fix empty secret namespace issue in reconciler #1753

Conversation

zakisk commented Aug 20, 2024 • edited Loading

What's happening?

codecov bot commented Aug 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chmouel commented Sep 9, 2024

chmouel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zakisk commented Sep 9, 2024

zakisk commented Sep 9, 2024

chmouel commented Sep 9, 2024

chmouel commented Sep 9, 2024

zakisk commented Sep 9, 2024

zakisk commented Sep 10, 2024

chmouel commented Sep 10, 2024

zakisk commented Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 20, 2024 •

edited

Loading