Deprecate Python2.6 by using Python3 on CentOS/RHEL 6 #5329

ohemorange · 2017-12-15T00:22:57Z

…stall python3

bmw · 2017-12-15T00:38:34Z

letsencrypt-auto-source/pieces/bootstrappers/rpm_python3.sh

+  # might have 2.6 or no python installed to get here
+
+  BootstrapRpmCommonBase $python_pkgs
+}


nit: There should be a newline at the end of the file.

bmw · 2017-12-15T00:42:54Z

letsencrypt-auto-source/pieces/bootstrappers/rpm_python3.sh

+
+BootstrapRpmPython3() {
+  # Tested with:
+  #   - CentOS 6 (EPEL must be installed manually)


EPEL shouldn't have to be installed manually on CentOS 6. The code that enables it is at https://github.com/certbot/certbot/pull/5329/files#diff-076974b35ed2e7defd212797e13f2796R29 and we should continue to use it.

However, this is currently true for this PR if we're hitting this function while bootstrapping for the first time on a system because $tool list python34 won't work until EPEL is available. There are a couple of options here:

Breakout the linked code into another function that can be called to install EPEL if necessary before running $tool list python34.

Assume python34 is the only case and remove the conditional setting of python_pkgs below. If we do this, we should definitely pull stats as I suggested in another comment.

I just copied this comment from the previous script. Is the comment made untrue by something changed in this PR, or was it just never updated previously? And this code runs at the same point as previously, so why would it be true for this PR but not true previously?

I forgot to update the comment from the previous script apparently. See #3259.

While this comment is inaccurate in the previous script, it's correct here that we're now requiring the user to manually install EPEL and I don't think we should do that. This is only true for new CentOS/RHEL 6 users who are bootstrapping for the first time.

The reason the PR currently works this way is EPEL is an additional repository for CentOS/RHEL and this repository contains the python34 packages on CentOS/RHEL 6. Until EPEL is installed, $TOOL list python34 below won't work.

How are we now requiring the user to manually install EPEL here in a way that we weren't before? This script uses the same logic at the same points in code execution, with respect to when we run $tool list python34.

Because the result of $tool list python34 depends on whether or not EPEL is installed.

$tool list <package> searches for a package with that name using the installed repositories. $tool list python34 will fail until EPEL is installed because python34 isn't available in the standard repositories, only in EPEL. If this bootstrap function is run on a CentOS 6 system without EPEL already installed, the if $tool list python34 branch will not be taken despite it containing the packages that should be used on this system.

For more info here, the code that installs EPEL if necessary is the block that starts with if ! $tool list *virtualenv >/dev/null 2>&1; then. Like $tool list python34, $tool list *virtualenv will fail to find matching packages on a RHEL6 based system without EPEL installed.

I feel like we're having a miscommunication here, so maybe we should talk in person tomorrow.

I understand why EPEL is needed to run this code, that's not what I'm asking. But don't we hit the code that installs EPEL before we get here? The way the code was before I ran this, we needed EPEL to check about python 2 packages, now we still need it to check about python 3 packages. In both cases, the code runs after we've installed EPEL for them. What am I missing that I've changed here to make it so we need EPEL earlier than we previously needed it?

Ah now I see. It's because we were previously running the enable EPEL code before checking for python packages, and now we're running it after checking for python packages because my refactoring changed the order.

bmw · 2017-12-15T00:57:40Z

letsencrypt-auto-source/pieces/bootstrappers/rpm_python3.sh

+    "
+  fi
+  # TODO: add some elifs and elses for other distros that
+  # might have 2.6 or no python installed to get here


This is a very clear TODO, but I think we should either remove the if statement above or have an else case where we blow up before landing this PR to prevent trying to continue when we don't recognize the system and failing later with an obscure error message after not installing any Python packages.

If you still don't have access, I'm happy to pull some stats from Let's Encrypt on certbot-auto users on Python 2.6 to see what other systems we may want to worry about here. My guess is there isn't (m)any, but we should certainly check this before assuming if we remove the if statement.

Yes, adding an else seems like the right call.

I don't have access yet, I'd love those stats. And also if you have any easier ways to check package lists on those systems than installing each one with docker.

I'm pulling the stats now for distinct certbot-auto installs in the past 2 months using Python 2.6. I'll post results here when the query is done.

Unfortunately I really don't have a better way to query versions for Red Hat based systems. All browser search tools I've seen are really buggy/inaccurate. If persistence is at all useful, I think you should feel free to spin up droplets/EC2 instances while you're working on this issue.

Here's the count of distinct IPs using Python 2.6 and certbot-auto by OS.

I've only included OSes with at least 5 users as below that, some of the values get pretty specific and start to identify the user. There are 61 more different OS strings with less than 5 users.

Also, keep in mind that we don't officially support many of the OSes below and the user had to do some tinkering on their end to make things work.

"os" "_count_distinct"

"CentOS 6.9" "22480"

"CentOS 6.8" "8180"

"CentOS 6.7" "3236"

"CentOS 6.5" "2885"

"CentOS 6.6" "2088"

"CentOS 6.4" "815"

"Red Hat Enterprise Linux Server 6.9" "455"

"CentOS 6.3" "390"

"CentOS 6.2" "220"

"Scientific Linux 6.9" "160"

"Oracle Linux Server 6.9" "108"

"CentOS Linux 6.0" "83"

"Red Hat Enterprise Linux Server 6.8" "73"

"Scientific Linux 6.8" "64"

"Oracle Linux Server 6.8" "53"

"Red Hat Enterprise Linux Server 6.7" "51"

"Red Hat Enterprise Linux Server 6.6" "47"

"Red Hat Enterprise Linux Server 6.5" "45"

"SHMZ 6.6" "42"

"CloudLinux Server 6.9" "31"

"Red Hat Enterprise Linux Server 6.4" "29"

"SHMZ 6.5" "23"

"ClearOS Community 6.9.0" "21"

"Scientific Linux 6.5" "18"

"debian 6.0.10" "17"

"Oracle Linux Server 6.7" "15"

"CentOS 6.1" "15"

"CentOS 5.11" "14"

"Scientific Linux 6.7" "14"

"Oracle Linux Server 6.6" "11"

"Scientific Linux 6.1" "11"

"Red Hat Enterprise Linux Server 6.2" "10"

"Ubuntu 10.04" "10"

"Scientific Linux 6.4" "10"

"Scientific Linux 6.6" "10"

"Red Hat Enterprise Linux Server 6.3" "9"

"Amazon Linux AMI 2014.09" "8"

"Oracle Linux Server 6.4" "8"

"Scientific Linux 6.3" "7"

"Scientific Linux CERN SLC 6.9" "7"

"Scientific Linux 6.2" "6"

"Oracle Linux Server 6.5" "6"

"Red Hat Enterprise Linux Server 6.0" "6"

"Scientific Linux 6.0" "6"

"Red Hat Enterprise Linux Server 6.1" "5"

"CentOS 5.8" "5"

As I understand it, using epel-release means package names are consistent, right? They're all pulling down the same package list once they have that? And they'll all have to, since everything here that we support that's redhat is a version of RHEL 6, essentially.

To be a little pedantic to share some background and make sure we're on the same page, yum install epel-release installs a CentOS specific package that automates setting up the EPEL repository on that system. It doesn't work on RHEL for example.

With that said, yes, the package names and contents in EPEL 6 are the same for all the RHEL 6 variants supported by the repo.

bmw · 2017-12-15T01:21:47Z

letsencrypt-auto-source/pieces/bootstrappers/rpm_python3.sh

+  #   - CentOS 6 (EPEL must be installed manually)
+
+  FindInstallTool
+  tool=$?


Unfortunately, there isn't proper support for functions in shell scripts. You can think of them more like subroutines allowing you to abstract and reuse pieces of code. The two ways to get tool out of this function are:

Remove this line entirely. tool was set inside the function. Yay for global state!

Have FindInstallTool print the name of the tool to stdout (probably by using echo) and change this line to tool="$(FindInstallTool)". If you do this, you should also modify FindInstallTool to send the error message to stderr so the tool environment variable doesn't eat it. Modifying the error function to print all messages to stderr is fine.

Either approach is fine, but I think the first approach is easier and is more common in certbot-auto. Bonus points for documenting that FindInstallTool does this.

EDIT: If we go with approach 1, we should probably change the variable tool to all caps.

done. by the way, how do I include the contents of rpm_common_base.sh in these scripts?

Sorry. I somehow missed this one yesterday.

To do this, modify letsencrypt-auto-source/letsencrypt-auto.template. letsencrypt-auto-source/build.py replaces instances of {{ path/to/file }} found in that template with the contents of the file. File paths should be relative to the letsencrypt-auto-source/pieces directory.

bmw · 2017-12-15T01:23:59Z

letsencrypt-auto-source/pieces/bootstrappers/rpm_common.sh

-    ca-certificates
-  "
+  FindInstallTool
+  tool=$?


See my comments in the Python 3 script about this.

bmw · 2017-12-15T01:46:31Z

letsencrypt-auto-source/letsencrypt-auto.template

+  DeterminePythonVersionBase $1 "$LE_PYTHON python2.7 python27 python2 python"
+}
+
+# TODO: use this to not upgrade if we already have python3 installed


