feat: add service principal auth for azure pipelines scaler#7578
feat: add service principal auth for azure pipelines scaler#7578m-amaresh wants to merge 10 commits intokedacore:mainfrom
Conversation
Signed-off-by: m-amaresh <115941749+m-amaresh@users.noreply.github.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Thank you for your contribution! 🙏 Please understand that we will do our best to review your PR and give you feedback as soon as possible, but please bear with us if it takes a little longer as expected. While you are waiting, make sure to:
Once the initial tests are successful, a KEDA member will ensure that the e2e tests are run. Once the e2e tests have been successfully completed, the PR may be merged at a later date. Please be patient. Learn more about our contribution guide. |
Signed-off-by: m-amaresh <115941749+m-amaresh@users.noreply.github.com>
|
This would actually complete my feature request #7581 and remove the requirement for a PAT in Azure Container Apps. |
- Remove triggerMetadata from order tags for clientSecret, clientCertificate, and clientCertificatePassword to prevent plaintext secrets in ScaledObject spec - Remove unreachable authType fallback block in getAzurePipelineRequest - Fix pre-existing tests that relied on the fallback by setting authType explicitly - Regenerate scalers schema with clientCertificate and clientCertificatePassword fields Signed-off-by: m-amaresh <115941749+m-amaresh@users.noreply.github.com>
…pers Drop the podIdentity parameter from getAzurePipelineRequest, getPoolIDFromName, and validatePoolID — auth dispatch now uses authContext.authType set during metadata parsing, so the parameter was dead code after the SPN auth refactor. Signed-off-by: m-amaresh <115941749+m-amaresh@users.noreply.github.com>
|
Hello Team, Can I get a review? |
rickbrouwer
left a comment
rickbrouwer
left a comment
There was a problem hiding this comment.
I see that the last commit wants to remove the podIdentity parameter from getPoolIDFromName and validatePoolID. But it only suppresses it with _.
I think that the parameter is still present in both signatures (parseAzurePipelinesMetadata still pass podIdentity).
Can you check this and compare it with getAzurePipelineRequest, where the parameter is removed from the signature and all calls.
Other than that, I think everything looks good.
…atePoolID Auth is carried via metadata.authContext which is already populated before these functions are called. The parameter was suppressed with _ but still present in both signatures and all call sites. Signed-off-by: m-amaresh <115941749+m-amaresh@users.noreply.github.com>
|
Hello @rickbrouwer , Can you please review the changes. Also If it goes thru, I was not sure about next release cadence and I have added the docs in 2.20 version https://deploy-preview-1726--keda.netlify.app/docs/2.20/scalers/azure-pipelines/ |
Yes, that is correct 👍🏼 |
Signed-off-by: m-amaresh <115941749+m-amaresh@users.noreply.github.com>
![semgrep-code-kedacore[bot]](https://avatars.githubusercontent.com/u/49917779?s=60&v=4){kind=small}
Signed-off-by: m-amaresh <115941749+m-amaresh@users.noreply.github.com>
|
/run-e2e azure_pipelines |
|
@rickbrouwer the e2e test failed Can you add the SPN to ADO org and grant read permission on pools. Also can you create a certificate and certificate password and configure for e2e test. |
|
Please don't push. We will do our best to check your PR as soon as can. |
JorTurFer
left a comment
JorTurFer
left a comment
There was a problem hiding this comment.
This is something that can be used by other Azure scalers as it relies on azcore.TokenCredentials, does it make sense to create another auth that can be used for all the azure scalers instead of adding the parameters on this scaler itself?
| if meta.TenantID != "" || meta.ClientID != "" || meta.ClientSecret != "" || meta.ClientCertificate != "" || meta.ClientCertificatePassword != "" { | ||
| missing := make([]string, 0, 3) | ||
| if meta.TenantID == "" { | ||
| missing = append(missing, "tenantId") | ||
| } | ||
| if meta.ClientID == "" { | ||
| missing = append(missing, "clientId") | ||
| } | ||
| usingClientSecret := meta.ClientSecret != "" | ||
| usingClientCertificate := meta.ClientCertificate != "" | ||
| if usingClientSecret && usingClientCertificate { | ||
| return nil, fmt.Errorf("clientSecret and clientCertificate are mutually exclusive") | ||
| } | ||
| if !usingClientSecret && !usingClientCertificate { | ||
| missing = append(missing, "one of clientSecret or clientCertificate") | ||
| } | ||
| if meta.ClientCertificatePassword != "" && !usingClientCertificate { | ||
| return nil, fmt.Errorf("clientCertificatePassword requires clientCertificate") | ||
| } | ||
| if len(missing) != 0 { | ||
| return nil, fmt.Errorf("incomplete service principal configuration, missing %s", strings.Join(missing, ", ")) | ||
| } | ||
|
|
||
| var ( | ||
| cred azcore.TokenCredential | ||
| err error | ||
| ) | ||
| if usingClientCertificate { | ||
| cred, err = newAzurePipelinesClientCertificateCredential(meta.TenantID, meta.ClientID, meta.ClientCertificate, meta.ClientCertificatePassword) | ||
| } else { | ||
| cred, err = newAzurePipelinesClientSecretCredential(meta.TenantID, meta.ClientID, meta.ClientSecret) | ||
| } | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| meta.authContext.authType = azurePipelinesAuthTypeServicePrincipal | ||
| return cred, nil |
There was a problem hiding this comment.
Please, move this code inside podIdentity none, as pod identity takes precedence over PAT
|
Checking the error
It looks like the clientID + client Secret isn't working. The error 401 suggest failures getting the token rather than missing permissions for the identity (it's used in other e2e tests as it's the identity set for WIF) |
|
@JorTurFer I dont have a full setup now but I added a some test code to check After this I added the Entra Application to ADO org and then same again |
|
When could this be merged? very interested :) |
|
/run-e2e azure_pipelines |
Signed-off-by: m-amaresh <115941749+m-amaresh@users.noreply.github.com>
|
Semgrep found 18
Consider to use well-defined context |
|
@rickbrouwer , as I have mentioned earlier, Can you please add the SPN to ADO org so that e2e can run successfully. |
Did you have chance to think about this? I really think that as this is on top of Personally, I love this feature as it extends the authentication options for Azure, but I'm not sure about adding it only at scaler level |
Agree! |
5 participants
Add Azure Pipelines scaler support for Azure AD service principal authentication.
This change extends the scaler to support:
tenantId+clientId+clientSecrettenantId+clientId+clientCertificate(+ optionalclientCertificatePassword)Existing authentication behavior remains supported:
personalAccessTokencontinues to use PAT-based Basic authenticationFor service principal and Azure Workload Identity authentication, the scaler acquires an Azure AD token for the Azure DevOps resource and calls the Azure DevOps REST API with bearer authentication.
This PR also:
Related Issue
Fixes #4853
Fixes #7581
Checklist
make generate-scalers-schemahas been run to update any outdated generated fileskeda-docs(if applicable)Relates to kedacore/keda-docs#1726
Notes
clientSecretandclientCertificateare mutually exclusiveclientCertificatePasswordis only valid whenclientCertificateis providedtenantIdandclientIdare required for service principal authentication