Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: static validation error and BCP error in avm.res.dev-ops-infrastructure.pool #3333

Merged
merged 4 commits into from
Sep 19, 2024

Conversation

johnlokerse
Copy link
Contributor

Description

This pull request fixes the following:

  • The static validation is now passing, fixed by rerunning the Set-AVMModule command
  • BCP error regarding a property/parameter that cannot be null - fixed this by making it required

Tagging @matebarabas @AlexanderSehr

Pipeline Reference

Pipeline
avm.res.dev-ops-infrastructure.pool

Type of Change

  • Update to CI Environment or utilities (Non-module affecting changes)
  • Azure Verified Module updates:
    • Bugfix containing backwards-compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in version.json:
      • Someone has opened a bug report issue, and I have included "Closes #{bug_report_issue_number}" in the PR description.
      • The bug was found by the module author, and no one has opened an issue to report it yet.
    • Feature update backwards compatible feature updates, and I have bumped the MINOR version in version.json.
    • Breaking changes and I have bumped the MAJOR version in version.json.
    • Update to documentation

Checklist

  • I'm sure there are no other open Pull Requests for the same update/change
  • I have run Set-AVMModule locally to generate the supporting module files.
  • My corresponding pipelines / checks run clean and green without any errors or warnings

@johnlokerse johnlokerse requested review from a team as code owners September 19, 2024 06:01
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Triage 🔍 Maintainers need to triage still label Sep 19, 2024
@avm-team-linter avm-team-linter bot added the Needs: Module Owner 📣 This module needs an owner to develop or maintain it label Sep 19, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue label Sep 19, 2024
@eriqua eriqua added Class: Resource Module 📦 This is a resource module and removed Needs: Triage 🔍 Maintainers need to triage still labels Sep 19, 2024
@eriqua eriqua enabled auto-merge (squash) September 19, 2024 23:25
Copy link
Contributor

@eriqua eriqua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🆗️

@eriqua eriqua merged commit f1d14ed into Azure:main Sep 19, 2024
4 checks passed
@matebarabas
Copy link
Contributor

@eriqua, @johnlokerse, the test deployments seem to have failed (the module is not published): https://github.com/Azure/bicep-registry-modules/actions/runs/10950416993

@johnlokerse
Copy link
Contributor Author

johnlokerse commented Sep 20, 2024

@matebarabas @eriqua This is error comes up due to incorrect quotas in the region that the MDP resource is deployed to. I know Alexander has tested a deployment on his side. I believe he tested it in northeurope, due to capacity limits in westeurope.

See prereqs docs: https://learn.microsoft.com/en-us/azure/devops/managed-devops-pools/prerequisites?view=azure-devops&tabs=azure-portal#create-a-quota-support-request

What is the best way to approach this? Hardcode the deployment location if we are sure which location @AlexanderSehr has deployed to?

@eriqua
Copy link
Contributor

eriqua commented Sep 20, 2024

On it @johnlokerse

@eriqua
Copy link
Contributor

eriqua commented Sep 21, 2024

Hey @johnlokerse, I'm retrieving a list of allowed locations for our target environment.
For now, I tried to enforce one of those (uksouth) through the pipeline, using the customLocation feature.
While the defaults test is successful, max and waf-aligned tests are failing due to UnauthorizedAccessToVirtualNetwork. It seems that the DevOpsInfrastructure service principal does not have Read access to the virtual network, and would require Reader and Network Contributor access.
Does that sound familiar?
Did you have to set that up in your testing environment?

Let me know if any additional prerequisite, so that we can test a successful deployment of all e2e tests before enforcing specific locations in code as a next step.


Update: I've manually assigned required permissions to the DevOpsInfrastructure service principal, but would suggest to instead use the CI secret feature, similarly to what this module already does for devops parameters.

Currently both max and waf-aligned tests are failing due to missing devops permissions for the deployment spn.

Let's please connect offline to identify the required prerequisites so that the module can pass required validation and get published.

@johnlokerse
Copy link
Contributor Author

Hey @eriqua, the permissions on the virtual network are set through the dependencies.bicep as seen below:

