everything is implemented, now on to testing

engine/resources/cloudflare_dns.go

@@ -32,7 +32,6 @@ package resources
 import (
 	"context"
 	"fmt"
-	"strings"
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
@@ -48,74 +47,40 @@ func init() {
 	engine.RegisterResource("cloudflare:dns", func() engine.Res { return &CloudflareDNSRes{} })
 }
 
-// CloudflareDNSRes is a resource for managing DNS records in Cloudflare zones.
-// This resource uses the Cloudflare API to create, update, and delete DNS
-// records in a specified zone. It supports various record types including A,
-// AAAA, CNAME, MX, TXT, NS, and PTR records. The resource requires polling to
-// detect changes, as the Cloudflare API does not provide an event stream. The
-// Purge functionality allows enforcing that only managed DNS records exist in
-// the zone, removing any unmanaged records.
+// TODO: description of cloudflare_dns resource
 type CloudflareDNSRes struct {
 	traits.Base
 	traits.GraphQueryable
 	init *engine.Init
 
-	// APIToken is the Cloudflare API token used for authentication. This is
-	// required and must have the necessary permissions to manage DNS records
-	// in the specified zone.
 	APIToken string `lang:"apitoken"`
 
-	// Comment is an optional comment to attach to the DNS record. This is
-	// stored in Cloudflare and can be used for documentation purposes.
 	Comment string `lang:"comment"`
 
-	// Content is the value for the DNS record. This is required when State
-	// is "exists" unless Purge is true. The format depends on the record
-	// Type (e.g., IP address for A records, hostname for CNAME records).
 	Content string `lang:"content"`
 
-	// Priority is the priority value for records that support it (e.g., MX
-	// records). This is a pointer to distinguish between an unset value and
-	// a zero value.
-	Priority *float64 `lang:"priority"`
+	// using a *int64 here to help with disambiguating nil values
+	Priority *int64 `lang:"priority"`
 
-	// Proxied specifies whether the record should be proxied through
-	// Cloudflare's CDN. This is a pointer to distinguish between an unset
-	// value and false. Only applicable to certain record types.
+	// using a *bool here to help with disambiguating nil values
 	Proxied *bool `lang:"proxied"`
 
-	// Purge specifies whether to delete all DNS records in the zone that are
-	// not defined in the mgmt graph. When true, this resource will query the
-	// graph for other cloudflare:dns resources in the same zone and delete
-	// any records not managed by those resources.
 	Purge bool `lang:"purge"`
 
-	// RecordName is the name of the DNS record (e.g., "www.example.com" or
-	// "@" for the zone apex). This is required.
 	RecordName string `lang:"record_name"`
 
-	// State determines whether the DNS record should exist or be absent.
-	// Valid values are "exists" (default) or "absent". When set to "absent",
-	// the record will be deleted if it exists.
 	State string `lang:"state"`
 
-	// TTL is the time-to-live value for the DNS record in seconds. Must be
-	// between 60 and 86400, or set to 1 for automatic TTL. Default is 1.
 	TTL int64 `lang:"ttl"`
 
-	// Type is the DNS record type (e.g., "A", "AAAA", "CNAME", "MX", "TXT",
-	// "NS", "SRV", "PTR"). This is required.
 	Type string `lang:"type"`
 
-	// Zone is the name of the Cloudflare zone (domain) where the DNS record
-	// should be managed (e.g., "example.com"). This is required.
 	Zone string `lang:"zone"`
 
 	client *cloudflare.Client
 	zoneID string
 }
 
-// Default returns some sensible defaults for this resource.
 func (obj *CloudflareDNSRes) Default() engine.Res {
 	return &CloudflareDNSRes{
 		State: "exists",
@@ -123,14 +88,13 @@ func (obj *CloudflareDNSRes) Default() engine.Res {
 	}
 }
 
-// Validate checks if the resource data structure was populated correctly.
 func (obj *CloudflareDNSRes) Validate() error {
 	if obj.RecordName == "" {
 		return fmt.Errorf("record name is required")
 	}
 
 	if obj.APIToken == "" {
-		return fmt.Errorf("api token is required")
+		return fmt.Errorf("API token is required")
 	}
 
 	if obj.Type == "" {
@@ -138,7 +102,7 @@ func (obj *CloudflareDNSRes) Validate() error {
 	}
 
 	if (obj.TTL < 60 || obj.TTL > 86400) && obj.TTL != 1 { // API requirement
-		return fmt.Errorf("ttl must be between 60s and 86400s, or set to 1")
+		return fmt.Errorf("TTL must be between 60s and 86400s, or set to 1")
 	}
 
 	if obj.Zone == "" {
@@ -153,21 +117,13 @@ func (obj *CloudflareDNSRes) Validate() error {
 		return fmt.Errorf("content is required when state is 'exists'")
 	}
 
-	if obj.Type == "MX" && obj.Priority == nil {
-		return fmt.Errorf("priority is required for MX records")
-	}
-
-	// cloudflare accepts ~4req/s so this is safe enough even when managing lots
-	// of records
-	if obj.MetaParams().Poll == 0 || obj.MetaParams().Poll < 60 {
-		return fmt.Errorf("cloudflare:dns requires polling, set Meta:poll param (e.g., 300s), min. 60s")
+	if obj.MetaParams().Poll == 0 {
+		return fmt.Errorf("cloudflare:dns requiers polling, set Meta:poll param	(e.g., 60 seconds)")
 	}
 
 	return nil
 }
 
-// Init runs some startup code for this resource. It initializes the Cloudflare
-// API client and validates that the specified zone exists.
 func (obj *CloudflareDNSRes) Init(init *engine.Init) error {
 	obj.init = init
 
@@ -175,6 +131,7 @@ func (obj *CloudflareDNSRes) Init(init *engine.Init) error {
 		option.WithAPIToken(obj.APIToken),
 	)
 
+	//TODO: does it make more sense to check it here or in CheckApply()?
 	zoneListParams := zones.ZoneListParams{
 		Name: cloudflare.F(obj.Zone),
 	}
@@ -193,8 +150,6 @@ func (obj *CloudflareDNSRes) Init(init *engine.Init) error {
 	return nil
 }
 
-// Cleanup is run by the engine to clean up after the resource is done. It
-// clears sensitive data and releases the API client connection.
 func (obj *CloudflareDNSRes) Cleanup() error {
 	obj.APIToken = ""
 	obj.client = nil
@@ -208,11 +163,22 @@ func (obj *CloudflareDNSRes) Watch(context.Context) error {
 	return fmt.Errorf("invalid Watch call: requires poll metaparam")
 }
 
-// CheckApply is the main convergence function for this resource. It checks the
-// current state of the DNS record against the desired state and applies changes
-// if necessary. If apply is false, it only checks if changes are needed. If
-// Purge is enabled, it will first delete any unmanaged records in the zone.
 func (obj *CloudflareDNSRes) CheckApply(ctx context.Context, apply bool) (bool, error) {
+	zone, err := obj.client.Zones.List(ctx, zones.ZoneListParams{
+		Name: cloudflare.F(obj.Zone),
+	})
+	if err != nil {
+		return false, err
+	}
+
+	if len(zone.Result) == 0 {
+		return false, fmt.Errorf("there's no zone registered with name %s", obj.Zone)
+	}
+
+	if len(zone.Result) > 1 {
+		return false, fmt.Errorf("there's more than one zone with name %s", obj.Zone)
+	}
+
 	// We start by checking the need for purging
 	if obj.Purge {
 		checkOK, err := obj.purgeCheckApply(ctx, apply)
@@ -224,12 +190,11 @@ func (obj *CloudflareDNSRes) CheckApply(ctx context.Context, apply bool) (bool,
 		}
 	}
 
-	// we're using `contains` so as to get the candidates, as `exact` might not
-	// give the expected results depending on how the user specified it.
+	// List existing records
 	listParams := dns.RecordListParams{
 		ZoneID: cloudflare.F(obj.zoneID),
 		Name: cloudflare.F(dns.RecordListParamsName{
-			Contains: cloudflare.F(obj.RecordName),
+			Exact: cloudflare.F(obj.RecordName), // this matches the exact name
 		}),
 		Type: cloudflare.F(dns.RecordListParamsType(obj.Type)),
 	}
@@ -239,15 +204,10 @@ func (obj *CloudflareDNSRes) CheckApply(ctx context.Context, apply bool) (bool,
 		return false, errwrap.Wrapf(err, "failed to list DNS records")
 	}
 
-	// here we filter to find the exact match
-	recordExists := false
+	recordExists := len(recordList.Result) > 0
 	var record dns.RecordResponse
-	for _, r := range recordList.Result {
-		if obj.matchesRecordName(r.Name) {
-			record = r
-			recordExists = true
-			break
-		}
+	if recordExists {
+		record = recordList.Result[0]
 	}
 
 	switch obj.State {
@@ -296,8 +256,6 @@ func (obj *CloudflareDNSRes) CheckApply(ctx context.Context, apply bool) (bool,
 	return true, nil
 }
 
-// Cmp compares two resources and returns an error if they differ. This is used
-// to determine if two resources are equivalent for graph operations.
 func (obj *CloudflareDNSRes) Cmp(r engine.Res) error {
 	if obj == nil && r == nil {
 		return nil
@@ -316,11 +274,8 @@ func (obj *CloudflareDNSRes) Cmp(r engine.Res) error {
 		return fmt.Errorf("apitoken differs")
 	}
 
-	if (obj.Proxied == nil) != (res.Proxied == nil) {
-		return fmt.Errorf("proxied values differ")
-	}
-
-	if obj.Proxied != nil && *obj.Proxied != *res.Proxied {
+	// check how this being a pointer influences this check
+	if obj.Proxied != res.Proxied {
 		return fmt.Errorf("proxied values differ")
 	}
 
@@ -356,20 +311,14 @@ func (obj *CloudflareDNSRes) Cmp(r engine.Res) error {
 		return fmt.Errorf("content param differs")
 	}
 
-	if (obj.Priority == nil) != (res.Priority == nil) {
-		return fmt.Errorf("the priority param differs")
-	}
-
-	if obj.Priority != nil && *obj.Priority != *res.Priority {
+	// check how this being a pointer influences this check
+	if obj.Priority != res.Priority {
 		return fmt.Errorf("the priority param differs")
 	}
 
 	return nil
 }
 
-// buildRecordParam creates the appropriate record parameter structure based on
-// the record type. This is a helper function used by buildNewRecordParam and
-// buildEditRecordParam.
 // TODO: double check the fields for each record, might have missed some
 func (obj *CloudflareDNSRes) buildRecordParam() (any, error) {
 	ttl := dns.TTL(obj.TTL)
@@ -431,7 +380,7 @@ func (obj *CloudflareDNSRes) buildRecordParam() (any, error) {
 			param.Proxied = cloudflare.F(*obj.Proxied)
 		}
 		if obj.Priority != nil { // required for MX record
-			param.Priority = cloudflare.F(*obj.Priority)
+			param.Priority = cloudflare.F(float64(*obj.Priority))
 		}
 		if obj.Comment != "" {
 			param.Comment = cloudflare.F(obj.Comment)
@@ -468,6 +417,20 @@ func (obj *CloudflareDNSRes) buildRecordParam() (any, error) {
 		}
 		return param, nil
 
+	case "SRV":
+		param := dns.SRVRecordParam{
+			Name: cloudflare.F(obj.RecordName),
+			Type: cloudflare.F(dns.SRVRecordTypeSRV),
+			TTL:  cloudflare.F(ttl),
+		}
+		if obj.Proxied != nil {
+			param.Proxied = cloudflare.F(*obj.Proxied)
+		}
+		if obj.Comment != "" {
+			param.Comment = cloudflare.F(obj.Comment)
+		}
+		return param, nil
+
 	case "PTR":
 		param := dns.PTRRecordParam{
 			Name:    cloudflare.F(obj.RecordName),
@@ -489,7 +452,7 @@ func (obj *CloudflareDNSRes) buildRecordParam() (any, error) {
 	}
 }
 
-// buildNewRecordParam creates the appropriate record parameter for creating new
+// buildNewRecordParam creates the appropriate record parameter for creating
 // records.
 func (obj *CloudflareDNSRes) buildNewRecordParam() (dns.RecordNewParamsBodyUnion, error) {
 	result, err := obj.buildRecordParam()
@@ -509,8 +472,6 @@ func (obj *CloudflareDNSRes) buildEditRecordParam() (dns.RecordEditParamsBodyUni
 	return result.(dns.RecordEditParamsBodyUnion), nil
 }
 
-// createRecord creates a new DNS record in Cloudflare using the resource's
-// parameters.
 func (obj *CloudflareDNSRes) createRecord(ctx context.Context) error {
 	recordParams, err := obj.buildNewRecordParam()
 	if err != nil {
@@ -530,8 +491,6 @@ func (obj *CloudflareDNSRes) createRecord(ctx context.Context) error {
 	return nil
 }
 
-// updateRecord updates an existing DNS record in Cloudflare with the resource's
-// parameters.
 func (obj *CloudflareDNSRes) updateRecord(ctx context.Context, recordID string) error {
 	recordParams, err := obj.buildEditRecordParam()
 	if err != nil {
@@ -551,8 +510,6 @@ func (obj *CloudflareDNSRes) updateRecord(ctx context.Context, recordID string)
 	return nil
 }
 
-// needsUpdate compares the current DNS record with the desired state and
-// returns true if an update is needed.
 func (obj *CloudflareDNSRes) needsUpdate(record dns.RecordResponse) bool {
 	if obj.Content != record.Content {
 		return true
@@ -569,25 +526,21 @@ func (obj *CloudflareDNSRes) needsUpdate(record dns.RecordResponse) bool {
 	}
 
 	if obj.Priority != nil {
-		if *obj.Priority != record.Priority {
+		if float64(*obj.Priority) != record.Priority {
 			return true
 		}
 	}
 
-	if obj.Comment != "" && obj.Comment != record.Comment {
+	if obj.Comment != record.Comment {
 		return true
 	}
 
-	//TODO: add more checks?
+	// TODO add more checks?
 
 	return false
 
 }
 
-// purgeCheckApply deletes all DNS records in the zone that are not defined in
-// the mgmt graph. It queries the graph for other cloudflare:dns resources in
-// the same zone and builds an exclusion list. If apply is false, it only checks
-// if purge is needed.
 func (obj *CloudflareDNSRes) purgeCheckApply(ctx context.Context, apply bool) (bool, error) {
 	listParams := dns.RecordListParams{
 		ZoneID: cloudflare.F(obj.zoneID),
@@ -628,12 +581,7 @@ func (obj *CloudflareDNSRes) purgeCheckApply(ctx context.Context, apply bool) (b
 		}
 
 		if cfRes.Zone == obj.Zone {
-			recordKey := fmt.Sprintf("%s:%s:%s", cfRes.RecordName, cfRes.Type,
-				cfRes.Content)
-			if cfRes.Priority != nil {
-				// corner case for MX records which require priority set
-				recordKey = fmt.Sprintf("%s:%g", recordKey, *cfRes.Priority)
-			}
+			recordKey := fmt.Sprintf("%s:%s", cfRes.RecordName, cfRes.Type)
 			excludes[recordKey] = true
 		}
 	}
@@ -641,11 +589,7 @@ func (obj *CloudflareDNSRes) purgeCheckApply(ctx context.Context, apply bool) (b
 	checkOK := true
 
 	for _, record := range allRecords {
-		recordKey := fmt.Sprintf("%s:%s:%s", record.Name, record.Type,
-			record.Content)
-		if record.Priority != 0 {
-			recordKey = fmt.Sprintf("%s:%g", recordKey, record.Priority)
-		}
+		recordKey := fmt.Sprintf("%s:%s", record.Name, record.Type)
 
 		if excludes[recordKey] {
 			continue
@@ -678,27 +622,3 @@ func (obj *CloudflareDNSRes) GraphQueryAllowed(opts ...engine.GraphQueryableOpti
 	}
 	return nil
 }
-
-// matchesRecordName checks if a record name from the API matches our desired
-// record name. Handles both FQDN (www.example.com) and short form (www)
-// comparisons.
-func (obj *CloudflareDNSRes) matchesRecordName(apiRecordName string) bool {
-	desired := obj.normalizeRecordName(obj.RecordName)
-	actual := obj.normalizeRecordName(apiRecordName)
-	return desired == actual
-}
-
-// normalizeRecordName converts a record name to a consistent format for
-// comparison. Converts to FQDN format (e.g., "www" -> "www.example.com", "@" ->
-// "example.com")
-func (obj *CloudflareDNSRes) normalizeRecordName(name string) string {
-	if name == "@" || name == obj.Zone {
-		return obj.Zone
-	}
-
-	if strings.HasSuffix(name, "."+obj.Zone) {
-		return name
-	}
-
-	return name + "." + obj.Zone
-}

engine/resources/virt_builder.go

@@ -36,7 +36,6 @@ import (
 	"net/url"
 	"os"
 	"os/exec"
-	"os/user"
 	"path"
 	"path/filepath"
 	"runtime"
@@ -622,15 +621,6 @@ func (obj *VirtBuilderRes) CheckApply(ctx context.Context, apply bool) (bool, er
 
 	cmd := exec.CommandContext(ctx, cmdName, cmdArgs...)
 
-	usr, err := user.Current()
-	if err != nil {
-		return false, err
-	}
-	// FIXME: consider building this from an empty environment?
-	cmd.Env = append(os.Environ(),
-		fmt.Sprintf("HOME=%s", usr.HomeDir),
-	)
-
 	// ignore signals sent to parent process (we're in our own group)
 	cmd.SysProcAttr = &syscall.SysProcAttr{
 		Setpgid: true,
@@ -648,7 +638,7 @@ func (obj *VirtBuilderRes) CheckApply(ctx context.Context, apply bool) (bool, er
 		return false, errwrap.Wrapf(err, "error starting cmd")
 	}
 
-	cmderr := cmd.Wait() // we can unblock this with the timeout
+	err := cmd.Wait() // we can unblock this with the timeout
 	out := b.String()
 
 	p := path.Join(obj.varDir, fmt.Sprintf("%d.log", start))
@@ -658,7 +648,7 @@ func (obj *VirtBuilderRes) CheckApply(ctx context.Context, apply bool) (bool, er
 		}
 	}
 
-	if cmderr == nil {
+	if err == nil {
 		obj.init.Logf("built image successfully!")
 		return false, nil // success!
 	}
@@ -677,7 +667,7 @@ func (obj *VirtBuilderRes) CheckApply(ctx context.Context, apply bool) (bool, er
 		obj.init.Logf("deleted partial output")
 	}
 
-	exitErr, ok := cmderr.(*exec.ExitError) // embeds an os.ProcessState
+	exitErr, ok := err.(*exec.ExitError) // embeds an os.ProcessState
 	if !ok {
 		// command failed in some bad way
 		return false, errwrap.Wrapf(err, "cmd failed in some bad way")