Added dummy identity provider to remove Keystone dependancy during testing #162

QuanMPhm · 2024-07-10T15:56:31Z

Closes #159 after the draft is complete. The dummy identity provider (idp) can be enabled by setting the environment variable ESI_DEBUG to True. For now, the dummy idp returns information about a dummy project.

Some functions from api/controllers/v1/utils.py have been moved into the only controllers that use them and turned into static class methods.

Assuming everyone is fine with these draft changes, the remaining steps would be to do some cleanup with the test cases.

esi_leap/common/idp/__init__.py

esi_leap/common/idp/baseIDP.py

esi_leap/common/idp/dummyIDP.py

QuanMPhm · 2024-07-12T18:55:12Z

My apologies for the ugly diff. I should have waited to do linting and formatting at the end.

I have re-written dummyIDP.py to now accept new projects and users at runtime. The option to pick an idp class has been added in conf.esi.py. The functions offer_get_dict_with_added_info and lease_get_dict_with_added_info has been moved per @larsks's suggestion.

The only question I have left is how the test cases should be modified. References to the moved functions above will of course be changed, as well as references to the keystone idp, but I'd still like to ask...

Will we be using dummyIDP or keystoneIDP in our test cases?
Given the refactoring and the addition of the configurable idp option, should any new tests be added?

larsks · 2024-07-15T21:29:34Z

Will we be using dummyIDP or keystoneIDP in our test cases?

We will be using dummyIDP in the unit tests, because the point of unit tests is to remove external dependencies (we're not trying to test keystone itself) and verify the functionality of small units of code.

Given the refactoring and the addition of the configurable idp option, should any new tests be added?

I'll have to think on that a bit.

esi_leap/common/idp/dummyIDP.py

QuanMPhm · 2024-07-19T19:11:52Z

esi-leap/esi_leap/common/idp/__init__.py

Lines 8 to 11 in 3288e63

    
           def get_idp(): 
        
               module_path, class_name = CONF.esi.idp_plugin_class.rsplit('.', 1) 
        
               module = import_module(module_path) 
        
               return getattr(module, class_name)()

@larsks I've made the function above to allow code in the esi_leap API to obtain the configured IDP class at runtime, instead of at import time, as we've talked about in our Tuesday meeting. Since test cases can override the config file at runtime, this allows us to make the esi_leap API use arbitrary IDP classes.

esi-leap/esi_leap/api/controllers/v1/utils.py

Lines 143 to 144 in 3288e63

    
           idp = get_idp() 
        
           if offer.lessee_id not in idp.get_parent_project_id_tree(project_id):

As shown above, with this change, code in the esi_leap API, such as the Controller classes, will obtain the idp by calling get_idp(). From our discussions, I believe you wanted the idp class to be passed to the Controller classes itself, or the entire API object, instead of having the idp obtained at the function level like above? If that is the case, I will do some more investigating to see if there's a simple way of doing so.

QuanMPhm · 2024-07-22T19:37:52Z

@larsks In this draft update, I have done the following:

Rebased the PR
Made sure all test cases passed, which involved:
- Changing every reference of keystone to idp.dummyIDP.DummyIDP in the test cases
- Overriding CONF.pecan.auth_enable to True in TestLeaseControllersGetAllFilters (For some unknown reason, your commits passed this test class, but when I fetch from upstream and run the tests on my VM, upstream does not pass)
- Overriding the idp class to dummyIDP in TestOfferLesseeUtils

I will fix the python 3.8 test later, since it requires a very small fix.
With the tests (mostly) passed, I have two questions that I will wait on you before moving forward:

I have just override the config options at two extra places, and had a mild feeling that these overrides could be better placed elsewhere. Should they be placed elsewhere?
You mentioned in the past about refactoring the test cases. How should they be refactored? When I took a brief review of the test cases, many of them check if certain idp functions were called, in which case I don't see how we can avoid mocking the idp. I can see how it can be avoided for other cases though.

QuanMPhm · 2024-08-26T20:18:09Z

@larsks Is this PR still being considered?

tzumainn · 2024-08-26T20:40:02Z

I'm not positive about my comment here (and I'd be interested to see what others thing), but I think that the IDP abstraction here is a layer too high. There are only two keystone calls being made - get and list - and I think those are the functions that should be abstracted out. This allows a lot of the logic duplicated in the dummy and keystone IDP to be un-duplicated.

In my opinion, I think the right path would be to:

change the IDPs to only abstract out project get and list
rename common/keystone.py to something like common/idp.py
have common/idp.py call the two IDP functions

larsks · 2024-08-26T21:25:08Z

Is this PR still being considered?

@QuanMPhm Yes, the past couple of weeks have just been a bit crazy. I'll try to take a look through the PR tomorrow.

QuanMPhm · 2024-10-15T17:43:46Z

@tzumainn I abstracted the IDP instead of the individual Keystone functions based on @larsks' suggestion in the original issue (#159). Performing a text search over the upstream/master branch, it seems that every function in common/keystone.py was being called somewhere in esi-leap, which meant that all of those functions had to be abstracted away and implemented differently for KeystoneIDP and DummyIDP.

Thinking about your suggestion a bit more, I can sort of understand that the only function in common/keystone.py that is strongly coupled with the Keystone service is get_keystone_client, and I can see how it may also make sense to only abstract that function, instead of the entire common/keystone.py. @tzumainn @larsks I will follow whichever approach you guys prefer.

I have revised my PR to make it more complete and to make my vision of the final PR more clear. I've split the PR into 2 commits, since one only involves moving two functions around. I've also updated the commit message to better reflect the changes I added. Hopefully that will make this mess of a PR easier to follow.

tzumainn · 2024-10-15T19:31:16Z

@tzumainn I abstracted the IDP instead of the individual Keystone functions based on @larsks' suggestion in the original issue (#159). Performing a text search over the upstream/master branch, it seems that every function in common/keystone.py was being called somewhere in esi-leap, which meant that all of those functions had to be abstracted away and implemented differently for KeystoneIDP and DummyIDP.

Thinking about your suggestion a bit more, I can sort of understand that the only function in common/keystone.py that is strongly coupled with the Keystone service is get_keystone_client, and I can see how it may also make sense to only abstract that function, instead of the entire common/keystone.py. @tzumainn @larsks I will follow whichever approach you guys prefer.

I have revised my PR to make it more complete and to make my vision of the final PR more clear. I've split the PR into 2 commits, since one only involves moving two functions around. I've also updated the commit message to better reflect the changes I added. Hopefully that will make this mess of a PR easier to follow.

Every function in common/keystone.py is definitely being called somewhere in esi-leap, but that's only because we're the ones that created those functions.

It's important to recognize that get_keystone_client is a strong point of commonality, but by itself it's only the means to an end. The client by itself is not useful; the fact that we can call client.projects.get or client.projects.list is.

That's why I'd strongly recommend those two functions being the point of abstraction; they represent an actual API call to keystone, and they are fundamentally what need to be abstracted if we want to replace keystone with an arbitrary IDP.

Thinking about it another way: right now your implementation abstracts out the functions in common/keystone.py, such as get_parent_project_id_tree and get_project_name. However much of those functions consist of ESI logic, which is now duplicated in each of your IDP implementations. If we instead abstract out the Keystone call that both of them use - client.projects.get - then we get rid of that duplication in logic.

during testing The identity provider used by `esi_leap` is now abstracted with a `BaseIDP` ABC, and implemented by two subclasses: A `KeystoneIDP` class which mostly copied the code from `common/keystone.py`, and `DummyIDP` which mocks a real IDP The IDP used by `esi_leap` can be set by overriding the `idp_plugin_class` CONF value like so: CONF.set_override( "idp_plugin_class", "esi_leap.common.idp.dummyIDP.DummyIDP", group="esi" ) As a consequence of abstracting the IDP, `common/keystone.py` has been removed. All references to `common/keystone.py` and the functions defined in it has been appropriately changed.

…ler`.

QuanMPhm · 2024-10-22T17:31:40Z

@tzumainn I have implemented your feedback, and will wait for further instructions before turning this draft into a PR.

tzumainn

Hi! Just a few comments inline. In addition, test_keystone.py should probably be renamed test_idp.py, and you'll want unit tests for both the keystone and dummy implementation. The resource_objects do something similar.

tzumainn · 2024-11-05T19:16:30Z

esi_leap/common/idp.py

@@ -23,25 +22,26 @@
 _cached_project_list = None


-def get_keystone_client():
+def get_idp_client():
    global _cached_keystone_client


it probably makes sense to rename this to _cached_idp_client

tzumainn · 2024-11-05T19:21:52Z

esi_leap/common/idp.py

+    # Get Client config option
+    module_path, class_name = CONF.esi.idp_plugin_class.rsplit(".", 1)
+    module = importlib.import_module(module_path)
+    cli = getattr(module, class_name)()


This feels a little delicate - can you do something similar to what we do for resource objects? (https://github.com/CCI-MOC/esi-leap/blob/master/esi_leap/objects/lease.py#L313 which calls https://github.com/CCI-MOC/esi-leap/blob/master/esi_leap/resource_objects/__init__.py#L34)

QuanMPhm requested review from larsks, tzumainn and knikolla July 10, 2024 15:56

larsks reviewed Jul 10, 2024

View reviewed changes

esi_leap/common/idp/__init__.py Outdated Show resolved Hide resolved

esi_leap/common/idp/baseIDP.py Outdated Show resolved Hide resolved

esi_leap/common/idp/dummyIDP.py Outdated Show resolved Hide resolved

QuanMPhm force-pushed the 159/remove_keystone_dep branch from 217fddd to 2e7a9bb Compare July 12, 2024 18:36

knikolla reviewed Jul 16, 2024

View reviewed changes

esi_leap/common/idp/dummyIDP.py Outdated Show resolved Hide resolved

QuanMPhm force-pushed the 159/remove_keystone_dep branch from 2e7a9bb to 3288e63 Compare July 19, 2024 18:58

QuanMPhm force-pushed the 159/remove_keystone_dep branch from 3288e63 to 488dedd Compare July 22, 2024 19:08

QuanMPhm force-pushed the 159/remove_keystone_dep branch from 488dedd to 0acb060 Compare July 30, 2024 18:03

QuanMPhm requested a review from larsks August 14, 2024 18:41

QuanMPhm force-pushed the 159/remove_keystone_dep branch from 0acb060 to 787749e Compare October 15, 2024 17:44

QuanMPhm added 2 commits October 22, 2024 13:29

Moved some utility functions into LeaseController and `OfferControl…

9941af3

…ler`.

QuanMPhm force-pushed the 159/remove_keystone_dep branch from 787749e to 9941af3 Compare October 22, 2024 17:30

tzumainn requested changes Nov 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added dummy identity provider to remove Keystone dependancy during testing #162

Added dummy identity provider to remove Keystone dependancy during testing #162

QuanMPhm commented Jul 10, 2024

QuanMPhm commented Jul 12, 2024

larsks commented Jul 15, 2024

QuanMPhm commented Jul 19, 2024

QuanMPhm commented Jul 22, 2024 •

edited

Loading

QuanMPhm commented Aug 26, 2024

tzumainn commented Aug 26, 2024

larsks commented Aug 26, 2024

QuanMPhm commented Oct 15, 2024

tzumainn commented Oct 15, 2024

QuanMPhm commented Oct 22, 2024

tzumainn left a comment

tzumainn Nov 5, 2024

tzumainn Nov 5, 2024

Added dummy identity provider to remove Keystone dependancy during testing #162

Are you sure you want to change the base?

Added dummy identity provider to remove Keystone dependancy during testing #162

Conversation

QuanMPhm commented Jul 10, 2024

QuanMPhm commented Jul 12, 2024

larsks commented Jul 15, 2024

QuanMPhm commented Jul 19, 2024

QuanMPhm commented Jul 22, 2024 • edited Loading

QuanMPhm commented Aug 26, 2024

tzumainn commented Aug 26, 2024

larsks commented Aug 26, 2024

QuanMPhm commented Oct 15, 2024

tzumainn commented Oct 15, 2024

QuanMPhm commented Oct 22, 2024

tzumainn left a comment

Choose a reason for hiding this comment

tzumainn Nov 5, 2024

Choose a reason for hiding this comment

tzumainn Nov 5, 2024

Choose a reason for hiding this comment

QuanMPhm commented Jul 22, 2024 •

edited

Loading