feat: Update avm/res/web/static-site - Add additional param for publicNetworkAccess
#4286
Conversation
…NetworkAccess` parameter due to outdated API Azure#4283
microsoft-github-policy-service
bot
added
the
Needs: Triage 🔍
|
Important The "Needs: Triage 🔍" label must be removed once the triage process is complete! Tip For additional guidance on how to triage this issue/PR, see the BRM Issue Triage documentation. |
microsoft-github-policy-service
bot
added
the
Type: AVM 🅰️ ✌️ Ⓜ️
|
Important If this is a module-related PR, being submitted by the sole owner of the module, the AVM core team must review and approve it (as module owners can't approve their own PRs). To indicate this PR needs the core team''s attention, apply the "Needs: Core Team 🧞" label! The core team will only review and approve PRs that have this label applied! |
ChrisSidebotham
added
the
Needs: Core Team 🧞
avm/res/web/static-site/main.bicep
Outdated
| @@ -88,6 +88,9 @@ param functionAppSettings object = {} | |||
| @description('Optional. The custom domains associated with this static site. The deployment will fail as long as the validation records are not present.') | |||
| param customDomains array = [] | |||
|
|
|||
| @description('Optional. The public network access settings for the static site. `Disabled` is configured by default.') | |||
There was a problem hiding this comment.
Leveraging the PR to trigger a discussion about this setting in general and current implementation across the library.
I see the inconsistency is strong with this one.
In CARML we started disabling publicNetworkAccess within the resource implementation instead of through its default value, and only in case private endpoint is enabled. This is still the current implementation for multiple modules, such as for example storage-account
The reasoning behind was that it won't be useful to disable public network access by default if private endpoint is not configured. In these cases the default value is set to empty ''.
Meanwhile, many other resource modules are now disabling publicNetworkAccess by default via default values in AVM. An example is RSV. In these cases the default value is set to Disabled.
Needless to say, we also have cases where the default value is set to Enabled.
I think it would be nice to align the behavior.
There was a problem hiding this comment.
I could imagine that some resources may have PSRule tests that request one or the other. Don't have an example at hand though (for RSV, for example, this test does not exist, so I guess the person implementing the parameter simply decided that it would be a good solution).
One added caveat is that aside from PEs, one could also navigate to a resource like a storage account using service endpoints, making the matter even harder to define a default for.
There was a problem hiding this comment.
I am about t commit another push to this which updates it to the same configuration as we use for PaaS Services, but I am removing networkacl as this is not supportted for this resource
There was a problem hiding this comment.
Thanks a lot @ChrisSidebotham I agree with latest changes! Only comment would be, if networkAcls are not supported, to remove that mention from the publicNetworkAccess description metadata. Would you agree? Providing a suggestion to clarify
eriqua
changed the title
publicNetworkAccess in avm-res-web-static-siteavm/res/web/static-site - Add additional param for publicNetworkAccess
eriqua
added
Class: Resource Module 📦
…d values for clarity and security
| @@ -88,6 +88,14 @@ param functionAppSettings object = {} | |||
| @description('Optional. The custom domains associated with this static site. The deployment will fail as long as the validation records are not present.') | |||
| param customDomains array = [] | |||
|
|
|||
| @description('Optional. Whether or not public network access is allowed for this resource. For security reasons it should be disabled. If not specified, it will be disabled by default if private endpoints are set and networkAcls are not set.') | |||
There was a problem hiding this comment.
| @description('Optional. Whether or not public network access is allowed for this resource. For security reasons it should be disabled. If not specified, it will be disabled by default if private endpoints are set and networkAcls are not set.') | |
| @description('Optional. Whether or not public network access is allowed for this resource. For security reasons it should be disabled. If not specified, it will be disabled by default if private endpoints are set.') |
Description
#Closes #4283
Pipeline Reference
Type of Change
version.json:version.json.version.json.Checklist
Set-AVMModulelocally to generate the supporting module files.