Skip to content

Conversation

@nirinchev
Copy link
Contributor

This appears to be the cause of the failures we're seeing in mongosh. The logs indicate that we're getting duplicate dns addresses, which causes us to fire up two servers, one of which will inevitably fail due to address being in use. Here's the corresponding log message:

2025-02-26T17:22:33.225Z connection controller INFO  OIDC-PLUGIN [
  { __value: 1002000028 },
  'compass-oidc',
  'Resolved hostnames for local server',
  {
    url: 'http://localhost:37197/redirect',
    urlPort: 37197,
    hostname: 'localhost',
    interfaces: [
      { address: '127.0.0.1', family: 4 },
      { address: '127.0.0.1', family: 4 }
    ]
  }
]

@nirinchev nirinchev requested review from addaleax and gagik February 26, 2025 18:10
@github-actions github-actions bot added the fix label Feb 26, 2025
Copy link
Contributor

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Code LGTM but this should have a ticket number before merging and an associated spec change

});
};

private static async _getAllInterfaces(
Copy link
Contributor

Choose a reason for hiding this comment

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

optional suggestions: Feels a bit silly to mark this private and then access it as if it weren't private, no? It's not exported from the package anyway, don't be shy to mark this public to make it a bit more obvious that it's tested directly

And while you're at it, you might as well pass in dns.lookup as an argument instead of mocking it in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was debating with myself whether I want to make it public or if I want to instead call .listen() and then inspect the logs to see what interfaces were discovered. I feel a bit iffy about exposing methods publicly only for the sake of testing, but perhaps that's a bit more acceptable in the JS world 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

If that feels iffy, I think you could also just export it as a separate method, it's fairly standalone and not really tied to the rest of the class semantically 🙂

@nirinchev nirinchev changed the title fix: filter out duplicate dns results fix: filter out duplicate dns results MONGOSH-2027 Feb 27, 2025
@github-actions github-actions bot added fix and removed fix labels Feb 27, 2025
@nirinchev nirinchev merged commit e4925f3 into main Feb 27, 2025
18 checks passed
@nirinchev nirinchev deleted the ni/dns-duplicates branch February 27, 2025 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants