Keystone: Endpoint controller #614
Conversation
go run ./cmd/scaffold-controller \
-interactive=false \
-kind Endpoint \
-gophercloud-client NewIdentityV3 \
-gophercloud-module github.com/gophercloud/gophercloud/v2/openstack/identity/v3/endpoints \
-required-create-dependency Service
Signed-off-by: Winicius Silva <[email protected]>
winiciusallan
force-pushed
the
endpoint-controller
branch
from
635e6e1 to
16996dd
Compare
Signed-off-by: Winicius Silva <[email protected]>
winiciusallan
force-pushed
the
endpoint-controller
branch
from
16996dd to
2783f0e
Compare
mandre
left a comment
mandre
left a comment
There was a problem hiding this comment.
I wasn't sure at first, but I now believe we should wait for the next gophercloud release to avoid the introduction of a backward incompatible change in the API when we drop the Name field.
| // +kubebuilder:default:=true | ||
| // +optional | ||
| Description *string `json:"description,omitempty"` | ||
| Enabled *bool `json:"enabled,omitempty"` |
There was a problem hiding this comment.
It feels like the Enabled field shouldn't be there until your gophercloud PR adding support for it makes it to a gophercloud release we can use in ORC. I see you've dropped Description that has the same issue.
Alternatively, we wait a bit for the next gophercloud release so that we have a clean PR from the start (we can include gophercloud/gophercloud#3581 in it too).
| // name will be the name of the created resource. If not specified, the | ||
| // name of the ORC object will be used. | ||
| // +optional | ||
| Name *OpenStackName `json:"name,omitempty"` |
There was a problem hiding this comment.
I'd prefer we do not merge this until we have removed the requirement for Name in gophercloud because this will be a breaking change in the ORC API. This is another argument for waiting for the next gophecloud release where you've had several improvements to the support of endpoint.
| // description is a human-readable description for the resource. | ||
| // enabled indicates whether the endpoint is enabled or not. | ||
| // +optional | ||
| Enabled *bool `json:"enabled,omitempty"` |
There was a problem hiding this comment.
We can drop the pointer in the status.
| Enabled *bool `json:"enabled,omitempty"` | ||
|
|
||
| // interface indicates the visibility of the endpoint. | ||
| // +kubebuilder:validation:Enum:=admin;internal;public |
There was a problem hiding this comment.
I think we want to drop the enum validation in the status and instead return whatever OpenStack gives us. We still want to have a MaxLength validation.
| | **controller** | **1.x** | **2.x** | **main** | | ||
| |:---------------------------:|:-------:|:-------:|:--------:| | ||
| | domain | | ✔ | ✔ | | ||
| | endpoint | | ◐ | ◐ | |
There was a problem hiding this comment.
I believe we can make it a ✔ when we're based on a newer gophercloud.
| Name: "Group", | ||
| }, | ||
| { | ||
| Name: "Endpoint", |
There was a problem hiding this comment.
We'll need to set IsNotNamed: true to drop the name from the API definition.
| commands: | ||
| # We should be able to delete the import dependencies | ||
| - command: kubectl delete service endpoint-import-dependency | ||
| - command: kubectl delete services.openstack.k-orc.cloud endpoint-import-dependency |
There was a problem hiding this comment.
We should update the generator to use the fully qualified name for kubectl commands to avoid problems like this. I'll create a PR.
2 participants
This PR introduces the Keystone Endpoint controller.
Fixes: #592
Reviewer notes:
Namefield on creation, and name is deprecated according to the docs, so since I can't omit it for now, I have decided to implement the controller with this field. I opened a PR to address itenabledfield on creation neither on update, so I have to omit it mainly in the tests. This PR was opened to fix it,