Alon Bar-Lev has posted comments on this change. Change subject: engine, webadmin: Webadmin read reports.xml from remote reports app ......................................................................
Patch Set 13: (9 comments) http://gerrit.ovirt.org/#/c/29723/13/backend/manager/modules/services/src/main/java/org/ovirt/engine/core/services/ReportsServletBase.java File backend/manager/modules/services/src/main/java/org/ovirt/engine/core/services/ReportsServletBase.java: Line 30: Line 31: @Override Line 32: protected void doGet(HttpServletRequest request, HttpServletResponse response) Line 33: throws ServletException, IOException { Line 34: response.setContentType("text/html"); please copy the content length and content type from remote. Line 35: String reportsUrl = Config.<String> getValue(ConfigValues.RedirectServletReportsPage); Line 36: Line 37: OutputStream out = response.getOutputStream(); Line 38: try { Line 41: } else { Line 42: handleCopy(reportsUrl, out); Line 43: } Line 44: } catch (IOException ex) { Line 45: response.setStatus(HttpServletResponse.SC_NOT_FOUND); this should be 500 not 4xx Line 46: } finally { Line 47: out.flush(); Line 48: out.close(); Line 49: } Line 44: } catch (IOException ex) { Line 45: response.setStatus(HttpServletResponse.SC_NOT_FOUND); Line 46: } finally { Line 47: out.flush(); Line 48: out.close(); please use try with resources Line 49: } Line 50: } Line 51: Line 52: public abstract void handleCopy(String reportsUrl, OutputStream out) throws IOException; Line 61: protected InputStream openHttpsUrlStream(String urlStr) throws IOException { Line 62: try { Line 63: // configure the SSLContext with a TrustManager Line 64: SSLContext ctx = SSLContext.getInstance("TLS"); Line 65: ctx.init(new KeyManager[0], new TrustManager[] {new DefaultTrustManager()}, new SecureRandom()); null at first and last parameters Line 66: SSLContext.setDefault(ctx); Line 67: } catch (NoSuchAlgorithmException|KeyManagementException ex) { Line 68: Line 69: } Line 62: try { Line 63: // configure the SSLContext with a TrustManager Line 64: SSLContext ctx = SSLContext.getInstance("TLS"); Line 65: ctx.init(new KeyManager[0], new TrustManager[] {new DefaultTrustManager()}, new SecureRandom()); Line 66: SSLContext.setDefault(ctx); you never modify the default context and effect the entire jre! Line 67: } catch (NoSuchAlgorithmException|KeyManagementException ex) { Line 68: Line 69: } Line 70: HttpsURLConnection conn = (HttpsURLConnection) new URL(urlStr).openConnection(); Line 63: // configure the SSLContext with a TrustManager Line 64: SSLContext ctx = SSLContext.getInstance("TLS"); Line 65: ctx.init(new KeyManager[0], new TrustManager[] {new DefaultTrustManager()}, new SecureRandom()); Line 66: SSLContext.setDefault(ctx); Line 67: } catch (NoSuchAlgorithmException|KeyManagementException ex) { never ignore exceptions, raise as runtimeexception Line 68: Line 69: } Line 70: HttpsURLConnection conn = (HttpsURLConnection) new URL(urlStr).openConnection(); Line 71: conn.setHostnameVerifier(new HostnameVerifier() { Line 88: } Line 89: return count; Line 90: } Line 91: Line 92: private static class DefaultTrustManager implements X509TrustManager { if you used inline for host verification, why not use inline for this? Line 93: Line 94: @Override Line 95: public void checkClientTrusted(X509Certificate[] arg0, String arg1) throws CertificateException {} Line 96: Line 98: public void checkServerTrusted(X509Certificate[] arg0, String arg1) throws CertificateException {} Line 99: Line 100: @Override Line 101: public X509Certificate[] getAcceptedIssuers() { Line 102: return null; return new java.security.cert.X509Certificate[] {}; Line 103: } Line 104: } Line 105: http://gerrit.ovirt.org/#/c/29723/13/backend/manager/modules/services/src/main/webapp/WEB-INF/web.xml File backend/manager/modules/services/src/main/webapp/WEB-INF/web.xml: Line 78: <servlet-class>org.ovirt.engine.core.services.ReportsXmlServlet</servlet-class> Line 79: </servlet> Line 80: <servlet-mapping> Line 81: <servlet-name>reports-xml</servlet-name> Line 82: <url-pattern>/reports-xml</url-pattern> I suggest to have consider to have a single reports interface /reports?command=status /reports?command=ui this will enable us to maintain single interface, while delegating this into the reports container Line 83: </servlet-mapping> Line 84: Line 85: <servlet> Line 86: <servlet-name>health</servlet-name> -- To view, visit http://gerrit.ovirt.org/29723 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I76db7ab889f21de083bb3c8276e8abb77b68fdb3 Gerrit-PatchSet: 13 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Shirly Radco <sra...@redhat.com> Gerrit-Reviewer: Yaniv Dary <yd...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
