Skip to content

Update artifacts and enhance task script for resource creation#33

Open
ilya776 wants to merge 4 commits into
mate-academy:mainfrom
ilya776:main
Open

Update artifacts and enhance task script for resource creation#33
ilya776 wants to merge 4 commits into
mate-academy:mainfrom
ilya776:main

Conversation

@ilya776

@ilya776 ilya776 commented Sep 17, 2025

Copy link
Copy Markdown

No description provided.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

Something went wrong while processing the review.
[CODE: GAE]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

I’m requesting changes because there are multiple critical blockers that will fail validation and/or break deployment.

Critical issues to fix:

  • Resource group mismatch (task.ps1, near the top where $resourceGroupName is set, e.g., line ~5): The workflow expects mate-resources. Using another RG will cause artifact generation/validation to miss your resources. Update to mate-resources and redeploy.
  • Missing explicit intra-VNet allow rules (task.ps1, NSG rule sections for web, management, and database): The checklist requires allowing traffic from within the VNet. Add an inbound rule like -SourceAddressPrefix VirtualNetwork -DestinationAddressPrefix VirtualNetwork -Access Allow -Direction Inbound -Protocol * -SourcePortRange * -DestinationPortRange * (e.g., name AllowVNet, priority 200) to each NSG.
  • Invalid web rule port specification (task.ps1, web NSG rule, around the New-AzNetworkSecurityRuleConfig for web): -DestinationPortRange 80,443 is invalid. Use either two rules (80 and 443) or -DestinationPortRanges @('80','443'). Ensure each rule has a unique name and distinct priority.
  • NSG creation must pass rules as an array (task.ps1, New-AzNetworkSecurityGroup calls): After adding AllowVNet (and separate HTTP/HTTPS if you split), pass them as -SecurityRules @($allowVNetRule, $allowHttpRule, $allowHttpsRule) (and similarly for management; database should include only AllowVNet). This guarantees all required rules are present before subnet association.
  • Artifacts workflow (artifacts.json): Ensure the resourcesTemplate URL was produced by running scripts/generate-artifacts.ps1 after a correct deployment to mate-resources. Then run scripts/validate-artifacts.ps1 and commit the updated artifacts.json (and result.json if required).

Minor improvements:

  • Verify NSG rule priorities don’t conflict; keep Internet allows at lower numbers (e.g., 100/110) and AllowVNet at a higher number (e.g., 200). Name rules clearly (e.g., AllowHTTPFromInternet, AllowHTTPSFromInternet, AllowSSHFromInternet, AllowVNet).

