render: Have the context function listen on a TCP port#163
Open
adamwg wants to merge 1 commit into
Open
Conversation
Previously, the context function (which injects and extracts function pipeline context when rendering) listened on a unix socket. This didn't work in environments where containers don't share a kernel with the CLI (e.g., macOS with Docker Desktop), causing render to time out in such environments. Update the context function to listen on a TCP port. We already handle functions listening on TCP on the host, since this is how the "development" function runtime works. Note that we have to listen on 0.0.0.0 (not localhost/127.0.0.1) to ensure we're reachable from Docker. Fixes crossplane#161 Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
Contributor
📝 WalkthroughWalkthroughThe context function listener now uses loopback TCP, publishes its target through function annotations, and render commands pass function addresses through unchanged. The Docker engine no longer adds unix-socket bind mounts, and listener tests were updated for the new target field. ChangesLoopback TCP context function rendering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/crossplane/render/contextfn/listener_test.go (1)
33-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the updated listener coverage table-driven.
Thanks for updating the dial target. Since this test now covers the TCP target contract, could you wrap it in the repo’s table-driven
args/wantshape with a named case/reason so future target cases can be added cleanly? As per path instructions, “**/*_test.go: Enforce table-driven test structure: PascalCase test names (no underscores), args/want pattern, use cmp.Diff with cmpopts.EquateErrors() for error testing.”🤖 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/crossplane/render/contextfn/listener_test.go` around lines 33 - 44, The listener round-trip test is not following the repo’s table-driven test pattern. Update TestListenerRoundTrip in listener_test.go to use a named table case with args/want fields and a reason/name for the TCP target scenario, so additional target cases can be added cleanly. Keep the test logic in the existing Start and grpc.NewClient path, but structure assertions with cmp.Diff and cmpopts.EquateErrors() where errors are checked, and preserve a PascalCase test name without underscores.Source: Path instructions
🤖 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/crossplane/render/contextfn/listener.go`:
- Line 39: The listener in listener.go is currently binding with an unspecified
address, which exposes the insecure gRPC server on all interfaces despite
advertising a loopback target. Update the listen address used by the listener
setup so it binds to 127.0.0.1 only, and keep the returned target consistent
with that local-only binding in the listener/handle flow.
- Around line 67-81: The context-function listener currently advertises a
loopback target in newHandle while binding with ":"0, which actually listens on
all interfaces. Update the listener setup in create/listener code around
lc.Listen and Handle construction to bind explicitly to 127.0.0.1:0 by default,
or add an explicit configuration path if a non-loopback bind is intended. Keep
the existing target formatting logic in Handle consistent with the actual bind
address so the advertised endpoint matches the listener behavior.
---
Nitpick comments:
In `@cmd/crossplane/render/contextfn/listener_test.go`:
- Around line 33-44: The listener round-trip test is not following the repo’s
table-driven test pattern. Update TestListenerRoundTrip in listener_test.go to
use a named table case with args/want fields and a reason/name for the TCP
target scenario, so additional target cases can be added cleanly. Keep the test
logic in the existing Start and grpc.NewClient path, but structure assertions
with cmp.Diff and cmpopts.EquateErrors() where errors are checked, and preserve
a PascalCase test name without underscores.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2bfab7bc-5f1b-4d8e-93a3-b4b8a6641df8
📒 Files selected for processing (6)
cmd/crossplane/render/contextfn/listener.gocmd/crossplane/render/contextfn/listener_test.gocmd/crossplane/render/contextfn/wire.gocmd/crossplane/render/engine_docker.gocmd/crossplane/render/op/cmd.gocmd/crossplane/render/xr/cmd.go
💤 Files with no reviewable changes (1)
- cmd/crossplane/render/engine_docker.go
| // FunctionInput address passed to the render engine. | ||
| Target string | ||
|
|
||
| target string |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'type Handle struct|Target\s+string|target\s+string' cmd/crossplane/render/contextfn/listener.go
if command -v gh >/dev/null; then
gh pr view --json labels --jq '.labels[].name'
fiRepository: crossplane/cli
Length of output: 273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline cmd/crossplane/render/contextfn/listener.go --view expanded
printf '\n--- file ---\n'
cat -n cmd/crossplane/render/contextfn/listener.go | sed -n '1,220p'
printf '\n--- references ---\n'
rg -n '\bHandle\b|\bTarget\b|\btarget\b' cmd/crossplane/render/contextfnRepository: crossplane/cli
Length of output: 6462
Bind the listener to loopback only. lc.Listen(ctx, "tcp", ":0") opens the server on all interfaces even though the handle advertises 127.0.0.1:PORT and uses insecure gRPC. If this is meant to stay local, can we bind 127.0.0.1:0 instead?
🤖 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/crossplane/render/contextfn/listener.go` at line 39, The listener in
listener.go is currently binding with an unspecified address, which exposes the
insecure gRPC server on all interfaces despite advertising a loopback target.
Update the listen address used by the listener setup so it binds to 127.0.0.1
only, and keep the returned target consistent with that local-only binding in
the listener/handle flow.
Source: Coding guidelines
Comment on lines
+67
to
+81
| lis, err := lc.Listen(ctx, "tcp", ":0") | ||
| if err != nil { | ||
| cleanup() | ||
| return nil, errors.Wrapf(err, "cannot listen on %q", sockPath) | ||
| } | ||
|
|
||
| cleanup = func() { | ||
| _ = lis.Close() | ||
| _ = os.RemoveAll(dir) | ||
| } | ||
|
|
||
| // In order for processes in Docker containers to connect to the socket, the | ||
| // socket must be world-writeable and its containing directory must be | ||
| // world-readable. | ||
| if err := os.Chmod(dir, 0o755); err != nil { //nolint:gosec // Necessary. | ||
| cleanup() | ||
| return nil, errors.Wrapf(err, "cannot make socket directory world-readable") | ||
| } | ||
| if err := os.Chmod(sockPath, 0o777); err != nil { //nolint:gosec // Necessary. | ||
| cleanup() | ||
| return nil, errors.Wrapf(err, "cannot make socket file writeable") | ||
| return nil, errors.Wrap(err, "cannot create listener for context function") | ||
| } | ||
|
|
||
| srv := grpc.NewServer(grpc.Creds(insecure.NewCredentials())) | ||
| fn := newServer(contextData) | ||
| fnv1.RegisterFunctionRunnerServiceServer(srv, fn) | ||
|
|
||
| addr := lis.Addr().(*net.TCPAddr) //nolint:forcetypeassert // We specified "tcp" above. | ||
|
|
||
| h := &Handle{ | ||
| Target: "unix://" + sockPath, | ||
| // Report the target as 127.0.0.1:PORT since the render machinery knows | ||
| // how to handle functions listening on loopback. | ||
| target: fmt.Sprintf("127.0.0.1:%d", addr.Port), |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'Listen\(ctx, "tcp", ":0"\)|127\.0\.0\.1|host\.docker\.internal|runtime-development-target' cmd/crossplane/renderRepository: crossplane/cli
Length of output: 3519
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the file with line numbers and inspect relevant call sites.
cat -n cmd/crossplane/render/contextfn/listener.go | sed -n '1,180p'
printf '\n---\n'
cat -n cmd/crossplane/render/render.go | sed -n '130,170p'Repository: crossplane/cli
Length of output: 5826
Bind the context-function listener to loopback by default. The server is advertised as 127.0.0.1:PORT, but ":0" listens on all interfaces. Since the Docker path already rewrites loopback addresses, can this use 127.0.0.1:0 instead? If a non-loopback bind is intentional, make it explicit/configured.
🤖 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/crossplane/render/contextfn/listener.go` around lines 67 - 81, The
context-function listener currently advertises a loopback target in newHandle
while binding with ":"0, which actually listens on all interfaces. Update the
listener setup in create/listener code around lc.Listen and Handle construction
to bind explicitly to 127.0.0.1:0 by default, or add an explicit configuration
path if a non-loopback bind is intended. Keep the existing target formatting
logic in Handle consistent with the actual bind address so the advertised
endpoint matches the listener behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
Previously, the context function (which injects and extracts function pipeline context when rendering) listened on a unix socket. This didn't work in environments where containers don't share a kernel with the CLI (e.g., macOS with Docker Desktop), causing render to time out in such environments.
Update the context function to listen on a TCP port. We already handle functions listening on TCP on the host, since this is how the "development" function runtime works. Note that we have to listen on 0.0.0.0 (not localhost/127.0.0.1) to ensure we're reachable from Docker.
Fixes #161
I have:
./nix.sh flake checkto ensure this PR is ready for review.- [ ] Linked a PR or a docs tracking issue to document this change.backport release-x.ylabels to auto-backport this PR.