Is this referring to when I said we should try to avoid rebootstrapping if they already have Python 3? It's my fault because I initially proposed this, but I believe in a follow up email I said that I think trying to do this can get really nasty and we should rely on the BOOTSTRAP_VERSION stuff to determine this.

Happy to chat/elaborate on this if you want though as I've been wrong before.

Yes. I thought having something like this might make it feasible, but on second thought we'd actually need all the other dependencies also, so unless we're switching entirely to checking for things before attempting to install them (which I am not proposing we do), we should probably just rebootstrap anyway.

bmw · 2017-12-15T01:48:58Z

letsencrypt-auto-source/letsencrypt-auto.template

    error "You have an ancient version of Python entombed in your operating system..."
    error "This isn't going to work; you'll need at least version 2.6."
    exit 1
  fi
+  export PYVER


Here's what you need to know about export for certbot-auto:

As you know, certbot-auto calls itself multiple times. If the environment variable needs to be preserved when certbot-auto reruns itself, you should export it, otherwise, you shouldn't as it's just clutter.

Looking at this code, PYVER is only used just after calling this function, so I think we can delete this line (but we should document that this variable is set).

I'd actually forgotten that that's how it accomplishes things, thanks for the reminder.

bmw · 2017-12-15T01:58:00Z

letsencrypt-auto-source/letsencrypt-auto.template

 DeterminePythonVersion() {
-  for LE_PYTHON in "$LE_PYTHON" python2.7 python27 python2 python; do
+  # TODO: check that this is the only executable we might have installed
+  if [ "$USE_PYTHON_3" = true ] ; then


We normally set variables like this equal to 1 and check for that rather than using true. There's some weird behavior around true and false that I don't fully understand and don't feel like digging into at the moment.

bmw · 2017-12-15T02:05:24Z

letsencrypt-auto-source/letsencrypt-auto.template

-  BOOTSTRAP_VERSION="BootstrapRpmCommon $BOOTSTRAP_RPM_COMMON_VERSION"
+  PREV_LE_PYTHON="$LE_PYTHON"
+  DeterminePython2Version "NOCRASH"
+  if [ "$PYVER" -eq 26 ] || [ "$PYVER" -eq 0 ]; then


Did you find Red Hat systems that might not have Python installed? Is our Python 3 approach expected to work on them?

We didn't previously assume that python would be installed, given that we try to install python as a dependency, so I was trying to keep that property. Unless we did assume that implicitly somewhere that I'd missed?

We didn't previously assume that, however, we do assume that yum is installed which (at least nowadays) is written in Python so it may be safe to make that assumption.

I'm just not sure if the "$PYVER" -eq 0 path will ever be used and if it is, if it's even the right thing to do. Do we expect a red hat system without a Python to be CentOS/RHEL 6 where we can install python34 from EPEL?

Yes, that is exactly what I was imagining. I didn't know that yum has python as a dependency.

I'm happy to remove the check. Worst thing that happens is that we accidentally fall through, install python2.6, then next time around we rebootstrap.

Actually, if I change this to if [ "$PYVER" -le 26 ] and fix up the other function, we could theoretically just as easily support people who have no python, python 2.6, or like some weird python 2.5 or whatever if that's even a thing, all just as easily, by installing python3 for them. Why not do that?

If there's a way we can easily do that, I'm certainly not against it, however, I'm not sure it's that simple because we're installing Python 3.4 using Red Hat tooling, package names, and a repository that only works on systems based on RHEL 6.

yeah ok supporting more things somehow sounds less fun, I'm ripping this nocrash stuff out.

bmw · 2017-12-15T02:09:18Z

letsencrypt-auto-source/letsencrypt-auto.template

+    BOOTSTRAP_VERSION="BootstrapRpmCommon $BOOTSTRAP_RPM_COMMON_VERSION"
+  fi
+  LE_PYTHON="$PREV_LE_PYTHON"
+  export LE_PYTHON


super nit: You can do this in one line like export LE_PYTHON="$PREV_LE_PYTHON".

bmw · 2017-12-19T05:18:05Z

I responded to specific threads, but since you're still working on this, I'd like to hold off giving it another full review until you think it's ready. If there's specific parts you'd like me to look at though, let me know.

ohemorange · 2017-12-19T20:24:00Z

Yes, given that I intended this as a WIP that wasn't ready for a full review anyway, very happy to not have another full review yet.

I would appreciate responses to #5329 (comment) and #5329 (comment).

bmw · 2017-12-20T23:58:44Z

Currently this is blocked by the problem described at #2208 (comment).

bmw · 2017-12-21T00:55:50Z

I think I responded to all the comments, but if there's anything I missed or new that you'd like my thoughts on, let me know.

ohemorange · 2017-12-21T01:16:41Z

letsencrypt-auto-source/letsencrypt-auto.template

+      BootstrapMessage "RedHat-based OSes that will use Python3"
+      BootstrapRpmPython3
+    }
+    export LE_PYTHON=python3


