Hi Ismael, Thanks for reviewing the KIP. I've made a first pass at updating based on your feedback.
Questions/comments inline... On Thu, Oct 14, 2021, at 6:20 AM, Ismael Juma wrote: > Hi Kirk, > > Thanks for the KIP. It looks good overall to me. A few nits: > > 1. "sasl.login.retry.wait.ms": these configs are typically called `backoff` > in Kafka. For example "retry.backoff.ms". The default for `retry.backoff.ms` > is 100ms. Is there a reason why we are using a different value for this > one? The `sasl.login.retry.max.wait.ms` should be renamed accordingly. Changed to sasl.login.retry.backoff.ms and sasl.login.retry.backoff.max.ms and changed the former to 100 ms. > 2. "sasl.login.attempts": do we need this at all? We have generally moved > away from number of retries in favor of timeouts for Kafka (the producer > has a retries config partly for historical reasons, but partly due to > semantics that are specific to the producer. Removed this option and now we just retry up to sasl.login.retry.backoff.max.ms. > 3. "sasl.login.read.timeout.ms" : we have two types of kafka timeouts, " > default.api.timeout.ms" and "request.timeout.ms". Is this similar to any of > the two or is it different? If similar to one of the existing ones, we > should name it similarly. This is specifically for the setReadTimeout on java.net.URLConnection when making the call to the OAuth/OIDC provider to retrieve the token. So it is SASL specific because reading the response from the external OAuth/OIDC provider (likely over WAN) may require a longer timeout. > 4. "sasl.login.connect.timeout.ms": is this the equivalent of " > socket.connection.setup.timeout.ms" in Kafka? I am unsure why we chose such > a long name, "connect.timeout.ms" would have been a lot better. However, if > it is similar, then we may want to follow the same naming convention. This is for the setConnectTimeout on java.net.URLConnection, similar to the above. > 5. Should there be a "connect.max.timeout.ms" too? AFAIK, we don't have that level of control per our use of URLConnection. > 6. What are the compatibility guarantees offered by the > "OAuthCompatibilityTest" CLI tool? Also, can we adjust the name so it's > clear that it's a Command versus a test suite? I changed the name to OAuthCompatibilityTool. Can you elaborate on what compatibility guarantees you'd like to see listed? I may just be misunderstanding the request. Thanks, Kirk > > Thanks! > > Ismael > > On Mon, Sep 27, 2021 at 10:20 AM Kirk True <k...@mustardgrain.com> wrote: > > > Hi all! > > > > I'd like to start a vote for KIP-768 that allows Kafka to connect to an > > OAuth/OIDC identity provider for authentication and token retrieval: > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=186877575 > > > > Thanks! > > Kirk >
