Juan Hernandez has posted comments on this change.
Change subject: packaging: redirect to TLS/SSL using application server
......................................................................
Patch Set 2: (11 inline comments)
Using the jboss specific capability to replace properties inside standard
deployment descriptors (once available) is a bad practice in my opinion,
because it moves us a small step farther from the possibility to deploy to
other containers (Tomcat, Jetty, standalone, etc). I think that replacing NONE
with CONFIDENTIAL while packaging is good enough.
....................................................
File backend/manager/modules/restapi/webapp/src/main/webapp/WEB-INF/web.xml
Line 15: <!-- TLS/SSL settings -->
This comment is a bit misleading, as the "security-constraint" block is not
only about TLS/SSL but about security in general. I would suggest to place it
before the "user-data-constraint" block.
Line 19: <description>Protected Context</description>
The "Protected Context" name and description are not very descriptive. People
tends to use names/descriptions that describe the URL pattern that follows,
things like "everything", "public-data", etc. I would suggest to use
"everything" to make clear that we want to protect all the application. The
description is optional, so if you are going to repeat the name it is better to
avoid it completely.
....................................................
File backend/manager/modules/root/src/main/webapp/index.html
Line 50: <div><a
href="OvirtEngineWeb/RedirectServlet?Page=Reports">Reports Portal</a></div>
The removed Javascript code used to generate absolute URLs here, but they are
relative now. That means that they will fail when the request is forwarded to
this page as a result of a 404 error.
Easy solution is to put an slash in fromt of the references:
<a href="/UserPortal">...
<a href="/webadmin">...
<a href="/OvirtEngineWeb/...">...
....................................................
File frontend/webadmin/modules/userportal-gwtp/src/main/webapp/WEB-INF/web.xml
Line 31: <!-- TLS/SSL settings -->
See the comments in root.war/WEB-INF/web.xml.
....................................................
File frontend/webadmin/modules/webadmin/src/main/webapp/WEB-INF/web.xml
Line 31: <!-- TLS/SSL settings -->
See comments in root.war/WEB-INF/web.xml.
....................................................
File packaging/fedora/engine-service.py
Line 382:
engineArgs.append("-DOVIRT_APPS_TRANSPORT_GUARANTEE=CONFIDENTIAL")
General consensus in the Java world is that system properties are all lower
case and with words separated by dots, so I suggest to use the following:
-Dovirt.apps.transport.guarantee=CONFIDENTIAL
We are (well at least I am) trying to avoid "ovirt" inside names of variables,
functions, methods, constants, etc. I suggest you use
"engine.apps.transport.guarantee".
....................................................
File packaging/fedora/engine-service.sysconfig
Line 68: #ENGINE_APPS_FORCE_SECURE=true
This deserves a section on its own, with a comment explaining what it does.
....................................................
File packaging/fedora/setup/basedefs.py
Line 64
I think this can't be removed, as it will be used by the upgrade code for
upgrades from 3.0 to 3.1. Makes sure it is moved to the upgrade code if you are
finally removing it.
....................................................
File packaging/fedora/setup/common_utils.py
Line 892
This function will be used by the upgrade code as well, in fact it was recently
added just for that. Make sure that it is moved to the upgrade code if you
finally remove it.
....................................................
File packaging/fedora/setup/ovirt_port80.py
Line 109
This file is not used at all, at least that was the response I got last time I
asked. Can you please double check that?
....................................................
File packaging/fedora/spec/ovirt-engine.spec.in
Line 364: done
Use spaces instead of tabs.
--
To view, visit http://gerrit.ovirt.org/6827
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d05ce7224548123c9f5f2a1ce09bf090625085
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Asaf Shakarchi <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Ofer Schreiber <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
