KAFKA-4580: Use sasl.jaas.config for some system tests by rajinisivaram · Pull Request #2323 · apache/kafka

rajinisivaram · 2017-01-06T10:14:55Z

Switched console_consumer, verifiable_consumer and verifiable_producer to use new sasl.jaas_config property instead of static JAAS configuration file when used with SASL_PLAINTEXT.

rajinisivaram · 2017-01-06T10:15:37Z

System test run started: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/649/

asfbot · 2017-01-06T10:55:01Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/570/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-01-06T11:47:10Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/569/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-01-06T12:10:49Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/568/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-01-09T13:56:31Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/626/
Test PASSed (JDK 8 and Scala 2.11).

rajinisivaram · 2017-01-09T14:02:42Z

Fixed a typo. Tests are running here: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/650/

asfbot · 2017-01-09T15:00:35Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/625/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-01-09T15:12:11Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/624/
Test FAILed (JDK 7 and Scala 2.10).

rajinisivaram · 2017-01-09T21:04:39Z

System test run https://jenkins.confluent.io/job/system-test-kafka-branch-builder/650/ completed with two failures that were unrelated : ReplicationTest.test_replication_with_broker_failure reported in KAFKA-4558 and StreamsSmokeTest.test_streams reported in KAFKA-4551.

ewencp · 2017-01-16T05:12:22Z

