Keystone: support for Service controller #566

winiciusallan · 2025-11-14T22:22:28Z

This pull request introduces support for the Keystone Service controller

Closes: #429

Reviewer notes:

Because of this issue, I needed to handle with Type field in some reconcile operations.
I was in doubt about the adoption implementation, so checking in other controllers, I decided to just make available all current list options (name and type).

github-actions · 2025-11-14T22:22:46Z

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

mandre · 2025-11-16T10:06:52Z

I have found it very helpful, when working on controllers after we created the scaffolding tool, to have at least 2 separate commits:

the first one that just runs the scaffolding tool and commits all the generated files. We should write the exact command we ran in the commit message for reproducibility.
a second commit that fills the blank and effectively implements the specific parts of the controller, including the API changes and the tests.
more commits as necessary.

This helps figure out quickly the specific parts of the controller and find potential bugs and missing bits more easily. If we agree on this patter, we may want to add it as a requirement in our developers docs.

github-actions · 2025-11-16T14:38:46Z

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

winiciusallan · 2025-11-16T14:40:24Z

This helps figure out quickly the specific parts of the controller and find potential bugs and missing bits more easily. If we agree on this patter, we may want to add it as a requirement in our developers docs.

As discussed in #567, we have some gaps to fill in the developer documentation. This may be a good addition.

I've reverted some stuff and pushed the commits. I will just need to make another commit to resolve merge conflicts.

github-actions · 2025-11-17T14:50:38Z

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

github-actions · 2025-11-17T15:20:52Z

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

mandre · 2025-11-17T16:49:02Z

api/v1alpha1/service_types.go

+	Enabled *bool `json:"enabled,omitempty"`
+
+	// extra indicates key-value information about the service.
+	Extra map[string]any `json:"extra,omitempty"`


That appears to be legacy, undocumented API. I'd recommend we stick with what's documented at https://docs.openstack.org/api-ref/identity/v3/#create-service.

@dlaw4608 had the same problem with the Role controller, and he ended up updating gophercloud to properly support the missing fields (so that we can eventually deprecate Extra in gophercloud). Have a look at gophercloud/gophercloud#3532.

Yeah, it makes sense to update gophercloud struct to support only keystone API fields. I can open a PR for this.

Although keystone has extra in its schema, it should get only valid values when making requests.

https://github.com/openstack/keystone/blob/master/keystone/catalog/backends/sql.py#L60

I think we can block this PR for now until gophercloud's PR is not accepted. If I keep working on this we'll need to change the Service controller and all others that depend on it when deprecating Extra. Wdyt?

If we don't want to wait until the next release, I can make it work with free-forms Extra values and make the changes to use valid API values after the release.

In any case, we don't want to expose undocumented Keystone fields in the ORC API. We should have Description even if internally we implement it via the Extra field, until gophercloud properly has first class support for Description.

Similarly, we want to add Name.

mandre

A few comments on the API (this is really the critical part in ORC and we want to get it right). We shouldn't expose the undocumented Extra fields of keystone, even if internally we may use it until gophercloud implements all the fields.

mandre · 2025-11-18T07:09:45Z

api/v1alpha1/service_types.go

+	Enabled *bool `json:"enabled,omitempty"`
+
+	// extra indicates key-value information about the service.
+	Extra map[string]any `json:"extra,omitempty"`


In any case, we don't want to expose undocumented Keystone fields in the ORC API. We should have Description even if internally we implement it via the Extra field, until gophercloud properly has first class support for Description.

api/v1alpha1/service_types.go

mandre · 2025-11-18T07:19:45Z

api/v1alpha1/service_types.go

+type ServiceResourceSpec struct {
+	// type indicates which resource the service is responsible for.
+	// +kubebuilder:validation:MinLength:=1
+	// +kubebuilder:validation:MaxLength:=255


The limit is 255 indeed: https://github.com/openstack/keystone/blob/19947349904082995b5477d4ec8272bf01f176b2/keystone/catalog/backends/sql.py#L53

mandre · 2025-11-18T07:22:55Z

api/v1alpha1/service_types.go

+
+	// enabled indicates whether the service is enabled or not.
+	// +kubebuilder:default=true
+	Enabled *bool `json:"enabled,omitempty"`


If there's a default value (docs say it's optional), we want to make the field optional. If not, we should make it required and drop the pointer.

Although docs say its optional, I decided to define a default value because the schema defines it.

https://github.com/openstack/keystone/blob/master/keystone/catalog/backends/sql.py#L57

README.md

mandre · 2025-11-18T07:31:49Z

api/v1alpha1/service_types.go

+	Enabled *bool `json:"enabled,omitempty"`
+
+	// extra indicates key-value information about the service.
+	Extra map[string]any `json:"extra,omitempty"`


Similarly, we want to add Name.

mandre · 2025-11-18T07:32:12Z

api/v1alpha1/service_types.go

-	// github.com/gophercloud/gophercloud/v2/openstack/identity/v3/services
+	// extra indicates key-value information about the service.
+	// +optional
+	Extra map[string]any `json:"extra,omitempty"`


There too, we'll want to add Name and Description. In the controller's implementation, we'll fetch that from the Extra fields until gophercloud has better support for these fields.

Done. Handling the description was kinda tricky, but I believe it is working properly now.

github-actions · 2025-11-23T02:19:29Z

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

github-actions · 2025-11-23T03:02:31Z

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

winiciusallan · 2025-11-23T03:25:36Z

@mandre do you have a clue why make-generate is failing? I have run locally a couple of times and it seems ok. Replacing the diff text manually makes sense?

also, I already fixed the lint. I'm waiting for your response to the above question to commit once. 😄

mandre · 2025-11-24T12:30:31Z

@mandre do you have a clue why make-generate is failing? I have run locally a couple of times and it seems ok. Replacing the diff text manually makes sense?

I believe it's because of #565, you'll need to rebase your change.

$ go run ./cmd/scaffold-controller \ -interactive=false \ -kind Service \ -gophercloud-client NewIdentityV3 \ -gophercloud-module github.com/gophercloud/gophercloud/v2/openstack/identity/v3/services Signed-off-by: Winicius Silva <winiciusab12@gmail.com>

mandre

This seems very close to a merging state. Really nice work!
I've noticed you made iterative commits, where you added new commits to address review comments. That's OK during the review phase as it makes each change stand out, but we should rebase and squash the commits to keep clean history when we're happy with the PR status. Would you mind doing it before we merge your PR?

mandre · 2025-11-24T15:43:06Z

api/v1alpha1/service_types.go

+	// +kubebuilder:validation:MinLength:=1
+	// +kubebuilder:validation:MaxLength:=255
+	// +optional
+	Description *string `json:"description,omitempty"`


We should drop the pointer as it's not needed.

I'm a little confused about when I need to use a pointer for types. If my Description isn't a pointer, when unmarshalling from the user input, if I don't specify the Description field in spec, will it fall as a default value (like an empty string)? And consequently omitted from json, as I passed omitempty.

Sorry if that wasn't clear, we should drop the pointer from Description in the ServiceResourceStatus but not in the ServiceResourceSpec, because we want to leave the possibility to unset the value in the Spec but we will always report what OpenStack returns in the Status (in other words, we shouldn't have changed the types the scaffolding did for Description).

That was my mistake, actually. I thought you were talking about Description in spec, so I removed pointer from there 😅 . I'll fix it

Thanks for the explanation, this makes a lot of sense to me. The docs is also very clear about it too.

mandre · 2025-11-24T16:23:06Z

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

 assertAll:
    - celExpr: "service.status.id != ''"
-    - celExpr: "service.status.resource.extra.name != ''"
+    - celExpr: "service.status.resource.name != ''"


