feat: Add support for Azure workload identity for Git and OCI repositories. #21118

jagpreetstamber · 2024-12-10T15:36:02Z

PR implements Azure workload identity authentication mechanism for authenticating with the Azure Git and OCI repositories

Azure Workload Identity enables the credential free authentication for Azure customers, enabling this feature will remove the credential management overhead from customers using argo on Azure Kubernetes clusters.

Checklist:

bunnyshell · 2024-12-10T15:36:10Z

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

🔵 /bns:start to start the environment
🚀 /bns:deploy to redeploy the environment
❌ /bns:delete to remove the environment

codecov · 2024-12-10T18:14:34Z

Codecov Report

Attention: Patch coverage is 61.42857% with 135 lines in your changes missing coverage. Please review.

Project coverage is 55.24%. Comparing base (8126508) to head (493651e).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
util/helm/creds.go	80.51%	22 Missing and 8 partials ⚠️
util/helm/cmd.go	46.15%	21 Missing and 7 partials ⚠️
util/helm/client.go	50.00%	9 Missing and 6 partials ⚠️
pkg/apis/application/v1alpha1/repository_types.go	22.22%	12 Missing and 2 partials ⚠️
util/git/creds.go	65.00%	11 Missing and 3 partials ⚠️
util/util.go	0.00%	8 Missing ⚠️
util/git/client.go	0.00%	7 Missing ⚠️
util/db/repository_secrets.go	57.14%	4 Missing and 2 partials ⚠️
util/helm/helm.go	20.00%	2 Missing and 2 partials ⚠️
util/workloadidentity/workloadidentity.go	73.33%	4 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #21118      +/-   ##
==========================================
+ Coverage   55.19%   55.24%   +0.04%     
==========================================
  Files         337      339       +2     
  Lines       57058    57352     +294     
==========================================
+ Hits        31496    31685     +189     
- Misses      22863    22948      +85     
- Partials     2699     2719      +20

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

util/helm/cmd.go

todaywasawesome · 2024-12-19T16:36:20Z

reposerver/repository/repository.go

