From 5f8397655452dbfbcc824b086ca7130bd9148ac2 Mon Sep 17 00:00:00 2001 From: Omar Jatoi Date: Sat, 18 May 2024 14:23:23 -0400 Subject: [PATCH 1/4] Add package tests --- provider_test.go | 265 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 264 insertions(+), 1 deletion(-) diff --git a/provider_test.go b/provider_test.go index 3ced87d..3563d07 100644 --- a/provider_test.go +++ b/provider_test.go @@ -1 +1,264 @@ -package dnsimple +package dnsimple_test + +import ( + "context" + "os" + "testing" + "time" + + "github.com/libdns/dnsimple" + "github.com/libdns/libdns" +) + +var ( + apiAccessToken = "" + zone = "" + apiUrl = "https://api.sandbox.dnsimple.com" + ttl = time.Duration(1 * time.Hour) +) + +type testRecordsCleanup = func() + +func TestMain(m *testing.M) { + apiAccessToken = os.Getenv("TEST_API_ACCESS_TOKEN") + zone = os.Getenv("TEST_ZONE") + + if len(apiAccessToken) == 0 || len(zone) == 0 { + panic("API Access Token, Zone, and Account ID must be set using environment variables") + } + + os.Exit(m.Run()) +} + +func setupTestRecords(t *testing.T, ctx context.Context, p *dnsimple.Provider) ([]libdns.Record, testRecordsCleanup) { + testRecords := []libdns.Record{ + { + Type: "TXT", + Name: "test1", + Value: "test1", + TTL: ttl, + }, { + Type: "TXT", + Name: "test2", + Value: "test2", + TTL: ttl, + }, { + Type: "TXT", + Name: "test3", + Value: "test3", + TTL: ttl, + }, + } + records, err := p.AppendRecords(context.Background(), zone, testRecords) + if err != nil { + t.Fatal(err) + return nil, func() {} + } + + return records, func() { + cleanupRecords(t, ctx, p, records) + } +} + +func cleanupRecords(t *testing.T, ctx context.Context, p *dnsimple.Provider, r []libdns.Record) { + _, err := p.DeleteRecords(ctx, zone, r) + if err != nil { + t.Fatalf("cleanup failed: %v", err) + } +} + +func Test_AppendRecords(t *testing.T) { + p := &dnsimple.Provider{ + APIAccessToken: apiAccessToken, + APIURL: apiUrl, + } + ctx := context.Background() + + testCases := []struct { + records []libdns.Record + expected []libdns.Record + }{ + { + records: []libdns.Record{ + {Type: "TXT", Name: "test_1", Value: "test_1", TTL: ttl}, + {Type: "TXT", Name: "test_2", Value: "test_2", TTL: ttl}, + {Type: "TXT", Name: "test_3", Value: "test_3", TTL: ttl}, + }, + expected: []libdns.Record{ + {Type: "TXT", Name: "test_1", Value: "test_1", TTL: ttl}, + {Type: "TXT", Name: "test_2", Value: "test_2", TTL: ttl}, + {Type: "TXT", Name: "test_3", Value: "test_3", TTL: ttl}, + }, + }, + { + records: []libdns.Record{ + {Type: "TXT", Name: "123.test", Value: "123", TTL: ttl}, + }, + expected: []libdns.Record{ + {Type: "TXT", Name: "123.test", Value: "123", TTL: ttl}, + }, + }, + { + records: []libdns.Record{ + {Type: "TXT", Name: "123.test", Value: "test", TTL: ttl}, + }, + expected: []libdns.Record{ + {Type: "TXT", Name: "123.test", Value: "test", TTL: ttl}, + }, + }, + { + records: []libdns.Record{ + {Type: "TXT", Name: "abc.test", Value: "test", TTL: ttl}, + }, + expected: []libdns.Record{ + {Type: "TXT", Name: "abc.test", Value: "test", TTL: ttl}, + }, + }, + } + + for _, c := range testCases { + func() { + result, err := p.AppendRecords(ctx, zone+".", c.records) + if err != nil { + t.Fatal(err) + } + defer cleanupRecords(t, ctx, p, result) + + if len(result) != len(c.records) { + t.Fatalf("len(resilt) != len(c.records) => %d != %d", len(c.records), len(result)) + } + + for k, r := range result { + if len(result[k].ID) == 0 { + t.Fatalf("len(result[%d].ID) == 0", k) + } + if r.Type != c.expected[k].Type { + t.Fatalf("r.Type != c.exptected[%d].Type => %s != %s", k, r.Type, c.expected[k].Type) + } + if r.Name != c.expected[k].Name { + t.Fatalf("r.Name != c.exptected[%d].Name => %s != %s", k, r.Name, c.expected[k].Name) + } + if r.Value != c.expected[k].Value { + t.Fatalf("r.Value != c.exptected[%d].Value => %s != %s", k, r.Value, c.expected[k].Value) + } + if r.TTL != c.expected[k].TTL { + t.Fatalf("r.TTL != c.exptected[%d].TTL => %s != %s", k, r.TTL, c.expected[k].TTL) + } + } + }() + } +} + +func Test_DeleteRecords(t *testing.T) { + p := &dnsimple.Provider{ + APIAccessToken: apiAccessToken, + APIURL: apiUrl, + } + ctx := context.Background() + + testRecords, cleanupFunc := setupTestRecords(t, ctx, p) + defer cleanupFunc() + + records, err := p.GetRecords(ctx, zone) + if err != nil { + t.Fatal(err) + } + + if len(records) < len(testRecords) { + t.Fatalf("len(records) < len(testRecords) => %d < %d", len(records), len(testRecords)) + } + + for _, testRecord := range testRecords { + var foundRecord *libdns.Record + for _, record := range records { + if testRecord.ID == record.ID { + foundRecord = &testRecord + } + } + + if foundRecord == nil { + t.Fatalf("Record not found => %s", testRecord.ID) + } + } +} + +func Test_GetRecords(t *testing.T) { + p := &dnsimple.Provider{ + APIAccessToken: apiAccessToken, + APIURL: apiUrl, + } + ctx := context.Background() + + testRecords, cleanupFunc := setupTestRecords(t, ctx, p) + defer cleanupFunc() + + records, err := p.GetRecords(ctx, zone) + if err != nil { + t.Fatal(err) + } + + if len(records) < len(testRecords) { + t.Fatalf("len(records) < len(testRecords) => %d < %d", len(records), len(testRecords)) + } + + for _, testRecord := range testRecords { + var foundRecord *libdns.Record + for _, record := range records { + if testRecord.ID == record.ID { + foundRecord = &testRecord + } + } + + if foundRecord == nil { + t.Fatalf("Record not found => %s", testRecord.ID) + } + } +} + +func Test_SetRecords(t *testing.T) { + p := &dnsimple.Provider{ + APIAccessToken: apiAccessToken, + APIURL: apiUrl, + } + ctx := context.Background() + + existingRecords, _ := setupTestRecords(t, ctx, p) + + newTestRecords := []libdns.Record{ + { + Type: "TXT", + Name: "new_test1", + Value: "new_test1", + TTL: ttl, + }, + { + Type: "TXT", + Name: "new_test2", + Value: "new_test2", + TTL: ttl, + }, + } + + allRecords := append(existingRecords, newTestRecords...) + allRecords[0].Value = "new_value" + + records, err := p.SetRecords(ctx, zone, allRecords) + if err != nil { + t.Fatal(err) + } + defer cleanupRecords(t, ctx, p, records) + + if len(records) != len(allRecords) { + t.Fatalf("len(records) != len(allRecords) => %d != %d", len(records), len(allRecords)) + } + + updated := false + for _, r := range records { + if r.Value == "new_value" { + updated = true + } + } + if !updated { + t.Fatalf("Did not update value on existing record") + } +} From b3ba66098d89384a0bccc4a9adb79074488f4cfc Mon Sep 17 00:00:00 2001 From: Omar Jatoi Date: Sat, 18 May 2024 14:23:43 -0400 Subject: [PATCH 2/4] Update README to include instructions on testing --- README.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/README.md b/README.md index ea06a18..b550085 100644 --- a/README.md +++ b/README.md @@ -17,6 +17,24 @@ This provider expects the following configuration: - `ACCOUNT_ID`: identifier for the account (only needed if using a user access token), see [accounts documentation](https://developer.dnsimple.com/v2/accounts/) - `API_URL`: hostname for the API to use (defaults to `api.dnsimple.com`), only useful for testing purposes, see [sandox documentation](https://developer.dnsimple.com/sandbox/) +## Testing + +In order to run the tests, you need to create an account on the [DNSimple sandbox environment](https://developer.dnsimple.com/sandbox/). After setup, create a new DNS zone, and create an `API_ACCESS_TOKEN` and take note of both. You will need both these values to run tests. + +``` +$ TEST_ZONE=example.com TEST_API_ACCESS_TOKEN=you_api_access_token go test -v +=== RUN Test_AppendRecords +--- PASS: Test_AppendRecords (1.23s) +=== RUN Test_DeleteRecords +--- PASS: Test_DeleteRecords (0.59s) +=== RUN Test_GetRecords +--- PASS: Test_GetRecords (0.58s) +=== RUN Test_SetRecords +--- PASS: Test_SetRecords (1.14s) +PASS +ok github.com/libdns/dnsimple 3.666s +``` + ## License Licensed under the Apache License, Version 2.0 (the "License"); From 3f2becf954420aeb827e4a8c08c18dc92028d3f1 Mon Sep 17 00:00:00 2001 From: Omar Jatoi Date: Sat, 18 May 2024 14:27:49 -0400 Subject: [PATCH 3/4] Several bug fixes - calling another method while caller holds lock on mutex would hang, issue in SetRecords, fixed by creating some helper methods and only using locks in the exported functions only - issue with the TTL, this is going to be returned in seconds by the provider API - bug where SetRecords was incorrectly keeping track of records to update and create due to incorrect usage of array slicing while ranging over the array --- provider.go | 91 +++++++++++++++++++++++++++++++++++------------------ 1 file changed, 60 insertions(+), 31 deletions(-) diff --git a/provider.go b/provider.go index ae19661..a17d396 100644 --- a/provider.go +++ b/provider.go @@ -30,7 +30,7 @@ func (p *Provider) initClient(ctx context.Context) { p.once.Do(func() { // Create new DNSimple client using the provided access token. tc := dnsimple.StaticTokenHTTPClient(ctx, p.APIAccessToken) - c := *dnsimple.NewClient(tc) + c := dnsimple.NewClient(tc) // Set the API URL if using a non-default API hostname (e.g. sandbox). if p.APIURL != "" { c.BaseURL = p.APIURL @@ -43,16 +43,13 @@ func (p *Provider) initClient(ctx context.Context) { p.AccountID = accountID } - p.client = c + p.client = *c }) } -// GetRecords lists all the records in the zone. -func (p *Provider) GetRecords(ctx context.Context, zone string) ([]libdns.Record, error) { - p.mutex.Lock() - defer p.mutex.Unlock() - p.initClient(ctx) - +// Internal helper function to fetch records from the provider, note that this function assumes +// the called is holding a lock on the mutex and has already initialized the client. +func (p *Provider) getRecordsFromProvider(ctx context.Context, zone string) ([]libdns.Record, error) { var records []libdns.Record resp, err := p.client.Zones.ListRecords(ctx, p.AccountID, unFQDN(zone), &dnsimple.ZoneRecordListOptions{}) @@ -65,7 +62,7 @@ func (p *Provider) GetRecords(ctx context.Context, zone string) ([]libdns.Record Type: r.Type, Name: r.Name, Value: r.Content, - TTL: time.Duration(r.TTL), + TTL: time.Duration(r.TTL * int(time.Second)), Priority: uint(r.Priority), } records = append(records, record) @@ -74,13 +71,19 @@ func (p *Provider) GetRecords(ctx context.Context, zone string) ([]libdns.Record return records, nil } -// AppendRecords adds records to the zone. It returns the records that were added. -func (p *Provider) AppendRecords(ctx context.Context, zone string, records []libdns.Record) ([]libdns.Record, error) { +// GetRecords lists all the records in the zone. +func (p *Provider) GetRecords(ctx context.Context, zone string) ([]libdns.Record, error) { p.mutex.Lock() defer p.mutex.Unlock() p.initClient(ctx) - var appendedRecords []libdns.Record + return p.getRecordsFromProvider(ctx, zone) +} + +// Internal helper function that actually creates the records, does not hold a lock since the called is +// assumed to be holding a lock on the mutex and is in charge of making sure the client is initialized. +func (p *Provider) createRecords(ctx context.Context, zone string, records []libdns.Record) ([]libdns.Record, error) { + var createdRecords []libdns.Record // Get the Zone ID from zone name resp, err := p.client.Zones.GetZone(ctx, p.AccountID, unFQDN(zone)) @@ -95,7 +98,7 @@ func (p *Provider) AppendRecords(ctx context.Context, zone string, records []lib Type: r.Type, Name: &r.Name, Content: r.Value, - TTL: int(r.TTL), + TTL: int(r.TTL.Seconds()), Priority: int(r.Priority), } resp, err := p.client.Zones.CreateRecord(ctx, p.AccountID, unFQDN(zone), attrs) @@ -105,31 +108,37 @@ func (p *Provider) AppendRecords(ctx context.Context, zone string, records []lib // See https://developer.dnsimple.com/v2/zones/records/#createZoneRecord if resp.HTTPResponse.StatusCode == http.StatusCreated { r.ID = strconv.FormatInt(resp.Data.ID, 10) - appendedRecords = append(appendedRecords, r) + createdRecords = append(createdRecords, r) } else { return nil, fmt.Errorf("error creating record: %s, error: %s", r.Name, resp.HTTPResponse.Status) } } - return appendedRecords, nil + return createdRecords, nil } -// SetRecords sets the records in the zone, either by updating existing records or creating new ones. -// It returns the updated records. -func (p *Provider) SetRecords(ctx context.Context, zone string, records []libdns.Record) ([]libdns.Record, error) { +// AppendRecords adds records to the zone. It returns the records that were added. +func (p *Provider) AppendRecords(ctx context.Context, zone string, records []libdns.Record) ([]libdns.Record, error) { p.mutex.Lock() defer p.mutex.Unlock() p.initClient(ctx) - var setRecords []libdns.Record + return p.createRecords(ctx, zone, records) +} - existingRecords, err := p.GetRecords(ctx, unFQDN(zone)) +// Internal helper function to get the lists of records to create and update respectively +func (p *Provider) getRecordsToCreateAndUpdate(ctx context.Context, zone string, records []libdns.Record) ([]libdns.Record, []libdns.Record, error) { + existingRecords, err := p.getRecordsFromProvider(ctx, zone) if err != nil { - return nil, err + return nil, nil, err } var recordsToUpdate []libdns.Record + updateMap := make(map[libdns.Record]bool) + var recordsToCreate []libdns.Record + // Figure out which records exist and need to be updated - for i, r := range records { + for _, r := range records { + updateMap[r] = true for _, er := range existingRecords { if r.Name != er.Name { continue @@ -138,15 +147,35 @@ func (p *Provider) SetRecords(ctx context.Context, zone string, records []libdns r.ID = er.ID } recordsToUpdate = append(recordsToUpdate, r) - // If this is a record that exists and will be updated, remove it from - // the records slice, so everything left will be a record that does not - // exist and needs to be created. - records = append(records[:i], records[i+1:]...) + updateMap[r] = false } } + // If the record is not updating an existing record, we want to create it + for r, updating := range updateMap { + if updating { + recordsToCreate = append(recordsToCreate, r) + } + } + + return recordsToCreate, recordsToUpdate, nil +} + +// SetRecords sets the records in the zone, either by updating existing records or creating new ones. +// It returns the updated records. +func (p *Provider) SetRecords(ctx context.Context, zone string, records []libdns.Record) ([]libdns.Record, error) { + p.mutex.Lock() + defer p.mutex.Unlock() + p.initClient(ctx) + + var setRecords []libdns.Record + + recordsToCreate, recordsToUpdate, err := p.getRecordsToCreateAndUpdate(ctx, zone, records) + if err != nil { + return nil, err + } // Create new records and append them to 'setRecords' - createdRecords, err := p.AppendRecords(ctx, unFQDN(zone), records) + createdRecords, err := p.createRecords(ctx, zone, recordsToCreate) if err != nil { return nil, err } @@ -168,7 +197,7 @@ func (p *Provider) SetRecords(ctx context.Context, zone string, records []libdns Type: r.Type, Name: &r.Name, Content: r.Value, - TTL: int(r.TTL), + TTL: int(r.TTL.Seconds()), Priority: int(r.Priority), } id, err := strconv.ParseInt(r.ID, 10, 64) @@ -232,12 +261,12 @@ func (p *Provider) DeleteRecords(ctx context.Context, zone string, records []lib // If we received records without an ID earlier, we're going to try and figure out the ID by calling // GetRecords and comparing the record name. If we're able to find it, we'll delete it, otherwise // we'll append it to our list of failed to delete records. - fetchedRecords, err := p.GetRecords(ctx, unFQDN(zone)) + existingRecords, err := p.getRecordsFromProvider(ctx, zone) if err != nil { - return nil, fmt.Errorf("failed to fetch records: %s", err.Error()) + return nil, fmt.Errorf("failed to get existing records: %s", err.Error()) } for _, r := range noID { - for _, fr := range fetchedRecords { + for _, fr := range existingRecords { if r.Name != fr.Name { continue } From be9ffd760d98b492b73fa92e89ffd04f027c2c18 Mon Sep 17 00:00:00 2001 From: Omar Jatoi Date: Sat, 18 May 2024 14:30:46 -0400 Subject: [PATCH 4/4] Put `os.Getenv` calls in variable block --- provider_test.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/provider_test.go b/provider_test.go index 3563d07..2c4fa59 100644 --- a/provider_test.go +++ b/provider_test.go @@ -11,8 +11,8 @@ import ( ) var ( - apiAccessToken = "" - zone = "" + apiAccessToken = os.Getenv("TEST_API_ACCESS_TOKEN") + zone = os.Getenv("TEST_ZONE") apiUrl = "https://api.sandbox.dnsimple.com" ttl = time.Duration(1 * time.Hour) ) @@ -20,9 +20,6 @@ var ( type testRecordsCleanup = func() func TestMain(m *testing.M) { - apiAccessToken = os.Getenv("TEST_API_ACCESS_TOKEN") - zone = os.Getenv("TEST_ZONE") - if len(apiAccessToken) == 0 || len(zone) == 0 { panic("API Access Token, Zone, and Account ID must be set using environment variables") }