feat: add extraPortMappings support to ClusterSpec - #16
Conversation
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: Clean feature addition -- CreateClusterConfig struct is a good refactor, port generation is correct (first CP node only), validation catches zero ports and duplicates, test coverage is solid. One validation gap where cluster port bindAddress isn't checked.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 1 |
326515b to
6a41f62
Compare
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: Prior bind_address finding addressed. One remaining validation gap: cluster port protocol is not validated against the set KIND supports.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 1 |
6a41f62 to
34036ea
Compare
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: Prior bind-address and protocol findings are cleanly resolved with proper validation and test coverage. One remaining cross-cluster validation gap.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 1 |
34036ea to
eb85838
Compare
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review (incremental)
Summary: Cross-cluster host port validation from the prior review is cleanly resolved — claimed BTreeMap correctly tracks ownership across all clusters with distinct same-cluster vs. cross-cluster error messages, and test coverage is thorough (host_port_claimed_by_two_clusters_rejected, distinct_host_ports_across_clusters_pass). One stale doc comment.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 1 |
eb85838 to
af111b7
Compare
nerdalert
left a comment
nerdalert
left a comment
There was a problem hiding this comment.
Thanks for adding this; the creation plumbing and focused validation are clear. I found two host-binding validation cases worth addressing so valid configurations are not rejected and real runtime collisions still fail during config validation rather than during Docker/Kind startup.
| // an opaque Docker "port is already allocated" error. Tracking the owning | ||
| // cluster lets the message name the conflict instead of just reporting a | ||
| // duplicate. | ||
| let mut claimed: BTreeMap<u16, &str> = BTreeMap::new(); |
There was a problem hiding this comment.
The cluster and service binding registries are currently separate, so a cluster mapping and a host service can both claim the same wildcard host binding (for example, 8080/tcp). Both validation passes succeed, but the second Docker publish fails later during forge up.
Could we validate cluster and service mappings through one environment-wide binding registry? The existing service conflict logic already models protocol and wildcard/specific bind overlap and looks reusable here. A focused test with one cluster port and one service port claiming the same binding would guard this case.
There was a problem hiding this comment.
Agreed — fixed by folding both into one registry.
check_service_port_conflicts is now check_host_port_conflicts and walks cluster mappings and service ports through a single BTreeMap<(host, protocol), Vec<(BindAddr, PortOwner)>>, reusing the existing parse_bind_addr wildcard/specific overlap rules. check_cluster_ports keeps only the per-port checks (non-zero, parseable bind address, protocol KIND accepts), and its own cross-cluster registry is gone — that guarantee now falls out of the shared one.
Conflicts name both claimants, so the message says which cluster or service already holds the binding:
service "web": host port binding 0.0.0.0:8080/tcp is already mapped by cluster "alpha"
Tests: cluster_and_service_claiming_one_binding_rejected is the case you described; wildcard_service_conflicts_with_specific_cluster_binding and cluster_and_service_on_distinct_bind_addresses_pass cover the overlap rules across the two kinds.
| for cluster in &config.spec.clusters { | ||
| for pm in &cluster.ports { | ||
| check_cluster_port(pm, &cluster.name)?; | ||
| if let Some(owner) = claimed.insert(pm.host, cluster.name.as_str()) { |
There was a problem hiding this comment.
Keying this map only by host rejects valid mappings that use the same numeric port with different protocols, such as 8080/tcp and 8080/udp; Docker and Kind treat those as distinct bindings.
Suggested change: include normalized protocol in the conflict key and reuse the existing BindAddr/binds_conflict behavior for address overlap. That would continue rejecting a real duplicate such as two wildcard 8080/tcp mappings while allowing the TCP/UDP pair. Please add focused accept/reject tests for both cases.
There was a problem hiding this comment.
Good catch — the key is now (host port, normalized protocol) and the protocol is lower-cased before comparison, so TCP and tcp are one binding while 8080/tcp and 8080/udp are two. Bind-address overlap goes through the existing BindAddr / wildcard-vs-specific logic rather than a second implementation.
Accept and reject tests, as asked:
cluster_tcp_and_udp_on_one_port_pass— same port, both protocols, one clustercluster_udp_and_service_tcp_on_one_port_pass— the same split across a cluster and a servicecluster_port_protocol_case_insensitive_conflict—TCPandtcpon one port still rejectedcluster_and_service_on_distinct_bind_addresses_pass/wildcard_service_conflicts_with_specific_cluster_binding— specific addresses do not overlap, a wildcard overlaps everything
Two pre-existing service-conflict tests now assert on the richer message (already mapped by service ...) instead of just duplicate, since a two-service conflict names both sides now.
af111b7 to
e92e9aa
Compare
e92e9aa to
ee1bf4b
Compare
Expose KIND extraPortMappings in the forge config schema via a new ports field on ClusterSpec. Enables mapping host ports to container NodePorts for accessing services (Grafana, Prometheus, MLflow) from the host machine. Includes config validation, KIND config generation, and an integration test with a port-mappings fixture. Host bindings are validated through one environment-wide registry covering both cluster mappings and service ports. They are published on the same host by the same container runtime and so compete for one set of bindings; two registries would let a cluster and a service both claim 8080/tcp, pass validation, and fail later during `forge up` with an opaque "port is already allocated". A binding is (host port, protocol, bind address): protocol is compared case-insensitively, TCP and UDP on one port are distinct bindings that Docker and KIND both accept, and a wildcard bind overlaps any other binding of that port and protocol while two specific addresses conflict only when equal. Conflicts name both claimants. Signed-off-by: Ladislav Smola <lsmola@redhat.com>
ee1bf4b to
3364b5f
Compare
github-project-automation
Bot
moved this from Review
to Done
in AI Gateway - Model Serving
Follow-ups to praxis-proxy#13, found by a review pass after it merged. Each one is a defect with an observable consequence; nothing here changes what the benchmark measures. Broken or dead: - The `ai-extended` scenario referenced two config files that do not exist, so selecting it aborted on the first `kubectl create` under `set -euo pipefail`, and report.sh had no branch for its result prefix. - The per-run `kubectl top pod` snapshot always failed into `|| true`, because no stack installs metrics-server and KIND does not ship it. It wrote nine empty `*-resources.txt` files per 3x3 run; the dashboards already plot CPU and memory from cAdvisor. - `export BRANCH=$(...)` in report.sh masked the command's exit status (SC2155), which shellcheck never saw because the lint target only covered `hack/` and `.hooks/`. - Two comments described a `[patch.crates-io]` table that the 0.5.4 bump had already removed. - The prerequisites told you to install forge from a feature branch. praxis-proxy/forge#16 has merged, so `extraPortMappings` is in main. Observability of the trace pipeline: - A `memory_limiter` now runs first in the collector pipeline. The container is capped at 512Mi and nothing shed load before it, so under exporter backpressure the queue grew until the kernel killed the collector and every buffered span went with it. Refusals land in `otelcol_processor_refused_spans`, charted beside the export failures. - The Span Export Failures panel filters to `exporter="otlp/tempo"`. The pipeline also has a `debug` exporter, so the unfiltered sum counted every span twice and a debug-exporter hiccup read as a Tempo failure. Supply chain and build: - praxis-ai is pinned to the commit tagged v0.3.0 rather than to the tag, which is what deny.toml's comment already claimed. A tag can be force-moved upstream; a commit cannot. - `make container FEATURES=otel` plumbs the build-arg through, so the repo's own tooling can build the image the demo READMEs ask for instead of the hand-rolled `docker build` lines they carry today. - A cargo git cache mount alongside the registry one: the ai dependency is a git source and was re-cloned on every image build. Signed-off-by: Ladislav Smola <lsmola@redhat.com>
Follow-ups to praxis-proxy#16, found by review after it merged. The host-binding registry compares bind addresses as `IpAddr`, which distinguishes an address by variant. `::ffff:127.0.0.1` and `127.0.0.1` name the same interface to the container runtime but never compared equal, so a cluster mapping and a service port using the two notations for one interface passed validation and then collided at `forge up` with the opaque "port is already allocated" the registry exists to prevent. `canonical_ip` collapses the mapped form before comparison. Two tests cover it: the mapped pair now conflicts, and `::1` against `127.0.0.1` still passes, because those are genuinely two addresses. Also: - The bind-address parse check was written twice, once for clusters and once for services, differing only in the error prefix. Folded into one `check_bind_address` taking a `PortOwner`, which already renders both prefixes. - `PortMapping::protocol` was documented as "tcp or udp". The field is shared by two validators that accept different values: cluster mappings take tcp, udp or sctp case-insensitively, matching KIND, and service ports take tcp only. The doc now says so, and the two protocol checks explain why they differ. - `generate_kind_config` claimed KIND only supports port mappings on control-plane nodes. KIND accepts the field on any node; one node is enough because kube-proxy makes a NodePort reachable through every node, and publishing the same host port from several would collide. Signed-off-by: Ladislav Smola <lsmola@redhat.com>
4 participants
Closes #17
Summary
ports: Vec<PortMapping>toClusterSpecfor KINDextraPortMappingsTest plan
tests/fixtures/port-mappings.yamlextraPortMappingson first control-plane nodecargo test+cargo clippyclean (451 tests)