Skip to content

Feature: Added RRSet comments support #87

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 66 additions & 24 deletions powerdns/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ import (
"strconv"
"strings"

freecache "github.com/coocood/freecache"
cleanhttp "github.com/hashicorp/go-cleanhttp"
"github.com/coocood/freecache"
"github.com/hashicorp/go-cleanhttp"
)

// DefaultSchema is the value used for the URL in case
Expand Down Expand Up @@ -197,11 +197,17 @@ type Record struct {

// ResourceRecordSet represents a PowerDNS RRSet object
type ResourceRecordSet struct {
Name string `json:"name"`
Type string `json:"type"`
ChangeType string `json:"changetype"`
TTL int `json:"ttl"` // For API v1
Records []Record `json:"records,omitempty"`
Name string `json:"name"`
Type string `json:"type"`
ChangeType string `json:"changetype"`
TTL int `json:"ttl"` // For API v1
Records []Record `json:"records,omitempty"`
Comments []Comment `json:"comments,omitempty"`
}

type Comment struct {
Content string `json:"content,omitempty"`
Account string `json:"account,omitempty"`
}

type zonePatchRequest struct {
Expand Down Expand Up @@ -460,11 +466,31 @@ func (client *Client) GetZoneInfoFromCache(zone string) (*ZoneInfo, error) {

// ListRecords returns all records in Zone
func (client *Client) ListRecords(zone string) ([]Record, error) {
rrsets, err := client.ListRRSets(zone)
if err != nil {
return nil, err
}

var records []Record
for _, rrs := range rrsets {
for _, record := range rrs.Records {
records = append(records, Record{
Name: rrs.Name,
Type: rrs.Type,
Content: record.Content,
TTL: rrs.TTL,
})
}
}

return records, nil
}

func (client *Client) getZoneInfo(zone string) (*ZoneInfo, error) {
zoneInfo, err := client.GetZoneInfoFromCache(zone)
if err != nil {
log.Printf("[WARN] module.freecache: %s: %s", zone, err)
}

if zoneInfo == nil {
req, err := client.newRequest("GET", fmt.Sprintf("/servers/localhost/zones/%s", zone), nil)
if err != nil {
Expand All @@ -491,26 +517,13 @@ func (client *Client) ListRecords(zone string) ([]Record, error) {

err = client.Cache.Set([]byte(zone), cacheValue, client.CacheTTL)
if err != nil {
return nil, fmt.Errorf("The cache for REST API requests is enabled but the size isn't enough: cacheSize: %db \n %s",
DefaultCacheSize, err)
fmt.Printf("[WARN] The cache for REST API requests is enabled but"+
Copy link
Author

Choose a reason for hiding this comment

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

@mbag
Have a look at this change. I think it's better to through not an error but a warning message here.
If it needed, I can create it as a separate MR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@PetrusHahol I think it should return error (for example JSON marshaling above returns error), because user enables caching and expects objects to be cached. But if there is error and they are not cached, however, the execution continues with only warning. This might not be something they want to pass.

Also, I'm wondering now if the error described is only possible error, i.e. if it wouldn't be better to just do return nil, err as in other error checks. Because, if in fact, the client.Cache.Set fails for some other reason, it might get obscured by our custom error message. But this was my error, that I haven't spotted before merging.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, I forgot to ask. Why do you think it's better to just log warning, and continue with the execution?

Copy link
Author

@PetrusHahol PetrusHahol Jun 18, 2021

Choose a reason for hiding this comment

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

Hmm.. Actually I also think it may lead to some different error. I will change it.
I have just totally otherwise picture of it. In case users use cache and something went wrong with it, we can still continue execution and in a meanwhile let them know that something crashed. User can immediately stop execution. But maybe you're right, it might lead to DDoS of PowerDNS API, like if it would be without caches enabled.

Choose a reason for hiding this comment

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

Hello guys, any update with the "comment" paramenter ?

" the size isn't enough: cacheSize: %db \n %s", DefaultCacheSize, err)
}
}
}

records := zoneInfo.Records
// Convert the API v1 response to v0 record structure
for _, rrs := range zoneInfo.ResourceRecordSets {
for _, record := range rrs.Records {
records = append(records, Record{
Name: rrs.Name,
Type: rrs.Type,
Content: record.Content,
TTL: rrs.TTL,
})
}
}

return records, nil
return zoneInfo, nil
}

// ListRecordsInRRSet returns only records of specified name and type
Expand Down Expand Up @@ -539,6 +552,35 @@ func (client *Client) ListRecordsByID(zone string, recID string) ([]Record, erro
return client.ListRecordsInRRSet(zone, name, tpe)
}

//GetResourceRecordSet by zone and id
func (client *Client) GetRRSetOfRecord(zone string, recID string) (*ResourceRecordSet, error) {
name, tpe, err := parseID(recID)
if err != nil {
return nil, err
}

rrsets, err := client.ListRRSets(zone)
if err != nil {
return nil, err
}

for _, rrset := range rrsets {
if rrset.Name == name && rrset.Type == tpe {
return &rrset, nil
}
}
return nil, fmt.Errorf("Error getting rrset %s. Not found.", name)
}

//ListRRSets by zone
func (client *Client) ListRRSets(zone string) ([]ResourceRecordSet, error) {
zoneInfo, err := client.getZoneInfo(zone)
if err != nil {
return nil, err
}
return zoneInfo.ResourceRecordSets, nil
}

// RecordExists checks if requested record exists in Zone
func (client *Client) RecordExists(zone string, name string, tpe string) (bool, error) {
allRecords, err := client.ListRecords(zone)
Expand Down
47 changes: 47 additions & 0 deletions powerdns/resource_powerdns_record.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,27 @@ func resourcePDNSRecord() *schema.Resource {
ForceNew: true,
Description: "For A and AAAA records, if true, create corresponding PTR.",
},

"comment": {
Type: schema.TypeSet,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"content": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
},
"account": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
},
},
},
Optional: true,
ForceNew: true,
Description: "A comment about an RRSet.",
},
},
}
}
Expand All @@ -70,6 +91,21 @@ func resourcePDNSRecordCreate(d *schema.ResourceData, meta interface{}) error {
TTL: d.Get("ttl").(int),
}

comments := d.Get("comment").(*schema.Set).List()
if len(comments) > 0 {
commentObjs := make([]Comment, 0, len(comments))
for _, comment := range comments {
commentMap := comment.(map[string]interface{})
commentObjs = append(
commentObjs,
Comment{
Content: commentMap["content"].(string),
Account: commentMap["account"].(string),
})
}
rrSet.Comments = commentObjs
}

zone := d.Get("zone").(string)
ttl := d.Get("ttl").(int)
recs := d.Get("records").(*schema.Set).List()
Expand Down Expand Up @@ -195,6 +231,8 @@ func resourcePDNSRecordImport(d *schema.ResourceData, meta interface{}) ([]*sche
log.Printf("[INFO] importing PowerDNS Record %s in Zone: %s", recordID, zoneName)

records, err := client.ListRecordsByID(zoneName, recordID)
rrset, err := client.GetRRSetOfRecord(zoneName, recordID)

if err != nil {
return nil, fmt.Errorf("couldn't fetch PowerDNS Record: %s", err)
}
Expand All @@ -208,11 +246,20 @@ func resourcePDNSRecordImport(d *schema.ResourceData, meta interface{}) ([]*sche
recs = append(recs, r.Content)
}

commentsSet := make([]map[string]interface{}, len(rrset.Comments))
for i, comment := range rrset.Comments {
commentsSet[i] = map[string]interface{}{
"content": comment.Content,
"account": comment.Account,
}
}

d.Set("zone", zoneName)
d.Set("name", records[0].Name)
d.Set("ttl", records[0].TTL)
d.Set("type", records[0].Type)
d.Set("records", recs)
d.Set("comment", commentsSet)
d.SetId(recordID)

return []*schema.ResourceData{d}, nil
Expand Down
44 changes: 44 additions & 0 deletions powerdns/resource_powerdns_record_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,31 @@ func TestAccPDNSRecord_TXT(t *testing.T) {
})
}

func TestAccPDNSRecord_WithComments(t *testing.T) {
resourceName := "powerdns_record.test-comments"
resourceID := `{"zone":"sysa.xyz.","id":"comment.sysa.xyz.:::A"}`

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckPDNSRecordDestroy,
Steps: []resource.TestStep{
{
Config: testPDSNRecordWithComments,
Check: resource.ComposeTestCheckFunc(
testAccCheckPDNSRecordExists(resourceName),
),
},
{
ResourceName: resourceName,
ImportStateId: resourceID,
ImportState: true,
ImportStateVerify: true,
},
},
})
}

func TestAccPDNSRecord_ALIAS(t *testing.T) {
resourceName := "powerdns_record.test-alias"
resourceID := `{"zone":"sysa.xyz.","id":"alias.sysa.xyz.:::ALIAS"}`
Expand Down Expand Up @@ -654,3 +679,22 @@ resource "powerdns_record" "test-soa" {
ttl = 3600
records = [ "something.something. hostmaster.sysa.xyz. 2019090301 10800 3600 604800 3600" ]
}`

const testPDSNRecordWithComments = `
resource "powerdns_record" "test-comments" {
zone = "sysa.xyz."
name = "comment.sysa.xyz."
type = "A"
ttl = 60
records = [ "1.1.1.1" ]

comment {
content = "Test comment #1"
account = "Test account #1"
}

comment {
content = "Test comment #2"
account = "Test account #2"
}
}`
27 changes: 27 additions & 0 deletions website/docs/r/record.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,30 @@ resource "powerdns_record" "foobar" {
}
```

### Record with comments
An example creating a record with comments:

```hcl
# Add a record with comments
resource "powerdns_record" "foobar" {
zone = "example.com."
name = "www.example.com."
type = "A"
ttl = 60
records = [ "1.1.1.1" ]

comment {
content = "Example comment #1"
account = "Example account #1"
}

comment {
content = "Example comment #2"
account = "Example account #2"
}
}
```

### MX record example
The following example shows, how to setup MX record with a priority of `10`.
Please note that priority is not set as other `powerdns_record` properties; rather, it's part of the string that goes into `records` list.
Expand Down Expand Up @@ -145,6 +169,9 @@ The following arguments are supported:
* `ttl` - (Required) The TTL of the record.
* `records` - (Required) A string list of records.
* `set_ptr` - (Optional) [**_Deprecated in PowerDNS 4.3.0_**] A boolean (true/false), determining whether API server should automatically create PTR record in the matching reverse zone. Existing PTR records are replaced. If no matching reverse zone, an error is thrown.
* `comment` - (Optional) A comment about an RRSet.
* `content` - (Required) The content of the comment.
* `account` - (Required) The account of the comment.

### Attribute Reference

Expand Down