Safely handle records without ResourceRecords #88
Conversation
Always check the length of ResourceRecords before trying to index. Fix #87
|
@mikkeloscar I agree that the slice length should be checked, however I would love us to get a better understanding of why the TXT record has an empty RR slice. IIRC this is not possible, so maybe we can try to get more details from the issue raiser |
|
/cc @mikkeloscar @linki let's merge now and see if the issue persists. add some logging |
|
lgtm |
|
👍 |
| 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)] = "" |
There was a problem hiding this comment.
If I comment out this line then no test fails. Since it's important that we "block" the record name in this case I would love to see something fail.
There was a problem hiding this comment.
@linki yes, to enable that need to split the function into two parts 1. building dnsname->groupId map and building dnsName->recordInfo map. Thanks for good catch 👍
There was a problem hiding this comment.
@linki added some test, it is ugly, but I didn't want to spend too much time on this
consumers/aws.go
Outdated
| groupIDMap := map[string]string{} //maps dns to group ID | ||
|
|
||
| for _, record := range records { | ||
| 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)) | ||
| log.Errorf("Unexpected response from AWS API, got TXT record with empty resources: %s . Record is excluded from syncing", aws.StringValue(record.Name)) |
There was a problem hiding this comment.
minor nitpick. The space looks wrong here.
|
/cc @mikkeloscar @linki please merge if everything is fine and we can ask @snoby to give it another try |
|
👍 |
Always check the length of ResourceRecords before trying to index.
Fix #87
@snoby this should fix the issue you were seeing. I have built an image
mikkeloscar/mate:v0.6.0-1-gfbdce6bwhich you can try out if you like.You can also build your own image:
Where
your_useris your docker hub username./cc @ideahitme @linki