Reconcile OpenStack port additional security groups

The OpenStack Port task tracks kOps-managed SecurityGroups and
user-supplied AdditionalSecurityGroups (by name). Two bugs prevented
AdditionalSecurityGroups from being reconciled on existing ports:

- newPortTaskFromCloud copied the desired AdditionalSecurityGroups into
  the actual state without checking the port, so drift was never
  detected.
- RenderOpenstack had no path to apply security-group changes to an
  existing port, so changes were only picked up when an instance was
  recreated.

Record an additional group in the actual state only when its ID is
actually attached to the port, and update the port in place via
UpdatePort with the merged managed + additional security-group IDs when
the actual and desired sets differ. Add unit tests for both the read
(drift detection) and write (in-place UpdatePort) paths.

Fixes #18731

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Rounak Debnath <r.debnath@x-ion.de>

Author: rdebnath-x-ion

upup/pkg/fi/cloudup/openstacktasks/port.go

@@ -18,6 +18,7 @@ package openstacktasks
 
 import (
 	"fmt"
+	"slices"
 	"sort"
 	"strings"
 
@@ -119,12 +120,31 @@ func getActualAllowedAddressPairs(port *ports.Port, find *Port) []ports.AddressP
 	return allowedAddressPairs
 }
 
+// securityGroupIDsEqual compares two ID-sorted lists of SecurityGroup tasks by ID.
+func securityGroupIDsEqual(a, e []*SecurityGroup) bool {
+	if len(a) != len(e) {
+		return false
+	}
+	for i := range a {
+		if fi.ValueOf(a[i].ID) != fi.ValueOf(e[i].ID) {
+			return false
+		}
+	}
+	return true
+}
+
 func newPortTaskFromCloud(cloud openstack.OpenstackCloud, lifecycle fi.Lifecycle, port *ports.Port, find *Port) (*Port, error) {
+	portSecurityGroupIDs := map[string]struct{}{}
+	for _, sgid := range port.SecurityGroups {
+		portSecurityGroupIDs[sgid] = struct{}{}
+	}
+
 	additionalSecurityGroupIDs := map[string]struct{}{}
+	var actualAdditionalSecurityGroups []string
 	if find != nil {
-		for _, sg := range find.AdditionalSecurityGroups {
+		for _, sgName := range find.AdditionalSecurityGroups {
 			opt := secgroup.ListOpts{
-				Name: sg,
+				Name: sgName,
 			}
 			gs, err := cloud.ListSecurityGroups(opt)
 			if err != nil {
@@ -133,7 +153,12 @@ func newPortTaskFromCloud(cloud openstack.OpenstackCloud, lifecycle fi.Lifecycle
 			if len(gs) == 0 {
 				continue
 			}
-			additionalSecurityGroupIDs[gs[0].ID] = struct{}{}
+			sgID := gs[0].ID
+			if _, ok := portSecurityGroupIDs[sgID]; !ok {
+				continue
+			}
+			additionalSecurityGroupIDs[sgID] = struct{}{}
+			actualAdditionalSecurityGroups = append(actualAdditionalSecurityGroups, sgName)
 		}
 	}
 	sgs := []*SecurityGroup{}
@@ -197,7 +222,7 @@ func newPortTaskFromCloud(cloud openstack.OpenstackCloud, lifecycle fi.Lifecycle
 	if find != nil {
 		find.ID = actual.ID
 		actual.InstanceGroupName = find.InstanceGroupName
-		actual.AdditionalSecurityGroups = find.AdditionalSecurityGroups
+		actual.AdditionalSecurityGroups = actualAdditionalSecurityGroups
 		actual.WellKnownServices = find.WellKnownServices
 	}
 	return actual, nil
@@ -292,6 +317,31 @@ func (*Port) RenderOpenstack(t *openstack.OpenstackAPITarget, a, e, changes *Por
 				return fmt.Errorf("error updating port: %v", err)
 			}
 		}
+		// Compare against the actual state rather than inspecting changes: BuildChanges
+		// copies the expected value into changes, so a list the user emptied out is
+		// indistinguishable there from one that did not change at all.
+		if !slices.Equal(a.AdditionalSecurityGroups, e.AdditionalSecurityGroups) || !securityGroupIDsEqual(a.SecurityGroups, e.SecurityGroups) {
+			klog.V(2).Infof("Updating security groups for Port with name: %q", fi.ValueOf(e.Name))
+			sgs := make([]string, 0, len(e.SecurityGroups)+len(e.AdditionalSecurityGroups))
+			for _, sg := range e.SecurityGroups {
+				sgs = append(sgs, fi.ValueOf(sg.ID))
+			}
+			for _, sgName := range e.AdditionalSecurityGroups {
+				gs, err := t.Cloud.ListSecurityGroups(secgroup.ListOpts{Name: sgName})
+				if err != nil {
+					return fmt.Errorf("error looking up additional security group %q: %v", sgName, err)
+				}
+				if len(gs) == 0 {
+					return fmt.Errorf("additional SecurityGroup not found for name %s", sgName)
+				}
+				sgs = append(sgs, gs[0].ID)
+			}
+			if _, err := t.Cloud.UpdatePort(fi.ValueOf(a.ID), ports.UpdateOpts{
+				SecurityGroups: &sgs,
+			}); err != nil {
+				return fmt.Errorf("error updating port security groups: %v", err)
+			}
+		}
 	}
 	e.ID = a.ID
 	klog.V(2).Infof("Using an existing Openstack port, id=%s", fi.ValueOf(e.ID))

upup/pkg/fi/cloudup/openstacktasks/port_test.go

@@ -508,6 +508,61 @@ func Test_NewPortTaskFromCloud(t *testing.T) {
 			},
 			expectedError: nil,
 		},
+		{
+			desc:      "cloud port found port not nil drops additional security groups not on the port",
+			lifecycle: fi.LifecycleSync,
+			cloud: &portCloud{
+				listSecurityGroups: map[string][]sg.SecGroup{
+					"add-1": {
+						{ID: "add-1-id", Name: "add-1"},
+					},
+					"add-2": {
+						{ID: "add-2-id", Name: "add-2"},
+					},
+				},
+			},
+			cloudPort: &ports.Port{
+				ID:        "id",
+				Name:      "name",
+				NetworkID: "networkID",
+				FixedIPs: []ports.IP{
+					{SubnetID: "subnet-a"},
+				},
+				SecurityGroups: []string{
+					"sg-1",
+					"add-1-id",
+				},
+			},
+			foundPort: &Port{
+				AdditionalSecurityGroups: []string{
+					"add-1",
+					"add-2",
+				},
+			},
+			modifiedFoundPort: &Port{
+				ID: new("id"),
+				AdditionalSecurityGroups: []string{
+					"add-1",
+					"add-2",
+				},
+			},
+			expectedPortTask: &Port{
+				ID:      new("id"),
+				Name:    new("name"),
+				Network: &Network{ID: new("networkID")},
+				SecurityGroups: []*SecurityGroup{
+					{ID: new("sg-1"), Lifecycle: fi.LifecycleSync},
+				},
+				AdditionalSecurityGroups: []string{
+					"add-1",
+				},
+				Subnets: []*Subnet{
+					{ID: new("subnet-a"), Lifecycle: fi.LifecycleSync},
+				},
+				Lifecycle: fi.LifecycleSync,
+			},
+			expectedError: nil,
+		},
 		{
 			desc:      "cloud port found port not nil honors allowed address pairs",
 			lifecycle: fi.LifecycleSync,
@@ -996,14 +1051,15 @@ func Test_Port_CheckChanges(t *testing.T) {
 
 func Test_Port_RenderOpenstack(t *testing.T) {
 	tests := []struct {
-		desc              string
-		target            *openstack.OpenstackAPITarget
-		actual            *Port
-		expected          *Port
-		changes           *Port
-		expectedCloudPort *ports.Port
-		expectedAfter     *Port
-		expectedError     error
+		desc                    string
+		target                  *openstack.OpenstackAPITarget
+		actual                  *Port
+		expected                *Port
+		changes                 *Port
+		expectedCloudPort       *ports.Port
+		expectedAfter           *Port
+		expectedError           error
+		expectedUpdatePortCalls []updatePortCall
 	}{
 		{
 			desc: "actual not nil",
@@ -1200,6 +1256,58 @@ func Test_Port_RenderOpenstack(t *testing.T) {
 			expectedCloudPort: nil,
 			expectedError:     nil,
 		},
+		{
+			desc: "changes in additional security groups merges with managed security groups",
+			target: &openstack.OpenstackAPITarget{
+				Cloud: &portCloud{
+					updatePort: &ports.Port{ID: "cloud-id"},
+					listSecurityGroups: map[string][]sg.SecGroup{
+						"add-1": {
+							{ID: "add-1-id", Name: "add-1"},
+						},
+					},
+				},
+			},
+			actual: &Port{
+				ID:      new("cloud-id"),
+				Name:    new("name"),
+				Network: &Network{ID: new("networkID")},
+				SecurityGroups: []*SecurityGroup{
+					{ID: new("sg-1")},
+				},
+			},
+			expected: &Port{
+				ID:      new("expected-id"),
+				Name:    new("name"),
+				Network: &Network{ID: new("networkID")},
+				SecurityGroups: []*SecurityGroup{
+					{ID: new("sg-1")},
+				},
+				AdditionalSecurityGroups: []string{"add-1"},
+			},
+			changes: &Port{
+				AdditionalSecurityGroups: []string{"add-1"},
+			},
+			expectedAfter: &Port{
+				ID:      new("cloud-id"),
+				Name:    new("name"),
+				Network: &Network{ID: new("networkID")},
+				SecurityGroups: []*SecurityGroup{
+					{ID: new("sg-1")},
+				},
+				AdditionalSecurityGroups: []string{"add-1"},
+			},
+			expectedUpdatePortCalls: []updatePortCall{
+				{
+					id: "cloud-id",
+					opts: ports.UpdateOpts{
+						SecurityGroups: &[]string{"sg-1", "add-1-id"},
+					},
+				},
+			},
+			expectedCloudPort: nil,
+			expectedError:     nil,
+		},
 	}
 
 	for _, testCase := range tests {
@@ -1212,6 +1320,13 @@ func Test_Port_RenderOpenstack(t *testing.T) {
 			if !reflect.DeepEqual(testCase.expected, testCase.expectedAfter) {
 				t.Errorf("Expected Port task differs:\n%v\n\tinstead of\n%v", testCase.expected, testCase.expectedAfter)
 			}
+
+			if testCase.expectedUpdatePortCalls != nil {
+				cloud := testCase.target.Cloud.(*portCloud)
+				if !reflect.DeepEqual(cloud.updatePortCalls, testCase.expectedUpdatePortCalls) {
+					t.Errorf("UpdatePort calls differ:\n%+v\n\tinstead of\n%+v", cloud.updatePortCalls, testCase.expectedUpdatePortCalls)
+				}
+			}
 		})
 	}
 }
@@ -1342,10 +1457,16 @@ type portCloud struct {
 	createPortError         error
 	updatePort              *ports.Port
 	updatePortError         error
+	updatePortCalls         []updatePortCall
 	listSecurityGroups      map[string][]sg.SecGroup
 	listSecurityGroupsError error
 }
 
+type updatePortCall struct {
+	id   string
+	opts ports.UpdateOptsBuilder
+}
+
 func (p *portCloud) ListPorts(opt ports.ListOptsBuilder) ([]ports.Port, error) {
 	return p.listPorts, p.listPortsError
 }
@@ -1359,6 +1480,7 @@ func (p *portCloud) ListSecurityGroups(opt sg.ListOpts) ([]sg.SecGroup, error) {
 }
 
 func (p *portCloud) UpdatePort(id string, opt ports.UpdateOptsBuilder) (*ports.Port, error) {
+	p.updatePortCalls = append(p.updatePortCalls, updatePortCall{id: id, opts: opt})
 	return p.updatePort, p.updatePortError
 }