From fbdce6b84900527ffd4a1fed1bcb9089152cb379 Mon Sep 17 00:00:00 2001 From: Mikkel Oscar Lyderik Larsen Date: Fri, 24 Feb 2017 20:34:14 +0100 Subject: [PATCH 1/5] Safely handle records without ResourceRecords Always check the length of ResourceRecords before trying to index. Fix #87 --- consumers/aws.go | 2 +- consumers/aws_test.go | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/consumers/aws.go b/consumers/aws.go index 65db631..d083f0d 100644 --- a/consumers/aws.go +++ b/consumers/aws.go @@ -265,7 +265,7 @@ func (a *awsConsumer) recordInfo(records []*route53.ResourceRecordSet) map[strin groupIDMap := map[string]string{} //maps dns to group ID for _, record := range records { - if (aws.StringValue(record.Type)) == "TXT" { + if aws.StringValue(record.Type) == "TXT" && len(record.ResourceRecords) > 0 { groupIDMap[aws.StringValue(record.Name)] = aws.StringValue(record.ResourceRecords[0].Value) } else { if _, exist := groupIDMap[aws.StringValue(record.Name)]; !exist { diff --git a/consumers/aws_test.go b/consumers/aws_test.go index 33c58fe..e0bce1b 100644 --- a/consumers/aws_test.go +++ b/consumers/aws_test.go @@ -150,6 +150,11 @@ func TestRecordInfo(t *testing.T) { }, }, }, + &route53.ResourceRecordSet{ + Type: aws.String("TXT"), + Name: aws.String("test.example.com."), + ResourceRecords: nil, + }, } recordInfoMap := client.recordInfo(records) if len(recordInfoMap) != 1 { From 69007a3fe95c15df09ad9dcd4908e8fa625844f7 Mon Sep 17 00:00:00 2001 From: ideahitme Date: Mon, 27 Feb 2017 12:02:23 +0100 Subject: [PATCH 2/5] track the record on bug and log the message --- consumers/aws.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/consumers/aws.go b/consumers/aws.go index d083f0d..9d7cff4 100644 --- a/consumers/aws.go +++ b/consumers/aws.go @@ -265,8 +265,13 @@ func (a *awsConsumer) recordInfo(records []*route53.ResourceRecordSet) map[strin groupIDMap := map[string]string{} //maps dns to group ID for _, record := range records { - if aws.StringValue(record.Type) == "TXT" && len(record.ResourceRecords) > 0 { - groupIDMap[aws.StringValue(record.Name)] = aws.StringValue(record.ResourceRecords[0].Value) + if aws.StringValue(record.Type) == "TXT" { + if len(record.ResourceRecords) > 0 { + groupIDMap[aws.StringValue(record.Name)] = aws.StringValue(record.ResourceRecords[0].Value) + } else { + log.Errorf("Unexpected response from AWS API, got TXT record with empty resources: %s. Record is excluded from syncing", aws.StringValue(record.Name)) + groupIDMap[aws.StringValue(record.Name)] = "" + } } else { if _, exist := groupIDMap[aws.StringValue(record.Name)]; !exist { groupIDMap[aws.StringValue(record.Name)] = "" From 0b71c119932f98ac8277566d74115338f2ff525b Mon Sep 17 00:00:00 2001 From: ideahitme Date: Mon, 27 Feb 2017 12:59:37 +0100 Subject: [PATCH 3/5] fix tests --- consumers/aws_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/consumers/aws_test.go b/consumers/aws_test.go index e0bce1b..fccc97e 100644 --- a/consumers/aws_test.go +++ b/consumers/aws_test.go @@ -152,12 +152,17 @@ func TestRecordInfo(t *testing.T) { }, &route53.ResourceRecordSet{ Type: aws.String("TXT"), - Name: aws.String("test.example.com."), + Name: aws.String("unusual-1.example.com."), ResourceRecords: nil, }, + &route53.ResourceRecordSet{ + Type: aws.String("TXT"), + Name: aws.String("unusual-2.example.com."), + ResourceRecords: []*route53.ResourceRecord{}, + }, } recordInfoMap := client.recordInfo(records) - if len(recordInfoMap) != 1 { + if len(recordInfoMap) != 3 { t.Errorf("Incorrect record info for %v", records) } if val, exist := recordInfoMap["test.example.com."]; !exist { From 6423e94bca14c1c964e5e0172c1ddfdce543ca04 Mon Sep 17 00:00:00 2001 From: ideahitme Date: Mon, 27 Feb 2017 19:24:00 +0100 Subject: [PATCH 4/5] improve test coverage --- consumers/aws.go | 11 ++++-- consumers/aws_test.go | 78 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 3 deletions(-) diff --git a/consumers/aws.go b/consumers/aws.go index 9d7cff4..a96ffce 100644 --- a/consumers/aws.go +++ b/consumers/aws.go @@ -260,8 +260,8 @@ func (a *awsConsumer) getAssignedTXTRecordObject(aliasRecord *route53.ResourceRe } } -//recordInfo returns the map of record assigned dns to its target and groupID (can be empty) -func (a *awsConsumer) recordInfo(records []*route53.ResourceRecordSet) map[string]*pkg.RecordInfo { +//groupIDInfo builds a map from dns name to its group ID +func (a *awsConsumer) groupIDInfo(records []*route53.ResourceRecordSet) map[string]string { groupIDMap := map[string]string{} //maps dns to group ID for _, record := range records { @@ -269,7 +269,7 @@ func (a *awsConsumer) recordInfo(records []*route53.ResourceRecordSet) map[strin if len(record.ResourceRecords) > 0 { groupIDMap[aws.StringValue(record.Name)] = aws.StringValue(record.ResourceRecords[0].Value) } else { - log.Errorf("Unexpected response from AWS API, got TXT record with empty resources: %s. Record is excluded from syncing", aws.StringValue(record.Name)) + log.Errorf("Unexpected response from AWS API, got TXT record with empty resources: %s . Record is excluded from syncing", aws.StringValue(record.Name)) groupIDMap[aws.StringValue(record.Name)] = "" } } else { @@ -278,7 +278,12 @@ func (a *awsConsumer) recordInfo(records []*route53.ResourceRecordSet) map[strin } } } + return groupIDMap +} +//recordInfo returns the map of record assigned dns to its target and groupID (can be empty string) +func (a *awsConsumer) recordInfo(records []*route53.ResourceRecordSet) map[string]*pkg.RecordInfo { + groupIDMap := a.groupIDInfo(records) infoMap := map[string]*pkg.RecordInfo{} //maps record DNS to its GroupID (if exists) and Target (LB) for _, record := range records { groupID := groupIDMap[aws.StringValue(record.Name)] diff --git a/consumers/aws_test.go b/consumers/aws_test.go index fccc97e..937bd76 100644 --- a/consumers/aws_test.go +++ b/consumers/aws_test.go @@ -127,6 +127,84 @@ func sameTargets(lb1, lb2 string) bool { return lb1 == lb2 } +func TestGroupIDInfo(t *testing.T) { + groupID := "test" + client := &awsConsumer{ + groupID: groupID, + } + records := []*route53.ResourceRecordSet{ + &route53.ResourceRecordSet{ + Type: aws.String("A"), + Name: aws.String("test.example.com."), + AliasTarget: &route53.AliasTarget{ + DNSName: aws.String("abc.def.ghi."), + HostedZoneId: aws.String("123"), + }, + }, + &route53.ResourceRecordSet{ + Type: aws.String("TXT"), + Name: aws.String("test.example.com."), + ResourceRecords: []*route53.ResourceRecord{ + &route53.ResourceRecord{ + Value: aws.String(client.getGroupID()), + }, + }, + }, + &route53.ResourceRecordSet{ + Type: aws.String("TXT"), + Name: aws.String("unusual-1.example.com."), + ResourceRecords: nil, + }, + &route53.ResourceRecordSet{ + Type: aws.String("TXT"), + Name: aws.String("unusual-2.example.com."), + ResourceRecords: []*route53.ResourceRecord{}, + }, + } + groupIDInfoMap := client.groupIDInfo(records) + if len(groupIDInfoMap) != 3 { + t.Errorf("Incorrect record info for %v", records) + } + if val, exist := groupIDInfoMap["test.example.com."]; !exist { + t.Errorf("Incorrect record info for %v", records) + } else { + if val != client.getGroupID() { + t.Errorf("Incorrect record info for %v", records) + } + } + records = []*route53.ResourceRecordSet{ + &route53.ResourceRecordSet{ + Type: aws.String("A"), + Name: aws.String("test.example.com."), + ResourceRecords: []*route53.ResourceRecord{ + &route53.ResourceRecord{ + Value: aws.String("54.32.12.32"), + }, + }, + }, + &route53.ResourceRecordSet{ + Type: aws.String("TXT"), + Name: aws.String("test.example.com."), + ResourceRecords: []*route53.ResourceRecord{ + &route53.ResourceRecord{ + Value: aws.String(client.getGroupID()), + }, + }, + }, + } + groupIDInfoMap = client.groupIDInfo(records) + if len(groupIDInfoMap) != 1 { + t.Errorf("Incorrect record info for %v", records) + } + if val, exist := groupIDInfoMap["test.example.com."]; !exist { + t.Errorf("Incorrect record info for %v", records) + } else { + if val != client.getGroupID() { + t.Errorf("Incorrect record info for %v", records) + } + } +} + func TestRecordInfo(t *testing.T) { groupID := "test" client := &awsConsumer{ From dae32dfb1400907cfd638fe54c44f37a61bc2192 Mon Sep 17 00:00:00 2001 From: ideahitme Date: Tue, 28 Feb 2017 09:53:21 +0100 Subject: [PATCH 5/5] fix the whitespace --- consumers/aws.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consumers/aws.go b/consumers/aws.go index a96ffce..639771c 100644 --- a/consumers/aws.go +++ b/consumers/aws.go @@ -269,7 +269,7 @@ func (a *awsConsumer) groupIDInfo(records []*route53.ResourceRecordSet) map[stri if len(record.ResourceRecords) > 0 { groupIDMap[aws.StringValue(record.Name)] = aws.StringValue(record.ResourceRecords[0].Value) } else { - log.Errorf("Unexpected response from AWS API, got TXT record with empty resources: %s . Record is excluded from syncing", aws.StringValue(record.Name)) + log.Errorf("Unexpected response from AWS API, got TXT record with empty resources: %s. Record is excluded from syncing", aws.StringValue(record.Name)) groupIDMap[aws.StringValue(record.Name)] = "" } } else {