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.

edit:

If the extension is enabled in the environment, the default value for this field is the Openstack default value, which is true. Otherwise, it'll be nil

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:12:26Z

+	// 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: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).

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-03-09T14:57:29Z

+	// 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:25:22Z

+	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.

Sorry for the late response.

The idea of this test is to show that using ports.Get(...).ExtractInto(&p), the fields are populated even though they're not explicitly set in the UnmarshalJSON function, as you highlighted in your initial comment, unlike using json.Unmarshal directly, which calls the custom Unmarshal and ignores the PortSecurityEnabled and PortBindings.

https://go.dev/play/p/iUMKGSjAFoo

After understanding more about how the parse actually works, I've decided to add PortSecurityExt and PortsBindingExt to the Unmarshal function.

This helps us especially when listing ports because we are not using the default Gophercloud's Extract mechanisms, and we do need to populate fields using the unmarshaler function.

We're now missing PortTrustedVIFExt as well 😢

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

@@ -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.

After discussion, we found a potential bug in Gophercloud, and a PR has been created. More information is there.

gophercloud/gophercloud#3726

So, do we also need the same thing for GetPort (and potentially CreatePort and UpdatePort)? It looks to me like we should wait for a new gophercloud that includes your fix.

As long as I can see, we don't need. For Get, Create, and Update Port, all of them pass a struct to the ExtractInto function, making the function pass through this path. At the end of execution, the unmarshaling of non-struct field is handled by the err = json.Unmarshal(b, &to).

I also don't like the way it is, so it is ok for me to wait for a new release and get it properly fixed. Wdyt?

That would be my preference as well.

winiciusallan · 2026-04-14T00:35:05Z

@mandre I'm working again to have this PR merged. A lot of work has been done since the last commit, so I'll rebase and start addressing the comments.

winiciusallan · 2026-04-23T17:45:29Z

@mandre let me know when you think it is good. I'd like to squash the commits. keeping the history clear 🤌

mandre · 2026-05-04T15:01:52Z

@winiciusallan Just looked at your changes again. They look good to me now. Would you like to do the rebase yourself, to fix the conflicts and squash what needs squashing, or would you like me to take care of it?

winiciusallan · 2026-05-04T15:10:07Z

@mandre I will do this, thanks for asking!

mandre · 2026-05-11T13:35:53Z

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


We're now missing PortTrustedVIFExt as well 😢

mandre · 2026-05-11T13:37:11Z

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


May I suggest we add a comment to drop this custom unmarshaller once we're on the next major version of gophercloud (v3). The function is going to be difficult to maintain, and I'd like to drop it at the first occasion.

Sure! Will add it.

mandre · 2026-05-11T15:23:11Z

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


So, do we also need the same thing for GetPort (and potentially CreatePort and UpdatePort)? It looks to me like we should wait for a new gophercloud that includes your fix.

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

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

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

mandre reviewed Jan 19, 2026

View reviewed changes

Comment thread internal/osclients/networking.go Outdated

mandre reviewed Mar 9, 2026

View reviewed changes

winiciusallan force-pushed the port-propagate-uplink-status branch 2 times, most recently from 0f1e900 to 60786e3 Compare April 23, 2026 17:03

port: add propagationUplinkStatus field

1749c64

winiciusallan force-pushed the port-propagate-uplink-status branch from 60786e3 to 1749c64 Compare May 4, 2026 16:05

mandre reviewed May 11, 2026

View reviewed changes

		@@ -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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Uh oh!

mandre commented Jan 15, 2026

Uh oh!

winiciusallan commented Jan 16, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

winiciusallan Apr 23, 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!

winiciusallan May 11, 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 commented Apr 14, 2026

Uh oh!

winiciusallan commented Apr 23, 2026

Uh oh!

mandre commented May 4, 2026

Uh oh!

winiciusallan commented May 4, 2026

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 12, 2026 •

edited

Loading

winiciusallan commented Jan 13, 2026 •

edited

Loading

winiciusallan Apr 23, 2026 •

edited

Loading

winiciusallan May 11, 2026 •

edited

Loading