New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@tzulitai
Contributor

tzulitai commented May 17, 2017

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).

@sunjincheng121

Hi @tzulitai Thanks for the PR. I only left a comment.

Thanks,
SunJincheng

return Collections.emptyList();
}
return Arrays.asList(value.split(","));

This comment has been minimized.

@sunjincheng121

sunjincheng121 May 17, 2017

Contributor

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

@sunjincheng121

sunjincheng121 May 17, 2017

Contributor

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

This comment has been minimized.

@tzulitai

tzulitai May 17, 2017

Contributor

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

@tzulitai

tzulitai May 17, 2017

Contributor

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

This comment has been minimized.

@tzulitai

tzulitai May 17, 2017

Contributor

Addressed!

@tzulitai

tzulitai May 17, 2017

Contributor

Addressed!

This comment has been minimized.

@EronWright

EronWright May 17, 2017

Contributor

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

@EronWright

EronWright May 17, 2017

Contributor

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

This comment has been minimized.

@sunjincheng121

sunjincheng121 May 18, 2017

Contributor

@EronWright your suggestion in some situations work well, but some situations are not. for example:

  1. new StringTokenizer(" a, b, c d, e ", ", ") ==> ["a", "b", "c", "d", "e"]
  2. new StringTokenizer(" a, b, c d, e ", ",") ==> ["a:, "b"," c d", "e"]
    But the correct result is `["a", "b", "c d", "e"].

So 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

@sunjincheng121

sunjincheng121 May 18, 2017

Contributor

@EronWright your suggestion in some situations work well, but some situations are not. for example:

  1. new StringTokenizer(" a, b, c d, e ", ", ") ==> ["a", "b", "c", "d", "e"]
  2. new StringTokenizer(" a, b, c d, e ", ",") ==> ["a:, "b"," c d", "e"]
    But the correct result is `["a", "b", "c d", "e"].

So 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

This comment has been minimized.

@EronWright

EronWright May 18, 2017

Contributor

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.

@EronWright

EronWright May 18, 2017

Contributor

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.

@tzulitai

This comment has been minimized.

Show comment
Hide comment
@tzulitai

tzulitai May 18, 2017

Contributor

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.

Contributor

tzulitai commented May 18, 2017

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.

@sunjincheng121

This comment has been minimized.

Show comment
Hide comment
@sunjincheng121

sunjincheng121 May 18, 2017

Contributor

+1 to merged.

Contributor

sunjincheng121 commented May 18, 2017

+1 to merged.

@tzulitai

This comment has been minimized.

Show comment
Hide comment
@tzulitai

tzulitai May 19, 2017

Contributor

Merging to 1.3 and master

Contributor

tzulitai commented May 19, 2017

Merging to 1.3 and master

tzulitai added a commit to tzulitai/flink that referenced this pull request May 19, 2017

@asfgit asfgit closed this in ffa9aa0 May 19, 2017

asfgit pushed a commit that referenced this pull request May 19, 2017

jinglining pushed a commit to jinglining/flink that referenced this pull request May 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment