Conversation
Policy override was causing lots of error messages in the Ironic view of Horizon. I've asked upstream if we can get this enabled by default: https://bugs.launchpad.net/horizon/+bug/2102214 But are yet to see any traction.
| # Intended scope(s): system, project | ||
| # Overridden: added role:admin | ||
| "baremetal:node:list_all": "role:admin or (role:reader and system_scope:all) or (role:service and system_scope:all) or rule:service_role" | ||
| "baremetal:node:list_all": "role:baremetal_node_list_all or (role:reader and system_scope:all) or (role:service and system_scope:all) or rule:service_role" |
There was a problem hiding this comment.
does it mean that regular admin users now won't be able to list all baremetals from cli? - I think that was idea behind this policy override. also baremetal_node_list_all don't exist by default?
There was a problem hiding this comment.
baremetal_node_list_all don't exist by default?
Correct, you'd have to create that.
does it mean that regular admin users now won't be able to list all baremetals from cli?
Correct, unless they own all nodes (via setting the owner property on baremetal nodes). You can add the new role if you need that behaviour (but it will break your horizon).
There was a problem hiding this comment.
Will this break Horizon, even with SYSTEM_SCOPE_SERVICES = ['ironic'] set?
There was a problem hiding this comment.
Yes, as I think system scope appears in the project drop down. So if you have a project selected (rather than system scope) and you navigate to the baremetal provisioning section, you will get a bunch of errors. You would be able to select system scope and browse to the same page though.
There was a problem hiding this comment.
@bbezak what I think we should do is make sure the nodes are all added into a single project, e.g. admin project, so the regular project scoped admin user can still list them all, and do all the usual actions to the nodes that worked in yoga, etc.
In my head, this extra role here is only needed to help in the Slurm case, where some other member in some other project, needs to list all baremetal nodes, to convert the baremetal node name into a uuid, to pass into the scheduler hint.
There was a problem hiding this comment.
I think John is right. We don't break horizion for ordinary users (non-admin). It is is breaking change for users with the admin role though - so perhaps best to merge in 2026.1?
| --- | ||
| upgrade: | ||
| - | | ||
| Users with the admin role can can no longer list all baremetal nodes by |
There was a problem hiding this comment.
nit: 'can' was written twice
| default. This broke horizon as it tries to collect data for any nodes | ||
| that it can see. Please add the baremetal_node_list_all role | ||
| to any users to want this capability; it is not recommened since | ||
| Horizon will remain broken for them. |
There was a problem hiding this comment.
I don't think that will break Horizon, right, as the member + baremetal_node_list_all user will not have the ironic tab appear in horizon, anyways.
The only remaining broken combination would be having the admin role, in a project other than the project that owns all the ironic nodes. I guess? Which I suppose you fix by adding the system scope thing?
Updated release notes to clarify role permissions and fix details.
Clarified the restriction on admin role access to baremetal nodes and added role assignment instructions.
Updated role requirement for listing baremetal nodes and fixed Horizon Ironic integration.
|
Happy Friday @jovial, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
3 similar comments
|
Happy Friday @jovial, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
|
Happy Friday @jovial, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
|
Happy Friday @jovial, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: stackhpc/stackhpc-kayobe-config/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 046e0919-17ff-4bc9-ab57-de63b7afb4e8
📒 Files selected for processing (3)
etc/kayobe/kolla/config/horizon/_9999-custom-settings.pyetc/kayobe/kolla/config/ironic/policy.yamlreleasenotes/notes/fix-ironic-horizon-integration-c844d19bd36ed014.yaml
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: stackhpc/check
- GitHub Check: Ansible 2.19 lint with Python 3.12
- GitHub Check: Tox releasenotes with Python 3.12
- GitHub Check: Ansible 2.20 lint with Python 3.12
🔇 Additional comments (2)
releasenotes/notes/fix-ironic-horizon-integration-c844d19bd36ed014.yaml (1)
1-8: LGTM!etc/kayobe/kolla/config/horizon/_9999-custom-settings.py (1)
1-1: 🎯 Functional CorrectnessDo not rely on this setting until the deployed ironic-ui revision is known to support it. The repository does not identify the ironic-ui revision inside the Horizon image. The current upstream Ironic panel documentation lists
permissionsbut no scope-awareallowed()override, and Bug 2102214 requests this support. If the deployed image uses that implementation, the panel may remain available with a project-scoped token and fail Ironic policy checks.
| # Intended scope(s): system, project | ||
| # Overridden: added role:admin | ||
| "baremetal:node:list_all": "role:admin or (role:reader and system_scope:all) or (role:service and system_scope:all) or rule:service_role" | ||
| "baremetal:node:list_all": "role:baremetal_node_list_all or (role:reader and system_scope:all) or (role:service and system_scope:all) or rule:service_role" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale policy comment.
The comment still says that this override added role:admin, but the rule now grants role:baremetal_node_list_all. Update the comment so operators do not infer the old access model.
This branch was successfully deployed
6 participants
Policy override was causing lots of error messages in the Ironic view of Horizon.
I've asked upstream if we can get this enabled by default:
https://bugs.launchpad.net/horizon/+bug/2102214
But are yet to see any traction.