Add ns update support

[jbe-dw]

This is an example for issue #63

[jbe-dw]

I don't know if this one is in line with your philosphy, as it tricks the zone update function to make a record update. It is an exception to not follow the API 1:1 but makes the use comfortable.

Another way would be to remove the nameserver option.

Regards

[mbag]

These lines are not related to NS change, and shouldn't be part of this PR.

[jbe-dw]

You're right, this branch is based on #74 and I lazily kept the commit to show the code. The plan is to drop this commit (9e79c28) if #74 is accepted.

[mbag]

The PRs don't have to be accepted in chronological order. I would like to keep separate issues separate.

[jbe-dw]

Ok, I'll drop it soon

[mbag]

OK, thanks 👍

[jbe-dw]

Removed

[mbag]

TTL shouldn't have value hard coded. It should be set, based on current value on the server. The 3600 is the default that PowerDNS server sets when none is provided(https://doc.powerdns.com/authoritative/settings.html#default-ttl), but according to first paragraph here: https://doc.powerdns.com/authoritative/settings.html, this can be changed in pdns.conf. file, meaning we would be overriding their setting. Also someone might change TTL to custom value, and we should override it.

[jbe-dw]

We could change the struct to mark this field with omitempty, at least.

I can also pull the records first to extract the TTL, or add a parameter to let the user provide one (and falling back to the retrieved value if none is provided).

[mbag]

Pull the records first and extract TTL. Adding parameter for NS TTL to zone doesn't make sense from user point of view as this wouldn't have any effect during zone creation, but only during NS update.

Putting omitempty is good idea, since based on documentation the PowerDNS will fall back to the default value if this field is not provided.

[jbe-dw]

Done. I tested it on my test instance:

• Setting a default ttl to 600
• Create the zone: the NS RRSet have a TTL of 600
• Adding a new name server in my tf code
• The RRSet is still 600.
• Removing the NS from tf also removes the zone NS.
• The API returns an error if the TTL is omitted in the RRSet: "Key 'ttl' not an Integer or not present". It is computed from existing NS (or default to the SOA value if none are present. The SOA can be removed. We can add a default value then or do not try the SOA at all)

[mbag]

Same argument as it was made above, TTL shouldn't be hardcoded

[jbe-dw]

Same here, but It looks like it is obsolete to set it at the record level. It's commented so in the client.
Let me know, I can apply the same logic as the RRSet or just remove the field from the client.

[jbe-dw]

I confirm that with the API v1 the TTL is on the RRSet and not on the record

[jbe-dw]

Changing this has no effect in PowerDNS > 4.3 but I use the RRSet value anyway

[mbag]

@jbe-dw why did you add condition for checking size of nameserver list on this line? https://github.com/pan-net/terraform-provider-powerdns/blob/master/powerdns/resource_powerdns_zone.go#L143

I just realized the tests are failing and I have identified this to be the reason. I have used fmt.Println to print the contents of the d.Get("nameservers") and found out that when tests succeeded in the past the length of this variable is 0, i.e. it's empty list. I think this has something to do with the fact we are using ImportStatePassthrough. In case you are importing existing zone, then d.Get("nameservers") would be empty list, as this doesn't exist in the statefile yet. I would like to remove the && len(..) condition, but I'm wondering if this will break the update nameserver functionality.

[jbe-dw]

I added this check for the update, because the state would be different than the definition and it triggers a change that never applies when nameservers paramater is not used. And you wouldn't want to delete the NS records as they can be managed through powerdns_record.

[mbag]

It seems that it breaks terraform import functionality for the zone as tests suggest. Can you fix make a quick fix, so that it works for import? Otherwise I will have to revert the commit adding this functionality until this is resolved.

Here is one of the tests that fails.

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -parallel 5 -run TestAccPDNSZoneMaster$ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-powerdns	[no test files]
=== RUN   TestAccPDNSZoneMaster
=== PAUSE TestAccPDNSZoneMaster
=== CONT  TestAccPDNSZoneMaster
    testing.go:569: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
        
        (map[string]string) {
        }
        
        
        (map[string]string) (len=3) {
         (string) (len=13) "nameservers.#": (string) (len=1) "2",
         (string) (len=21) "nameservers.186496283": (string) (len=13) "ns2.sysa.abc.",
         (string) (len=22) "nameservers.2088986603": (string) (len=13) "ns1.sysa.abc."
        }
        
--- FAIL: TestAccPDNSZoneMaster (0.24s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-powerdns/powerdns	0.252s
FAIL

[selfuryon]

Tests are still broken in importing nameservers section :(