Refactor cert_expiry for 3.7+, retry transient errors, wait for HA HTTP#32001
Refactor cert_expiry for 3.7+, retry transient errors, wait for HA HTTP#32001jjlawren wants to merge 4 commits intohome-assistant:devfrom
Conversation
|
Hey there @cereal2nd, mind taking a look at this pull request as its been labeled with a integration ( |
Codecov Report
@@ Coverage Diff @@
## dev #32001 +/- ##
==========================================
+ Coverage 94.69% 94.7% +<.01%
==========================================
Files 766 766
Lines 55567 55590 +23
==========================================
+ Hits 52619 52645 +26
+ Misses 2948 2945 -3
Continue to review full report at Codecov.
|
| _LOGGER.error("Certificate does not match host: %s", host) | ||
| self._errors[CONF_HOST] = "wrong_host" | ||
| elif "certificate has expired" in err.verify_message: | ||
| _LOGGER.error("Certificate has expired: %s", host) |
There was a problem hiding this comment.
Why is this an error for setting up the config flow? Isn't the whole reason this sensor exists to check this?
There was a problem hiding this comment.
The error is meant to interactively help fix an existing broken setup. We could create a new sensor and assume the user will check the logs for more info on why the sensor is reporting a failed cert, but presenting this info in the UI feels like it would help resolve the problem faster.
There was a problem hiding this comment.
But what if the user wants to set up the sensor, see that it's invalid, then fix the sensor and see it jump to valid ?
There was a problem hiding this comment.
You wouldn't prevent a leak detector from being paired if it was currently detecting water.
There was a problem hiding this comment.
@balloob : I'm not so sure... this component give us the number of days before expiration. If the certificate is already expired, the state will be negative and I'm not sure it will really work.
I would agree with you if the state was "IsExpired: yes/no" and one of the attribute was the number of days to go to the expiration.
There was a problem hiding this comment.
After some thought I agree with @balloob and this will be allowed in a new PR.
| def retry_delay(self): | ||
| """Return the retry delay in seconds.""" | ||
| return int(min(2 ** (self._retry_attempts - 1) * 30, 3600)) | ||
|
|
There was a problem hiding this comment.
I'm personally not a fan of such retry logic if that is just simple fix with SCAN_INTERVAL = timedelta(30min) and trow an Retry error on config entry setup if the server is not available. Just 2 lines that reduce the complexity of this PR and remove - 60 lines
There was a problem hiding this comment.
Simpler is definitely better. However, some (but not all) users monitoring the cert of the http interface still encounter this on every startup. Having the sensor unavailable for even 30min on startup is a long time. And a 30min interval is far too often for properly validating certs. I still like the exception-based retry mechanism for this use case and a 12h interval otherwise.
With this retry in place, the startup delays based on EVENT_HOMEASSISTANT_START can probably be removed to make things a bit simpler again.
What do you think?
There was a problem hiding this comment.
Okay, I now see the ConfigEntryNotReady exception you're referencing. I think that should handle the retry logic on startup. 👍
|
Closing this in favor of #32066. |
5 participants
Proposed change
With Python 3.7+ there are new methods available in the
sslmodule which provide more detailed information on errors. This PR refactors around those messages.Additionally, some of these errors can be considered transient, so an async retry mechanism with backoff has been added. The previous default of 12h was quite a long time to wait if a site was temporarily unavailable at startup.
Finally, the
httpintegration has been marked as a dependency to hopefully resolve issues like #31964.Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale: