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

Restore conservative configuration #5759

Merged
merged 1 commit into from
Jan 27, 2021
Merged

Restore conservative configuration #5759

merged 1 commit into from
Jan 27, 2021

Conversation

rmol
Copy link
Contributor

@rmol rmol commented Jan 27, 2021

Status

Ready for review

Description of Changes

Fixes #5757.

Bring back the try/except blocks needed to support long-running instance configurations that may lack newer attributes.

Testing

  • make test-config
  • comment out various settings, e.g. SESSION_EXPIRATION_MINUTES
  • make dev

The dev server should start without errors.

Deployment

This should be restoring the conservative configuration handling used in 1.6.0 and earlier.

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

@emkll emkll added this to the 1.7.1 milestone Jan 27, 2021
Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

Test plan passes for SESSION_EXPIRATION_MINUTES and locale config settings, fails with numerous errors for other config settings. For some of them it should fail (like SCRYPT_*, there should never be default values for those.) Testing scope might need to be narrowed.


from sdconfig import SDConfig

SDConfig()
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this breaks subsequent tests that rely on the removed attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, added another reload at the end, which resolved it with a subset locally.

@rmol
Copy link
Contributor Author

rmol commented Jan 27, 2021

I should have written that the dev server should start without errors thrown from sdconfig.py, and that any errors seen are raised where the configuration values are used.

Bring back the try/except blocks needed to support long-running
instance configurations that may lack newer attributes.
Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

Tested the changes in a staging server, from a 1.6.0 instance (with SESSION_EXPIRATION_MINUTES removed from config.py. Upon upgrading to 1.7.0, observed the error at upgrade time (and 500 on source interface):

  File "<frozen importlib._bootstrap_external>", line 665, in exec_module
  File "<frozen importlib._bootstrap>", line 222, in _call_with_frames_removed
  File "/var/www/securedrop/alembic/versions/3da3fcab826a_delete_orphaned_submissions.py", line 18, in <module>
    from journalist_app import create_app
  File "/var/www/securedrop/journalist_app/__init__.py", line 13, in <module>
    import i18n
  File "/var/www/securedrop/i18n.py", line 32, in <module>
    from sdconfig import SDConfig
  File "/var/www/securedrop/sdconfig.py", line 98, in <module>
    config = SDConfig()  # type: SDConfig
  File "/var/www/securedrop/sdconfig.py", line 42, in __init__
    self.SESSION_EXPIRATION_MINUTES = _config.SESSION_EXPIRATION_MINUTES  # type: int
AttributeError: module 'config' has no attribute 'SESSION_EXPIRATION_MINUTES'
dpkg: error processing package securedrop-app-code (--configure):
 subprocess installed post-installation script returned error exit status 1
Errors were encountered while processing:
 securedrop-app-code
E: Sub-process /usr/bin/dpkg returned an error code (1)
Cannot start "/usr/sbin/sendmail": executable not found (adjust *sendmail* variable)
"/root/dead.letter" 1197/65704
... message not sent
root@app-staging:/var/www/securedrop# apt list --installed | grep securedrop-app-code

After running cron-apt on a local apt server with the changes in this branch, I can confirm this resolves the issue, and the source no longer returns 500 and accepts submissions.

Deferring to @zenmonkeykstop for final review, but good to merge for 1.7.1, from my perspective, once CI passes

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

Successfully merging this pull request may close these issues.

Error 500 on source interface and journalist interface after 1.7.0 upgrade
3 participants