NMS-19640: Sentinel: Investigate if its possible to move away from Confd#8391
Draft
mershad-manesh wants to merge 2 commits intodevelopfrom
Draft
NMS-19640: Sentinel: Investigate if its possible to move away from Confd#8391mershad-manesh wants to merge 2 commits intodevelopfrom
mershad-manesh wants to merge 2 commits intodevelopfrom
Conversation
mershad-manesh
requested review from
cgorantla,
christianpape,
Copilot,
indigo423 and
synqotik
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the Sentinel container’s dependency on confd-based template rendering and replaces it with a simpler environment-variable-driven configuration approach baked into the container filesystem and entrypoint logic.
Changes:
- Add baseline Karaf
.cfgfiles that support environment-variable interpolation for Kafka IPC and Elastic flows persistence. - Update
entrypoint.shto configure IPC/features and selected config files based on environment variables, and remove confd execution. - Remove confd templates/scripts/configs from the Sentinel container build and stop copying them in the Dockerfile.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| opennms-container/sentinel/container-fs/etc/org.opennms.features.flows.persistence.elastic.cfg | Add default Elastic URL using env interpolation. |
| opennms-container/sentinel/container-fs/etc/org.opennms.core.ipc.sink.kafka.consumer.cfg | Add default Kafka bootstrap servers via env interpolation. |
| opennms-container/sentinel/container-fs/etc/org.opennms.core.ipc.sink.kafka.cfg | Add default Kafka bootstrap servers via env interpolation. |
| opennms-container/sentinel/container-fs/etc/featuresBoot.d/templates/sentinel-ipc.boot | Add a featuresBoot template to enable Kafka IPC. |
| opennms-container/sentinel/container-fs/entrypoint.sh | Replace confd-driven config with env parsing + feature boot templating; add ERR trap. |
| opennms-container/sentinel/container-fs/confd/templates/prom-jmx-exporter.yaml.tmpl | Remove confd template. |
| opennms-container/sentinel/container-fs/confd/templates/org.opennms.sentinel.controller.cfg.tmpl | Remove confd template. |
| opennms-container/sentinel/container-fs/confd/templates/org.opennms.netmgt.distributed.datasource.cfg.tmpl | Remove confd template. |
| opennms-container/sentinel/container-fs/confd/templates/org.opennms.features.flows.persistence.elastic.cfg.tmpl | Remove confd template. |
| opennms-container/sentinel/container-fs/confd/templates/org.opennms.core.ipc.sink.kafka.consumer.cfg.tmpl | Remove confd template. |
| opennms-container/sentinel/container-fs/confd/templates/org.opennms.core.ipc.sink.kafka.cfg.tmpl | Remove confd template. |
| opennms-container/sentinel/container-fs/confd/templates/ipc-strategy.boot.tmpl | Remove confd template. |
| opennms-container/sentinel/container-fs/confd/templates/custom.system.properties.tmpl | Remove confd template. |
| opennms-container/sentinel/container-fs/confd/templates/confd-flows-feature.xml.tmpl | Remove confd template. |
| opennms-container/sentinel/container-fs/confd/scripts/remove-if-empty | Remove confd helper script. |
| opennms-container/sentinel/container-fs/confd/scripts/confd_lib.sh | Remove confd helper library. |
| opennms-container/sentinel/container-fs/confd/confd.toml | Remove confd configuration. |
| opennms-container/sentinel/container-fs/confd/conf.d/prom-jmx-exporter.toml | Remove confd template mapping. |
| opennms-container/sentinel/container-fs/confd/conf.d/org.opennms.sentinel.controller.cfg.toml | Remove confd template mapping. |
| opennms-container/sentinel/container-fs/confd/conf.d/org.opennms.netmgt.distributed.datasource.cfg.toml | Remove confd template mapping. |
| opennms-container/sentinel/container-fs/confd/conf.d/org.opennms.features.flows.persistence.elastic.cfg.toml | Remove confd template mapping. |
| opennms-container/sentinel/container-fs/confd/conf.d/org.opennms.core.ipc.sink.kafka.consumer.cfg.toml | Remove confd template mapping. |
| opennms-container/sentinel/container-fs/confd/conf.d/org.opennms.core.ipc.sink.kafka.cfg.toml | Remove confd template mapping. |
| opennms-container/sentinel/container-fs/confd/conf.d/ipc-strategy.boot.toml | Remove confd template mapping. |
| opennms-container/sentinel/container-fs/confd/conf.d/custom.system.properties.toml | Remove confd template mapping. |
| opennms-container/sentinel/container-fs/confd/conf.d/confd-flows-feature.xml.toml | Remove confd template mapping. |
| opennms-container/sentinel/Dockerfile | Stop copying confd assets; copy container-specific /etc overlay instead. |
Comments suppressed due to low confidence (1)
opennms-container/sentinel/container-fs/entrypoint.sh:215
parseEnvironmentandapplyFeatureBootTemplatesonly run inside the "not yet configured" branch. After the first boot (whenetc/configuredexists), changes toKAFKA_IPC_*,ELASTICSEARCH_*,OPENNMS_INSTANCE_ID, orSENTINEL_IPCwill be ignored, and a previously-selected IPC strategy can’t be changed without deleting the marker file. If runtime configurability via environment variables is the goal, these steps should run on every container start (or at least beforestart()).
if [ ! -f ${SENTINEL_HOME}/etc/configured ]; then
# Create SSH Key-Pair to use with the Karaf Shell
mkdir -p "${SENTINEL_HOME}/.ssh" && \
chmod 700 "${SENTINEL_HOME}/.ssh" && \
ssh-keygen -t rsa -f "${SENTINEL_HOME}/.ssh/id_rsa" -q -N "" && \
echo "sentinel=$(cat "${SENTINEL_HOME}/.ssh/id_rsa.pub" | awk '{print $2}'),viewer" > "${SENTINEL_HOME}/etc/keys.properties" && \
echo "_g_\\:admingroup = group,admin,manager,viewer,systembundles,ssh" >> "${SENTINEL_HOME}/etc/keys.properties" && \
chmod 600 "${SENTINEL_HOME}/.ssh/id_rsa"
# Expose Karaf Shell
sed -i "/^sshHost/s/=.*/= 0.0.0.0/" ${SENTINEL_HOME}/etc/org.apache.karaf.shell.cfg
# Expose the RMI registry and server
sed -i "/^rmiRegistryHost/s/=.*/= 0.0.0.0/" ${SENTINEL_HOME}/etc/org.apache.karaf.management.cfg
sed -i "/^rmiServerHost/s/=.*/= 0.0.0.0/" ${SENTINEL_HOME}/etc/org.apache.karaf.management.cfg
# Set Sentinel location and connection to OpenNMS instance
SENTINEL_CONFIG=${SENTINEL_HOME}/etc/org.opennms.sentinel.controller.cfg
echo "location = ${SENTINEL_LOCATION}" > ${SENTINEL_CONFIG}
echo "id = ${SENTINEL_ID:=$(uuidgen)}" >> ${SENTINEL_CONFIG}
echo "broker-url = ${OPENNMS_BROKER_URL}" >> ${SENTINEL_CONFIG}
# Configure datasource
DB_CONFIG=${SENTINEL_HOME}/etc/org.opennms.netmgt.distributed.datasource.cfg
echo "datasource.url = jdbc:postgresql://${POSTGRES_HOST}:${POSTGRES_PORT}/${POSTGRES_DB}" > ${DB_CONFIG}
echo "datasource.username = ${POSTGRES_USER}" >> ${DB_CONFIG}
echo "datasource.password = ${POSTGRES_PASSWORD}" >> ${DB_CONFIG}
echo "datasource.databaseName = ${POSTGRES_DB}" >> ${DB_CONFIG}
parseEnvironment
applyFeatureBootTemplates
# Mark as configured
echo "Configured $(date)" > ${SENTINEL_HOME}/etc/configured
else
echo "OpenNMS Sentinel is already configured, skipped."
fi
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| set -eE | ||
|
|
||
| # shellcheck disable=SC2064 | ||
| trap 'rc=$?; echo "[Startup][ERROR] entrypoint failed at line ${LINENO} (exit=${rc})"; exit ${rc}' ERR |
There was a problem hiding this comment.
The ERR trap message uses ${LINENO}, which will report the line number inside the trap handler rather than the line that failed. This makes startup failures hard to diagnose; consider logging ${BASH_LINENO[0]} and/or ${BASH_COMMAND} instead (and keep -E so it works across functions).
Suggested change
| trap 'rc=$?; echo "[Startup][ERROR] entrypoint failed at line ${LINENO} (exit=${rc})"; exit ${rc}' ERR | |
| trap 'rc=$?; echo "[Startup][ERROR] entrypoint failed at line ${BASH_LINENO[0]}: ${BASH_COMMAND} (exit=${rc})"; exit ${rc}' ERR |
Comment on lines
+101
to
+106
| # Omit $value here, in case there is sensitive information | ||
| echo "[Configuring] '$key' in '$file'" | ||
|
|
||
| # If config exists in file, replace it. Otherwise, append to file. | ||
| if grep -E -q "^#?\s*$key\s*=" "$file"; then | ||
| sed -r -i "s@^#?\s*$key\s*=.*@$key=$value@g" "$file" #note that no config values may contain an '@' char |
There was a problem hiding this comment.
updateConfig uses grep -E/sed -r patterns with \s, but \s is not whitespace in POSIX ERE (it matches a literal 's' in many grep/sed implementations). Also, $key is interpolated unescaped into the regex, so keys containing regex metacharacters (e.g. . in bootstrap.servers) can match unintended lines. Use [[:space:]] classes and escape $key for regex (or avoid regex matching altogether).
Suggested change
| # Omit $value here, in case there is sensitive information | |
| echo "[Configuring] '$key' in '$file'" | |
| # If config exists in file, replace it. Otherwise, append to file. | |
| if grep -E -q "^#?\s*$key\s*=" "$file"; then | |
| sed -r -i "s@^#?\s*$key\s*=.*@$key=$value@g" "$file" #note that no config values may contain an '@' char | |
| # Escape key for use in extended regular expressions | |
| key_regex=$(printf '%s\n' "$key" | sed -e 's/[][\.^$*+?(){}|/]/\\&/g') | |
| # Omit $value here, in case there is sensitive information | |
| echo "[Configuring] '$key' in '$file'" | |
| # If config exists in file, replace it. Otherwise, append to file. | |
| if grep -E -q "^#?[[:space:]]*${key_regex}[[:space:]]*=" "$file"; then | |
| sed -r -i "s@^#?[[:space:]]*${key_regex}[[:space:]]*=.*@$key=$value@g" "$file" #note that no config values may contain an '@' char |
Comment on lines
+104
to
+106
| # If config exists in file, replace it. Otherwise, append to file. | ||
| if grep -E -q "^#?\s*$key\s*=" "$file"; then | ||
| sed -r -i "s@^#?\s*$key\s*=.*@$key=$value@g" "$file" #note that no config values may contain an '@' char |
There was a problem hiding this comment.
updateConfig injects $value directly into the sed replacement without escaping. Values containing &, backslashes, or the chosen delimiter (@) will produce incorrect output, and this is likely for URLs/JAAS configs/passwords. Please escape the replacement string (and/or validate/forbid unsupported characters) before calling sed, or switch to a safer file-editing approach that doesn't treat the value as a regex replacement.
Suggested change
| # If config exists in file, replace it. Otherwise, append to file. | |
| if grep -E -q "^#?\s*$key\s*=" "$file"; then | |
| sed -r -i "s@^#?\s*$key\s*=.*@$key=$value@g" "$file" #note that no config values may contain an '@' char | |
| # Prepare a safely escaped value for use in the sed replacement part. | |
| # Escape backslashes, '&' (match reference), and the '@' delimiter. | |
| escaped_value=$value | |
| escaped_value=${escaped_value//\\/\\\\} | |
| escaped_value=${escaped_value//&/\\&} | |
| escaped_value=${escaped_value//@/\\@} | |
| # If config exists in file, replace it. Otherwise, append to file. | |
| if grep -E -q "^#?\s*$key\s*=" "$file"; then | |
| sed -r -i "s@^#?\s*$key\s*=.*@$key=$escaped_value@g" "$file" |
| } | ||
|
|
||
| function parseEnvironment() { | ||
| IFS=$'\n' |
There was a problem hiding this comment.
parseEnvironment sets IFS=$'\n' but never restores it. Since IFS is global, this can break later word-splitting in the script (e.g., the comma/space splitting loop in applyKarafDebugLogging). Make IFS local to the function (or use a while IFS= read -r loop) so the rest of the script keeps the default IFS.
Suggested change
| IFS=$'\n' | |
| local IFS=$'\n' |
Comment on lines
+144
to
+151
| function applyFeatureBootTemplates() { | ||
| local managed_boot_files=( | ||
| "sentinel-ipc.boot" | ||
| ) | ||
| local boot_file | ||
| for boot_file in "${managed_boot_files[@]}"; do | ||
| rm -f "${FEATURES_BOOT_DIR}/${boot_file}" | ||
| done |
There was a problem hiding this comment.
applyFeatureBootTemplates only removes/controls sentinel-ipc.boot. In previous releases, IPC selection was written to featuresBoot.d/ipc-strategy.boot (via confd), which may still exist on upgraded/persisted volumes and could conflict with the new mechanism. Consider also removing the legacy ipc-strategy.boot (and any other previously-managed IPC boot files) to ensure deterministic behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Opening this PR to gather feedback;
All Contributors
External References