Sorry....by out of band, It could be done once per writer or even in the connector once...but I think we're in agreement Drob, Mike.
On Fri, Aug 19, 2016 at 2:30 PM, Marc P. <[email protected]> wrote: > Funny, exactly the same thing I mentioned to Josh last night. Are you > watching us? > > In something I"m doing now I'm making the thrift calls and modeled it > after what the client code does; however, > once I removed the authenticateUser my throughput increased by 25 per > cent. In my trace table it was by and large the greatest audited call. > > I think we could change how that works and potentially keep that out of > band. The fail fast is great if the user doesn't have permissions to that > table and may save the server some work...but as it is now, I'm always > authenticated and I'm causing more work for the server. Perhaps we let the > natural exception that's thrown stop the client ( if it's being a good > client ). I think the 25 per cent improvement was well worth the > alternative risk. > > On Fri, Aug 19, 2016 at 2:16 PM, Mike Drob <[email protected]> wrote: > >> Devs, >> >> I was recently running a 1.7.2 cluster under a heavy write workload and >> noticed a _lot_ of audit messages showing up in the logs. After some >> digging, I found that one of the causes is the following call chain: >> >> TabletServerBatchWriter::sendMutationsToTabletServer >> calls TabletServer.ThriftClientHandler::startUpdate >> calls SecurityOperation::authenticateUser <-- always return true >> calls SecurityOperation::canAskAboutUser <-- always return true >> calls AuditedSecurityOperation::canPerformSystemActions <-- fails; logs >> "action: performSystemAction" >> calls AuditedSecurityAction::authenticate <-- succeeds; logs "action: >> authenticate;" >> >> There are two separate but related problems here, I think, that I want to >> confirm. >> >> When startUpdate calls security.authenticateUser(credentials, >> credentials); >> there is the comment that this is done to "// Make sure user is real" >> presumably to fail fast. This makes sense, but does it actually work? In >> authenticateUser and canAskAboutUser we end up comparing the two >> credentials to each other and this will always succeed because they're the >> same object. I expect the write fails later in the TabletServer code, >> possibly in the UpdateSession, but I haven't been able to untangle this >> part yet. >> >> Second, I think it makes sense to switch the order of the OR in >> 'canAskAboutUser.' If this is in the write path, then it seems like the >> most common case would be users asking about themselves, so we should >> check >> that before checking if a user is an admin. This also has the advantage of >> removing two uninformative audit log lines. We won't have an audit message >> that somebody wrote data, but we don't have that currently anyway. >> >> I did some visual spot checking and this looks like an issue in master >> also, but I haven't run that code to find out. >> >> Thoughts? >> >> Mike >> > >
