Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #174 +/- ##
=========================================
Coverage ? 58.52%
=========================================
Files ? 25
Lines ? 1401
Branches ? 136
=========================================
Hits ? 820
Misses ? 559
Partials ? 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request addresses OpenStack authentication/authorization issues by introducing an unscoped Keystone client with system-level permissions. The main change enables querying user information across projects, which may fail when using project-scoped tokens due to OpenStack's permission model.
Changes:
- Added
keystone_unscopedclient attribute toBaseOpenStackExtractorfor system-scoped authentication - Modified
_get_keystone_clientmethod to support both project-scoped and unscoped clients via aproject_scopedparameter - Updated
_get_keystone_userto use the unscoped client for retrieving user information
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -90,10 +91,12 @@ def _get_keystone_session(self): | |||
| session = keystone_client.get_session(CONF, self.project) | |||
| return session | |||
|
|
|||
| def _get_keystone_client(self): | |||
| def _get_keystone_client(self, project_scoped=True): | |||
| """Get a Keystone Client for the configured project in the object.""" | |||
| client = keystone_client.get_client( | |||
| CONF, project=self.project, system_scope="all" | |||
| CONF, | |||
| project=self.project if project_scoped else None, | |||
| system_scope="all" | |||
| ) | |||
| return client | |||
|
|
|||
| @@ -125,7 +128,7 @@ def _get_project_id(self): | |||
| def _get_keystone_user(self, uuid): | |||
| """Get the Keystone username for a given uuid.""" | |||
| try: | |||
| user = self.keystone.users.get(user=uuid) | |||
| user = self.keystone_unscoped.users.get(user=uuid) | |||
There was a problem hiding this comment.
The new keystone_unscoped client attribute and its usage in _get_keystone_user lack test coverage. While the existing tests mock BaseOpenStackExtractor.__init__ (which prevents breaking existing tests), consider adding tests that verify:
- The unscoped client is created with
project=Noneandsystem_scope="all" - The
_get_keystone_usermethod correctly uses the unscoped client to fetch user information - Error handling works as expected when the unscoped client lacks permissions
This is important because the change addresses what appears to be an OpenStack permissions issue that should be validated through testing.
| """Get the Keystone username for a given uuid.""" | ||
| try: | ||
| user = self.keystone.users.get(user=uuid) | ||
| user = self.keystone_unscoped.users.get(user=uuid) |
There was a problem hiding this comment.
Consider adding a comment explaining why an unscoped Keystone client is needed here. The change from using self.keystone to self.keystone_unscoped suggests this addresses an OpenStack permissions issue where project-scoped tokens may not have permission to query user information. A brief comment would help future maintainers understand the reasoning, for example:
"Use unscoped client with system scope to query user information, as project-scoped tokens may lack permissions to access users across all projects."
…e related documentation
|
No description provided.