Port: add propagationUplinkStatus field by winiciusallan · Pull Request #641 · k-orc/openstack-resource-controller

winiciusallan · 2026-01-12T14:42:17Z

This PR introduces the propagationUplinkStatus

https://docs.openstack.org/api-ref/network/v2/index.html#uplink-status-propagation

Notes:

I've decided to keep the default value for that field as false, even if the doc says that the default value is true. The reason is that in environments where this extension is not enabled, we can't update that field and it always returns false, so if we enforce it as true, it may raise an error. So if the user wants to enable it, they must explicitly define it.

Partial #616

winiciusallan · 2026-01-12T22:51:34Z

weird... the uplink_status_propagation extension was not added to the devstack deployment. I tested locally and it only works by setting Q_ML2_PLUGIN_EXT_DRIVERS directly.

Has anyone ever seen something like this?

winiciusallan · 2026-01-13T17:28:42Z

Ok, here we go. I needed to add the neutron plugin on CI so that devstack can run this line and enable uplink-status-propagation accordingly. However, it looks like that Dalmatian release doesn't enable the ability to update propagateUplinkStatus by default on Devstack like did in newer releases, so the CI ended up failing. I believe that adding a line explicitly enabling uplink_status_propagation_updatable extension should work.

About the job failing on Flamingo I really don't get the why. The E2E job has failed in domain-import-error tests.

cc. @mandre I'll wait for you feedback before making any additional change.

mandre · 2026-01-15T15:08:46Z

api/v1alpha1/port_types.go

 	// the port.
 	// +optional
-	PropagateUplinkStatus *bool `json:"propagateUplinkStatus,omitempty"`
+	PropagateUplinkStatus bool `json:"propagateUplinkStatus,omitempty"`


I believe we want to keep the pointer, don't we? This signifies that neutron doesn't have enable the uplink-status-propagation extension.

Yep, we do. I'll change it together with making the field immutable.

mandre · 2026-01-15T15:12:26Z

api/v1alpha1/port_types.go

+	// propagateUplinkStatus represents the uplink status propagation of
+	// the port.
+	// +optional
+	PropagateUplinkStatus *bool `json:"propagateUplinkStatus,omitempty"`


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.

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.

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

mandre · 2026-01-15T15:13:02Z

.github/workflows/e2e.yaml

        enable_workaround_docker_io: 'false'
        branch: ${{ matrix.openstack_version }}
-        enabled_services: "openstack-cli-server"
+        enabled_services: "openstack-cli-server,neutron-uplink-status-propagation"


Interesting, the dalmatian job still failed?

Yep. I don't know the why, but there is two extensions - one for enable the field, and one for make it mutable. In dalmatian, the devstack configuration enable just the first one, so the jobs are failing on port-update test.

For reference, compare these two [1][2]

Since we're making this field immutable, this wouldn't be a problem for now.

[1] https://github.com/openstack/neutron/blob/master/devstack/lib/uplink_status_propagation
[2] https://github.com/openstack/neutron/blob/stable/2024.2/devstack/lib/uplink_status_propagation

Gotcha, thanks for the explanation.

So according to the commit in neutron, openstack/neutron@498be54 we'll need neutron-lib >= v3.16.0, which we don't have in Dalmatian. I believe we'll have to make the field immutable until we drop support for Dalmatian.

Gazpacho will be released in the end of march, so will take a little long to remove Dalmatian from the CI, it worth to merge this with this field as immutable indeed.

I'll re-changed things.

mandre · 2026-01-15T15:35:21Z

About the job failing on Flamingo I really don't get the why. The E2E job has failed in domain-import-error tests.

I re-ran this job and the error is gone. I was most likely a flake (bound to happen as we add more tests).

We have a missing pointer on Gophercloud, this is causing a misbehavior. For workarouding purposes, was added a new struct to support a pointer for that field. Tests were also fixed.

winiciusallan · 2026-01-16T18:06:44Z

@mandre let me know when you think it is ready to go, I can squash the commits to make the PR cleaner. :)

mandre · 2026-01-19T16:35:46Z

internal/osclients/networking.go

 	ports.Port
 	portsecurity.PortSecurityExt
 	portsbinding.PortsBindingExt
+	PortTmpExt //nolint:govet


The nolint marker silenced the error with golangci-lint, however we're still getting an error when running go vet directly:

❯ make vet go vet ./... # github.com/k-orc/openstack-resource-controller/v2/internal/osclients internal/osclients/networking.go:67:2: struct field PropagateUplinkStatusPtr repeats json tag "propagate_uplink_status" also at ../../../../../go/pkg/mod/github.com/gophe rcloud/gophercloud/v2@v2.10.0/openstack/networking/v2/ports/results.go:115 make: *** [Makefile:111: vet] Error 1

I suggest removing the workaround since we don't allow mutability now.

We need the workaround to be able to not return the field if the extension is not enabled.

edit: I didn't find any way to suppress vet warning. Do you have any suggestion?

I think it should work if you add the field at the top level instead of embedding:

Suggested change

PortTmpExt //nolint:govet

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

See this small test.

In 87a3ecf I added the Unmarshal function that we discussed in DM, but I had to change the logic for ListPort because it was not able to unmarshal ports when importing from OpenStack.

Just to be clear, I had a little help from Claude :)

mandre · 2026-03-09T14:57:29Z

api/v1alpha1/port_types.go

+	// propagateUplinkStatus represents the uplink status propagation of
+	// the port.
+	// +optional
+	PropagateUplinkStatus *bool `json:"propagateUplinkStatus,omitempty"`


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.

mandre · 2026-03-09T15:13:22Z

internal/controllers/port/tests/port-create-minimal/00-assert.yaml

    adminStateUp: true
    portSecurityEnabled: true
-    propagateUplinkStatus: false
+    propagateUplinkStatus: true


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.

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.

mandre · 2026-03-09T15:25:22Z

internal/osclients/networking.go

+	PropagateUplinkStatusPtr *bool `json:"propagate_uplink_status,omitempty"`
+}
+
+func (p *PortExt) UnmarshalJSON(b []byte) error {


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?).

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?

A well crafted test should give us the answer.

mandre · 2026-03-09T15:28:11Z

internal/osclients/networking.go

@@ -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] {


We shouldn't have to update ListPort I believe.

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.

port: add propagationUplinkStatus field

6901189

winiciusallan mentioned this pull request Jan 12, 2026

Port controller parity with CAPO #616

Open

7 tasks

github-actions bot added the semver:major Breaking change label Jan 12, 2026

add neutron plugin to the CI

4d21547

winiciusallan force-pushed the port-propagate-uplink-status branch from 8cbef64 to 4d21547 Compare January 13, 2026 16:40

mandre reviewed Jan 15, 2026

View reviewed changes

port: fix update flow for PropagateUplinkStatus

f4abfe6

We have a missing pointer on Gophercloud, this is causing a misbehavior. For workarouding purposes, was added a new struct to support a pointer for that field. Tests were also fixed.

github-actions bot added semver:minor Backwards-compatible change and removed semver:major Breaking change labels Jan 16, 2026

change: make propagateUplinkStatus immutable

7112a07

mandre reviewed Jan 19, 2026

View reviewed changes

remove temp struct and add new Unmarshal logic

87a3ecf

mandre reviewed Mar 9, 2026

View reviewed changes

	PortTmpExt //nolint:govet
	PropagateUplinkStatusPtr *bool `json:"propagate_uplink_status,omitempty"`

		@@ -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] {

Conversation

winiciusallan commented Jan 12, 2026

Uh oh!

winiciusallan commented Jan 12, 2026

Uh oh!

winiciusallan commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mandre commented Jan 15, 2026

Uh oh!

winiciusallan commented Jan 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winiciusallan Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winiciusallan Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winiciusallan Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

winiciusallan commented Jan 13, 2026 •

edited

Loading

winiciusallan Jan 19, 2026 •

edited

Loading

winiciusallan Feb 3, 2026 •

edited

Loading

winiciusallan Mar 10, 2026 •

edited

Loading