-
Notifications
You must be signed in to change notification settings - Fork 287
🌱 Add OpenStackAuthenticationSucceeded condition #2985
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
🌱 Add OpenStackAuthenticationSucceeded condition #2985
Conversation
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Maybe a better name would be |
I do indeed think that
|
d6cabc9 to
8cbf7e4
Compare
I like this also! Changing :) |
8cbf7e4 to
290723e
Compare
30ea69d to
9df7bdd
Compare
lentzi90
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.
/hold cancel
|
/cc @mandre @bnallapeta |
|
/lgtm |
|
Should we try to define a condition that is perhaps a little bit less specific? Do you think a more generic condition like |
|
Obviously, if we're using a condition that express "I can't connect to the cloud", we would need to change its status in more places. |
|
Hmm, yes it would be good to capture more issues, but I am not sure how much more we can do at this early stage. If the secret is missing or malformed, we get a reconciliation error here. Those errors are included in the message with the condition, so the user do get that information. When there are issues later on, it is usually quite hard to know if they are related to the credentials without parsing the error messages. I am hoping that we do set relevant conditions and messages in most places though. cluster-api-provider-openstack/controllers/openstackcluster_controller.go Lines 330 to 333 in 9e848d9
Sigh... we set no condition. |
|
Documenting findings from manual tests. I was wrong about how far we check the credentials when creating the scope. We do notice all kinds of issues here immediately, so that is really great. I'll still create an issue for adding a condition to that @mandre what do you think about these below? Are they good enough (at least as a first step)? Here is the condition for a cluster with non-existing credentials secret: status:
conditions:
- lastTransitionTime: "2026-01-29T11:05:25Z"
message: 'Failed to create OpenStack client scope: secrets "dev-test-cloud-config"
not found'
reason: OpenStackCredentialsFailed
severity: Error
status: "False"
type: OpenStackCredentialsAvailable
ready: falseThis I am quite happy with. It clearly shows which secret it is unhappy about and why. "OpenStack client scope" is perhaps not immediately understandable, but it avoids making assumptions about the kind of error we get. We cannot know if there is some parsing error or something else entirely going on so I think it is quite ok. Next, I created a secret with matching name but nonsense content. Then I got this: status:
conditions:
- lastTransitionTime: "2026-01-29T11:12:59Z"
message: 'Failed to create OpenStack client scope: OpenStack credentials secret
dev-test-cloud-config did not contain key clouds.yaml'
reason: OpenStackCredentialsFailed
severity: Error
status: "False"
type: OpenStackCredentialsAvailable
ready: falseThis is also really nice IMO! Then I did one with proper content but wrong clouds: spec:
identityRef:
cloudName: capo-e2e # <-- This does not exist in the clouds.yaml
name: dev-test-cloud-config
type: Secret
...
status:
conditions:
- lastTransitionTime: "2026-01-29T11:16:19Z"
message: 'Failed to create OpenStack client scope: auth option failed for cloud
: Missing input for argument [auth_url]'
reason: OpenStackCredentialsFailed
severity: Error
status: "False"
type: OpenStackCredentialsAvailable
ready: falseIt is looking for capo-e2e but it doesn't exist. Not the best error message, but I am not sure how to improve. Should we add extra validation somewhere? I think this is coming from gophercloud currently. Next, correct cloud entry except the password was wrong: status:
conditions:
- lastTransitionTime: "2026-01-29T11:21:47Z"
message: 'Failed to create OpenStack client scope: providerClient authentication
err: Expected HTTP response code [201 202] when accessing [POST http://10.0.3.15/identity/v3/auth/tokens],
but got 401 instead: {"error":{"code":401,"message":"The request you have made
requires authentication.","title":"Unauthorized"}}'
reason: OpenStackCredentialsFailed
severity: Error
status: "False"
type: OpenStackCredentialsAvailable
ready: falseQuite ok I would say. Finally, wrong URL: status:
conditions:
- lastTransitionTime: "2026-01-29T11:23:19Z"
message: 'Failed to create OpenStack client scope: providerClient authentication
err: Get "http://10.0.3.150/identity/": EOF'
reason: OpenStackCredentialsFailed
severity: Error
status: "False"
type: OpenStackCredentialsAvailable
ready: falseAlso ok IMO. All in all, I think this is a clear improvement. Before this PR we did not get any conditions at all. Users had to dig through logs. |
|
Thanks for manually checking the different cases. I believe this covers the most important ones indeed. I'd still rename the condition to |
|
Good point! I guess there is a little risk that authentication fails later, in the middle of operations. Then we probably would not be able to set this condition. But the next reconcile should handle that anyway so should be fine. I also realized now that we should check all other controllers and see if we can do the same there. This one is the most critical, but we could have different credentials per OpenStackMachine and those should also have conditions then. |
9df7bdd to
97c19ee
Compare
lentzi90
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.
/hold
TODO: I should check if I can remove the //nolint comments now after renaming...
97c19ee to
4220771
Compare
|
@lentzi90 please update once this is ready for review. Thanks. |
|
/hold cancel |
|
I forgot to mention that I didn't include OpenStackMachineTemplate here. It has no conditions at all yet and I didn't want to "mess with the API" just to add these conditions there also. I'll instead do a follow up PR where I add conditions there. |
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.
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mandre The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
bnallapeta
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.
Looks good in general. Pending those minor comments.
|
Thanks @bnallapeta ! Good findings! |
- Sets OpenStackAuthenticationSucceededCondition to True/False based on credential validity during reconciliation. - Adds tests for condition handling on credential errors and missing secrets. The condition is set for OpenStackCluster, OpenStackMachine, OpenStackServer and OpenStackFloatingIPPool. OpenStackMachineTemplate is currently missing conditions completely. These will be added separately. Signed-off-by: Lennart Jern <[email protected]>
4220771 to
36e4343
Compare
|
/lgtm |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #2264
Special notes for your reviewer:
TODOs:
/hold