Repository navigation
feat: label-only selector and port-filter default - #13
Merged
Merged
Conversation
Tighten the management contract for cozy-proxy: - The service.kubernetes.io/service-proxy-name=cozy-proxy label is now the sole selector. Services without the label are no longer managed even if they carry the networking.cozystack.io/wholeIP annotation — the annotation alone has no selecting effect. - Whole-IP passthrough becomes opt-in: wholeIPPassthrough returns true only for explicit wholeIP="true". Absent or any other value keeps the service in port-filter mode (consistent with wholeIP="false"), so declared Service.spec.ports are the only ports forwarded by default. Drop the now-unused hasWholeIPAnnotation helper. Tests updated to cover the new isCozyProxyService selector and the new wholeIPPassthrough default. README rewritten: drop the dual-selector narrative and the "Which selector should I use?" section, update the ingress-mode table to mark port-filter as the default, and add wholeIP="true" to the Usage example so the bogus port-65535 demonstration of whole-IP behaviour keeps working. BREAKING CHANGE: Services that previously relied on the wholeIP annotation alone for selection must add the service.kubernetes.io/service-proxy-name: cozy-proxy label. Services that previously relied on the absent-annotation passthrough default must add networking.cozystack.io/wholeIP: "true" to keep that behaviour. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe changes simplify service selection to depend solely on the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the service selection logic for cozy-proxy, making the 'service.kubernetes.io/service-proxy-name: cozy-proxy' label the sole requirement for service management. The 'networking.cozystack.io/wholeIP' annotation now only controls the ingress mode, with the default behavior changing from whole-IP passthrough to per-port filtering. Corresponding updates were made to the documentation and unit tests to align with these logic changes. I have no feedback to provide.
This was referenced Apr 28, 2026
Andrei Kvapil (kvaps)
added a commit
to cozystack/cozystack
that referenced
this pull request
Apr 28, 2026
Bumps the vendored cozy-proxy chart and image tag from v0.2.0 to v0.3.0 by running `make update` in `packages/system/cozy-proxy`. cozy-proxy v0.3.0 release: https://github.com/cozystack/cozy-proxy/releases/tag/v0.3.0 ## Why cozy-proxy v0.3.0 is the companion of #2501. The chart fix in #2501 finally honors the documented PortList semantics (only `externalPorts` reachable, ICMP preserved by default), but only takes effect once cozy-proxy v0.3.0 is in place — v0.2.0 has no port-aware logic, so the chart fix silently no-ops on it. This PR ships that runtime side. ## Upgrade impact For cozystack users this is a **bug fix, not a breaking change**. cozy-proxy v0.3.0 introduces two contract changes upstream — (1) label-only selector via `service.kubernetes.io/service-proxy-name: cozy-proxy`, and (2) port-filter as the default ingress mode — both of which are absorbed by the vm-instance chart (label landed in #2357, `wholeIP` + `allowICMP` wired in #2501). - **`externalMethod: WholeIP`** (default) — chart renders label + `wholeIP: "true"` in both old and new versions. cozy-proxy keeps passthrough. **No change.** - **`externalMethod: PortList`** — previously the chart always rendered `wholeIP: "true"` regardless of method, and cozy-proxy v0.2.0 had no port-aware logic, so PortList was silently a no-op (all ports reachable). After this bump + #2501, PortList behaves as documented: only ports listed in `externalPorts` reach the VM. Users who configured PortList but reached undeclared ports (relying on the silent no-op) will see those ports become unreachable — that's the documented intent of PortList finally taking effect. ICMP keeps working by default thanks to `externalAllowICMP: true`. - **DaemonSet rollover** — re-init of the nft table during cozy-proxy pod restart is a brief blip on existing flows; same as any DaemonSet upgrade. Out-of-tree consumers using cozy-proxy directly (without the `service-proxy-name` label, or relying on the absent-annotation passthrough default) need to migrate per the upstream v0.3.0 release notes — not a cozystack concern. ## Order of operations This PR is functionally complete on its own (image bump), but the user-facing PortList fix it enables only happens when #2501 also lands. Recommended merge order: 1. #2501 first (chart adapts to the new contract) 2. This PR second (cozy-proxy DaemonSet picks up the new image) The reverse order is also safe — cozy-proxy v0.3.0 with the unmodified vm-instance chart still produces correct results for `externalMethod: WholeIP` (most common case) and keeps PortList silently broken until #2501 lands. ## Test plan - [ ] `helm template packages/system/cozy-proxy` — image tag `v0.3.0` rendered - [ ] `make unit-tests` passes - [ ] After #2501 lands, end-to-end: `externalMethod: PortList` with `externalPorts: [22]` filters all other ports, ping works - [ ] WholeIP-method VM Service unchanged after upgrade ## Companion PRs - #2501 — vm-instance chart wires `wholeIP` / `allowICMP` per `externalMethod` - cozystack/cozy-proxy#13 (merged) — label-only selector + port-filter default - cozy-proxy v0.3.0 release notes — https://github.com/cozystack/cozy-proxy/releases/tag/v0.3.0 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Released version 0.3.0 with updated Helm chart metadata and container image configuration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
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.
Summary
Tightens the management contract before the next release:
service.kubernetes.io/service-proxy-name: cozy-proxyare no longer managed, even if they carrynetworking.cozystack.io/wholeIP. The annotation alone has no selecting effect anymore.wholeIPPassthroughis now an explicit opt-in viawholeIP: "true". Absent or any other value falls into per-port filtering, so a Service's declaredspec.portsare the only ports cozy-proxy forwards by default.Drops the now-unused
hasWholeIPAnnotationhelper. Tests cover the new selector and the new default. README rewritten — drops the dual-selector narrative, removes the "Which selector should I use?" section, marks port-filter as default, and updates the Usage example to includewholeIP: "true"so the bogus-port-65535 passthrough demonstration still makes sense.Breaking changes
wholeIPannotation without theservice-proxy-namelabel are no longer managed. Add the label.networking.cozystack.io/wholeIP: "true"to keep the previous behavior.Both behaviors are recent (#9, #11), unreleased, so the breakage scope is limited to in-tree consumers.
Test plan
Summary by CodeRabbit
Documentation
Changes