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

Merge after #1160 - Docker - update to using Ubuntu 20.04 + minor changes related to recent PRs #1242

Merged
merged 11 commits into from
Mar 26, 2021

Conversation

taniwallach
Copy link
Member

Update to using Ubuntu 20.04 as base OS image.

Additional Ubuntu packages:

docker-compose.yml:

  • Set fixed container names (so names will not depend on name of directory where docker-compose is being run.

  • Sample lines to use some new sample DB config files + brief comments.

  • Sample line to make host /etc/localtime read-only available to the DB container.

  • Sample ulimits + timezone lines for the DB container.

  • Set the Docker container to mount the updated Lite.pm with the UTF-8 patch.

  • Sample lines to use either DB connector. Default remains the "old" DBD::mysql driver.

  • Modify the bottom "main" volumes section, so fixed persistent volumes are used for the OPL and the DB storage, regardless of the directory name where docker-compose is run.

Additional Ubuntu packages:
	libcgi-pm-perl (for CGI::Cookie)
	libdbd-mariadb-perl (for alternate DB connector)
	iputils-ping (prevents ping during startup from failing)
	imagemagick (needed by openwebwork/pg#541)

docker-compose.yml:

  * Set fixed container names (so names will not depend on name of directory where docker-compose is being run.

  * Sample lines to use some new sample DB config files + brief comments.

  * Sample line to make host /etc/localtime read-only available to the DB container.

  * Sample ulimits + timezone lines for the DB container.

  * Set the Docker container to mount the updated Lite.pm with the UTF-8 patch.

  * Sample lines to use either DB connector. Default remains the "old" DBD::mysql driver.

  * Modify the bottom "main" volumes section, so fixed persistant volumes are used for the OPL and the DB storage, regardless of the directory name where docker-compose is run.
@drgrice1
Copy link
Member

I think it would be a good idea to add
sed -i 's/<policy domain="coder" rights="none" pattern="PDF" \/>/<policy domain="coder" rights="read" pattern="PDF" \/>/' /etc/ImageMagick-6/policy.xml
to the RUN command that starts on line 258 of the Dockerfile also. This is needed to actually make the PGtikz.pl macro work. Installing imagemagick by itself doesn't quite do that as you know.

I will try to take a deeper look at the rest of this.

…code to work, and a comment explaining how to enable it via the prepared sample line in docker-compose.yml.
@taniwallach
Copy link
Member Author

I think it would be a good idea to add
sed -i 's/<policy domain="coder" rights="none" pattern="PDF" \/>/<policy domain="coder" rights="read" pattern="PDF" \/>/' /etc/ImageMagick-6/policy.xml
to the RUN command that starts on line 258 of the Dockerfile also. This is needed to actually make the PGtikz.pl macro work. Installing imagemagick by itself doesn't quite do that as you know.

Since we think that making that change involves a potential security risk if some malicious PDF file were to be processed (somehow) - I think it is better to force the end user to actively enable that change. I set up such an option in the newly added second commit.

@drgrice1
Copy link
Member

I agree that it is better to be optional. However, there has to be better way to do it than adding the entire policy.xml file for a single word change.

@taniwallach
Copy link
Member Author

I agree that it is better to be optional. However, there has to be better way to do it than adding the entire policy.xml file for a single word change.

I cannot think of anything simple which requires minimal effort for the end user.

  1. We could tell users how to set this up themselves (docker cp and then edit docker-config.yml).
  2. We could add a control setting as an environment variable, and the modify the file on each container startup using docker-entrypoint.sh if the setting was enabled.

I prefer to minimize repeated work on each startup, so don't like the second option.

Many of the current and potential Docker users start off as newbies to Docker (so did I) and may find the process challenging when they get started and choose to ignore it until the issue makes problems.

Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

It seems that there is a lot of work to do on getting webwork ready to go for using the docker install in production. This pull request perhaps addresses some of that. Of course one of the biggest things needed is better documenation, but that is another matter.

One thing that needs to be done is to rename docker-compose.yml to docker-compose.yml.dist. That is a file that someone using the docker installation will need to modify, and so directions should specify to copy the .dist file as usual. The .env file also needs to be renamed to .env.dist as that file will also need to be modified for a production installation. Most of the files in the docker_config directory will needs this as well. It is debatable as to if the Dockerfile should also be renamed in this way.

The current instructions on the wiki state to build the container using docker build .. However, as we are using docker-compose.yml, this should be docker-compose build. This is the proper way to build the container when using a docker-compose.yml file. If building in this way it also adds to what you can do in terms of configuration via the docker-compose.yml file. For example, you can use the options to the build. You could add args that apply at build time and are used in the Dockerfile.

There is no need to add the entire Lite.pm file or the ImageMagick-6/policy.xml file.

For the Lite.pm file, that change is always done, so just modify the file in the Dockerfile. There are ways this can be done without including the entire Lite.pm file in the repository. There is a much bigger issue here though. That is why is this being done at all? If there is an issue that need this change, then what is it? This is just thrown in here with no rationale. If this change is needed, then this will also need to be documented for those that are not using a docker install. The limits.conf, mariadb.cnf, and my.cnf files added here need explanation as well for those installing without the docker approach.

For the policy.xml file the pdf read policy should be optional as we have already discussed. One way that this can be implemented without importing the entire linux distribution file that is convenient for the end user, and does not require anything added to the docker-entrypoint.sh script is to utilize build args in the docker-compose.yml file. Of course, this involves switching to using docker-compose build. Build args can still be utilized even if that switch is not made, but can be passed on the command line. They would just need to be documented.

There is another issue with how the webwork.apache2.4-config file is added to the docker build. The way that is done in the Dockerfile does not allow for customization by the system administrator. That is necessary. Even with the docker build there is no one size fits all for that file.

Also, this pull request should not be merged until after #1160.

docker-compose.yml Outdated Show resolved Hide resolved
@taniwallach taniwallach requested a review from drgrice1 March 11, 2021 22:54
@taniwallach
Copy link
Member Author

Although I tested on my local Docker setup with the PerlPassEnv changes from the last commit made manually to webwork.conf - I have not yet rebuilt an image after these last changes. I'm pretty confident that the change to Dockerfile was done properly, and with the changes to the database settings from this and the code from #1160 in use - the database and the relevant bin/ scripts all seem to work properly for me.

Unfortunately, I will not have more time to work on / test this until next week.

@taniwallach taniwallach changed the title Docker - update to using Ubuntu 20.04 + minor changes related to recent PRs Merge after #1160 - Docker - update to using Ubuntu 20.04 + minor changes related to recent PRs Mar 11, 2021
and resolve conflict in Dockerfile.
@taniwallach
Copy link
Member Author

One thing that needs to be done is to rename docker-compose.yml to docker-compose.yml.dist. That is a file that someone using the docker installation will need to modify, and so directions should specify to copy the .dist file as usual. The .env file also needs to be renamed to .env.dist as that file will also need to be modified for a production installation.

A simple Docker user can just use the file as is, and until now it has been distributed as docker-compose.yml and not as docker-compose.yml.dist. Same for .env, which was set up to keep some passwords and config out of the Dockerfile, but does require local customization.

It is debatable as to if the Dockerfile should also be renamed in this way.

I think the goal at present was to make starting to use WW via Docker as simple as possible, and the https://github.com/openwebwork/webwork2/wiki/Docker-newbie-instructions assume that this is how things are. For now, I would prefer to leave things more or less as is, and consider how to change the Docker setup in the future.

The current instructions on the wiki state to build the container using docker build. However, as we are using docker-compose.yml, this should be docker-compose build. This is the proper way to build the container when using a docker-compose.yml file.

Fixed. I've been doing this in practice for a long time, and did not recall that the documentation still had docker build.

If building in this way it also adds to what you can do in terms of configuration via the docker-compose.yml file. For example, you can use the options to the build. You could add args that apply at build time and are used in the Dockerfile.

Fixed.

There is no need to add the entire Lite.pm file or the ImageMagick-6/policy.xml file.

I agree that a better approach would be good, but it should happen once at image build time, and at present I don't have time to work on creating code in the Dockerfile to conditionally modify ImageMagick-6/policy.xml and always to modify Lite.pm.

For the Lite.pm file, that change is always done, so just modify the file in the Dockerfile. There are ways this can be done without including the entire Lite.pm file in the repository. There is a much bigger issue here though. That is why is this being done at all? If there is an issue that need this change, then what is it?

Needed for UTF-8 support in XMLRPC. Covered elsewhere in GitHub discussions.

This is just thrown in here with no rationale. If this change is needed, then this will also need to be documented for those that are not using a docker install. The limits.conf, mariadb.cnf, and my.cnf files added here need explanation as well for those installing without the docker approach.

For the policy.xml file the pdf read policy should be optional as we have already discussed. One way that this can be implemented without importing the entire linux distribution file that is convenient for the end user, and does not require anything added to the docker-entrypoint.sh script is to utilize build args in the docker-compose.yml file. Of course, this involves switching to using docker-compose build. Build args can still be utilized even if that switch is not made, but can be passed on the command line. They would just need to be documented.

If you can make a PR to my branch to do this, I'll test and merge it.

There is another issue with how the webwork.apache2.4-config file is added to the docker build. The way that is done in the Dockerfile does not allow for customization by the system administrator. That is necessary. Even with the docker build there is no one size fits all for that file.

Before my time, and I think most real local modification is best done by creating fixed local config files and mounting them via docker-compose.yml. The exiting code was set up to allow a "localhost" developer using Docker to get up and running with minimal effort, and I think that works well enough.

@drgrice1
Copy link
Member

One thing that needs to be done is to rename docker-compose.yml to docker-compose.yml.dist.

A simple Docker user can just use the file as is, and until now it has been distributed as docker-compose.yml and not as docker-compose.yml.dist. Same for .env, which was set up to keep some passwords and config out of the Dockerfile, but does require local customization.

A copy command or two is not complicated. Until this is done we are saying that the Docker approach is not ready for production yet. If that is our current status, then we can leave it the way it is. If the Docker approach is ready to be used in production though, then this needs to change now.

For the Lite.pm file, ... That is why is this being done at all? If there is an issue that need this change, then what is it?

Needed for UTF-8 support in XMLRPC. Covered elsewhere in GitHub discussions.

Can you provide a reference? I have not found any reference to this in any GitHub discussions.

For the policy.xml file ...

If you can make a PR to my branch to do this, I'll test and merge it.

I will try to put in a pull request that implements a mechanism for changing the policy.xml and Lite.pm files without pulling in the entire files.

There is another issue with how the webwork.apache2.4-config file is added to the docker build. ...

Before my time, and I think most real local modification is best done by creating fixed local config files and mounting them via docker-compose.yml. The exiting code was set up to allow a "localhost" developer using Docker to get up and running with minimal effort, and I think that works well enough.

This is okay. You can mount via docker-compose.yml and override the installed file.

@taniwallach
Copy link
Member Author

A copy command or two is not complicated.

For Lite.pm using a COPY command in Dockerfile makes sense. For UTF-8 support, that is mission critical.

I'm not sure how to conditionally change the ImageMagick-6/policy.xml file in a Dockerfile so it will only be active if the admin wants it to be.

Until this is done we are saying that the Docker approach is not ready for production yet. If that is our current status, then we can leave it the way it is. If the Docker approach is ready to be used in production though, then this needs to change now.

I'm running several WW "servers" via Docker, as apparently are a few other people (UBC and people who have corresponded with me). As far as I know, minor tuning via docker-compose.yml mounts works well enough, and makes it pretty obvious what was locally changed from a "clean" install.

For the Lite.pm file, ... That is why is this being done at all? If there is an issue that need this change, then what is it?

Needed for UTF-8 support in XMLRPC. Covered elsewhere in GitHub discussions.

Can you provide a reference? I have not found any reference to this in any GitHub discussions.

For the policy.xml file ...

If you can make a PR to my branch to do this, I'll test and merge it.

I will try to put in a pull request that implements a mechanism for changing the policy.xml and Lite.pm files without pulling in the entire files.

Thanks.

There is another issue with how the webwork.apache2.4-config file is added to the docker build. ...

Before my time, and I think most real local modification is best done by creating fixed local config files and mounting them via docker-compose.yml. The exiting code was set up to allow a "localhost" developer using Docker to get up and running with minimal effort, and I think that works well enough.

This is okay. You can mount via docker-compose.yml and override the installed file.

Yes - and that is what I do. The many commented out sample lines in docker-compose.yml are intended to help new users understand some of the capabilities of these features.

@drgrice1
Copy link
Member

A copy command or two is not complicated.

For Lite.pm using a COPY command in Dockerfile makes sense. For UTF-8 support, that is mission critical.

I was referring to copying docker-compose.yml.dist to docker-compose.yml in the instructions for setting up Docker. Not installing Lite.pm. A COPY command in the Dockerfile is better than a docker-compose.yml mount, but doesn't solve the whole file inclusion issue.

Until this is done we are saying that the Docker approach is not ready for production yet. If that is our current status, then we can leave it the way it is. If the Docker approach is ready to be used in production though, then this needs to change now.

I'm running several WW "servers" via Docker, as apparently are a few other people (UBC and people who have corresponded with me). As far as I know, minor tuning via docker-compose.yml mounts works well enough, and makes it pretty obvious what was locally changed from a "clean" install.

You and a few other people may be using it in this way. That doesn't change anything about production readiness. If there is widespread usage of Docker and this change is not made, there are going to be upgrade issues. My statement is still correct and applicable. Again, a simple copy command is not hard to add to the instructions.

For the Lite.pm file, ... That is why is this being done at all? If there is an issue that need this change, then what is it?
Needed for UTF-8 support in XMLRPC. Covered elsewhere in GitHub discussions.
Can you provide a reference? I have not found any reference to this in any GitHub discussions.
* #967 (comment)
* #1107 (comment)
* #1099 (comment)

Thanks.

@taniwallach
Copy link
Member Author

taniwallach commented Mar 21, 2021

@drgrice1 I don't claim to be a huge Docker expert, but I do find it to make running several servers which all run somewhat modified versions of WW to be quite reasonable to handle.

Please read @xcompass's post at #972 (comment) where he explains some of the original decisions for how the WW Docker support was set up, and the discussion thread there.

For more historical perspective please also have a look at

A copy command or two is not complicated.

For Lite.pm using a COPY command in Dockerfile makes sense. For UTF-8 support, that is mission critical.

I was referring to copying docker-compose.yml.dist to docker-compose.yml in the instructions for setting up Docker. Not installing Lite.pm. A COPY command in the Dockerfile is better than a docker-compose.yml mount, but doesn't solve the whole file inclusion issue.

About the file inclusion - carrying a few small "extra" files in the repository vs. the effort in coding a method to recreate the customized files is a matter of cost-benefit. From my perspective, several files totally a few KB of data is "cheaper" than the developer effort needed to avoid proving the ready customized files.

I accept the other two points. I'll try to work on improving the https://github.com/openwebwork/webwork2/wiki/Docker-newbie-instructions and change to .dist files for at least the docker-compose.yml and .env file, and on having the Dockerfile copy the modified Lite.pm file into the image.


Deferred making such changes - see #1242 (comment)


For production use - I relocate the "main" Docker config/control files (and all other local modifications) elsewhere, and set the build: settings and the relevant mount settings appropriately. Although it may not be ideal in terms of "system overhead" - I prefer to keep all my locally modified files outside the image, and obvious in the docker-compose.yml file. That makes it easier to maintain the servers from the sys-admin side of things. This could be related to the fact that I have been running code from various PRs long before they were ready to be merged into master (especially the UTF-8 support code) and have also tested patches, bug fixes, and various local "hacks" using a similar approach. Having only the "locally modified" files in a special tree makes it easy to see what is currently "local code", and to carefully merge those changes into a next-gen set of "local files".

Until this is done we are saying that the Docker approach is not ready for production yet. If that is our current status, then we can leave it the way it is. If the Docker approach is ready to be used in production though, then this needs to change now.

I'm running several WW "servers" via Docker, as apparently are a few other people (UBC and people who have corresponded with me). As far as I know, minor tuning via docker-compose.yml mounts works well enough, and makes it pretty obvious what was locally changed from a "clean" install.

You and a few other people may be using it in this way. That doesn't change anything about production readiness. If there is widespread usage of Docker and this change is not made, there are going to be upgrade issues. My statement is still correct and applicable. Again, a simple copy command is not hard to add to the instructions.

Accept this point and will implement some of the proposed changes soon.


Deferred making such changes - see #1242 (comment)


@drgrice1
Copy link
Member

I also use a Docker files that are not in the webwork2 location when testing things. I modify the docker file to not pull anything from git, and to use the clones already available on my system among other things.

I think that we can defer the docker-compose.yml to docker-compose.yml.dist change for now. At this point there isn't widespread production usage of webwork2 via docker. Lets focus on other things for now.

I did some more research on the imagemagick policy issue. It seems that the reason for the policy was due to a bug in ghostscript, and that bug has been fixed as of version 9.24 (Ubuntu 20.04 has version 9.50 and Ubuntu 18.04 has version 9.26). So there is no real security issue with always enabling that pdf read policy.
See https://www.kb.cert.org/vuls/id/332928/ and
https://stackoverflow.com/questions/52998331/imagemagick-security-policy-pdf-blocking-conversion

@taniwallach
Copy link
Member Author

I think that we can defer the docker-compose.yml to docker-compose.yml.dist change for now. At this point there isn't widespread production usage of webwork2 via docker. Lets focus on other things for now.

OK - thanks.

I did some more research on the imagemagick policy issue. It seems that the reason for the policy was due to a bug in ghostscript, and that bug has been fixed as of version 9.24 (Ubuntu 20.04 has version 9.50 and Ubuntu 18.04 has version 9.26). So there is no real security issue with always enabling that pdf read policy.
See https://www.kb.cert.org/vuls/id/332928/ and
https://stackoverflow.com/questions/52998331/imagemagick-security-policy-pdf-blocking-conversion

I looked there, also at https://www.kb.cert.org/vuls/search/?q=Ghostscript which is not showing newer issues with Ghostscript. I agree with your assessment that at present it seems safe to make the change to /etc/ImageMagick-6/policy.xml automatically.

drgrice1 and others added 2 commits March 22, 2021 17:07
Dockerfile instead.

Also a bit of tweaking of the Dockerfile.  Set the args
`WEBWORK2_GIT_URL`, `WEBWORK2_BRANCH`, `PG_GIT_URL`, and `PG_BRANCH` in
docker-compose.yml instead of in the Dockerfile.  Also, don't convert
those to environment variables.  That isn't needed.  This requires that
`docker-compose build` is used rather than `docker build .`.
Change Lite.pm and ImageMagick-6-policy.xml via patches in Dockerfile (and another small change)
@drgrice1
Copy link
Member

drgrice1 commented Mar 23, 2021

I noticed that the Dockerfile is still installing CGI::Cookie via cpanm. It should now install the Ubuntu package libcgi-pm-perl.

Edit: I see that the libcgi-pm-perl package is installed. So just delete the installation of GCI::Cookie via cpanm. And also the comment about it and Ubuntu 18.04.

… so there is no need to install via CPAN. Also drop an old comment.
@taniwallach
Copy link
Member Author

I noticed that the Dockerfile is still installing CGI::Cookie via cpanm. It should now install the Ubuntu package libcgi-pm-perl.

Edit: I see that the libcgi-pm-perl package is installed. So just delete the installation of GCI::Cookie via cpanm. And also the comment about it and Ubuntu 18.04.

Just done. @drgrice1 - Thanks for noticing this omission.

@drgrice1
Copy link
Member

Also, it seems that the docker-entrypoint.sh handling of site.conf needs to be updated.

Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

This looks good. This definitely needs to be merged before the release. The default docker build is broken without it.

@drgrice1
Copy link
Member

I am going to go ahead and merge this.

@drgrice1 drgrice1 merged commit 8665a40 into openwebwork:develop Mar 26, 2021
@taniwallach taniwallach deleted the docker-to-ubuntu-2004 branch March 29, 2021 14:17
@taniwallach taniwallach removed the Do Not Merge Yet PR to allow others to inspect -- not ready for prime time label Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants