search

dmacvicar / terraform-provider-libvirt

Terraform provider to provision infrastructure with Linux's KVM using libvirt
Apache License 2.0
1.54k stars 457 forks source link

Fix DNS enabled behavior #993

Closed omertuc closed 8 months ago

omertuc commented 1 year ago

tl;dr

This commit fixes a bug where a regression in the getDNSEnableFromResource function made it impossible to turn off DNS for libvirt networks

Background

In libvirt, DNS is enabled by default in a network if unspecified.

In this provider, according to the docs, DNS is disabled if unspecified.

This means libvirt and the provider have opposing defaults for DNS enabled. This is legitimate behavior and not the bug being fixed by this PR.

Issue 1

Commit eef2d215d caused a change in behavior where libvirt-terraform-provider changed its behavior to be different from what the docs says - it defaults to DNS enabled if unspecified.

Cause

This is caused by returning an empty string in getDNSEnableFromResource instead of "no", which delegates the defaulting to libvirt itself, but that's the wrong thing to do because the default behavior of libvirt is to enable DNS when unspecified.

Issue 2 (same cause)

The second return value of the Terraform SDK GetOK function is defined as "whether or not the key has been set to a non-zero value at some point". So even if the user explicitly sets DNS enabled to false, the GetOK function would still return false as its second return value (since false is considered a "zero" value for booleans) and so the getDNSEnableFromResource function will return an empty string and DNS would get enabled

ghuntley commented 1 year ago

bump :P