@justinburke FYI.
It doesn't look like this is consistent with what we discussed here: #10721
Am I missing something?

Yep, you're right. First pass at implementing this we were aiming to drop in identical code logic that we had in Java where specifying a custom hostname verifier completely replaces the original hostname verification.

But I'll concede that's a separate (and possibly undesirable) concern. Let me take another pass at this PR with the skip option pulled out.

A few comments about verify_options:

• Please add API documentation.
• It seems there is only one option available at the moment, correct? What are the additional options that might be potentially added in the future. Should they go under verify_options or as an additional named argument? Should the singular option become the argument instead? (e.g. verify_callback=callback_fn)
• I prefer validation_options or verification_options or just options if this is supposed to be more generic (based on the answer to the above question).

There was a bit of churn before the PR was opened that led to this being an object. At first there were more options (like assert fingerprint, skip hostname verification, ...) but all that got pulled out. Given that, I think it would be perfectly fine just to have a verify_callback=None argument that could be passed in directly. It could be helpful to forward-plan for additional options inside the verify_options, but I can't name any off the top of my head I think should be added that the callback couldn't implement. I also think it's reasonably pythonic if we wanted to support more options in the future to just to add those as additional optional named arguments.

So given that context, unless you think otherwise I'll just change this to a verify_callback parameter.

Let's go with verify_callback!

Sounds good! PR updates pushed.

I think the way this callback is invoked with respect to concurrency needs to be changed.

The C-Core is completely non-blocking and asynchronous. Since some C-Core thread (which may or may not be a ruby thread), is directly acquiring the interpreter lock and calling in to this ruby application handler, this has the potential to indefinitely block a C-Core thread and prevent intended work from being done.

One fix for this could be to dedicate a ruby thread to the task of invoking these callbacks. Such a thread could poll for work on a queue of "ready-to-run" "veroify peer callbacks". The actual verify peer callback handler that the C-Core invokes would just add to that queue, signal conditional variable, etc.. We already have such a thread for the task is invoking ruby-defined "call credentials" callbacks (See https://github.com/grpc/grpc/blob/master/src/ruby/ext/grpc/rb_event_thread.c#L126), so I think that adding another would could be reasonable. An optimization could be to combine both of these thread in to one, but I'm not sure that's worth doing.

^ on second though, it might actually be simpler to combiner invocation of these handlers in to the call credentials callback thread, by using e.g. a "queue node tag type", but I'm not sure

^^^ Oooh... good point!

Also, just to add, granted an indefinitely blocking application callback would also deadlock a dedicated callback thread and prevent more callbacks from being fired, but IMO overall the dedicated thread is simpler; it's difficult to reason about what happens when a c-core thread acquires a ruby lock and runs ruby code.

Hmm, what you're saying makes total sense, but the verify callback exposed by C-Core at present expects the callback to be synchronous so it doesn't pass in the on_peer_checked closure that would allow the callback to be asynchronous.
I don't think it would be a big deal to tweak the C-Core API so that callbacks are asynchronous. Which sounds like it might be important given interpreter locks. Is there anyone that should weight in before I go down the route of changing that core API?

cc @jiangtaoli2016 @markdroth for this core API change proposal (to allow the verify peer callback to be async)

I agree, it's not okay to block in a callback from a C-core thread.

I'm not thrilled about adding yet another case where we need an asynchronous callback from C-core to the application. We already have a few of these, and they've caused a number of headaches over time; it turns out to be fairly tricky to get the synchronization and state machinery right when leaving and re-entering C-core.

That having been said, I don't have a better suggestion, and the C-core API is not a public API, so we do have the flexibility to change this later if we can come up with something better. So I'd say go ahead and add the callback to the API.

I suggest that you model the changes after one of the existing implementations. The best one to look at is probably the metadata credentials plugin, whose API is defined here:

https://github.com/grpc/grpc/blob/master/include/grpc/grpc_security.h#L304

The implementation is here:

https://github.com/grpc/grpc/blob/master/src/core/lib/security/credentials/plugin/plugin_credentials.cc
https://github.com/grpc/grpc/blob/master/src/core/lib/security/credentials/plugin/plugin_credentials.h

Note that this API allows both sync and async returns. The sync return is much simpler and more efficient in the case where no blocking is needed; the async return requires a callback and a thread hop, so it's more complex and less efficient.

One thing that is not handled correctly in the metadata credentials plugin API is cancellation. Unfortunately, this API didn't originally support cancellation, and all of the wrapped language APIs were added without cancellation support. When we later realized that we needed to support this, we were able to change the C-core API, but it was too late to fix the wrapped language APIs. So we did a poor man's approach: we have C-core mark the request as cancelled, and when the wrapped language eventually invokes the callback, nothing actually happens. But this means that we still wind up doing a lot of processing in the application that isn't actually necessary, because we have no way to inform it that the work is no longer needed. I highly recommend not making that mistake with this new API: let's make sure that we expose a way of propagating cancelations to the wrapped language, so that this can be handled as a first-class citizen.

Please let me know if you have any questions.

Thanks for all the pointers! I'll work on putting together an update to the core API and put that in a new PR. I'll come back to the python/ruby bindings in this PR once we get that ironed out. It'll probably be a little bit before I can get through that, so I'll flag you on it when it's ready.