Ok, changed some things around here and also inside DeterminePythonVersion. These are the cases I considered:

their python as specified by LE_PYTHON doesn't exist

they didn't set le_python and they have no python installed

le_python is 2.6

le_python is 3.x

le_python is 2.7

I can go through why I think this is the right decision for each of them, just not right now.

LE_PYTHON affecting bootstrap selection

I'm unsure if we want to be taking LE_PYTHON set by the user into account when determining the bootstrap script to use.

My main reason for thinking this is the bootstrap script changing based on the user's environment can lead to some strange/obnoxious behavior. For example, you're running certbot-auto in cron as root and you get root email that says that certbot-auto needs to be run interactively to get permission to install new packages. You log in as your normal user and run certbot-auto. If you have LE_PYTHON set in one environment and not the other, a different bootstrap script may be selected in the two environments. This is similar to the issues we were having when there wasn't one Certbot install location on the system and it depended on the user's home directory.

If LE_PYTHON is set for the non-root user, it may not rebootstrap despite certbot-auto saying you have to. If LE_PYTHON is set for the root user, you may rebootstrap when running as a non-root user, but when certbot-auto is run again from cron, a different bootstrap script is selected so certbot-auto thinks you have to rebootstrap again!

If we do this, I think the only downside is we may install packages we may end up not needing, but doing this for the very few users who have been setting LE_PYTHON seems like it might be less harmful, confusing, and complex than the above behavior. If users know nothing else needs to be installed, there's a --no-bootstrap flag.

Overwriting LE_PYTHON

When setting LE_PYTHON here though, I think we should respect the value the user set it to and not just overwrite it. Setting LE_PYTHON=python2.6 and having it suddenly run in Python 3 is a little confusing and unexpected. I think we should only set this value to python3 if it wasn't set by the user.

My thought here is in the next release, you'll still be able to force Certbot to use Python 2.6 if you set LE_PYTHON. In the next release, we'll also start printing deprecation warnings if you're running in Python 2.6. In a subsequent release, we'll update DeterminePythonVersion to complain about your entombed Python version if it's less than 27 which should force you to stop using 2.6 entirely.

bmw · 2017-12-21T03:55:23Z

And while still a workaround, I think I suggested an even easier approach in #2208 (comment).

bmw · 2017-12-21T04:10:34Z

If you agree this new approach is simpler and cleaner, notice the CLI is slightly different and the flags we currently pass to virtualenv aren't supported by venv.

bmw · 2017-12-21T04:53:31Z

The way I'd probably do this is to use "$LE_PYTHON" -m venv if the python version is 3.3+.

Also, I have a TODO item for myself to talk to you about how to write tests for this. Writing meaningful automated tests for certbot-auto is really hard due to all the pieces here, the Docker tests using mock/dummy packages for a lot of stuff, auto update mechanisms, and the versions of all Python packages being pinned inside the script.

