Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP base: rewrite apache configuration generator #2645

Closed
wants to merge 12 commits into from

Conversation

dset0x
Copy link
Contributor

@dset0x dset0x commented Jan 6, 2015

  • The templates were merged into one, and so was the resulting
    configuration file. Should the user need to enable or disable SSL for
    example, the recommended way is to edit the invenio configuration and
    re-run create-config.
  • All variables that are used in the template must now pass through
    apache.py, even if no operation is performed on them. All logic
    that does not logically belong in Jinja2 has been removed from the
    template.
  • Apache version 2.2 OpenSSL 1.0.1e are now the minimum supported. These
    are the official CentOS 6.6 packages.
  • mod_headers is now necessary. So is socache_shmcb for apache >= 2.3.3
    in order to use SSLStaplingCache.
  • SSL allowed protocols and ciphers have been set according to mozilla's
    recommendations.
  • The switches for inveniomanage apache create-config have changed:
    --force was dropped as it was a NO-OP.
  • It is now possible to manually set the paths to the desired
    certificates, as the following configuration directives have been
    ported from zenodo: 'APACHE_CERTIFICATE_PEM_FILE',
    'APACHE_CERTIFICATE_FILE', 'APACHE_CERTIFICATE_KEY_FILE'.
  • invenio/legacy/inveniocfg.py:_detect_ip_address exception handling
    is improved and it now uses 'google.com' instead of 'invenio.org' to
    test connectivity, because of their uptime.

Fixes #2626

@tiborsimko tiborsimko added this to the v2.0.x milestone Jan 6, 2015
@dset0x dset0x force-pushed the apache-config branch 6 times, most recently from 6dbf552 to aaeabfa Compare January 12, 2015 11:42
@@ -832,9 +832,9 @@ def _detect_ip_address():
"""
try:
s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
s.connect(('invenio-software.org', 0))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep the invenio-software.org

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this is not really part of this patch, but I expect google.com to have a greater uptime. Perhaps the issue is mentioning google in the code?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the issue is mentioning google in the code?

invenio-software.org should be up and running if you want to install demosite hence it should be fine for this case too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be missing something here. What is the relationship between invenio-software.org being up and someone creating the apache configuration for their website (it doesn't have to be the demosite).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not mention google in the code.

@jirikuncar
Copy link
Member

@dset0x it looks good. If you are done, please remove WIP from the PR title, improve PEP8/257, rebase and re-push. Thanks

@dset0x dset0x force-pushed the apache-config branch 5 times, most recently from a101558 to aebe9c9 Compare January 19, 2015 09:42
try:
from shutil import which
except ImportError:
from distutils.spawn import find_executable as which
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check and amend invenio.utils.shell:which. Thank you

@jirikuncar
Copy link
Member

Please add NOTE to commit message with explanation how to migrate current templates. (cc @lnielsen )

@jirikuncar
Copy link
Member

@dset0x when you are done can you try to enable Apache tests in .travis.yml? Thank you

@dset0x
Copy link
Contributor Author

dset0x commented Feb 20, 2015

@jirikuncar I have yet to see this run once:

@lovasko Any progress with testing this? Are there any tests other than wget -O /dev/null http://localhost you would like to see ran?

@jirikuncar
Copy link
Member

Are there any tests other than wget -O /dev/null http://localhost you would like to see ran?

That one should be enough for now.

'login_url': app.config.get('SSO_LOGIN_URL', None)
}
if sso['enabled']:
assert sso.login_url is not None, 'SSO enabled by URL not set'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/by /but /

@dset0x
Copy link
Contributor Author

dset0x commented Jun 5, 2015

@kaplun Thanks for the heads up, but how did do you know? Was this openssl_md_meth_names issue manifesting itself elsewhere too?

Edit: The problem is still present in this PR.

@kaplun
Copy link
Member

kaplun commented Jun 5, 2015

Ah nope sorry, I simply see that Travis succeeds right now. Also around on the web I have seen this was related to a temporary problem... Wasn't this PR blocked by the fact that Travis was not passing?

@dset0x
Copy link
Contributor Author

dset0x commented Jun 5, 2015

I simply see that Travis succeeds right now.

I have edited travis.yml to continue after wget fails so that I can have it print some debugging information afterwards:

(regardless of it being temporarily marked as passing)


Wasn't this PR blocked by the fact that Travis was not passing?

Yes, but there is also a new problem now: #3077 (comment) . Unfortunately I have not gotten around to working towards some RFC about it, due to working on other modules.

@jirikuncar
Copy link
Member

There has been more than one month of inactivity. Shall we close this PR as someday?

@dset0x dset0x force-pushed the apache-config branch 7 times, most recently from bf632eb to 2f3c595 Compare July 17, 2015 14:53
* Redesigns .travis.yml to use the new container-based infrastructure.
  This lowers the waiting time for the queue, gives us the ability to
  use caches for apt and pip and improves testing time down to ~15min.
  We are not allowed too use `sudo` any more but we should need it,
  because Apache is not tested anyway.

Signed-off-by: Marco Neumann <marco@crepererum.net>
@dset0x
Copy link
Contributor Author

dset0x commented Jul 20, 2015

@jirikuncar I understand that this functionality will be moved to the demosite, as I see we no longer want it to be tested in invenio itself (#3371). If you prefer this to be closed for organizational reasons until/if it works, feel free.

@jirikuncar
Copy link
Member

@dset0x the functionality of generating Apache configuration stays in Invenio. It's just hard to review your changes as the codebase is evolving a lot and you are not updating this PR.

@dset0x
Copy link
Contributor Author

dset0x commented Jul 20, 2015

@jirikuncar This PR is not awaiting review - it is not working: There is commentary about the issues above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants