Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@kafka.apache.org
In-Reply-To: <CAOJcB3_rCtqTO9B=hwq_z-iybKzZfgqfQWAA2LEViQuaC3Q6ew@mail.gmail.com>
To: dev@kafka.apache.org
Subject: Re: [DISCUSS] KIP-152 - Improve diagnostics for SASL authentication
 failures
From: "Vahid S Hashemian" <vahidhashemian@us.ibm.com>
Date: Fri, 18 Aug 2017 09:56:36 -0700
References: <CAOJcB3-yz9u=8R8dG6dbzoLH5_8fTkdh-nAyCyGSsfpj1gpc0w@mail.gmail.com>
 <CAD5tkZZdxbfNWA=UqndfLH-6LsLMsqKifAZHYA9dw5-VisiQGw@mail.gmail.com>
 <CAOJcB3_pOu-0is9sHZC2d-YiQvDATZ7VpZiMpyyh_MnBzZVEAw@mail.gmail.com>
 <CAD5tkZadazb1TZ=E2ZxLhQJfHXVw3Hc+vx=nnQ1R7wZ8=oB1Mw@mail.gmail.com>
 <CAOJcB38wS=HBtatThWskiGA=kMCXW_OwmCk4OjrDuB6k0uAf=Q@mail.gmail.com>
 <CAD5tkZb+t4-K_fQGB=Y-wPmheEjbGR09ufe1_Z8g1hjayttrDQ@mail.gmail.com>
 <CAOJcB3_rCtqTO9B=hwq_z-iybKzZfgqfQWAA2LEViQuaC3Q6ew@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/alternative; boundary="=_alternative 005D11B288258180_="
Message-Id: <OF7DDA56F7.4EF1C87E-ON00258180.005CEBA9-88258180.005D125C@notes.na.collabserv.com>
archived-at: Fri, 18 Aug 2017 16:56:46 -0000

--=_alternative 005D11B288258180_=
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="US-ASCII"

Hi Rajini,

Thanks for the KIP. It looks good to me.
It would be great if it can make it to 1.0.0.

--Vahid


From:   Rajini Sivaram <rajinisivaram@gmail.com>
To:     dev <dev@kafka.apache.org>
Date:   08/15/2017 01:23 PM
Subject:        Re: [DISCUSS] KIP-152 - Improve diagnostics for SASL=20
authentication failures


I have updated the KIP based on the discussions so far. It will be good if
we can get some more feedback so that this can be implemented for 1.0.0.


Thanks,

Rajini


On Thu, May 4, 2017 at 10:22 PM, Ismael Juma <ismael@juma.me.uk> wrote:

> Hi Rajini,
>
> I think we were talking about slightly different things. I was just
> referring to the fact that there are cases where we throw an
> AuthorizationException back to the user without retrying from various
> methods (poll, commitSync, etc).
>
> As you said, my initial preference was for not retrying at all because=20
it
> is what you want in the common case of a misconfigured application. I
> hadn't considered credential updates for authenticators that rely on
> eventual consistency. Thinking about it some more, it seems like this
> should be solved by the authenticator implementation as well. For=20
example,
> it could refresh the cached data for a user if authentication failed (a
> good implementation would be a bit more involved to avoid going to the
> underlying data source too often).
>
> Given that, not retrying sounds good to me.
>
> Ismael
>
> On Thu, May 4, 2017 at 4:04 PM, Rajini Sivaram <rajinisivaram@gmail.com>
> wrote:
>
> > Hi Ismael,
> >
> > I thought the blocking waits in the producer and consumer are always
> > related to retrying for metadata. So an authorization exception that
> > impacts this wait can only be due to Describe authorization failure -
> that
> > always retries?
> >
> > I agree that connecting to different brokers when authentication fails
> with
> > one is not desirable. But I am not keen on retrying with a suitable
> backoff
> > until timeout either. Because that has the same problem as the=20
scenario
> > that you described. The next metadata request could be to broker-1 to
> which
> > authentication succeeds and subsequent produce/consume  to broker-0=20
could
> > still fail.
> >
> > How about we just fail fast if one authentication fails - I think that =

