APEXCORE-488: Issues in SSL communication with StrAM #357

pradeepdalvi · 2016-07-11T23:24:30Z

Fixed Application Master trackingURL
StramAgent shall not assume always HTTP

sandeshh · 2016-07-11T23:27:41Z

engine/src/main/java/com/datatorrent/stram/client/StramAgent.java

+      String url;
+      if (!info.appMasterTrackingUrl.startsWith("http://")
+          && !info.appMasterTrackingUrl.startsWith("https://")) {
+        url = "http://" + info.appMasterTrackingUrl;


It should be logged, saying falling back to http

If it is a secure cluster, then fallback should be https?

Its not falling back. When protocol scheme is not specified in Tracking URL, it shall use the default protocol scheme i.e. 'http://'

pradeepdalvi · 2016-07-12T00:09:33Z

@PramodSSImmaneni Could you please review this?

vrozov · 2016-07-12T02:16:34Z

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();


Why is it necessary to have Configuration and YarnConfiguration? Can this code be simplified to YarnConfiguration = StramClientUtils.getYarnConfiguration(getConfig());

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.

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.

pramodin · 2016-07-13T01:37:24Z

engine/src/main/java/com/datatorrent/stram/StreamingAppMasterService.java

-      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();


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.

pramodin · 2016-07-13T14:01:37Z

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)


Change the name to isSSLEnabled or something like that, secure in hadoop context typically refers to kerberos security

gauravgopi123 · 2016-07-13T23:54:56Z

engine/src/main/java/com/datatorrent/stram/StreamingAppMasterService.java

+      // 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);


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?

@gauravgopi123 to preserve compatibility with prior gateway version that assumes "http:" sceme and prepends "http://" to the tracking URL, it is necessary to check if SSL is enabled before prepending scheme, but it is not necessary to have if-else. It should be sufficient to prepend "https://" in case SSL is enabled:

appMasterTrackingUrl = NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port(); if (ConfigUtils.isSSLEnabled(config)) { appMasterTrackingUrl = "https://" + appMasterTrackingUrl; }

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

Or something like this:

this.appMasterTrackingUrl = (ConfigUtils.isSSLEnabled(config) ? ConfigUtils.getSchemePrefix(config) : "") + NetUtils.getConnectAddress(webApp.getListenerAddress()).getHostName() + ":" + webApp.port();

@pradeepdalvi : @vrozov had answered your question. Calling getSchemePrefix is again calling isSSLEnabled. Why is that requried

- Fixed Application Master trackingURL - StramAgent shall not assume always HTTP

gauravgopi123 · 2016-07-14T17:33:20Z

engine/src/main/java/com/datatorrent/stram/util/ConfigUtils.java

  {
-    if (HttpConfig.Policy.HTTPS_ONLY == HttpConfig.Policy.fromString(conf.get(YarnConfiguration.YARN_HTTP_POLICY_KEY, YarnConfiguration.YARN_HTTP_POLICY_DEFAULT))) {
+    if (isSSLEnabled(conf)) {


I think @vrozov also pointed out this earlier, but this function is not required.. I see all the usages passing Yarnconfiguration

Do you mean the getSchemePrefix(YarnConfiguration...) method? Needed for binary compatibility.

Are you good with that explanation @gauravgopi123

@PramodSSImmaneni : I am talking about getSchemePrefix(Configuration) method...

@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; }

@PramodSSImmaneni : I missed one thing. New method is needed as AppMaster has Configuration and YarnConfiguration object..

Changes look good

sandeshh reviewed Jul 11, 2016
View reviewed changes

vrozov reviewed Jul 12, 2016
View reviewed changes

pradeepdalvi force-pushed the APEXCORE-488.ssl-issues branch from d8b64f3 to 38dcce3 Compare July 13, 2016 00:17

pramodin reviewed Jul 13, 2016
View reviewed changes

pradeepdalvi force-pushed the APEXCORE-488.ssl-issues branch from 38dcce3 to 22783a7 Compare July 13, 2016 03:06

pramodin reviewed Jul 13, 2016
View reviewed changes

pradeepdalvi force-pushed the APEXCORE-488.ssl-issues branch from 22783a7 to 069a6ef Compare July 13, 2016 21:55

pradeepdalvi closed this Jul 13, 2016

pradeepdalvi reopened this Jul 13, 2016

pradeepdalvi closed this Jul 13, 2016

pradeepdalvi reopened this Jul 13, 2016

pradeepdalvi force-pushed the APEXCORE-488.ssl-issues branch from 069a6ef to 10619d6 Compare July 13, 2016 22:31

gauravgopi123 reviewed Jul 13, 2016
View reviewed changes

APEXCORE-488: Issues in SSL communication with StrAM

4f3ab00

- Fixed Application Master trackingURL - StramAgent shall not assume always HTTP

pradeepdalvi force-pushed the APEXCORE-488.ssl-issues branch from 10619d6 to 4f3ab00 Compare July 14, 2016 03:35

gauravgopi123 reviewed Jul 14, 2016
View reviewed changes

asfgit merged commit 4f3ab00 into apache:master Jul 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APEXCORE-488: Issues in SSL communication with StrAM #357

APEXCORE-488: Issues in SSL communication with StrAM #357

pradeepdalvi commented Jul 11, 2016

sandeshh Jul 11, 2016

sandeshh Jul 11, 2016

pradeepdalvi Jul 11, 2016

pradeepdalvi commented Jul 12, 2016

vrozov Jul 12, 2016

pradeepdalvi Jul 12, 2016

vrozov Jul 12, 2016

pramodin Jul 13, 2016

pramodin Jul 13, 2016

gauravgopi123 Jul 13, 2016

vrozov Jul 14, 2016 •

edited

pradeepdalvi Jul 14, 2016 •

edited

pradeepdalvi Jul 14, 2016

gauravgopi123 Jul 14, 2016

gauravgopi123 Jul 14, 2016

pramodin Jul 14, 2016

pramodin Jul 14, 2016

gauravgopi123 Jul 14, 2016

gauravgopi123 Jul 14, 2016

gauravgopi123 Jul 14, 2016

APEXCORE-488: Issues in SSL communication with StrAM #357

APEXCORE-488: Issues in SSL communication with StrAM #357

Conversation

pradeepdalvi commented Jul 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradeepdalvi commented Jul 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrozov Jul 14, 2016 • edited

Choose a reason for hiding this comment

pradeepdalvi Jul 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrozov Jul 14, 2016 •

edited

pradeepdalvi Jul 14, 2016 •

edited