Skip to content

Add additional docker env variables for commonly changed settings#26757

Closed
yolabingo wants to merge 4 commits into
masterfrom
issue-26751-provide-docker-web.xml-template
Closed

Add additional docker env variables for commonly changed settings#26757
yolabingo wants to merge 4 commits into
masterfrom
issue-26751-provide-docker-web.xml-template

Conversation

@yolabingo
Copy link
Copy Markdown
Member

@yolabingo yolabingo commented Nov 20, 2023

Proposed Changes

  1. Adds a web.xml file to docker templates, which is copied by docker/dotcms/ROOT/srv/20-copy-overriden-files.sh

cp /srv/OVERRIDE/WEB-INF/web.xml $TOMCAT_HOME/webapps/ROOT/WEB-INF/web.xml
We currently have template files for server.xml and context.xml.

  1. Adds the following env variables to docker/dotcms/ROOT/srv/00-config-defaults.sh
${CMS_ANTI_CLICK_JACKING_ENABLED:-"true"}
${CMS_ANTI_CLICK_JACKING_OPTION:-"SAMEORIGIN"}
${CMS_HSTS_ENABLED:-"true"}
${CMS_HSTS_MAX_AGE_SECONDS:-"31536000"}
${CMS_HSTS_INCLUDE_SUB_DOMAINS:-"true"}
${CMS_HSTS_PRELOAD:-"false"}
${CMS_SESSION_TIMEOUT:-"30"}
${CMS_MAX_HTTP_HEADER_SIZE:-"16384"} # added this env var to server.xml - not used by web.xml

Note - CMS_HSTS_MAX_AGE_SECONDS and CMS_MAX_HTTP_HEADER_SIZE increase the current default settings.
Do we want to increase default CMS_SESSION_TIMEOUT from 30 minutes?

  1. Increases default maxHttpHeaderSize in server.xml template and exposes it as an env variable
<Connector
   ...
   maxHttpHeaderSize="${CMS_MAX_HTTP_HEADER_SIZE}"

Related Issues

#26751 provide web.xml template in docker image

#20515 Increase Tomcat maxHttpHeaderSize above 8k default

${CMS_MAX_HTTP_HEADER_SIZE:-"16384"}
${CMS_ANTI_CLICK_JACKING_ENABLED:-"true"}
${CMS_ANTI_CLICK_JACKING_OPTION:-"SAMEORIGIN"}
${CMS_HSTS_ENABLED:-"true"}
${CMS_HSTS_MAX_AGE_SECONDS:-"31536000"}
${CMS_HSTS_INCLUDE_SUB_DOMAINS:-"true"}

export CMS_SESSION_TIMEOUT=${CMS_SESSION_TIMEOUT:-"30"}

Adds web.xml docker template file

override webapps/ROOT/WEB-INF/web.xml

Adds additional docker env vars for tomcat config
Copy link
Copy Markdown
Contributor

@spbolton spbolton left a comment

Choose a reason for hiding this comment

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

docker/dotcms is no longer used in maven and we should be removing this. The equivalent is in dotCMS/src/main/docker/original

We currently also have dotCMS/src/main/docker/original/ROOT/srv/OVERRIDE/tomcat/conf that is copied in as the original Dockerfile scripts from dotCMS/src/main/docker/original/ROOT/srv/20-copy-overriden-files.sh

The use of these files in the OVERRIDE folder are deprecated and we should end up removing these. The base tomcat files already gets overriden from the files in dotCMS/src/main/container/tomcat9x/conf. Changing the old script may impact how engineering overrides these files at startup so for the time being server.xml and context.xml in dotCMS/src/main/docker/original/ROOT/srv/OVERRIDE/tomcat/conf and dotCMS/src/main/container/tomcat9x/conf should be kept in sync. The former only ends up running when using docker rather than local tomcat and the latter will only run when not using docker. We want these to end up being consistent.

@@ -0,0 +1,617 @@
<?xml version="1.0"?>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we discussed adding some changes to web.xml for a temporary basis, but already I cannot work out easily what is different in here to the base file in dotCMS/src/main/webapp/WEB-INF/web.xml. We are going to setting ourselves up for more confusion and pain with differences we may miss.

Is it just the addition of the parametized values like ${CMS_HSTS_ENABLED}. If this works can we not change the base web.xml.

Note in the new base maven server.xml dotCMS/src/main/docker/original/ROOT/srv/OVERRIDE/tomcat/conf/server.xml. we have integrated the configurable parameters that were in the override files and then separately defaulted in 00-config-defaults.sh. We do not need to separate these out as we can set the default value in place e.g. I am not sure if we can use the same thing in the web.xml also. It would be good to keep the configurable logic for web.xml in just the one file

Copy link
Copy Markdown
Member Author

@yolabingo yolabingo Jan 9, 2024

Choose a reason for hiding this comment

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

Is it just the addition of the parameterized values like ${CMS_HSTS_ENABLED}. If this works can we not change the base web.xml.

Yes, the only changes to web.xml are these parameterized values. We also set the defaults for these params in 00-config-defaults.sh.

If we can refactor this PR to provide the same functionality by modifying dotCMS/src/main/webapp/WEB-INF/web.xml instead of using having to duplicate that file as we do with server.xml, all the better.

I realize this "shell template" mechanism is deprecated, but this is an urgent need. The current situation is becoming untenable. It puts a considerable burden on the engineering team to not be able to set the commonly-changed settings in web.xml with environment variables. Rather than having to manage 2 versions of web.xml in a single repo, we currently have to manage dozens of custom web.xml files.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will test to see if web.xml will fetch parameters from environment variables.

@github-actions
Copy link
Copy Markdown
Contributor

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions Bot added the stale label Feb 11, 2024
@github-actions
Copy link
Copy Markdown
Contributor

This PR was closed because it has been stalled with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants