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

Virtual network empty string in DNS Servers crashes terraform #2010

Closed
tlcowling opened this issue Oct 3, 2018 · 3 comments · Fixed by #2305
Closed

Virtual network empty string in DNS Servers crashes terraform #2010

tlcowling opened this issue Oct 3, 2018 · 3 comments · Fixed by #2305

Comments

@tlcowling
Copy link

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

I came across a Terraform crash when accidentally passing a variable containing an empty string to a list of DNS servers in the azurerm_virtual_network resource:

Terraform Version

Terraform v0.11.8

  • provider.azurerm v1.16.0

Affected Resource(s)

  • azurerm_virtual_network

Terraform Configuration Files

locals {
  name     = "dns_empty_test"
  location = "centralus"
}

resource "azurerm_resource_group" "test" {
  name     = "${local.name}"
  location = "${local.location}"
}

resource "azurerm_virtual_network" "test" {
  address_space       = ["10.0.0.0/16"]
  location            = "${local.location}"
  name                = "${local.name} network"
  resource_group_name = "${azurerm_resource_group.test.name}"

  dns_servers = [
    "",
  ]
}

Panic Output

panic: interface conversion: interface {} is nil, not string
github.com/terraform-providers/terraform-provider-azurerm/azurerm/resource_arm_virtual_network.go:239 +0xda5

Expected Behavior

An indication that the input is invalid instead of a terraform crash on apply. Maybe there could be a validation message at the plan stage?

Actual Behavior

Terraform crash

Steps to Reproduce

  1. terraform apply (against above config example)
@ghost ghost added bug crash labels Oct 3, 2018
@tlcowling
Copy link
Author

tlcowling commented Oct 3, 2018

My understanding is when terraform reads the dns_servers attribute from the resource, [“”] in hcl becomes [<nil>] - the type assertion to a []interface{} works - but when we do dns.(string)) to string we end up with a conversion error because dns is not a string its a nil

dnses := []string{}
for _, dns := range d.Get("dns_servers").([]interface{}) {
	dnses = append(dnses, dns.(string))
}

Proposed Solution

master...tlcowling:dnsserverbug

I tried adding a validation function to dns_servers in the resource to catch this case and give a useful error. That would have helped me when I accidentally added an empty variable

Error: azurerm_virtual_network.test: DNS Server Entry is invalid, cannot be empty string

For now I’m just guarding against empty strings which cause terraform to crash. Validation of the DNS server is done by the Azure API where an invalid entry ends up throwing an error on apply like this:

* azurerm_virtual_network.test: Error Creating/Updating Virtual Network "dns_empty_test network" (Resource Group "dns_empty_test"): network.VirtualNetworksClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="InvalidResourceName" Message="Resource name dns_empty_test network is invalid. The name can be up to 80 characters long. It must begin with a word character, and it must end with a word character or with '_'. The name may contain word characters or '.', '-', '_'." Details=[]

Do you think its worth adding more validation to the DNS Server entries? A more thorough DNS validation for example... I think theres value in getting feedback in a plan before an apply

@JunyiYi JunyiYi self-assigned this Nov 14, 2018
@JunyiYi JunyiYi added this to the 1.20.0 milestone Nov 14, 2018
@JunyiYi
Copy link

JunyiYi commented Nov 14, 2018

Thanks @tlcowling for reporting the issue. And your proposal is correct, I have created a PR (#2305) to make sure they are not empty strings.

For now, I will just fix the crash here. Of course we can do a more sophisticated validation (for example, resource name validation, IPv4 and IPv6 validations). But let's keep it as a TODO item.

@tombuildsstuff tombuildsstuff modified the milestones: 1.20.0, 1.19.0 Nov 15, 2018
@ghost
Copy link

ghost commented Mar 5, 2019

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 Mar 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants