Skip to content

EDG-912: honour HIVEMQ_CONFIG_SECONDARY, gate base-dn, allow several LDAP servers - #1721

Open
marregui wants to merge 1 commit into
masterfrom
fix/config-secondary-folder-ignored
Open

EDG-912: honour HIVEMQ_CONFIG_SECONDARY, gate base-dn, allow several LDAP servers#1721
marregui wants to merge 1 commit into
masterfrom
fix/config-secondary-folder-ignored

Conversation

@marregui

Copy link
Copy Markdown
Contributor

Found during a local QA run of the Helm chart against current Edge master. Details and evidence in EDG-912.

Not required for 2026.13. All three are pre-existing — identical code on 2026.12 — and none affects a non-k8s deployment.

1. HIVEMQ_CONFIG_SECONDARY was ignored

SystemInformationImpl.setFolders():

secondaryConfigFolder = Objects.requireNonNullElseGet(
        configFolder,                       // never null — assigned three statements earlier
        () -> setUpHiveMQFolder(CONFIG_FOLDER_SECONDARY, ...));   // so this never runs

The else-branch was a no-op: the secondary folder always equalled the primary one. Consequence in k8s — Edge read topic filters from conf-k8s instead of the chart's conf-secondary ConfigMap, found none, and wrote an empty topic-filters.xml into the primary folder. Anything declared in topicFilters: was invisible.

Now calls setUpHiveMQFolder directly.

Blast radius is small and I checked it deliberately: getSecondaryHiveMQHomeFolder() has exactly one production consumer, TopicFilterPersistenceReaderWriter. Deployments that do not set the variable are bit-for-bit unaffected. The persistence file now lands on a read-only ConfigMap mount, so I went looking for a write path that would start failing — there isn't one, all four write endpoints in TopicFilterResourceImpl are guarded by isConfigWriteable() and the chart hardcodes HIVEMQ_CONFIG_WRITEABLE=false.

2. <base-dn> crash-looped when unset

docker/config-k8s.xml referenced ${ENV:HIVEMQ_LDAP_BASE_DN} unconditionally while the chart only sets it when configured, so EnvVarUtil aborted the boot. base-dn is genuinely optional — minOccurs="0", and its absence selects the documented legacy mode where rdns is treated as an absolute DN.

Wrapped in ${IF:HIVEMQ_LDAP_BASE_DN_ENABLED} and registered in IfUtil.SUPPORTED_ENVS, the same idiom already used for required-object-class.

Rejected: having the chart emit an empty element. base-dn is typed nonEmptyString, so that trades a boot abort for a schema violation. Also rejected: making baseDn mandatory in the chart, which removes a supported Edge mode.

3. More than one <ldap-server> was schema-invalid

maxOccurs defaulted to 1, so a second entry failed with cvc-complex-type.2.4.d. Edge's own model is a List<LdapServerEntity> and LdapClient builds a RoundRobinServerSet over it, so the schema was the defect, not the model. Now maxOccurs="unbounded".

Merge order

Companion chart PR: hivemq/helm-charts#1039

  • Do not merge the base-dn gate ahead of the chart PR. Today the chart sets HIVEMQ_LDAP_BASE_DN and the template renders the element unconditionally. Add the gate on this side alone and every user who has configured a base DN silently loses it and falls back to legacy resolution. Either both, or neither.
  • The config.xsd change is safe to merge first and de-risks the chart PR — nobody can currently have a second <ldap-server>, so it cannot regress an existing config.
  • The SystemInformationImpl fix is fully independent.

Tests

ConfigK8sTemplateTest is new: it drives the real docker/config-k8s.xml through the actual render-and-validate pipeline with the environment the chart sets. This is the class of bug behind all three findings — a missing ${IF:} gate deletes an element rather than failing — so it now has coverage.

Each new test was confirmed to fail against the unfixed code:

Test Fails without
SystemInformationImplTest — 3 of the 4 new cases fix 1
ConfigK8sTemplateTest.ldapWithoutABaseDn_isStillAValidConfiguration fix 2
ApiConfiguratorTest.ldapWithSeveralServers_isAccepted, ConfigK8sTemplateTest.ldapWithThreeServersAndATruststore_survivesTheTemplate fix 3

Full local run of what check-backend.yml runs — :test :hivemq-edge:forbiddenApis :hivemqEdgeZip6389 tests, 0 failures, 3 skipped. Spotless clean via the composite. Verified end to end in kind against an image built from this branch.

@github-actions

Copy link
Copy Markdown

Test Results

  667 files  + 1    667 suites  +1   8m 58s ⏱️ +15s
6 360 tests +11  6 357 ✅ +11  3 💤 ±0  0 ❌ ±0 
6 389 runs  +11  6 386 ✅ +11  3 💤 ±0  0 ❌ ±0 

Results for commit d7b8a17. ± Comparison against base commit 4c526da.

@github-actions

Copy link
Copy Markdown

Coverage Report

Overall Project 70.84%
Files changed 100%

File Coverage
IfUtil.java 94.13%
SystemInformationImpl.java 94.04%

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