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

mxresolv.Lookup() now properly randomized hosts in some situations #175

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

thrawn01
Copy link
Contributor

@thrawn01 thrawn01 commented Jun 20, 2023

Purpose

mxresolv.Lookup() was not properly randomzing MX hosts returned in some situations.

Implementation

  • Tests can now mxresolv.SetDeterministic() to improve the quality of testing
  • Switched to mockdns for lookups instead of relying on actual DNS records on the internet.
  • Now using r := rand.New(rand.NewSource()) instead of the global source in the rand package.
  • Added tests to cover actual MX records found in the wild.
  • Fixed an issue where the shuffle would ignore the last MX record in the last preference group.
  • Fixed an issue where the shuffle would ignore the first MX record in the first preference group.
  • Fixed an issue where prefer was ignored if there were only 2 MX records with difference preferences.
  • Fixed an issue where the the first MX record would always be chosen if there were only 2 MX records with the same preference.
  • Fixed Fix and enable mxresolv.TestLookupError #155
  • Added benchmarks which show golang optimizing the rand.New() call.
  • Disabled (gosec) linter for mxresolv as it complained I was using math/rand instead of crypto/rand
// pkg: github.com/mailgun/holster/v4/mxresolv
// BenchmarkShuffleWithNew
// BenchmarkShuffleWithNew-10       61962	     18434 ns/op    5376 B/op       1 allocs/op
// BenchmarkShuffleGlobal
// BenchmarkShuffleGlobal-10        65205	    18480 ns/op       0 B/op	    0 allocs/op

Copy link
Contributor

@ToutPetitAdrien ToutPetitAdrien left a comment

Choose a reason for hiding this comment

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

Third time lucky

@thrawn01 thrawn01 merged commit fa93174 into master Jun 21, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix and enable mxresolv.TestLookupError
2 participants