Skip to content

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Dec 20, 2024

Motivation

It seems our tests broke due to maybe some change in how AWS was returning their DNS records, and it seems we return the full chain of resolution.

When doing the following request:
request = dns.message.make_query(hostname, "A")
We would get a full answer containing CNAME records:
[<DNS sts.amazonaws.com. IN CNAME RRset: [<reg.sts-ge.amazonaws.com.>]>, <DNS reg.sts-ge.amazonaws.com. IN CNAME RRset: [<use1-default-r.sts-ge.amazonaws.com.>]>, <DNS use1-default-r.sts-ge.amazonaws.com. IN CNAME RRset: [<sts-global-default.us-east-1.amazonaws.com.>]>, <DNS sts-global-default.us-east-1.amazonaws.com. IN A RRset: [<209.54.177.164>]>]

Which then lead to an issue when trying to access the address field:
AttributeError: 'CNAME' object has no attribute 'address'

I've introduced a filter step to make sure we're only trying to get A records before choosing. However, I'm not quite sure if this is expected behavior, I believe that might be a change in how AWS returns their DNS and introducing a few resolving steps in between. I don't know if we make the query better? Should we only get the last answer instead of filtering? Something like list(response.answer[-1].items.keys()) instead? My knowledge of DNS stops here 😄

Manual run for bootstraps tests: #/12434395225

Changes

  • filter the returned list[RRSet] to only contains A type before trying to access the address field.

@bentsku bentsku requested a review from simonrw December 20, 2024 15:46
@bentsku bentsku requested a review from thrau as a code owner December 20, 2024 15:46
@bentsku bentsku self-assigned this Dec 20, 2024
@bentsku bentsku added the semver: patch Non-breaking changes which can be included in patch releases label Dec 20, 2024
Copy link

github-actions bot commented Dec 20, 2024

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   4m 20s ⏱️
441 tests 389 ✅  52 💤 0 ❌
882 runs  778 ✅ 104 💤 0 ❌

Results for commit 5037582.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@simonrw simonrw left a comment

Choose a reason for hiding this comment

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

It looks like the DNS response from 8.8.8.8 (the default upstream) is

; <<>> DiG 9.18.28 <<>> @8.8.8.8 sts.amazonaws.com A
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 29178
;; flags: qr rd ra; QUERY: 1, ANSWER: 4, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 512
;; QUESTION SECTION:
;sts.amazonaws.com.		IN	A

;; ANSWER SECTION:
sts.amazonaws.com.	3365	IN	CNAME	reg.sts-ge.amazonaws.com.
reg.sts-ge.amazonaws.com. 2521	IN	CNAME	use1-default-r.sts-ge.amazonaws.com.
use1-default-r.sts-ge.amazonaws.com. 2199 IN CNAME sts-global-default.us-east-1.amazonaws.com.
sts-global-default.us-east-1.amazonaws.com. 12 IN A 54.239.29.25

;; Query time: 19 msec
;; SERVER: 8.8.8.8#53(8.8.8.8) (UDP)
;; WHEN: Fri Dec 20 16:47:56 GMT 2024
;; MSG SIZE  rcvd: 159

which is very strange, but consistent with the behaviour we see. Thanks for making this change!

ip_addresses = list(response.answer[0].items.keys())
ip_addresses = []
for answer in response.answer:
if answer.match(dns.rdataclass.IN, dns.rdatatype.A, dns.rdatatype.TYPE0):
Copy link
Contributor

Choose a reason for hiding this comment

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

q: what is TYPE0 in this context? The argument is called covers but I don't know what that means.

Does this mean "anything" as 0 is not a special value perhaps?

Copy link
Contributor Author

@bentsku bentsku Dec 20, 2024

Choose a reason for hiding this comment

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

Good question. 😬 I hoped you'd be able to answer this, I've printed all the different answer objects and printed the covers field of them, and all of them returned TYPE0. Looking more into it, it seems related with RRSIG record types: https://simpledns.plus/help/rrsig-records

https://dnspython.readthedocs.io/en/latest/rdata-set-classes.html#dns.node.Node.find_rdataset

covers, a dns.rdatatype.RdataType, the covered type. Usually this value is dns.rdatatype.NONE, but if the rdtype is dns.rdatatype.SIG or dns.rdatatype.RRSIG, then the covers value will be the rdata type the SIG/RRSIG covers. The library treats the SIG and RRSIG types as if they were a family of types, e.g. RRSIG(A), RRSIG(NS), RRSIG(SOA). This makes RRSIGs much easier to work with than if RRSIGs covering different rdata types were aggregated into a single RRSIG rdataset.

So I suppose it does not apply to type A records and can safely be put to TYPE0 or NONE (both are equal to 0 in the Enum, so I've replaced TYPE0 by NONE which is a bit more explicit?

Interesting design decision to make this field mandatory even if it applies to only a few record types.

Copy link

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 50m 59s ⏱️ -7s
3 902 tests ±0  3 595 ✅ ±0  307 💤 ±0  0 ❌ ±0 
3 904 runs  ±0  3 595 ✅ ±0  309 💤 ±0  0 ❌ ±0 

Results for commit 5037582. ± Comparison against base commit d6e0887.

@bentsku bentsku merged commit 96eec2e into master Dec 20, 2024
38 checks passed
@bentsku bentsku deleted the fix-dns-resolving branch December 20, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants