Sorry about this, thanks for fixing.

GitHub user tzulitai opened a pull request:

https://github.com/apache/flink/pull/3928

FLINK-6608 [security, config] Relax Kerberos login contexts parsing

This PR allows whitespaces in the list of configured Kerberos login contexts.
It generally covers some over scenarios as well (covered in the additional parsing test in `SecurityUtilsTest`).

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/tzulitai/flink FLINK-6608

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/flink/pull/3928.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #3928

commit c7fd98d2fde681cef2943c5772a93e20d162fc88
Author: Tzu-Li (Gordon) Tai <tzulitai@apache.org>
Date: 2017-05-17T08:00:37Z

FLINK-6608 [security, config] Relax Kerberos login contexts parsing

Github user sunjincheng121 commented on a diff in the pull request:

https://github.com/apache/flink/pull/3928#discussion_r117001581

— Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
@@ -230,10 +231,21 @@ private void validate() {
}

private static List<String> parseList(String value) {

• if(value == null) {
  + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
• return Arrays.asList(value.split(","));
  • • End diff –

I think can we using regular expression: `return Arrays.asList(value.replaceAll("\\s*,
s*", ",").split(","));`

Github user tzulitai commented on a diff in the pull request:

https://github.com/apache/flink/pull/3928#discussion_r117003024

— Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
@@ -230,10 +231,21 @@ private void validate() {
}

private static List<String> parseList(String value) {

• if(value == null) {
  + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
• return Arrays.asList(value.split(","));
  • • End diff –

Ah, right, thanks for the review and suggestion Will use regex instead.

Github user tzulitai commented on a diff in the pull request:

https://github.com/apache/flink/pull/3928#discussion_r117009648

— Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
@@ -230,10 +231,21 @@ private void validate() {
}

private static List<String> parseList(String value) {

• if(value == null) {
  + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
• return Arrays.asList(value.split(","));
  • • End diff –

Addressed!

Github user EronWright commented on a diff in the pull request:

https://github.com/apache/flink/pull/3928#discussion_r117068349

— Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
@@ -230,10 +231,21 @@ private void validate() {
}

private static List<String> parseList(String value) {

• if(value == null) {
  + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
• return Arrays.asList(value.split(","));
  • • End diff –

An alternative to consider:
`Collections.list(new StringTokenizer(",X, ,,Y , Z,", ", "))` produces `[X, Y, Z]`.

Github user sunjincheng121 commented on a diff in the pull request:

https://github.com/apache/flink/pull/3928#discussion_r117142993

— Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
@@ -230,10 +231,21 @@ private void validate() {
}

private static List<String> parseList(String value) {

• if(value == null) {
  + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
• return Arrays.asList(value.split(","));
  • • End diff –

@EronWright your suggestion will be work. But I'd like using regular expression. The JDK DOC also has the same recommend:
`StringTokenizer is a legacy class that is retained for compatibility reasons although its use is discouraged in new code. It is recommended that anyone seeking this functionality use the split method of String or the java.util.regex package instead.`
Please see: http://docs.oracle.com/javase/7/docs/api/java/util/StringTokenizer.html
What do you think ? @EronWright @tzulitai
Best,
SunJincheng

Github user EronWright commented on a diff in the pull request:

https://github.com/apache/flink/pull/3928#discussion_r117144044

— Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
@@ -230,10 +231,21 @@ private void validate() {
}

private static List<String> parseList(String value) {

• if(value == null) {
  + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
• return Arrays.asList(value.split(","));
  • • End diff –

The wisdom of that paragraph has been debated extensively on stackoverflow. I'll just say that the `StringTokenizer` is not deprecated and, in my opinion, fits this scenario uniquely well. Go ahead either way.

Github user sunjincheng121 commented on a diff in the pull request:

https://github.com/apache/flink/pull/3928#discussion_r117146767

— Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
@@ -230,10 +230,15 @@ private void validate() {
}

private static List<String> parseList(String value) {

• if(value == null) {
  + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
• return Arrays.asList(value.split(","));
  +
  + return Arrays.asList(value
  + .replaceAll("\\s*,
  s*", ",") // remove whitespaces surrounding commas
  + .replaceAll(",,", ",") // remove empty entries
  • • End diff –

Hi, @tzulitai `.replaceAll("\\s*,
s*", ",").replaceAll(",,", ",") ` can not deal with `" a, b,,, c d, e "`
I think we can using the expression as follows:
`str.trim().replaceAll("(\\s*,
s*)", ",")` OR `str.replace("/^\\s+|\\s+$/g","").replaceAll("(\\s*,
s*)+", ",");`

Github user tzulitai commented on a diff in the pull request:

https://github.com/apache/flink/pull/3928#discussion_r117205738

— Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
@@ -230,10 +230,15 @@ private void validate() {
}

private static List<String> parseList(String value) {

• if(value == null) {
  + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
• return Arrays.asList(value.split(","));
  +
  + return Arrays.asList(value
  + .replaceAll("\\s*,
  s*", ",") // remove whitespaces surrounding commas
  + .replaceAll(",,", ",") // remove empty entries
  • • End diff –

Thanks again, that makes sense

Github user tzulitai commented on the issue:

https://github.com/apache/flink/pull/3928

Thanks for the reviews @sunjincheng121 @EronWright.
I personally prefer to stick with the regex approach. But really either way is fine

Don't really expect any test errors on this, but doing one final run just to be safe, and then merging.

Github user sunjincheng121 commented on the issue:

https://github.com/apache/flink/pull/3928

+1 to merged.

Github user tzulitai commented on the issue:

https://github.com/apache/flink/pull/3928

Merging to 1.3 and master

Github user asfgit closed the pull request at:

https://github.com/apache/flink/pull/3928

Fixed for 1.3 via 1a43badcd2e7893819f6aba9e7cd7688176efc58.
Fixed for master via ffa9aa0ee08abeb2e167fc8d343b4522ba3d2ce2.