Skip to content

Port: add propagationUplinkStatus field #641

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
winiciusallan wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Port: add propagationUplinkStatus field #641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6901189
port: add propagationUplinkStatus field
winiciusallan Jan 12, 2026
4d21547
add neutron plugin to the CI
winiciusallan Jan 13, 2026
f4abfe6
port: fix update flow for PropagateUplinkStatus
winiciusallan Jan 16, 2026
7112a07
change: make propagateUplinkStatus immutable
winiciusallan Jan 16, 2026
87a3ecf
remove temp struct and add new Unmarshal logic
winiciusallan Feb 3, 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
4 changes: 3 additions & 1 deletion .github/workflows/e2e.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ jobs:
with:
enable_workaround_docker_io: 'false'
branch: ${{ matrix.openstack_version }}
enabled_services: "openstack-cli-server"
enabled_services: "openstack-cli-server,neutron-uplink-status-propagation"
conf_overrides: |
enable_plugin neutron https://opendev.org/openstack/neutron.git ${{ matrix.openstack_version }}

- name: Deploy a Kind Cluster
uses: helm/kind-action@92086f6be054225fa813e0a4b13787fc9088faab
Expand Down
6 changes: 6 additions & 0 deletions api/v1alpha1/port_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,12 @@ type PortResourceSpec struct {
// +kubebuilder:validation:MaxLength=36
// +optional
HostID string `json:"hostID,omitempty"`

// propagateUplinkStatus represents the uplink status propagation of
// the port.
// +optional
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="propagateUplinkStatus is immutable"
PropagateUplinkStatus *bool `json:"propagateUplinkStatus,omitempty"`
Copy link
Collaborator

@mandre mandre Jan 15, 2026

Choose a reason for hiding this comment

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

We've discussed this privately already, after you brought the issue to my attention: gophercloud implements the PropagateUplinkStatus in the response Port structure as a bool while it should really be a *bool with omitempty, and because of this we can't reliably know the status for PropagateUplinkStatus. I suggest that until the issue if fixed in gophercloud, we make that field immutable.

Copy link
Collaborator

@mandre mandre Mar 9, 2026

Choose a reason for hiding this comment

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

It's been a while and I don't remember all of the discussion. From reading the PR comments again, I believe we're past the gophercloud limitation (thanks to custom unmarshalling) but we still need to make the field immutable because dalmatien doesn't support updating the field. If that's effectively the case, we should add that as a comment so we know when we can change that behavior.

Copy link
Contributor Author

@winiciusallan winiciusallan Mar 10, 2026

Choose a reason for hiding this comment

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

Yes, you're absolutely right. I'll add this comment.

}

type PortResourceStatus struct {
Expand Down
5 changes: 5 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions cmd/models-schema/zz_generated.openapi.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions config/crd/bases/openstack.k-orc.cloud_ports.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,14 @@ spec:
x-kubernetes-validations:
- message: projectRef is immutable
rule: self == oldSelf
propagateUplinkStatus:
description: |-
propagateUplinkStatus represents the uplink status propagation of
the port.
type: boolean
x-kubernetes-validations:
- message: propagateUplinkStatus is immutable
rule: self == oldSelf
securityGroupRefs:
description: |-
securityGroupRefs are the names of the security groups associated
Expand Down
13 changes: 7 additions & 6 deletions internal/controllers/port/actuator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,12 +171,13 @@ func (actuator portActuator) CreateResource(ctx context.Context, obj *orcv1alpha
}

createOpts := ports.CreateOpts{
NetworkID: *network.Status.ID,
Name: getResourceName(obj),
Description: string(ptr.Deref(resource.Description, "")),
ProjectID: projectID,
AdminStateUp: resource.AdminStateUp,
MACAddress: resource.MACAddress,
NetworkID: *network.Status.ID,
Name: getResourceName(obj),
Description: string(ptr.Deref(resource.Description, "")),
ProjectID: projectID,
AdminStateUp: resource.AdminStateUp,
MACAddress: resource.MACAddress,
PropagateUplinkStatus: resource.PropagateUplinkStatus,
}

if len(resource.AllowedAddressPairs) > 0 {
Expand Down
5 changes: 4 additions & 1 deletion internal/controllers/port/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ func (portStatusWriter) ApplyResourceStatus(log logr.Logger, osResource *osResou
WithNetworkID(osResource.NetworkID).
WithTags(osResource.Tags...).
WithSecurityGroups(osResource.SecurityGroups...).
WithPropagateUplinkStatus(osResource.PropagateUplinkStatus).
WithVNICType(osResource.VNICType).
WithPortSecurityEnabled(osResource.PortSecurityEnabled).
WithRevisionNumber(int64(osResource.RevisionNumber)).
Expand Down Expand Up @@ -104,5 +103,9 @@ func (portStatusWriter) ApplyResourceStatus(log logr.Logger, osResource *osResou
resourceStatus.WithFixedIPs(fixedIPs...)
}

if osResource.PropagateUplinkStatusPtr != nil {
resourceStatus.WithPropagateUplinkStatus(*osResource.PropagateUplinkStatusPtr)
}

statusApply.WithResource(resourceStatus)
}
1 change: 1 addition & 0 deletions internal/controllers/port/tests/port-create-full/00-create-resource.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,4 @@ spec:
projectRef: port-create-full
macAddress: fa:16:3e:23:fd:d7
hostID: devstack
propagateUplinkStatus: false
2 changes: 1 addition & 1 deletion internal/controllers/port/tests/port-create-minimal/00-assert.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ status:
name: port-create-minimal
adminStateUp: true
portSecurityEnabled: true
propagateUplinkStatus: false
propagateUplinkStatus: true
Copy link
Collaborator

@mandre mandre Mar 9, 2026

Choose a reason for hiding this comment

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

So the default is still true, contrary to what the PR description says. The value changed from false to true in this test because we've enabled the neutron-uplink-status-propagation extension in devstack.

Copy link
Contributor Author

@winiciusallan winiciusallan Mar 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. Since we enabled the extension and added the custom struct with the field as a pointer, we're now able to have this value as nil without the extension, and as true (default) if enabled.

I'll change the PR description to avoid confusion.

revisionNumber: 1
status: DOWN
vnicType: normal
Expand Down
4 changes: 2 additions & 2 deletions internal/controllers/port/tests/port-create-sriov/00-assert.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ status:
description: Port from "create sriov" test
adminStateUp: true
portSecurityEnabled: false
propagateUplinkStatus: false
propagateUplinkStatus: true
status: DOWN
vnicType: direct
tags:
Expand All @@ -35,4 +35,4 @@ assertAll:
- celExpr: "port.status.resource.fixedIPs[0].subnetID == subnet.status.id"
- celExpr: "port.status.resource.fixedIPs[0].ip == '192.168.155.122'"
- celExpr: "!has(port.status.resource.allowedAddressPairs)"
- celExpr: "!has(port.status.resource.securityGroups)"
- celExpr: "!has(port.status.resource.securityGroups)"
4 changes: 2 additions & 2 deletions internal/controllers/port/tests/port-update/00-assert.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ status:
name: port-update
adminStateUp: true
portSecurityEnabled: false
propagateUplinkStatus: false
propagateUplinkStatus: true
revisionNumber: 1
status: DOWN
vnicType: normal
Expand All @@ -53,7 +53,7 @@ status:
name: port-update-admin
adminStateUp: true
portSecurityEnabled: true
propagateUplinkStatus: false
propagateUplinkStatus: true
revisionNumber: 1
status: DOWN
vnicType: normal
Expand Down
3 changes: 1 addition & 2 deletions internal/controllers/port/tests/port-update/01-assert.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ status:
description: port-update-updated
adminStateUp: true
portSecurityEnabled: true
propagateUplinkStatus: false
status: DOWN
vnicType: direct
allowedAddressPairs:
Expand Down Expand Up @@ -63,4 +62,4 @@ status:
reason: Success
- type: Progressing
status: "False"
reason: Success
reason: Success
4 changes: 2 additions & 2 deletions internal/controllers/port/tests/port-update/02-assert.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ status:
name: port-update
adminStateUp: true
portSecurityEnabled: false
propagateUplinkStatus: false
propagateUplinkStatus: true
status: DOWN
vnicType: normal
conditions:
Expand All @@ -35,4 +35,4 @@ status:
- type: Progressing
message: OpenStack resource is up to date
status: "False"
reason: Success
reason: Success
40 changes: 38 additions & 2 deletions internal/osclients/networking.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package osclients

import (
"context"
"encoding/json"
"fmt"
"iter"

Expand Down Expand Up @@ -56,6 +57,24 @@ type PortExt struct {
ports.Port
portsecurity.PortSecurityExt
portsbinding.PortsBindingExt

PropagateUplinkStatusPtr *bool `json:"propagate_uplink_status,omitempty"`
}

func (p *PortExt) UnmarshalJSON(b []byte) error {
Copy link
Collaborator

@mandre mandre Mar 9, 2026

Choose a reason for hiding this comment

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

This custom unmarshaller omits portsecurity.PortSecurityExt or portsbinding.PortsBindingExt meaning that they'll always have the zero value (it's weird that we do not detect this bug via the tests, don't we have coverage for those fields?).

Copy link
Contributor Author

@winiciusallan winiciusallan Mar 10, 2026

Choose a reason for hiding this comment

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

It is something that I was in doubt about... I'm not sure, and if you can correct me if I'm wrong, I'll appreciate it, but it looks like Gophercloud uses reflection to populate the result rather than simply using a json.Unmarshal, am I wrong?

https://github.com/gophercloud/gophercloud/blob/main/results.go#L70

You're right that they will always have the zero value, but maybe in the above phase, the fields are populated using reflection. I need to better understand this. Any initial thought?

Copy link
Collaborator

@mandre mandre Mar 10, 2026

Choose a reason for hiding this comment

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

A well crafted test should give us the answer.

if err := json.Unmarshal(b, &p.Port); err != nil {
return err
}

var tmp struct {
PropagateUplinkStatusPtr *bool `json:"propagate_uplink_status"`
}
if err := json.Unmarshal(b, &tmp); err != nil {
return err
}

p.PropagateUplinkStatusPtr = tmp.PropagateUplinkStatusPtr
return nil
}

type NetworkClient interface {
Expand Down Expand Up @@ -172,13 +191,30 @@ func (c networkClient) UpdateFloatingIP(ctx context.Context, id string, opts flo

func (c networkClient) ListPort(ctx context.Context, opts ports.ListOptsBuilder) iter.Seq2[*PortExt, error] {
Copy link
Collaborator

@mandre mandre Mar 9, 2026

Choose a reason for hiding this comment

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

We shouldn't have to update ListPort I believe.

Copy link
Contributor Author

@winiciusallan winiciusallan Mar 10, 2026

Choose a reason for hiding this comment

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

IIRC, there was a failing test, and the solution was to change the ListPort function, but I'll double-check this to confirm what exactly the error was.

extractPortExt := func(p pagination.Page) ([]PortExt, error) {
var resources []PortExt
err := ports.ExtractPortsInto(p, &resources)
bodyMap, ok := p.(ports.PortPage).Body.(map[string]interface{})
if !ok {
return nil, fmt.Errorf("unexpected body type: %T", p.(ports.PortPage).Body)
}

portsData, ok := bodyMap["ports"]
if !ok {
return nil, fmt.Errorf("ports key not found in response")
}

// Marshal and unmarshal to trigger UnmarshalJSON
jsonData, err := json.Marshal(portsData)
if err != nil {
return nil, err
}

var resources []PortExt
if err := json.Unmarshal(jsonData, &resources); err != nil {
return nil, err
}

return resources, nil
}

pager := ports.List(c.serviceClient, opts)
return func(yield func(*PortExt, error) bool) {
_ = pager.EachPage(ctx, yieldPage(extractPortExt, yield))
Expand Down
35 changes: 22 additions & 13 deletions pkg/clients/applyconfiguration/api/v1alpha1/portresourcespec.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions pkg/clients/applyconfiguration/internal/internal.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions website/docs/crd-reference.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2172,6 +2172,7 @@ _Appears in:_
| `projectRef` _[KubernetesNameRef](#kubernetesnameref)_ | projectRef is a reference to the ORC Project this resource is associated with.<br />Typically, only used by admin. | | MaxLength: 253 <br />MinLength: 1 <br /> |
| `macAddress` _string_ | macAddress is the MAC address of the port. | | MaxLength: 32 <br /> |
| `hostID` _string_ | hostID is the ID of host where the port resides. | | MaxLength: 36 <br /> |
| `propagateUplinkStatus` _boolean_ | propagateUplinkStatus represents the uplink status propagation of<br />the port. | | |


#### PortResourceStatus
Expand Down