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

Add connectivity tests on startup to the actual instances #348

Closed
hilts-vaughan opened this issue Feb 12, 2020 · 10 comments · Fixed by #1832 or #1811
Closed

Add connectivity tests on startup to the actual instances #348

hilts-vaughan opened this issue Feb 12, 2020 · 10 comments · Fixed by #1832 or #1811
Assignees
Labels
priority: p0 Highest priority. Critical issue. P0 implies highest priority. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@hilts-vaughan
Copy link
Contributor

Some customers are confused by "Ready for new connections" and then try and connect and find it fails. This is often a routing problem but some of them incorrectly assume that the message above means everything was validated and fine.

Should we perform some kind of connectivity test when creating the instances or is this too much?

@kurtisvg kurtisvg added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Feb 12, 2020
@kurtisvg
Copy link
Contributor

I think we should definitely provide this feature. It probably should be toggle-able by a flag, and I'm unsure if it should be enabled by default. I'd be open to other folks opinions on the matter.

@hilts-vaughan
Copy link
Contributor Author

I think unless all the examples show the flag it won't see widespread adoption and many users will still confuse themselves. I would be inclined for default but it may not technically be backwards compatible if for some reason on startup the proxy can't connect (but presumably it is later able to)

@tmaiaroto
Copy link

tmaiaroto commented Feb 28, 2020

Yes please! I'm experience an issue with this now. Randomly my container is failing with OAuth and finally got into a broken container today to look around. There's an issue with SSL certs. Yet will continue along saying "Ready for new connections" ... Which it's not. While I'd really love to know why sometimes it's unable to make any HTTPS requests, in the least a failure would help a lot. It could re-deploy a working container.

@hilts-vaughan
Copy link
Contributor Author

Hey Kurtis,

In the case where we detect failed connectivity on instances, what should we do? What if there are multiple instances only one of them fail? Crash? Emit a warning?

@kurtisvg kurtisvg changed the title The proxy does not perform connectivity tests on startup to the actual instances Add connectivity tests on startup to the actual instances Feb 29, 2020
@kurtisvg
Copy link
Contributor

As a developer, I would find it most useful if it failed and emitted a reason why.

@hilts-vaughan
Copy link
Contributor Author

Maybe we could implement this as just a basic TCP connectivity test, since that would let us remain agnostic of the actual underlying protocols?

This would rule out routing and firewall problems, but of course not catch EVERYTHING that could go wrong.

@tmaiaroto
Copy link

Not being able to connect to the OAuth endpoint is a pretty big thing to go wrong though. Would love to see it fail should that happen. If I can get the time, I may take a look and see if I can add a pull request or something.

@hilts-vaughan
Copy link
Contributor Author

I think you're right -- we should try and catch that too. Though, let's fry one fish at a time. :)

@enocom
Copy link
Member

enocom commented Feb 9, 2021

Related to #137.

@enocom enocom added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Feb 9, 2021
@enocom enocom added the v2 label Dec 2, 2021
@enocom enocom removed the v2 label Dec 2, 2021
@enocom
Copy link
Member

enocom commented Aug 29, 2022

This would be an easy feature to add now that we have a CheckConnections method on the internal proxy struct. We currently use this when health checks are enabled. It would be easy to allow callers to opt-in to a connection check on proxy startup.

@enocom enocom added priority: p0 Highest priority. Critical issue. P0 implies highest priority. and removed priority: p2 Moderately-important priority. Fix may not be included in next release. labels Apr 3, 2023
@enocom enocom self-assigned this Apr 5, 2023
enocom added a commit that referenced this issue Jun 2, 2023
When callers want to ensure their instance is in fact reachable, the
--run-connection-test flag will attempt to connection to all registered
instances.

Fixes #348
enocom added a commit that referenced this issue Jun 7, 2023
When callers want to ensure their instance is in fact reachable, the
--run-connection-test flag will attempt to connection to all registered
instances.

Fixes #348
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p0 Highest priority. Critical issue. P0 implies highest priority. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
4 participants