test#46
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The NSG naming convention must match the subnet names exactly (e.g., 'webservers', 'management', 'database') instead of using prefixed names like 'nsg-webservers'. Additionally, inbound security rules must be added to each NSG to allow traffic between subnets within the virtual network (10.20.30.0/24). The current implementation lacks these rules and contains misleading placeholder comments.
✨ 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
| $mngSubnetName = "management" | ||
| $mngSubnetIpRange = "10.20.30.128/26" | ||
|
|
||
| $webnsgname = "nsg-" + $webSubnetName |
There was a problem hiding this comment.
The NSG name 'nsg-webservers' does not match the requirement that NSGs must have the same name as the subnet they are attached to. According to the task requirements, this NSG should be named 'webservers', not 'nsg-webservers'.
| $mngSubnetIpRange = "10.20.30.128/26" | ||
|
|
||
| $webnsgname = "nsg-" + $webSubnetName | ||
| $mngnsgname = "nsg" + $mngSubnetName |
There was a problem hiding this comment.
The NSG name 'nsgmanagement' is missing a hyphen and also should be just 'management' according to the requirement. Should be $mngnsgname = $mngSubnetName instead.
|
|
||
| $webnsgname = "nsg-" + $webSubnetName | ||
| $mngnsgname = "nsg" + $mngSubnetName | ||
| $dbnsgname = "nsg-" + $dbSubnetName |
There was a problem hiding this comment.
The NSG name 'nsg-database' does not match the requirement. According to the task requirements, this NSG should be named 'database', not 'nsg-database'. Should be $dbnsgname = $dbSubnetName instead.
| # NETWORK RULES | ||
| $webnsgruleHTTP = New-AzNetworkSecurityRuleConfig -Name $webnsgrulenameHTTP -Access Allow -Protocol Tcp -Direction Inbound -Priority 110 -SourceAddressPrefix * -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 80, 443 | ||
| $mngnsgruleSSH = New-AzNetworkSecurityRuleConfig -Name $mngnsgrulenameSSH -Access Allow -Protocol Tcp -Direction Inbound -Priority 100 -SourceAddressPrefix * -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 22 | ||
|
|
||
| # NETWORK SECURITY GROUPS |
There was a problem hiding this comment.
The task requirement states that 'all subnets should allow traffic from other subnets within the virtual network', but no rules are configured for inter-subnet communication. You need to add rules that allow inbound traffic from the virtual network's address prefix (10.20.30.0/24) for each NSG.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed - This implementation has critical runtime bugs that will cause NSG rules to be silently lost. The main issues are duplicate security rule names within NSGs. In Azure, security rules with identical names in the same NSG get merged - the later rule overwrites the earlier one. This means: 1. Webservers NSG - Both webnsgruleHTTP and webnsgruleInternal are assigned the name "HTTP". When deployed, the internal rule will silently overwrite the HTTP/HTTPS from Internet rule, breaking external access. 2. Management NSG - Both $mngnsgruleSSH and $mngnsgruleInternal use the name "SSH". Similarly, the internal rule will overwrite the SSH from Internet rule. 3. Resource group name - Uses mate-azure-task-16 instead of the required mate-resources. To fix: Use unique names like HTTP-Internal/AllowHTTPInternal and SSH-Internal/AllowSSHInternal for the internal traffic rules, and update the resource group to mate-resources. Once fixed, the webservers and management NSGs will correctly have two rules each (Internet-facing and internal), and resources will be placed in the correct resource group.
✨ 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 your code for creation of management NSG here -> | ||
|
|
||
| $webnsgruleHTTP = New-AzNetworkSecurityRuleConfig -Name $webnsgrulenameHTTP -Access Allow -Protocol Tcp -Direction Inbound -Priority 110 -SourceAddressPrefix * -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 80, 443 | ||
| $webnsgruleInternal = New-AzNetworkSecurityRuleConfig -Name $webnsgrulenameHTTP -Access Allow -Protocol Tcp -Direction Inbound -Priority 110 -SourceAddressPrefix "10.20.30.0/24" -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 80, 443 |
There was a problem hiding this comment.
Critical bug: This rule uses the same name 'HTTP' as the rule on line 32. PowerShell NSGs overwrite rules with duplicate names, so $webnsgruleHTTP will be silently lost and this NSG won't accept HTTP/HTTPS from the Internet. Use a unique name like 'HTTP-Internal' or 'AllowHTTPInternal'.
| $webnsgruleHTTP = New-AzNetworkSecurityRuleConfig -Name $webnsgrulenameHTTP -Access Allow -Protocol Tcp -Direction Inbound -Priority 110 -SourceAddressPrefix * -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 80, 443 | ||
| $webnsgruleInternal = New-AzNetworkSecurityRuleConfig -Name $webnsgrulenameHTTP -Access Allow -Protocol Tcp -Direction Inbound -Priority 110 -SourceAddressPrefix "10.20.30.0/24" -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 80, 443 | ||
| $mngnsgruleSSH = New-AzNetworkSecurityRuleConfig -Name $mngnsgrulenameSSH -Access Allow -Protocol Tcp -Direction Inbound -Priority 100 -SourceAddressPrefix * -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 22 | ||
| $mngnsgruleInternal = New-AzNetworkSecurityRuleConfig -Name $mngnsgrulenameSSH -Access Allow -Protocol Tcp -Direction Inbound -Priority 100 -SourceAddressPrefix "10.20.30.0/24" -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 22 |
There was a problem hiding this comment.
Critical bug: This rule uses the same name 'SSH' as the rule on line 34. The $mngnsgruleSSH rule will be silently lost, so this NSG won't accept SSH from the Internet. Use a unique name like 'SSH-Internal' or 'AllowSSHInternal'.
| $webnsgruleInternal = New-AzNetworkSecurityRuleConfig -Name $webnsgrulenameHTTP -Access Allow -Protocol Tcp -Direction Inbound -Priority 110 -SourceAddressPrefix "10.20.30.0/24" -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 80, 443 | ||
| $mngnsgruleSSH = New-AzNetworkSecurityRuleConfig -Name $mngnsgrulenameSSH -Access Allow -Protocol Tcp -Direction Inbound -Priority 100 -SourceAddressPrefix * -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 22 | ||
| $mngnsgruleInternal = New-AzNetworkSecurityRuleConfig -Name $mngnsgrulenameSSH -Access Allow -Protocol Tcp -Direction Inbound -Priority 100 -SourceAddressPrefix "10.20.30.0/24" -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 22 | ||
| $dbnsgruleInternal = New-AzNetworkSecurityRuleConfig -Name $mngnsgrulenameSSH -Access Allow -Protocol Tcp -Direction Inbound -Priority 100 -SourceAddressPrefix "10.20.30.0/24" -SourcePortRange * -DestinationAddressPrefix * -DestinationPortRange 22 |
There was a problem hiding this comment.
Semantic bug: This rule is for database internal traffic but uses the variable $mngnsgrulenameSSH which has value 'SSH'. This is misleading and should use a distinct rule name like 'Internal' or 'AllowInternal'.
| @@ -1,4 +1,4 @@ | |||
| $location = "uksouth" | |||
| $location = "polandcentral" | |||
| $resourceGroupName = "mate-azure-task-16" | |||
There was a problem hiding this comment.
Resource group name mismatch: The requirement specifies the resource group must be named 'mate-resources', but this script uses 'mate-azure-task-16'. Either update the name or ensure this is intentional for testing.
| @@ -23,8 +29,18 @@ Write-Host "Creating mngSubnet network security group..." | |||
| Write-Host "Creating dbSubnet network security group..." | |||
There was a problem hiding this comment.
The comment says 'management NSG' but this is actually for the database subnet. This inconsistency could cause confusion during maintenance.
| @@ -23,8 +29,18 @@ Write-Host "Creating mngSubnet network security group..." | |||
| Write-Host "Creating dbSubnet network security group..." | |||
| # Write your code for creation of management NSG here -> | |||
There was a problem hiding this comment.
Comment text is identical to line 27, suggesting copy-paste error. This should reference the database subnet instead.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved! All NSGs are properly created with unique rule names and attached to their respective subnets. Great job fixing the duplicate rule names issue — webservers now correctly uses 'HTTP/HTTPS' and 'Internal', while management uses 'SSH' and 'Internal'. The resource group is correctly named 'mate-resources', and each subnet has appropriate Internet-facing access rules with internal traffic properly configured between all subnets. Well done on completing this Azure networking task!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.