feat: Support Azure Service Principal authentication for Azure DevOps repositories

[allanyung]

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Title of the PR
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.
• Optional. My organization is added to USERS.md.
• Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

Closes #25220

Notes:

I used the GitHub App auth feature #5355 as a reference of how to implement this and followed conventions from there.

I can confirm that in a local build:

• The GUI can be used to add repositories and templates
• The argocd CLI can be used to add repositories and templates
• The repository returns an error and does not save if the connection test fails
• Applications and ApplicationSets that reference a repository render successfully

About tests:

I used the azure-sdk-for-go library. Unfortunately, the design of it means that it's not really possible to override the GetToken function to return a static token rather than attempt to connect to Azure Entra ID. As such there aren't any unit tests. Also, the GitHub App auth PR didn't include any tests either so hopefully this is acceptable

[bunnyshell]

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

• 🚀 /bns:deploy to deploy the environment

[allanyung]

Looks like the e2e test infra is unstable. The e2e checks have all independently passed, but not in the same CI run. So as to not waste resources, I won't push anymore CI triggers until I hear from a maintainer. Thanks!

[codecov]

Codecov Report

❌ Patch coverage is 56.97674% with 111 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.63%. Comparing base (6f21978) to head (337e55b).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
util/git/creds.go	2.63%	73 Missing and 1 partial ⚠️
cmd/argocd/commands/repo.go	12.12%	29 Missing ⚠️
cmd/util/repo.go	0.00%	4 Missing ⚠️
util/git/client.go	33.33%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #25324      +/-   ##
==========================================
+ Coverage   62.61%   62.63%   +0.02%     
==========================================
  Files         353      353              
  Lines       50147    50314     +167     
==========================================
+ Hits        31398    31516     +118     
- Misses      15732    15770      +38     
- Partials     3017     3028      +11     

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

[allanyung]

Looks like the e2e test infra is unstable. The e2e checks have all independently passed, but not in the same CI run. So as to not waste resources, I won't push anymore CI triggers until I hear from a maintainer. Thanks!

Turns out it was the Cloudflare outage. All passing now 🎉

[ppapapetrou76]

I can't review the UI parts - I only reviewed the golang code

[allanyung]

@ppapapetrou76 thank you for the review. I've pushed a commit that addresses your comments

[ppapapetrou76]

LGTM - thanks for addressing my comments

[ppapapetrou76]

I see you added more commits .
When you do so please ask for a new set of reviews

[allanyung]

I see you added more commits . When you do so please ask for a new set of reviews

Apologies, you did in fact already review these changes but I rebased and force pushed as master branch advanced and the PR could no longer be cleanly merged. Is this generally the preferred way, or would doing a merge commit of master into my branch be better?

[ppapapetrou76]

LGTM 🏅 - thanks for addressing my comments

[allanyung]

As requested in the contributer meeting, here are some screenshots demonstrating the changes

• The connect repo UI with the new settings
  
• The repo has been added and the connection is successful
  
• Additional input appears when choosing Azure Other Cloud
  

[blakepettersson]

See #26174, lmk what you think

[allanyung]

See #26174, lmk what you think

@blakepettersson I agree with the proposal in principle, however I don't believe it's applicable for the case that this PR aims to solve.

The situation we have is:

• A large number of multitenant EKS clusters where:
  • There is a single, cluster wide ArgoCD deployment
  • RBAC is configured such that each tenant only has access to their own namespace, and an AppProject that matches said namespace
  • Each tenant has git repositories in Azure DevOps
  • Each tenant has their own Azure Service Principal (client_id and client_secret) that has access to their own git repositories

The K8s clusters are EKS, yet need to auth to Azure DevOps (don't ask 🙈). As such workload identity cannot be used for this, and even if it could, it wouldn't be appropriate as the single identity would need to have access to all tenant repositories.

Hope this makes sense and this PR doesn't need heavy modifications

[blakepettersson]

The K8s clusters are EKS, yet need to auth to Azure DevOps (don't ask 🙈). As such workload identity cannot be used for this

Currently (speaking of only the Azure part of this) the proposal only accounts for the direct exchange of Azure -> ACR tokens, but I don't see a reason why it couldn't be extended to do EKS (via the EKS OIDC provider) -> Azure -> ACR

and even if it could, it wouldn't be appropriate as the single identity would need to have access to all tenant repositories.

The intent of my proposal is to be able to scope identity per app project (by requiring a separate k8s service account for each argo app project that wants to make use of it).

[allanyung]

Currently (speaking of only the Azure part of this) the proposal only accounts for the direct exchange of Azure -> ACR tokens, but I don't see a reason why it couldn't be extended to do EKS (via the EKS OIDC provider) -> Azure -> ACR

I'm still failing to see how this would work for Service Principal auth to git repositories - you need to provide tenantID, clientID and clientSecret to exchange for the short lived token. For me, this feels more like the GitHub App auth method, where you're required to pass in a private key for the token exchange.

I'm not sure how these secrets can be "natively integrated with cloud provider identity systems"

The intent of my proposal is to be able to scope identity per app project (by requiring a separate k8s service account for each argo app project that wants to make use of it).

This wouldn't really work for our use case either:

• A team owns the EKS cluster and the AWS account in which it lives
• The single cluster scoped ArgoCD is owned by the same team
• Each tenant for this cluster has their own AWS account and Azure DevOps project with one or more git repositories
• Each tenant has a Service Principal with access to their git repositories, which is added to ArgoCD via UI or k8s Secret provisioned by External Secrets Operator

As per my comment above I don't think a service account per project helps as you still need to store the per tenant clientSecret etc somewhere.

One of the proposal goals is

Backward compatibility: Existing repositories with static credentials continue to work unchanged.

Our ideal outcome is that this PR is merged as is and then we would use it in the static credentials way. It's still not clear to me how this would work with the new proposed system, but as long as the method in this PR exists, that's no problem.

I'm concerned that if we were not to merge this PR, then what would happen is that an alternative method is introduced that doesn't actually support our requirements. As this PR is not yet an "existing repository" there is then no obligation to maintain support for it.