feat: add host binding configuration option - #920
Conversation
📝 WalkthroughWalkthroughThis PR adds configurable HTTP server bind addresses through schema, JSON and application configuration types, a CLI flag, and server address construction. Empty hosts default to ChangesHost Address Configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/appconf/json_config.go (1)
37-37: ⚡ Quick winConsider defaulting
HostinsetDefaults()for config-layer consistency.Right now the schema advertises a
0.0.0.0default, but JSON config defaults do not set it explicitly. Centralizing that default insetDefaults()keeps behavior/documentation aligned across consumers.♻️ Suggested tweak
func (j *JSONConfig) setDefaults() { if j.Port == 0 { j.Port = 4000 } + if j.Host == "" { + j.Host = "0.0.0.0" + } if j.Env == "" { j.Env = "development" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/appconf/json_config.go` at line 37, The config struct's Host field is advertised to default to "0.0.0.0" but that default isn't being applied in setDefaults(); update the setDefaults() function to explicitly set Host = "0.0.0.0" when Host is empty (or zero-valued) so JSON-configured instances align with the schema/documentation—look for the Host field on the config struct and the setDefaults() method to add this conditional assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/api/app.go`:
- Around line 153-157: The local variable host defined in app.go (host :=
cfg.Host with fallback to "0.0.0.0") is never used which causes a compile error;
update the server startup to use that host variable instead of cfg.Host (e.g.,
replace any direct use of cfg.Host when constructing the server address/Addr
with the local host variable), or alternatively remove the unused host variable
and implement the fallback directly where the server address is built so the
effective host value is used by the http.Server/start call.
---
Nitpick comments:
In `@internal/appconf/json_config.go`:
- Line 37: The config struct's Host field is advertised to default to "0.0.0.0"
but that default isn't being applied in setDefaults(); update the setDefaults()
function to explicitly set Host = "0.0.0.0" when Host is empty (or zero-valued)
so JSON-configured instances align with the schema/documentation—look for the
Host field on the config struct and the setDefaults() method to add this
conditional assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33c06213-6379-4f77-aa5b-5fc8eaed1b20
📒 Files selected for processing (6)
cmd/api/app.gocmd/api/main.goconfig.example.jsonconfig.schema.jsoninternal/appconf/config.gointernal/appconf/json_config.go
fix : host reference
8d1c11c to
f46490b
Compare
|
|
Shall I update the appropriate comments and documentation for this addition ? |
|
@aaronbrethorst @Ahmedhossamdev Should I make further changes ? |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/api/app_test.go`:
- Line 169: Add table-driven t.Run cases in the server address test around
srv.Addr, retaining the empty-host fallback case and adding an explicitly
configured Host: "127.0.0.1" case that verifies the resulting bind address;
include an IPv6 case only if the implementation supports it. Ensure each host
scenario constructs the server with its table-driven configuration and asserts
the expected address.
In `@cmd/api/app.go`:
- Line 205: Update the server address construction in the visible server
configuration to use net.JoinHostPort with host and strconv.Itoa(cfg.Port)
instead of fmt.Sprintf, preserving correct formatting for IPv4 and IPv6 hosts.
Add a test covering an IPv6 host such as ::1 and confirming the resulting
address is bracketed with the port.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c5965865-caab-4902-a06c-8b50e18d5d69
📒 Files selected for processing (3)
cmd/api/app.gocmd/api/app_test.gointernal/appconf/config.go
|
|
||
| assert.NotNil(t, srv, "Server should not be nil") | ||
| assert.Equal(t, ":8080", srv.Addr, "Server address should match port") | ||
| assert.Equal(t, "0.0.0.0:8080", srv.Addr, "Server address should match port and address") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test the explicitly configured host path.
This test covers only the empty-host fallback. Add a table-driven case for Host: "127.0.0.1" (and the IPv6 case if supported) so the new non-default binding path is verified.
As per coding guidelines, every new branch or condition must be covered by tests, with table-driven t.Run cases preferred for multiple scenarios.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/api/app_test.go` at line 169, Add table-driven t.Run cases in the server
address test around srv.Addr, retaining the empty-host fallback case and adding
an explicitly configured Host: "127.0.0.1" case that verifies the resulting bind
address; include an IPv6 case only if the implementation supports it. Ensure
each host scenario constructs the server with its table-driven configuration and
asserts the expected address.
Source: Coding guidelines
|
|
||
| srv := &http.Server{ | ||
| Addr: fmt.Sprintf(":%d", cfg.Port), | ||
| Addr: fmt.Sprintf("%s:%d", host, cfg.Port), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate app.go =="
fd -a 'app\.go$' . | sed 's#^\./##'
echo "== relevant lines =="
if [ -f cmd/api/app.go ]; then
nl -ba cmd/api/app.go | sed -n '180,220p'
fi
echo "== host config usage =="
rg -n "server\.ListenAndServe|Address|Addr:|Host|CFG|config|port|8080" cmd/api app 2>/dev/null || true
echo "== Go resolver behavior for IPv6 literals =="
cat >/tmp/ipv6_addr_probe.go <<'EOF'
package main
import (
"fmt"
"net"
"strconv"
)
func main() {
for _, host := range []string{"localhost", "127.0.0.1", "::1"} {
port := 8080
fmt.Printf("%-9s => fmt=%q JoinHostPort=%q\n", host, fmt.Sprintf("%s:%d", host, port), net.JoinHostPort(host, strconv.Itoa(port)))
if listener, err := net.Listen("tcp", fmt.Sprintf("%s:%d", host, port)); err != nil {
fmt.Printf("%-9s fmt err = %T %v\n", host, err, err)
} else {
listener.Close()
fmt.Printf("%-9s fmt OK\n", host)
}
if listener, err := net.Listen("tcp", net.JoinHostPort(host, strconv.Itoa(port))); err != nil {
fmt.Printf("%-9s join err = %T %v\n", host, err, err)
} else {
listener.Close()
fmt.Printf("%-9s join OK\n", host)
}
}
}
EOF
go run /tmp/ipv6_addr_probe.goRepository: OneBusAway/maglev
Length of output: 270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant lines =="
if [ -f cmd/api/app.go ]; then
awk '{printf "%6d\t%s\n", NR, $0}' cmd/api/app.go | sed -n '180,220p'
fi
echo "== host/config/address usage in cmd/api and nearby =="
rg -n "server\.ListenAndServe|Addr:|ListenAndServe|Config|host|Host|Port" cmd/api/go.mod 2>/dev/null || true
echo "== Go netaddr parser/listener probe =="
cat >/tmp/ipv6_addr_probe.go <<'EOF'
package main
import (
"fmt"
"net"
"strconv"
)
func main() {
for _, host := range []string{"localhost", "127.0.0.1", "::1"} {
port := 8080
fmt.Printf("%-9s fmt=%q\n", host, fmt.Sprintf("%s:%d", host, port))
if _, err := net.ResolveTCPAddr("tcp", fmt.Sprintf("%s:%d", host, port)); err != nil {
fmt.Printf("%-9s ResolveTCPAddr err: %T %v\n", host, err, err)
}
if listener, err := net.Listen("tcp", fmt.Sprintf("%s:%d", host, port)); err != nil {
fmt.Printf("%-9s Listen fmt err: %T %v\n", host, err, err)
} else {
listener.Close()
fmt.Printf("%-9s Listen fmt OK\n", host)
}
addr := net.JoinHostPort(host, strconv.Itoa(port))
fmt.Printf("%-9s JoinHostPort=%q\n", host, addr)
if listener, err := net.Listen("tcp", addr); err != nil {
fmt.Printf("%-9s Listen join err: %T %v\n", host, err, err)
} else {
listener.Close()
fmt.Printf("%-9s Listen join OK\n", host)
}
}
}
EOF
go run /tmp/ipv6_addr_probe.goRepository: OneBusAway/maglev
Length of output: 2943
Use net.JoinHostPort for IPv6-safe binding.
fmt.Sprintf("%s:%d", host, cfg.Port) produces ::1:8080 for an IPv6 host, which Go rejects as too many colons in address; IPv6 literals must be bracketed. Use net.JoinHostPort(host, strconv.Itoa(cfg.Port)) and add an IPv6 test case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/api/app.go` at line 205, Update the server address construction in the
visible server configuration to use net.JoinHostPort with host and
strconv.Itoa(cfg.Port) instead of fmt.Sprintf, preserving correct formatting for
IPv4 and IPv6 hosts. Add a test covering an IPv6 host such as ::1 and confirming
the resulting address is bracketed with the port.
Code reviewFound 4 issues:
Lines 152 to 157 in 229117a
Lines 204 to 206 in 229117a
Lines 3 to 5 in 229117a
Lines 168 to 170 in 229117a 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
Thanks for this — a configurable bind address is a genuinely useful option to have, and the plumbing is done thoroughly: JSONConfig, ToAppConfig, the CLI flag, the schema, and the -f mutual-exclusion path are all handled consistently with how port is threaded through. That's the tedious part and you got it right.
There are three things I need changed before this can land, all in the same area: the defaults change behavior rather than preserve it.
1. 0.0.0.0 is not the current default behavior.
Today Addr is fmt.Sprintf(":%d", cfg.Port). Go binds a bare :port as a dual-stack IPv6 wildcard socket ([::]:port with IPV6_V6ONLY=0), which accepts both IPv4 and IPv6 connections. 0.0.0.0:port binds AF_INET only, so IPv6 clients silently stop being able to connect. That's the opposite of the PR's stated goal of preserving existing behavior, and it's the kind of regression that won't show up until someone deploys on an IPv6 network.
The fix is to make the empty host stay empty rather than substituting a literal:
srv := &http.Server{
Addr: net.JoinHostPort(cfg.Host, strconv.Itoa(cfg.Port)),
...
}net.JoinHostPort("", "4000") yields ":4000", exactly matching today's behavior. Please also update the schema default and the flag help text away from 0.0.0.0 — describing it as "all interfaces" is fine, but 0.0.0.0 specifically is the IPv4-only wildcard.
2. fmt.Sprintf("%s:%d", host, cfg.Port) can't express an IPv6 address.
-host ::1 produces ::1:4000, which net.Listen rejects with "too many colons in address". So the new option works for hostnames and IPv4 literals but not IPv6 ones. net.JoinHostPort (above) handles the bracketing and fixes this at the same time. CodeRabbit flagged this one too.
3. config.example.json shouldn't set "host": "127.0.0.1".
Both README.markdown and CLAUDE.md instruct users to copy config.example.json to config.json verbatim as the setup step. With this line added, every fresh install becomes loopback-only — a new contributor's first make run would work locally but be unreachable from anything else, with nothing obvious pointing at the cause. It also contradicts the schema's own "default": "0.0.0.0" in the same PR.
Please drop the key from config.example.json. The examples block in config.schema.json is a fine place to show "host": "127.0.0.1" as an illustration, since nobody copies that one.
A couple of smaller notes, not blockers:
- The default would sit more naturally in
JSONConfig.setDefaults()(internal/appconf/json_config.go), which is whereport,env,rate-limit,data-path, andlog-levelall resolve their defaults. Putting it inCreateServermeans the resolved value never exists at the config layer. This becomes moot if you go withnet.JoinHostPortand an empty default, so use your judgment. TestCreateServeronly covers the empty-host path. A case asserting that a configured host actually reachessrv.Addrwould be worth adding — it's the one branch the feature introduces.- The
hostblock inconfig.schema.jsonis indented 4 spaces where its siblings use 6.
Happy to re-review as soon as the bind-address defaults are sorted — the rest of this is in good shape.
|
2 participants
Adds a
hostfield to the configuration file to control the server's binding address, allowing operators to restrict the server to a specific interface (e.g.127.0.0.1) instead of binding to all interfaces (0.0.0.0).The field is optional and defaults to
0.0.0.0to preserve existing behavior.Fixes #896
@aaronbrethorst
@chrisls121
Summary by CodeRabbit
Summary by CodeRabbit
New Features
-hostcommand-line flag and newhostconfiguration field to control the server bind address.Bug Fixes
0.0.0.0:<port>).Documentation
hostwith a default value.