With this in mind, I'm personally not too worried about you submitting tests here. I'd test manually and maybe hack together a test farm test for local use if there's something there you want to test. I'll be doing the same when I review it.

bmw · 2017-12-21T19:22:22Z

letsencrypt-auto-source/pieces/bootstrappers/rpm_python3.sh

+  if $TOOL list python34 >/dev/null 2>&1; then
+    python_pkgs="python34
+      python34-devel
+      python-virtualenv


This is largely copied from the existing bootstrap script so you don't have to change it if you want, but I don't think we need python34-tools and python-pip. If we're using python3 -m venv, I don't think we need python-virtualenv either.

EDIT: Getting rid of python-virtualenv is probably fine, however, if we ever decided we wanted to switch back to virtualenv in this case, we'd have to rebootstrap if we don't include it now.

…ting LE_PYTHON

bmw

I still need to do more thorough testing, but other than some stylistic/code health comments below, this looks good to me. Everything worked great with the testing I've done so far.

Nice work and thanks for powering through all of the setbacks here.

bmw · 2018-01-03T20:13:05Z

letsencrypt-auto-source/letsencrypt-auto

-    $EXISTS "$LE_PYTHON" > /dev/null && break
-  done
+  # Arguments: "NOCRASH" if we shouldn't crash if we don't find a good python
+  if [ "$USE_PYTHON_3" -eq 0 ]; then


nit: This is probably fine, but if in the future certbot-auto is modified to call this function before the export USE_PYTHON_3=0 line below, this if statement will be false and we'll use the else branch and expect Python 3.

Since we're only currently using Python 3 on one system, I think having the opposite behavior if USE_PYTHON_3 is undefined is safer. To do this, I'd actually just flip these two branches, change this line to "$USE_PYTHON_3" = 1, and remove setting the variable for other systems.

We take this approach in a number of places in the script, notably during argument parsing where we only set the environment variable to 1 and leave it unset otherwise.

I agree. I wrote this before I realized there's a way to check for test if variables are set and set to something without crashing.

bmw · 2018-01-04T00:28:48Z

letsencrypt-auto-source/letsencrypt-auto

+      BootstrapMessage "RedHat-based OSes that will use Python3"
+      BootstrapRpmPython3
+    }
+    export USE_PYTHON_3=1


nit: I don't think we have to export this value as it will be set every time certbot-auto reruns itself.

bmw · 2018-01-04T00:28:51Z

letsencrypt-auto-source/letsencrypt-auto

