Expand list of accepted ENV vars for openstack.AuthOptionsFromEnv - #3657
Expand list of accepted ENV vars for openstack.AuthOptionsFromEnv#3657danchild wants to merge 2 commits into
Conversation
github-actions
Bot
added
edit:openstack
|
coverage: 63.689% (+0.2%) from 63.519% — danchild:patch-issue-3440 into gophercloud:main |
|
Hi @mandre regarding the failed tests, I just pushed a new commit that allows for OS_DOMAIN_X to be set alongside OS_USER_DOMAIN_X and OS_PROJECT_DOMAIN_X provided that they are the same values. I also added a check to test if there is ambiguity between OS_TENANT_X and OS_PROJECT_X which hadn't been done in previous versions. |
1c4055c to
84ea7ea
Compare
There was a problem hiding this comment.
hi @danchild, I took a look at your changes, but I will need to do another round of review. This PR is very important to improve how Gophercloud is handling authentication.
I would say that my impression is that we now have a long function which is handling a lot of validations--most of them are doing the same kind of verification--, and this may be hard to maintain in the future. So, I believe we could extract these things to separate helper functions and use them accordingly.
Besides, IMO it would be good to document how the validation process is. For example, 1) verify the scope, 2) override domain variables for backward compatibility, ...
Another pair of eyes here would be good
|
Hi @winiciusallan sorry for the late reply here, and thank you for your feedback. I agree with you that creating helper functions would make the intent clearer and documenting the process as well. I will work on those and message you with an update. |
winiciusallan
left a comment
winiciusallan
left a comment
There was a problem hiding this comment.
Hi, I've added a few comments, nothing that big. If you want to wait for another review to submit a new commit, I'd say it's up to you.
I would like to see how tests will behave.
mandre
left a comment
mandre
left a comment
There was a problem hiding this comment.
I'm really not sure about this PR. This adds a lot of complexity in a critical, and already complex part of the code.
The root problem seems to be that gophercloud.AuthOptions has a single DomainID/DomainName field rather than separate user-domain and project-domain fields. Until we fix this properly and add more fields to gophercloud.AuthOptions, I think we can't make it work for all cases.
Perhaps, instead of trying to make it work for all cases with the current gophercloud.AuthOptions limitation, we should instead focus on making the failure case for downloaded default openrc from horizon more user friendly (if that's really the issue)?
|
Hi @mandre thank you for the feedback and I apologize for the delay. Give me a few weeks and I'll push changes with the default openrc as the primary lens for acceptance |
c3ca82e to
b845a23
Compare
|
Hi @mandre - I took a stab at applying the fix through the lens of accepting the default openrc file from horizon. It's become clear that ToTokenV3ScopeMap was too strict in its original design. For example, it doesn't allow project name and project id to be set at the same time, and yet they are both provided on the default openrc. The patch here does the following: 1) Removes the restrictiveness, relying on keystone to handle its own errors This works and is perhaps overly permissive, but it jives with the project's goals of prioritizing freedom over restrictions. Can you please provide your feedback on the patch? @winiciusallan - if you have any thoughts too please chime in. |
AuthOptionsFromEnv now reads OS_USER_DOMAIN_ID, OS_USER_DOMAIN_NAME, OS_PROJECT_DOMAIN_ID, and OS_PROJECT_DOMAIN_NAME. User-domain variables take precedence over the legacy OS_DOMAIN_* variables, and project-domain variables are used to scope the authentication request. Overly strict scope validation that rejected valid OpenStack configurations (e.g. ProjectID with DomainID, or ProjectName without an explicit domain) has been removed along with the corresponding error types ErrScopeDomainIDOrDomainName, ErrScopeProjectIDOrProjectName, and ErrScopeProjectIDAlone. For gophercloud#3440
Cover admin and demo openrc configurations, OS_USER_DOMAIN_* precedence over OS_DOMAIN_*, OS_PROJECT_DOMAIN_* handling, and legacy fallback behavior.
b845a23 to
6d68ffa
Compare
github-actions
Bot
added
semver:major
4 participants
Fixes #3440 #3240
According to the OpenStack API documentation, OS_USER_DOMAIN_ID, OS_USER_DOMAIN_NAME, OS_PROJECT_DOMAIN_ID, AND OS_PROJECT_DOMAIN_NAME are all valid inputs for identity v3. The current
implementation of openstack.AuthOptionsFromEnv does not accept these environment variables as inputs.
This fix keeps the gophercloud.AuthOptions struct intact, keeping the semantics of the name domainID and domainName unchanged so we do not need refactor code that consumes the the struct. Instead a new implementation
of openstack.AuthOptionsFromEnv would check for the presence of the OS_USER_DOMAIN_X and
OS_PROJECT_DOMAIN_X environment variables and set domainID and domainName accordingly.
The difficult part is to resolve ambiguity between the env vars passed in and provide either warnings or errors
to the users depending on different combinations. Given that the approach to not changing gophercloud.AuthOptions
is the best path forward, dealing with this ambiguity needs to be iterated on, and I certainly need some help determining what users expect and what makes sense base on OpenStack identity v3 conventions.
Links to the line numbers/files in the OpenStack source code that support the
code in this PR:
API Reference: https://docs.openstack.org/python-openstackclient/2025.2/cli/authentication.html