Skip to content

Commit bbe0e86

Browse files
kvapskvaps
authored
[vm-instance] Fix PortList not filtering ingress ports (#2501)
## What this PR does The `vm-instance` chart now drives the cozy-proxy `wholeIP` and `allowICMP` annotations explicitly so that `externalMethod: PortList` actually filters ingress traffic to declared ports while keeping ping/PMTU functional. - Render `networking.cozystack.io/wholeIP: "false"` when `externalMethod: PortList` (was always `"true"`, which silently disabled the PortList semantics in cozy-proxy). - Add `externalAllowICMP` value (default `true`) propagated as `networking.cozystack.io/allowICMP` when `externalMethod: PortList`. Without this, cozy-proxy drops ICMP in port-filter mode (ping/PMTU broken). Operators can set `externalAllowICMP: false` to 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.Z` commit at release time, per project convention. ## Why `externalMethod: PortList` is 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 set `wholeIP: "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 - cozystack/cozy-proxy#11 (merged) — per-service ingress port filtering - cozystack/cozy-proxy#12 (merged) — `allowICMP` annotation for port-filter mode ## Test plan - [x] Built cozy-proxy with the companion fix locally, deployed on a 3-node Talos lab (Cozystack v1.3.0) - [x] `wholeIP: "false"` Service with `spec.ports: [22]`: only port 22 reachable from outside; ports 80/443/8080/9999 filtered - [x] WholeIP-annotated Service unchanged: all listening ports reachable - [x] Egress IP preservation works in both modes (TCP curl + UDP DNS) - [x] `nft list table ip cozy_proxy` confirms expected ruleset - [x] `helm template` renders the expected annotation matrix: `PortList` default → `wholeIP=false, allowICMP=true`; `PortList` opt-out → `allowICMP=false`; `WholeIP` → only `wholeIP=true` - [x] `make unit-tests` passes locally - [ ] CI unit tests - [ ] CI E2E ## Backport Suggesting `backport-v1.3` once merged. ## Release note ```release-note [vm-instance] Make `externalMethod: PortList` actually filter ingress traffic to ports listed in `externalPorts`. New `externalAllowICMP` knob (default true) propagates the cozy-proxy `allowICMP` annotation to keep ping/PMTU functional in port-filter mode. Combined with cozy-proxy v0.3.0+, only listed ports plus ICMP are reachable from the VM's LoadBalancer IP. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added `externalAllowICMP` configuration option to control ICMP traffic acceptance for VM external access in PortList mode (enabled by default). * **Documentation** * Updated parameter documentation to include the new ICMP traffic control setting. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents 82d1f8a + b0afc9a commit bbe0e86

6 files changed

Lines changed: 43 additions & 28 deletions

File tree

‎api/apps/v1alpha1/vminstance/types.go‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎packages/apps/vm-instance/README.md‎

Lines changed: 26 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -36,31 +36,32 @@ virtctl ssh <user>@<vm>
3636

3737
### Common parameters
3838

39-
| Name | Description | Type | Value |
40-
| ------------------- | --------------------------------------------------------------------------------------------------------------------------------- | ---------- | ----------- |
41-
| `external` | Enable external access from outside the cluster. | `bool` | `false` |
42-
| `externalMethod` | Method to pass through traffic to the VM. | `string` | `PortList` |
43-
| `externalPorts` | Ports to forward from outside the cluster. | `[]int` | `[22]` |
44-
| `runStrategy` | Requested running state of the VirtualMachineInstance | `string` | `Always` |
45-
| `instanceType` | Virtual Machine instance type. | `string` | `u1.medium` |
46-
| `instanceProfile` | Virtual Machine preferences profile. | `string` | `ubuntu` |
47-
| `disks` | List of disks to attach. | `[]object` | `[]` |
48-
| `disks[i].name` | Disk name. | `string` | `""` |
49-
| `disks[i].bus` | Disk bus type (e.g. "sata"). | `string` | `""` |
50-
| `networks` | Networks to attach the VM to. | `[]object` | `[]` |
51-
| `networks[i].name` | Network attachment name. | `string` | `""` |
52-
| `subnets` | Deprecated: use networks instead. | `[]object` | `[]` |
53-
| `subnets[i].name` | Network attachment name. | `string` | `""` |
54-
| `gpus` | List of GPUs to attach (NVIDIA driver requires at least 4 GiB RAM). | `[]object` | `[]` |
55-
| `gpus[i].name` | The name of the GPU resource to attach. | `string` | `""` |
56-
| `cpuModel` | Model specifies the CPU model inside the VMI. List of available models https://github.com/libvirt/libvirt/tree/master/src/cpu_map | `string` | `""` |
57-
| `resources` | Resource configuration for the virtual machine. | `object` | `{}` |
58-
| `resources.cpu` | Number of CPU cores allocated. | `quantity` | `""` |
59-
| `resources.memory` | Amount of memory allocated. | `quantity` | `""` |
60-
| `resources.sockets` | Number of CPU sockets (vCPU topology). | `quantity` | `""` |
61-
| `sshKeys` | List of SSH public keys for authentication. | `[]string` | `[]` |
62-
| `cloudInit` | Cloud-init user data. | `string` | `""` |
63-
| `cloudInitSeed` | Seed string to generate SMBIOS UUID for the VM. | `string` | `""` |
39+
| Name | Description | Type | Value |
40+
| ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------- | ----------- |
41+
| `external` | Enable external access from outside the cluster. | `bool` | `false` |
42+
| `externalMethod` | Method to pass through traffic to the VM. | `string` | `PortList` |
43+
| `externalPorts` | Ports to forward from outside the cluster. | `[]int` | `[22]` |
44+
| `externalAllowICMP` | 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. | `bool` | `true` |
45+
| `runStrategy` | Requested running state of the VirtualMachineInstance | `string` | `Always` |
46+
| `instanceType` | Virtual Machine instance type. | `string` | `u1.medium` |
47+
| `instanceProfile` | Virtual Machine preferences profile. | `string` | `ubuntu` |
48+
| `disks` | List of disks to attach. | `[]object` | `[]` |
49+
| `disks[i].name` | Disk name. | `string` | `""` |
50+
| `disks[i].bus` | Disk bus type (e.g. "sata"). | `string` | `""` |
51+
| `networks` | Networks to attach the VM to. | `[]object` | `[]` |
52+
| `networks[i].name` | Network attachment name. | `string` | `""` |
53+
| `subnets` | Deprecated: use networks instead. | `[]object` | `[]` |
54+
| `subnets[i].name` | Network attachment name. | `string` | `""` |
55+
| `gpus` | List of GPUs to attach (NVIDIA driver requires at least 4 GiB RAM). | `[]object` | `[]` |
56+
| `gpus[i].name` | The name of the GPU resource to attach. | `string` | `""` |
57+
| `cpuModel` | Model specifies the CPU model inside the VMI. List of available models https://github.com/libvirt/libvirt/tree/master/src/cpu_map | `string` | `""` |
58+
| `resources` | Resource configuration for the virtual machine. | `object` | `{}` |
59+
| `resources.cpu` | Number of CPU cores allocated. | `quantity` | `""` |
60+
| `resources.memory` | Amount of memory allocated. | `quantity` | `""` |
61+
| `resources.sockets` | Number of CPU sockets (vCPU topology). | `quantity` | `""` |
62+
| `sshKeys` | List of SSH public keys for authentication. | `[]string` | `[]` |
63+
| `cloudInit` | Cloud-init user data. | `string` | `""` |
64+
| `cloudInitSeed` | Seed string to generate SMBIOS UUID for the VM. | `string` | `""` |
6465

6566

6667
## U Series

‎packages/apps/vm-instance/templates/service.yaml‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,10 @@ metadata:
99
{{- if .Values.external }}
1010
service.kubernetes.io/service-proxy-name: "cozy-proxy"
1111
annotations:
12-
networking.cozystack.io/wholeIP: "true"
12+
networking.cozystack.io/wholeIP: {{ ternary "true" "false" (eq .Values.externalMethod "WholeIP") | quote }}
13+
{{- if eq .Values.externalMethod "PortList" }}
14+
networking.cozystack.io/allowICMP: {{ ternary "true" "false" (ne .Values.externalAllowICMP false) | quote }}
15+
{{- end }}
1316
{{- end }}
1417
spec:
1518
type: {{ ternary "LoadBalancer" "ClusterIP" .Values.external }}

‎packages/apps/vm-instance/values.schema.json‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,11 @@
2626
"type": "integer"
2727
}
2828
},
29+
"externalAllowICMP": {
30+
"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.",
31+
"type": "boolean",
32+
"default": true
33+
},
2934
"runStrategy": {
3035
"description": "Requested running state of the VirtualMachineInstance",
3136
"type": "string",

‎packages/apps/vm-instance/values.yaml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,9 @@ externalMethod: PortList
3131
externalPorts:
3232
- 22
3333

34+
## @param {bool} externalAllowICMP - 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.
35+
externalAllowICMP: true
36+
3437
## @enum {string} RunStrategy - Requested running state of the VirtualMachineInstance
3538
## @value Always - VMI should always be running
3639
## @value Halted - VMI should never be running

0 commit comments

Comments
 (0)