Created NSG for subnets#43
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements Network Security Groups (NSGs) for Azure subnets in a todo application infrastructure. The changes update the Azure location from UK South to France Central and add comprehensive NSG configurations for web, database, and management subnets with appropriate security rules.
Changes:
- Updated Azure deployment region from "uksouth" to "francecentral"
- Created NSG rules for web subnet (HTTP/HTTPS access), management subnet (SSH access), and database subnet (SQL access from web and management subnets)
- Associated the created NSGs with their respective subnet configurations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| task.ps1 | Added NSG creation with security rules for web, database, and management subnets; updated location; reformatted code with backtick line continuations |
| scripts/artifacts.json | Added new artifacts configuration file with Azure Blob Storage template URL |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -ResourceGroupName $resourceGroupName ` | ||
| -Location $location ` | ||
| -Name $dbSubnetName ` | ||
| -SecurityRules $db_nsg_rule_web, $db_nsg_rule_mng |
There was a problem hiding this comment.
The database NSG is missing the general inbound and outbound rules ($nsg_rule_inbound, $nsg_rule_outbound) that are included in the web and management NSGs. This inconsistency could lead to unexpected connectivity issues between subnets. Consider adding these rules to maintain consistent inter-subnet communication.
| -SecurityRules $db_nsg_rule_web, $db_nsg_rule_mng | |
| -SecurityRules $db_nsg_rule_web, $db_nsg_rule_mng, $nsg_rule_inbound, $nsg_rule_outbound |
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because several core task requirements are not yet met, which would block proper validation and artifact generation. First, the script creates a resource group named mate-azure-task-16 instead of the required mate-resources; update $resourceGroupName (or the RG creation call) so the infrastructure and artifacts are created in mate-resources exactly. Second, the task explicitly requires a storage account and a task-artifacts container inside mate-resources, but task.ps1 does not create them; add commands such as New-AzStorageAccount and New-AzStorageContainer targeting the mate-resources resource group so scripts/generate-artifacts.ps1 can upload the exported template. Third, your intra‑VNet NSG rules currently use -Protocol Tcp for both inbound and outbound; change these to -Protocol * (or Any) so all protocols are allowed between subnets, as required. Fourth, the database NSG only includes the SQL-specific rules; you need to also attach the general VNet allow rules (or add an explicit Allow rule with SourceAddressPrefix set to VirtualNetwork) so the DB subnet also fully participates in intra‑VNet traffic. Once you’ve made these changes, re-run the artifact generation/validation scripts and commit the updated task.ps1, scripts/artifacts.json (already present) and the resulting result.json; your NSG structure is otherwise on the right track, so these adjustments should bring the solution fully in line with the checklist.
✨ 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
|
|
||
| Write-Host "Creating a resource group $resourceGroupName ..." | ||
| New-AzResourceGroup -Name $resourceGroupName -Location $location | ||
| New-AzResourceGroup ` |
There was a problem hiding this comment.
The script creates the resource group using the variable $resourceGroupName (value mate-azure-task-16). The task requires creating a resource group named mate-resources. Update the variable or creation call so the resource group is named exactly mate-resources (or create that RG as required by the checklist).
| -Name $resourceGroupName ` | ||
| -Location $location |
There was a problem hiding this comment.
The task requires creating a storage account (any name) and a task-artifacts container inside the mate-resources resource group. This script does not create a storage account or container — add steps that create a storage account and then create the container (e.g. New-AzStorageAccount and New-AzStorageContainer) in the required RG so scripts/generate-artifacts.ps1 can upload artifacts.
| -Name all-inbound-rule ` | ||
| -Description "Allow all subnets Inbound" ` | ||
| -Access Allow ` | ||
| -Protocol Tcp ` |
There was a problem hiding this comment.
The VNet-wide inbound rule uses -Protocol Tcp. The requirement says “All subnets should allow traffic from other subnets within the virtual network.” That should allow all protocols between subnets — change -Protocol Tcp to -Protocol * (or -Protocol Any) so non-TCP traffic between subnets is permitted.
| -Name all-outbound-rule ` | ||
| -Description "Allow all subnets Outbound" ` | ||
| -Access Allow ` | ||
| -Protocol Tcp ` |
There was a problem hiding this comment.
Same issue for the outbound rule: it uses -Protocol Tcp. Make this rule apply to all protocols (use -Protocol *) so intra-VNet traffic is not unintentionally restricted to TCP only.
| -ResourceGroupName $resourceGroupName ` | ||
| -Location $location ` | ||
| -Name $dbSubnetName ` | ||
| -SecurityRules $db_nsg_rule_web, $db_nsg_rule_mng |
There was a problem hiding this comment.
When creating $db_nsg you only attach the two SQL-specific rules ($db_nsg_rule_web, $db_nsg_rule_mng). The checklist requires that all subnets allow traffic from other subnets within the virtual network — add the general VNet allow rules ($nsg_rule_inbound and $nsg_rule_outbound) to the database NSG as well (or add an explicit Allow rule with SourceAddressPrefix/SourceApplicationSecurityGroup set to the VNet/VirtualNetwork tag).
|
@mateacademy-ai-mentor Webservers повинна приймати лише HTTP/HTTPS-трафік з Інтернету. Моє рішення: Висновок: У описі завдання: ❌ немає вимоги створювати RG з фіксованою назвою |
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This requires a few changes to meet the project's core requirements. Thank you for your detailed message; let's clarify the points you raised.
There are two main areas to address:
-
Prerequisites: The task description, under the
## Prerequisitessection, specifies that the resource group must be named"mate-resources". Your script also needs to create a storage account and a"task-artifacts"container within that group for the validation scripts to function. Currently, your script uses a different resource group name and omits the storage resources. -
Database NSG Configuration: The automated check is failing because the
databaseNSG is missing the required rules for internal VNet communication. The requirement is that "all subnets should allow traffic from other subnets within the virtual network." Your current rule only allows PostgreSQL traffic. To fix this, please add your existing$nsg_rule_inboundand$nsg_rule_outboundrules to theNew-AzNetworkSecurityGroupcommand for thedatabaseNSG, just as you did for thewebserversandmanagementNSGs.
Addressing these points will align your solution with the task requirements and allow the validation to pass.
✨ 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
| Write-Host "Creating a resource group $resourceGroupName ..." | ||
| New-AzResourceGroup -Name $resourceGroupName -Location $location | ||
| New-AzResourceGroup ` | ||
| -Name $resourceGroupName ` |
There was a problem hiding this comment.
Згідно з вимогами завдання (розділ Prerequisites), назва групи ресурсів має бути mate-resources. Поточне значення mate-azure-task-16 є невірним і призведе до помилок під час валідації. Будь ласка, оновіть значення змінної $resourceGroupName на початку скрипта.
| -ResourceGroupName $resourceGroupName ` | ||
| -Location $location ` | ||
| -Name $dbSubnetName ` | ||
| -SecurityRules $db_nsg_rule |
There was a problem hiding this comment.
Вимога завдання — «all subnets should allow traffic from other subnets within the virtual network» — означає, що потрібно дозволити увесь трафік між підмережами, а не тільки для певного порту. Поточне правило $db_nsg_rule дозволяє лише трафік для PostgreSQL. Вам потрібно додати сюди загальні правила для VNet-трафіку ($nsg_rule_inbound та $nsg_rule_outbound), як ви це зробили для webservers та management.
|
Неправильне зауваження: |
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! I am approving your solution as it is now fully compliant with all requirements.
Thank you for your clarification regarding the mate-resources resource group. You were absolutely correct, and my previous feedback on that was mistaken—I apologize for the confusion. Your approach of creating a separate resource group for the task's resources was the right one.
You've done a fantastic job implementing all the Network Security Group rules for each subnet. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "resourcesTemplate": "https://matefiles789.blob.core.windows.net/task-artifacts/task16/exported-template.json?sv=2025-07-05&se=2026-03-16T10%3A33%3A30Z&sr=b&sp=r&sig=yO4hamsUt7unsWCHNN%2BvnNoC0naEowI4XaAsRn1EZns%3D" | |||
There was a problem hiding this comment.
The task requirements specify that the resource group must be named mate-resources. The automated validation scripts expect to find all deployed resources, including the virtual network and NSGs, within this specific resource group.
No description provided.