Maybe not critical, but can we rearrange this to use the existing single client_config method? Seems like we could potentially add just the node parameter, default None. I'm a bit unclear why our existing paths all use static_jaas_conf=True but the client_config_setup path uses static_jaas_conf = self.has_sasl and self.has_ssl -- since both has_ssl and has_sasl existed before, they don't seem like the right criteria (but I also haven't really paid attention to the relevant KIPs...). If there's a good reason for the difference, I think it should probably be clearer, either in naming, arguments triggering dynamic JAAS configs, or at least comments.

There is a comment in client_config_setup where static_jaas_conf is set that explains why it is set to a different value. SASL_SSL is self.has_sasl and self.has_ssl. Let me know if the comment needs to be rewritten. Basically, any criteria could have been chosen to set static for some and dynamic for others. I just chose SSL/non-SSL. The intention was to use existing tests to test a new KIP without adding to the time taken for running system tests by modifying some tests to use the new dynamic jaas config.

I have switched to using a single method and expanded the comment a bit.

ewencp · 2017-01-16T05:13:58Z

I am not sure if it would be better, but it feels like this is the kind of tweaking of templates that should be done in the templates themselves based on forwarding parameters to the template via the class members. In other words, if there are bits that should be swapped out, maybe we should encode those as methods on this class or simply conditionals in the template instead of doing regex search & replace here.

@ewencp Thank you for the review. I have updated the code.

asfbot · 2017-01-16T15:10:34Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/912/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-01-16T15:10:39Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/914/
Test PASSed (JDK 8 and Scala 2.11).

ijuma

Looks fine to me apart from one comment. Would be good to run the system tests branch builder once again after review comments have been addressed.

ijuma · 2017-01-16T15:55:54Z

Is this intend right?

@ijuma Thank you for the review. I had used tab instead of space, fixed and started a new system test run: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/672/

asfbot · 2017-01-16T17:09:21Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/916/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-01-16T18:31:34Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/914/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-01-16T19:21:52Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/914/
Test FAILed (JDK 8 and Scala 2.12).

ijuma · 2017-01-17T09:54:17Z

@rajinisivaram, there were a number of system test failures that could be related. Could you have a look? If you think it's an environmental issue, you could re-run just one of the security related tests that failed.

rajinisivaram · 2017-01-17T10:45:45Z

@ijuma Thank you for checking (I had forgotten all about the system test run). That was my mistake, a missing space in the property. I have fixed it and started another run (https://jenkins.confluent.io/job/system-test-kafka-branch-builder/673/)

asfbot · 2017-01-17T10:48:37Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/925/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-01-17T11:31:42Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/925/
Test PASSed (JDK 8 and Scala 2.12).

ijuma · 2017-01-17T12:58:29Z

It seems like there are no conflicts, but it would probably be good to rebase this branch now that 55abe65 has been merged.

asfbot · 2017-01-17T13:43:31Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/927/
Test FAILed (JDK 8 and Scala 2.11).

rajinisivaram · 2017-01-17T16:06:16Z

@ijuma Thank you, I have rebased and started a test run of just the replication tests with the rebased code: https://jenkins.confluent.io/job/system-test-kafka-branch-builder-2/175/. The earlier run of all the tests has not yet completed, but it is looking ok so far.

asfbot · 2017-01-17T16:50:50Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/931/
Test PASSed (JDK 8 and Scala 2.12).

rajinisivaram · 2017-01-17T18:02:31Z

The system tests from this morning had two unrelated streams test failures (https://jenkins.confluent.io/job/system-test-kafka-branch-builder/673/), all security related tests passed. Replication tests from the rebased code passed (https://jenkins.confluent.io/job/system-test-kafka-branch-builder-2/175/)

asfbot · 2017-01-17T18:09:24Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/931/
Test FAILed (JDK 7 and Scala 2.10).

ijuma

LGTM, merging to trunk and 0.10.2 branches.

Switched console_consumer, verifiable_consumer and verifiable_producer to use new sasl.jaas_config property instead of static JAAS configuration file when used with SASL_PLAINTEXT. Author: Rajini Sivaram <rajinisivaram@googlemail.com> Reviewers: Ewen Cheslack-Postava <ewen@confluent.io>, Ismael Juma <ismael@juma.me.uk> Closes #2323 from rajinisivaram/KAFKA-4580 (cherry picked from commit 3f6c4f6) Signed-off-by: Ismael Juma <ismael@juma.me.uk>

asfbot · 2017-01-17T19:03:00Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/933/
Test FAILed (JDK 8 and Scala 2.11).

Switched console_consumer, verifiable_consumer and verifiable_producer to use new sasl.jaas_config property instead of static JAAS configuration file when used with SASL_PLAINTEXT. Author: Rajini Sivaram <rajinisivaram@googlemail.com> Reviewers: Ewen Cheslack-Postava <ewen@confluent.io>, Ismael Juma <ismael@juma.me.uk> Closes apache#2323 from rajinisivaram/KAFKA-4580

rajinisivaram force-pushed the KAFKA-4580 branch from dbe6d36 to 5622b77 Compare January 9, 2017 13:16

ewencp reviewed Jan 16, 2017

View reviewed changes

rajinisivaram force-pushed the KAFKA-4580 branch from efead4c to 2565004 Compare January 16, 2017 14:22

ijuma reviewed Jan 16, 2017

View reviewed changes

rajinisivaram added 4 commits January 17, 2017 15:59

KAFKA-4580: Use sasl.jaas.config for some system tests

50d7478

KAFKA-4580: Address review comments

912f9e5

KAFKA-4580: Replace tab with space

5c665cc

KAFKA-4580: Add space separator in JAAS config prop

7c89133

rajinisivaram force-pushed the KAFKA-4580 branch from 366bf43 to 7c89133 Compare January 17, 2017 16:02

ijuma approved these changes Jan 17, 2017

View reviewed changes

asfgit closed this in 3f6c4f6 Jan 17, 2017

Conversation

rajinisivaram commented Jan 6, 2017

Uh oh!

rajinisivaram commented Jan 6, 2017

Uh oh!

asfbot commented Jan 6, 2017

Uh oh!

asfbot commented Jan 6, 2017

Uh oh!

asfbot commented Jan 6, 2017

Uh oh!

asfbot commented Jan 9, 2017

Uh oh!

rajinisivaram commented Jan 9, 2017

Uh oh!

asfbot commented Jan 9, 2017

Uh oh!

asfbot commented Jan 9, 2017

Uh oh!

rajinisivaram commented Jan 9, 2017

Uh oh!

ewencp Jan 16, 2017

Choose a reason for hiding this comment

Uh oh!

rajinisivaram Jan 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ewencp Jan 16, 2017

Choose a reason for hiding this comment

Uh oh!

rajinisivaram Jan 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jan 16, 2017

Uh oh!

asfbot commented Jan 16, 2017

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma Jan 16, 2017

Choose a reason for hiding this comment

Uh oh!

rajinisivaram Jan 16, 2017

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jan 16, 2017

Uh oh!

asfbot commented Jan 16, 2017

Uh oh!

asfbot commented Jan 16, 2017

Uh oh!

ijuma commented Jan 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajinisivaram commented Jan 17, 2017

Uh oh!

asfbot commented Jan 17, 2017

Uh oh!

asfbot commented Jan 17, 2017

Uh oh!

ijuma commented Jan 17, 2017

Uh oh!

asfbot commented Jan 17, 2017

Uh oh!

rajinisivaram commented Jan 17, 2017

Uh oh!

asfbot commented Jan 17, 2017

Uh oh!

rajinisivaram commented Jan 17, 2017

Uh oh!

asfbot commented Jan 17, 2017

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jan 17, 2017

Uh oh!

rajinisivaram Jan 16, 2017 •

edited

Loading

rajinisivaram Jan 16, 2017 •

edited

Loading

ijuma commented Jan 17, 2017 •

edited

Loading