Skip to content
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

Remove AccessRestrictionIPRange empty defaultvalue #109

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

jvhoof
Copy link
Contributor

@jvhoof jvhoof commented Jun 22, 2023

Parameter AccessRestrictionIPRange needs to have a proper ip range value for the template to deploy. Removing the defaultvalue will cause the deployment via Azure Portal to ask for a value before it can deploy.

@JaydenLiang JaydenLiang self-requested a review July 20, 2023 17:31
@JaydenLiang JaydenLiang self-requested a review July 20, 2023 17:34
Copy link
Contributor

@JaydenLiang JaydenLiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jvhoof putting a default empty value there is on purposes, which is for users who don't want to set any ip restrictions to their function app (see: https://learn.microsoft.com/en-us/azure/app-service/app-service-ip-restrictions)

any thought?

@jvhoof
Copy link
Contributor Author

jvhoof commented Jul 21, 2023

The current empty defaultvalue will cause the deployment to fail. 2 possible options:

  • Remove the defaultvalue and it becomes a required field to be provided and a customer can make a choice
  • Set the defaultvalue to '0.0.0.0/0' which leaves the Azure Function open by default
    The goal here is to make the deployment of the templates work with only filling in the required values. We have had several deployments internal and external fail because of this.

@JaydenLiang
Copy link
Contributor

JaydenLiang commented Jul 25, 2023

The current empty defaultvalue will cause the deployment to fail. 2 possible options:

  • Remove the defaultvalue and it becomes a required field to be provided and a customer can make a choice
  • Set the defaultvalue to '0.0.0.0/0' which leaves the Azure Function open by default
    The goal here is to make the deployment of the templates work with only filling in the required values. We have had several deployments internal and external fail because of this.

@jvhoof May I know which step it is causing failiures? or is it failing in other deployment tools like terraform? It should not cause failures if it is deployed with empty string as the default value. Otherwise, it is a flaw in the code which needs to be fixed

@jvhoof
Copy link
Contributor Author

jvhoof commented Jul 26, 2023

@JaydenLiang sure, to make it easy and uniform I have a deployment script you can find here: https://github.com/40net-cloud/fortinet-azure-solutions/blob/main/FortiGate/Autoscale/deploy.sh

This is the Azure CLI command to deploy the ARM template:
az deployment group create --resource-group "$rg"
--name "$deploymentName"
--template-file $templatefilename
--parameters $parameterfilename
--parameters ResourceNamePrefix="$prefix" ServicePrincipalAppID="$appid" ServicePrincipalAppSecret="$appsecret"
ServicePrincipalObjectID="$clientid" FortiAnalyzerIntegrationOptions="no"
FortiGatePSKSecret="$passwd" AdminUsername="$username" AdminPassword="$passwd"
InstanceType="$instancetype" AccessRestrictionIPRange="0.0.0.0/0"

When I remove the AccessRestrictionIPRange="0.0.0.0/0" which keeps the default empty defaultvalue get errors in SetupFunctionApp -> Update Function.

Hope this gives you an idea. In the end I think a defaultvalue of 0.0.0.0/0 or removing the empty defaultvalue make sense as both will provide an IP range if filled in correctly.

Errors on the various levels:
{
"code": "DeploymentFailed",
"message": "At least one resource deployment operation failed. Please list deployment operations for details. Please see https://aka.ms/arm-deployment-operations for usage details.",
"details": [
{
"code": "InvalidTemplate",
"message": "Deployment template validation failed: 'The template 'copy' definition at line '2' and column '3' has an invalid copy count. The copy count must be a positive integer value and cannot exceed '800'. Please see https://aka.ms/arm-resource-loops for usage details.'."
}
]
}

{
"code": "InvalidTemplate",
"message": "Deployment template validation failed: 'The template 'copy' definition at line '2' and column '3' has an invalid copy count. The copy count must be a positive integer value and cannot exceed '800'. Please see https://aka.ms/arm-resource-loops for usage details.'.",
"additionalInfo": [
{
"type": "TemplateViolation",
"info": {
"lineNumber": 2,
"linePosition": 3,
"path": "[0]"
}
}
]
}

{
"code": "DeploymentFailed",
"target": "/subscriptions/f7f4728a-781f-470f-b029-bac8a9df75af/resourceGroups/JVH25-RG/providers/Microsoft.Resources/deployments/JVH25-RG-westeurope",
"message": "At least one resource deployment operation failed. Please list deployment operations for details. Please see https://aka.ms/arm-deployment-operations for usage details.",
"details": [
{
"code": "ResourceDeploymentFailure",
"target": "/subscriptions/f7f4728a-781f-470f-b029-bac8a9df75af/resourceGroups/JVH25-RG/providers/Microsoft.Resources/deployments/SetupFunctionApp",
"message": "The resource write operation failed to complete successfully, because it reached terminal provisioning state 'Failed'."
},
{
"code": "ConflictError",
"message": "A vault with the same name already exists in deleted state. You need to either recover or purge existing key vault. Follow this link https://go.microsoft.com/fwlink/?linkid=2149745 for more information on soft delete."
}
]
}
Screenshot 2023-07-26 at 21 56 36
Screenshot 2023-07-26 at 21 54 49
Screenshot 2023-07-26 at 21 54 09

@alexLUyz alexLUyz merged commit 2e85cb0 into fortinet:main Dec 14, 2023
4 checks passed
@alexLUyz alexLUyz mentioned this pull request Dec 20, 2023
@jvhoof jvhoof deleted the ARMTemplateDefaults branch January 3, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants