KAFKA-2949: Make EndToEndAuthorizationTest replicated.

[fpj]

No description provided.

[ijuma]

Nitpick: I think there is an extra space in both lines above.

[ijuma]

I think we should change the producer ack config so that it waits for the data to be replicated to all the brokers, at the moment it's:

this.producerConfig.setProperty(ProducerConfig.ACKS_CONFIG, "1")

[fpj]

@ijuma Testing all servers sounds better because the ACLs are propagated asynchronously via ZooKeeper, so I've made the change.

It turns out that the acks configuration where you commented wasn't really doing anything. The acks mode was hardwired in IntegrationTestHarness, so I've removed it and I've let it use the default of -1.

[ijuma]

This PR LGTM.

Regarding how IntegrationTestHarness creates the producer and consumer via TestUtils.createNewProducer and TestUtils.createNewConsumer (changed as part of the introduction of EndToEndAuthorizationTest), I think this introduces a bit of surprising behaviour like we saw here (where the acks configured in the subclass was ignored). I think it would be good to rethink this in the future.

[ijuma]

Just to confirm: the test fails if topicBrokerReadAclArgs is not added right?

[fpj]

@ijuma yes, some of the test cases are supposed to fail in the case we remove that ACL. testNoProduceAcl doesn't fail, though, because the authorization to produced is refused before anything else. Interestingly, the failure is that there aren't enough replicas available:

org.apache.kafka.common.errors.NotEnoughReplicasAfterAppendException: Messages are written to 
the log, but to fewer in-sync replicas than required.

because it isn't possible to replicate without authorization, which we can see in the logs:

ERROR [ReplicaFetcherThread-0-1], Error for partition [e2etopic,0] to broker 
1:org.apache.kafka.common.errors.AuthorizationException: Topic authorization failed. 
(kafka.server.ReplicaFetcherThread:97)

Although not an issue foe this PR, it doesn't sound like a great way to expose the problem to the application in the case ACLs haven't been properly set, but at the same time, it is not clear how much of internals we want to expose. It is worth thinking about a better way to expose these errors, though.

[ijuma]

Thanks @fpj, good to confirm. Good question regarding how to expose the error. Because the replication error is a server misconfiguration, my feeling is that we don't want to leak that to the clients. In that sense, NotEnoughReplicas seems fine. What I think we should do is to have a mechanism to validate the server security config (could be a tool or a start-up option, not sure).

[fpj]

@ijuma

my feeling is that we don't want to leak that to the clients

I'm not saying we leak it to the client, but just that the message right now gives little guidance to the client about how to solve the problem. I don't think it would be a problem to say something like "there is an authorization issue with your cluster, go talk to your admin." or something like that. But yeah, a topic for a different issue.

[junrao]

Thanks for the patch. LGTM