-
Notifications
You must be signed in to change notification settings - Fork 34
Add KeyPair controller with Server integration #568
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hey Ella, if you could split your work in multiple commits that would help with the review. I have found it very helpful, when working on controllers after we created the scaffolding tool, to have at least 2 separate commits:
I'll try to have a first pass at your work today. |
|
Hi @mandre I'll work on splitting it tomorrow just 1 missing thing that I already know is missing as I haven't got to is is kuttl testing for the integration with the existing server controller so I'll try to get that into the patch tomorrow as well. |
9052675 to
86592b4
Compare
|
Hi, @eshulman2. Thanks for the work! Before reviewing, I want to confirm one thing. Do we want to support compute API microversions before 2.92? I ask because the generation of key pairs has been removed from the Nova API.[1][2] [1] https://specs.openstack.org/openstack/nova-specs/specs/zed/implemented/keypair-generation-removal.html |
|
@winiciusallan raised a good point. I believe we should remove the ability to generate keypairs as it's gone from nova. BTW, I'm getting a different result than the one you have in the first commit when running the command from the commit message. I assume you passed arguments interactively. May I suggest you add the |
f509d42 to
c53a3ca
Compare
|
Hi @winiciusallan , @mandre I believe those issues should be fixed. I encountered another issue with the KeyPairImport. keypairs uses names as their UUID and doesn't have an ID that matches the existing ID validation (kubebuilder:validation:Format:=uuid). This is currently a problem for the keypairimport as it is generated expecting to work with a UUID. I added a flag to the templates to allow having a non kubebuilder:validation:Format:=uuid validation for the name based id. |
9bf600a to
1812f92
Compare
That's ok for me. There are still cases which we currently don't handle yet, so adding a new spec for it seems good. Btw, I'll try to take a look at your code today or at the latest tomorrow. |
internal/controllers/keypair/tests/keypair-create-full/00-assert.yaml
Outdated
Show resolved
Hide resolved
46f5dc6 to
3d5e8fc
Compare
internal/controllers/keypair/tests/keypair-create-minimal/00-assert.yaml
Outdated
Show resolved
Hide resolved
internal/controllers/keypair/tests/keypair-create-full/00-assert.yaml
Outdated
Show resolved
Hide resolved
470b043 to
a02dcda
Compare
|
Failed to assess the semver bump. See logs for details. |
Keypairs in openstack uses names instead of ID to enable proper generation of Imports and other add a template option to allow it
command: go run ./cmd/scaffold-controller \ -kind KeyPair \ -gophercloud-client "NewComputeV2" \ -gophercloud-module "github.com/gophercloud/gophercloud/v2/openstack/compute/v2/keypairs" \ -gophercloud-type "KeyPair" \ -openstack-json-object "keypair"
This commit adds complete KeyPair controller support including: - API types with validation for public key import and creation - Controller implementation with client-side name filtering for import/adoption - Unit tests for filtering logic with mock client - KUTTL integration tests for create, import, and error scenarios - CRD, RBAC, and sample manifests KeyPairs are immutable resources where the name serves as the unique identifier. The OpenStack KeyPairs API only supports server-side userID filtering, so client-side filtering is implemented for name-based import and adoption. issue: k-orc#136 Assisted-by: Claude
Enables servers to be created with SSH keypairs by adding optional keypairRef field to ServerResourceSpec. Uses gophercloud's keypairs.CreateOptsExt to inject key_name into server creation. The keypair dependency is watched but has no deletion guard since keypairs are only injected during server boot. Assisted-by: claude
mandre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! Ready to merge.
Summary
This PR implements full KeyPair controller support and integrates it with the Server resource, enabling SSH key management and server access control through Kubernetes resources.
Changes
Commit 1: Add KeyPair controller implementation
Implements complete KeyPair resource controller with the following features:
API & Types:
KeyPairImportfor importing public keys from secretsKeyPairSpecfor OpenStack resource creation with public keyController Implementation:
Commit 2: Add keypair integration to Server resource
Enables servers to be created with SSH keypairs for access:
API Changes:
keypairReffield toServerResourceSpecController Changes:
Actuator Changes:
keypairs.CreateOptsExtextension pattern to injectkey_namekeypair.Status.Resource.NameNotes
spec.resource.nameis setFixes: #136
Assisted-by: claude