// Network Contributor role assignment
resource roleAssignments 'Microsoft.Authorization/roleAssignments@2020-04-01-preview' = {
  name: guid(subscription().subscriptionId, 'DevOpsInfrastructure', 'Network Contributor', 'max')
  properties: {
    principalId: devOpsInfrastructureObjectID // DevOpsInfrastructure service principal
    #disable-next-line use-resource-id-functions
    roleDefinitionId: '/providers/Microsoft.Authorization/roleDefinitions/4d97b98b-1d4f-4787-a291-c67834d212e7'
    principalType: 'ServicePrincipal'
  }
  scope: virtualNetwork
}

The deployment uses the new CI secret feature. My validation deployments are done through that way. As far as I can see it is also the case in the AVM test deployment (https://github.com/Azure/bicep-registry-modules/actions/runs/10963081104/job/30458820341#step:4:542):

CleanShot 2024-09-23 at 10 26 31@2x

@eriqua What are the deployment permission differences between Default and Max or WAF? The AVM validation deployment has a successful Default deployment, which also deploys an agent pool in Azure DevOps.

Below the output of a Defaults deployment (agent pool in AzDo):
CleanShot 2024-09-23 at 10 49 00@2x

@eriqua
Copy link
Contributor

eriqua commented Sep 23, 2024

Hi @johnlokerse thanks for the above details and sorry if I'm not aligned if discussions already happened prior to raising this PR.

As you know, as per the new CI secrets feature, we should also set up the brm environment in the same way you did for yours. That includes setting up secrets in the BRM keyvault. I see we have secrets created in our keyvault too, but we are missing values.
I'll set up the DevOpsInfrastructure one, but which values should we set up for the other 2 secrets ado organization and project?
Also, do you know if ado permissions have been set up already for our deployment spn?

I'd appreciate if we could connect offline so that I can support setting the above in order to publish this module.

@johnlokerse
Copy link
Contributor Author

@eriqua Reached out to you via Teams 😄

@eriqua
Copy link
Contributor

eriqua commented Sep 26, 2024

🚀 The module is published 🚀

Just providing updates here for future reference:

  • InsufficientCoreQuota error: Solved via the enforcedLocation (uksouth) workaround in code. Ref PR fix: Added a fixed uksouth location for avm.res.dev-ops-infrastructure.pool #3351
  • UnauthorizedAccessToVirtualNetwork error: Custom CI secrets have been updated in upstream key vault. In particular, the CI-DevOpsInfrastructureObjectID value.
  • PoolProvisioningFailed (SPN missing create permissions) error: In ADO, the SPN has been added to the Project collection administrator group. Setting the SPN Administrator on agent pools at Organizational settings level (least privilege) as per documentation was not enough for max and waf-aligned tests.

@AlexanderSehr
Copy link
Contributor

  • create permissions) error: In ADO, the SPN has been added to the Project collection administrator group. Setting the SPN Administrator on agent pools at Organizational settings level (least privilege) as per documentation was not enough for max and waf-aligned tests.

Hey @eriqua & @johnlokerse, was #3 added to the test/documentation by any chance?

@eriqua
Copy link
Contributor

eriqua commented Oct 1, 2024

  • create permissions) error: In ADO, the SPN has been added to the Project collection administrator group. Setting the SPN Administrator on agent pools at Organizational settings level (least privilege) as per documentation was not enough for max and waf-aligned tests.

Hey @eriqua & @johnlokerse, was #3 added to the test/documentation by any chance?

Hey @AlexanderSehr I believe there are conversations ongoing with the PG to investigate why documented permissions were not enough. @johnlokerse please chime in if any news.
Depending on the outcome of that discussion, I agree it would be beneficial to document it

@johnlokerse
Copy link
Contributor Author

@AlexanderSehr @eriqua I have not heard anything back yet. I will post something here when I hear something (or just add it to the docs ;-))

@AlexanderSehr
Copy link
Contributor

I see. Thanks for elaborating 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Class: Resource Module 📦 This is a resource module Needs: Module Owner 📣 This module needs an owner to develop or maintain it Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants