Skip to content
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

Ref #670 - Change MX result caching to IP-based #889

Closed
wants to merge 1 commit into from
Closed

Conversation

mxsasha
Copy link
Collaborator

@mxsasha mxsasha commented Mar 1, 2023

This is a partial, but simple change: our original cache was based on the hostname in the MX result. Now, it's based on the A+AAAA that this hostname resolves to, at the cost of an extra resolve call, but resolving is cheap. This helps when multiple hostnames resolve to the same set of A+AAAA, but not when different MX hostnames resolve to slightly different sets of IPs. The latter would be more complex, and I'd first want to have a plan for #714.

@mxsasha mxsasha added this to the v1.7 milestone Mar 1, 2023
@mxsasha mxsasha self-assigned this Mar 1, 2023
@gthess
Copy link
Collaborator

gthess commented Mar 6, 2023

I don't remember the details but could this result in tests that rely on the hostname (like DANE and the certificate tests) to be shared among different hostnames with the same IPs?

@mxsasha
Copy link
Collaborator Author

mxsasha commented Mar 20, 2023

I don't remember the details but could this result in tests that rely on the hostname (like DANE and the certificate tests) to be shared among different hostnames with the same IPs?

You are entirely right. Thanks for spotting this!

@mxsasha mxsasha closed this Mar 20, 2023
@baknu
Copy link
Contributor

baknu commented Mar 20, 2023

Ah yes, so this is unfortunately not going to work. The idea behind it was mainly to prevent hitting rate limits.

Idea (not for v1.7): maybe (mainly for the batch test) we can prevent MX's that have a different hostname but have the same IP address(es) from being tested too soon after each other. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants