Repository navigation
fix(apiserver): keep the unauthenticated plain listener on loopback - #215
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Address validation and startup wiring cmd/apiserver/bind.go, cmd/apiserver/main.go, cmd/apiserver/*_test.go, cmd/controller/bind.go, cmd/controller/main.go, cmd/controller/*_test.go |
Both binaries default the plain REST listener to 127.0.0.1:3370 and validate configured addresses. Empty or malformed addresses produce errors. Loopback addresses and localhost are not reported as exposed; other valid hosts are. Startup warns for off-loopback addresses. Tests cover the defaults, validation, and flag registration. |
Loopback deployment and access guidance stand/blockstor-apiserver-deploy.yaml, cmd/apiserver/bind_test.go, docs/layer-stack.md, docs/usage.md, pkg/rest/server.go, tests/e2e/tiebreaker-r-d-cleanup.sh |
The apiserver manifest binds the debug listener to loopback and removes its rest-debug container port declaration. The manifest test checks configured addresses. Documentation and comments describe loopback binding and port-forward access. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix · Severity of issue fixed: High
Merge Risk: 
972fa
-
The plain HTTP listener defaults to loopback. No merge-blocking regression was identified.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 76.92% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: keeping the unauthenticated plain listener on loopback. Linked Issues check Issue #201requires preventing unauthenticated plain-HTTP access from other pods. The PR changes the apiserver default to127.0.0.1:3370, applies the same default to the controller single-binary RES…Out of Scope Changes check The changed code, manifest, tests, documentation, and comments all support the loopback-only plain-HTTP listener objective. Startup validation and warnings support safe handling of explicit non-loopba… Full details: Linked Issues check
-
Explanation
Issue
#201requires preventing unauthenticated plain-HTTP access from other pods. The PR changes the apiserver default to127.0.0.1:3370, applies the same default to the controller single-binary REST mode, and updates the stand manifest to pass the loopback address. The PR adds tests for both defaults, flag registration, and the manifest value. These changes implement the loopback option in#201. The supplied PR evidence is used because the repository diff was unavailable.
Full details: Out of Scope Changes check
-
Explanation
The changed code, manifest, tests, documentation, and comments all support the loopback-only plain-HTTP listener objective. Startup validation and warnings support safe handling of explicit non-loopback addresses. No unrelated change is demonstrated.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 76.92% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 13 functions across 10 files. (3 skipped: 3 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
59f380c to
388cb02
Compare
The plain-HTTP listener serves the whole API without a client certificate. It bound every pod interface by default, so any pod that could reach the apiserver pod IP had full read-write access; only its absence from the Service kept it off the cluster network, and nothing enforced that. The default is now 127.0.0.1:3370, in the apiserver and in the controller's legacy REST mode, and the stand manifest passes the same. kubectl port-forward still reaches it, since it connects inside the pod's network namespace. Tests pin the default and the manifest value to a loopback address. Fixes #201 Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
388cb02 to
59dace5
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) NOT LGTM, but only for a missing license header. The fix itself works.
I tested it on 59dace5, which sits directly on current main. The unit tests in both binaries pass and golangci-lint finds nothing on the new lines. Then I broke the fix piece by piece: the default as :3370, 0.0.0.0:3370 and [::]:3370, the flag registered with a literal or through flag.String, and the manifest argument back to :3370. Every one turned a test red. In-repo clients of the plain port all go through kubectl port-forward. CSI, piraeus and the observability scenarios dial 3371, so nothing in the cluster loses access, and the green e2e lanes on this head agree.
The blocker: four new files have no // SPDX-License-Identifier: Apache-2.0 line. These are bind.go and bind_test.go in both cmd/apiserver and cmd/controller. AGENTS.md:29-30 asks for it on every blockstor Go file, and these are the only Go files under cmd/ and pkg/ without it.
Non-blocking:
- The binaries still accept any
--rest-bind-addresswithout a word. A deployment applied from the old stand manifest passes:3370explicitly, so it stays exposed until the manifest is re-applied. An empty value is worse. The TLS flag treats empty as disabled, butnet.Listen("tcp", "")binds every interface on a random port. So an operator who tries to switch the plain listener off gets it on all addresses. A startup warning for a non-loopback address, plus rejecting an empty one, would cover both. - This conflicts with #216 in
cmd/controller/main.go, on the line above therest-bind-addressflag. If":3370"comes back while resolving it,TestPlainListenerFlagUsesTheLoopbackDefaultcatches that. Also, the networking page in #205 says the manifests bind 3370 on every pod address. After this merges it should say loopback.
…rn off loopback An empty --rest-bind-address reached net.Listen and bound every interface on a random port, so turning the listener off the way the TLS flag allows opened it wider. It is refused now, and an address off loopback, such as the :3370 an older manifest still passes, logs a warning at start. The new files carry the license header. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Thanks, head 972face. All four files carry the license header now. Both binaries also refuse an empty I'll resolve the #216 conflict in |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) LGTM. The license header is in on every Go file under cmd/ and pkg/, and the new check for empty and off-loopback addresses works.
I tested 972face on a trial merge with main at 7bb6606. go vet is clean and the unit tests in both binaries pass. I broke the new code to see the tests catch it: marking localhost as exposed or inverting the loopback check turns TestCheckPlainBindAddress red in both binaries. Putting :3370 back in the controller's flag registration or in the stand manifest still reddens the older tests.
The red integration job on this head is not caused by this PR. The harness builds rest.Server with its own address and never goes through cmd/, and the only pkg/rest change is a comment. The same job is red on main at 7bb6606, with other GroupG and GroupKWF timeouts.
Non-blocking:
- No test covers the call itself. If I replace
checkPlainBindAddresswithfalse, nilin bothmain.gofiles, every test stays green, and an empty--rest-bind-addressbinds every interface again.bind_flag_test.gomakes the same argument for the flag default, so its AST walk could require the call too. - The two
bind.gofiles are byte-identical. One copy inpkg/restwould keep them from drifting. - The conflict with #216 in
cmd/controller/main.gois still there. It's only the--enable-rest-apihelp text above the flag. - The site in #205 still says the manifests bind port 3370 on every pod address and that any pod reaching the pod IP can use it. That's in the networking page (port table, API server section, the list at the end) and in the
linstorclient section of the compatibility page. After this merges it's no longer true.
Main now binds the unauthenticated port 3370 to the pod's loopback (#215) and runs the snapshot stream on 3372 over mutual TLS with a CA of its own (#219). Say so on the networking, compatibility and installation pages, add the stream certificate and the new bind address to the upgrade note, and list the stream TLS failures that keep a satellite from starting. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The apiserver's plain-HTTP listener serves the whole LINSTOR API with no client certificate, and it bound every pod interface by default. Only its absence from the Service kept it off the cluster network, and nothing enforced that: any pod that could reach an apiserver pod IP had full read-write access.
The listener now defaults to
127.0.0.1:3370, in the apiserver and in the controller's single-binary REST mode, and the stand manifest passes the same and no longer declares the container port.kubectl port-forwardkeeps working, since it connects inside the pod's network namespace. Every in-repo client of the plain port already goes through a port-forward; CSI and piraeus use the mutual-TLS port.The controller's single-binary REST mode (
--enable-rest-api=true) has no TLS listener, so with the new default it has no network endpoint at all: in-cluster clients get connection refused. Nothing deployed runs that mode;--rest-bind-address=:3370brings the old behaviour back, exposure included.Fixes #201
Testing
:3370reddens a test.go test ./...: green apart from the knownserver did not stop within 2s after cancelflake inpkg/rest.Summary by CodeRabbit
127.0.0.1:3370by default, limiting access to the local pod.kubectl port-forwardand connect throughlocalhost:3370.