codalab / codabench

Codabench is a flexible, easy-to-use and reproducible benchmarking platform. Check our paper at Patterns Cell Press https://hubs.li/Q01fwRWB0
Apache License 2.0
76 stars 28 forks source link

Caddy #1424

Closed ObadaS closed 6 months ago

ObadaS commented 7 months ago

@ 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:

Issues this PR resolves

1349

A checklist for hand testing

Checklist

ihsaan-ullah commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago
#Global directives
{
  auto_https off 
}
ObadaS commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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.