-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Have letsencrypt-auto do a real upgrade in leauto-upgrades #5371
Conversation
git checkout -f "$BRANCH" | ||
EXPECTED_VERSION=$(grep -m1 LE_AUTO_VERSION letsencrypt-auto | cut -d\" -f2) | ||
if ! ./letsencrypt-auto -v --debug --version --no-self-upgrade 2>&1 | grep $EXPECTED_VERSION ; then | ||
# Now that python and openssl have been installed, we can set up a fake server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting that this might break eventually when future systems stop supporting older pythons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. What might break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, if we just use whatever old python is installed by the old version.
# to provide a new version of letsencrypt-auto. First, we start the server and | ||
# directory to be served. | ||
MY_TEMP_DIR=$(mktemp -d) | ||
SERVER_PORT=12345 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should do something like https://github.com/certbot/certbot/blob/master/letsencrypt-auto-source/tests/auto_test.py#L84 if we don't want this test to be flaky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, if SimpleHTTPServer can get a port for you and tell you what it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you tell SimpleHTTPServer
to use port 0, the OS gives it a random free port. I had a version that did this but encountered some issues where Python wasn't flushing the output stream when I had it redirected somewhere, causing it to never print the port it chose. I just tested this locally and after waiting for multiple minutes, it still hasn't printed the port.
I can either add a simple recreation of this server to tools/
that manually flushes the output stream or we can pick a random, unprivileged port like 12345 (or another number) and assume/hope there won't be any collisions on our simple EC2 instances. Preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to do what https://github.com/certbot/certbot/blob/master/letsencrypt-auto-source/tests/auto_test.py#L84 does? Pick a range of ports, and try ports in that range until it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's an awkward race condition. We start SimpleHTTPServer
in the background and since it's running in parallel, we're just have to pick an arbitrary amount of time to wait and see if it's going to fail. Also, I'm not aware of a good way to check the exit status of a background process (without blocking on the process exiting).
EDIT: This is assuming we're using bash
. If we're going to shell out to Python, I'd rather just let the OS pick the port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put a version that has the OS pick the port in #5390. Pick whichever you like better or let me know if there's another approach you prefer that I'm missing.
# Finally, we wait for the server to start. | ||
sleep 5s | ||
|
||
if ! ./letsencrypt-auto -v --debug --version || ! diff letsencrypt-auto letsencrypt-auto-source/letsencrypt-auto ; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
! diff
--> they are different. We want them to be different, so this should just be diff
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ! diff a b
branch is run if files a
and b
differ. Since we just upgraded letsencrypt-auto
in the repo root to the version in letsencrypt-auto-source
, we expect these files to be identical.
Closing in favor of #5390. |
Rather than replacing the file with
git
, let's haveletsencrypt-auto
download a new version of itself from a dummy server.This PR takes advantage of #5370, but:
master
, this will still work.NO_CERT_VERIFY
from Deprecate Python2.6 by using Python3 on CentOS/RHEL 6 #5329 which obviously isn't available in older versions ofcertbot-auto
.I tested this successfully with both
master
and the #5329 branch.