mlevkov commented on PR #3804:
URL: https://github.com/apache/iggy/pull/3804#issuecomment-5228212163
All ten addressed in `a6ebf76d1`. The first one changed the shape of the PR,
so taking them in that order.
**The exposure is not read-only.** Correct, and I had the threat model
wrong, not just the wording. I traced it: `PUBLIC_PATHS` is `["/", "/health"]`,
`restart_connector` calls `get_sink_config(key, None)` then
`start_connector(key, &config, ..)`, so write-then-restart is a repoint using
the runtime's own Iggy credentials. The warning and the README now say read
*and* rewrite, name `restart` and `DELETE`, and spell out the config-write plus
activate plus restart chain rather than leaving it as "disclosure".
**CORS.** Confirmed the layer order: `.layer(auth)` then `.layer(cors)` puts
CORS outermost, and shipped `allowed_origins = ["*"]` maps to
`AllowOrigin::any()`. Added as one of three named ways the loopback containment
goes away, with the point that a browser is a local process and that setting
`api_key` closes it.
**TLS.** Also correct, and it made the old advice actively harmful: move the
address, set the key, leave `http.tls` alone, and the key plus the
credential-bearing responses all go out in clear. Named next to `api_key` in
both the README and the warning.
**The endpoint list.** Added `DELETE /{sinks,sources}/{key}/configs` and
`POST /{sinks,sources}/{key}/restart`. You were right that this was what made
the read-only framing look correct.
**Blocking `getaddrinfo`.** Switched to `tokio::net::lookup_host`, same as
`quic_client.rs:424`. Kept it classification-only and did not bind the result,
for the `localhost` to `[::1, 127.0.0.1]` reason you flagged. The four
predicate tests are now `#[tokio::test]`.
**Empty iterator.** Collecting to `Vec<SocketAddr>` and testing `is_empty()`
first, so a resolve-to-nothing reports exposed rather than inheriting the
vacuous `all`. Took the collapse to `is_ok_and` as far as the let-else form,
which reads better now the empty case needs naming.
**The wrong rationale.** This one I would not have caught:
`HttpConfig::default()` really is unreachable because the embedded
`config.toml` is the first figment layer, so `127.0.0.1:8081` is the effective
default and would have parsed. The doc comment now gives the real reason,
`address` being free-form and accepting hostnames, and the test message that
repeated the claim is fixed too.
**`config.api_key` vs `context.api_key`.** Left reading `config`, as you
suggested, but the helper now takes the key as a parameter so a later test
cannot set one and not the other.
**`free_port`.** Deleted, `127.0.0.1:0` straight into `config()`. You were
right that nothing needed the port.
**Global subscriber.** Now per-test: `Arc<Mutex<Vec<(Level, String)>>>`
behind a `set_default` guard, no statics, no shared buffer, and the address
filtering that only existed because of sharing is gone. Added
`.with_filter(LevelFilter::INFO)`, which fixes the `max_level_hint` problem you
noted separately and deletes the hand-rolled level check. The tests document
that they rely on `#[tokio::test]`'s current-thread runtime for the spawned
task to see the thread-local subscriber, and that a multi-thread flavour would
fail them rather than pass silently.
**`ip_nonlocal_bind`.** Comment corrected on both counts, including that a
successful bind leaves a listener for the rest of the binary rather than the
test.
**The missing positive control.** The best catch of the set. The loopback
test asserted only an absence, so it would have kept passing if `enabled` ever
defaulted to false, taking the only in-`init` loopback coverage with it. It now
asserts `init` reached the listener first. Verified by mutating `if
!config.enabled` to `if true`: the control fails with "init must reach the
listener, or the assertion below proves nothing", where previously that
mutation left the test green.
Re-mutated the wiring after the harness change, since I had rewritten the
capture path: removing the guard from `init` still fails with "init must
consult the guard and name the address it is exposing".
Gate: fmt, sort, taplo, hawkeye, typos, markdownlint, clippy `-D warnings`,
`cargo test -p iggy-connectors` 128 passing, all exit 0.
Still open for your call, unchanged: whether #3802's redaction should be a
key-name heuristic over opaque `plugin_config` JSON or a hard `api_key`
requirement on the config routes. This PR does not close that issue.
--
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]