@@ -698,6 +754,7 @@ BootstrapMageiaCommon() {
 # that function. If Bootstrap is set to a function that doesn't install any
 # packages (either because --no-bootstrap was included on the command line or
 # we don't know how to bootstrap on this system), BOOTSTRAP_VERSION is not set.
+export USE_PYTHON_3=0


nit: I don't think we have to export this value as it will be set every time certbot-auto reruns itself.

bmw · 2018-01-04T00:53:31Z

letsencrypt-auto-source/pieces/bootstrappers/rpm_common_base.sh

+# If new packages are installed by BootstrapRpmCommonBase below, version
+# numbers in rpm_common.sh and rpm_python3.sh must be increased.
+
+# Sets TOOL to the name of the package manager


This function now does more than just determine the name of the package manager and sets more environment variables.

This function also sets QUIET_FLAG and yes_flag that are used outside of the function. It's not a big deal, but documenting this seems like a good idea to me and we should probably change yes_flag to uppercase while we're at it.

bmw · 2018-01-04T01:00:24Z

letsencrypt-auto-source/pieces/bootstrappers/rpm_python3.sh

+  if $TOOL list python34 >/dev/null 2>&1; then
+    python_pkgs="python34
+      python34-devel
+      python34-tools


Did you determine that this package is necessary?

nope, but I don't know what we were using it for in the first place, in the non-3 python version.

Looking into this, I found #1823. There and in the linked blogpost, one person claims python-tools and python-pip need to be installed without really saying why. Initially, the post talks about CentOS 7 and later says they are necessary to fix InsecurePlatformWarning. A few things:

The installation of these packages do not fix InsecurePlatformWarning per my testing.

InsecurePlatformWarning is not a fatal error.

The error isn't shown in normal output (unless you asked for increase verbosity) because the error is irrelevant as we're verifying the hashes of the downloaded packages.

With that said, I looked into the contents of the python34-tools package using repoquery as described here. This package contains:

2to3 and IDLE placed in the user's PATH.

A bunch of Python developer tools and demos that aren't placed in the user's PATH or in a location importable by Python.

I think this package is cruft and can be removed, especially in the Python 3.4 case which doesn't (currently) have InsecurePlatformWarning problems. I think this cleans up certbot-auto and reduces our dependency count a bit, but the installed package is only 1.2 MB. If you'd prefer to leave it in despite what I've found, that's fine with me.

It sounds like this isn't a python3-specific problem, which is what this PR is about fixing. If we're removing it, it belongs in a separate PR that can be separately reverted if something goes wrong. Happy to also get that in for this release.

bmw · 2018-01-04T01:01:46Z

letsencrypt-auto-source/tests/auto_test.py

@@ -349,6 +350,7 @@ def test_openssl_failure(self):
                    self.assertTrue("Couldn't verify signature of downloaded "
                                    "certbot-auto." in exc.output)
                else:
+                    print(out)


Did you mean to leave this print statement here?

bmw · 2018-01-04T01:34:47Z

letsencrypt-auto-source/pieces/fetch.py

+    # PROTOCOL_TLS isn't available before 2.7.13 but this code is for 2.7.9+, so use this.
+    context = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
+
+    context.check_hostname = False


nit: I don't think this line is necessary if verify_mode is set to CERT_NONE.

ohemorange · 2018-01-04T02:39:00Z

For posterity, I was using these commands for installing python and testing manually and such:

sudo su

# from https://danieleriksson.net/2017/02/08/how-to-install-latest-python-on-centos/

# Start by making sure your system is up-to-date:
yum update
# Compilers and related tools:
yum groupinstall -y "development tools"
# Libraries needed during compilation to enable all features of Python:
yum install -y zlib-devel bzip2-devel openssl-devel ncurses-devel sqlite-devel readline-devel tk-devel gdbm-devel db4-devel libpcap-devel xz-devel expat-devel
# If you are on a clean "minimal" install of CentOS you also need the wget tool:
yum install -y wget

# Python 2.7.14:
wget http://python.org/ftp/python/2.7.14/Python-2.7.14.tar.xz
tar xf Python-2.7.14.tar.xz
cd Python-2.7.14
./configure --prefix=/usr/local --enable-unicode=ucs4 --enable-shared LDFLAGS="-Wl,-rpath /usr/local/lib"
make && make altinstall

cp /usr/local/bin/python2.7 /usr/bin/
cd -
exit


pytest -v -s certbot/letsencrypt-auto-source/tests

# should not install python3 packages
sudo certbot/letsencrypt-auto-source/letsencrypt-auto --no-self-upgrade

sudo rm -f /usr/bin/python2.7
# should install python3 packages
sudo certbot/letsencrypt-auto-source/letsencrypt-auto --no-self-upgrade

…d only if appropriate to do so.

ohemorange · 2018-01-06T00:33:42Z

Expected output of the test I just added:

user@machine:~/certbot/letsencrypt-auto-source$ ./build.py && docker build -f Dockerfile.centos6 -t lea . && docker run --rm -t -i lea 
Sending build context to Docker daemon 200.7 kB
Step 1/16 : FROM centos:6
 ---> df3764b1d215
Step 2/16 : RUN yum install -y epel-release
 ---> Using cache
 ---> b49f8dcbb76d
Step 3/16 : RUN yum install -y python-pip sudo
 ---> Using cache
 ---> 034ae3f17580
Step 4/16 : COPY ./pieces/pipstrap.py /opt
 ---> Using cache
 ---> 5fef3b4722ea
Step 5/16 : RUN /opt/pipstrap.py
 ---> Using cache
 ---> 8df8335f85ac
Step 6/16 : RUN pip install pytest==3.2.5
 ---> Using cache
 ---> b4ec309aa6ed
Step 7/16 : RUN useradd --create-home --home-dir /home/lea --shell /bin/bash --groups wheel --uid 1000 lea
 ---> Using cache
 ---> 94cf48ffe842
Step 8/16 : RUN sed -i.bkp -e       's/# %wheel\(NOPASSWD: ALL\)\?/%wheel/g'       /etc/sudoers
 ---> Using cache
 ---> 5dd3e730f731
Step 9/16 : RUN mkdir -p /home/lea/certbot
 ---> Using cache
 ---> 1ce4cfafca9c
Step 10/16 : COPY ./tests/certs/ca/my-root-ca.crt.pem /usr/local/share/ca-certificates/
 ---> Using cache
 ---> f4c64eaecf86
Step 11/16 : RUN update-ca-trust
 ---> Using cache
 ---> e9c210bdf50d
Step 12/16 : COPY . /home/lea/certbot/letsencrypt-auto-source
 ---> 0249d6180351
Removing intermediate container 647ce3a0ac85
Step 13/16 : USER lea
 ---> Running in efc4c2d9dae7
 ---> 2af281e22d92
Removing intermediate container efc4c2d9dae7
Step 14/16 : WORKDIR /home/lea
 ---> 89cec1c128a7
Removing intermediate container 372f4cbb8e65
Step 15/16 : RUN sudo chmod +x certbot/letsencrypt-auto-source/tests/centos6_tests.sh
 ---> Running in f07f1390a3f7
 ---> abe1de68b910
Removing intermediate container f07f1390a3f7
Step 16/16 : CMD sudo certbot/letsencrypt-auto-source/tests/centos6_tests.sh
 ---> Running in 864a4080d505
 ---> de9a946aad41
Removing intermediate container 864a4080d505
Successfully built de9a946aad41

PASSED: Did not upgrade to Python3 when Python2.7 is present.
PASSED: Successfully upgraded to Python3 when only Python2.6 is present.

============================================================================== test session starts ==============================================================================
platform linux2 -- Python 2.6.6, pytest-3.2.5, py-1.4.34, pluggy-0.4.0 -- /usr/bin/python
cachedir: .cache
rootdir: /home/lea, inifile:
collected 5 items                                                                                                                                                                

certbot/letsencrypt-auto-source/tests/auto_test.py::tests_dir SKIPPED
certbot/letsencrypt-auto-source/tests/auto_test.py::AutoTests::test_openssl_failure PASSED
certbot/letsencrypt-auto-source/tests/auto_test.py::AutoTests::test_phase2_upgrade PASSED
certbot/letsencrypt-auto-source/tests/auto_test.py::AutoTests::test_pip_failure PASSED
certbot/letsencrypt-auto-source/tests/auto_test.py::AutoTests::test_successes PASSED

===================================================================== 4 passed, 1 skipped in 42.00 seconds ======================================================================

bmw

We have a couple things that we need to fix that are due to the rebootstrapping system (see #5387 and #5388), but we're merging this now because I think the PR otherwise looks good and it's easier to work on it after it has been squashed and merged.

@erikrose, we'd still like your review on this if you're able to give one.

@ohemorange, I added a little more of my thoughts to #5388 if you're interested.

erikrose · 2018-01-09T19:19:27Z

letsencrypt-auto-source/letsencrypt-auto.template

-    BootstrapRpmCommon
-  }
-  BOOTSTRAP_VERSION="BootstrapRpmCommon $BOOTSTRAP_RPM_COMMON_VERSION"
+  prev_le_python="$LE_PYTHON"


I think I finally figured out what's going on here, but I could use a nice big comment, something like the lines of "We run DeterminePythonVersion to decide, on the basis of available Python versions, whether to use 2.x or 3.x on RedHat-like systems. Then we revert the returned values to their previous states."

Though I notice we don't revert PYVER. Is that going to be a problem?

Adding a comment is a good idea. Not reverting PYVER isn't a problem.

(I can explain more if you want; it's related to #5388)

As long as you have the PYVER reasoning clear in your head, that's good enough for me. :-)

erikrose · 2018-01-09T19:29:39Z

letsencrypt-auto-source/letsencrypt-auto.template

-    $EXISTS "$LE_PYTHON" > /dev/null && break
-  done
+  # Arguments: "NOCRASH" if we shouldn't crash if we don't find a good python
+  if [ -n "$USE_PYTHON_3" ]; then


While this is technically correct, we might dodge a future surprise if we use [ -n "$USE_PYTHON_3" != "1" ] instead, so somebody doesn't come along later (or even outside the script), set USE_PYTHON_3=0, and expect the obvious thing to happen.

erikrose · 2018-01-09T19:46:07Z

letsencrypt-auto-source/letsencrypt-auto.template

+      BootstrapRpmPython3
+    }
+    USE_PYTHON_3=1
+    BOOTSTRAP_VERSION="BootstrapRpmPython3 $BOOTSTRAP_RPM_PYTHON3_VERSION"


I think our assumption before was than a given box would never change the bootstrapper it's using. Let me try to construct a scenario that would break.... No, I think we're okay. If the bootstrapper OR the bootstrap version changes, we simply rebootstrap entirely. We never implemented the "differential" bootstrapping, where we install, say, only the new packages that are required.

erikrose · 2018-01-09T19:57:32Z

letsencrypt-auto-source/pieces/bootstrappers/rpm_common_base.sh

+      sleep 1s
+      /bin/echo -ne "\e[0K\rEnabling the EPEL repository in 2 seconds..."
+      sleep 1s
+      /bin/echo -e "\e[0K\rEnabling the EPEL repository in 1 seconds..."


If we're going to repeat ourselves, we might as well say "1 second". :-)

I would like to state for posterity that the line was there before (this part was just moved to a new name) and I am not personally responsible for this grammatical heresy.

Ha! Okay, you are absolved.

erikrose · 2018-01-09T20:12:51Z

letsencrypt-auto-source/pieces/fetch.py

@@ -112,6 +120,14 @@ def verified_new_le_auto(get, tag, temp_dir):
                            "certbot-auto.", exc)


