Changed code to report invalid host - #2544
Conversation
📝 WalkthroughWalkthroughListener binding callbacks now propagate errors through site exposure and listener update flows. Exposed hosts are validated as DNS-1035 labels, and listener status reports validation or binding failures. Tests cover invalid listener and multi-key-listener hosts and prevent Service creation for invalid values. ChangesListener error propagation and host validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CheckListener
participant Bindings
participant ExtendedBindings
participant HostValidation
participant ListenerStatus
CheckListener->>Bindings: UpdateListener
Bindings->>ExtendedBindings: ListenerUpdated
ExtendedBindings->>HostValidation: validateExposedHost
HostValidation-->>ExtendedBindings: validation result
ExtendedBindings-->>Bindings: exposure error or success
Bindings-->>CheckListener: update and error
CheckListener->>ListenerStatus: set configured status
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/kube/site/site_test.go (1)
778-843: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood targeted regression coverage for the PR's core fix.
Both new tests correctly assert the listener/MKL status reflects "Invalid host" and that no Service gets created. Consider also asserting that no router bridge/listener entry is created for the invalid host, since
s.updateRouterConfig(update)currently still runs regardless of the validation error (see the related comment ininternal/site/bindings.go).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 04348bd8-003f-49e0-99cd-fcbfab2287ee
📒 Files selected for processing (7)
internal/kube/site/extended_bindings.gointernal/kube/site/ports.gointernal/kube/site/site.gointernal/kube/site/site_test.gointernal/site/bindings.gointernal/site/bindings_test.gopkg/nonkube/api/site_state.go
| func (b *Bindings) UpdateListener(name string, listener *skupperv2alpha1.Listener) (qdr.ConfigUpdate, error) { | ||
| if listener == nil { | ||
| return b.deleteListener(name) | ||
| } | ||
| return b.updateListener(listener) | ||
| } | ||
|
|
||
| func (b *Bindings) updateListener(latest *skupperv2alpha1.Listener) qdr.ConfigUpdate { | ||
| func (b *Bindings) updateListener(latest *skupperv2alpha1.Listener) (qdr.ConfigUpdate, error) { | ||
| name := latest.ObjectMeta.Name | ||
| existing, ok := b.listeners[name] | ||
| b.listeners[name] = latest | ||
|
|
||
| if !ok || !reflect.DeepEqual(existing.Spec, latest.Spec) { | ||
| var err error | ||
| if b.handler != nil { | ||
| b.handler.ListenerUpdated(latest) | ||
| err = b.handler.ListenerUpdated(latest) | ||
| } | ||
| return b | ||
| return b, err | ||
| } | ||
| return nil | ||
| return nil, nil | ||
| } | ||
|
|
||
| func (b *Bindings) deleteListener(name string) qdr.ConfigUpdate { | ||
| func (b *Bindings) deleteListener(name string) (qdr.ConfigUpdate, error) { | ||
| if existing, ok := b.listeners[name]; ok { | ||
| delete(b.listeners, name) | ||
| var err error | ||
| if b.handler != nil { | ||
| b.handler.ListenerDeleted(existing) | ||
| err = b.handler.ListenerDeleted(existing) | ||
| } | ||
| return b | ||
| return b, err | ||
| } | ||
| return nil | ||
| return nil, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how a listener without an allocated port/exposed host is treated
# when building the router bridge configuration.
rg -n -A 30 'func \(a \*ExtendedBindings\) updateBridgeConfigForListener' internal/kube/site/extended_bindings.go
# Inspect how *site.Bindings implements qdr.ConfigUpdate.Apply
rg -n -A 30 'func \(b \*Bindings\) Apply' internal/site/bindings.goRepository: skupperproject/skupper
Length of output: 1730
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## relevant extended_bindings locations"
rg -n -A 50 -B 10 'func \(a \*ExtendedBindings\) ListenerUpdated|func \(a \*ExtendedBindings\) UpdateListener|func \(a \*ExtendedBindings\) multiKeyListenerUpdated|func \(a \*ExtendedBindings\) UpdateMultiKeyListener|updateBridgeConfigForListener|updateBridgeConfigForMultiKeyListener|UpdateBridgeConfigForListenerWithHostAndPort|UpdateBridgeConfigForMultiKeyListenerWithHostAndPort' internal/kube/site/extended_bindings.go
echo
echo "## relevant site-bindings locations"
rg -n -A 120 -B 30 'func \(site \*Site\) CheckListener|func \(site \*Site\) updateRouterConfig|func \(site \*Site\) updateBridgeConfigForListener|func \(site \*Site\) updateBridgeConfigForMultiKeyListener|func UpdateBridgeConfigForListenerWithHostAndPort|func UpdateBridgeConfigForMultiKeyListenerWithHostAndPort' internal/kube/site/site.go internal
echo
echo "## bindings.go relevant section"
sed -n '1,240p' internal/site/bindings.go | cat -n
echo
echo "## qdr router config update bridge config definitions"
rg -n -A 80 'func \(.*\*RouterConfig\) UpdateBridgeConfig|type BridgeConfig|func UpdateBridgeConfigForListener|func UpdateBridgeConfigForMultiKeyListenerForListener|UpdateBridgeConfigForListenerWithHostAndPort|UpdateBridgeConfigForMultiKeyListenerWithHostAndPort' internal qdr 2>/dev/null || trueRepository: skupperproject/skupper
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## extended_bindings update/check wiring"
rg -n -A 90 -B 20 'func \(a \*ExtendedBindings\) UpdateListener|func \(a \*ExtendedBindings\) UpdateMultiKeyListener|func \(s \*Site\) CheckListener|func \(s \*Site\) updateRouterConfig|func \(s \*Site\) updateBridgeConfigForListener|func \(s \*Site\) updateBridgeConfigForMultiKeyListener' internal/kube/site/extended_bindings.go internal/kube/site/site.go
echo
echo "## qdr port mapping definitions"
rg -n -A 80 -B 10 'type PortMapping|func RecoverPortMapping|func \(.*PortMapping\) GetPortForKey|func \(.*PortMapping\) ReleasePortForKey|PortMapping' internal/qdr internal/kube/site
echo
echo "## behavior probe from source: invalid host does not expose, but bridge config can allocate port if mapping succeeds"
python3 - <<'PY'
from pathlib import Path
for rel in ["internal/kube/site/extended_bindings.go", "internal/kube/site/site.go"]:
p = Path(rel)
print(f"\n=== {rel} ===")
text = p.read_text()
for start in [text.index("func (a *ExtendedBindings) ListenerUpdated"), text.index("func (a *ExtendedBindings) UpdateListener"), text.index("func (a *ExtendedBindings) UpdateMultiKeyListener")]:
end = text.index("\n}", start)
block = text[start:end+2]
print(block)
PYRepository: skupperproject/skupper
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## internal/qdr/port_mapping.go"
sed -n '1,100p' internal/qdr/port_mapping.go | cat -n
echo
echo "## internal/qdr/router config Apply/UpdateBridgeConfig paths"
rg -n -A 50 -B 10 'func \(.*Bindings.*\) Apply|UpdateBridgeConfig|ToBridgeConfig|ToBridgeConfigWith|AddTcpListener|AddListenerAddress' internal/qdr internal/site internal/kube/site/extended_bindings.go internal/kube/site/site.go | sed -n '1,220p'
echo
echo "## read-only call-graph/data-shape verifier for invalid-listener path"
python3 - <<'PY'
from pathlib import Path
import re
files = {
"bindings": Path("internal/site/bindings.go").read_text(),
"extended_bindings": Path("internal/kube/site/extended_bindings.go").read_text(),
"site": Path("internal/kube/site/site.go").read_text(),
"listener": Path("internal/site/listener.go").read_text(),
"multikeylistener": Path("internal/site/multikeylistener.go").read_text(),
"port_mapping": Path("internal/qdr/port_mapping.go").read_text(),
}
def extract_func(name, text):
m = re.search(rf"func \(.*?\) {name}\([^)]*\).*?\n{{", text)
if not m:
return None
start = m.start()
depth = 0
for i in range(start, len(text)):
if text[i] == "{":
depth += 1
elif text[i] == "}":
depth -= 1
if depth == 0:
return text[start:i+1]
return None
funcs = {
"site.Bindings.updateListener": extract_func("updateListener", files["bindings"]),
"internal/kube/site.ExtendedBindings.ListenerUpdated": extract_func("ListenerUpdated", files["extended_bindings"]),
"internal/kube/site.ExtendedBindings.UpdateListener": extract_func("UpdateListener", files["extended_bindings"]),
"internal/kube/site.ExtendedBindings.multiKeyListenerUpdated": extract_func("multiKeyListenerUpdated", files["extended_bindings"]),
"internal/kube/site.ExtendedBindings.UpdateMultiKeyListener": extract_func("UpdateMultiKeyListener", files["extended_bindings"]),
"internal/kube/site.ExtendedBindings.updateBridgeConfigForListener": extract_func("updateBridgeConfigForListener", files["extended_bindings"]),
"internal/kube/site.ExtendedBindings.updateBridgeConfigForMultiKeyListener": extract_func("updateBridgeConfigForMultiKeyListener", files["extended_bindings"]),
"internal/kube/site.Site.CheckListener": extract_func("CheckListener", files["site"]),
"internal/kube/site.Site.CheckMultiKeyListener": extract_func("CheckMultiKeyListener", files["site"]),
}
for name, text in funcs.items():
print(f"\n=== {name} ===")
if text is None:
print("MISSING")
continue
for line_no, line in enumerate(text.splitlines(), 1):
print(f"{line_no:4}: {line}", end=":")
for token in ("return err", "return b", "return nil", "GetPortForKey", "UpdateBridgeConfigForListenerWithHostAndPort", "UpdateBridgeConfigForMultiKeyListenerWithHostAndPort", "unexpose", "Expose", "return false", "return true"):
if token in line:
print(f" // contains {token}")
break
print("\n=== source-derived invalid-listener path invariants ===")
check = funcs["internal/kube/site.Site.CheckListener"]
u = funcs["internal/site/bindings.go"].extract_func("updateListener", files["listeners"]) if False else extract_func("updateListener", files["bindings"])
lu = extract_func("ListenerUpdated", files["extended_bindings"])
cu = extract_func("updateBridgeConfigForListener", files["extended_bindings"])
lm = extract_func("listener.go.UpdateBridgeConfigForListenerWithHostAndPort", files["listener"])
pm = extract_func("GetPortForKey", files["port_mapping"])
checks = {
("ListenerUpdated returns before exposing on invalid host", "validateExposedHost(listener.Spec.Host)" in lu and "return err" in lu.split("allocatedRouterPort, err := a.mapping.GetPortForKey", 1)[0]),
("Listener callback returns error for invalid host", "return err" in lu),
("updateListener returns ConfigUpdate even when listener callback returns non-nil err", "return b, err" in u and "return nil, nil" not in u.split("if !ok ", 1)[1].split("return b, err", 1)[1]),
("sit.CheckListener applies router config when update is non-nil", "return stderrors.Join(err1, s.updateRouterConfig(update))" in check and "err2 := s.updateRouterConfig(update)" in check),
("Bridge listener config uses allocatedHost empty string and allocated port", "site.UpdateBridgeConfigForListenerWithHostAndPort(siteId, listener, \"\", port, config)" in cu),
("Bridge multikeylistener config uses allocatedHost empty string and allocated port", "site.UpdateBridgeConfigForMultiKeyListenerWithHostAndPort(siteId, mkl, \"\", port, config)" in extract_func("updateBridgeConfigForMultiKeyListener", files["extended_bindings"])),
("UpdateBridgeConfigForListenerWithHostAndPort unconditionally adds tcp listener", "config.AddTcpListener(qdr.TcpEndpoint" in funcs["internal/site/listener.go"]),
("PortMapping GetPortForKey allocates a new PortMapping entry on cache miss", "allocated, err := p.pool.NextFreePort()" in pm and "p.mappings[key] = allocated" in pm),
}
for desc, ok in checks.items():
print(f"- {desc}: {'yes' if ok else 'no'}")
PYRepository: skupperproject/skupper
Length of output: 20020
Gate listener router configuration on a valid host.
For invalid listener/MultiKeyListener hosts, ListenerUpdated / multiKeyListenerUpdated return the host validation error before exposing the service or allocating ExposedPorts, but site.Bindings.updateListener still returns b, and ExtendedBindings.Appy builds bridge config with qdr.GetPortForKey. That allocates a new port entry and UpdateBridgeConfigForListenerWithHostAndPort(..., "", port, ...) adds an exposed host listener to the router config. Suppress the bridge-config update/error path when host validation fails, handling MultiKeyListeners as well.
fgiorgetti
left a comment
fgiorgetti
left a comment
There was a problem hiding this comment.
It works great with both Listeners and MultiKeyListeners.
My only suggestion is to add the same host validation at the CLI level, when running skupper listener create or skupper listener generate.
But one thing to be cautions about it. On system sites (nonkube) the host can represent either a FQDN or an IP Address. So if we add such validation, it must be done exclusively to kubernetes sites. @nluaces @c-kruse thoughts? |
It makes sense, currently the listener commands for create and generate in kubernetes platforms don't validate the hostname. But there is already a host validation for the non-kube commands listener create/generate: hostname: (for the listener generate command the host validation is identical) |
3 participants
Addresses #2542
Changed code to return an error instead of smoothing over it and having it only appearing in the control log
Summary by CodeRabbit
Bug Fixes
Tests