You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apex.apache.org by pradeepdalvi <gi...@git.apache.org> on 2016/07/11 23:24:33 UTC
[GitHub] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
GitHub user pradeepdalvi opened a pull request:
https://github.com/apache/apex-core/pull/357
APEXCORE-488: Issues in SSL communication with StrAM
- Fixed Application Master trackingURL
- StramAgent shall not assume always HTTP
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pradeepdalvi/apex-core APEXCORE-488.ssl-issues
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/apex-core/pull/357.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 #357
----
commit d8b64f323e2e9d59dda7935807893a95802f328a
Author: Pradeep A. Dalvi <pr...@apache.org>
Date: 2016-07-11T22:59:32Z
APEXCORE-488: Issues in SSL communication with StrAM
- Fixed Application Master trackingURL
- StramAgent shall not assume always HTTP
----
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by pradeepdalvi <gi...@git.apache.org>.
Github user pradeepdalvi closed the pull request at:
https://github.com/apache/apex-core/pull/357
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by sandeshh <gi...@git.apache.org>.
Github user sandeshh commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70355804
--- Diff: engine/src/main/java/com/datatorrent/stram/client/StramAgent.java ---
@@ -204,7 +204,14 @@ private UriBuilder getStramWebURIBuilder(WebServicesClient webServicesClient, St
if (info != null) {
//ws = wsClient.resource("http://" + info.appMasterTrackingUrl).path(WebServices.PATH).path(info.version).path("stram");
// the filter should convert to the right version
- ub = UriBuilder.fromUri("http://" + info.appMasterTrackingUrl).path(WebServices.PATH).path(WebServices.VERSION).path("stram");
+ String url;
+ if (!info.appMasterTrackingUrl.startsWith("http://")
+ && !info.appMasterTrackingUrl.startsWith("https://")) {
+ url = "http://" + info.appMasterTrackingUrl;
--- End diff --
If it is a secure cluster, then fallback should be https?
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by PramodSSImmaneni <gi...@git.apache.org>.
Github user PramodSSImmaneni commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70553112
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingAppMasterService.java ---
@@ -607,14 +608,15 @@ protected void serviceStart() throws Exception
}
try {
- Configuration config = getConfig();
+ YarnConfiguration config = new YarnConfiguration(getConfig());
if (SecurityUtils.isStramWebSecurityEnabled()) {
- config = new Configuration(config);
config.set("hadoop.http.filter.initializers", StramWSFilterInitializer.class.getCanonicalName());
}
WebApp webApp = WebApps.$for("stram", StramAppContext.class, appContext, "ws").with(config).start(new StramWebApp(this.dnmgr));
LOG.info("Started web service at port: " + webApp.port());
- this.appMasterTrackingUrl = NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port();
+ String scheme = ConfigUtils.getSchemePrefix(config);
+ String hostname = NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName();
+ this.appMasterTrackingUrl = scheme + hostname + ":" + webApp.port();
--- End diff --
This is changing the contract of appMasterTrackingUrl in the web service response even though this is probably how it should have been in the first place. This would be a backwards incompatible change. Suggest to use a separate property for scheme.
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by pradeepdalvi <gi...@git.apache.org>.
Github user pradeepdalvi commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70516680
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingAppMasterService.java ---
@@ -608,13 +609,16 @@ protected void serviceStart() throws Exception
try {
Configuration config = getConfig();
+ YarnConfiguration conf = new YarnConfiguration();
--- End diff --
Configuration is needed by WebApp server. We could probably do something like `YarnConfiguration conf = StramClientUtils.getYarnConfiguration(config);`
or
`YarnConfiguration conf = new YarnConfiguration(config);`
However in both cases there is not much significant difference unless & until config is instanceof YarnConfiguration.
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by pradeepdalvi <gi...@git.apache.org>.
Github user pradeepdalvi commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70740041
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingAppMasterService.java ---
@@ -614,7 +615,16 @@ protected void serviceStart() throws Exception
}
WebApp webApp = WebApps.$for("stram", StramAppContext.class, appContext, "ws").with(config).start(new StramWebApp(this.dnmgr));
LOG.info("Started web service at port: " + webApp.port());
- this.appMasterTrackingUrl = NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port();
+ String host = NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port();
+
+ // For backward compatibility, not adding scheme in TrackingURL for non-HTTPS
+ // TODO: Remove the check in next major release and add scheme always
+ if (ConfigUtils.isSSLEnabled(config)) {
+ String scheme = ConfigUtils.getSchemePrefix(config);
--- End diff --
Or something like this:
`this.appMasterTrackingUrl = (ConfigUtils.isSSLEnabled(config) ? ConfigUtils.getSchemePrefix(config) : "") + NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port();`
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by gauravgopi123 <gi...@git.apache.org>.
Github user gauravgopi123 commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70899542
--- Diff: engine/src/main/java/com/datatorrent/stram/util/ConfigUtils.java ---
@@ -68,15 +68,30 @@ public static String getRMUsername(Configuration conf)
return principal;
}
- public static String getSchemePrefix(YarnConfiguration conf)
+ public static boolean isSSLEnabled(Configuration conf)
+ {
+ if (HttpConfig.Policy.HTTPS_ONLY == HttpConfig.Policy.fromString(
+ conf.get(YarnConfiguration.YARN_HTTP_POLICY_KEY, YarnConfiguration.YARN_HTTP_POLICY_DEFAULT))) {
+ return true;
+ }
+ return false;
+ }
+
+ public static String getSchemePrefix(Configuration conf)
{
- if (HttpConfig.Policy.HTTPS_ONLY == HttpConfig.Policy.fromString(conf.get(YarnConfiguration.YARN_HTTP_POLICY_KEY, YarnConfiguration.YARN_HTTP_POLICY_DEFAULT))) {
+ if (isSSLEnabled(conf)) {
--- End diff --
@PramodSSImmaneni : I am talking about getSchemePrefix(Configuration) method...
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70368327
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingAppMasterService.java ---
@@ -608,13 +609,16 @@ protected void serviceStart() throws Exception
try {
Configuration config = getConfig();
+ YarnConfiguration conf = new YarnConfiguration();
--- End diff --
Why is it necessary to have Configuration and YarnConfiguration? Can this code be simplified to `YarnConfiguration = StramClientUtils.getYarnConfiguration(getConfig());`
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by gauravgopi123 <gi...@git.apache.org>.
Github user gauravgopi123 commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70740902
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingAppMasterService.java ---
@@ -614,7 +615,16 @@ protected void serviceStart() throws Exception
}
WebApp webApp = WebApps.$for("stram", StramAppContext.class, appContext, "ws").with(config).start(new StramWebApp(this.dnmgr));
LOG.info("Started web service at port: " + webApp.port());
- this.appMasterTrackingUrl = NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port();
+ String host = NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port();
+
+ // For backward compatibility, not adding scheme in TrackingURL for non-HTTPS
+ // TODO: Remove the check in next major release and add scheme always
+ if (ConfigUtils.isSSLEnabled(config)) {
+ String scheme = ConfigUtils.getSchemePrefix(config);
--- End diff --
@pradeepdalvi : @vrozov had answered your question. Calling getSchemePrefix is again calling isSSLEnabled. Why is that requried
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by gauravgopi123 <gi...@git.apache.org>.
Github user gauravgopi123 commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70900700
--- Diff: engine/src/main/java/com/datatorrent/stram/util/ConfigUtils.java ---
@@ -68,15 +68,30 @@ public static String getRMUsername(Configuration conf)
return principal;
}
- public static String getSchemePrefix(YarnConfiguration conf)
+ public static boolean isSSLEnabled(Configuration conf)
+ {
+ if (HttpConfig.Policy.HTTPS_ONLY == HttpConfig.Policy.fromString(
+ conf.get(YarnConfiguration.YARN_HTTP_POLICY_KEY, YarnConfiguration.YARN_HTTP_POLICY_DEFAULT))) {
+ return true;
+ }
+ return false;
+ }
+
+ public static String getSchemePrefix(Configuration conf)
{
- if (HttpConfig.Policy.HTTPS_ONLY == HttpConfig.Policy.fromString(conf.get(YarnConfiguration.YARN_HTTP_POLICY_KEY, YarnConfiguration.YARN_HTTP_POLICY_DEFAULT))) {
+ if (isSSLEnabled(conf)) {
--- End diff --
@PramodSSImmaneni : I missed one thing. New method is needed as AppMaster has Configuration and YarnConfiguration object..
Changes look good
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70736396
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingAppMasterService.java ---
@@ -614,7 +615,16 @@ protected void serviceStart() throws Exception
}
WebApp webApp = WebApps.$for("stram", StramAppContext.class, appContext, "ws").with(config).start(new StramWebApp(this.dnmgr));
LOG.info("Started web service at port: " + webApp.port());
- this.appMasterTrackingUrl = NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port();
+ String host = NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port();
+
+ // For backward compatibility, not adding scheme in TrackingURL for non-HTTPS
+ // TODO: Remove the check in next major release and add scheme always
+ if (ConfigUtils.isSSLEnabled(config)) {
+ String scheme = ConfigUtils.getSchemePrefix(config);
--- End diff --
Agree with @gauravgopi123. It will be also good to get rid of `this`.
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by PramodSSImmaneni <gi...@git.apache.org>.
Github user PramodSSImmaneni commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70897093
--- Diff: engine/src/main/java/com/datatorrent/stram/util/ConfigUtils.java ---
@@ -68,15 +68,30 @@ public static String getRMUsername(Configuration conf)
return principal;
}
- public static String getSchemePrefix(YarnConfiguration conf)
+ public static boolean isSSLEnabled(Configuration conf)
+ {
+ if (HttpConfig.Policy.HTTPS_ONLY == HttpConfig.Policy.fromString(
+ conf.get(YarnConfiguration.YARN_HTTP_POLICY_KEY, YarnConfiguration.YARN_HTTP_POLICY_DEFAULT))) {
+ return true;
+ }
+ return false;
+ }
+
+ public static String getSchemePrefix(Configuration conf)
{
- if (HttpConfig.Policy.HTTPS_ONLY == HttpConfig.Policy.fromString(conf.get(YarnConfiguration.YARN_HTTP_POLICY_KEY, YarnConfiguration.YARN_HTTP_POLICY_DEFAULT))) {
+ if (isSSLEnabled(conf)) {
--- End diff --
Are you good with that explanation @gauravgopi123
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by sandeshh <gi...@git.apache.org>.
Github user sandeshh commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70355340
--- Diff: engine/src/main/java/com/datatorrent/stram/client/StramAgent.java ---
@@ -204,7 +204,14 @@ private UriBuilder getStramWebURIBuilder(WebServicesClient webServicesClient, St
if (info != null) {
//ws = wsClient.resource("http://" + info.appMasterTrackingUrl).path(WebServices.PATH).path(info.version).path("stram");
// the filter should convert to the right version
- ub = UriBuilder.fromUri("http://" + info.appMasterTrackingUrl).path(WebServices.PATH).path(WebServices.VERSION).path("stram");
+ String url;
+ if (!info.appMasterTrackingUrl.startsWith("http://")
+ && !info.appMasterTrackingUrl.startsWith("https://")) {
+ url = "http://" + info.appMasterTrackingUrl;
--- End diff --
It should be logged, saying falling back to http
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/apex-core/pull/357
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by pradeepdalvi <gi...@git.apache.org>.
Github user pradeepdalvi commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70356242
--- Diff: engine/src/main/java/com/datatorrent/stram/client/StramAgent.java ---
@@ -204,7 +204,14 @@ private UriBuilder getStramWebURIBuilder(WebServicesClient webServicesClient, St
if (info != null) {
//ws = wsClient.resource("http://" + info.appMasterTrackingUrl).path(WebServices.PATH).path(info.version).path("stram");
// the filter should convert to the right version
- ub = UriBuilder.fromUri("http://" + info.appMasterTrackingUrl).path(WebServices.PATH).path(WebServices.VERSION).path("stram");
+ String url;
+ if (!info.appMasterTrackingUrl.startsWith("http://")
+ && !info.appMasterTrackingUrl.startsWith("https://")) {
+ url = "http://" + info.appMasterTrackingUrl;
--- End diff --
Its not falling back. When protocol scheme is not specified in Tracking URL, it shall use the default protocol scheme i.e. 'http://'
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by gauravgopi123 <gi...@git.apache.org>.
Github user gauravgopi123 commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70728942
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingAppMasterService.java ---
@@ -614,7 +615,16 @@ protected void serviceStart() throws Exception
}
WebApp webApp = WebApps.$for("stram", StramAppContext.class, appContext, "ws").with(config).start(new StramWebApp(this.dnmgr));
LOG.info("Started web service at port: " + webApp.port());
- this.appMasterTrackingUrl = NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port();
+ String host = NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port();
+
+ // For backward compatibility, not adding scheme in TrackingURL for non-HTTPS
+ // TODO: Remove the check in next major release and add scheme always
+ if (ConfigUtils.isSSLEnabled(config)) {
+ String scheme = ConfigUtils.getSchemePrefix(config);
--- End diff --
why do you need to make `ConfigUtils.getSchemePrefix` call? If `ConfigUtils.isSSLEnabled == true` then scheme is always https://..
Why not have `this.appMasterTrackingUrl = ConfigUtils.getSchemePrefix(config) + host` instead of `if-else` loop?
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by gauravgopi123 <gi...@git.apache.org>.
Github user gauravgopi123 commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70900320
--- Diff: engine/src/main/java/com/datatorrent/stram/util/ConfigUtils.java ---
@@ -68,15 +68,30 @@ public static String getRMUsername(Configuration conf)
return principal;
}
- public static String getSchemePrefix(YarnConfiguration conf)
+ public static boolean isSSLEnabled(Configuration conf)
+ {
+ if (HttpConfig.Policy.HTTPS_ONLY == HttpConfig.Policy.fromString(
+ conf.get(YarnConfiguration.YARN_HTTP_POLICY_KEY, YarnConfiguration.YARN_HTTP_POLICY_DEFAULT))) {
+ return true;
+ }
+ return false;
+ }
+
+ public static String getSchemePrefix(Configuration conf)
{
- if (HttpConfig.Policy.HTTPS_ONLY == HttpConfig.Policy.fromString(conf.get(YarnConfiguration.YARN_HTTP_POLICY_KEY, YarnConfiguration.YARN_HTTP_POLICY_DEFAULT))) {
+ if (isSSLEnabled(conf)) {
--- End diff --
@PramodSSImmaneni : In-fact I won't make any change in this file. I would just add following code in StreamingAppMasterService
`if ("https//:".equals.(ConfigUtils. getSchemePrefix(config))) {
appMasterTrackingUrl = "https://" + appMasterTrackingUrl;
}
`
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by pradeepdalvi <gi...@git.apache.org>.
GitHub user pradeepdalvi reopened a pull request:
https://github.com/apache/apex-core/pull/357
APEXCORE-488: Issues in SSL communication with StrAM
- Fixed Application Master trackingURL
- StramAgent shall not assume always HTTP
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pradeepdalvi/apex-core APEXCORE-488.ssl-issues
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/apex-core/pull/357.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 #357
----
commit 069a6efde3ebafdf560537020c56c7a921f90ca1
Author: Pradeep A. Dalvi <pr...@apache.org>
Date: 2016-07-11T22:59:32Z
APEXCORE-488: Issues in SSL communication with StrAM
- Fixed Application Master trackingURL
- StramAgent shall not assume always HTTP
----
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by PramodSSImmaneni <gi...@git.apache.org>.
Github user PramodSSImmaneni commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70630193
--- Diff: engine/src/main/java/com/datatorrent/stram/util/ConfigUtils.java ---
@@ -68,15 +68,30 @@ public static String getRMUsername(Configuration conf)
return principal;
}
- public static String getSchemePrefix(YarnConfiguration conf)
+ public static boolean isSecure(Configuration conf)
--- End diff --
Change the name to isSSLEnabled or something like that, secure in hadoop context typically refers to kerberos security
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by pradeepdalvi <gi...@git.apache.org>.
Github user pradeepdalvi closed the pull request at:
https://github.com/apache/apex-core/pull/357
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by PramodSSImmaneni <gi...@git.apache.org>.
Github user PramodSSImmaneni commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70850997
--- Diff: engine/src/main/java/com/datatorrent/stram/util/ConfigUtils.java ---
@@ -68,15 +68,30 @@ public static String getRMUsername(Configuration conf)
return principal;
}
- public static String getSchemePrefix(YarnConfiguration conf)
+ public static boolean isSSLEnabled(Configuration conf)
+ {
+ if (HttpConfig.Policy.HTTPS_ONLY == HttpConfig.Policy.fromString(
+ conf.get(YarnConfiguration.YARN_HTTP_POLICY_KEY, YarnConfiguration.YARN_HTTP_POLICY_DEFAULT))) {
+ return true;
+ }
+ return false;
+ }
+
+ public static String getSchemePrefix(Configuration conf)
{
- if (HttpConfig.Policy.HTTPS_ONLY == HttpConfig.Policy.fromString(conf.get(YarnConfiguration.YARN_HTTP_POLICY_KEY, YarnConfiguration.YARN_HTTP_POLICY_DEFAULT))) {
+ if (isSSLEnabled(conf)) {
--- End diff --
Do you mean the getSchemePrefix(YarnConfiguration...) method? Needed for binary compatibility.
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by pradeepdalvi <gi...@git.apache.org>.
GitHub user pradeepdalvi reopened a pull request:
https://github.com/apache/apex-core/pull/357
APEXCORE-488: Issues in SSL communication with StrAM
- Fixed Application Master trackingURL
- StramAgent shall not assume always HTTP
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pradeepdalvi/apex-core APEXCORE-488.ssl-issues
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/apex-core/pull/357.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 #357
----
commit 069a6efde3ebafdf560537020c56c7a921f90ca1
Author: Pradeep A. Dalvi <pr...@apache.org>
Date: 2016-07-11T22:59:32Z
APEXCORE-488: Issues in SSL communication with StrAM
- Fixed Application Master trackingURL
- StramAgent shall not assume always HTTP
----
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by pradeepdalvi <gi...@git.apache.org>.
Github user pradeepdalvi commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70738521
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingAppMasterService.java ---
@@ -614,7 +615,16 @@ protected void serviceStart() throws Exception
}
WebApp webApp = WebApps.$for("stram", StramAppContext.class, appContext, "ws").with(config).start(new StramWebApp(this.dnmgr));
LOG.info("Started web service at port: " + webApp.port());
- this.appMasterTrackingUrl = NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port();
+ String host = NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port();
+
+ // For backward compatibility, not adding scheme in TrackingURL for non-HTTPS
+ // TODO: Remove the check in next major release and add scheme always
+ if (ConfigUtils.isSSLEnabled(config)) {
+ String scheme = ConfigUtils.getSchemePrefix(config);
--- End diff --
@gauravgopi123 @vrozov I had same thing in mind, when I introduced isSSLEnabled function. And it was basically introduced to add scheme conditionally and to avoid compatibility issues before major release.
As you already got an idea, these changes were made while keeping future changes in mind. Ultimately we would like to see something like:
` this.appMasterTrackingUrl = NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port();
this.appMasterTrackingUrl = ConfigUtils.getSchemePrefix(config) + this.appMasterTrackingUrl;`
So IMO, we can make changes as below. And during next major release simply remove if condition.
` this.appMasterTrackingUrl = NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port();
// For backward compatibility, not adding scheme in TrackingURL for non-HTTPS
// TODO: Remove the check in next major release and add scheme always
if (ConfigUtils.isSSLEnabled(config)) {
this.appMasterTrackingUrl = ConfigUtils.getSchemePrefix(config) + this.appMasterTrackingUrl;
}`
Please let me know your thoughts on this.
---
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] apex-core issue #357: APEXCORE-488: Issues in SSL communication with StrAM
Posted by pradeepdalvi <gi...@git.apache.org>.
Github user pradeepdalvi commented on the issue:
https://github.com/apache/apex-core/pull/357
@PramodSSImmaneni Could you please review this?
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by gauravgopi123 <gi...@git.apache.org>.
Github user gauravgopi123 commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70848037
--- Diff: engine/src/main/java/com/datatorrent/stram/util/ConfigUtils.java ---
@@ -68,15 +68,30 @@ public static String getRMUsername(Configuration conf)
return principal;
}
- public static String getSchemePrefix(YarnConfiguration conf)
+ public static boolean isSSLEnabled(Configuration conf)
+ {
+ if (HttpConfig.Policy.HTTPS_ONLY == HttpConfig.Policy.fromString(
+ conf.get(YarnConfiguration.YARN_HTTP_POLICY_KEY, YarnConfiguration.YARN_HTTP_POLICY_DEFAULT))) {
+ return true;
+ }
+ return false;
+ }
+
+ public static String getSchemePrefix(Configuration conf)
{
- if (HttpConfig.Policy.HTTPS_ONLY == HttpConfig.Policy.fromString(conf.get(YarnConfiguration.YARN_HTTP_POLICY_KEY, YarnConfiguration.YARN_HTTP_POLICY_DEFAULT))) {
+ if (isSSLEnabled(conf)) {
--- End diff --
I think @vrozov also pointed out this earlier, but this function is not required.. I see all the usages passing Yarnconfiguration
---
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] apex-core pull request #357: APEXCORE-488: Issues in SSL communication with ...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/apex-core/pull/357#discussion_r70526864
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingAppMasterService.java ---
@@ -608,13 +609,16 @@ protected void serviceStart() throws Exception
try {
Configuration config = getConfig();
+ YarnConfiguration conf = new YarnConfiguration();
--- End diff --
As `Configuration` is a super class for `YarnConfiguration`, `Configuration` is not needed by WebApp server, it may use `YarnConfiguration`. Please simplify code to avoid creating multiple Configuration/YarnConfiguration objects and reading .conf files multiple times.
---
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.
---