is
> > what you were suggesting in the first place? We don't need to blackout
> any
> > nodes beyond the reconnect backoff interval. Applications can still=20
retry
> > if they want to. In the case of credential updates, it will be up to=20
the
> > application to retry. During regular operation, a misconfigured
> application
> > fails fast with a meaningful exception. What do you think?
> >
> > Regards,
> >
> > Rajini
> >
> >
> > On Thu, May 4, 2017 at 3:01 PM, Ismael Juma <ismael@juma.me.uk> wrote:
> >
> > > H Rajini,
> > >
> > > Comments inline.
> > >
> > > On Thu, May 4, 2017 at 2:29 PM, Rajini Sivaram <
> rajinisivaram@gmail.com>
> > > wrote:
> > >
> > > > Hi Ismael,
> > > >
> > > > Thank you for reviewing the KIP.
> > > >
> > > > An authenticated client that is not authorized to access a topic=20
is
> > never
> > > > told that the operation was not authorized. This is to prevent the
> > client
> > > > from finding out if the topic exists by sending an unauthorized
> > request.
> > > So
> > > > in this case, the client will retry metadata requests with the
> > configured
> > > > backoff until it times out.
> > >
> > >
> > > This is true if the user does not have Describe permission. If the=20
user
> > has
> > > Describe access and no Read or Write access, then the user is=20
informed
> > that
> > > the operation was not authorized.
> > >
> > >
> > > > Another important distinction for authorization failures is that=20
the
> > > > connection is not terminated.
> > > >
> > > > For unauthenticated clients, we do want to inform the client that
> > > > authentication failed. The connection is terminated by the broker.
> > > > Especially if the client is using SASL=5FSSL, we really do want to
> avoid
> > > > reconnections that result in unnecessary expensive handshakes. So=20
we
> > want
> > > > to return an exception to the user with minimal retries.
> > > >
> > >
> > > Agreed.
> > >
> > > I was thinking that it may be useful to try more than one broker for
> the
> > > > case where brokers are being upgraded and some brokers haven't yet
> seen
> > > the
> > > > latest credentials. I suppose I was thinking that at the moment we
> keep
> > > on
> > > > retrying every broker forever in the consumer and suddenly if we=20
stop
> > > > retrying altogether, it could potentially lead to some unforeseen
> > timing
> > > > issues. Hence the suggestion to try every broker once.
> > > >
> > >
> > > I see. Retrying forever is a side-effect of auto-topic creation, but
> it's
> > > something we want to move away from. As mentioned, we actually don't
> > retry
> > > at all if the user has Describe permission.
> > >
> > > Broker upgrades could be fixed by ensuring that the latest=20
credentials
> > are
> > > loaded before the broker starts serving requests. More problematic=20
is
> > > dealing with credential updates. This is another distinction when
> > compared
> > > to authorization.
> > >
> > > I am not sure if trying different brokers really helps us though.=20
Say,
> we
> > > fail to authenticate with broker 0 and then we succeed with broker=20
1.
> > This
> > > helps with metadata requests, but we will be in trouble when we try=20
to
> > > produce or consume to broker 0 (because it's the leader of some
> > > partitions). So maybe we just want to retry with a suitable backoff
> > until a
> > > timeout?
> > >
> > > Yes, I agree that blacking out nodes forever isn't a good idea. When =

we
> > > > throw AuthenticationFailedException for the current operation or=20
if
> > > > authentication to another broker succeeds, we can clear the=20
blackout
> so
> > > > that any new request from the client can attempt reconnection=20
after
> the
> > > > reconnect backoff period as they do now.
> > > >
> > >
> > > Yes, that would be better if we decide that connecting to different
> > brokers
> > > is worthwhile for the requests that can be sent to any broker.
> > >
> > > Ismael
> > >
> > > On Thu, May 4, 2017 at 2:29 PM, Rajini Sivaram <
> rajinisivaram@gmail.com>
> > > wrote:
> > >
> > > > Hi Ismael,
> > > >
> > > > Thank you for reviewing the KIP.
> > > >
> > > > An authenticated client that is not authorized to access a topic=20
is
> > never
> > > > told that the operation was not authorized. This is to prevent the
> > client
> > > > from finding out if the topic exists by sending an unauthorized
> > request.
> > > So
> > > > in this case, the client will retry metadata requests with the
> > configured
> > > > backoff until it times out. Another important distinction for
> > > authorization
> > > > failures is that the connection is not terminated.
> > > >
> > > > For unauthenticated clients, we do want to inform the client that
> > > > authentication failed. The connection is terminated by the broker.
> > > > Especially if the client is using SASL=5FSSL, we really do want to
> avoid
> > > > reconnections that result in unnecessary expensive handshakes. So=20
we
> > want
> > > > to return an exception to the user with minimal retries.
> > > >
> > > > I was thinking that it may be useful to try more than one broker=20
for
> > the
> > > > case where brokers are being upgraded and some brokers haven't yet
> seen
> > > the
> > > > latest credentials. I suppose I was thinking that at the moment we
> keep
> > > on
> > > > retrying every broker forever in the consumer and suddenly if we=20
stop
> > > > retrying altogether, it could potentially lead to some unforeseen
> > timing
> > > > issues. Hence the suggestion to try every broker once.
> > > >
> > > > Yes, I agree that blacking out nodes forever isn't a good idea.=20
When
> we
> > > > throw AuthenticationFailedException for the current operation or=20
if
> > > > authentication to another broker succeeds, we can clear the=20
blackout
> so
> > > > that any new request from the client can attempt reconnection=20
after
> the
> > > > reconnect backoff period as they do now.
> > > >
> > > > Regards,
> > > >
> > > > Rajini
> > > >
> > > > On Thu, May 4, 2017 at 12:51 PM, Ismael Juma <ismael@juma.me.uk>
> > wrote:
> > > >
> > > > > Thanks Rajini. This is a good improvement. One question, the
> proposal
> > > > > states:
> > > > >
> > > > > Producer waitForMetadata and consumer ensureCoordinatorReady=20
will
> be
> > > > > > updated to throw AuthenticationFailedException if connections=20
to
> > all
> > > > > > available brokers fail authentication.
> > > > >
> > > > >
> > > > > Can you elaborate on the reason why we would treat=20
authentication
> > > > failures
> > > > > differently from authorization failures? It would be good to
> > understand
> > > > > under which scenario it would be beneficial to try all the=20
brokers
> > (it
> > > > > seems that the proposal also suggests blacking out brokers
> > permanently
> > > if
> > > > > we fail authentication, so that could also eventually cause
> issues).
> > > > >
> > > > > Ismael
> > > > >
> > > > >
> > > > > On Thu, May 4, 2017 at 12:37 PM, Rajini Sivaram <
> > > rajinisivaram@gmail.com
> > > > >
> > > > > wrote:
> > > > >
> > > > > > Hi all,
> > > > > >
> > > > > > I have created a KIP to improve diagnostics for SASL
> authentication
> > > > > > failures and reduce retries and blocking when authentication
> fails:
> > > > > >
> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-152+-+
> > > > > > Improve+diagnostics+for+SASL+authentication+failures
> > > > > >
> > > > > > Comments and suggestions are welcome.
> > > > > >
> > > > > > Thank you...
> > > > > >
> > > > > > Regards,
> > > > > >
> > > > > > Rajini
> > > > > >
> > > > >
> > > >
> > >
> >
>


--=_alternative 005D11B288258180_=--