You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by tzulitai <gi...@git.apache.org> on 2017/05/17 08:08:22 UTC
[GitHub] flink pull request #3928: [FLINK-6608] [security, config] Relax Kerberos log...
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 <tz...@apache.org>
Date: 2017-05-17T08:00:37Z
[FLINK-6608] [security, config] Relax Kerberos login contexts parsing
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3928: [FLINK-6608] [security, config] Relax Kerberos log...
Posted by sunjincheng121 <gi...@git.apache.org>.
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*)+", ",");`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3928: [FLINK-6608] [security, config] Relax Kerberos log...
Posted by EronWright <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3928: [FLINK-6608] [security, config] Relax Kerberos log...
Posted by EronWright <gi...@git.apache.org>.
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]`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3928: [FLINK-6608] [security, config] Relax Kerberos login cont...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/3928
Merging to 1.3 and master
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3928: [FLINK-6608] [security, config] Relax Kerberos login cont...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3928
+1 to merged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3928: [FLINK-6608] [security, config] Relax Kerberos log...
Posted by tzulitai <gi...@git.apache.org>.
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!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3928: [FLINK-6608] [security, config] Relax Kerberos log...
Posted by tzulitai <gi...@git.apache.org>.
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 :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3928: [FLINK-6608] [security, config] Relax Kerberos log...
Posted by sunjincheng121 <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3928: [FLINK-6608] [security, config] Relax Kerberos log...
Posted by sunjincheng121 <gi...@git.apache.org>.
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(","));`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3928: [FLINK-6608] [security, config] Relax Kerberos log...
Posted by tzulitai <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3928: [FLINK-6608] [security, config] Relax Kerberos log...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/3928
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3928: [FLINK-6608] [security, config] Relax Kerberos login cont...
Posted by tzulitai <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---