FMQ config update for v1.0 - #576
Open
dborovcanin wants to merge 12 commits into
Open
Conversation
Configuration decoding used plain yaml.Unmarshal, so an unknown or misspelled key was silently discarded, and a --config path that did not exist fell back to built-in defaults. A typo could start a broker with none of the operator's settings, auth and TLS included, while reporting healthy. Decoding now uses yaml.Decoder with KnownFields(true), and a named config file that does not exist is an error. The previous fallback moves to LoadOptional, reachable with the new --config-optional flag. Reload uses Load, so a config file that goes missing under a running broker fails the reload and retains the live configuration. Validate rejects two listeners bound to the same address, keeping TCP and UDP separate since CoAP may reuse a TCP port number. Strict decoding surfaced four example files still declaring tcp.plain and websocket.plain, slots renamed to v3/v5 earlier. The blocks were discarded whole, so each silently opened two plaintext TCP and two plaintext WebSocket listeners where it declared one of each. The bind check then caught examples/production.yaml binding the WebSocket v5 default to its declared WSS port, in a file stating three times that it serves no plaintext. server.tcp and server.websocket said nothing about MQTT while sitting beside server.amqp and server.amqp091. Both move under server.mqtt, with TCPConfig renamed to MQTTTCPConfig, WebSocketConfig to MQTTWebSocketConfig, and a new MQTTConfig grouping the two transports. BREAKING CHANGE: server.tcp and server.websocket become server.mqtt.tcp and server.mqtt.websocket. Existing configurations now fail at startup naming the offending key instead of silently losing listener settings. Signed-off-by: dusan <borovcanindusan1@gmail.com>
A queue's delivery address is not injective. queueDeliveryTopic absorbs the leading level when it already equals the queue name, so a capture of m/acme/temp into queue m, a capture of acme/temp into queue m, and an explicit publish to $queue/m/acme/temp all arrive as $queue/m/acme/temp. $queue/q/y is likewise both a capture of y and a capture of q/y. A consumer could not recover where a message came from, and the address format freezes at v1. Stamp the origin into a broker-owned x-source-topic property instead of changing the address. The address keeps its current meaning, so nothing a consumer subscribes to today moves; what changes is that the origin is now recoverable. The property is written after the publisher's own properties are copied in, so a publisher cannot forge it, and it is reserved, so it is never mistaken for a user property. The remote path rebuilds its own envelope from an address that has already been converted and cannot be reversed, so QueueMessage carries the source topic as its own field. It travels in the existing property map on the wire and is lifted back out on decode, so no protocol definition changes. The collisions are pinned in tests as the contract they now are, rather than left to be rediscovered. Signed-off-by: dusan <borovcanindusan1@gmail.com> (cherry picked from commit f3655c1)
ValidateLocalPrincipals opened every secret file to check its contents, so validating a production configuration required that machine to hold the production secrets. That is why config-local-principal.yaml, the one shipped file exercising two AMQP 0.9.1 listeners and a local-principal listener, was the only configuration CI could not gate, and why `fluxmq config validate` could not check a real file on a workstation. The checks were redundant. broker/localauth loadFingerprint already reads the file, strips the terminal newline, and rejects embedded CR/LF, NUL bytes, and secrets under 32 bytes — the same rules, enforced where the credential material is actually loaded. That runs at startup and on every reload, so nothing is now unchecked: a broker with a missing or weak secret still refuses to start. Configuration validation keeps what is genuinely declarative, including that a secret file is named and not blank. The shipped local-principal configuration joins make validate-configs, so all six are gated. A secret-content failure now surfaces from the local-principal reload callback rather than from config.Load, so the reload manager's failure observer no longer fires for it — production already records that case inside the callback, and firing both would double-count. The observer keeps its own coverage for the case it exists for: a configuration that cannot be loaded at all, where the callback is never reached. Signed-off-by: dusan <borovcanindusan1@gmail.com> (cherry picked from commit 139020f)
Addresses were checked for being non-empty and unique and nothing else, so every malformed form was accepted: a port above 65535, a negative port, a non-numeric port, a bare "1883" with no colon, and plain prose. The first sign of a typo was a broker that logged a bind failure and exited. Decide what the string alone can decide. The address must split into host and port, the port must be a number in 1 to 65535, and the host, if given, must not contain whitespace. The host is deliberately not resolved: validation has to work on a machine that cannot see the deployment's DNS, so ":1883", "127.0.0.1:1883", "[::1]:1883" and "broker.internal:1883" all pass. Port 0 is refused as well. It asks the kernel for an arbitrary free port, and a broker nobody can be told the address of is not a deployment, so the message says to choose a fixed port rather than reporting a range error. The check runs over the same listener set the duplicate-bind check already walks, so it covers the messaging listeners, admin, health, and the CoAP bridge. Every failure names the key it came from. Ported from the v1 configuration work, which carried it against a listener schema this branch does not adopt. Signed-off-by: dusan <borovcanindusan1@gmail.com>
storage.sync_writes reads as "fsync all storage", but it only reaches the broker key-value store that holds retained messages and sessions. The queue append-only log is a separate engine whose acknowledgement policy is not configurable at all: ordinary appends are buffered and only one internal path calls AppendAndSync. An operator setting sync_writes: true on a durable-queue deployment would reasonably think they had bought queue durability, and they had not. Rename it to storage.badger_sync_writes so the key names its engine. Renaming is free now and breaking after the 1.0 freeze, and it stops the ambiguity growing when a queue-side durability key lands beside it. Strict decoding means an old file fails with "storage.sync_writes: unknown field" rather than silently losing the setting. The doc comments on the runtime type and the reference documentation now state which engine the key reaches, and that queue durability is not it. The schema test pins the new name and asserts the old one is gone. Also adds v1.md, the stable-core hardening plan written alongside the v1 configuration work, so the release scope is tracked in the repository rather than in a working tree. Ported from the v1 configuration work. Signed-off-by: dusan <borovcanindusan1@gmail.com>
Listeners were changed so that an omitted key takes the default and a written 0 means unlimited or no deadline. Everything else kept the older convention where 0 silently selected the default, so the same literal meant opposite things depending on the section: max_connections: 0 gave an uncapped listener, capture_workers: 0 gave four workers. The rule is now one rule everywhere: a value the operator writes is never replaced by a different one. Where zero is a coherent setting it is honoured, as it is for listener limits and timeouts. Where it is not, it is refused with a message that says to omit the key instead. capture_workers, capture_queue_depth, capture_drain_timeout, identity_cache_size, identity_cache_ttl, and hooks.timeout have no coherent zero — no capture lanes, a cache holding nothing, or a blocking hook with no deadline are not settings anyone wants — so they become optional and reject a written zero. Omitting them still takes the built-in default, which is unchanged, so no existing behaviour moves. The reference documentation now lists the real default for each of them rather than 0, and says a written zero is rejected. Ported from the v1 configuration work. Signed-off-by: dusan <borovcanindusan1@gmail.com>
Strict decoding turns a stale documentation example into a broker that refuses to start, so the examples are now part of the contract rather than prose beside it. Five yaml blocks no longer loaded: - configuration/server.md declared server.mqtt.websocket.plain. The page documenting the listener rename kept the slot the rename removed. - messaging/consumer-groups.md documented five queue_manager keys that have never existed. Only auto_commit_interval is configurable; the rest are queue/consumer.DefaultConfig values with no YAML surface, so the section now says which are fixed and at what value. - configuration/security.md granted an exact publish target without cluster.enabled: false, which the same page's prose calls a startup error. - deployment/internal-amqp-local-principals.md shows the removed flat auth form deliberately, and reference/configuration-reference.md quotes one section in isolation. Both are marked as blocks that must not load. TestDocumentedConfigsLoad closes the gap that let these through: TestShippedConfigsDecodeStrictly covers examples/ and deployments/ but never looked at documentation. It guards 27 blocks across docs/ and README.md, treats a block as broker configuration when any top-level key is a config key so manifests and payload samples need no annotation, and takes an explicit <!-- fluxmq:config-skip: reason --> marker for the blocks that must fail. Signed-off-by: dusan <borovcanindusan1@gmail.com>
The repository carried two independently written v1 plans. v1.md estimated 54-81 engineer-days; the working-tree roadmap estimated 31-44. The gap was scope rather than disagreement, and the union of the two is larger than either: 67-100 days to the tag. ROADMAP.md is now the single plan and V1-READINESS.md the evidence behind it, both tracked so they cannot follow plan.md and the three design documents that were deleted before them. Every finding imported from v1.md was re-verified against this branch first. Confirmed and scheduled: the admin API and Connect queue service have no authentication (P0-9), DLQ movement and replication both fail open (P0-10), etcd peer traffic is cleartext (P0-11), the AMQP 1.0 handshake is unbounded, and images are published before they are scanned. One was stale and rewritten: readiness already checks the broker, storage, and peer reachability, so the work is extending it rather than adding it. Two were already delivered on this branch: the queue delivery address is settled by types.PropSourceTopic, and topic matching is a trie. The config branch stays parked rather than merged; its listener model is superseded by server.mqtt, and the three commits still worth salvaging are named in the progress log. Signed-off-by: dusan <borovcanindusan1@gmail.com>
Four defects and two stale documents, from review of this branch. Trailing YAML documents were silently dropped. The decoder ran once and never checked for a second document, so a `---` above an auth or TLS section started the broker on defaults while the file plainly showed otherwise — the same failure strict decoding exists to end. Decoding now continues past the first document and rejects anything that is not EOF, naming the line. Moved listener sections now report where they went. server.tcp and server.websocket were removed in favour of server.mqtt.*, and the decoder alone said only "field tcp not found", which tells an operator that something broke but not what to write. The load path names the replacement, reusing the mechanism that already rejects the flat auth keys. This makes the cutover legible; it does not make it survivable, and the deployments consuming the published image still have to be sequenced. Duplicate-bind validation now matches the listeners startup opens. It counted server.health_addr even when health_enabled was false, which refused a legitimate port reuse, and omitted the amqp091 internal and service aliases, which let a genuine collision through to fail at bind time instead. Documentation: the CLI reference still described the missing-file fallback this branch removed and never mentioned --config-optional, and both hook pages told operators that an explicit `timeout: 0` selects the 500ms default when it is now rejected at load. Copying either one prevented startup. The queue delivery contract is qualified rather than overstated. The source topic travels in a broker-stamped property, and MQTT 3.1.1 cannot encode properties, so a 3.1.1 consumer of a captured message receives the queue identity and nothing about the origin. Making the address injective does not fix it: a capture of acme/temp into queue m renders exactly as an explicit publish to $queue/m/acme/temp, so separating them would need a marker level imposed on every protocol to serve the one that cannot read properties. Recorded as an open decision. Signed-off-by: dusan <borovcanindusan1@gmail.com>
A 3.1.1 consumer could subscribe to a classic queue, receive messages, and never settle them, because settlement needs identifiers the protocol cannot carry. The broker now settles those deliveries on PUBACK, which resolves the open contract question this document raised: origin recovery for a captured message still needs MQTT 5.0 or AMQP, and the support matrix says so rather than claiming both MQTT versions are equivalent. The implementation is on fmq-mqtt-queue-ack, which targets main rather than stacking on this branch: it depends on nothing here, and this branch cannot merge until magistrala is pinned off the latest image tag. Opens one item: a cross-node takeover rebuilds inflight messages without their properties, so a settled delivery redelivers after a node move. Closing that needs an additive proto/cluster/v1 field, which is cheaper before the tag than after. Signed-off-by: dusan <borovcanindusan1@gmail.com>
dborovcanin
force-pushed
the
fmq-config-strict
branch
from
August 21, 2026 12:27
242d4bd to
de5df97
Compare
Two validations refused files and deployments that are fine, both found by running the reviewer's own reproduction probes against the branch. A trailing document separator is not a dropped document. The strict decoder rejected anything after the first `---`, which caught the case it was written for — a second document carrying auth or TLS settings that would be silently ignored — but also caught a file that merely ends in a separator, or one followed by comments. yaml.v3 reports those as a null document rather than as the end of the stream. An empty trailing document now passes, and a file may end in as many separators as its templating emits. Duplicate-bind detection treated any wildcard as colliding with any host on the same port, so 0.0.0.0:1883 and [::1]:1883 were refused although both bind: an IPv4 wildcard does not accept IPv6 connections. Wildcards now collide only with the families they accept, with "::" treated as dual-stack because that is the Linux default and the case that really collides. The error also names both addresses; it previously reported one side twice, so a conflict involving a wildcard printed the address the operator had not written. Signed-off-by: dusan <borovcanindusan1@gmail.com>
The MQTT 3.1.1 queue settlement work landed on main as absmach#580 while this branch waits on the magistrala pin, so the entry describing it as pending, and the next-session list still naming 1.2 as unstarted, no longer matched the tree. Signed-off-by: dusan <borovcanindusan1@gmail.com>
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
1 participant
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.
What type of PR is this?
What does this do?
Which issue(s) does this PR fix/relate to?
Have you included tests for your changes?
Did you document any new/modified features?
Notes