Added dummy identity provider to remove Keystone dependancy during testing

[QuanMPhm]

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.

[QuanMPhm]

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]

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.

[QuanMPhm]

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]

@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]

@larsks Is this PR still being considered?

[tzumainn]

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]

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]

@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]

@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.

[QuanMPhm]

@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]

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]

@tzumainn I have addressed your comments so far