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

Added production docker image with preview domain support #1627

Merged
merged 28 commits into from
Oct 2, 2024
Merged

Conversation

tobiasmcnulty
Copy link
Member

@tobiasmcnulty tobiasmcnulty commented Sep 26, 2024

Background

  • Python 3.8 has reached security EOL. Right now we are tied to 3.8 because that's what ships with Ubuntu 20.04.
  • Python 3.12 is chosen as the target because that's the most recent version supported by Django 4.2.
  • As discussed in the comments, this PR prepares the infrastructure to upgrade to Python 3.12 but does not actually apply the upgrade.

Overview

This PR includes the following changes:

  • Updates the Dockerfile to support a production environment, including gunicorn and the necessary C libraries to operate the site.
  • Adds GH Actions workflows to build and push an image to GH Packages repo
  • Adds the option of configuring the site DOMAIN_NAME (formerly, hard-coded to djangoproject.com) from the environment. This is necessary to access a "preview" or "staging" version of the site on a different server before it goes live. Previously, we could issue a certificate via ACME DNS on the backend to access the site, but Fastly uses the same ACME challenge URL, so this is not an option. Since the main site uses HSTS, it's no
  • Prepares the CI and tox configs to work with Python 3.12, including consolidating the isort and flake8 checks to run only via pre-commit

Why is all this necessary? Isn't there an easier way to get Python 3.12?

  • While it may be tempting to install Python 3.12 via deadsnakes ppa, I prefer to avoid this option (a) because of the security disclaimer they provide at the top, and (b) because the frequent minor version updates provided by such a repo require frequent recreation of what would otherwise be persistent virtual environments. In my experience this can be disruptive for production environments (sometimes a virtual env will continue working between minor python version upgrades, sometimes not).
  • Upgrading Ubuntu is also possible (after all, 3.12 is the default Python in Ubuntu 24.04), however, I believe in the long run it will be advantageous to decouple the djangoproject.com Python version from the underlying OS upgrades (which is time intensive on the ops side and makes it harder to coordinate).
  • We already deploy Trac with Podman in this way, so this PR will help consolidate both sites to be deployed the same way while affording more flexibility to the individual repos regarding the Python version.

@tobiasmcnulty
Copy link
Member Author

@pauloxnet Unfortunately I see the same HTTP 403 after downgrading to stripe==3.1.0

I will try downgrading the docker image to 3.8 and the old stripe to see if it's some other problem (maybe related to the alternate domain?).

Screenshot 2024-10-02 at 10 44 29 AM

@pauloxnet
Copy link
Member

pauloxnet commented Oct 2, 2024

@pauloxnet Unfortunately I see the same HTTP 403 after downgrading to stripe==3.1.0

I will try downgrading the docker image to 3.8 and the old stripe to see if it's some other problem (maybe related to the alternate domain?).

I see. Good Idea. But I think you have downgrade from Python 3.12 to Python 3.8 also in the pipelines. Maybe.

@tobiasmcnulty
Copy link
Member Author

@pauloxnet Unfortunately I see the same HTTP 403 after downgrading to stripe==3.1.0
I will try downgrading the docker image to 3.8 and the old stripe to see if it's some other problem (maybe related to the alternate domain?).

I see. Good Idea. But I think you have downgrade from Python 3.12 to Python 3.8 also in the pipelines. Maybe.

Thanks! Yeah pyupgrade got me :-\ Hopefully it's deploying now...

@pauloxnet
Copy link
Member

@pauloxnet Unfortunately I see the same HTTP 403 after downgrading to stripe==3.1.0

I will try downgrading the docker image to 3.8 and the old stripe to see if it's some other problem (maybe related to the alternate domain?).

Do you think we can separate this PR having in place all the changes but not the Python 3.12 upgrade ? In this way we can have a preview instance as similar as possible as the production environment? After that we would upgrade to Python 3.12?

@tobiasmcnulty
Copy link
Member Author

tobiasmcnulty commented Oct 2, 2024

@pauloxnet I see the same 403 with older Stripe and Python 3.8. It's deployed now if you want to try it: https://www.preview.djangoproject.com/fundraising/ (basic auth: admin / abc123)

Maybe the preview domain needs to be allowed in Stripe or something? Or Stripe is trying to post back to us and getting caught by basic auth? I'm sorry I am not familiar with how this works.

@tobiasmcnulty
Copy link
Member Author

Or Stripe is trying to post back to us and getting caught by basic auth? I'm sorry I am not familiar with how this works.

Sorry, I didn't pay close enough attention to the error. It looks like jQuery is not passing along the basic auth so we're sending a 403 to ourselves.

@tobiasmcnulty
Copy link
Member Author

@pauloxnet I deployed without basic auth, but I'm afraid I still see the same 403, and I'm out of time to debug at the moment. I'll leave it this way for a little bit and re-install basic auth later today or tomorrow. Please feel free to leave any suggested changes and/or let me know if you would like me to deploy again.

Copy link
Member

@pauloxnet pauloxnet left a comment

Choose a reason for hiding this comment

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

