-
Notifications
You must be signed in to change notification settings - Fork 47
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
switch to ssllabs v3 api endpoint #34
Conversation
The v2 endpoint has been dead for the last ~60 days.
There is a failing rspec test but it is unrelated to the PR and is failing on master. |
Eh, actually, it looks like v2 isn't dead but has started taking over 2 minutes to return checks on most of my hosts. Hmm. |
I can confirm the tests are failing on master. I will take a closer look at the tests. Makes sense that they would keep the old api running for a while even if its slower. I am all for updating it as long as it does not break anything else. Can you provide a manual or automated testing artifact? It can be a simple display of redacted input and output. I will try to take a look at the test and see why it is failing. |
I am seeing both of them as unbearably slow although sometimes it returns in a somewhat reasonable time period:
From a bit of research v2 is indeed deprecated: https://github.com/ssllabs/ssllabs-scan/blob/master/ssllabs-api-docs-v2-deprecated.md When I use curl it returns super quickly:
This leads me to believe while we should indeed switch there is something horribly wrong in the code that causes it to be that inefficient. I looked through the options and even with attempting a single check occurence it is timing out.
|
I tested as far back as 1.0.0 and it is having the same issues regardless of api version. |
Because I was paranoid of caching:
|
So I did some more testing and there is something very odd it also does not seem to be very fast when clearing the cache and running the test from the browser. I added some debug code and seem to be able to get it to work pretty consistently. I noticed after putting in some debug code that there was an eta field which seems like something we should look at rather than randomly sleep for some arbitrary number. In some cases I saw the eta return over 200 which makes me feel like that is the way to go. Can you provide a bit of context on how you previously used this check? What I am looking for specifically is what kind of interval did you set this check to run on, how many check attempts, and what was your time between value? I am trying to gauge if I have unrealistic expectations for how fast it should return as it seems like it takes too long.
Here is the diff of my debug code:
I did one more test and when setting the cache to true it comes back very quickly. I feel like maybe that is an implementation detail best left to the user rather than hard coded to say the first request is always without cache. I realize that there is limited value in running this with cache turned on I guess depending on how often they clear their cache it might make sense. |
While I'm actually running the check under an old school nagios setup. The check is setup on run once per day on ~10 hosts. This had been working acceptable for about a year. I think the high rate of failure started about 2 months ago. I've been wondering if I should drop them a line to see if they are throttling requests by IP address.
|
Thanks for the context, from my tests when submitting without cache it seems to work OK although slower than most sensu/nagios checks I would expect to run but it is an async operation and not something we can control. I will submit a PR with those changes minus any debug bits.
I don't think so, you would receive a 429 when submitting too many requests: https://github.com/ssllabs/ssllabs-scan/blob/master/ssllabs-api-docs-v3.md#access-rate-and-rate-limiting |
I created #35 to better address the issues here, thank you for your help! I will try to get to it over the weekend if I can. If you want to take a stab at it before then feel free to. You should be able to take the rough code and clean it up a bit if you want to. |
The v2 endpoint has been dead for the last ~60 days.
Pull Request Checklist
Resolves #33
General
Update Changelog following the conventions laid out here
RuboCop passes
Existing tests pass