+def create_CERT_NONE_context():


For consistency with the noun-like names of other functions that have return values, this would be better named "cert_none_context".

erikrose · 2018-01-09T20:15:30Z

letsencrypt-auto-source/tests/auto_test.py

@@ -202,6 +202,7 @@ def run_le_auto(le_auto_path, venv_dir, base_url, **kwargs):
 Z5o/NDiQB11m91yNB0MmPYY9QSbnOA9j7IaaC97AwRLuwXY+/R2ablTcxurWou68
 iQIDAQAB
 -----END PUBLIC KEY-----""",
+             NO_CERT_VERIFY='1',


I'm curious: what changed to make this needed?

We run the tests using a different version of python in some cases. At some point Python changed the default behavior from "don't bother actually checking if a cert is valid" to "actually do". But we need it to not do that for the test.

Innnnteresting. I'm still not sure if we're on the same page. All that stuff under tests/certs/ is the gymnastics I did to ensure that the certs would in fact be valid. And I think I was using 2.7.9 or higher when I wrote it; that's the version when the cert validation started happening. Where were we having validation errors?

erikrose · 2018-01-09T20:17:19Z

letsencrypt-auto-source/pieces/fetch.py

    # metadata['info']['version'] actually returns the latest of any kind of
    # release release, contrary to https://wiki.python.org/moin/PyPIJSON.
    # The regex is a sufficient regex for picking out prereleases for most
    # packages, LE included.
    return str(max(LooseVersion(r) for r
-                   in metadata['releases'].iterkeys()
+                   in iter(metadata['releases'].keys())


I think iter() is pretty much a no-op in this situation: it just makes explicit the calling of __iter__ on the result of keys(), which the generator expression does implicitly.

I thought it was needed for Python3, but I'll double check that.

erikrose · 2018-01-09T20:36:51Z

letsencrypt-auto-source/tests/centos6_tests.sh

+fi
+
+# bootstrap, but don't install python 3.
+certbot/letsencrypt-auto-source/letsencrypt-auto --no-self-upgrade -n > /dev/null 2> /dev/null


I recoil from this a bit. It seems we're splitting the tests (for CentOS only) across the Dockerfile and the Python tests in auto_test.py. None of the other Dockerfiles make assertions or emit pass/fail messages. Unless there's a strong motivation I'm missing, I urge us to incorporate these into the Python-driven tests. If this is very awkward to spell in Python, we should at least kick off a shell script from auto_test.

The Python tests are run on all the systems that we test. We don't want these tests to be run on anything other than CentOS. We could add a flag to the Python tests to run certain tests, but given that these tests are mostly about installing packages then rerunning certbot-auto, it's more straightforward to just do it this way. The Python test would essentially be calling out to the shell to install things, which seems even weirder.

We don't want these tests to be run on anything other than CentOS.

Ah, that's the strong motivation I was missing. Thanks! In that case, what I could use is a few comments: one demarcating the division between the Dockerfile tests and the Python ones saying pretty much what you said above, and another explaining how Python 2.7 disappears by line 42. (Does le-auto remove a repo or something, and it goes poof?)

erikrose · 2018-01-09T20:39:37Z

A couple of cleanliness things and 2 or 3 things of more import. No huge showstoppers. That's all I have to say about that! :-) Thanks for your hard work on a twisty maze of shell commands, @ohemorange!

* Use josepy instead of acme.jose. (#5203) * Parse variables without whitespace separator correctly in CentOS family of distributions (#5318) * Pin josepy in letsencrypt-auto (#5321) * pin josepy in le-auto * Put pinned versions in sorted order * Pin dependencies in oldest tests (#5316) * Add tools/merge_requirements.py * Revert "Fix oldest tests by pinning Google DNS deps (#5000)" This reverts commit f68fba2. * Add tools/oldest_constraints.txt * Remove oldest constraints from tox.ini * Rename dev constraints file * Update tools/pip_install.sh * Update install_and_test.sh * Fix pip_install.sh * Don't cat when you can cp * Add ng-httpsclient to dev constraints for oldest tests * Bump tested setuptools version * Update dev_constraints comment * Better document oldest dependencies * test against oldest versions we say we require * Update dev constraints * Properly handle empty lines * Update constraints gen in pip_install * Remove duplicated zope.component * Reduce pyasn1-modules dependency * Remove blank line * pin back google-api-python-client * pin back uritemplate * pin josepy for oldest tests * Undo changes to install_and_test.sh * Update install_and_test.sh description * use split instead of partition * More pip dependency resolution workarounds (#5339) * remove pyopenssl and six deps * remove outdated tox.ini dep requirement * Fix auto_tests on systems with new bootstrappers (#5348) * Fix pytest on macOS in Travis (#5360) * Add tools/pytest.sh * pass TRAVIS through in tox.ini * Use tools/pytest.sh to run pytest * Add quiet to pytest.ini * ignore pytest cache * print as a string (#5359) * Use apache2ctl modules for Gentoo systems. (#5349) * Do not call Apache binary for module reset in cleanup() * Use apache2ctl modules for Gentoo * Broader git ignore for pytest cache files (#5361) Make gitignore take pytest cache directories in to account, even if they reside in subdirectories. If pytest is run for a certain module, ie. `pytest certbot-apache` the cache directory is created under `certbot-apache` directory. * Fix letsencrypt-auto name and long forms of -n (#5375) * Deprecate Python2.6 by using Python3 on CentOS/RHEL 6 (#5329) * If there's no python or there's only python2.6 on red hat systems, install python3 * Always check for python2.6 * address style, documentation, nits * factor out all initialization code * fix up python version return value when no python installed * add no python error and exit * document DeterminePythonVersion parameters * build letsencrypt-auto * close brace * build leauto * fix syntax errors * set USE_PYTHON_3 for all cases * rip out NOCRASH * replace NOCRASH, update LE_PYTHON set logic * use built-in venv for py3 * switch to LE_PYTHON not affecting bootstrap selection and not overwriting LE_PYTHON * python3ify fetch.py * get fetch.py working with python2 and 3 * don't verify server certificates in fetch.py HttpsGetter * Use SSLContext and an environment variable so that our tests continue to never verify server certificates. * typo * build * remove commented out code * address review comments * add documentation for YES_FLAG and QUIET_FLAG * Add tests to centos6 Dockerfile to make sure we install python3 if and only if appropriate to do so. * Allow non-interactive revocation without deleting certificates (#5386) * Add --delete-after-revoke flags * Use delete_after_revoke value * Add delete_after_revoke unit tests * Add integration tests for delete-after-revoke. * Have letsencrypt-auto do a real upgrade in leauto-upgrades option 2 (#5390) * Make leauto_upgrades do a real upgrade * Cleanup vars and output * Sleep until the server is ready * add simple_http_server.py * Use a randomly assigned port * s/realpath/readlink * wait for server before getting port * s/localhost/all interfaces * update Apache ciphersuites (#5383) * Fix macOS builds for Python2.7 in Travis (#5378) * Add OSX Python2 tests * Make sure python2 is originating from homebrew on macOS * Upgrade the already installed python2 instead of trying to reinstall

erikrose · 2018-01-10T15:24:00Z

letsencrypt-auto-source/tests/centos6_tests.sh

+python2.7 --version 2> /dev/null
+RESULT=$?
+if [ $RESULT -ne 0 ]; then
+  error "Python3 is not available."


Do you mean Python 2.7?

ohemorange added 2 commits December 12, 2017 18:07

If there's no python or there's only python2.6 on red hat systems, in…

dee46d8

…stall python3

Always check for python2.6

f60ff96

bmw requested changes Dec 15, 2017

View reviewed changes

address style, documentation, nits

defd06b

ohemorange added 8 commits December 19, 2017 19:40

factor out all initialization code

533b45c

fix up python version return value when no python installed

694d7b1

add no python error and exit

a2c1e4b

document DeterminePythonVersion parameters

2b2b93f

build letsencrypt-auto

8502502

close brace

4076ae7

build leauto

dd52dfa

Merge branch 'master' into killpy26

6b5100d

ohemorange added 3 commits December 20, 2017 16:16

fix syntax errors

48cbaf2

set USE_PYTHON_3 for all cases

f23dc97

rip out NOCRASH

eed279c

replace NOCRASH, update LE_PYTHON set logic

07975a5

ohemorange commented Dec 21, 2017

View reviewed changes

bmw reviewed Dec 21, 2017

View reviewed changes

ohemorange added 5 commits December 21, 2017 13:58

use built-in venv for py3

c5102f1

switch to LE_PYTHON not affecting bootstrap selection and not overwri…

48f0cf1

…ting LE_PYTHON

python3ify fetch.py

173c819

get fetch.py working with python2 and 3

80ef7c6

don't verify server certificates in fetch.py HttpsGetter

5a58af7

bmw requested changes Jan 4, 2018

View reviewed changes

address review comments

835e4c0

ohemorange assigned bmw Jan 4, 2018

add documentation for YES_FLAG and QUIET_FLAG

007383d

bmw mentioned this pull request Jan 4, 2018

Stop installing python-pip and python-tools in Red Hat Bootstrappers #5364

Closed

ohemorange requested a review from erikrose January 5, 2018 01:20

ohemorange assigned erikrose Jan 5, 2018

bmw mentioned this pull request Jan 5, 2018

Have letsencrypt-auto do a real upgrade in leauto-upgrades #5371

Closed

Add tests to centos6 Dockerfile to make sure we install python3 if an…

1d3141e

…d only if appropriate to do so.

bmw mentioned this pull request Jan 6, 2018

Fix letsencrypt-auto name and long forms of -n #5375

Merged

bmw approved these changes Jan 8, 2018

View reviewed changes

bmw merged commit 8585cdd into master Jan 8, 2018

bmw added this to the 0.21.0 milestone Jan 8, 2018

erikrose reviewed Jan 9, 2018

View reviewed changes

ohemorange added a commit that referenced this pull request Jan 10, 2018

Address erikrose's comments on #5329

dcf6c64

ohemorange mentioned this pull request Jan 10, 2018

Address erikrose's comments on #5329 #5400

Merged

bmw pushed a commit that referenced this pull request Jan 10, 2018

Address erikrose's comments on #5329 (#5400)

6eb4593

erikrose reviewed Jan 10, 2018

View reviewed changes

bmw mentioned this pull request Feb 12, 2018

python3 renamings #5060

Closed

"os"	"_count_distinct"
"CentOS 6.9"	"22480"
"CentOS 6.8"	"8180"
"CentOS 6.7"	"3236"
"CentOS 6.5"	"2885"
"CentOS 6.6"	"2088"
"CentOS 6.4"	"815"
"Red Hat Enterprise Linux Server 6.9"	"455"
"CentOS 6.3"	"390"
"CentOS 6.2"	"220"
"Scientific Linux 6.9"	"160"
"Oracle Linux Server 6.9"	"108"
"CentOS Linux 6.0"	"83"
"Red Hat Enterprise Linux Server 6.8"	"73"
"Scientific Linux 6.8"	"64"
"Oracle Linux Server 6.8"	"53"
"Red Hat Enterprise Linux Server 6.7"	"51"
"Red Hat Enterprise Linux Server 6.6"	"47"
"Red Hat Enterprise Linux Server 6.5"	"45"
"SHMZ 6.6"	"42"
"CloudLinux Server 6.9"	"31"
"Red Hat Enterprise Linux Server 6.4"	"29"
"SHMZ 6.5"	"23"
"ClearOS Community 6.9.0"	"21"
"Scientific Linux 6.5"	"18"
"debian 6.0.10"	"17"
"Oracle Linux Server 6.7"	"15"
"CentOS 6.1"	"15"
"CentOS 5.11"	"14"
"Scientific Linux 6.7"	"14"
"Oracle Linux Server 6.6"	"11"
"Scientific Linux 6.1"	"11"
"Red Hat Enterprise Linux Server 6.2"	"10"
"Ubuntu 10.04"	"10"
"Scientific Linux 6.4"	"10"
"Scientific Linux 6.6"	"10"
"Red Hat Enterprise Linux Server 6.3"	"9"
"Amazon Linux AMI 2014.09"	"8"
"Oracle Linux Server 6.4"	"8"
"Scientific Linux 6.3"	"7"
"Scientific Linux CERN SLC 6.9"	"7"
"Scientific Linux 6.2"	"6"
"Oracle Linux Server 6.5"	"6"
"Red Hat Enterprise Linux Server 6.0"	"6"
"Scientific Linux 6.0"	"6"
"Red Hat Enterprise Linux Server 6.1"	"5"
"CentOS 5.8"	"5"

		@@ -112,6 +120,14 @@ def verified_new_le_auto(get, tag, temp_dir):
		"certbot-auto.", exc)


		def create_CERT_NONE_context():

Deprecate Python2.6 by using Python3 on CentOS/RHEL 6 #5329

Deprecate Python2.6 by using Python3 on CentOS/RHEL 6 #5329

Conversation

ohemorange commented Dec 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmw Dec 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmw Dec 20, 2017 • edited

Choose a reason for hiding this comment

bmw Dec 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmw Dec 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmw Dec 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmw Dec 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmw commented Dec 19, 2017

ohemorange commented Dec 19, 2017

bmw commented Dec 20, 2017

bmw commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LE_PYTHON affecting bootstrap selection

Overwriting LE_PYTHON

bmw commented Dec 21, 2017

bmw commented Dec 21, 2017

bmw commented Dec 21, 2017

bmw Dec 21, 2017 • edited

Choose a reason for hiding this comment

bmw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohemorange commented Jan 4, 2018 • edited

ohemorange commented Jan 6, 2018

bmw Dec 19, 2017 •

edited

bmw Dec 20, 2017 •

edited

bmw Dec 20, 2017 •

edited

bmw Dec 15, 2017 •

edited

bmw Dec 15, 2017 •

edited

bmw Dec 15, 2017 •

edited

bmw Dec 21, 2017 •

edited

ohemorange commented Jan 4, 2018 •

edited

erikrose Jan 9, 2018 •

edited

erikrose Jan 9, 2018 •

edited