I still see some error in the pipeline.
I suggest to revert all upgrade of Python and leave in this PR all changes useful to have a preview instance.
When we'll have a working environment to test the donation flow we'll open another PR to upgrade Python and other packages step by step.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tobiasmcnulty and others added 2 commits October 2, 2024 11:25
Co-authored-by: Paolo Melchiorre <paolo@melchiorre.org>
@tobiasmcnulty tobiasmcnulty changed the title Upgraded to Python 3.12 Added production docker image with preview domain support Oct 2, 2024
@pauloxnet
Copy link
Member

pauloxnet commented Oct 2, 2024

@pauloxnet I deployed without basic auth, but I'm afraid I still see the same 403, and I'm out of time to debug at the moment. I'll leave it this way for a little bit and re-install basic auth later today or tomorrow. Please feel free to leave any suggested changes and/or let me know if you would like me to deploy again.

The issue seems related with CSRF because other POST actions are not working either like:

In the response of the POST call I read CSRF verification failed. Request aborted.

@tobiasmcnulty
Copy link
Member Author

tobiasmcnulty commented Oct 2, 2024

In the response of the POST call I read CSRF verification failed. Request aborted.

Good catch. Can you tell why this is happening for the preview domain but not www.djangoproject.com? It seems like this check should pass according to the docs and headers passed:

For requests that include the Origin header, Django’s CSRF protection requires that header match the origin present in the Host header.

Screenshot 2024-10-02 at 11 59 46 AM

Since it looks like jQuery was passing on the basicauth after all, I re-deployed it for now to prevent search engines from indexing the site while we debug this.

@pauloxnet
Copy link
Member

In the response of the POST call I read CSRF verification failed. Request aborted.

Good catch. Can you tell why this is happening for the preview domain but not www.djangoproject.com? It seems like this check should pass according to the docs and headers passed:

For requests that include the Origin header, Django’s CSRF protection requires that header match the origin present in the Host header.

Since it looks like jQuery was passing on the basicauth after all, I re-deployed it for now to prevent search engines from indexing the site while we debug this.

It's strange indeed.

I think you activate before DEBUG mode and this what I saw when I tried to login in the admin of the preview page:

Forbidden (403)

CSRF verification failed. Request aborted.
You are seeing this message because this site requires a CSRF cookie when submitting forms. This cookie is required for security reasons, to ensure that your browser is not being hijacked by third parties.
If you have configured your browser to disable cookies, please re-enable them, at least for this site, or for “same-origin” requests.

Help

Reason given for failure:
CSRF cookie not set.

In general, this can occur when there is a genuine Cross Site Request Forgery, or when Django’s CSRF mechanism has not been used correctly. For POST forms, you need to ensure:

  • Your browser is accepting cookies.
  • The view function passes a request to the template’s render method.
  • In the template, there is a {% csrf_token %} template tag inside each POST form that targets an internal URL.
  • If you are not using CsrfViewMiddleware, then you must use csrf_protect on any views that use the csrf_token template tag, as well as those that accept the POST data.
  • The form has a valid CSRF token. After logging in in another browser tab or hitting the back button after a login, you may need to reload the page with the form, because the token is rotated after a login.
    You’re seeing the help section of this page because you have DEBUG = True in your Django settings file. Change that to False, and only the initial error message will be displayed.
    You can customize this page using the CSRF_FAILURE_VIEW setting.

I've no idea right now (in Europe). I tag this PR for searching for help from @django/djangoproject-com-maintainters and @django/ops-team

@pauloxnet pauloxnet added on hold ops Operations help-needed Help needed labels Oct 2, 2024
@tobiasmcnulty
Copy link
Member Author

@pauloxnet It's working now. I forgot to include X-Forwarded-Proto in the nginx config, so Django (correctly) didn't match the Origin when checking CSRF.

I believe this PR should be safe to merge even before migrating production to Podman, since it uses the same Python version (gunicorn will also be installed even though we're using uwsgi, but oh well).

I will look through the PR one more time and mark it ready for review.

@tobiasmcnulty tobiasmcnulty marked this pull request as ready for review October 2, 2024 18:45
@pauloxnet
Copy link
Member

I'll do a final review ASAP

.github/workflows/docker-publish.yml Outdated Show resolved Hide resolved
.github/workflows/docker-test-build.yml Outdated Show resolved Hide resolved
tox.ini Outdated
@@ -1,22 +1,13 @@
[tox]
envlist =py38-{tests,flake8,black,isort}
envlist =py38-{tests,precommit}
Copy link
Member

@pauloxnet pauloxnet Oct 2, 2024

Choose a reason for hiding this comment

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

I would do the changes in this file in a separate PR, I think is not blocking for the goal of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. I will remove these changes and make a new PR to add Python 3.12 support, hopefully both at the same time, and include these changes there.

tobiasmcnulty and others added 2 commits October 2, 2024 15:52
Co-authored-by: Paolo Melchiorre <paolo@melchiorre.org>
Copy link
Member

@pauloxnet pauloxnet left a comment

Choose a reason for hiding this comment

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

LGTM

@tobiasmcnulty tobiasmcnulty merged commit 1be8f13 into main Oct 2, 2024
4 checks passed
@tobiasmcnulty tobiasmcnulty deleted the py312 branch October 2, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants