Repository navigation
[vm-instance] Fix PortList not filtering ingress ports - #2501
Andrei Kvapil (kvaps) merged 2 commits into
Conversation
Render `networking.cozystack.io/wholeIP: "false"` on the Service when `externalMethod: PortList` is configured (was always `"true"` before). Combined with cozy-proxy v0.3.0+ which adds per-port filtering for "false"-annotated services, this makes `externalMethod: PortList` behave as documented: only ports listed in `externalPorts` are reachable from the LoadBalancer IP. Backward-compatible: existing services with `externalMethod: WholeIP` continue to set `wholeIP: "true"` and behave identically. cozy-proxy versions older than v0.3.0 ignore Services with `wholeIP: "false"`, which means PortList Services on older cozy-proxy will lose their egress IP preservation — but that path was already non-functional ingress-wise, so this is not a regression for users actually relying on PortList. Signed-off-by: mattia-eleuteri <[email protected]>
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
github-actions
Bot
added
the
area/uncategorized
- Add `externalAllowICMP` value (default true) propagated as `networking.cozystack.io/allowICMP` annotation on the rendered Service when `externalMethod: PortList`. The cozy-proxy companion (released as part of cozystack/cozy-proxy#11 + cozystack#12) drops ICMP by default in port-filter mode, which breaks ping and PMTU discovery; defaulting the chart to "true" preserves user expectations while still allowing operators to opt out by setting `externalAllowICMP: false`. - Remove the v1.3.1.md changelog entry. Project convention is to add changelogs in a dedicated "docs: add changelog for vX.Y.Z" commit at release time, not as part of feature/fix PRs. Signed-off-by: mattia-eleuteri <[email protected]>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
dosubot
Bot
added
size/M
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enables functional ingress port filtering for Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 introduces a new configuration option, externalAllowICMP, to allow ICMP traffic to virtual machines when using the PortList mode. The changes include updates to the API types, Helm templates, and schema definitions. Regarding the review feedback, the comments correctly identify that manual edits to auto-generated files (such as api/apps/v1alpha1/vminstance/types.go and packages/system/vm-instance-rd/cozyrds/vm-instance.yaml) should be avoided in favor of using make generate. Additionally, the comment regarding values.schema.json correctly notes that manual additions of description fields will be stripped during regeneration.
| // Whether to accept ICMP traffic to the VM in PortList mode (preserves ping and PMTU discovery). No effect in WholeIP mode. Default true so ping behaves as users expect even when port filtering is in effect. | ||
| // +kubebuilder:default:=true | ||
| ExternalAllowICMP bool `json:"externalAllowICMP"` |
There was a problem hiding this comment.
This file is auto-generated as indicated by the header on line 1. Manual changes to this file will be overwritten during the next build or when make generate is executed. According to the project guidelines, you should update the package schema (via values.yaml annotations, which you have already done) and run make generate to update this file automatically.
References
- Suggest changes to the relevant package schema rather than hand-editing generated types. (link)
| plural: vminstances | ||
| openAPISchema: |- | ||
| {"title":"Chart Values","type":"object","properties":{"external":{"description":"Enable external access from outside the cluster.","type":"boolean","default":false},"externalMethod":{"description":"Method to pass through traffic to the VM.","type":"string","default":"PortList","enum":["PortList","WholeIP"]},"externalPorts":{"description":"Ports to forward from outside the cluster.","type":"array","default":[22],"items":{"type":"integer"}},"runStrategy":{"description":"Requested running state of the VirtualMachineInstance","type":"string","default":"Always","enum":["Always","Halted","Manual","RerunOnFailure","Once"]},"instanceType":{"description":"Virtual Machine instance type.","type":"string","default":"u1.medium"},"instanceProfile":{"description":"Virtual Machine preferences profile.","type":"string","default":"ubuntu","enum":["alpine","centos.7","centos.7.desktop","centos.stream10","centos.stream10.desktop","centos.stream8","centos.stream8.desktop","centos.stream8.dpdk","centos.stream9","centos.stream9.desktop","centos.stream9.dpdk","cirros","fedora","fedora.arm64","opensuse.leap","opensuse.tumbleweed","rhel.10","rhel.10.arm64","rhel.7","rhel.7.desktop","rhel.8","rhel.8.desktop","rhel.8.dpdk","rhel.9","rhel.9.arm64","rhel.9.desktop","rhel.9.dpdk","rhel.9.realtime","sles","ubuntu","windows.10","windows.10.virtio","windows.11","windows.11.virtio","windows.2k16","windows.2k16.virtio","windows.2k19","windows.2k19.virtio","windows.2k22","windows.2k22.virtio","windows.2k25","windows.2k25.virtio",""]},"disks":{"description":"List of disks to attach.","type":"array","default":[],"items":{"type":"object","required":["name"],"properties":{"bus":{"description":"Disk bus type (e.g. \"sata\").","type":"string"},"name":{"description":"Disk name.","type":"string"}}}},"networks":{"description":"Networks to attach the VM to.","type":"array","default":[],"items":{"type":"object","properties":{"name":{"description":"Network attachment name.","type":"string"}}}},"subnets":{"description":"Deprecated: use networks instead.","type":"array","default":[],"items":{"type":"object","properties":{"name":{"description":"Network attachment name.","type":"string"}}}},"gpus":{"description":"List of GPUs to attach (NVIDIA driver requires at least 4 GiB RAM).","type":"array","default":[],"items":{"type":"object","required":["name"],"properties":{"name":{"description":"The name of the GPU resource to attach.","type":"string"}}}},"cpuModel":{"description":"Model specifies the CPU model inside the VMI. List of available models https://github.com/libvirt/libvirt/tree/master/src/cpu_map","type":"string","default":""},"resources":{"description":"Resource configuration for the virtual machine.","type":"object","default":{},"properties":{"cpu":{"description":"Number of CPU cores allocated.","pattern":"^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$","anyOf":[{"type":"integer"},{"type":"string"}],"x-kubernetes-int-or-string":true},"memory":{"description":"Amount of memory allocated.","pattern":"^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$","anyOf":[{"type":"integer"},{"type":"string"}],"x-kubernetes-int-or-string":true},"sockets":{"description":"Number of CPU sockets (vCPU topology).","pattern":"^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$","anyOf":[{"type":"integer"},{"type":"string"}],"x-kubernetes-int-or-string":true}}},"sshKeys":{"description":"List of SSH public keys for authentication.","type":"array","default":[],"items":{"type":"string"}},"cloudInit":{"description":"Cloud-init user data.","type":"string","default":""},"cloudInitSeed":{"description":"Seed string to generate SMBIOS UUID for the VM.","type":"string","default":""}}} | ||
| {"title":"Chart Values","type":"object","properties":{"external":{"description":"Enable external access from outside the cluster.","type":"boolean","default":false},"externalMethod":{"description":"Method to pass through traffic to the VM.","type":"string","default":"PortList","enum":["PortList","WholeIP"]},"externalPorts":{"description":"Ports to forward from outside the cluster.","type":"array","default":[22],"items":{"type":"integer"}},"externalAllowICMP":{"description":"Whether to accept ICMP traffic to the VM in PortList mode (preserves ping and PMTU discovery). No effect in WholeIP mode. Default true so ping behaves as users expect even when port filtering is in effect.","type":"boolean","default":true},"runStrategy":{"description":"Requested running state of the VirtualMachineInstance","type":"string","default":"Always","enum":["Always","Halted","Manual","RerunOnFailure","Once"]},"instanceType":{"description":"Virtual Machine instance type.","type":"string","default":"u1.medium"},"instanceProfile":{"description":"Virtual Machine preferences profile.","type":"string","default":"ubuntu","enum":["alpine","centos.7","centos.7.desktop","centos.stream10","centos.stream10.desktop","centos.stream8","centos.stream8.desktop","centos.stream8.dpdk","centos.stream9","centos.stream9.desktop","centos.stream9.dpdk","cirros","fedora","fedora.arm64","opensuse.leap","opensuse.tumbleweed","rhel.10","rhel.10.arm64","rhel.7","rhel.7.desktop","rhel.8","rhel.8.desktop","rhel.8.dpdk","rhel.9","rhel.9.arm64","rhel.9.desktop","rhel.9.dpdk","rhel.9.realtime","sles","ubuntu","windows.10","windows.10.virtio","windows.11","windows.11.virtio","windows.2k16","windows.2k16.virtio","windows.2k19","windows.2k19.virtio","windows.2k22","windows.2k22.virtio","windows.2k25","windows.2k25.virtio",""]},"disks":{"description":"List of disks to attach.","type":"array","default":[],"items":{"type":"object","required":["name"],"properties":{"bus":{"description":"Disk bus type (e.g. \"sata\").","type":"string"},"name":{"description":"Disk name.","type":"string"}}}},"networks":{"description":"Networks to attach the VM to.","type":"array","default":[],"items":{"type":"object","properties":{"name":{"description":"Network attachment name.","type":"string"}}}},"subnets":{"description":"Deprecated: use networks instead.","type":"array","default":[],"items":{"type":"object","properties":{"name":{"description":"Network attachment name.","type":"string"}}}},"gpus":{"description":"List of GPUs to attach (NVIDIA driver requires at least 4 GiB RAM).","type":"array","default":[],"items":{"type":"object","required":["name"],"properties":{"name":{"description":"The name of the GPU resource to attach.","type":"string"}}}},"cpuModel":{"description":"Model specifies the CPU model inside the VMI. List of available models https://github.com/libvirt/libvirt/tree/master/src/cpu_map","type":"string","default":""},"resources":{"description":"Resource configuration for the virtual machine.","type":"object","default":{},"properties":{"cpu":{"description":"Number of CPU cores allocated.","pattern":"^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$","anyOf":[{"type":"integer"},{"type":"string"}],"x-kubernetes-int-or-string":true},"memory":{"description":"Amount of memory allocated.","pattern":"^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$","anyOf":[{"type":"integer"},{"type":"string"}],"x-kubernetes-int-or-string":true},"sockets":{"description":"Number of CPU sockets (vCPU topology).","pattern":"^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$","anyOf":[{"type":"integer"},{"type":"string"}],"x-kubernetes-int-or-string":true}}},"sshKeys":{"description":"List of SSH public keys for authentication.","type":"array","default":[],"items":{"type":"string"}},"cloudInit":{"description":"Cloud-init user data.","type":"string","default":""},"cloudInitSeed":{"description":"Seed string to generate SMBIOS UUID for the VM.","type":"string","default":""}}} |
There was a problem hiding this comment.
The openAPISchema in the ApplicationDefinition appears to have been updated manually. This content is typically synchronized from the package's values.schema.json using make generate. Manual edits to this minified JSON string are error-prone and will be lost on the next regeneration.
References
- Suggest changes to the relevant package schema rather than hand-editing generated types. (link)
| } | ||
| }, | ||
| "externalAllowICMP": { | ||
| "description": "Whether to accept ICMP traffic to the VM in PortList mode (preserves ping and PMTU discovery). No effect in WholeIP mode. Default true so ping behaves as users expect even when port filtering is in effect.", |
There was a problem hiding this comment.
The description field in values.schema.json is automatically stripped when running make generate. It is better to rely on the generator to handle this file based on the annotations in values.yaml.
References
- make generate regenerates this file and strips title, description, and x-* custom annotations. (link)
|
A note on PR title and commit messages: the project adopted Conventional Commits ( Suggested rewording:
If the behavior change for accidentally-open ports under PortList is considered user-visible enough to be a break, append I also stacked the cozy-proxy v0.3.0 chart bump on this branch in mattia-eleuteri#2 — the cozy-proxy side has just been released, and the new datapath only takes effect once both land. Happy to rebase or fold it into your PR however you prefer; the diff is minimal ( |
Pulls in the per-port filtering and allowICMP support that the companion vm-instance chart fix in #2501 relies on. cozy-proxy v0.3.0 also tightens the selector to the standard service.kubernetes.io/service-proxy-name=cozy-proxy label and switches the default ingress mode to port-filter; both are already covered by the vm-instance chart (label landed in #2357, wholeIP/allowICMP wired explicitly in #2501), so VM workloads upgrade transparently. Out-of-tree consumers using cozy-proxy annotations directly (without the label, or relying on the absent-annotation passthrough default) are called out in the upstream v0.3.0 release notes: https://github.com/cozystack/cozy-proxy/releases/tag/v0.3.0 Signed-off-by: Andrei Kvapil <[email protected]>
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
LGTM. Verified:
- Template logic in service.yaml:
wholeIP=truewhenexternalMethod=WholeIP,wholeIP=false+allowICMP(default true) whenPortList. Matches the cozy-proxy v0.3.0 contract. - types.go / values.schema.json / values.yaml / vm-instance-rd cozyrds all updated consistently for the new
externalAllowICMPfield. - README parameter table regenerated.
- Build, E2E, DCO, pre-commit all green.
Companion #2510 ships cozy-proxy v0.3.0 to make this take effect.
Andrei Kvapil (kvaps)
removed
the
do-not-merge/work-in-progress
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-1.2
git worktree add -d .worktree/backport-2501-to-release-1.2 origin/release-1.2
cd .worktree/backport-2501-to-release-1.2
git switch --create backport-2501-to-release-1.2
git cherry-pick -x 80631bc9160a4567cce8facdf781799f15455497 b0afc9a07c8a842915af1fa98ff1e7d17ccb39c8 |
Andrei Kvapil (kvaps)
removed
the
kind/backport-previous
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 -->
What this PR does
The
vm-instancechart now drives the cozy-proxywholeIPandallowICMPannotations explicitly so thatexternalMethod: PortListactually filters ingress traffic to declared ports while keeping ping/PMTU functional.networking.cozystack.io/wholeIP: "false"whenexternalMethod: PortList(was always"true", which silently disabled the PortList semantics in cozy-proxy).externalAllowICMPvalue (defaulttrue) propagated asnetworking.cozystack.io/allowICMPwhenexternalMethod: PortList. Without this, cozy-proxy drops ICMP in port-filter mode (ping/PMTU broken). Operators can setexternalAllowICMP: falseto opt out.The changelog entry is intentionally not part of this PR — it will be added in a dedicated
docs: add changelog for vX.Y.Zcommit at release time, per project convention.Why
externalMethod: PortListis documented as filtering ingress traffic to declared ports but has been non-functional on Cozystack v1.3.0 — verified empirically on a 3-node Talos lab. Root cause was twofold: chart always setwholeIP: "true", and cozy-proxy v0.2.0 had no port-aware logic. The cozy-proxy side was fixed in cozystack/cozy-proxy#11 (merged) and cozystack/cozy-proxy#12 (allowICMP follow-up); this PR completes the user-visible fix on the chart side.Companion PRs
allowICMPannotation for port-filter modeTest plan
wholeIP: "false"Service withspec.ports: [22]: only port 22 reachable from outside; ports 80/443/8080/9999 filterednft list table ip cozy_proxyconfirms expected rulesethelm templaterenders the expected annotation matrix:PortListdefault →wholeIP=false, allowICMP=true;PortListopt-out →allowICMP=false;WholeIP→ onlywholeIP=truemake unit-testspasses locallyBackport
Suggesting
backport-v1.3once merged.Release note
Summary by CodeRabbit
New Features
externalAllowICMPconfiguration option to control ICMP traffic acceptance for VM external access in PortList mode (enabled by default).Documentation