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

[fix] jetty configuration changes don't stick, and eventually crash eXist-db #4899

Conversation

alanpaxton
Copy link
Collaborator

@alanpaxton alanpaxton commented May 2, 2023

Description:

Updated jetty-http.xml and jetty-ssl.xml were being written by the configurator with a bad DTD, which on subsequent read by the configurator caused an exception, so that the NEXT versions of jetty-http.xml and jetty-ssl.xml were made empty. This then caused a startup crash.

  1. Fix the DTD reference.

Also, configurator was reading the wrong field(s) for the http configuration, so

  1. make configurator read the correct fields for jetty http configuration, so that subsequent edits of an already re-configured jetty don’t end up re-introducing the default (8080)

Reference:

Closes #4729

Type of tests:

Unfortunately it is not simple to automate this testing. We have built a test distribution with the changes, installed it (java -jar ./exist-installer/target/exist-installer-7.0.0-SNAPSHOT.jar) and confirmed through the use of ./bin/launcher.sh in the new installation that modifications of the jetty configuration are stored, restored and used on startup as the jetty ports. A distribution built without the change allowed us to reproduce the failed configuration changes, crashes and empty files produced in the initial problem report.

Updated jetty-http.xml and jetty-ssl.xml were being written by the configurator with a bad DTD, which on subsequent read by the configurator caused an exception, so that the NEXT versions of jetty-http.xml and jetty-ssl.xml were made empty.

1. Fix the DTD reference.

Also, configurator was reading the wrong field(s) for the http configuration, so

2. make configurator read the correct fields for jetty http configuration, so that subsequent edits of an already re-configured jetty don’t end up re-introducing the default (8080)
@sonarcloud
Copy link

sonarcloud bot commented May 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@adamretter adamretter self-requested a review May 2, 2023 19:32
@adamretter adamretter added the bug issue confirmed as bug label May 2, 2023
Copy link
Member

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @alanpaxton

@adamretter adamretter added this to the eXist-7.0.0 milestone May 2, 2023
@duncdrum
Copy link
Contributor

duncdrum commented May 3, 2023

@alanpaxton all the steps you describe should be easy to test with the docker workflow on the CI here. Following the template for modifying conf.xml. In essence, modify file, start modified container, assert that change is applied, end container.

Please add a test

@adamretter
Copy link
Member

@duncdrum How would you see using the Launcher Configuration GUI working from inside a Docker container in CI exactly?

@duncdrum
Copy link
Contributor

duncdrum commented May 3, 2023

@adamretter

Also, configurator was reading the wrong field(s) for the http configuration, so

make configurator read the correct fields for jetty http configuration, so that subsequent edits of an already re-configured jetty don’t end up re-introducing the default (8080)

does this only refer to the configurator of the initial launch agent, or to the configurations that is read whenever you start exist?

@alanpaxton
Copy link
Collaborator Author

alanpaxton commented May 3, 2023

The configurator that is started on initial, and whenever you run "System Configuration" from the (GUI) Menu Bar, is the same configurator code with the same problem.

@reinhapa reinhapa merged commit 9f02d3a into eXist-db:develop May 3, 2023
8 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug issue confirmed as bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] System configuration leads to empty jetty-http.xml and jetty-ssl.xml
5 participants