Add Trunk resource controller implementation #581

aldokimi · 2025-11-22T17:43:35Z

Summary

Adds complete Trunk resource controller implementation with subport implementation (which is necessary for the Trunk type), dependency handling, and comprehensive test coverage.

Key Features

Full CRUD operations for OpenStack trunks
Subport reconciliation (add/remove/update)
Dependency management (ports, projects)
Resource adoption and import support
Comprehensive test coverage

Testing

Unit tests for all actuator methods
Test coverage 35.7%
All tests passing

E2E Testing

-All resources are created in the K8s side
-All resources are created in the Openstack side
-All CRUD functionalities are passing the tests

Checklist

aldokimi · 2025-11-22T17:47:19Z

This fixes #364 which is mentioned as well in #577

winiciusallan

Hi, @aldokimi! I've left a few comments in the API types. Thanks for submitting this pull request, it will be very important in achieving parity with CAPO.

api/v1alpha1/trunk_type.go

winiciusallan · 2025-11-25T19:26:51Z

api/v1alpha1/trunk_type.go

+type Subport struct {
+	// portRef is a reference to the ORC Port which will be used as a subport.
+	// +required
+	PortRef KubernetesNameRef `json:"portRef"`


Suggested change

PortRef KubernetesNameRef `json:"portRef"`

PortRef KubernetesNameRef `json:"portRef,omitempty"`

winiciusallan · 2025-11-25T19:54:30Z

api/v1alpha1/trunk_type.go

+	PortID string `json:"portID,omitempty"`
+
+	// segmentationType is the type of segmentation used.
+	// +kubebuilder:validation:MaxLength=1024


I believe this should match the same validation in spec for this field.

It's OK if it's higher in the Status than in the Spec (the opposite is not OK).

api/v1alpha1/trunk_type.go

mandre

I suspect you haven't used the scaffolding tool. If it's not too much trouble, would it be possible for you to re-work your controller to use it? It ensures consistency with the other controllers and makes it much easier for reviewers to see your modifications, since there's typically a lot of boilerplate code in PRs for new controllers. See #568 (comment) for the rational. I'm currently working on updating the developers doc to make it more obvious.

I bet it was a lot of work to contribute this controller without the help of the scaffolding tool, so if that turns out to be too complicated to rework your change to make use of it, I'm ready to make an exception.

FYI, here's how I expect us to run the scaffolding tool, based on what the resource supports:

go run ./cmd/scaffold-controller \
        -interactive=false \
        -kind Trunk \
        -gophercloud-client NewNetworkV2 \
        -gophercloud-module github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/trunks \
        -import-dependency Port \
        -import-dependency Project \
        -optional-create-dependency Project \
        -required-create-dependency Port

api/v1alpha1/trunk_type.go

mandre · 2025-12-01T15:59:05Z

api/v1alpha1/trunk_type.go

+	PortID string `json:"portID,omitempty"`
+
+	// segmentationType is the type of segmentation used.
+	// +kubebuilder:validation:MaxLength=1024


It's OK if it's higher in the Status than in the Spec (the opposite is not OK).

api/v1alpha1/trunk_type.go

github-actions · 2025-12-07T15:16:13Z

Failed to assess the semver bump. See logs for details.

winiciusallan

Hey @aldokimi, I've left more comments. This kind of PR tends to be very large, so I'll need to take another look. But for now, thanks for addressing the comments. =D

api/v1alpha1/trunk_type.go

winiciusallan · 2025-12-07T18:57:26Z

cmd/resource-generator/main.go

 	},
+	{
+		Name:             "Trunk",
+		ExistingOSClient: true,


Could you please remove this field? This uses an existing client, but we want to separate clients for each controller.

You may remove and run make generate. Maybe do you need to refactor other parts along the code.

But I don't really understand why removing this field is necessary, could elaborate more please?

Good question. To be honest, I don't know the exactly reason. I believe @mandre can explain it better.

My guess is, if you need some extra logic for a specific client, you won't impact the others implemented in the same interface. As I said, is just I guess, so don't take it as the truth 😅.

config/samples/openstack_v1alpha1_trunk.yaml

api/v1alpha1/trunk_type.go

internal/controllers/trunk/actuator.go

winiciusallan · 2025-12-08T20:33:32Z

internal/controllers/trunk/actuator.go

+		return nil
+	}
+
+	updateOpts.RevisionNumber = &osResource.RevisionNumber


May the revision number be nil, or does the API always returns a valid value?

internal/controllers/trunk/actuator.go

winiciusallan · 2025-12-08T20:42:05Z

internal/controllers/trunk/actuator.go

+			yield(nil, err)
+		}, nil
+	}
+	return func(yield func(*osResourceT, error) bool) {


Same about the comment on ListForAdoption.

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

winiciusallan · 2025-12-19T17:18:06Z

@aldokimi I may be a little busy (resting...) in the next few days, so maybe I'll take a little while to review your commits, but at the start of the year, I should take a look into it.

Thanks for contributing, we really appreciate it. =D

winiciusallan · 2025-12-22T13:09:13Z

Hey @aldokimi, I had some time to take a look into your PR.

It looks like you have overrided your old implementation for Trunk types with the code generated by the scaffolding tool. Are you still working on this? Also, I saw that you changed a lot of tests files from other controllers, I believe we don't want to touch another controllers on this pull request. Do you have made this changes for a specific reason?

If you are still working on this, you can ignore my comment. Let me know when you believe it is ready for another review :)

aldokimi · 2025-12-22T15:46:59Z

Hey @aldokimi, I had some time to take a look into your PR.

It looks like you have overrided your old implementation for Trunk types with the code generated by the scaffolding tool. Are you still working on this? Also, I saw that you changed a lot of tests files from other controllers, I believe we don't want to touch another controllers on this pull request. Do you have made this changes for a specific reason?

If you are still working on this, you can ignore my comment. Let me know when you believe it is ready for another review :)

Hello @winiciusallan , I am still going to upload a last commit, the changes on the test files where because of the scaffolding tool, my last commit will be about fixing the issues regarding the scaffolding and then we will be ready for a new review!

Thank you for your comment, happy holidays!

aldokimi · 2025-12-28T03:32:32Z

Hello @winiciusallan and @mandre , happy new year!

So, I am sorry for this mess, this is the biggest PR of my life xD
I think now the PR is ready for a new review, if needed I am down for a code review session!

Thanks,

winiciusallan

Hey @aldokimi, thanks for taking time to work on it.

I have left a few comments, especially on the API part. This particular part is good overall. Let's see if Martin has comments to make.

I feel that it is missing some logic on the actuator file to handle import, creation and the update of the resource. I avoided adding repetitive comments inline to not make the review cluttered. Are you probably still working on it? If you need to discuss something, I'm online on kubernetes slack workspace, or on the #gophercloud channel.

Happy new year for you too =) 🎆

winiciusallan · 2025-12-28T05:05:49Z

Makefile

 GOLANGCI_LINT_VERSION ?= v2.7.2
 KAL_VERSION ?= v0.0.0-20250924094418-502783c08f9d
-MOCKGEN_VERSION ?= v0.5.0
+MOCKGEN_VERSION ?= v0.6.0


Martin could give better feedback here, but I believe we should bump mockgen version on a separate PR to better track the changes, if it is really necessary.

Well, in this case we will need this separate PR to go in before this one, because Mokcing fails if the version is 0.6.0.

Can you explain how mockgen fails with v0.5.0? I've tried setting it back to v0.5.0 in a checkout from your branch and didn't see any errors when running make generate-go. What am I missing?

I don't see a problem bumping a library, but ideally we would do that in a separate PR.

winiciusallan · 2025-12-28T06:00:59Z

api/v1alpha1/trunk_types.go

+	// segmentationType is the segmentation type for the subport (e.g. vlan).
+	// +required
+	// +kubebuilder:validation:MinLength:=1
+	// +kubebuilder:validation:MaxLength:=255


Let's sticky the maximum length to 32 bytes as the source code defines.

If we set it to 255, the user can pass any value, so the openstack may return a fail code whe trying to get or create the resource. If we stick to the maximum length permitted by the API, we avoid making this unecessary request to the OpenStack API.

Other suggestion is use enumeration, since the only two options are inherit and vlan. We have an example here.

winiciusallan · 2025-12-28T06:17:42Z

api/v1alpha1/trunk_types.go

+	// status indicates whether the trunk is currently operational. Possible values include
+	// `ACTIVE', `DOWN', `BUILD', `DEGRADED' or `ERROR'. Plug-ins might define additional values.
+	// +optional
+	Status *string `json:"status,omitempty"`


Could you please set a maximum length here?

When you don't specify, CEL works by calculating the worst case for that validation, so we always try to set this value, even if it is an estimate. Here it is a talk that explore more about it (thanks Martin).

winiciusallan · 2025-12-28T06:20:02Z

api/v1alpha1/trunk_types.go

+	// tenantID is the project owner of the trunk (alias of projectID in some deployments).
+	// +kubebuilder:validation:MaxLength=1024
+	// +optional
+	TenantID string `json:"tenantID,omitempty"`


Although tenant_id is for old Keystone APIs, I believe it won't gonna hurt to have it here.

winiciusallan · 2025-12-28T06:22:34Z

api/v1alpha1/trunk_types.go

+
+	// tags is a list of Neutron tags to apply to the trunk.
+	//
+	// NOTE: ORC does not currently reconcile tag updates for Trunk.


Could you explain better why? Is it because the networking API does not support updating the tags for the Trunk port?

If so, I believe it is worth having it annotated here.

So basically, I thought that it will be a different approach to update the tags (than a normal port), but I am wrong, I will add the implementation to reconcile tags!

winiciusallan · 2025-12-28T06:25:06Z

internal/controllers/trunk/actuator.go

+	// TODO(scaffolding) If you need to filter resources on fields that the List() function
+	// of gophercloud does not support, it's possible to perform client-side filtering.
+	// Check osclients.ResourceFilter


Maybe you forgot to delete it? There is others "TODO" comments along this same file.

winiciusallan · 2025-12-28T06:28:42Z

internal/controllers/trunk/actuator.go

+		Description: string(ptr.Deref(filter.Description, "")),
+		PortID:    ptr.Deref(port.Status.ID, ""),
+		ProjectID: ptr.Deref(project.Status.ID, ""),
+		// TODO(scaffolding): Add more import filters


I believe we can add more fields as you declared on types. Status and AdminStateUp are missing. Could you add it please?

I will add the AdminStateUp , but the Status is a read-only so I will skip it

mandre

Could you also add the neutron-trunk service to our devstack config so that we can test this change in CI?

mandre · 2026-01-06T07:30:51Z

Makefile

 GOLANGCI_LINT_VERSION ?= v2.7.2
 KAL_VERSION ?= v0.0.0-20250924094418-502783c08f9d
-MOCKGEN_VERSION ?= v0.5.0
+MOCKGEN_VERSION ?= v0.6.0


Can you explain how mockgen fails with v0.5.0? I've tried setting it back to v0.5.0 in a checkout from your branch and didn't see any errors when running make generate-go. What am I missing?

I don't see a problem bumping a library, but ideally we would do that in a separate PR.

mandre · 2026-01-07T11:25:58Z

internal/controllers/trunk/controller.go

+		}
+		return portRefs
+	},
+	finalizer, externalObjectFieldOwner,


You'll want to use a different finalizer here, for example orcstrings.GetFinalizerName("trunk-subport") to differentiate with the finalizer used for the parent port.

You are right, I am not sure what problems I encountered earlier when I started the PR, but things now look okay, so I will be removing the MOCKGEN_VERSION update.
Regarding creating a new Finalizer name, this fixed the issue!

mandre · 2026-01-07T11:27:59Z

internal/controllers/trunk/tests/trunk-dependency/00-create-resources-missing-deps.yaml

+  name: trunk-dependency-no-project
+spec:
+  cloudCredentialsRef:
+    cloudName: openstack


You'll need elevated privilege to create a trunk within a specific project:

Suggested change

cloudName: openstack

cloudName: openstack-admin

Are you sure? The policy reference says that he can create the trunk if he's a project member

I can't link the rule, but it is create_trunk https://docs.openstack.org/neutron/latest/configuration/policy.html

mandre · 2026-01-07T11:28:28Z

internal/controllers/trunk/tests/trunk-dependency/01-create-dependencies.yaml

+  name: trunk-dependency
+spec:
+  cloudCredentialsRef:
+    cloudName: openstack


Suggested change

cloudName: openstack

cloudName: openstack-admin

github-actions · 2026-01-08T16:06:36Z

Failed to assess the semver bump. See logs for details.

winiciusallan

I left a few nit comments -- for those, I think they wouldn't block you PR, so it is up to you.

There is other comments that need more attention.

Some points on testing:

We use the xx-assert.yaml files to validate the expected status of the resource, so having both the messages and the resource fields there helps the tests work as expected.
We recommend to use CEL expressions when you don't know the field value upfront, so you can make elaborate validation with it if necessary.

winiciusallan · 2026-01-09T13:35:54Z

internal/controllers/trunk/actuator.go

+	return actuator.osClient.ListTrunks(ctx, listOpts), true
+}
+
+func (actuator trunkActuator) ListOSResourcesForImport(ctx context.Context, obj orcObjectPT, filter filterT) (iter.Seq2[*osResourceT, error], progress.ReconcileStatus) {


nit: we can refactor later these dependencies to use the FetchDependency helper implemented in #621

maybe in another PR, since it doesn't impact the controller functionality.

winiciusallan · 2026-01-09T13:40:29Z

internal/controllers/trunk/actuator.go

+		NotTagsAny:   tags.Join(filter.NotTagsAny),
+	}
+	if filter.Status != nil {
+		listOpts.Status = *filter.Status
+	}


nit: you may can refactor this part

Suggested change

NotTagsAny: tags.Join(filter.NotTagsAny),

}

if filter.Status != nil {

listOpts.Status = *filter.Status

}

NotTagsAny: tags.Join(filter.NotTagsAny),

Status: ptr.Deref(filter.Status, ""),

}

winiciusallan · 2026-01-09T13:51:27Z

internal/controllers/trunk/actuator.go

+	}
+
+	// Build actual subports map: portID -> subport
+	actualSubports := make(map[string]trunks.Subport)


nit: you can set a length for the subport port here, so the runtime allocates the necessary memory upfront.

Suggested change

actualSubports := make(map[string]trunks.Subport)

actualSubports := make(map[string]trunks.Subport, osResource.Subports)

winiciusallan · 2026-01-09T13:57:43Z

internal/controllers/trunk/actuator_test.go

+			if len(subportsToAdd) != tt.expectedToAdd {
+				t.Errorf("Expected %d subports to add, got %d", tt.expectedToAdd, len(subportsToAdd))
+			}
+			if len(subportsToRemove) != tt.expectedToRemove {
+				t.Errorf("Expected %d subports to remove, got %d", tt.expectedToRemove, len(subportsToRemove))
+			}


For better consistency, wouldn't it be better to use some AssertEquals function to compare if subports to add/remove are as expected, instead of comparing the number of subports? What do you think?

winiciusallan · 2026-01-09T14:05:28Z

internal/osclients/mock/networking.go

I believe that the changes on this file should be in the dedicated file internal/osclients/mock/trunk.go, likely a result of the resource-generator.

I thought the same initially but then I changed my mind. The reason being that:

all of the networking client calls are already in the same file

more importantly, we need a reference over the network client anyway for the management of the tags, and if we can avoid to keep a reference to 2 clients in the trunk controller, that's better.

I suggest deleting the internal/osclients/trunk.go and using the NetworkClient instead of a dedicated TrunkClient.

winiciusallan · 2026-01-09T14:11:04Z

internal/controllers/trunk/tests/trunk-dependency/00-create-resources-missing-deps.yaml

+  name: trunk-dependency-no-project
+spec:
+  cloudCredentialsRef:
+    cloudName: openstack


Are you sure? The policy reference says that he can create the trunk if he's a project member

I can't link the rule, but it is create_trunk https://docs.openstack.org/neutron/latest/configuration/policy.html

winiciusallan · 2026-01-09T14:15:30Z

internal/controllers/trunk/tests/trunk-create-full/00-create-resource.yaml

+    cloudName: openstack
+    secretName: openstack-clouds
+  managementPolicy: managed
+  resource:


We usually add all the available fields in the spec on create-full scenario, so I think it would be good to have adminStateUp and []tags here

winiciusallan · 2026-01-09T14:26:40Z

internal/controllers/trunk/tests/trunk-create-full/00-assert.yaml

+    - celExpr: "trunk.status.resource.subports.size() == 2"
+    - celExpr: "trunk.status.resource.subports.exists(s, s.portID == subport1.status.id && s.segmentationID == 100 && s.segmentationType == 'vlan')"
+    - celExpr: "trunk.status.resource.subports.exists(s, s.portID == subport2.status.id && s.segmentationID == 200 && s.segmentationType == 'vlan')"


Instead of using CEL expressions to validate the resource status, you can fill in the trunk status with what you are expecting, and KUTTL will match the declared status with the actual status.

An example: https://github.com/k-orc/openstack-resource-controller/blob/main/internal/controllers/network/tests/network-create-full/00-assert.yaml

winiciusallan · 2026-01-09T14:34:15Z

internal/controllers/trunk/tests/trunk-import/00-assert.yaml

+    - type: Available
+      message: Waiting for Port/trunk-import to be created
+      status: "False"
+      reason: Progressing
+    - type: Progressing
+      message: Waiting for Port/trunk-import to be created
+      status: "True"
+      reason: Progressing


It is better if we don't rely on a transitional state here, we are already testing it in trunk-import-dependency, so I believe you can create the dependency on 00-import-resource.yaml and use it.

For better context: #535 (comment)

winiciusallan · 2026-01-09T14:40:29Z

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

+assertAll:
+    - celExpr: "trunk.status.id != ''"
+    - celExpr: "trunk.status.resource.portID == port.status.id"


Suggested change

assertAll:

- celExpr: "trunk.status.id != ''"

- celExpr: "trunk.status.resource.portID == port.status.id"

assertAll:

- celExpr: "trunk.status.id != ''"

- celExpr: "trunk.status.resource.portID == port.status.id"

- celExpr: "!has(trunk.status.resource.description)"

github-actions bot added the semver:major Breaking change label Nov 22, 2025

mandre mentioned this pull request Nov 24, 2025

Migrate from CAPO Internal OpenStackServer Controller to External ORC Controller kubernetes-sigs/cluster-api-provider-openstack#2814

Open

winiciusallan reviewed Nov 25, 2025

View reviewed changes

mandre reviewed Dec 1, 2025

View reviewed changes

aldokimi force-pushed the dev/trunk_type branch from ae0d632 to b1f5243 Compare December 7, 2025 15:15

github-actions bot removed the semver:major Breaking change label Dec 7, 2025

winiciusallan reviewed Dec 8, 2025

View reviewed changes

winiciusallan mentioned this pull request Dec 18, 2025

📖 proposal: add support for defining trunk subports kubernetes-sigs/cluster-api-provider-openstack#2862

Closed

aldokimi force-pushed the dev/trunk_type branch from b1f5243 to 960bd52 Compare December 18, 2025 23:48

github-actions bot added the semver:major Breaking change label Dec 18, 2025

aldokimi force-pushed the dev/trunk_type branch from 710a752 to 545a5fa Compare December 28, 2025 03:26

winiciusallan reviewed Dec 28, 2025

View reviewed changes

mandre reviewed Jan 7, 2026

View reviewed changes

github-actions bot removed the semver:major Breaking change label Jan 8, 2026

aldokimi added 9 commits January 8, 2026 16:14

Trunk resource: add Trunk resource controller implementation

7c84bf2

Trunk resource: auto-generated files + fixing some bugs

6150f8c

Trunk resource: increase test coverage + fix bugs

1acb6e5

Trunk resource: add improvments + fix bugs

81998d1

Trunk resource: fixed testing

c81b739

Trunk type: fix scaffolding issues + minor bugs

af0d4a5

Trunk type: Added tags reconsiling + fix some bugs

a8d9fbc

Trunk type: Added subports list reconsiling

7d6bd28

Trunk type: fix PortRef pointer problem

b723cdf

Trunk type: fixed finalizer issue + minor bugs

82e2ff6

aldokimi force-pushed the dev/trunk_type branch from dacf651 to 82e2ff6 Compare January 8, 2026 16:21

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

Trunk type: fixed CI testing issues

9e54e53

winiciusallan reviewed Jan 9, 2026

View reviewed changes

	PortRef KubernetesNameRef `json:"portRef"`
	PortRef KubernetesNameRef `json:"portRef,omitempty"`

	actualSubports := make(map[string]trunks.Subport)
	actualSubports := make(map[string]trunks.Subport, osResource.Subports)

Add Trunk resource controller implementation #581

Are you sure you want to change the base?

Add Trunk resource controller implementation #581

Conversation

aldokimi commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key Features

Testing

E2E Testing

Checklist

Uh oh!

aldokimi commented Nov 22, 2025

Uh oh!

winiciusallan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mandre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 7, 2025

Uh oh!

winiciusallan left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

winiciusallan commented Dec 22, 2025

Uh oh!

aldokimi commented Dec 22, 2025

Uh oh!

aldokimi commented Dec 28, 2025

Uh oh!

winiciusallan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aldokimi commented Nov 22, 2025 •

edited

Loading

winiciusallan commented Dec 19, 2025 •

edited

Loading