You are viewing a plain text version of this content. The canonical link for it is here.
Posted to yarn-issues@hadoop.apache.org by "Szilard Nemeth (JIRA)" <ji...@apache.org> on 2018/08/16 10:18:00 UTC
[jira] [Comment Edited] (YARN-8448) AM HTTPS Support
[ https://issues.apache.org/jira/browse/YARN-8448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16582321#comment-16582321 ]
Szilard Nemeth edited comment on YARN-8448 at 8/16/18 10:17 AM:
----------------------------------------------------------------
Hey [~rkanter]!
Here are my review comments:
- hadoop project/pom.xml: Please use a variable for the bouncycastle version instead of using 1.59 twice.
- There are unused imports in: ResourceManager, TestWebAppProxyServlet
- I don't really see any usage of the introduced method {{ org.apache.hadoop.security.ssl.KeyStoreTestUtil.setAllowAllSSL(javax.net.ssl.HttpsURLConnection) }}
Actually there are two definitions of {{setAllowAllSSL}}, they both public and I can't see any usage of those methods.
- {{KeyStoreTestUtil}}: Throws clause for {{CertificateException}} can be removed since {{checkClientTrusted}} and {{checkServerTrusted}} never throws this exception
- {{ProxyCAManager}}: LOG is unused, rmContext is unused. Did you intend to use rmContext in recover? I would assume this as there is a TODO there.
- {{container-executor.c}}: The first block you added to {{create_script_paths}} uses {{exit_code = OUT_OF_MEMORY;}} if the keystore/truststore file destinations could not be created. Is this intentional?
- {{container-executor.c}}: Still in {{create_script_paths}}, the code black that try to open keystore/truststore files,
the exit codes seem to be bad here:
{{exit_code = INVALID_ARGUMENT_NUMBER;}}
Should be instead: {{COULD_NOT_CREATE_KEYSTORE_FILE or COULD_NOT_CREATE_TRUSTSTORE_FILE}}
- {{org.apache.hadoop.yarn.server.resourcemanager.TestApplicationMasterLauncher#testSetupTokens: }} This method could be private.
Moreover, I would extract the code setting up {{proxyCA}} with the mocked methods to a new method, for the sake of readability.
- {{org.apache.hadoop.yarn.server.resourcemanager.TestApplicationMasterLauncher.MyAMLauncher#createAndSetAMRMToken: }} Type argument AMRMTokenIdentifierfor Token can be removed
- {{org.apache.hadoop.yarn.server.webproxy.ProxyCA#createCACertAndKeyPair: }} I think the {{to}} Date could be a private static final field of this class as it is a fixed date.
- {{org.apache.hadoop.yarn.server.webproxy.ProxyCA#createCACertAndKeyPair }}: When {{createCert}} is invoked, is it intentional that you used the same string for the issuer and the subject?
- {{ProxyCA{{, in method {{createTrustManager}}: {{checkClientTrusted}} does not throw a {{CertificateException}} so you can remove it from the signature
was (Author: snemeth):
Hey [~rkanter]!
Here are my review comments:
- hadoop project/pom.xml: Please use a variable for the bouncycastle version instead of using 1.59 twice.
- There are unused imports in: ResourceManager, TestWebAppProxyServlet
- I don't really see any usage of the introduced method {{ org.apache.hadoop.security.ssl.KeyStoreTestUtil#setAllowAllSSL(javax.net.ssl.HttpsURLConnection) }}
Actually there are two definitions of {{setAllowAllSSL}}, they both public and I can't see any usage of those methods.
- {{KeyStoreTestUtil}}: Throws clause for {{CertificateException}} can be removed since {{checkClientTrusted}} and {{checkServerTrusted}} never throws this exception
- {{ProxyCAManager}}: LOG is unused, rmContext is unused. Did you intend to use rmContext in recover? I would assume this as there is a TODO there.
- {{container-executor.c}}: The first block you added to {{create_script_paths}} uses {{exit_code = OUT_OF_MEMORY;}} if the keystore/truststore file destinations could not be created. Is this intentional?
- {{container-executor.c}}: Still in {{create_script_paths}}, the code black that try to open keystore/truststore files,
the exit codes seem to be bad here:
{{exit_code = INVALID_ARGUMENT_NUMBER;}}
Should be instead: {{COULD_NOT_CREATE_KEYSTORE_FILE or COULD_NOT_CREATE_TRUSTSTORE_FILE}}
- {{org.apache.hadoop.yarn.server.resourcemanager.TestApplicationMasterLauncher#testSetupTokens: }} This method could be private.
Moreover, I would extract the code setting up {{proxyCA}} with the mocked methods to a new method, for the sake of readability.
- {{org.apache.hadoop.yarn.server.resourcemanager.TestApplicationMasterLauncher.MyAMLauncher#createAndSetAMRMToken: }} Type argument AMRMTokenIdentifierfor Token can be removed
- {{org.apache.hadoop.yarn.server.webproxy.ProxyCA#createCACertAndKeyPair: }} I think the {{to}} Date could be a private static final field of this class as it is a fixed date.
- {{org.apache.hadoop.yarn.server.webproxy.ProxyCA#createCACertAndKeyPair }}: When {{createCert}} is invoked, is it intentional that you used the same string for the issuer and the subject?
- {{ProxyCA{{, in method {{createTrustManager}}: {{checkClientTrusted}} does not throw a {{CertificateException}} so you can remove it from the signature
> AM HTTPS Support
> ----------------
>
> Key: YARN-8448
> URL: https://issues.apache.org/jira/browse/YARN-8448
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Robert Kanter
> Assignee: Robert Kanter
> Priority: Major
> Attachments: YARN-8448.001.patch, YARN-8448.002.patch, YARN-8448.003.patch, YARN-8448.004.patch, YARN-8448.005.patch
>
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org