feature(azure devops): support multiple organisations

sanderaernouts commented 11 months ago

Hey, I just made a Pull Request!

I Added an AzureDevOpsCredentialProvider, similar to the GitHub and AWS integrations, that can return a token based on the provided URL. This PR resolves #10431 by adding support for organisations to the Azure integration config.

The updated Azure integration configuration looks like this:

integrations:
  azure:
  - host: my.devops.server
    credentials:
    - token: my-pat
  - host: dev.azure.com
    credentials:
    - clientId: <id>
      clientSecret: <secret>
      tenantId: <tenant>
      organisations:
      - my-org
      - my-other-org
      - yet-another-org
    - token: my-org-pat
      organisations:
      - my-other-org

The current token and credential fields in the Azure integration config are deprecated in favour of the credentials field

:heavy_check_mark: Checklist

[x] A changeset describing the change and affected packages. (more info)
[x] Added or updated documentation
[x] Tests for new functionality and regression tests for bug fixes
[x] ~Screenshots attached (for UI changes)~
[x] All your commits have a Signed-off-by line in the message. (more info)

backstage-goalie[bot] commented 11 months ago

Changed Packages

Package Name	Package Path	Changeset Bump	Current Version
@backstage/backend-common	packages/backend-common	patch	`v0.19.2`
@backstage/integration	packages/integration	minor	`v1.6.0`
@backstage/plugin-catalog-backend-module-azure	plugins/catalog-backend-module-azure	patch	`v0.1.19`
@backstage/plugin-scaffolder-backend	plugins/scaffolder-backend	patch	`v1.16.0`

backstage-goalie[bot] commented 11 months ago

Thanks for the contribution! All commits need to be DCO signed before they are reviewed. Please refer to the the DCO section in CONTRIBUTING.md or the DCO status for more info.

sanderaernouts commented 11 months ago

@awanlin, no, I won't be touching that part. I typed the YAML config example from memory and forgot that the Azure integration config is an array of configs. To clarify, I plan to replace the token and credential fields with a single credentials field. This field is an array of token, service principal or managed identity credentials. The credentials will have an optional organisations field where you can specify which Azure DevOps organisation the credential applies.

integrations:
  azure:
    - host: dev.azure.com
      credentials: 
      - organisations:
        - my-org
        token: my-pat 
      - organisations:
        - my-other-org
        token: my-other-pat
    - host: some.devops.server
      credentials: 
      - token: my-pat

I have updated the PR description 👍

awanlin commented 11 months ago

Awesome, thanks for the follow up @sanderaernouts. Another question popup though: why the extra organizations?

Could this:

integrations:
  azure:
    - host: dev.azure.com
      credentials: 
      - organisations:
        - my-org
        token: my-pat 
      - organisations:
        - my-other-org
        token: my-other-pat
    - host: some.devops.server
      credentials: 
      - token: my-pat

Be something like this instead:

integrations:
  azure:
    - host: dev.azure.com
      credentials: 
        - organization: my-org
          token: my-pat 
        - organization: my-other-org
          token: my-other-pat
    - host: some.devops.server
      credentials:
        - organization: my-org
          token: my-pat

With Azure DevOps Server the concept of Team Collections is more or less the same as Organizations in Azure DevOps Services. I think going this way seeing as we will have a breaking change makes this more consistent

sanderaernouts commented 11 months ago

@awanlin I added an array of organizations to support reusing the same service principal or managed identity for multiple organizations. A single service principal or managed identity could be used as long as the same Azure AD tenant backs the Azure DevOps organizations.

However, I'm also okay with organization; you can still duplicate the service principal or managed identity.

awanlin commented 11 months ago

You're last comment helped me better understand what you where trying to do with the config, that works for me then 👍

Maybe we update the description with this then:

integrations:
  azure:
    - host: dev.azure.com
      credentials: 
      - organisations:
        - my-org
        - my-org-related
        - my-org-another-related
        token: my-pat 
      - organisations:
        - my-other-org
        token: my-other-pat
    - host: some.devops.server
      credentials: 
      - token: my-pat

That shows the credential reuse a little better (well it does for me 😉 )

sanderaernouts commented 11 months ago

