Rescue all errors in get_hostname_from_routes #1120
Conversation
ManageIQ#972 was not enough, e.g. OpenSSL::X509::CertificateError parsing errors still spill through. turns out OpenSSL::SSL::SSLError is not the parent of all openssl errors, that would be OpenSSL::OpenSSLError. But I won't repeating the mistake a third time, let's catch everything. This function is a best-effort guess anyway. https://bugzilla.redhat.com/show_bug.cgi?id=1436221
|
Checked commit cben@d647cd6 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
| @@ -475,7 +475,7 @@ def get_hostname_from_routes(ems, endpoint_hash, token) | |||
| client = ems.class.raw_connect(endpoint.hostname, endpoint.port, | |||
| :service => :openshift, :bearer => token, :ssl_options => ssl_options) | |||
| client.get_route('hawkular-metrics', 'openshift-infra').try(:spec).try(:host) | |||
| rescue KubeException, OpenSSL::SSL::SSLError => e | |||
| rescue StandardError => e | |||
There was a problem hiding this comment.
I think that is the same as rescue => bang
any way I think rescue KubeException, OpenSSL::SSL::OpenSSLError => e is better for us.
We would not like this to catch an undefined method for example.
There was a problem hiding this comment.
Hmm. Problem is unexpected exceptions crash UI in a way that doesn't reach user.
Let me make the other PR that reports them better to user, then I'll re-think this rescue...
you could go for a generic ssl validation error. that way you can easily satisfy internationalization and not worry about cutting long strings |
#972 was not enough, e.g. OpenSSL::X509::CertificateError parsing errors still spill through.
Turns out
OpenSSL::SSL::SSLErroris not the parent of all openssl errors, that would beOpenSSL::OpenSSLError:-(But I won't repeating the mistake a third time, let's catch everything. This function is a best-effort guess anyway.
https://bugzilla.redhat.com/show_bug.cgi?id=1436221 (master)
https://bugzilla.redhat.com/show_bug.cgi?id=1441670 (fine)
This is an improvement, but not a sufficient fixes.
I'll follow up with at least one PR to make the backend return exceptions as JSON. Should have done that long ago, would have saved a lot of effort testing & triaging.
Before, with a malformed custom CA:
After:
The "header is too long" message is too unclear :-( It's not even clear what field the error refers to.
This is a case where the model doesn't even pass rails validations, I'm looking at reporting this better...
@miq-bot add-label compute/containers, bug, fine/yes, blocker
@yaacov @moolitayer @AparnaKarve Please review.