These two checks (name and type being != '') are redundant with the kuttl matcher. Not a big deal, but would be cleaner to remove them.

winiciusallan · 2025-11-24T17:44:08Z

This seems very close to a merging state. Really nice work! I've noticed you made iterative commits, where you added new commits to address review comments. That's OK during the review phase as it makes each change stand out, but we should rebase and squash the commits to keep clean history when we're happy with the PR status. Would you mind doing it before we merge your PR?

Thanks! That's ok for me!

For the sake of clarity, do you want me to keep only commits with changes after running the scaffolding-tool and the implementation itself?

Before merging, I would like to highlight something I noticed yesterday. I don't implement support for filling the Extra field, so if a user wants to add an email, for example, it won't be possible. Actually, I don't see many use cases for free-forms extra fields, but I've seen in gophercloud test files this such case.

mandre · 2025-11-24T19:55:39Z

This seems very close to a merging state. Really nice work! I've noticed you made iterative commits, where you added new commits to address review comments. That's OK during the review phase as it makes each change stand out, but we should rebase and squash the commits to keep clean history when we're happy with the PR status. Would you mind doing it before we merge your PR?

Thanks! That's ok for me!

For the sake of clarity, do you want me to keep only commits with changes after running the scaffolding-tool and the implementation itself?

The scaffolding should be its own commit (just running the command that is documented in the commit message + make generate), then all the necessary commits to implement the controller. Typically, you'd end up with 2 commits: the scaffolding and the implementation of the controller + tests, but it's very possible that you'd have more commits if for example you need to do refactoring or have other controllers make use of your new controllers. We want to split the commits in logical changes that make sense separately (I'll leave that up to you how you want to logically split your commits). Take a look at how @eshulman2 did on #568, this is a very good example.

Before merging, I would like to highlight something I noticed yesterday. I don't implement support for filling the Extra field, so if a user wants to add an email, for example, it won't be possible. Actually, I don't see many use cases for free-forms extra fields, but I've seen in gophercloud test files this such case.

That's fine, we don't want to expose Extra from the ORC API because:

it's undocumented and could go away at any time
we don't need it

winiciusallan · 2025-11-24T22:28:20Z

Hi @mandre! Thanks for the clear explanation. I've decided to keep only two commits (one with scaffolding tool generated code, and another with controller implementation itself).

You may take a last look, but I believe it is now ready to go.

mandre · 2025-11-25T07:13:56Z

api/v1alpha1/service_types.go

+	// +kubebuilder:validation:MinLength:=1
+	// +kubebuilder:validation:MaxLength:=255
+	// +optional
+	Description *string `json:"description,omitempty"`


Sorry if that wasn't clear, we should drop the pointer from Description in the ServiceResourceStatus but not in the ServiceResourceSpec, because we want to leave the possibility to unset the value in the Spec but we will always report what OpenStack returns in the Status (in other words, we shouldn't have changed the types the scaffolding did for Description).

mandre · 2025-11-25T07:26:58Z

api/v1alpha1/service_types.go

-	// name is a Human-readable name for the resource. Might not be unique.
-	// +kubebuilder:validation:MaxLength=1024
+	// name indicates the name of service.
+	// +kubebuilder:validation:MinLength:=1


Let's not add minimum length validation to the status. We will just return what OpenStack return [1]. We however want to have a limit on the maximum size of the resource for CEL validation budget. You may want to check this talk from Joel Speed who explains the reasons [2].

[1] https://k-orc.cloud/development/api-contracts/#resourcestatus
[2] https://www.youtube.com/watch?v=IfaPAqDfJHk

Thanks! In the video, Joel talks a lot about MaxLength, but out of curiosity, can MinLength in some way affect the cost of validation? Well, if kube-apiserver considers the worst case for the cost, I don't think it should be considered.

internal/controllers/service/actuator.go

internal/controllers/service/tests/service-update/00-assert.yaml

internal/controllers/service/tests/service-update/00-minimal-resource.yaml

internal/controllers/service/tests/service-update/00-assert.yaml

internal/controllers/service/tests/service-update/02-assert.yaml

internal/controllers/service/tests/service-import/01-create-trap-resource.yaml

mandre

Sorry for the lag, I thought I submitted my review yesterday but looks like not.

I just have minor nits, then it's good to go.

mandre · 2025-11-25T15:45:41Z

api/v1alpha1/service_types.go

-	// description is a human-readable description for the resource.
-	// +kubebuilder:validation:MaxLength=1024
+	// description indicates the description of service.
+	// +kubebuilder:validation:MinLength:=1


let's drop the MinLength check in the resource status (we should make that part of the linter if we figure out how to do it).

api/v1alpha1/service_types.go

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

Signed-off-by: Winicius Silva <winiciusab12@gmail.com>

winiciusallan force-pushed the service-controller branch from 95db1e9 to c41700b Compare November 16, 2025 14:38

winiciusallan force-pushed the service-controller branch from 7039f65 to cf16119 Compare November 17, 2025 15:20

winiciusallan marked this pull request as draft November 17, 2025 16:47

mandre reviewed Nov 17, 2025

View reviewed changes

mandre reviewed Nov 18, 2025

View reviewed changes

winiciusallan force-pushed the service-controller branch from b97b23d to 43bc4a5 Compare November 24, 2025 13:48

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

winiciusallan marked this pull request as ready for review November 24, 2025 14:33

mandre reviewed Nov 24, 2025

View reviewed changes

winiciusallan force-pushed the service-controller branch from 43bc4a5 to c9de13a Compare November 24, 2025 22:24

winiciusallan force-pushed the service-controller branch from c9de13a to ed70f86 Compare November 24, 2025 23:33

mandre reviewed Nov 25, 2025

View reviewed changes

winiciusallan force-pushed the service-controller branch from ed70f86 to 8c85525 Compare November 25, 2025 12:46

mandre reviewed Nov 26, 2025

View reviewed changes

winiciusallan force-pushed the service-controller branch from 8c85525 to 6132aa5 Compare November 26, 2025 10:57

keystone: add service controller implementation

85a5f41

Signed-off-by: Winicius Silva <winiciusab12@gmail.com>

winiciusallan force-pushed the service-controller branch from 6132aa5 to 85a5f41 Compare November 26, 2025 11:00

mandre approved these changes Nov 26, 2025

View reviewed changes

mandre added this pull request to the merge queue Nov 26, 2025

Merged via the queue into k-orc:main with commit f5861de Nov 26, 2025
9 checks passed

winiciusallan deleted the service-controller branch December 1, 2025 17:33

mandre added this to the Release 2.4 milestone Dec 17, 2025

Keystone: support for Service controller #566

Keystone: support for Service controller #566

Uh oh!

Conversation

winiciusallan commented Nov 14, 2025

Uh oh!

github-actions bot commented Nov 14, 2025

Uh oh!

mandre commented Nov 16, 2025

Uh oh!

github-actions bot commented Nov 16, 2025

Uh oh!

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

Uh oh!

Uh oh!

github-actions bot commented Nov 17, 2025

Uh oh!

github-actions bot commented Nov 17, 2025

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 left a comment

Choose a reason for hiding this comment

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!

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!

github-actions bot commented Nov 23, 2025

Uh oh!

github-actions bot commented Nov 23, 2025

Uh oh!

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

Uh oh!

Uh oh!

mandre commented Nov 24, 2025

Uh oh!

mandre left a comment

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!

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

Uh oh!

Uh oh!

mandre commented Nov 24, 2025

Uh oh!

winiciusallan commented Nov 24, 2025

winiciusallan commented Nov 16, 2025 •

edited

Loading

winiciusallan commented Nov 23, 2025 •

edited

Loading

winiciusallan commented Nov 24, 2025 •

edited

Loading