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

azurerm_firewall: support snat private ip ranges #7535

Closed
wants to merge 1 commit into from

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Jun 30, 2020

Support SNAT private ip ranges in azurerm_firewall.

Test Result

💤 via 🦉 v1.14.4 make testacc TEST=./azurerm/internal/services/network/tests TESTARGS="-run='TestAccAzureRMFirewall_basic'"
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/network/tests -v -run='TestAccAzureRMFirewall_basic' -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMFirewall_basic
=== PAUSE TestAccAzureRMFirewall_basic
=== CONT  TestAccAzureRMFirewall_basic
--- PASS: TestAccAzureRMFirewall_basic (1044.62s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/network/tests       1044.638s

(Fix #7504)

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @magodo

Thanks for this PR :)

Taking a look through whilst this works it appears these fields need to be added to the Swagger?

Thanks!

rangeSlice := utils.ExpandStringSlice(v.(*schema.Set).List())
if rangeSlice != nil {
privateIpRanges = strings.Join(*rangeSlice, ",")
parameters.AdditionalProperties["Network.SNAT.PrivateRanges"] = &privateIpRanges
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason these fields aren't exposed in the Swagger? historically the Networking API's have introduced breaking schema changes - so this is less likely if these are typed, since they'll be caught via ARM review?

@magodo
Copy link
Collaborator Author

magodo commented Jul 1, 2020

@tombuildsstuff Seems ARM review will not check the changes on those mappings, as reviewer might not have knowledge on whether that is a regular map accepting arbitrary kv or a map playing role as special config using certain keys (as this one)

@tombuildsstuff
Copy link
Contributor

@magodo in this instance we've had similar issues with DataBricks in the past and ended up having to send Swagger PR's to fix this (e.g. Azure/azure-rest-api-specs#7872) - so that'd be my next suggestion here

@tombuildsstuff tombuildsstuff added upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR sdk/requires-swagger-changes Changes need to be made in the Swagger specifications to enable this functionality labels Jul 1, 2020
@magodo
Copy link
Collaborator Author

magodo commented Jul 2, 2020

@tombuildsstuff Thank you for the example/suggestion to submit a PR to the Swagger. Whilst currently there is no document enumerating all the available KVs. Hence I'll submit an issue (Azure/azure-rest-api-specs#10015) instead.

@katbyte katbyte added this to the Blocked milestone Jul 2, 2020
@katbyte
Copy link
Collaborator

katbyte commented Jul 7, 2020

as this is blocked on swagger i'm going to close it @magodo, we can revive the branch once they've updated it.

@katbyte katbyte closed this Jul 7, 2020
@ghost
Copy link

ghost commented Aug 7, 2020

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!

@ghost ghost locked and limited conversation to collaborators Aug 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation enhancement sdk/requires-swagger-changes Changes need to be made in the Swagger specifications to enable this functionality service/firewall size/S upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR waiting-response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Azure Firewall SNAT private ranges
3 participants