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.

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.

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?

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

aldokimi added 4 commits December 17, 2025 13:53

Trunk resource: add Trunk resource controller implementation

e2ca1e1

Trunk resource: auto-generated files + fixing some bugs

29faf66

Trunk resource: increase test coverage + fix bugs

ba26726

Trunk resource: add improvments + fix bugs

8fe327a

winiciusallan mentioned this pull request Dec 18, 2025

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

Open

Trunk resource: fixed testing

960bd52

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

Trunk type: fix scaffolding issues + minor bugs

545a5fa

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

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

Add Trunk resource controller implementation #581

Are you sure you want to change the base?

Add Trunk resource controller implementation #581

Uh oh!

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

aldokimi commented Nov 22, 2025 •

edited

Loading

winiciusallan commented Dec 19, 2025 •

edited

Loading