Allow direct communication to Openshift API through get_federated_user
#177
Conversation
| self.id_provider = resource.get_attribute(attributes.RESOURCE_IDENTITY_NAME) | ||
| self.apis = {} | ||
|
|
||
| self.functional_tests = os.environ.get("FUNCTIONAL_TESTS", "").lower() |
There was a problem hiding this comment.
I am uncomfortable that the code is "aware" of whether or not it's running under test. It seems like we should either be able to inject configuration in the tests to achieve the desired behavior, or we should be mocking out functionality as appropriate if we don't want it running during tests.
There was a problem hiding this comment.
As far as I know from looking at git blame, this design choice was made a long time ago and is present is both the openshift and openstack allocators. I open could a new issue for this unless there were strong reasons why this design choice was made. @knikolla Do you have any thoughts?
There was a problem hiding this comment.
In hindsight, I should have called that environment variable something like INSECURE_HTTPS because that's the only thing it allows AFAIK.
@larsks do you want the env variable to be renamed to the above now or in a follow up patch? (there are other occurrences already present in the codebase in the other drivers)
There was a problem hiding this comment.
@knikolla A followup patch would be fine.
There was a problem hiding this comment.
I see that there's already a OPENSHIFT_{self.safe_resource_name}_VERIFY environment variable here, so we can rely on just that and remove the functional tests references in this PR.
There was a problem hiding this comment.
The https check when making the openshift client will now only depend on OPENSHIFT_{self.safe_resource_name}_VERIFY. This lead to a small change in the CI file
QuanMPhm
force-pushed
the
175/merge_get_federated
branch
from
73b751b to
8839477
Compare
| @@ -19,6 +19,7 @@ class CloudAllocationAttribute: | |||
|
|
|||
|
|
|||
| RESOURCE_AUTH_URL = 'Identity Endpoint URL' | |||
| RESOURCE_IDENTITY_NAME = 'Identity Name' | |||
There was a problem hiding this comment.
A better name for this would be OpenShift Identity Provider Name as that matches what it is referencing in an unambiguous way, while Identity Name is confusing among all the other resource attributes.
| self.id_provider = resource.get_attribute(attributes.RESOURCE_IDENTITY_NAME) | ||
| self.apis = {} | ||
|
|
||
| self.functional_tests = os.environ.get("FUNCTIONAL_TESTS", "").lower() |
There was a problem hiding this comment.
I see that there's already a OPENSHIFT_{self.safe_resource_name}_VERIFY environment variable here, so we can rely on just that and remove the functional tests references in this PR.
There was a problem hiding this comment.
This file is not actually being picked up by the test runner and the tests are not being executed. Python requires a __init__.py file to consider directories as Python modules and it's missing here.
QuanMPhm
force-pushed
the
175/merge_get_federated
branch
from
8839477 to
54922e7
Compare
As the first step towards merging the account manager into our coldfront cloud plugin, the `get_federated_user` function in the Openshift allocator will now (through several functions) directly call the Openshift API. Much of the functions added are copied from the `moc_openshift` module in the account manager. Aside from copying some functions, implementation of this feature also involved: - A new resource attribute `Identity Name` for the Openshift idp - A new unit test for the `get_federated_user` function - Changes to the CI file to enable these new unit tests - A top-level logger for the Openshift allocator - New additions to the package requirements
QuanMPhm
force-pushed
the
175/merge_get_federated
branch
from
54922e7 to
6752900
Compare
|
Fix merged, can we kick off the test again? |
|
@joachimweyl Yep, now the tests are passing |
|
I'll review this week, though don't want to rush this in for tomorrow's maintenance window. |
Closes #175. As the first step towards merging the account manager into our coldfront cloud plugin, the
get_federated_userfunction in the Openshift allocator will now (through several functions) directly call the Openshift API. Much of the functions added are copied from themoc_openshiftmodule in the account manager.Aside from copying some functions,
implementation of this feature also involved:
Identity Namefor the Openshift idpget_federated_userfunction