Skip to content

OSASINFRA-4329: Ensure tag extension is available #421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
danchild wants to merge 4 commits into
base: release-0.9
Choose a base branch
from
+150 −34
Open

OSASINFRA-4329: Ensure tag extension is available #421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
699f9e0
Merge pull request #2612 from shiftstack/webhooks-0.9
k8s-ci-robot Aug 26, 2025
591861a
Fix instance deletion order
mandre Nov 19, 2025
2c507cd
Merge pull request #2842 from shiftstack/server-interface-deletion-or…
k8s-ci-robot Nov 20, 2025
29cec45
fix: check for standard-attr-tag extension before tagging ports
danchild Jun 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 7 additions & 26 deletions pkg/cloud/services/compute/instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,12 +657,13 @@ func (s *Service) DeleteInstance(openStackCluster *infrav1.OpenStackCluster, eve
return err
}

// get and delete trunks
for _, port := range instanceInterfaces {
if err = s.deleteAttachInterface(eventObject, instanceStatus.InstanceIdentifier(), port.PortID); err != nil {
return err
}
// Delete the instance before ports, since openstack likely prevents the deletion of the primary NIC.
// Deleting the instance also automatically detach the interfaces.
if err := s.deleteInstance(eventObject, instanceStatus.InstanceIdentifier()); err != nil {
return err
}

for _, port := range instanceInterfaces {
if trunkSupported {
if err = networkingService.DeleteTrunk(eventObject, port.PortID); err != nil {
return err
Expand All @@ -685,7 +686,7 @@ func (s *Service) DeleteInstance(openStackCluster *infrav1.OpenStackCluster, eve
}
}

return s.deleteInstance(eventObject, instanceStatus.InstanceIdentifier())
return nil
}

func (s *Service) deleteVolumes(instanceSpec *InstanceSpec) error {
Expand Down Expand Up @@ -740,26 +741,6 @@ func (s *Service) deletePorts(eventObject runtime.Object, nets []servers.Network
return nil
}

func (s *Service) deleteAttachInterface(eventObject runtime.Object, instance *InstanceIdentifier, portID string) error {
err := s.getComputeClient().DeleteAttachedInterface(instance.ID, portID)
if err != nil {
if capoerrors.IsNotFound(err) {
record.Eventf(eventObject, "SuccessfulDeleteAttachInterface", "Attach interface did not exist: instance %s, port %s", instance.ID, portID)
return nil
}
if capoerrors.IsConflict(err) {
// we don't want to block deletion because of Conflict
// due to instance must be paused/active/shutoff in order to detach interface
return nil
}
record.Warnf(eventObject, "FailedDeleteAttachInterface", "Failed to delete attach interface: instance %s, port %s: %v", instance.ID, portID, err)
return err
}

record.Eventf(eventObject, "SuccessfulDeleteAttachInterface", "Deleted attach interface: instance %s, port %s", instance.ID, portID)
return nil
}

func (s *Service) deleteInstance(eventObject runtime.Object, instance *InstanceIdentifier) error {
err := s.getComputeClient().DeleteServer(instance.ID)
if err != nil {
Expand Down
7 changes: 3 additions & 4 deletions pkg/cloud/services/compute/instance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1032,13 +1032,12 @@ func TestService_DeleteInstance(t *testing.T) {
Alias: "trunk",
},
}}, nil)
r.compute.DeleteAttachedInterface(instanceUUID, portUUID).Return(nil)
r.compute.DeleteServer(instanceUUID).Return(nil)
r.compute.GetServer(instanceUUID).Return(nil, gophercloud.ErrDefault404{})

// FIXME: Why we are looking for a trunk when we know the port is not trunked?
r.network.ListTrunk(trunks.ListOpts{PortID: portUUID}).Return([]trunks.Trunk{}, nil)
r.network.DeletePort(portUUID).Return(nil)

r.compute.DeleteServer(instanceUUID).Return(nil)
r.compute.GetServer(instanceUUID).Return(nil, gophercloud.ErrDefault404{})
},
wantErr: false,
},
Expand Down
29 changes: 25 additions & 4 deletions pkg/cloud/services/networking/port.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,25 @@ func (s *Service) GetOrCreatePort(eventObject runtime.Object, clusterName string
var tags []string
tags = append(tags, instanceTags...)
tags = append(tags, portOpts.Tags...)
if len(tags) > 0 {

tagsAreSupported, err := s.getStdAttrTagSupport()

if err != nil {
record.Warnf(eventObject, "FailedGetTagSupport", "Failed to verify neutron support for the standard-attr-tag extension, skipping tags: %v", err)
}

// Return error if user sets port tags; Skip over but warn for instance tags
if !tagsAreSupported {
if len(portOpts.Tags) > 0 {
err = fmt.Errorf("cannot tag port %s using port tags; tags are configured but neutron does not support the standard-attr-tag extension", portName)
record.Warnf(eventObject, "FailedTagPort", "Failed to tag port %s: %v", portName, err)
return nil, err
}

record.Warnf(eventObject, "FailedTagPort", "Neutron does not support the standard-attr-tag extension, skipping instance tags for port %s", portName)
}
Comment on lines +173 to +188
Copy link
Copy Markdown

@stephenfin stephenfin Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we place all of this inside an if len(tags) > 0 block? I don't see any point making a request to the server if we don't have any tags to update. Something like

tagsAreSupported := False
if len(tags) > 0 {
	// ... your changes
}

if len(tags) > 0 && tagsAreSupported


if len(tags) > 0 && tagsAreSupported {
if err = s.replaceAllAttributesTags(eventObject, portResource, port.ID, tags); err != nil {
record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace port tags %s: %v", portName, err)
return nil, err
Expand All @@ -182,9 +200,12 @@ func (s *Service) GetOrCreatePort(eventObject runtime.Object, clusterName string
record.Warnf(eventObject, "FailedCreateTrunk", "Failed to create trunk for port %s: %v", portName, err)
return nil, err
}
if err = s.replaceAllAttributesTags(eventObject, trunkResource, trunk.ID, tags); err != nil {
record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace trunk tags %s: %v", portName, err)
return nil, err

if tagsAreSupported {
if err = s.replaceAllAttributesTags(eventObject, trunkResource, trunk.ID, tags); err != nil {
record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace trunk tags %s: %v", portName, err)
return nil, err
}
}
}

Expand Down
100 changes: 100 additions & 0 deletions pkg/cloud/services/networking/port_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,12 @@ limitations under the License.
package networking

import (
"errors"
"testing"

"github.com/golang/mock/gomock"
common "github.com/gophercloud/gophercloud/openstack/common/extensions"
"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions"
"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/attributestags"
"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/portsbinding"
"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/portsecurity"
Expand Down Expand Up @@ -149,6 +152,7 @@ func Test_GetOrCreatePort(t *testing.T) {
AllowedAddressPairs: []ports.AddressPair{},
},
}).Return(&ports.Port{ID: portID1}, nil)
m.ListExtensions().Return([]extensions.Extension{}, nil)
},
&ports.Port{ID: portID1},
false,
Expand Down Expand Up @@ -232,6 +236,7 @@ func Test_GetOrCreatePort(t *testing.T) {
Return(&ports.Port{
ID: portID1,
}, nil)
m.ListExtensions().Return([]extensions.Extension{{Extension: common.Extension{Alias: "standard-attr-tag"}}}, nil)
m.ReplaceAllAttributesTags("ports", portID1, attributestags.ReplaceAllOpts{Tags: []string{"my-port-tag"}}).Return([]string{"my-port-tag"}, nil)
m.
ListSubnet(subnets.ListOpts{
Expand Down Expand Up @@ -323,6 +328,7 @@ func Test_GetOrCreatePort(t *testing.T) {
},
},
).Return(&ports.Port{ID: portID1}, nil)
m.ListExtensions().Return([]extensions.Extension{}, nil)
},
&ports.Port{ID: portID1},
false,
Expand Down Expand Up @@ -352,6 +358,7 @@ func Test_GetOrCreatePort(t *testing.T) {
AllowedAddressPairs: []ports.AddressPair{},
},
}).Return(&ports.Port{ID: portID1}, nil)
m.ListExtensions().Return([]extensions.Extension{{Extension: common.Extension{Alias: "standard-attr-tag"}}}, nil)
m.ReplaceAllAttributesTags("ports", portID1, attributestags.ReplaceAllOpts{Tags: []string{"my-instance-tag"}}).Return([]string{"my-instance-tag"}, nil)
},
&ports.Port{ID: portID1},
Expand Down Expand Up @@ -383,6 +390,7 @@ func Test_GetOrCreatePort(t *testing.T) {
AllowedAddressPairs: []ports.AddressPair{},
},
}).Return(&ports.Port{ID: portID1}, nil)
m.ListExtensions().Return([]extensions.Extension{{Extension: common.Extension{Alias: "standard-attr-tag"}}}, nil)
m.
ReplaceAllAttributesTags("ports", portID1, attributestags.ReplaceAllOpts{Tags: []string{"my-instance-tag", "my-port-tag"}}).
Return([]string{"my-instance-tag", "my-port-tag"}, nil)
Expand Down Expand Up @@ -429,12 +437,102 @@ func Test_GetOrCreatePort(t *testing.T) {
Description: "Created by cluster-api-provider-openstack cluster test-cluster",
}).Return(&trunks.Trunk{ID: trunkID}, nil)

m.ListExtensions().Return([]extensions.Extension{{Extension: common.Extension{Alias: "standard-attr-tag"}}}, nil)
m.ReplaceAllAttributesTags("ports", portID1, attributestags.ReplaceAllOpts{Tags: []string{"my-tag"}}).Return([]string{"my-tag"}, nil)
m.ReplaceAllAttributesTags("trunks", trunkID, attributestags.ReplaceAllOpts{Tags: []string{"my-tag"}}).Return([]string{"my-tag"}, nil)
},
&ports.Port{Name: "foo-port-1", ID: portID1},
false,
},
{
"skips instance tags when standard-attr-tag extension is not supported",
"foo-port-1",
infrav1.PortOpts{
Network: &infrav1.NetworkFilter{
ID: netID,
},
},
nil,
[]string{"my-instance-tag"},
func(m *mock.MockNetworkClientMockRecorder) {
m.
ListPort(ports.ListOpts{
Name: "foo-port-1",
NetworkID: netID,
}).Return([]ports.Port{}, nil)
m.CreatePort(portsbinding.CreateOptsExt{
CreateOptsBuilder: ports.CreateOpts{
Name: "foo-port-1",
Description: "Created by cluster-api-provider-openstack cluster test-cluster",
NetworkID: netID,
AllowedAddressPairs: []ports.AddressPair{},
},
}).Return(&ports.Port{ID: portID1}, nil)
m.ListExtensions().Return([]extensions.Extension{}, nil)
},
&ports.Port{ID: portID1},
false,
},
{
"errors when port tags are configured but standard-attr-tag extension is not supported",
"foo-port-1",
infrav1.PortOpts{
Network: &infrav1.NetworkFilter{
ID: netID,
},
Tags: []string{"my-port-tag"},
},
nil,
nil,
func(m *mock.MockNetworkClientMockRecorder) {
m.
ListPort(ports.ListOpts{
Name: "foo-port-1",
NetworkID: netID,
}).Return([]ports.Port{}, nil)
m.CreatePort(portsbinding.CreateOptsExt{
CreateOptsBuilder: ports.CreateOpts{
Name: "foo-port-1",
Description: "Created by cluster-api-provider-openstack cluster test-cluster",
NetworkID: netID,
AllowedAddressPairs: []ports.AddressPair{},
},
}).Return(&ports.Port{ID: portID1}, nil)
m.ListExtensions().Return([]extensions.Extension{}, nil)
},
nil,
true,
},
{
"errors when port tags are configured and ListExtensions fails",
"foo-port-1",
infrav1.PortOpts{
Network: &infrav1.NetworkFilter{
ID: netID,
},
Tags: []string{"my-port-tag"},
},
nil,
nil,
func(m *mock.MockNetworkClientMockRecorder) {
m.
ListPort(ports.ListOpts{
Name: "foo-port-1",
NetworkID: netID,
}).Return([]ports.Port{}, nil)
m.CreatePort(portsbinding.CreateOptsExt{
CreateOptsBuilder: ports.CreateOpts{
Name: "foo-port-1",
Description: "Created by cluster-api-provider-openstack cluster test-cluster",
NetworkID: netID,
AllowedAddressPairs: []ports.AddressPair{},
},
}).Return(&ports.Port{ID: portID1}, nil)
m.ListExtensions().Return(nil, errors.New("connection refused"))
},
nil,
true,
},
{
"creates port with value_specs",
"foo-port-1",
Expand Down Expand Up @@ -469,6 +567,7 @@ func Test_GetOrCreatePort(t *testing.T) {
ValueSpecs: &valueSpecs,
},
}).Return(&ports.Port{ID: portID1}, nil)
m.ListExtensions().Return([]extensions.Extension{}, nil)
},
&ports.Port{ID: portID1},
false,
Expand Down Expand Up @@ -502,6 +601,7 @@ func Test_GetOrCreatePort(t *testing.T) {
PropagateUplinkStatus: pointerToTrue,
},
}).Return(&ports.Port{ID: portID1, PropagateUplinkStatus: *pointerToTrue}, nil)
m.ListExtensions().Return([]extensions.Extension{}, nil)
},
&ports.Port{ID: portID1, PropagateUplinkStatus: *pointerToTrue},
false,
Expand Down
15 changes: 15 additions & 0 deletions pkg/cloud/services/networking/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,18 @@ func (s *Service) replaceAllAttributesTags(eventObject runtime.Object, resourceT
record.Eventf(eventObject, "SuccessfulReplaceAllAttributeTags", "Replaced all attributestags for %s with tags %s", resourceID, uniqueTags)
return nil
}

// Checks if neutron supports Standard Attributes Tag Extension
func (s *Service) getStdAttrTagSupport() (bool, error) {
allExts, err := s.client.ListExtensions()
if err != nil {
return false, err
}

for _, ext := range allExts {
if ext.Alias == "standard-attr-tag" {
return true, nil
}
}
return false, nil
}