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

Caddy #1424

Merged
merged 9 commits into from
May 28, 2024
Merged

Caddy #1424

merged 9 commits into from
May 28, 2024

Conversation

ObadaS
Copy link
Collaborator

@ObadaS ObadaS commented Apr 26, 2024

@ mention of reviewers

@Didayolo @ihsaan-ullah

A brief description of the purpose of the changes contained in this PR.

This should fix the problem of persistent certificates on caddy:

  • I changed the caddy image to the official one, upgrading it to version 2.7.6 (the one we were using was an unofficial 1.0.3)
  • I updated the syntax of the Caddyfile to support the changes caddy v2 brings
  • Changed the volumes in the docker-compose.yml for the container to use the recommended ones from caddy's Docker Hub
    • Added ./caddy_data:/data which is used by the image to store important information, certificates included.
    • Added ./caddy_config:/config which is used by the image to store some configuration (I'm not sure if it's really needed in our case)
    • Renamed ./Caddyfile:/etc/Caddyfile to ./Caddyfile:/etc/caddy/Caddyfile (new official location for the Caddyfile)
    • Removed ./certs/caddy:/etc/caddycerts which is not present on the official Docker Hub

Issues this PR resolves

#1349

A checklist for hand testing

  • Make sure the Caddyfile changes are enough for every part of the website

Checklist

  • Code review by me
  • Hand tested by me
  • I'm proud of my work
  • Code review by reviewer
  • Hand tested by reviewer
  • CircleCi tests are passing
  • Ready to merge

@ihsaan-ullah
Copy link
Collaborator

This looks good. We tested it together on Obada's system and everything was working perfectly. Certificates are stored in the data directory and the restart of the stack does not affect the certificates. We also deleted the certificate to verify that new certificate is generated. Another test may be needed on Codabench test

@Didayolo Didayolo linked an issue Apr 26, 2024 that may be closed by this pull request
@Didayolo
Copy link
Collaborator

This looks good. We tested it together on Obada's system and everything was working perfectly. Certificates are stored in the data directory and the restart of the stack does not affect the certificates. We also deleted the certificate to verify that new certificate is generated. Another test may be needed on Codabench test

Additionally, CircleCI tests are not passing

E       selenium.common.exceptions.TimeoutException: Message: TimedPromise timed out after 300000 ms
/usr/local/lib/python3.8/site-packages/selenium/webdriver/remote/errorhandler.py:242: TimeoutException
=========================== short test summary info ============================
FAILED src/tests/functional/test_competitions.py::TestCompetitions::test_manual_competition_creation
FAILED src/tests/functional/test_login.py::TestLogin::test_login - selenium.c...
FAILED src/tests/functional/test_submissions.py::TestSubmissions::test_v15_iris_code_submission_end_to_end
FAILED src/tests/functional/test_submissions.py::TestSubmissions::test_v15_iris_result_submission_end_to_end
============= 4 failed, 5 passed, 9 warnings in 1420.46s (0:23:40) =============

@ObadaS
Copy link
Collaborator Author

ObadaS commented Apr 29, 2024

I updated the branch (it was 31 commits behind develop), the CircleCI tests now fail at a different place

src/tests/functional/test_competitions.py ....                           [ 44%]
src/tests/functional/test_login.py .                                     [ 55%]
src/tests/functional/test_submissions.py .FFFE                           [100%]

[...]

=========================== short test summary info ============================
FAILED src/tests/functional/test_submissions.py::TestSubmissions::test_v15_iris_result_submission_end_to_end
FAILED src/tests/functional/test_submissions.py::TestSubmissions::test_v18_submission_end_to_end
FAILED src/tests/functional/test_submissions.py::TestSubmissions::test_v2_submission_end_to_end
ERROR src/tests/functional/test_submissions.py::TestSubmissions::test_v2_submission_end_to_end
========= 3 failed, 6 passed, 9 warnings, 1 error in 511.02s (0:08:31) =========

Which is weird because it works on my local machine. I was able to create a new competition after installing Codabench from scratch from the caddy branch.

@ihsaan-ullah
Copy link
Collaborator

Which is weird because it works on my local machine. I was able to create a new competition after installing Codabench from scratch from the caddy branch.

When you run the tests locally do you get any error?

@ObadaS
Copy link
Collaborator Author

ObadaS commented Apr 29, 2024

This is after running the tests locally (I copied the same commands that CircleCI launches):

src/tests/functional/test_competitions.py ....                                                 [ 44%]
src/tests/functional/test_login.py .                                                           [ 55%]
src/tests/functional/test_submissions.py ....                                                  [100%]
========================== 9 passed, 9 warnings in 189.03s (0:03:09) ============================

This is the result with the .env_circleci file, it should be close to what CircleCI does.

@ObadaS
Copy link
Collaborator Author

ObadaS commented Apr 29, 2024

For some reason, the tests now successfully passes in CircleCI after the latest commit, which only fixed some spaces in the docker-compose.yml (and added better formatting for the logs for caddy, but it should not change anything for CircleCI)

@Didayolo
Copy link
Collaborator

@ObadaS I am testing this PR and I'm facing issues.

When I try to access http://localhost/, I get redirected to https://localhost/, and the platform is not loading.

In the logs, I have this error:

codabench-caddy-1           | {"level":"info","ts":1715929871.8110924,"msg":"using provided configuration","config_file":"/etc/caddy/Caddyfile","config_adapter":"caddyfile"}
codabench-caddy-1           | Error: adapting config using caddyfile: parsing caddyfile tokens for 'tls': wrong argument count or unexpected line ending after 'tls', at /etc/caddy/Caddyfile:3
codabench-caddy-1 exited with code 1

Is there any procedure I missed that is necessary for the update?
Also, I did not understand this indication: "Make sure the Caddyfile changes are enough for every part of the website".

@ObadaS
Copy link
Collaborator Author

ObadaS commented May 21, 2024

@ObadaS I am testing this PR and I'm facing issues.

When I try to access http://localhost/, I get redirected to https://localhost/, and the platform is not loading.

In the logs, I have this error:

codabench-caddy-1           | {"level":"info","ts":1715929871.8110924,"msg":"using provided configuration","config_file":"/etc/caddy/Caddyfile","config_adapter":"caddyfile"}
codabench-caddy-1           | Error: adapting config using caddyfile: parsing caddyfile tokens for 'tls': wrong argument count or unexpected line ending after 'tls', at /etc/caddy/Caddyfile:3
codabench-caddy-1 exited with code 1

@Didayolo Looking at the error message, it seems like you might not have entered a valid email address in the .env file, you need to uncomment the TLS_EMAIL variable in the .env, OR comment the tls {$TLS_EMAIL} line in the Caddyfile

Also, I did not understand this indication: "Make sure the Caddyfile changes are enough for every part of the website".

I meant mostly to test different functionalities, like submitting a competition, loading a competition, creating a new user etc...

@Didayolo
Copy link
Collaborator

#Global directives
{
  auto_https off 
}

@ObadaS
Copy link
Collaborator Author

ObadaS commented May 27, 2024

@Didayolo Commenting tls {$TLS_EMAIL} (or adding an email in the .env file) fixes the issue.
Disabling auto HTTPS does not work, the same error appears, Caddy wants an email address for its tls directive.
For now, I recommend commenting tls {$TLS_EMAIL} in the Caddyfile.

@Didayolo
Copy link
Collaborator

@ObadaS OK, but then if we add a TLS_EMAIL in the .env file it won't be reflected right? And it would be required to update the Caddyfile.

Couldn't we make this more robust and convenient? Note that we also have a EMAIL_USE_TLS variable in the .env that we could use.

@ObadaS
Copy link
Collaborator Author

ObadaS commented May 27, 2024

We already have a TLS_EMAIL in the .env file, but it is commented by default. I am guessing that people that deploy the website and add HTTPS with caddy will uncomment it anyway to add their email address to get notified about potential certificate problems.

Since TLS_EMAIL is commented by default, I also commented the line in the Caddyfile, which means that people will have to uncomment (and add their email) the line in the .env file and uncomment the line in the Caddyfile.

@Didayolo Didayolo merged commit 94028a4 into develop May 28, 2024
1 check passed
@Didayolo Didayolo deleted the caddy branch May 28, 2024 12:39
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.

TLS certificates don't persist
3 participants