@awanlin I updated the PR description and will use similar examples when updating the docs. I have also added an example for the Azure DevOps server. Meanwhile, I made good progress on the credential provider yesterday, so there already is some stuff to review if you want to.

github-actions[bot] commented 11 months ago

Uffizzi Preview deployment-33710 was deleted.

sanderaernouts commented 11 months ago

@awanlin, I updated the docs as well. Should we restrict personal access token (PAT) credentials to zero or one organization in the integration config? For Azure DevOps, the PAT is organization-specific, so filling out more than one organization makes no sense.

sanderaernouts commented 11 months ago

@Rugvip I deprecated the fields and functions instead of removing them. I don't think the changes are breaking anymore. I'm unsure about the AzureUrlReader, though. I changed the constructor, but it seems a static factory (create) is used to construct it.

awanlin commented 11 months ago

@awanlin, I updated the docs as well. Should we restrict personal access token (PAT) credentials to zero or one organization in the integration config? For Azure DevOps, the PAT is organization-specific, so filling out more than one organization makes no sense.

Yeah, that makes sense to me 👍

sanderaernouts commented 11 months ago

@awanlin, I updated the docs as well. Should we restrict personal access token (PAT) credentials to zero or one organization in the integration config? For Azure DevOps, the PAT is organization-specific, so filling out more than one organization makes no sense.

Yeah, that makes sense to me 👍

Done ✅

sanderaernouts commented 11 months ago

I ran some tests locally with a client secret credential for one AAD-backed Azure DevOps organization and a PAT for my personal Azure DevOps organization. I was able to read catalog-info.yaml files from both organizations 🥳.

sanderaernouts commented 11 months ago

Updated the PR description since it is no longer a work in progress

sanderaernouts commented 11 months ago

@awanlin @Rugvip can you have another look? 👀

awanlin commented 11 months ago

FYI - I have it on my schedule on Friday to test this with out Azure DevOps Server test environment with multiple Collections and also using a both Cloud and Server. Will follow up with my results 🚀

sanderaernouts commented 11 months ago

👍 I'm on the Backstage discord server, so if you run into anything while testing, you can ping me there as well. My Discord nickname is the same as my GitHub handle

awanlin commented 11 months ago

OK, did some testing with the environments I had:

Multiple Team Collections on Azure DevOps Server 2020 worked
Multiple Organizations on Azure DevOps Services work
A combination of Team Collections on Azure DevOps Server 2020 and Organizations on Azure DevOps Services works

Awesome work @sanderaernouts, once this gets merged in I'll work on updating the Azure DevOps plugin to support this 🚀

sanderaernouts commented 11 months ago

@Rugvip can you have another look? Had some questions based on review

awanlin commented 11 months ago

@sanderaernouts just a heads up that @Rugvip is on vacation for a few weeks now, sorry bad timing I know

Oh, but in the mean time if you rebase your E2E tests will start passing 👍

sanderaernouts commented 11 months ago

@awanlin done ✅ . Is there anyone else who can have a look at this PR instead of @Rugvip? I was hoping to get this out with 1.16.0 🚀.

From his review, there is one unresolved comment, https://github.com/backstage/backstage/pull/18213#discussion_r1230223085, but I need some guidance on how to resolve that. So if anyone can help me resolve that, that would be great.

sanderaernouts commented 11 months ago

It turns out I did not read @Rugvip's comment carefully enough. I refactored the AzureUrlReader class and have resolved the comment 👍 .

awanlin commented 11 months ago

It turns out I did not read @Rugvip's comment carefully enough. I refactored the AzureUrlReader class and have resolved the comment 👍 .

The soonest I'll have time to retest this is On Friday July 7th

github-actions[bot] commented 10 months ago

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

sanderaernouts commented 10 months ago

@Rugvip, thanks for the review; I got through most of your comments and will do the rest tomorrow. Feel free to have another look :).

sanderaernouts commented 10 months ago

@awanlin, do you want to retest this based on the changes I made following the review from @Rugvip? The only significant difference is how the credentials are mapped from the config. I retested this with two Azure DevOps organisations ✅

awanlin commented 10 months ago

@sanderaernouts sure, that would be wise to do. I can only do this on Friday. I'm fine with this getting merged before then but only after the release comes out on Tuesday. 👍

sanderaernouts commented 10 months ago

@awanlin since it's not going to be in the release today we might as well be on the safe side and retest it 👍. FYI, I have an open review comment in #18507 that might change how the Azure credentials are read from the config.

awanlin commented 10 months ago

Retested just now and it's all still working as expected. The test cases I used were:

Multiple Team Collections on Azure DevOps Server 2020 worked
Multiple Organizations on Azure DevOps Services work
A combination of Team Collections on Azure DevOps Server 2020 and Organizations on Azure DevOps Services works

sanderaernouts commented 10 months ago

Thanks @awanlin! @Rugvip can you have another look?

sanderaernouts commented 10 months ago

@Rugvip, can you have another look? I need some input from you based on your previous review. @awanlin already retested my changes, and everything looks good, so I'd want to get this PR merged.

sanderaernouts commented 10 months ago

Tagging the other maintainers: @freben, @benjdlambert, @jhaals. This PR has been open for almost 2 months now, I know it's holiday season, but I'd love to get into 1.1.17.

MarcBruins commented 9 months ago

Any progress on this? We would very much like to have this supported.

sanderaernouts commented 9 months ago

@afscrome thanks for your review, I have processed your review comments 👍

sanderaernouts commented 9 months ago

Refactored the DefaultAzureDevOpsCredentialsProvider a bit more so that it always uses a DefaultAzureCredential as the fallback for dev.azure.com unless a credential without organizations is specified. The DefaultAzureDevOpsCredentialsProvider always tries to find a credential for the specific organization before using a non organization specific credential. So a DefaultAzureCredential is used unless:

you specify a credential for the specific organization
you specify a credential without organizations

sanderaernouts commented 9 months ago

@awanlin tested this locally against two dev.azure.com organizations. Do you want to retest it for Azure DevOps server?

awanlin commented 9 months ago

Ok, retested but I think we need to update the PR description, changeset, and the docs based on the latest changes. I needed to change token to personalAccessToken in my config, I'm fine with that but had to dig to figure that out. Also I needed to change the config structure as well and include new things I didn't need to add before. I was nice that you didn't need to list the org before.

I started with:

azure:
    - host: azure-devop-server.company.com
      token: ${TOKEN}
    - host: dev.azure.com
      credentials:
      - token: ${ANOTHER_TOKEN}
        organisations:
        - some-org

and ended with:

azure:
    - host: azure-devop-server.company.com
      credentials:
      - personalAccessToken: ${TOKEN}
        organisations:
        - server-collection
    - host: dev.azure.com
      credentials:
      - personalAccessToken: ${ANOTHER_TOKEN}
        organisations:
        - some-org

With the config above with changes to test the various scenarios it all worked

sanderaernouts commented 9 months ago

Nice, thank you! 🎉

Just two nits. It would be nice to have some clarity on #18213 (comment) but I don't feel it's a requirement

There are a lot of PRs we want to get into 1.17 😅

@Rugvip, thanks; I know you guys are busy. I'd prefer to have #18213 (comment) fixed resolved before merging.

I can imagine many contributors are pushing for 1.17, but my PR is the most important one to get merged 😜.

awanlin commented 9 months ago

I'd just like to note my worries about this getting into the release today. I strongly feel that this needs to be included in at least one of the weekly releases.

Rugvip commented 9 months ago

@awanlin will do!

awanlin commented 9 months ago

@Rugvip, this is ready to be merged. Also, it would be good to include this in the release notes for 1.18.0, how do we make sure that happens?

Rugvip commented 9 months ago

@awanlin nice! 🎉

It'll be in the release notes, I've added a section to our internal draft of the notes

github-actions[bot] commented 9 months ago

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.18.0 release, scheduled for Tue, 19 Sep 2023.

awanlin commented 9 months ago

Thanks @Rugvip, and many, many, many thanks to @sanderaernouts for the work done in this PR! 🚀

sanderaernouts commented 9 months ago

Thanks, @awanlin and @Rugvip, for your time and feedback 🚀 🎉

backstage / backstage