feat: Fixed manifest-generate-paths monorepo support for Bitbucket #18968 #20993

rahulbollisetty · 2024-11-28T16:21:13Z

PR Summary:

This PR addresses an issue with the argocd.argoproj.io/manifest-generate-paths Manifest Paths Annotation in Argo CD when used with Bitbucket repositories.

Key Changes:

Prevents spurious refreshes in cases where multiple Applications are using the same Git monorepo stored in Bitbucket.
Ensures that the manifest-generate-paths annotation is handled correctly, refreshing the app only when the files changed match with the paths in manifest-generate-paths .

Steps to Implement:

Add Bitbucket Repository to ArgoCD:
Use the following command to add the Bitbucket repository to ArgoCD with authentication:
```
argocd repo add <REPO_URL> --username <BITBUCKET_USERNAME> --password <BITBUCKET_APP_PASSWORD>
```
- <REPO_URL>: Replace with your Bitbucket repository URL.
- <BITBUCKET_USERNAME>: Your Bitbucket account username.
- <BITBUCKET_APP_PASSWORD>: An app password generated in Bitbucket (can be found in your Bitbucket account setting
Generate an Access Token for the Repository in Bitbucket:
- Create an access token from your Bitbucket repository.
- Use the access token in your repository's secret.
Store the Access Token as a Secret:
Convert the generated access token to Base64 format. You can use the following command:
```
echo "ACCESS_TOKEN" | base64 -w0
```
- Replace ACCESS_TOKEN with your actual Bitbucket access token.
- Use the access token generated in the previous step to add a Kubernetes secret for the repository added in Step 1.Store the access token in the secret under the key data.accessToken

Checklist:

…#18968 Signed-off-by: B Rahul <rahul20bollisetty@gmail.com>

bunnyshell · 2024-11-28T16:21:21Z

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

🚀 /bns:deploy to deploy the environment

codecov · 2024-11-28T17:10:36Z

Codecov Report

Attention: Patch coverage is 12.08791% with 80 lines in your changes missing coverage. Please review.

Project coverage is 54.98%. Comparing base (9a14d7f) to head (c4ba9ad).
Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
util/webhook/webhook.go	15.27%	59 Missing and 2 partials ⚠️
util/settings/settings.go	0.00%	19 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #20993      +/-   ##
==========================================
- Coverage   55.07%   54.98%   -0.10%     
==========================================
  Files         324      324              
  Lines       55466    55552      +86     
==========================================
- Hits        30547    30543       -4     
- Misses      22303    22388      +85     
- Partials     2616     2621       +5

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

andrii-korotkov-verkada · 2024-11-29T04:19:35Z

util/settings/settings.go

+		if secretRepoURL == repoURL {
+			if tokenBytes, exists := secret.Data["accessToken"]; exists {
+				return string(tokenBytes), nil
+			}


I'm a bit uncomfortable with using selector and processing multiple items. What if somebody hijacks the flow by adding a new secret with a different/wrong token?

Could you please elaborate a bit more about this?

Why don't we just query a specific secret?

well see, if we are adding a private repo the secret that are created are of the format repo-178100904 , so you cannot directly query a specific secret, you have to iterate all the repository type secret and the select the one that matches with the URL.

I think we can have the same repository-access-token (RAT) used while creating the repository for doing a call from the webhook as well.

andrii-korotkov-verkada · 2024-11-29T04:20:52Z

util/webhook/webhook.go

+	// Check if a handler was passed
+	var webhookHandler *ArgoCDWebhookHandler
+	if len(optionalHandler) > 0 {
+		if handler, ok := optionalHandler[0].(*ArgoCDWebhookHandler); ok {


This leads to a confusing function signature, which implies multiple optional handlers can be passed. Maybe use a pointer and have it be nil if the optional handler is not available?

fixed pls check

andrii-korotkov-verkada · 2024-11-29T04:21:33Z

util/webhook/webhook.go

+	// https://developer.atlassian.com/cloud/bitbucket/rest/api-group-commits/#api-repositories-workspace-repo-slug-diffstat-spec-get
+
+	url := fmt.Sprintf("https://api.bitbucket.org/2.0/repositories/%s/%s/diffstat/%s",
+		workspace, repoSlug, spec)


Fix formatting, either one argument per line or all in one line.

fixed pls check

Signed-off-by: B Rahul <rahul20bollisetty@gmail.com>

andrii-korotkov-verkada · 2024-11-29T05:43:47Z

util/webhook/webhook.go

+
+			if webhookHandler != nil {
+				spec := change.shaAfter + ".." + change.shaBefore
+				accessToken, err := webhookHandler.settingsSrc.GetRepositorySecretAccessToken(webURLs[0])


webURLs[0] doesn't read well here, i.e. what makes index 0 special?

Pls have a look at the webhook payload, you can see that the bitbucket includes multiple changes in a single event, so I have appended the URL of repo in webURL. So only one webURLs is present in case of bitbucket, whereas it is different for github, gitlab, etc. So it has to be a array, wherein only one item is present in case of bitbucket. That's why index 0 is used.

andrii-korotkov-verkada · 2024-11-29T05:46:53Z

util/webhook/webhook.go

+		return nil, fmt.Errorf("error creating request: %w", err)
+	}
+
+	trimmedAccessToken := strings.TrimSpace(accessToken)


Why do we have to trim it? Why can't it be without spaces?

Whenever fetching the accessToken from the secrets, a new line is added by default. When this accessToken is passed without removing the spaces/newline, the HTTP GET call fails, with error - invalid header field value for \"Authorization\""

andrii-korotkov-verkada · 2024-11-29T06:08:19Z

util/webhook/webhook.go

 	refParts := strings.SplitN(ref, "/", 3)
 	return refParts[len(refParts)-1]
 }

 // affectedRevisionInfo examines a payload from a webhook event, and extracts the repo web URL,
 // the revision, and whether or not this affected origin/HEAD (the default branch of the repository)
-func affectedRevisionInfo(payloadIf interface{}) (webURLs []string, revision string, change changeInfo, touchedHead bool, changedFiles []string) {
+func affectedRevisionInfo(payloadIf interface{}, optionalHandler *ArgoCDWebhookHandler) (webURLs []string, revision string, change changeInfo, touchedHead bool, changedFiles []string) {


Why not just call this webhookHandler and use it directly without declaring extra vars?

fixed it, please check

Signed-off-by: B Rahul <rahul20bollisetty@gmail.com>

nitishfy

I'll spend some time taking a look at the code you've written so far. Meanwhile, can you briefly explain how did you solve this problem?

rahulbollisetty · 2024-11-29T16:25:33Z

I'll spend some time taking a look at the code you've written so far. Meanwhile, can you briefly explain how did you solve this problem?

The webhook payload didn't consist of files changed. Bitbucket provides API, which gives the path of files that are changed. Using the webhook payload, I pull out the hash of new and old commit, workspace name and repoSlug, and then get the access token from repo secret (repo secret is created when a repo is added using argocd repo add command, then user has manually add the accessToken inside the data key of repo secret), using these I make a GET call. I have added a method (GetRepositorySecretAccessToken) in util/settings.go to get the secret, as it was the best possible option to get the secret from the added repo when called from util/webhook.go. To access the util/settings.go , I have modified the signature of affectedRevisionInfo in util/webhook.go specifically for bitbucket.

jarlebh · 2025-01-10T07:51:46Z

We really need this fix, any timeline for getting this merged ?

nitishfy · 2025-01-16T17:12:54Z

I'll start with reviewing this PR in depth now.

util/webhook/webhook.go

nitishfy

Tested and works well. We need to update the docs to tell about the implementation which can be done in the same PR.

nitishfy · 2025-01-20T16:44:42Z

util/webhook/webhook.go

+	// Getting the files changed from diff API:
+	// https://developer.atlassian.com/cloud/bitbucket/rest/api-group-commits/#api-repositories-workspace-repo-slug-diffstat-spec-get


Suggested change

// Getting the files changed from diff API:

// https://developer.atlassian.com/cloud/bitbucket/rest/api-group-commits/#api-repositories-workspace-repo-slug-diffstat-spec-get

// fetchDiffstatFromBitbucket gets the files changed from diff API - https://developer.atlassian.com/cloud/bitbucket/rest/api-group-commits/#api-repositories-workspace-repo-slug-diffstat-spec-get

Please add this as a function comment (at the top of function)

Co-authored-by: Nitish Kumar <justnitish06@gmail.com> Signed-off-by: B Rahul <rahul20bollisetty@gmail.com>

crenshaw-dev · 2025-01-31T16:14:29Z

util/webhook/webhook.go

@@ -244,14 +271,66 @@ func affectedRevisionInfo(payloadIf interface{}) (webURLs []string, revision str
 	return webURLs, revision, change, touchedHead, changedFiles
 }

+func fetchDiffstatFromBitbucket(workspace, repoSlug, spec, accessToken string) ([]string, error) {


I'm concerned about this. iirc our webhook handler isn't auth-protected. So anyone with API access could hammer the endpoint and trigger SSRF. I don't have time to give a thorough review right now, but requesting changes so this doesn't get merged without further conversation.

anandf · 2025-02-07T01:26:50Z

I have created an improved version of this PR. #21811
the improvements I have done in my branch are,

Use existing repo secret for the webhook callback whereas the original one required a new kind of secret to store auth tokens for making calls from within the webhook handler.
Use BitBucket APIs to do the REST call instead of using http client.

nitishfy · 2025-02-19T09:50:21Z

Closing this in favour of #21811

Fixed manifest-generate-paths monorepo support for Bitbucket argoproj…

390df11

…#18968 Signed-off-by: B Rahul <rahul20bollisetty@gmail.com>

rahulbollisetty requested a review from a team as a code owner November 28, 2024 16:21

rahulbollisetty changed the title ~~Fixed manifest-generate-paths monorepo support for Bitbucket #18968~~ feat: Fixed manifest-generate-paths monorepo support for Bitbucket #18968 Nov 28, 2024

andrii-korotkov-verkada reviewed Nov 29, 2024

View reviewed changes

Modified affectedRevisionInfo signature and formatting code

da976f5

Signed-off-by: B Rahul <rahul20bollisetty@gmail.com>

andrii-korotkov-verkada reviewed Nov 29, 2024

View reviewed changes

Fixed function signature

c4ba9ad

Signed-off-by: B Rahul <rahul20bollisetty@gmail.com>

andrii-korotkov-verkada approved these changes Nov 29, 2024

View reviewed changes

andrii-korotkov-verkada added the ready-for-review label Nov 29, 2024

nitishfy reviewed Nov 29, 2024

View reviewed changes

andrii-korotkov-verkada mentioned this pull request Nov 30, 2024

application doesn't refresh automatically after revision branch is being created, although a webhook exists #20922

Open

bashlion approved these changes Jan 5, 2025

View reviewed changes

nitishfy self-assigned this Jan 16, 2025

nitishfy reviewed Jan 20, 2025

View reviewed changes

util/webhook/webhook.go Outdated Show resolved Hide resolved

nitishfy approved these changes Jan 20, 2025

View reviewed changes

nitishfy reviewed Jan 20, 2025

View reviewed changes

nitishfy approved these changes Jan 20, 2025

View reviewed changes

Update util/webhook/webhook.go

ac55994

Co-authored-by: Nitish Kumar <justnitish06@gmail.com> Signed-off-by: B Rahul <rahul20bollisetty@gmail.com>

nitishfy mentioned this pull request Jan 23, 2025

REQUEST: Promotion to Reviewer for nitishfy argoproj/argoproj#349

Closed

6 tasks

crenshaw-dev requested changes Jan 31, 2025

View reviewed changes

anandf mentioned this pull request Feb 7, 2025

feat(webhook): Fixed manifest-generate-paths annotation support for monorepos in BitBucket #21811

Merged

14 tasks

nitishfy closed this Feb 19, 2025

		// Getting the files changed from diff API:
		// https://developer.atlassian.com/cloud/bitbucket/rest/api-group-commits/#api-repositories-workspace-repo-slug-diffstat-spec-get

	// Getting the files changed from diff API:
	// https://developer.atlassian.com/cloud/bitbucket/rest/api-group-commits/#api-repositories-workspace-repo-slug-diffstat-spec-get
	// fetchDiffstatFromBitbucket gets the files changed from diff API - https://developer.atlassian.com/cloud/bitbucket/rest/api-group-commits/#api-repositories-workspace-repo-slug-diffstat-spec-get

feat: Fixed manifest-generate-paths monorepo support for Bitbucket #18968 #20993

feat: Fixed manifest-generate-paths monorepo support for Bitbucket #18968 #20993

Uh oh!

Conversation

rahulbollisetty commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary:

Key Changes:

Steps to Implement:

Uh oh!

bunnyshell bot commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Preview Environment deleted from Bunnyshell

Uh oh!

codecov bot commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitishfy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahulbollisetty commented Nov 29, 2024

Uh oh!

jarlebh commented Jan 10, 2025

Uh oh!

nitishfy commented Jan 16, 2025

Uh oh!

Uh oh!

nitishfy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anandf commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nitishfy commented Feb 19, 2025

Uh oh!

Uh oh!

rahulbollisetty commented Nov 28, 2024 •

edited

Loading

bunnyshell bot commented Nov 28, 2024 •

edited

Loading

codecov bot commented Nov 28, 2024 •

edited

Loading

nitishfy left a comment •

edited

Loading

nitishfy left a comment •

edited

Loading

anandf commented Feb 7, 2025 •

edited

Loading