You’re close: the NSGs and associations are in place and the intent for web and management is correct. Addressing the items above should make the deployment pass validation. Keep going—you’ve got this!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread artifacts.json Outdated
@@ -1,3 +1,3 @@
{
"resourcesTemplate": ""
"resourcesTemplate": "https://mateartefacts.blob.core.windows.net/task-artifacts/task16/exported-template.json?sv=2025-05-05&se=2025-10-17T14%3A32%3A36Z&sr=b&sp=r&sig=T6JtYMBnGFzWKKMCNroKaKa8pz83noNwyi1Bg3%2Fi2YE%3D"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This line holds the "resourcesTemplate" artifact URL. Ensure this entry was created by running scripts/generate-artifacts.ps1 and that the file was committed after generation. If this URL is a placeholder or was not produced by your run, re-run the generation script so the artifacts.json contains the actual artifact link(s) for validation. Also verify the URL points to the correct storage container (task-artifacts in the mate-resources setup) as required by the task workflow .

Comment thread task.ps1 Outdated
# --------------------------
Write-Host "Creating a resource group $resourceGroupName ..."
New-AzResourceGroup -Name $resourceGroupName -Location $location
New-AzResourceGroup -Name $resourceGroupName -Location $location -Force

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resource group mismatch — action required.

The script creates the resource group using $resourceGroupName (see the New-AzResourceGroup call). The task artifact/validation workflow and the task description expect resources to be created in the mate-resources resource group. Please update the configuration so the deployment targets mate-resources (or ensure that the artifact/validation workflow is pointed at the RG you actually use). This is required for the artifact generation/validation steps to work correctly .

Comment thread task.ps1 Outdated
# --------------------------
Write-Host "Creating webservers NSG..."
$webRule = New-AzNetworkSecurityRuleConfig -Name "AllowWeb" -Priority 100 -Direction Inbound -Access Allow -Protocol Tcp `
-SourceAddressPrefix Internet -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 80,443

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Invalid port specification for web rule.

The web rule uses -DestinationPortRange 80,443 which is not a valid single value. Use either two separate rules (one for port 80, one for 443) or specify multiple ports with -DestinationPortRanges @('80','443'). Also give each rule a distinct name and priority so they evaluate correctly.

Comment thread task.ps1 Outdated
# NSG for Webservers
# --------------------------
Write-Host "Creating webservers NSG..."
$webRule = New-AzNetworkSecurityRuleConfig -Name "AllowWeb" -Priority 100 -Direction Inbound -Access Allow -Protocol Tcp `

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Add explicit intra-VNet allow rule for the web NSG.

You currently create a web rule allowing Internet ports but there is no explicit VirtualNetwork allow rule. The task requires that subnets allow traffic from other subnets within the same VNet. Create a rule such as New-AzNetworkSecurityRuleConfig -Name 'AllowVNet' -Priority 200 -Direction Inbound -Access Allow -Protocol * -SourceAddressPrefix VirtualNetwork -DestinationAddressPrefix VirtualNetwork -SourcePortRange * -DestinationPortRange * and include it in the NSG's -SecurityRules array when creating the NSG.

Comment thread task.ps1 Outdated
$webRule = New-AzNetworkSecurityRuleConfig -Name "AllowWeb" -Priority 100 -Direction Inbound -Access Allow -Protocol Tcp `
-SourceAddressPrefix Internet -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 80,443

$webNSG = New-AzNetworkSecurityGroup -ResourceGroupName $resourceGroupName -Location $location -Name $webSubnetName -SecurityRules $webRule

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When creating NSG, pass rules as an array and include VNet rule.

You call New-AzNetworkSecurityGroup -SecurityRules $webRule. After you add the AllowVNet rule (and possibly separate HTTP/HTTPS rules), pass them as an array: -SecurityRules @($allowVNetRule, $allowHttpRule, $allowHttpsRule). This ensures the NSG contains all required rules before attaching it to the subnet.

Comment thread task.ps1 Outdated
$mgmtRule = New-AzNetworkSecurityRuleConfig -Name "AllowSSH" -Priority 100 -Direction Inbound -Access Allow -Protocol Tcp `
-SourceAddressPrefix Internet -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 22

$mgmtNSG = New-AzNetworkSecurityGroup -ResourceGroupName $resourceGroupName -Location $location -Name $mngSubnetName -SecurityRules $mgmtRule

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Management NSG: add intra-VNet allow rule and verify priorities.

The management NSG allows SSH from Internet which matches the requirement, but it also needs an explicit VirtualNetwork allow rule so other subnets can reach management hosts. Also ensure the priority numbers do not accidentally block the VNet rule; pick priorities so the intended allow rules are evaluated properly.

Comment thread task.ps1 Outdated
# --------------------------
Write-Host "Creating database NSG..."
# Порожній NSG (немає правил)
$dbNSG = New-AzNetworkSecurityGroup -ResourceGroupName $resourceGroupName -Location $location -Name $dbSubnetName

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Database NSG must remain closed to Internet but allow intra-VNet traffic.

Creating an empty NSG (no inbound allow rules) ensures no Internet inbound access — which matches the requirement — but you still must add an explicit VirtualNetwork allow rule to permit inter-subnet communication per the checklist. Do not add any rule that allows Internet as a source for this NSG.

Comment thread task.ps1 Outdated
Comment on lines +51 to +53
$webSubnet = New-AzVirtualNetworkSubnetConfig -Name $webSubnetName -AddressPrefix $webSubnetIpRange -NetworkSecurityGroup $webNSG
$dbSubnet = New-AzVirtualNetworkSubnetConfig -Name $dbSubnetName -AddressPrefix $dbSubnetIpRange -NetworkSecurityGroup $dbNSG
$mngSubnet = New-AzVirtualNetworkSubnetConfig -Name $mngSubnetName -AddressPrefix $mngSubnetIpRange -NetworkSecurityGroup $mgmtNSG

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Subnet NSG association — ensure NSG objects already include the VNet rule.

You attach NSG objects to subnets via New-AzVirtualNetworkSubnetConfig -NetworkSecurityGroup $webNSG etc. Make sure the NSG objects ($webNSG, $dbNSG, $mgmtNSG) include the AllowVNet rule (and other required rules) before creating the virtual network, otherwise update the subnet NSG association after creating the NSG with Set-AzVirtualNetwork or use Set-AzNetworkSecurityGroup to update rules and then re-apply associations.

Comment thread task.ps1 Outdated
$dbSubnet = New-AzVirtualNetworkSubnetConfig -Name $dbSubnetName -AddressPrefix $dbSubnetIpRange
$mngSubnet = New-AzVirtualNetworkSubnetConfig -Name $mngSubnetName -AddressPrefix $mngSubnetIpRange
New-AzVirtualNetwork -Name $virtualNetworkName -ResourceGroupName $resourceGroupName -Location $location -AddressPrefix $vnetAddressPrefix -Subnet $webSubnet,$dbSubnet,$mngSubnet
Write-Host "All resources created successfully ✅"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Post-deployment workflow reminder.

After you update the script and successfully deploy, run scripts/generate-artifacts.ps1 and then scripts/validate-artifacts.ps1. Commit the updated artifacts.json and result.json to the repo as required by the task workflow. The task description explains this sequence and that validation must pass before submission .

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

I’m requesting changes because there are critical blockers that can prevent successful deployment and validation.

  • Critical: Idempotency is not implemented in task.ps1. The script calls New-AzNetworkSecurityGroup and New-AzVirtualNetwork unconditionally. On re-run or if resources already exist, this will fail and breaks the “safe re-run” requirement. Fix by checking for existing resources and updating them:
    • NSGs: use Get-AzNetworkSecurityGroup -Name <name> -ResourceGroupName mate-resources -ErrorAction SilentlyContinue. If found, update .SecurityRules and call Set-AzNetworkSecurityGroup; else create with New-AzNetworkSecurityGroup.
    • VNet/Subnets: use Get-AzVirtualNetwork -Name $vnetName -ResourceGroupName mate-resources -ErrorAction SilentlyContinue. If it exists, set each subnet’s .NetworkSecurityGroup = <corresponding NSG> and call Set-AzVirtualNetwork; otherwise create with New-AzVirtualNetwork.
  • Critical: Artifacts/validation workflow is incomplete. artifacts.json is present, but result.json is missing and the SAS URL may be stale. This will cause scripts/validate-artifacts.ps1 to fail. After fixing idempotency and deploying, run:
    1. pwsh ./scripts/generate-artifacts.ps1 (ensures artifacts.json points to the exported-template blob with a valid SAS)
    2. pwsh ./scripts/validate-artifacts.ps1 (ensure it passes)
    3. Commit the updated artifacts.json and result.json.

What you did well: NSG rules and priorities are correct for web (HTTP/HTTPS), management (SSH), and database (VNet only), and the RG is correctly set to mate-resources. Optional improvements: add brief comments mapping rules to requirements and consider explicit deny-from-Internet for DB only if your validator requires it. Once idempotency and artifact validation are addressed, this should be ready to approve.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread artifacts.json
@@ -1,3 +1,3 @@
{
"resourcesTemplate": ""
"resourcesTemplate": "https://mateartefacts.blob.core.windows.net/task-artifacts/task16/exported-template.json?sv=2025-05-05&se=2025-10-17T20%3A18%3A38Z&sr=b&sp=r&sig=7TVH%2FnOasqAmakmKzXWJ4KXKHWJRyqkd5S75pQCgTMg%3D"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The URL is present and in the expected resourcesTemplate property. Verify that this exact URL was produced by scripts/generate-artifacts.ps1 and that the SAS token is valid (not expired) and points to the exported-template blob. If the SAS is expired or the blob is missing, scripts/validate-artifacts.ps1 will fail: run pwsh ./scripts/generate-artifacts.ps1 then pwsh ./scripts/validate-artifacts.ps1 and commit the resulting artifacts.json and result.json after validation passes.

Comment thread task.ps1 Outdated
# --------------------------
$location = "uksouth"
$resourceGroupName = "mate-azure-task-16"
$resourceGroupName = "mate-resources" # ✅ Fix: must be mate-resources

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resource group is correctly set to the required mate-resources which fixes the earlier RG mismatch and matches the task prerequisites. Good — this ensures artifacts/validation will look in the expected RG (description requires mate-resources) .

Comment thread task.ps1 Outdated
Comment on lines +30 to +39
$webRuleHttp = New-AzNetworkSecurityRuleConfig -Name "AllowHTTPFromInternet" -Priority 100 -Direction Inbound -Access Allow -Protocol Tcp `
-SourceAddressPrefix Internet -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 80

$webRuleHttps = New-AzNetworkSecurityRuleConfig -Name "AllowHTTPSFromInternet" -Priority 110 -Direction Inbound -Access Allow -Protocol Tcp `
-SourceAddressPrefix Internet -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 443

$webRuleVnet = New-AzNetworkSecurityRuleConfig -Name "AllowVNet" -Priority 200 -Direction Inbound -Access Allow -Protocol * `
-SourceAddressPrefix VirtualNetwork -SourcePortRange * -DestinationAddressPrefix VirtualNetwork -DestinationPortRange *

$webNSG = New-AzNetworkSecurityGroup -ResourceGroupName $resourceGroupName -Location $location -Name $webSubnetName -SecurityRules @($webRuleHttp,$webRuleHttps,$webRuleVnet)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Web NSG rules are defined correctly: separate rules for HTTP (port 80) and HTTPS (port 443) from the Internet, plus an AllowVNet rule for intra-VNet traffic. Priorities (100, 110, 200) are unique within the NSG and will not shadow each other. This satisfies the webservers requirements (allow HTTP/HTTPS from Internet and intra-VNet) .

Comment thread task.ps1 Outdated
Comment on lines +46 to +52
$mgmtRuleSsh = New-AzNetworkSecurityRuleConfig -Name "AllowSSHFromInternet" -Priority 120 -Direction Inbound -Access Allow -Protocol Tcp `
-SourceAddressPrefix Internet -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 22

$mgmtRuleVnet = New-AzNetworkSecurityRuleConfig -Name "AllowVNet" -Priority 200 -Direction Inbound -Access Allow -Protocol * `
-SourceAddressPrefix VirtualNetwork -SourcePortRange * -DestinationAddressPrefix VirtualNetwork -DestinationPortRange *

$mgmtNSG = New-AzNetworkSecurityGroup -ResourceGroupName $resourceGroupName -Location $location -Name $mngSubnetName -SecurityRules @($mgmtRuleSsh,$mgmtRuleVnet)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Management NSG rules are correct: AllowSSHFromInternet (TCP 22) and AllowVNet for intra-VNet. Priorities are unique in this NSG (120 and 200). This satisfies the management subnet requirement (allow SSH from Internet and intra-VNet traffic) .

Comment thread task.ps1 Outdated
Comment on lines +59 to +62
$dbRuleVnet = New-AzNetworkSecurityRuleConfig -Name "AllowVNet" -Priority 200 -Direction Inbound -Access Allow -Protocol * `
-SourceAddressPrefix VirtualNetwork -SourcePortRange * -DestinationAddressPrefix VirtualNetwork -DestinationPortRange *

Write-Host "Creating web network security group..."
# Write your code for creation of Web NSG here ->
$dbNSG = New-AzNetworkSecurityGroup -ResourceGroupName $resourceGroupName -Location $location -Name $dbSubnetName -SecurityRules @($dbRuleVnet)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Database NSG contains only an AllowVNet rule and no Internet allow. That preserves default deny for Internet and satisfies the requirement that the database subnet should not accept traffic from the Internet. If your validator expects an explicit deny-from-Internet rule you can add one, but it is typically unnecessary because default inbound deny already applies in Azure NSGs .

Comment thread task.ps1 Outdated
$webRuleVnet = New-AzNetworkSecurityRuleConfig -Name "AllowVNet" -Priority 200 -Direction Inbound -Access Allow -Protocol * `
-SourceAddressPrefix VirtualNetwork -SourcePortRange * -DestinationAddressPrefix VirtualNetwork -DestinationPortRange *

$webNSG = New-AzNetworkSecurityGroup -ResourceGroupName $resourceGroupName -Location $location -Name $webSubnetName -SecurityRules @($webRuleHttp,$webRuleHttps,$webRuleVnet)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Critical: NSG creation uses New-AzNetworkSecurityGroup unconditionally. Add idempotency: check for existing NSG with Get-AzNetworkSecurityGroup and either create (if not exist) or update its SecurityRules (Set-AzNetworkSecurityGroup) to avoid failures on re-run. Apply this to the web, management and database NSG creation blocks (lines where New-AzNetworkSecurityGroup is called) .

Comment thread task.ps1 Outdated
Comment on lines +72 to +76
New-AzVirtualNetwork -Name $virtualNetworkName `
-ResourceGroupName $resourceGroupName `
-Location $location `
-AddressPrefix $vnetAddressPrefix `
-Subnet $webSubnet,$dbSubnet,$mngSubnet

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Critical: VNet creation block uses New-AzVirtualNetwork without checking whether the VNet already exists. If the VNet exists, use Get-AzVirtualNetwork and update its Subnets to attach the NSGs (modify subnet.NetworkSecurityGroup and call Set-AzVirtualNetwork). Make the script handle both new creation and update scenarios so it works reliably in the typical exercise flow where VNet may already exist .

Comment thread task.ps1 Outdated
@@ -1,30 +1,78 @@
# --------------------------

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Artifacts/validation step: ensure you ran scripts/generate-artifacts.ps1 and scripts/validate-artifacts.ps1 after deploying. I see artifacts.json in the repo, but you must commit result.json and the updated artifacts after successful validation. If validation fails, follow validator output, fix deployment, regenerate artifacts and re-run validation until it passes .

Comment thread task.ps1 Outdated
Comment on lines +28 to +37
Write-Host "Creating webservers NSG..."

$webRuleHttp = New-AzNetworkSecurityRuleConfig -Name "AllowHTTPFromInternet" -Priority 100 -Direction Inbound -Access Allow -Protocol Tcp `
-SourceAddressPrefix Internet -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 80

$webRuleHttps = New-AzNetworkSecurityRuleConfig -Name "AllowHTTPSFromInternet" -Priority 110 -Direction Inbound -Access Allow -Protocol Tcp `
-SourceAddressPrefix Internet -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 443

$webRuleVnet = New-AzNetworkSecurityRuleConfig -Name "AllowVNet" -Priority 200 -Direction Inbound -Access Allow -Protocol * `
-SourceAddressPrefix VirtualNetwork -SourcePortRange * -DestinationAddressPrefix VirtualNetwork -DestinationPortRange *

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Optional: Consider adding a brief comment above each rule or block that states which checklist requirement it implements (e.g., # Allow intra-VNet traffic (requirement 1.3)). This will make future reviews faster and clearer.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

Educational approval: you’ve implemented the core NSG logic well, and I’m approving to help you keep momentum while you address a few remaining items.

What you did well

  • AllowVNet rule uses the VirtualNetwork service tag and is applied to each NSG — solid alignment with the intra‑VNet traffic requirement.
  • Web NSG correctly allows only HTTP/HTTPS from Internet; Management NSG allows only SSH (22) from Internet; Database NSG has no Internet allows — meets the rule semantics and priorities.
  • NSG creation/update pattern is idempotent and clean.

What needs improvement (actionable)

  • artifacts.json: Regenerate and validate artifacts, then commit both files. After deploying, run: 1) pwsh ./scripts/generate-artifacts.ps1 (commit the refreshed artifacts.json with a fresh SAS), 2) pwsh ./scripts/validate-artifacts.ps1 (commit the generated result.json). This is required by the task and currently blocking final validation.
  • task.ps1: When the VNet already exists, add a check to ensure the three subnets exist (webservers, database, management). If any are missing, create them or fail with a clear message so NSG association doesn’t silently skip them. Consider a null-check for $vnet before associating NSGs and log missing subnets for visibility.

Minor polish

  • If you later modify rules dynamically, avoid reusing the same rule objects across NSGs. Optionally add an explicit DB deny-from-Internet rule for clarity (default deny already covers it).

Great progress — once you add the subnet checks and commit the generated result.json (with refreshed artifacts.json), your submission should fully satisfy the checklist.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread artifacts.json
@@ -1,3 +1,3 @@
{
"resourcesTemplate": ""
"resourcesTemplate": "https://mateartefacts.blob.core.windows.net/task-artifacts/task16/exported-template.json?sv=2025-05-05&se=2025-10-17T20%3A18%3A38Z&sr=b&sp=r&sig=7TVH%2FnOasqAmakmKzXWJ4KXKHWJRyqkd5S75pQCgTMg%3D"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This resourcesTemplate value is the SAS URL that the validator will use. Make sure to (1) run pwsh ./scripts/generate-artifacts.ps1 to refresh this link (if needed) and commit the updated artifacts.json, then (2) run pwsh ./scripts/validate-artifacts.ps1 and commit the generated result.json. The task description requires both artifacts.json and result.json to be produced and committed as part of submission .

Comment thread task.ps1
Comment on lines +26 to +27
} else {
Write-Host "VNet already exists, using existing one."

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When the VNet exists the script only prints "VNet already exists, using existing one." but does not ensure the three expected subnets exist. If the existing VNet is missing any of the webservers, database, or management subnets the later NSG association will silently skip them. Add logic here to check for and create any missing subnets (or fail with a clear message) so the script is robust and idempotent across existing VNets.

Comment thread task.ps1
# ===== RULES =====
# Shared intra-VNet allow
$allowVNetRule = New-AzNetworkSecurityRuleConfig -Name "AllowVNet" -Priority 200 -Direction Inbound -Access Allow -Protocol * -SourceAddressPrefix VirtualNetwork -DestinationAddressPrefix VirtualNetwork -SourcePortRange * -DestinationPortRange *

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good: the AllowVNet rule is created using the VirtualNetwork service tag, which meets the requirement to allow intra-VNet traffic. This matches the task requirement to permit subnet-to-subnet traffic within the VNet.

Comment thread task.ps1
Comment on lines +35 to 36
$allowHttpsRule = New-AzNetworkSecurityRuleConfig -Name "AllowHTTPSFromInternet" -Priority 110 -Direction Inbound -Access Allow -Protocol Tcp -SourceAddressPrefix Internet -DestinationAddressPrefix * -SourcePortRange * -DestinationPortRange 443

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

HTTP and HTTPS rules use SourceAddressPrefix 'Internet' and correct ports (80, 443). This satisfies the requirement that webservers accept only HTTP/HTTPS from the Internet. Ensure no other Internet allow rules are added to this NSG elsewhere in your repo.

Comment thread task.ps1
# Write your code for creation of management NSG here ->
# Management (SSH)
$allowSshRule = New-AzNetworkSecurityRuleConfig -Name "AllowSSHFromInternet" -Priority 100 -Direction Inbound -Access Allow -Protocol Tcp -SourceAddressPrefix Internet -DestinationAddressPrefix * -SourcePortRange * -DestinationPortRange 22

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

SSH rule for management uses SourceAddressPrefix 'Internet' and DestinationPortRange 22 — this satisfies the requirement for management subnet. Confirm there are no additional Internet allow rules in this NSG (the script currently only adds the VNet allow and the SSH allow).

Comment thread task.ps1
Comment on lines +42 to +52
if (-not $webNsg) {
Write-Host "Creating Webservers NSG..."
$webNsg = New-AzNetworkSecurityGroup -Name $webNsgName -ResourceGroupName $resourceGroupName -Location $location -SecurityRules @($allowHttpRule, $allowHttpsRule, $allowVNetRule)
} else {
Write-Host "Updating Webservers NSG..."
$webNsg.SecurityRules.Clear()
$webNsg.SecurityRules.Add($allowHttpRule)
$webNsg.SecurityRules.Add($allowHttpsRule)
$webNsg.SecurityRules.Add($allowVNetRule)
Set-AzNetworkSecurityGroup -NetworkSecurityGroup $webNsg | Out-Null
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Idempotent NSG creation/update: you correctly Get existing NSG and either New or Clear/Add and Set. This is good. One caution: re-using the same rule objects across NSGs is acceptable, but if you later mutate the objects in-place it could lead to surprises — consider creating separate rule objects per NSG if you change rules dynamically.

Comment thread task.ps1
Comment on lines +81 to +87
$vnet.Subnets | ForEach-Object {
if ($_.Name -eq $webSubnetName) { $_.NetworkSecurityGroup = $webNsg }
elseif ($_.Name -eq $dbSubnetName) { $_.NetworkSecurityGroup = $dbNsg }
elseif ($_.Name -eq $mgmtSubnetName) { $_.NetworkSecurityGroup = $mgmtNsg }
}
$vnet | Set-AzVirtualNetwork | Out-Null

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When associating NSGs to subnets you re-query the VNet and iterate vnet.Subnets. Add a defensive null-check to ensure $vnet was returned (avoid unhandled errors) and consider logging if an expected subnet wasn't found so it's visible in CI/automation logs.

Comment thread task.ps1 Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

After the script finishes the repository must still run the artifacts generation and validation scripts and commit the produced artifacts.json and result.json as required by the task. The script's final Write-Host message is fine, but remember to run and commit the artifact workflow before submitting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants