-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
fix: possible nil ptr for invalid records like too long name #4295
Conversation
Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
if ep == nil { | ||
continue | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmmmh 🤔
Wdyt about avoiding to append it when it's nil ?
Something like this on line 399
ep := endpoint.NewEndpointWithTTL(wildcardUnescape(aws.StringValue(r.Name)), aws.StringValue(r.Type), ttl, targets...)
if ep != nil {
if aws.StringValue(r.Type) == endpoint.RecordTypeCNAME {
ep = ep.WithProviderSpecific(providerSpecificAlias, "false")
}
newEndpoints = append(newEndpoints, ep)
}
And add a nil check also on line 412:
ep := endpoint.
NewEndpointWithTTL(wildcardUnescape(aws.StringValue(r.Name)), endpoint.RecordTypeA, ttl, aws.StringValue(r.AliasTarget.DNSName))
if ep != nil {
ep = ep.WithProviderSpecific(providerSpecificEvaluateTargetHealth, fmt.Sprintf("%t", aws.BoolValue(r.AliasTarget.EvaluateTargetHealth))).
WithProviderSpecific(providerSpecificAlias, "true")
newEndpoints = append(newEndpoints, ep)
}
@@ -353,6 +353,12 @@ func TestAWSRecords(t *testing.T) { | |||
TTL: aws.Int64(recordTTL), | |||
ResourceRecords: []*route53.ResourceRecord{{Value: aws.String("8.8.8.8")}}, | |||
}, | |||
{ | |||
Name: aws.String("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA.ext-dns-test-2.teapot.zalan.do."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a test for a label >63 chars? I've been using this-is-an-exceedingly-long-label-that-external-dns-should-reject
locally which is 65.
@@ -416,6 +416,9 @@ func (p *AWSProvider) records(ctx context.Context, zones map[string]*route53.Hos | |||
} | |||
|
|||
for _, ep := range newEndpoints { | |||
if ep == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, records()
is a callback for ListResourceRecordSetsPagesWithContext()
so isn't it a fair assumption that all endpoints here are non-nil?
Some slack discussion happened with #4293, so I will close this one in favor of the upcoming changes. |
fix: possible nil ptr for invalid records like too long name
see also: https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/DomainNameFormat.html
closes #4293
Checklist