@@ -1008,6 +1009,7 @@ func getHelmRepos(appPath string, repositories []*v1alpha1.Repository, helmRepoC
 					if _, err := url.Parse("oci://" + dep.Repo); err == nil && cred.EnableOCI && strings.HasPrefix(dep.Repo, cred.Repo) {
 						repo.Username = cred.Username
 						repo.Password = cred.Password
+						repo.UseAzureWorkloadIdentity = cred.UseAzureWorkloadIdentity


@blakepettersson please review

todaywasawesome · 2024-12-19T16:38:09Z

@reggie-k @andrii-korotkov-verkada Can you take a look? We're shooting for adding this in 2.15.

andrii-korotkov-verkada

Generally LGTM, mostly minor cpomments

util/git/creds.go

util/git/creds_test.go

util/helm/creds.go

andrii-korotkov-verkada · 2024-12-20T17:03:05Z

Please, fix the DCO (click on the DCO link and follow the instructions from there).

andrii-korotkov-verkada · 2024-12-20T17:03:36Z

Also, please avoid force push, since it makes it harder to review incrementally and also gets rid of reasons for updates.

jagpreetstamber · 2024-12-20T17:07:23Z

Also, please avoid force push, since it makes it harder to review incrementally and also gets rid of reasons for updates.

Sure, will take care for future changes

andrii-korotkov-verkada · 2024-12-21T15:32:13Z

Curious why you are merging from the master branch directly instead of a custom branch, and also why is commit message merge master into master. Maybe this has something to do with test artifacts step failure.

jagpreetstamber · 2024-12-23T18:19:41Z

Curious why you are merging from the master branch directly instead of a custom branch, and also why is commit message merge master into master. Maybe this has something to do with test artifacts step failure.

It is because the branch from my fork is master branch. Hence it merges from upstream master to master on my fork which results in this message.

Signed-off-by: Jagpreet Singh Tamber <jagpreetstamber@gmail.com>

jagpreetstamber · 2024-12-31T20:54:03Z

Had to force push by rebasing as the coverage report was incorrect it was considering the merges from master as the changes in the patch and reducing the code coverage. After rebasing it is now correct.

reggie-k

Thanks for the PR! Made some comments/questions.
Generally, LGTM, but I think the test coverage should be improved.

util/git/creds.go

util/helm/client_test.go

util/helm/creds.go

Signed-off-by: Jagpreet Singh Tamber <jagpreetstamber@gmail.com>

jagpreetstamber requested a review from a team as a code owner December 10, 2024 15:36

jagpreetstamber force-pushed the master branch 7 times, most recently from d15b63d to fdf129b Compare December 10, 2024 17:33

jagpreetstamber force-pushed the master branch 3 times, most recently from 24d9816 to 2defb6b Compare December 10, 2024 19:19

dtzar mentioned this pull request Dec 10, 2024

[Feature] Implement option to fetch repository credentials at runtime #10218

Open

jagpreetstamber force-pushed the master branch from 2defb6b to 2dfe9a2 Compare December 10, 2024 22:02

This was referenced Dec 10, 2024

Helm OCI repository (ACR) #6365

Open

Adding ACR repo gives 'unsupported protocol scheme' #12634

Open

jagpreetstamber force-pushed the master branch 2 times, most recently from c499219 to 94a0fbf Compare December 11, 2024 15:52

jagpreetstamber requested a review from a team as a code owner December 11, 2024 15:52

jagpreetstamber force-pushed the master branch 6 times, most recently from c0abdd4 to 06a0d1e Compare December 12, 2024 18:24

jagpreetstamber marked this pull request as draft December 12, 2024 19:02

github-advanced-security bot found potential problems Dec 12, 2024

View reviewed changes

util/helm/cmd.go Fixed Show fixed Hide fixed

jagpreetstamber force-pushed the master branch from 61b8f67 to 7202476 Compare December 12, 2024 19:36

github-advanced-security bot found potential problems Dec 12, 2024

View reviewed changes

util/helm/cmd.go Fixed Show fixed Hide fixed

jagpreetstamber force-pushed the master branch 4 times, most recently from 2716e26 to beacb05 Compare December 19, 2024 15:06

todaywasawesome reviewed Dec 19, 2024

View reviewed changes

todaywasawesome added this to the v.2.15 milestone Dec 19, 2024

todaywasawesome assigned reggie-k Dec 19, 2024

andrii-korotkov-verkada reviewed Dec 20, 2024

View reviewed changes

util/git/creds.go Outdated Show resolved Hide resolved

util/git/creds.go Outdated Show resolved Hide resolved

util/git/creds_test.go Outdated Show resolved Hide resolved

util/helm/creds.go Show resolved Hide resolved

util/helm/creds.go Show resolved Hide resolved

jagpreetstamber force-pushed the master branch from beacb05 to 0d1836a Compare December 20, 2024 17:00

jagpreetstamber force-pushed the master branch from 0d1836a to 7eff3e2 Compare December 20, 2024 17:06

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

View reviewed changes

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

jagpreetstamber added 4 commits December 31, 2024 12:18

Add support for Azure workload identity for Git and OCI repositories.

17019e7

Signed-off-by: Jagpreet Singh Tamber <jagpreetstamber@gmail.com>

Resolve comments

a1f0442

Signed-off-by: Jagpreet Singh Tamber <jagpreetstamber@gmail.com>

Lint code.

4b3b4e5

Signed-off-by: Jagpreet Singh Tamber <jagpreetstamber@gmail.com>

Lint the Errors using errors.New instead of fmt.Errorf

493651e

Signed-off-by: Jagpreet Singh Tamber <jagpreetstamber@gmail.com>

jagpreetstamber force-pushed the master branch from ee2e48c to 493651e Compare December 31, 2024 17:18

reggie-k reviewed Jan 2, 2025

View reviewed changes

Resolve Comments.

671ad35

Signed-off-by: Jagpreet Singh Tamber <jagpreetstamber@gmail.com>

jagpreetstamber force-pushed the master branch from 347ef3b to 671ad35 Compare January 2, 2025 19:03

jagpreetstamber added 3 commits January 2, 2025 14:27

add tests for workloadidentity in client_test.go.

5c5e08a

Signed-off-by: Jagpreet Singh Tamber <jagpreetstamber@gmail.com>

Merge remote-tracking branch 'upstream/master'

2c118c6

add more tests.

823ae50

Signed-off-by: Jagpreet Singh Tamber <jagpreetstamber@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for Azure workload identity for Git and OCI repositories. #21118

feat: Add support for Azure workload identity for Git and OCI repositories. #21118

jagpreetstamber commented Dec 10, 2024 •

edited

Loading

bunnyshell bot commented Dec 10, 2024 •

edited

Loading

codecov bot commented Dec 10, 2024 •

edited

Loading

todaywasawesome Dec 19, 2024

todaywasawesome commented Dec 19, 2024

andrii-korotkov-verkada left a comment

andrii-korotkov-verkada commented Dec 20, 2024

andrii-korotkov-verkada commented Dec 20, 2024

jagpreetstamber commented Dec 20, 2024

andrii-korotkov-verkada commented Dec 21, 2024

jagpreetstamber commented Dec 23, 2024

jagpreetstamber commented Dec 31, 2024

reggie-k left a comment •

edited

Loading

feat: Add support for Azure workload identity for Git and OCI repositories. #21118

Are you sure you want to change the base?

feat: Add support for Azure workload identity for Git and OCI repositories. #21118

Conversation

jagpreetstamber commented Dec 10, 2024 • edited Loading

bunnyshell bot commented Dec 10, 2024 • edited Loading

🔴 Preview Environment stopped on Bunnyshell

codecov bot commented Dec 10, 2024 • edited Loading

Codecov Report

todaywasawesome Dec 19, 2024

Choose a reason for hiding this comment

todaywasawesome commented Dec 19, 2024

andrii-korotkov-verkada left a comment

Choose a reason for hiding this comment

andrii-korotkov-verkada commented Dec 20, 2024

andrii-korotkov-verkada commented Dec 20, 2024

jagpreetstamber commented Dec 20, 2024

andrii-korotkov-verkada commented Dec 21, 2024

jagpreetstamber commented Dec 23, 2024

jagpreetstamber commented Dec 31, 2024

reggie-k left a comment • edited Loading

Choose a reason for hiding this comment

jagpreetstamber commented Dec 10, 2024 •

edited

Loading

bunnyshell bot commented Dec 10, 2024 •

edited

Loading

codecov bot commented Dec 10, 2024 •

edited

Loading

reggie-k left a comment •

edited

Loading