Solution#49
Conversation
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 one core requirement is not fully implemented yet: all subnets must allow traffic from other subnets within the virtual network. Right now, your NSGs rely on Azure’s default VirtualNetwork-allow rules, but the task explicitly expects you to plan and create NSG rules, which means you should add explicit Allow rules with SourceAddressPrefix = 'VirtualNetwork' (or the VNet CIDR) for each NSG. For example, in task.ps1, where you define the webservers and management NSGs (look around the New-AzNetworkSecurityRuleConfig calls for HTTP/HTTPS and SSH), add additional rules that allow inbound traffic from VirtualNetwork on the appropriate ports, and include them in the -SecurityRules array you pass into New-AzNetworkSecurityGroup. For the database NSG (near the comment above its creation, which currently incorrectly refers to the management NSG), add at least one explicit inbound rule that allows traffic from VirtualNetwork while still not adding any rule with SourceAddressPrefix = 'Internet', so the database stays non‑Internet-facing. After updating task.ps1, re-run it to deploy, then re-run scripts/generate-artifacts.ps1 and scripts/validate-artifacts.ps1 to refresh artifacts.json and ensure all tests pass, and commit the updated results (including any required result.json). Overall, your structure, subnet-to-NSG wiring, and Internet rules are solid; once you add clear intra‑VNet allow rules and fix the small comment typo above the database NSG, your solution will fully align with the stated requirements.
✨ 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 web network security group..." | ||
| # Write your code for creation of Web NSG here -> | ||
| # Write your code for creation of Web NSG here -> |
There was a problem hiding this comment.
The comment still says "Write your code" – the implementation is present and correct for web NSG creation, but consider verifying whether you also need an explicit rule here to allow VNet (inter-subnet) traffic, instead of relying only on default NSG behavior, since the task explicitly mentions allowing traffic from other subnets.
| -SourcePortRange * ` | ||
| -DestinationAddressPrefix * ` | ||
| -DestinationPortRange 22 | ||
| $mngNsg = New-AzNetworkSecurityGroup ` |
There was a problem hiding this comment.
This NSG currently has only the SSH-from-Internet rule; double-check whether you must explicitly allow traffic from VirtualNetwork to satisfy the "all subnets should allow traffic from other subnets" requirement, rather than depending on default rules.
| Write-Host "Creating dbSubnet network security group..." | ||
| # Write your code for creation of management NSG here -> | ||
| # Write your code for creation of management NSG here -> | ||
| $dbNsg = New-AzNetworkSecurityGroup ` |
There was a problem hiding this comment.
The database NSG has no explicit inbound rules, which correctly blocks Internet traffic; however, if the tests require explicit intra-VNet allow rules, you may need to add a rule that allows traffic from VirtualNetwork while still not permitting Internet as a source.
3 participants
No description provided.