SSLError not always translated to KubeException

[cben]

There is a specific test that it should be (and #195 will add a second test):
https://github.com/abonas/kubeclient/blob/f489a35027a7/test/test_kubeclient.rb#L95-L105
but in reality I'm seeing an SSLError escaping:
https://gist.github.com/cben/faf8a9f7f104561c0b36c88ae02f2509
That makes sense, handle_exception only rescues RestClient::Exception.
Why does the test pass?

Aha. RestClient has its own http://www.rubydoc.info/github/rest-client/rest-client/RestClient/SSLCertificateNotVerified, wraps some SSLErrors:
https://github.com/rest-client/rest-client/blob/8874463a4/lib/restclient/request.rb#L742-L761

      # TODO: deprecate and remove RestClient::SSLCertificateNotVerified and just
      # pass through OpenSSL::SSL::SSLError directly.
    ...
      if error.message.include?("certificate verify failed")
        raise SSLCertificateNotVerified.new(error.message)

In above gist the message was "hostname ... does not match the server certificate", not "certificate verify failed", so it isn't wrapped.

• Looks unwise to depend on SSLCertificateNotVerified wrapping or not wrapping.
• Would Kubeclient always wrapping SSLError be desired behavior?
  • I have a (weak) use case for distinguishing SSLError from other errors. Would be nice to use a subclass, similar to Introduce Kubeclient::ResourceNotFoundError #233.
• Another alternative would be never wrapping. This would require re-raising SSLCertificateNotVerified, and users having to expect either SSLError or SSLCertificateNotVerified. IMO less nice and unexpected.

I think either change would be slightly backward-incompatible.

[simon3z]

@cben what's the final impact here. Does this deserve a bug to be tracked in MIQ?
Update: ah I see this was as result of https://bugzilla.redhat.com/show_bug.cgi?id=1436221

[moolitayer]

Based on the link @cben posted I think we should catch OpenSSL::SSL::SSLError to keep the behavior we have (or thought we had). Introducing a new subclass sound good but I'd like to see this resolved first.

@cben do you plan to send a PR for this? I have some time to do that, let me know
(I think you have a lot on your plate ATM and this is high priority)

[moolitayer]

Some test code and different exceptions I got

test code

require 'kubeclient'

cert_store = OpenSSL::X509::Store.new
cert_store.add_cert(OpenSSL::X509::Certificate.new(IO.read('ca_bad.crt')))

ssl_options = {
    cert_store: cert_store,
    verify_ssl: OpenSSL::SSL::VERIFY_PEER
}
client = Kubeclient::Client.new(
  'https://vm-48-131.eng.lab.tlv.redhat.com:8443',
  'v1',
  :ssl_options  => ssl_options,
  :auth_options => {:bearer_token => token}
)
puts client.get_nodes

/home/mtayer/.gem/ruby/2.3.1/gems/kubeclient-2.3.0/lib/kubeclient/common.rb:107:in `rescue in handle_exception': SSL_connect returned=1 errno=0 state=error: certificate verify failed (KubeException)
	from /home/mtayer/.gem/ruby/2.3.1/gems/kubeclient-2.3.0/lib/kubeclient/common.rb:99:in `handle_exception'
	from /home/mtayer/.gem/ruby/2.3.1/gems/kubeclient-2.3.0/lib/kubeclient/common.rb:433:in `fetch_entities'
	from /home/mtayer/.gem/ruby/2.3.1/gems/kubeclient-2.3.0/lib/kubeclient/common.rb:423:in `load_entities'
	from /home/mtayer/.gem/ruby/2.3.1/gems/kubeclient-2.3.0/lib/kubeclient/common.rb:111:in `discover'
	from /home/mtayer/.gem/ruby/2.3.1/gems/kubeclient-2.3.0/lib/kubeclient/common.rb:78:in `method_missing'
	from /home/mtayer/.local/bin/kubeclient-local-ssl.rb:17:in `<main>'

/home/mtayer/.local/bin/kubeclient-local-ssl.rb:5:in `initialize': header too long (OpenSSL::X509::CertificateError)
	from /home/mtayer/.local/bin/kubeclient-local-ssl.rb:5:in `new'
	from /home/mtayer/.local/bin/kubeclient-local-ssl.rb:5:in `<main>'

/home/mtayer/.local/bin/kubeclient-local-ssl.rb:5:in `initialize': nested asn1 error (OpenSSL::X509::CertificateError)
	from /home/mtayer/.local/bin/kubeclient-local-ssl.rb:5:in `new'
	from /home/mtayer/.local/bin/kubeclient-local-ssl.rb:5:in `<main>'

I'm not sure we should catch & wrap, what rest client is doing is silly. It should be fix in rest client. Also It would break backward compatibility as @cben mentioned. So I'm favor of not changing anything in kubeclient

@simon3z @abonas What do you think?

[abonas]

Is there an issue reported on this to rest client repo?

[moolitayer]

Is there an issue reported on this to rest client repo?

rest-client/rest-client#168 (from 2013)
This might also be related: rest-client/rest-client#286

[cben]

I propose for kubeclient 3, we solve this by adding Kubeclient::SSLError which will wrap both OpenSSL::SSL::SSLError and RestClient::SSLCertificateNotVerified.
There is a tradeoff with wrapping exceptions — makes easier to recognize errors that passed through us, but harder to know what exactly they were — but IMHO this would make kubeclient nicer to use on the balance.

• Should the new Kubeclient::SSLError subclass Kubeclient::HttpError? Technically, SSL errors may happen before HTTP even comes into the picture. But if it doesn't subclass, we may want a new parent Kubeclient::Error for both — or again users would need to remember one common and one rare class to rescue. I think pragmatically, we can subclass, keeping HttpError as our top error.

[cben]

@grosser any opinion/advice? I see we can learn from your workarounds...

• https://github.com/zendesk/samson/pull/2388/files#diff-cd8b4517098ba133e33da78bca46c6bdR14 — users also have to deal with os-level network errors. You also catch ECONNREFUSED there. And we saw EBADF in Catch EBADF when watch_stream fails #280 (though that was internally caused, now we catch it).
  I don't think we want to wrap OS errors. I wouldn't suggest wrapping SSL if RestClient didn't make a mess of it :-)
• do not assume the exception has error code since it could be ssl error zendesk/samson#2391 — it's annoying that HttpError doesn't always have error_code. It's not only SSL that doesn't, but if we subclass HttpError it'll contribute to the problem.
  RestClient has a separate ExceptionWithResponse class. So one way is to split something like Kubeclient::HttpErrorWithResponse < Kubeclient::HttpError. meh.
  Perhaps just ensure HttpError always responds to .error_code but sometimes returns nil?

[grosser]

rescuing all and having error_code nil for ssl/ECONNREFUSED could be good ... the error.cause will always point to the original

…

On Tue, Jan 23, 2018 at 2:27 AM, Beni Cherniavsky-Paskin < ***@***.***> wrote: @grosser <https://github.com/grosser> any opinion/advice? I see we can learn from your workarounds... - https://github.com/zendesk/samson/pull/2388/files#diff- cd8b4517098ba133e33da78bca46c6bdR14 <https://github.com/zendesk/samson/pull/2388/files#diff-cd8b4517098ba133e33da78bca46c6bdR14> — users also have to deal with os-level network errors. You also catch ECONNREFUSED there. And we saw EBADF in #280 <#280> (though that was internally caused, now we catch it). I don't think we want to wrap OS errors. I wouldn't suggest wrapping SSL if RestClient didn't make a mess of it :-) - zendesk/samson#2391 <zendesk/samson#2391> — it's annoying that HttpError doesn't always have error_code. It's not only SSL that doesn't, but if we subclass HttpError it'll contribute to the problem. RestClient has a separate ExceptionWithResponse class <http://www.rubydoc.info/github/rest-client/rest-client/RestClient/ExceptionWithResponse>. So one way is to split something like Kubeclient::HttpErrorWithResponse < Kubeclient::HttpError. meh. Perhaps just ensure HttpError always responds to .error_code but sometimes returns nil? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#240 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAsZxv5wxZhDHwwstRMVGYmz1o-CP5cks5tNbQOgaJpZM4Mq2kK> .