-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
azurerm_app_service
ip_restriction
- support for name
& priority
#6705
azurerm_app_service
ip_restriction
- support for name
& priority
#6705
Conversation
azurerm/helpers/azure/app_service.go
Outdated
Type: schema.TypeInt, | ||
Optional: true, | ||
Computed: true, | ||
ValidateFunc: validation.IntAtLeast(1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you check here as well if priority is maximum 65000?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure about the max value being 65000? after some testing, it looks like the max value I can set for priority is 2147483647. If I go above that, priority will be set to a negative value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question actually. I just tried to set a rule via az powershell:
Add-AzWebAppAccessRestrictionRule: Cannot validate argument on parameter 'Priority'. The 65001 argument is greater than the maximum allowed range of 65000. Supply an argument that is less than or equal to 65000 and then try the command again.
The portal lets you also set something larger than 65000 and enforces the limit at 2147483647. So I guess you are right, that should be it.
@@ -309,6 +309,10 @@ A `ip_restriction` block supports the following: | |||
|
|||
-> **NOTE:** One of either `ip_address` or `virtual_network_subnet_id` must be specified | |||
|
|||
* `name` - (Optional) The name for this IP Restriction. | |||
|
|||
* `priority` - (Optional) The priority for this IP Restriction. Restrictions are enforced in priority order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a note that it defaults to 65000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, thanks for the info
"priority": { | ||
Type: schema.TypeInt, | ||
Optional: true, | ||
Computed: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as @sebader noted, priority defaults to 65000 when not provided. Should we make that an explicit default or let it be computed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure where the default of 65000 is injected in this case. Might be in the Go SDK? Since in CLI, Powershell and the portal the parameter must be manually specified, there is no default there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So might actually be better so explicitly set it?!
nice, looks good to me know @aqche Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @aqche
Thanks for this PR. Just a few minor changes needed and I think this should be good to merge.
Ste
azurerm/helpers/azure/app_service.go
Outdated
"name": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
ValidateFunc: validation.StringIsNotEmpty, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll want Computed: true,
on here.
azurerm/helpers/azure/app_service.go
Outdated
"priority": { | ||
Type: schema.TypeInt, | ||
Optional: true, | ||
Default: 65000, | ||
ValidateFunc: validation.IntBetween(1, 2147483647), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be Computed: true,
rather than setting a default. If more than one IP Restriction is used this could cause a conflict. The "default" value is assigned by the platform, rather than it being in the SDK, so can safely be read back from the API if not explicitly set.
@@ -514,6 +514,27 @@ func TestAccAzureRMAppService_oneIpRestriction(t *testing.T) { | |||
}) | |||
} | |||
|
|||
func TestAccAzureRMAppService_completeIpRestriction(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have an additional test that updates an app_service by adding, then removing, a 2nd ip_restriction
? (or extend this one)
@jackofallops thanks for the review! made the updates you suggested. let me know what you think!
|
When I compare with the configuration options in the Azure portal, it seems that the action "allow/deny" is missing. If there were no deny rules, then there would be no need for assigning priorities - similar to NSG rules. |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
Fixes: #6388
Adds the
name
andpriority
options to theazurerm_app_service
resource'sip_restriction
option.