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

Collect connection address in connectivity test #223

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

fortuna
Copy link
Contributor

@fortuna fortuna commented Apr 19, 2024

We now have connectivity test functions for dialer wrappers, rather than just for resolvers.
This also adds config.WrapPacketDialer

}
return conn, err
})
streamDialer, err := config.WrapStreamDialer(baseDialer, *transportFlag)
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 tried to intercept the streamDialer instead of the baseDialer, but it didn't work because the RemoteAddress is for the target in the PacketListenerDialer. Code: https://github.com/Jigsaw-Code/outline-sdk/compare/fortuna-address2?expand=1

It's not clear what the RemoteTarget should be, so I'm not sure that is a god approach. I think I prefer to keep this base dialer interception. But we could think about some sort of event listener instead, to reduce wrapping.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fortuna I like the idea of intercept the streamDialer. btw in the suggested implementation, if connect fails and conn is nil, connectionAddress will be left empty and we don't know what endpoint was attempted. I can address this by extracting attempted endpoint from err by assessing error type with err.(*net.OpError).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have the dialed address in the transport specification.

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 updated the PR. We now have functions that can test dialer wrappers, since the one that takes the resolver was not enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see your point. We won't have the address for errors, so it's hard to tell if it's a bad resolution, for instance.

I think we can extend the code to do Happy Eyeballs or Dialer.ControlContext. But perhaps in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If multiple endpoints are attempted, which one do we select? The first one? The last one? All of them? Perhaps we need to collect all of them, then specify which one we picked for the transport part.

I don't think we are able to capture all of them by looking at the error. We need to do the resolution ourselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is still a bit clunky, but I think the data model is better aligned. Example:

% go run . -transport $KEY -proto tcp | jq
{
  "resolver": "8.8.8.8:53",
  "proto": "tcp",
  "transport": "ss://[email protected]:80?prefix=",
  "connections": [
    {
      "address": {
        "host": "159.203.76.146",
        "port": "80"
      },
      "error": null
    }
  ],
  "selected_address": {
    "host": "159.203.76.146",
    "port": "80"
  },
  "time": "2024-04-24T02:22:09Z",
  "duration_ms": 30,
  "error": {
    "op": "receive",
    "msg": "chacha20poly1305: message authentication failed"
  }
}

@fortuna
Copy link
Contributor Author

fortuna commented Apr 24, 2024

This needs to be updated. See #209 (comment)

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.

2 participants