mlevkov commented on PR #3804:
URL: https://github.com/apache/iggy/pull/3804#issuecomment-5229123116
Ran a self-review over this branch before handing it back, and it caught
something I had introduced in the previous commit rather than inherited. Pushed
`e1a3a8ae9`.
**The fix documented three exposure paths and warned about one of them.**
That commit added the README block naming three ways the containment goes
away, and the guard only covered the first. So:
- `[http.cors] enabled = true` on the shipped loopback default, no key:
silent. Loopback does not contain it, since a browser is a local process and
the layer wraps outside auth, which is the whole point of the note you left.
That is the case the warning most needed to catch and it was the one it missed.
- `address = "0.0.0.0:8081"`, `api_key` set, `http.tls` disabled: silent,
because the guard returned early on a non-empty key. Having a key says nothing
about whether the key and the credential-bearing responses cross the wire in
clear.
`warn_on_weak_containment` now emits one warning per path. Separate rather
than combined because they compose independently, and an operator who closes
one has not closed the others. The address check drops its key test and becomes
`resolves_beyond_loopback`, which is all it was ever classifying.
That also resolves the remediation wording you flagged, from the other
direction than I first took it. Conditioning TLS advice on `config.tls.enabled`
means it is a separate warning that only fires when TLS is actually off, so it
can no longer tell an operator to enable something they already enabled, and
the awkward "unless ... may cross in cleartext" clause is gone.
**`example_config/config.toml` was still carrying the old comments.** I had
updated the embedded default and the README and missed the file operators
actually copy, which defeated the point of the documentation half. Fixed, and
the two no longer diverge.
**Tests.** Added the CORS path, the key-without-TLS path, and `enabled =
false` (nothing should warn about an API that is not listening; without it,
hoisting the guard above the enabled check would go unnoticed). The loopback
assertion was matching "no WARN containing this address", which goes vacuous
the moment the message is reworded, so it now asserts no warning at all, which
the per-test capture makes safe. 129 passing.
All four mutation-checked: disabling either new warning block fails exactly
its own test.
**Deliberately not changed, in case you would rather I did:**
- The address is resolved twice at startup, once here and once by `bind`.
That is the direct cost of your "do not bind what you resolve" note, and I
would rather pay it than lose the `localhost` to `[::1, 127.0.0.1]` fallback.
Recorded in the doc comment as the trade rather than left implicit.
- Still the `Vec<SocketAddr>` form you suggested rather than a `peekable`
variant.
- The ordering test still detects "warned before binding" via the bind
panic. It now also asserts no `Started` event was captured, so the ordering
claim rests on two observations instead of one.
Separately, while re-checking a claim in the README block I filed #3848:
`restart_connector` asks the provider for `get_{sink,source}_config(key,
None)`, which the local provider resolves to the highest version while the HTTP
provider resolves to the active one, so a rollback via `PUT /configs/active`
survives a process start and is undone by `POST /restart`. Not touched here.
Gate: fmt, sort, taplo, hawkeye, typos, markdownlint, clippy `-D warnings`,
`cargo test -p iggy-connectors` 129 passing, all exit 0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
