OpenStack: Port additionalSecurityGroups are neither reconciled nor applied to existing ports
- Dominant language
- Go
- Stars
- 16.7k
- Forks
- 4.7k
- Avg merge
- 17h 21m
- Merged PRs (30d)
- 68
Description
/kind bug
**1. What `kops` version are you running?**
`master` (HEAD). The behaviour is present in released versions that contain the OpenStack `Port` task as well — the affected code is not new.
**2. What Kubernetes version are you running?**
N/A — this is a kOps reconciliation bug in the OpenStack provider and is independent of the Kubernetes version.
**3. What cloud provider are you using?**
OpenStack
**4. What commands did you run? What is the simplest way to reproduce this issue?**
```
kops edit ig # add or change spec.additionalSecurityGroups
kops update cluster --yes # expected to attach/detach the group on existing ports
```
**5. What happened after the commands executed?**
Nothing changed on the existing ports. `kops update cluster` reports no port change, and the security groups attached to already-running instances' ports are left untouched. The new `additionalSecurityGroups` value only takes effect on instances that happen to be recreated later (e.g. via a `rolling-update`), because groups are applied at port-**create** time.
**6. What did you expect to happen?**
`kops update cluster --yes` should reconcile the port's security groups in place — attaching newly-added `additionalSecurityGroups` and removing ones no longer desired — without requiring the instance to be recreated.
## Background
The OpenStack `Port` task (`upup/pkg/fi/cloudup/openstacktasks/port.go`) tracks two distinct sets of security groups:
- `SecurityGroups` — groups kOps creates and manages itself.
- `AdditionalSecurityGroups` — extra groups, referenced **by name**, that the user attaches but kOps does not own.
An OpenStack port exposes a single flat list of security-group IDs, so the task has to split that list into "managed" vs "additional" when reading, and merge the two back into one list when writing. There are two independent bugs at that split/merge boundary.
### Bug 1 — the actual state does not reflect reality (drift is invisible)
In `newPortTaskFromCloud` (the function backing `Port.Find`), the reconstructed "actual" state simply copies the desired configuration:
```go
actual.AdditionalSecurityGroups = find.AdditionalSecurityGroups
```
It never checks whether those groups are actually present on the port. As a result, "actual" always equals "expected" for this field, so kOps can never detect that an additional group is missing (or was removed out-of-band) and never schedules a correction.
### Bug 2 — changes are never applied to an existing port
`Port.RenderOpenstack` handles creation, and for existing ports it updates `Tags` and `AllowedAddressPairs` — but it has **no code path** that pushes a security-group change to an existing port. So even when a change is desired, it is silently dropped; the only way a group ends up on a port is at port creation (`portCreateOptsFromPortTask`), i.e. when the instance is (re)created.
## How to reproduce it (precisely)
1. Create a cluster with an instance group that has no (or one) `additionalSecurityGroups`.
2. Add a security group to `spec.additionalSecurityGroups` (the group must already exist in OpenStack), then `kops update cluster --yes`.
3. Inspect a running instance's port: `openstack port show -c security_group_ids`.
4. Observe the new group is **not** attached. It only appears on instances created after the change (e.g. after a `kops rolling-update cluster`).
The mirror case: manually detach an `additionalSecurityGroups` member from a port out-of-band, then `kops update cluster --yes` — kOps reports no drift and does not re-attach it (Bug 1).
## Proposed fix
Two parts, matching the two bugs:
1. In `newPortTaskFromCloud`, resolve each configured additional group name to its ID and record it in the actual state **only if that ID is actually present on the port**, so a missing group correctly surfaces as drift.
2. In `RenderOpenstack`, when the actual vs desired security groups differ, build the merged set of managed group IDs + resolved additional group IDs and call `UpdatePort` with it. Because OpenStack replaces the entire security-group set on update, both kinds must be sent together.
The change detection compares the **actual** and **expected** tasks directly rather than inspecting `changes`, because `BuildChanges` copies the expected value into `changes` for slice fields, making "the user emptied the list" indistinguishable there from "no change".
I have a change ready with unit tests covering both the read (drift detection) and write (in-place `UpdatePort`) paths, and will open a PR referencing this issue.
Contributor guide
Research direction
Start in upup/pkg/fi/cloudup/openstacktasks/port.go, reading newPortTaskFromCloud and RenderOpenstack along with the existing Port unit tests. Verify that actual additional security groups reflect IDs present on the port and that updates reconcile the complete security-group set in place; add tests covering both read-side drift detection and the write-side UpdatePort path.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- cloud, infrastructure
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100