From 79eed763d0acc8eb0c2401f6328988d17fb1d402 Mon Sep 17 00:00:00 2001 From: Gavin Date: Tue, 26 Jan 2021 17:29:54 -0800 Subject: [PATCH 01/12] pass a UserClientSet into SDKMethodRunner UCS contains the clients that need to be instantiated on a per user basis. UCS is passed in to SDKMR to allow for eventual unit testing of SDKMR, since the UCS and workspace client can be mocked via create_autospec. --- lib/execution_engine2/constants.py | 5 + .../execution_engine2Impl.py | 72 +++++++----- .../sdk/EE2Authentication.py | 17 ++- lib/execution_engine2/sdk/EE2Runjob.py | 4 +- lib/execution_engine2/sdk/EE2Status.py | 5 +- lib/execution_engine2/sdk/SDKMethodRunner.py | 35 ++---- lib/execution_engine2/utils/clients.py | 111 ++++++++++++++++++ test/tests_for_auth/ee2_admin_mode_test.py | 36 ++++-- .../ee2_SDKMethodRunner_EE2Logs_test.py | 3 +- .../ee2_SDKMethodRunner_test.py | 21 ++-- ...ee2_SDKMethodRunner_test_EE2Runjob_test.py | 3 +- ...ee2_SDKMethodRunner_test_EE2Status_test.py | 3 +- test/tests_for_sdkmr/ee2_load_test.py | 5 +- test/tests_for_utils/clients_test.py | 22 ++++ test/utils_shared/test_utils.py | 5 + 15 files changed, 248 insertions(+), 99 deletions(-) create mode 100644 lib/execution_engine2/constants.py create mode 100644 lib/execution_engine2/utils/clients.py create mode 100644 test/tests_for_utils/clients_test.py diff --git a/lib/execution_engine2/constants.py b/lib/execution_engine2/constants.py new file mode 100644 index 000000000..073f60065 --- /dev/null +++ b/lib/execution_engine2/constants.py @@ -0,0 +1,5 @@ +''' +Global EE2 constants. +''' +ADMIN_READ_ROLE = "EE2_ADMIN_RO" +ADMIN_WRITE_ROLE = "EE2_ADMIN" diff --git a/lib/execution_engine2/execution_engine2Impl.py b/lib/execution_engine2/execution_engine2Impl.py index fdd718e89..c8846b690 100644 --- a/lib/execution_engine2/execution_engine2Impl.py +++ b/lib/execution_engine2/execution_engine2Impl.py @@ -6,6 +6,7 @@ from lib.execution_engine2.db.MongoUtil import MongoUtil from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner +from execution_engine2.utils.clients import UserClientSet #END_HEADER @@ -41,6 +42,10 @@ class execution_engine2: ADMIN_ROLES_CACHE_SIZE = 500 ADMIN_ROLES_CACHE_EXPIRE_TIME = 300 # seconds + + def get_user_clients(self, ctx) -> UserClientSet: + return UserClientSet(self.config, ctx['user_id'], ctx['token']) + #END_CLASS_HEADER # config contains contents of config file in a hash or None if it couldn't @@ -227,7 +232,8 @@ def run_job(self, ctx, params): # return variables are: job_id #BEGIN run_job mr = SDKMethodRunner( - self.config, user_id=ctx.get("user_id"), token=ctx.get("token"), + self.config, + user_clients=self.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util ) @@ -298,7 +304,8 @@ def run_job_batch(self, ctx, params, batch_params): # return variables are: job_ids #BEGIN run_job_batch mr = SDKMethodRunner( - self.config, user_id=ctx.get("user_id"), token=ctx.get("token"), + self.config, + user_clients=self.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util ) @@ -326,7 +333,8 @@ def abandon_children(self, ctx, params): # return variables are: parent_and_child_ids #BEGIN abandon_children mr = SDKMethodRunner( - self.config, user_id=ctx.get("user_id"), token=ctx.get("token"), + self.config, + user_clients=self.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util ) @@ -408,7 +416,8 @@ def run_job_concierge(self, ctx, params, concierge_params): # return variables are: job_id #BEGIN run_job_concierge mr = SDKMethodRunner( - self.config, user_id=ctx.get("user_id"), token=ctx.get("token"), + self.config, + user_clients=self.get_user_clients(ctx), mongo_util=self.mongo_util ) job_id = mr.run_job_concierge(params=params,concierge_params=concierge_params) @@ -478,8 +487,7 @@ def get_job_params(self, ctx, params): #BEGIN get_job_params mr = SDKMethodRunner( self.config, - user_id=ctx.get("user_id"), - token=ctx.get("token"), + user_clients=self.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -508,8 +516,7 @@ def update_job_status(self, ctx, params): #BEGIN update_job_status mr = SDKMethodRunner( self.config, - user_id=ctx.get("user_id"), - token=ctx.get("token"), + user_clients=self.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -547,8 +554,7 @@ def add_job_logs(self, ctx, params, lines): #BEGIN add_job_logs mr = SDKMethodRunner( self.config, - user_id=ctx.get("user_id"), - token=ctx.get("token"), + user_clients=self.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -599,8 +605,7 @@ def get_job_logs(self, ctx, params): mr = SDKMethodRunner( self.config, - user_id=ctx.get("user_id"), - token=ctx.get("token"), + user_clients=self.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -641,8 +646,7 @@ def finish_job(self, ctx, params): #BEGIN finish_job mr = SDKMethodRunner( self.config, - user_id=ctx.get("user_id"), - token=ctx.get("token"), + user_clients=self.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -671,8 +675,7 @@ def start_job(self, ctx, params): #BEGIN start_job mr = SDKMethodRunner( self.config, - user_id=ctx.get("user_id"), - token=ctx.get("token"), + user_clients=self.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -784,7 +787,8 @@ def check_job(self, ctx, params): # return variables are: job_state #BEGIN check_job mr = SDKMethodRunner( - self.config, user_id=ctx.get("user_id"), token=ctx.get("token"), + self.config, + user_clients=self.get_user_clients(ctx), mongo_util=self.mongo_util ) job_state = mr.check_job( @@ -990,7 +994,8 @@ def check_job_batch(self, ctx, params): # return variables are: returnVal #BEGIN check_job_batch mr = SDKMethodRunner( - self.config, user_id=ctx.get("user_id"), token=ctx.get("token"), + self.config, + user_clients=self.get_user_clients(ctx), mongo_util=self.mongo_util ) returnVal = mr.check_job_batch( @@ -1108,7 +1113,8 @@ def check_jobs(self, ctx, params): # return variables are: returnVal #BEGIN check_jobs mr = SDKMethodRunner( - self.config, user_id=ctx.get("user_id"), token=ctx.get("token"), + self.config, + user_clients=self.get_user_clients(ctx), mongo_util=self.mongo_util ) returnVal = mr.check_jobs( @@ -1228,7 +1234,8 @@ def check_workspace_jobs(self, ctx, params): # ctx is the context object # return variables are: returnVal #BEGIN check_workspace_jobs - mr = SDKMethodRunner(self.config, user_id=ctx["user_id"], token=ctx["token"], + mr = SDKMethodRunner(self.config, + user_clients=self.get_user_clients(ctx), mongo_util=self.mongo_util) returnVal = mr.check_workspace_jobs( params.get("workspace_id"), @@ -1261,8 +1268,7 @@ def cancel_job(self, ctx, params): #BEGIN cancel_job mr = SDKMethodRunner( self.config, - user_id=ctx.get("user_id"), - token=ctx.get("token"), + user_clients=self.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -1300,7 +1306,8 @@ def check_job_canceled(self, ctx, params): # return variables are: result #BEGIN check_job_canceled mr = SDKMethodRunner( - self.config, user_id=ctx.get("user_id"), token=ctx.get("token"), + self.config, + user_clients=self.get_user_clients(ctx), mongo_util=self.mongo_util ) result = mr.check_job_canceled(job_id=params["job_id"], as_admin=params.get('as_admin')) @@ -1327,8 +1334,7 @@ def get_job_status(self, ctx, params): #BEGIN get_job_status mr = SDKMethodRunner( self.config, - user_id=ctx.get("user_id"), - token=ctx.get("token"), + user_clients=self.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -1455,7 +1461,8 @@ def check_jobs_date_range_for_user(self, ctx, params): # return variables are: returnVal #BEGIN check_jobs_date_range_for_user mr = SDKMethodRunner( - self.config, user_id=ctx.get("user_id"), token=ctx.get("token"), + self.config, + user_clients=self.get_user_clients(ctx), mongo_util=self.mongo_util ) returnVal = mr.check_jobs_date_range_for_user( @@ -1590,7 +1597,8 @@ def check_jobs_date_range_for_all(self, ctx, params): # return variables are: returnVal #BEGIN check_jobs_date_range_for_all mr = SDKMethodRunner( - self.config, user_id=ctx.get("user_id"), token=ctx.get("token"), + self.config, + user_clients=self.get_user_clients(ctx), mongo_util=self.mongo_util ) returnVal = mr.check_jobs_date_range_for_user( @@ -1624,7 +1632,8 @@ def handle_held_job(self, ctx, cluster_id): # return variables are: returnVal #BEGIN handle_held_job mr = SDKMethodRunner( - self.config, user_id=ctx.get("user_id"), token=ctx.get("token"), + self.config, + user_clients=self.get_user_clients(ctx), mongo_util=self.mongo_util ) returnVal = mr.handle_held_job(cluster_id=cluster_id) @@ -1646,7 +1655,8 @@ def is_admin(self, ctx): # return variables are: returnVal #BEGIN is_admin mr = SDKMethodRunner( - self.config, user_id=ctx.get("user_id"), token=ctx.get("token"), + self.config, + user_clients=self.get_user_clients(ctx), mongo_util=self.mongo_util ) returnVal = mr.check_is_admin() @@ -1671,7 +1681,8 @@ def get_admin_permission(self, ctx): # return variables are: returnVal #BEGIN get_admin_permission mr = SDKMethodRunner( - self.config, user_id=ctx.get("user_id"), token=ctx.get("token"), + self.config, + user_clients=self.get_user_clients(ctx), mongo_util=self.mongo_util ) returnVal = mr.get_admin_permission() @@ -1692,6 +1703,7 @@ def get_client_groups(self, ctx): # ctx is the context object # return variables are: client_groups #BEGIN get_client_groups + # TODO I think this need to be actually extracted from the config file client_groups = ['njs', 'bigmem', 'bigmemlong', 'extreme', 'concierge', 'hpc', 'kb_upload', 'terabyte', 'multi_tb', 'kb_upload_bulk'] #END get_client_groups diff --git a/lib/execution_engine2/sdk/EE2Authentication.py b/lib/execution_engine2/sdk/EE2Authentication.py index c79c66cb5..bcb297f5d 100644 --- a/lib/execution_engine2/sdk/EE2Authentication.py +++ b/lib/execution_engine2/sdk/EE2Authentication.py @@ -5,6 +5,7 @@ from lib.execution_engine2.authorization.authstrategy import can_read_job, can_write_job from lib.execution_engine2.authorization.roles import AdminAuthUtil from lib.execution_engine2.db.models.models import Job +from execution_engine2.constants import ADMIN_READ_ROLE, ADMIN_WRITE_ROLE class JobPermissions(Enum): @@ -33,12 +34,12 @@ def _lookup_admin_permissions(self): aau = AdminAuthUtil(self.sdkmr.auth_url, self.sdkmr.admin_roles) p = aau.get_admin_role( token=self.sdkmr.token, - read_role=self.sdkmr.ADMIN_READ_ROLE, - write_role=self.sdkmr.ADMIN_WRITE_ROLE, + read_role=ADMIN_READ_ROLE, + write_role=ADMIN_WRITE_ROLE, ) - if p == self.sdkmr.ADMIN_READ_ROLE: + if p == ADMIN_READ_ROLE: return AdminPermissions.READ - elif p == self.sdkmr.ADMIN_WRITE_ROLE: + elif p == ADMIN_WRITE_ROLE: return AdminPermissions.WRITE else: return AdminPermissions.NONE @@ -149,16 +150,12 @@ def test_job_permissions( perm = False try: if level.value == JobPermissions.READ.value: - perm = can_read_job( - job, self.sdkmr.user_id, self.sdkmr.get_workspace_auth() - ) + perm = can_read_job(job, self.sdkmr.user_id, self.sdkmr.workspace_auth) self._update_job_permission_cache( job_id, self.sdkmr.user_id, level, perm ) elif level.value == JobPermissions.WRITE.value: - perm = can_write_job( - job, self.sdkmr.user_id, self.sdkmr.get_workspace_auth() - ) + perm = can_write_job(job, self.sdkmr.user_id, self.sdkmr.workspace_auth) self._update_job_permission_cache( job_id, self.sdkmr.user_id, level, perm ) diff --git a/lib/execution_engine2/sdk/EE2Runjob.py b/lib/execution_engine2/sdk/EE2Runjob.py index 5ffcf01d6..33c259f44 100644 --- a/lib/execution_engine2/sdk/EE2Runjob.py +++ b/lib/execution_engine2/sdk/EE2Runjob.py @@ -143,7 +143,7 @@ def _check_ws_objects(self, source_objects) -> None: def _check_workspace_permissions(self, wsid): if wsid: - if not self.sdkmr.get_workspace_auth().can_write(wsid): + if not self.sdkmr.workspace_auth.can_write(wsid): self.logger.debug( f"User {self.sdkmr.user_id} doesn't have permission to run jobs in workspace {wsid}." ) @@ -152,7 +152,7 @@ def _check_workspace_permissions(self, wsid): ) def _check_workspace_permissions_list(self, wsids): - perms = self.sdkmr.get_workspace_auth().can_write_list(wsids) + perms = self.sdkmr.workspace_auth.can_write_list(wsids) bad_ws = [key for key in perms.keys() if perms[key] is False] if bad_ws: self.logger.debug( diff --git a/lib/execution_engine2/sdk/EE2Status.py b/lib/execution_engine2/sdk/EE2Status.py index 79f70f88b..c8e4c16ac 100644 --- a/lib/execution_engine2/sdk/EE2Status.py +++ b/lib/execution_engine2/sdk/EE2Status.py @@ -445,7 +445,7 @@ def check_jobs( "Checking for read permission to: {}".format(job_ids) ) perms = can_read_jobs( - jobs, self.sdkmr.user_id, self.sdkmr.get_workspace_auth() + jobs, self.sdkmr.user_id, self.sdkmr.workspace_auth ) except RuntimeError as e: self.sdkmr.logger.error( @@ -502,8 +502,7 @@ def check_workspace_jobs(self, workspace_id, exclude_fields=None, return_list=No if exclude_fields is None: exclude_fields = [] - ws_auth = self.sdkmr.get_workspace_auth() - if not ws_auth.can_read(workspace_id): + if not self.sdkmr.workspace_auth.can_read(workspace_id): self.sdkmr.logger.debug( f"User {self.sdkmr.user_id} doesn't have permission to read jobs in workspace {workspace_id}." ) diff --git a/lib/execution_engine2/sdk/SDKMethodRunner.py b/lib/execution_engine2/sdk/SDKMethodRunner.py index 2a14d8b3a..340f3d314 100644 --- a/lib/execution_engine2/sdk/SDKMethodRunner.py +++ b/lib/execution_engine2/sdk/SDKMethodRunner.py @@ -19,6 +19,7 @@ from installed_clients.WorkspaceClient import Workspace from installed_clients.authclient import KBaseAuth from lib.execution_engine2.authorization.workspaceauth import WorkspaceAuth +from execution_engine2.constants import ADMIN_WRITE_ROLE, ADMIN_READ_ROLE from lib.execution_engine2.db.MongoUtil import MongoUtil from lib.execution_engine2.db.models.models import Job from lib.execution_engine2.exceptions import AuthError @@ -35,6 +36,7 @@ from lib.execution_engine2.utils.EE2Logger import get_logger from lib.execution_engine2.utils.KafkaUtils import KafkaClient from lib.execution_engine2.utils.SlackUtils import SlackClient +from execution_engine2.utils.clients import UserClientSet class JobPermissions(Enum): @@ -53,37 +55,31 @@ class SDKMethodRunner: """ JOB_PERMISSION_CACHE_SIZE = 500 JOB_PERMISSION_CACHE_EXPIRE_TIME = 300 # seconds - ADMIN_READ_ROLE = "EE2_ADMIN_RO" - ADMIN_WRITE_ROLE = "EE2_ADMIN" def __init__( self, config, - user_id: str, - token: str, + user_clients: UserClientSet, job_permission_cache=None, admin_permissions_cache=None, mongo_util=None, ): - if not user_id or not user_id.strip(): - raise ValueError("user_id is required") - if not token or not token.strip(): - raise ValueError("token is required") + if not user_clients: + raise ValueError("user_clients is required") self.deployment_config_fp = os.environ["KB_DEPLOYMENT_CONFIG"] self.config = config self.mongo_util = mongo_util self.condor = None - self.workspace = None - self.workspace_auth = None + self.workspace = user_clients.workspace() + self.workspace_auth = user_clients.workspace_auth() self.admin_roles = config.get("admin_roles", ["EE2_ADMIN", "EE2_ADMIN_RO"]) self.catalog_utils = CatalogUtils( config["catalog-url"], config["catalog-token"] ) - self.workspace_url = config.get("workspace-url") self.auth_url = config.get("auth-url") self.auth = KBaseAuth(auth_url=config.get("auth-service-url")) - self.user_id = user_id - self.token = token + self.user_id = user_clients.user_id() + self.token = user_clients.token() self.debug = SDKMethodRunner.parse_bool_from_string(config.get("debug")) self.logger = get_logger() @@ -137,11 +133,6 @@ def get_jobs_status(self) -> EE2Status.JobsStatus: self._ee2_status = EE2Status.JobsStatus(self) return self._ee2_status - def get_workspace_auth(self) -> WorkspaceAuth: - if self.workspace_auth is None: - self.workspace_auth = WorkspaceAuth(self.user_id, self.get_workspace()) - return self.workspace_auth - def get_mongo_util(self) -> MongoUtil: if self.mongo_util is None: self.mongo_util = MongoUtil(self.config) @@ -152,11 +143,6 @@ def get_condor(self) -> Condor: self.condor = Condor(self.deployment_config_fp) return self.condor - def get_workspace(self) -> Workspace: - if self.workspace is None: - self.workspace = Workspace(token=self.token, url=self.workspace_url) - return self.workspace - # Permissions Decorators #TODO Verify these actually work #TODO add as_admin to these def allow_job_read(func): @@ -454,8 +440,7 @@ def check_workspace_jobs( if as_admin: self.check_as_admin(requested_perm=JobPermissions.READ) else: - ws_auth = self.get_workspace_auth() - if not ws_auth.can_read(workspace_id): + if not self.workspace_auth.can_read(workspace_id): self.logger.debug( f"User {self.user_id} doesn't have permission to read jobs in workspace {workspace_id}." ) diff --git a/lib/execution_engine2/utils/clients.py b/lib/execution_engine2/utils/clients.py new file mode 100644 index 000000000..07f367566 --- /dev/null +++ b/lib/execution_engine2/utils/clients.py @@ -0,0 +1,111 @@ +''' +Contains the various clients EE2 needs to communicate with other services it depends on. +''' + +# Note on testing - this class is not generally unit-testable, and is only tested fully in +# integration tests. + +from typing import Dict + +from execution_engine2.authorization.roles import AdminAuthUtil +from execution_engine2.authorization.workspaceauth import WorkspaceAuth +from execution_engine2.utils.CatalogUtils import CatalogUtils +from execution_engine2.utils.Condor import Condor +from execution_engine2.constants import ADMIN_READ_ROLE, ADMIN_WRITE_ROLE +from execution_engine2.utils.KafkaUtils import KafkaClient +from execution_engine2.utils.SlackUtils import SlackClient + +from installed_clients.authclient import KBaseAuth +from installed_clients.WorkspaceClient import Workspace + + +class UserClientSet: + ''' + Clients rquired by EE2 for communicating with other services that need to be instantiated + on a per user basis. Also contains the user credentials for ease of use. + ''' + def __init__(self, cfg: Dict[str, str], user_id: str, token: str): + ''' + Initialize the client set. + + cfg - the configuration dictionary + user_id - the ID of the user to be used to initialize the client set. + token - the token of the user to be used to initialize the client set. Note that the set + trusts that the token actually belongs to the user ID, and currently does not independently + check the validity of the user ID. + + Expected keys in config: + workspace-url - the URL of the kbase workspace service + ''' + if not user_id or not user_id.strip(): + raise ValueError('user_id is required') + if not token or not token.strip(): + raise ValueError('token is required') + if not cfg: + raise ValueError('cfg is required') + self._user_id = user_id + self._token = token + + # Do a check that the url actually points to the workspace? + # Also maybe consider passing in the workspace url rather than the dict, but the ClientSet + # below will need lots of params so a dict makes sense there, maybe keep the apis similar? + # ws_url = cfg.get('workspace_url') # may want to make the keys constants? + # if not ws_url or not ws_url.strip: + # raise ValueError('missing workspace-url key in configuration') + # TODO Originally did the above check but caused 36 test failures so... meh for now. + self._workspace = Workspace(cfg['workspace-url'], token=token) + self._workspace_auth = WorkspaceAuth(user_id, self._workspace) + + # create_autospec can't mock instance variables, so we add some boilerplate + # can make these properties but then they return MagicMocks which don't update with API + # changes + + def user_id(self): + return self._user_id + + def token(self): + return self._token + + def workspace(self): + return self._workspace + + def workspace_auth(self): + return self._workspace_auth + + +class ClientSet: + ''' + Clients required by EE2 for communicating with other services. + + These are not user-specific and can be reused throughout the application. + ''' + + def __init__(self, cfg: Dict[str, str], cfg_path: str, debug: bool = False): + ''' + Initialize the client set from a configuration dictionary. + + cfg - the configuration dictionary + cfg_path - the path to the configuration file + debug - set clients that support it to debug mode + + Expected keys in config: + auth-url - the root URL of the kbase auth service + catalog-url - the URL of the catalog service + catalog-token - a token to use with the catalog service. Ideally a service token + kafka-host - the host string for a Kafka service + slack-token - a token for contacting Slack + ''' + # TODO seems like it'd make sense to init Condor from a config dict like everything else + self.condor = Condor(cfg_path) + self.catalog_utils = CatalogUtils(cfg['catalog-url'], cfg['catalog-token']) + auth_url = cfg['auth-url'] + self.auth = KBaseAuth(auth_url=auth_url + '/api/legacy/KBase/Sessions/Login') + # TODO using hardcoded roles for now to avoid possible bugs with mismatched cfg roles + # these should probably be configurable + self.auth_admin = AdminAuthUtil(auth_url, [ADMIN_READ_ROLE, ADMIN_WRITE_ROLE]) + + # KafkaClient has a nice error message when the arg is None + self.kafka_client = KafkaClient(cfg.get('kafka-host')) + # SlackClient handles None arguments + self.slack_client = SlackClient( + cfg.get("slack-token"), debug=debug, endpoint=cfg.get("ee2-url")) diff --git a/test/tests_for_auth/ee2_admin_mode_test.py b/test/tests_for_auth/ee2_admin_mode_test.py index abf89cbe3..a22127a3b 100644 --- a/test/tests_for_auth/ee2_admin_mode_test.py +++ b/test/tests_for_auth/ee2_admin_mode_test.py @@ -3,6 +3,8 @@ import unittest from configparser import ConfigParser +from unittest.mock import create_autospec + import bson from mock import MagicMock from mock import patch @@ -10,10 +12,12 @@ from installed_clients.CatalogClient import Catalog from lib.execution_engine2.authorization.roles import AdminAuthUtil from lib.execution_engine2.authorization.workspaceauth import WorkspaceAuth +from execution_engine2.constants import ADMIN_READ_ROLE, ADMIN_WRITE_ROLE from lib.execution_engine2.db.models.models import Status from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner from lib.execution_engine2.utils.Condor import Condor from lib.execution_engine2.utils.CondorTuples import SubmissionInfo +from execution_engine2.utils.clients import UserClientSet from test.utils_shared.test_utils import ( get_sample_job_params, get_sample_condor_info, @@ -41,7 +45,7 @@ def setUpClass(cls): cls.token = "token" cls.method_runner = SDKMethodRunner( - cls.cfg, user_id=cls.user_id, token=cls.token + cls.cfg, UserClientSet(cls.cfg, cls.user_id, cls.token) ) def setUp(self) -> None: @@ -82,11 +86,11 @@ def tearDown(self) -> None: self.condor_patch.stop() self.condor_patch2.start() - def getRunner(self) -> SDKMethodRunner: + def getRunner(self, user_clients=None) -> SDKMethodRunner: # Initialize these clients from None - runner = SDKMethodRunner( - self.cfg, user_id=self.user_id, token=self.token - ) # type : SDKMethodRunner + if not user_clients: + user_clients = UserClientSet(self.cfg, self.user_id, self.token) + runner = SDKMethodRunner(self.cfg, user_clients) # type : SDKMethodRunner runner.get_jobs_status() runner.get_runjob() runner.get_job_logs() @@ -104,13 +108,22 @@ def get_runner_with_condor(self) -> SDKMethodRunner: # TODO How do you test ADMIN_MODE without increasing too much coverage + def get_mocks(self) -> (UserClientSet, WorkspaceAuth): + # add workspace client mock if needed + ucs = create_autospec(UserClientSet, instance=True, spec_set=True) + wsa = create_autospec(WorkspaceAuth, instance=True, spec_set=True) + ucs.workspace_auth.return_value = wsa + return ucs, wsa + @patch.object(Catalog, "get_module_version", return_value="module.version") - @patch.object(WorkspaceAuth, "can_write", return_value=True) @patch.object(AdminAuthUtil, "_fetch_user_roles") - def test_regular_user(self, aau, workspace, catalog): + def test_regular_user(self, aau, catalog): # Regular User lowly_user = "Access Denied: You are not an administrator" - runner = self.getRunner() + ucs, wsa = self.get_mocks() + ucs.user_id.return_value = self.user_id + wsa.can_write.return_value = True + runner = self.getRunner(ucs) aau.return_value = ["RegularJoe"] method_1 = "module_name.function_name" job_params_1 = get_sample_job_params(method=method_1, wsid=self.ws_id) @@ -127,6 +140,7 @@ def test_regular_user(self, aau, workspace, catalog): job_id = runner.run_job(params=job_params_1, as_admin=False) self.assertTrue(bson.objectid.ObjectId.is_valid(job_id)) + wsa.can_write.assert_called_once_with(self.ws_id) # RUNJOB BUT ATTEMPT TO BE AN ADMIN with self.assertRaisesRegexp( @@ -189,7 +203,7 @@ def test_admin_writer(self, aau, workspace, catalog): # Admin User with WRITE runner = self.getRunner() - aau.return_value = [runner.ADMIN_READ_ROLE] + aau.return_value = [ADMIN_READ_ROLE] method_1 = "module_name.function_name" job_params_1 = get_sample_job_params(method=method_1, wsid=self.ws_id) @@ -201,7 +215,7 @@ def test_admin_writer(self, aau, workspace, catalog): runner = self.getRunner() # SET YOUR ADMIN STATUS HERE - aau.return_value = [runner.ADMIN_WRITE_ROLE] + aau.return_value = [ADMIN_WRITE_ROLE] method_1 = "module_name.function_name" job_params_1 = get_sample_job_params(method=method_1, wsid=self.ws_id) @@ -247,7 +261,7 @@ def test_admin_reader(self, aau): # Admin User with WRITE lowly_admin = r"Access Denied: You are a read-only admin. This function requires write access" runner = self.getRunner() - aau.return_value = [runner.ADMIN_READ_ROLE] + aau.return_value = [ADMIN_READ_ROLE] method_1 = "module_name.function_name" job_params_1 = get_sample_job_params(method=method_1, wsid=self.ws_id) diff --git a/test/tests_for_sdkmr/ee2_SDKMethodRunner_EE2Logs_test.py b/test/tests_for_sdkmr/ee2_SDKMethodRunner_EE2Logs_test.py index a70178c41..eb83c60b9 100644 --- a/test/tests_for_sdkmr/ee2_SDKMethodRunner_EE2Logs_test.py +++ b/test/tests_for_sdkmr/ee2_SDKMethodRunner_EE2Logs_test.py @@ -10,6 +10,7 @@ from lib.execution_engine2.db.MongoUtil import MongoUtil from lib.execution_engine2.db.models.models import Job, JobLog from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner +from execution_engine2.utils.clients import UserClientSet from test.utils_shared.test_utils import ( bootstrap, run_job_adapter, @@ -35,7 +36,7 @@ def setUpClass(cls): cls.token = "token" cls.method_runner = SDKMethodRunner( - cls.cfg, user_id=cls.user_id, token=cls.token + cls.cfg, UserClientSet(cls.cfg, cls.user_id, cls.token) ) cls.mongo_util = MongoUtil(cls.cfg) cls.mongo_helper = MongoTestHelper(cls.cfg) diff --git a/test/tests_for_sdkmr/ee2_SDKMethodRunner_test.py b/test/tests_for_sdkmr/ee2_SDKMethodRunner_test.py index 30d23299d..220cb1780 100644 --- a/test/tests_for_sdkmr/ee2_SDKMethodRunner_test.py +++ b/test/tests_for_sdkmr/ee2_SDKMethodRunner_test.py @@ -23,12 +23,14 @@ from lib.execution_engine2.exceptions import InvalidStatusTransitionException from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner from lib.execution_engine2.utils.CondorTuples import SubmissionInfo, CondorResources +from execution_engine2.utils.clients import UserClientSet from test.tests_for_sdkmr.ee2_SDKMethodRunner_test_utils import ee2_sdkmr_test_helper from test.utils_shared.test_utils import ( bootstrap, get_example_job, validate_job_state, run_job_adapter, + assert_exception_correct, ) from tests_for_db.mongo_test_helper import MongoTestHelper @@ -62,7 +64,7 @@ def setUpClass(cls): cls.token = "token" cls.method_runner = SDKMethodRunner( - cls.cfg, user_id=cls.user_id, token=cls.token + cls.cfg, UserClientSet(cls.cfg, cls.user_id, cls.token) ) cls.mongo_util = MongoUtil(cls.cfg) cls.mongo_helper = MongoTestHelper(cls.cfg) @@ -123,20 +125,13 @@ def create_job_rec(self): # self.assertEqual(len(git_commit_1), len(git_commit_2)) # self.assertNotEqual(git_commit_1, git_commit_2) - def assert_exception_correct(self, got: Exception, expected: Exception): - assert got.args == expected.args - assert type(got) == type(expected) - def test_init_fail(self): - self._init_fail({}, None, "foo", ValueError("user_id is required")) - self._init_fail({}, " \t ", "foo", ValueError("user_id is required")) - self._init_fail({}, "user", None, ValueError("token is required")) - self._init_fail({}, "user", " \t ", ValueError("token is required")) + self._init_fail({}, None, ValueError("user_clients is required")) - def _init_fail(self, cfg, user, token, expected): + def _init_fail(self, cfg, user_clients, expected): with raises(Exception) as e: - SDKMethodRunner(cfg, user, token) - self.assert_exception_correct(e.value, expected) + SDKMethodRunner(cfg, user_clients) + assert_exception_correct(e.value, expected) # Status @patch("lib.execution_engine2.utils.Condor.Condor", autospec=True) @@ -675,7 +670,7 @@ def test_check_job_global_perm(self, rq_mock): # now test with a different user other_method_runner = SDKMethodRunner( - self.cfg, user_id="some_other_user", token="other_token" + self.cfg, UserClientSet(self.cfg, "some_other_user", "other_token") ) job_states = other_method_runner.get_jobs_status().check_workspace_jobs( self.ws_id diff --git a/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Runjob_test.py b/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Runjob_test.py index 039edfdfa..c3d95024c 100644 --- a/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Runjob_test.py +++ b/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Runjob_test.py @@ -13,6 +13,7 @@ from lib.execution_engine2.db.models.models import Job from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner from lib.execution_engine2.utils.CondorTuples import SubmissionInfo, CondorResources +from execution_engine2.utils.clients import UserClientSet from test.utils_shared.test_utils import ( bootstrap, get_example_job, @@ -50,7 +51,7 @@ def setUpClass(cls): cls.token = "token" cls.method_runner = SDKMethodRunner( - cls.cfg, user_id=cls.user_id, token=cls.token + cls.cfg, UserClientSet(cls.cfg, cls.user_id, cls.token) ) cls.mongo_util = MongoUtil(cls.cfg) diff --git a/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Status_test.py b/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Status_test.py index c34ec4f0a..4b2a4defb 100644 --- a/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Status_test.py +++ b/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Status_test.py @@ -13,6 +13,7 @@ from lib.execution_engine2.db.models.models import Job from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner from lib.execution_engine2.utils.CondorTuples import SubmissionInfo, CondorResources +from execution_engine2.utils.clients import UserClientSet from test.tests_for_sdkmr.ee2_SDKMethodRunner_test_utils import ee2_sdkmr_test_helper from test.utils_shared.test_utils import bootstrap, get_example_job @@ -45,7 +46,7 @@ def setUpClass(cls): cls.token = "token" cls.method_runner = SDKMethodRunner( - cls.cfg, user_id=cls.user_id, token=cls.token + cls.cfg, UserClientSet(cls.cfg, cls.user_id, cls.token) ) cls.cr = CondorResources( request_cpus="1", diff --git a/test/tests_for_sdkmr/ee2_load_test.py b/test/tests_for_sdkmr/ee2_load_test.py index 3671d2c1e..031f985bb 100644 --- a/test/tests_for_sdkmr/ee2_load_test.py +++ b/test/tests_for_sdkmr/ee2_load_test.py @@ -9,7 +9,7 @@ from configparser import ConfigParser from unittest.mock import patch -from lib.execution_engine2.authorization.workspaceauth import WorkspaceAuth +from execution_engine2.authorization.workspaceauth import WorkspaceAuth from lib.execution_engine2.db.MongoUtil import MongoUtil from lib.execution_engine2.db.models.models import Job, Status from lib.execution_engine2.execution_engine2Impl import execution_engine2 @@ -17,6 +17,7 @@ from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner from lib.execution_engine2.utils.Condor import Condor from lib.execution_engine2.utils.CondorTuples import SubmissionInfo +from execution_engine2.utils.clients import UserClientSet from test.utils_shared.test_utils import ( bootstrap, get_sample_job_params, @@ -43,7 +44,7 @@ def setUpClass(cls): cls.ctx = {"token": cls.token, "user_id": cls.user_id} cls.impl = execution_engine2(cls.cfg) cls.method_runner = SDKMethodRunner( - cls.cfg, user_id=cls.user_id, token=cls.token + cls.cfg, UserClientSet(cls.cfg, cls.user_id, cls.token) ) cls.mongo_util = MongoUtil(cls.cfg) cls.mongo_helper = MongoTestHelper(cls.cfg) diff --git a/test/tests_for_utils/clients_test.py b/test/tests_for_utils/clients_test.py new file mode 100644 index 000000000..add67fdcc --- /dev/null +++ b/test/tests_for_utils/clients_test.py @@ -0,0 +1,22 @@ +# This test only tests code that can be exercised without a network connection to services. +# That code is tested in integration tests. + +from pytest import raises + +from execution_engine2.utils.clients import UserClientSet +from utils_shared.test_utils import assert_exception_correct + + +def test_user_client_set_init_fail(): + user_client_set_init_fail(None, 'foo', 'bar', ValueError('cfg is required')) + user_client_set_init_fail({}, 'foo', 'bar', ValueError('cfg is required')) + user_client_set_init_fail({'a': 'b'}, None, 'bar', ValueError('user_id is required')) + user_client_set_init_fail({'a': 'b'}, ' \t ', 'bar', ValueError('user_id is required')) + user_client_set_init_fail({'a': 'b'}, 'foo', None, ValueError('token is required')) + user_client_set_init_fail({'a': 'b'}, 'foo', ' \t ', ValueError('token is required')) + + +def user_client_set_init_fail(cfg, user, token, expected): + with raises(Exception) as e: + UserClientSet(cfg, user, token) + assert_exception_correct(e.value, expected) diff --git a/test/utils_shared/test_utils.py b/test/utils_shared/test_utils.py index a49517f11..a2c4fe7b2 100644 --- a/test/utils_shared/test_utils.py +++ b/test/utils_shared/test_utils.py @@ -379,3 +379,8 @@ def get_sample_job_params(method=None, wsid="123"): } return job_params + + +def assert_exception_correct(got: Exception, expected: Exception): + assert got.args == expected.args + assert type(got) == type(expected) From 5c01612474aa777e24740bbc9f4e1bcabe854edd Mon Sep 17 00:00:00 2001 From: Gavin Date: Thu, 28 Jan 2021 14:37:54 -0800 Subject: [PATCH 02/12] typos --- lib/execution_engine2/execution_engine2Impl.py | 2 +- lib/execution_engine2/utils/clients.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/execution_engine2/execution_engine2Impl.py b/lib/execution_engine2/execution_engine2Impl.py index c8846b690..6f08c8336 100644 --- a/lib/execution_engine2/execution_engine2Impl.py +++ b/lib/execution_engine2/execution_engine2Impl.py @@ -1703,7 +1703,7 @@ def get_client_groups(self, ctx): # ctx is the context object # return variables are: client_groups #BEGIN get_client_groups - # TODO I think this need to be actually extracted from the config file + # TODO I think this needs to be actually extracted from the config file client_groups = ['njs', 'bigmem', 'bigmemlong', 'extreme', 'concierge', 'hpc', 'kb_upload', 'terabyte', 'multi_tb', 'kb_upload_bulk'] #END get_client_groups diff --git a/lib/execution_engine2/utils/clients.py b/lib/execution_engine2/utils/clients.py index 07f367566..6524988a7 100644 --- a/lib/execution_engine2/utils/clients.py +++ b/lib/execution_engine2/utils/clients.py @@ -21,7 +21,7 @@ class UserClientSet: ''' - Clients rquired by EE2 for communicating with other services that need to be instantiated + Clients required by EE2 for communicating with other services that need to be instantiated on a per user basis. Also contains the user credentials for ease of use. ''' def __init__(self, cfg: Dict[str, str], user_id: str, token: str): @@ -57,7 +57,7 @@ def __init__(self, cfg: Dict[str, str], user_id: str, token: str): self._workspace_auth = WorkspaceAuth(user_id, self._workspace) # create_autospec can't mock instance variables, so we add some boilerplate - # can make these properties but then they return MagicMocks which don't update with API + # Could make these properties but then they return MagicMocks which don't update with API # changes def user_id(self): From b074b38f6916cc67b5f356a7d38ed978440f40a9 Mon Sep 17 00:00:00 2001 From: Gavin Date: Thu, 28 Jan 2021 15:08:58 -0800 Subject: [PATCH 03/12] I thought I ran pre-commmit on this but I guess not Also added pre-commit instructions --- README.md | 14 +++++++++ lib/execution_engine2/constants.py | 4 +-- lib/execution_engine2/utils/clients.py | 40 ++++++++++++++------------ test/tests_for_utils/clients_test.py | 18 ++++++++---- 4 files changed, 49 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index ea433f314..d371ba5f3 100644 --- a/README.md +++ b/README.md @@ -52,6 +52,20 @@ PYTHONPATH=.:lib:test pytest --cov-report=xml --cov lib/execution_engine2/ --ver ## To run a specific test file via PyCharm See [Testing with Pycharm](docs/testing_with_pycharm.md) +## To run pre-commit hooks + +`exec` into the docker container as before and switch to the `/ee2` directory. + +``` +pip install pre-commit +pre-commit install +pre-commit run --all-files +``` + +To remove the pre commit hooks: +``` +pre-commit uninstall +``` ## Test Running Options diff --git a/lib/execution_engine2/constants.py b/lib/execution_engine2/constants.py index 073f60065..52cace674 100644 --- a/lib/execution_engine2/constants.py +++ b/lib/execution_engine2/constants.py @@ -1,5 +1,5 @@ -''' +""" Global EE2 constants. -''' +""" ADMIN_READ_ROLE = "EE2_ADMIN_RO" ADMIN_WRITE_ROLE = "EE2_ADMIN" diff --git a/lib/execution_engine2/utils/clients.py b/lib/execution_engine2/utils/clients.py index 6524988a7..b3aee0ab7 100644 --- a/lib/execution_engine2/utils/clients.py +++ b/lib/execution_engine2/utils/clients.py @@ -1,6 +1,6 @@ -''' +""" Contains the various clients EE2 needs to communicate with other services it depends on. -''' +""" # Note on testing - this class is not generally unit-testable, and is only tested fully in # integration tests. @@ -20,12 +20,13 @@ class UserClientSet: - ''' + """ Clients required by EE2 for communicating with other services that need to be instantiated on a per user basis. Also contains the user credentials for ease of use. - ''' + """ + def __init__(self, cfg: Dict[str, str], user_id: str, token: str): - ''' + """ Initialize the client set. cfg - the configuration dictionary @@ -36,13 +37,13 @@ def __init__(self, cfg: Dict[str, str], user_id: str, token: str): Expected keys in config: workspace-url - the URL of the kbase workspace service - ''' + """ if not user_id or not user_id.strip(): - raise ValueError('user_id is required') + raise ValueError("user_id is required") if not token or not token.strip(): - raise ValueError('token is required') + raise ValueError("token is required") if not cfg: - raise ValueError('cfg is required') + raise ValueError("cfg is required") self._user_id = user_id self._token = token @@ -53,7 +54,7 @@ def __init__(self, cfg: Dict[str, str], user_id: str, token: str): # if not ws_url or not ws_url.strip: # raise ValueError('missing workspace-url key in configuration') # TODO Originally did the above check but caused 36 test failures so... meh for now. - self._workspace = Workspace(cfg['workspace-url'], token=token) + self._workspace = Workspace(cfg["workspace-url"], token=token) self._workspace_auth = WorkspaceAuth(user_id, self._workspace) # create_autospec can't mock instance variables, so we add some boilerplate @@ -74,14 +75,14 @@ def workspace_auth(self): class ClientSet: - ''' + """ Clients required by EE2 for communicating with other services. These are not user-specific and can be reused throughout the application. - ''' + """ def __init__(self, cfg: Dict[str, str], cfg_path: str, debug: bool = False): - ''' + """ Initialize the client set from a configuration dictionary. cfg - the configuration dictionary @@ -94,18 +95,19 @@ def __init__(self, cfg: Dict[str, str], cfg_path: str, debug: bool = False): catalog-token - a token to use with the catalog service. Ideally a service token kafka-host - the host string for a Kafka service slack-token - a token for contacting Slack - ''' + """ # TODO seems like it'd make sense to init Condor from a config dict like everything else self.condor = Condor(cfg_path) - self.catalog_utils = CatalogUtils(cfg['catalog-url'], cfg['catalog-token']) - auth_url = cfg['auth-url'] - self.auth = KBaseAuth(auth_url=auth_url + '/api/legacy/KBase/Sessions/Login') + self.catalog_utils = CatalogUtils(cfg["catalog-url"], cfg["catalog-token"]) + auth_url = cfg["auth-url"] + self.auth = KBaseAuth(auth_url=auth_url + "/api/legacy/KBase/Sessions/Login") # TODO using hardcoded roles for now to avoid possible bugs with mismatched cfg roles # these should probably be configurable self.auth_admin = AdminAuthUtil(auth_url, [ADMIN_READ_ROLE, ADMIN_WRITE_ROLE]) # KafkaClient has a nice error message when the arg is None - self.kafka_client = KafkaClient(cfg.get('kafka-host')) + self.kafka_client = KafkaClient(cfg.get("kafka-host")) # SlackClient handles None arguments self.slack_client = SlackClient( - cfg.get("slack-token"), debug=debug, endpoint=cfg.get("ee2-url")) + cfg.get("slack-token"), debug=debug, endpoint=cfg.get("ee2-url") + ) diff --git a/test/tests_for_utils/clients_test.py b/test/tests_for_utils/clients_test.py index add67fdcc..c0c3de792 100644 --- a/test/tests_for_utils/clients_test.py +++ b/test/tests_for_utils/clients_test.py @@ -8,12 +8,18 @@ def test_user_client_set_init_fail(): - user_client_set_init_fail(None, 'foo', 'bar', ValueError('cfg is required')) - user_client_set_init_fail({}, 'foo', 'bar', ValueError('cfg is required')) - user_client_set_init_fail({'a': 'b'}, None, 'bar', ValueError('user_id is required')) - user_client_set_init_fail({'a': 'b'}, ' \t ', 'bar', ValueError('user_id is required')) - user_client_set_init_fail({'a': 'b'}, 'foo', None, ValueError('token is required')) - user_client_set_init_fail({'a': 'b'}, 'foo', ' \t ', ValueError('token is required')) + user_client_set_init_fail(None, "foo", "bar", ValueError("cfg is required")) + user_client_set_init_fail({}, "foo", "bar", ValueError("cfg is required")) + user_client_set_init_fail( + {"a": "b"}, None, "bar", ValueError("user_id is required") + ) + user_client_set_init_fail( + {"a": "b"}, " \t ", "bar", ValueError("user_id is required") + ) + user_client_set_init_fail({"a": "b"}, "foo", None, ValueError("token is required")) + user_client_set_init_fail( + {"a": "b"}, "foo", " \t ", ValueError("token is required") + ) def user_client_set_init_fail(cfg, user, token, expected): From e7f8c960cb0f5d11b3305430ff5bbb186a5995dd Mon Sep 17 00:00:00 2001 From: Gavin Date: Thu, 28 Jan 2021 15:13:34 -0800 Subject: [PATCH 04/12] Remove unused imports Surprised flake8 didn't highlight these --- lib/execution_engine2/sdk/SDKMethodRunner.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/execution_engine2/sdk/SDKMethodRunner.py b/lib/execution_engine2/sdk/SDKMethodRunner.py index 340f3d314..be14887bc 100644 --- a/lib/execution_engine2/sdk/SDKMethodRunner.py +++ b/lib/execution_engine2/sdk/SDKMethodRunner.py @@ -16,10 +16,7 @@ import dateutil -from installed_clients.WorkspaceClient import Workspace from installed_clients.authclient import KBaseAuth -from lib.execution_engine2.authorization.workspaceauth import WorkspaceAuth -from execution_engine2.constants import ADMIN_WRITE_ROLE, ADMIN_READ_ROLE from lib.execution_engine2.db.MongoUtil import MongoUtil from lib.execution_engine2.db.models.models import Job from lib.execution_engine2.exceptions import AuthError From ebf56714e326b96ebd6856cd08e00be7db13e2c5 Mon Sep 17 00:00:00 2001 From: Gavin Date: Thu, 28 Jan 2021 15:30:37 -0800 Subject: [PATCH 05/12] Codacy won't let you have... ... a 1 line triple quoted comment unless the quotes are one the same line. --- lib/execution_engine2/constants.py | 4 +--- lib/execution_engine2/utils/clients.py | 8 +++----- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/execution_engine2/constants.py b/lib/execution_engine2/constants.py index 52cace674..813987ff9 100644 --- a/lib/execution_engine2/constants.py +++ b/lib/execution_engine2/constants.py @@ -1,5 +1,3 @@ -""" -Global EE2 constants. -""" +""" Global EE2 constants. """ ADMIN_READ_ROLE = "EE2_ADMIN_RO" ADMIN_WRITE_ROLE = "EE2_ADMIN" diff --git a/lib/execution_engine2/utils/clients.py b/lib/execution_engine2/utils/clients.py index b3aee0ab7..68a9f006c 100644 --- a/lib/execution_engine2/utils/clients.py +++ b/lib/execution_engine2/utils/clients.py @@ -1,6 +1,4 @@ -""" -Contains the various clients EE2 needs to communicate with other services it depends on. -""" +""" Contains the various clients EE2 needs to communicate with other services it depends on. """ # Note on testing - this class is not generally unit-testable, and is only tested fully in # integration tests. @@ -32,8 +30,8 @@ def __init__(self, cfg: Dict[str, str], user_id: str, token: str): cfg - the configuration dictionary user_id - the ID of the user to be used to initialize the client set. token - the token of the user to be used to initialize the client set. Note that the set - trusts that the token actually belongs to the user ID, and currently does not independently - check the validity of the user ID. + trusts that the token actually belongs to the user ID, and currently does not + independently check the validity of the user ID. Expected keys in config: workspace-url - the URL of the kbase workspace service From f456931f3e452faa670f06aab9d17a87fa8383bd Mon Sep 17 00:00:00 2001 From: Gavin Date: Fri, 29 Jan 2021 11:02:43 -0800 Subject: [PATCH 06/12] Combine constants files --- lib/execution_engine2/constants.py | 3 --- lib/execution_engine2/sdk/EE2Authentication.py | 2 +- lib/execution_engine2/sdk/EE2Constants.py | 4 ++++ lib/execution_engine2/utils/clients.py | 2 +- test/tests_for_auth/ee2_admin_mode_test.py | 2 +- 5 files changed, 7 insertions(+), 6 deletions(-) delete mode 100644 lib/execution_engine2/constants.py diff --git a/lib/execution_engine2/constants.py b/lib/execution_engine2/constants.py deleted file mode 100644 index 813987ff9..000000000 --- a/lib/execution_engine2/constants.py +++ /dev/null @@ -1,3 +0,0 @@ -""" Global EE2 constants. """ -ADMIN_READ_ROLE = "EE2_ADMIN_RO" -ADMIN_WRITE_ROLE = "EE2_ADMIN" diff --git a/lib/execution_engine2/sdk/EE2Authentication.py b/lib/execution_engine2/sdk/EE2Authentication.py index bcb297f5d..973103a67 100644 --- a/lib/execution_engine2/sdk/EE2Authentication.py +++ b/lib/execution_engine2/sdk/EE2Authentication.py @@ -5,7 +5,7 @@ from lib.execution_engine2.authorization.authstrategy import can_read_job, can_write_job from lib.execution_engine2.authorization.roles import AdminAuthUtil from lib.execution_engine2.db.models.models import Job -from execution_engine2.constants import ADMIN_READ_ROLE, ADMIN_WRITE_ROLE +from execution_engine2.sdk.EE2Constants import ADMIN_READ_ROLE, ADMIN_WRITE_ROLE class JobPermissions(Enum): diff --git a/lib/execution_engine2/sdk/EE2Constants.py b/lib/execution_engine2/sdk/EE2Constants.py index 8ecbcac87..7d3f3cd0f 100644 --- a/lib/execution_engine2/sdk/EE2Constants.py +++ b/lib/execution_engine2/sdk/EE2Constants.py @@ -9,6 +9,10 @@ KBASE_CONCIERGE_USERNAME = "kbaseconcierge" CONCIERGE_CLIENTGROUP = "kbase_concierge" +# these also probably should be configurable. +ADMIN_READ_ROLE = "EE2_ADMIN_RO" +ADMIN_WRITE_ROLE = "EE2_ADMIN" + class JobError(NamedTuple): name: str diff --git a/lib/execution_engine2/utils/clients.py b/lib/execution_engine2/utils/clients.py index 68a9f006c..4e740915d 100644 --- a/lib/execution_engine2/utils/clients.py +++ b/lib/execution_engine2/utils/clients.py @@ -9,7 +9,7 @@ from execution_engine2.authorization.workspaceauth import WorkspaceAuth from execution_engine2.utils.CatalogUtils import CatalogUtils from execution_engine2.utils.Condor import Condor -from execution_engine2.constants import ADMIN_READ_ROLE, ADMIN_WRITE_ROLE +from execution_engine2.sdk.EE2Constants import ADMIN_READ_ROLE, ADMIN_WRITE_ROLE from execution_engine2.utils.KafkaUtils import KafkaClient from execution_engine2.utils.SlackUtils import SlackClient diff --git a/test/tests_for_auth/ee2_admin_mode_test.py b/test/tests_for_auth/ee2_admin_mode_test.py index a22127a3b..d7d03d72a 100644 --- a/test/tests_for_auth/ee2_admin_mode_test.py +++ b/test/tests_for_auth/ee2_admin_mode_test.py @@ -12,7 +12,7 @@ from installed_clients.CatalogClient import Catalog from lib.execution_engine2.authorization.roles import AdminAuthUtil from lib.execution_engine2.authorization.workspaceauth import WorkspaceAuth -from execution_engine2.constants import ADMIN_READ_ROLE, ADMIN_WRITE_ROLE +from execution_engine2.sdk.EE2Constants import ADMIN_READ_ROLE, ADMIN_WRITE_ROLE from lib.execution_engine2.db.models.models import Status from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner from lib.execution_engine2.utils.Condor import Condor From 25b8dfc66ad19c3c08e30a7322d4670f05b83894 Mon Sep 17 00:00:00 2001 From: Gavin Date: Fri, 29 Jan 2021 11:24:39 -0800 Subject: [PATCH 07/12] test that workspace url is provided in user client set --- lib/execution_engine2/utils/clients.py | 10 ++++++---- test/tests_for_utils/clients_test.py | 21 ++++++++++++++++++--- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/lib/execution_engine2/utils/clients.py b/lib/execution_engine2/utils/clients.py index 4e740915d..c414436e1 100644 --- a/lib/execution_engine2/utils/clients.py +++ b/lib/execution_engine2/utils/clients.py @@ -48,11 +48,13 @@ def __init__(self, cfg: Dict[str, str], user_id: str, token: str): # Do a check that the url actually points to the workspace? # Also maybe consider passing in the workspace url rather than the dict, but the ClientSet # below will need lots of params so a dict makes sense there, maybe keep the apis similar? - # ws_url = cfg.get('workspace_url') # may want to make the keys constants? - # if not ws_url or not ws_url.strip: - # raise ValueError('missing workspace-url key in configuration') + # TODO the client throws a 'X is not a valid url' error if the url isn't valid, improve + # by catching & rethrowing with a more clear message that the config is wrong + ws_url = cfg.get("workspace-url") # may want to make the keys constants? + if not ws_url or not ws_url.strip(): + raise ValueError("missing workspace-url in configuration") # TODO Originally did the above check but caused 36 test failures so... meh for now. - self._workspace = Workspace(cfg["workspace-url"], token=token) + self._workspace = Workspace(ws_url, token=token) self._workspace_auth = WorkspaceAuth(user_id, self._workspace) # create_autospec can't mock instance variables, so we add some boilerplate diff --git a/test/tests_for_utils/clients_test.py b/test/tests_for_utils/clients_test.py index c0c3de792..07437d31f 100644 --- a/test/tests_for_utils/clients_test.py +++ b/test/tests_for_utils/clients_test.py @@ -8,17 +8,32 @@ def test_user_client_set_init_fail(): + ws_err = "missing workspace-url in configuration" user_client_set_init_fail(None, "foo", "bar", ValueError("cfg is required")) user_client_set_init_fail({}, "foo", "bar", ValueError("cfg is required")) + user_client_set_init_fail({"a": "b"}, "foo", "bar", ValueError(ws_err)) + user_client_set_init_fail({"workspace-url": None}, "foo", "bar", ValueError(ws_err)) user_client_set_init_fail( - {"a": "b"}, None, "bar", ValueError("user_id is required") + {"workspace-url": " \t "}, "foo", "bar", ValueError(ws_err) ) user_client_set_init_fail( - {"a": "b"}, " \t ", "bar", ValueError("user_id is required") + {"workspace-url": "https://ws.com"}, + None, + "bar", + ValueError("user_id is required"), + ) + user_client_set_init_fail( + {"workspace-url": "https://ws.com"}, + " \t ", + "bar", + ValueError("user_id is required"), ) user_client_set_init_fail({"a": "b"}, "foo", None, ValueError("token is required")) user_client_set_init_fail( - {"a": "b"}, "foo", " \t ", ValueError("token is required") + {"workspace-url": "https://ws.com"}, + "foo", + " \t ", + ValueError("token is required"), ) From f138b68e21d7bc1135ab9009633e352fc64458f2 Mon Sep 17 00:00:00 2001 From: Gavin Date: Fri, 29 Jan 2021 11:26:19 -0800 Subject: [PATCH 08/12] remove done TODO --- lib/execution_engine2/utils/clients.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/execution_engine2/utils/clients.py b/lib/execution_engine2/utils/clients.py index c414436e1..cd8ce0ddf 100644 --- a/lib/execution_engine2/utils/clients.py +++ b/lib/execution_engine2/utils/clients.py @@ -53,7 +53,6 @@ def __init__(self, cfg: Dict[str, str], user_id: str, token: str): ws_url = cfg.get("workspace-url") # may want to make the keys constants? if not ws_url or not ws_url.strip(): raise ValueError("missing workspace-url in configuration") - # TODO Originally did the above check but caused 36 test failures so... meh for now. self._workspace = Workspace(ws_url, token=token) self._workspace_auth = WorkspaceAuth(user_id, self._workspace) From 4cc92c34866e8708312f96799d4cdfeffe77e592 Mon Sep 17 00:00:00 2001 From: Gavin Date: Wed, 3 Feb 2021 13:17:28 -0800 Subject: [PATCH 09/12] Remove need to mock UserClientSet Effectively makes it a value class which doesn't need to be mocked. Just pass mocks into the UCS instead. --- .../execution_engine2Impl.py | 4 +- lib/execution_engine2/sdk/SDKMethodRunner.py | 8 +- lib/execution_engine2/utils/clients.py | 87 ++++++++++--------- test/tests_for_auth/ee2_admin_mode_test.py | 28 +++--- .../ee2_SDKMethodRunner_EE2Logs_test.py | 4 +- .../ee2_SDKMethodRunner_test.py | 7 +- ...ee2_SDKMethodRunner_test_EE2Runjob_test.py | 4 +- ...ee2_SDKMethodRunner_test_EE2Status_test.py | 4 +- test/tests_for_sdkmr/ee2_load_test.py | 4 +- test/tests_for_utils/clients_test.py | 53 ++++++++--- 10 files changed, 120 insertions(+), 83 deletions(-) diff --git a/lib/execution_engine2/execution_engine2Impl.py b/lib/execution_engine2/execution_engine2Impl.py index 6f08c8336..eba1fb4c3 100644 --- a/lib/execution_engine2/execution_engine2Impl.py +++ b/lib/execution_engine2/execution_engine2Impl.py @@ -6,7 +6,7 @@ from lib.execution_engine2.db.MongoUtil import MongoUtil from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner -from execution_engine2.utils.clients import UserClientSet +from execution_engine2.utils.clients import get_user_client_set, UserClientSet #END_HEADER @@ -44,7 +44,7 @@ class execution_engine2: ADMIN_ROLES_CACHE_EXPIRE_TIME = 300 # seconds def get_user_clients(self, ctx) -> UserClientSet: - return UserClientSet(self.config, ctx['user_id'], ctx['token']) + return get_user_client_set(self.config, ctx['user_id'], ctx['token']) #END_CLASS_HEADER diff --git a/lib/execution_engine2/sdk/SDKMethodRunner.py b/lib/execution_engine2/sdk/SDKMethodRunner.py index be14887bc..14399dc9d 100644 --- a/lib/execution_engine2/sdk/SDKMethodRunner.py +++ b/lib/execution_engine2/sdk/SDKMethodRunner.py @@ -67,16 +67,16 @@ def __init__( self.config = config self.mongo_util = mongo_util self.condor = None - self.workspace = user_clients.workspace() - self.workspace_auth = user_clients.workspace_auth() + self.workspace = user_clients.workspace + self.workspace_auth = user_clients.workspace_auth self.admin_roles = config.get("admin_roles", ["EE2_ADMIN", "EE2_ADMIN_RO"]) self.catalog_utils = CatalogUtils( config["catalog-url"], config["catalog-token"] ) self.auth_url = config.get("auth-url") self.auth = KBaseAuth(auth_url=config.get("auth-service-url")) - self.user_id = user_clients.user_id() - self.token = user_clients.token() + self.user_id = user_clients.user_id + self.token = user_clients.token self.debug = SDKMethodRunner.parse_bool_from_string(config.get("debug")) self.logger = get_logger() diff --git a/lib/execution_engine2/utils/clients.py b/lib/execution_engine2/utils/clients.py index cd8ce0ddf..d123d2ad0 100644 --- a/lib/execution_engine2/utils/clients.py +++ b/lib/execution_engine2/utils/clients.py @@ -23,54 +23,61 @@ class UserClientSet: on a per user basis. Also contains the user credentials for ease of use. """ - def __init__(self, cfg: Dict[str, str], user_id: str, token: str): + def __init__( + self, + user_id: str, + token: str, # TODO is this needed? + workspace: Workspace, + workspace_auth: WorkspaceAuth, + ): """ Initialize the client set. - cfg - the configuration dictionary - user_id - the ID of the user to be used to initialize the client set. - token - the token of the user to be used to initialize the client set. Note that the set - trusts that the token actually belongs to the user ID, and currently does not - independently check the validity of the user ID. - - Expected keys in config: - workspace-url - the URL of the kbase workspace service + user_id - The user's ID. + token - The users's token + workspace - A workspace client initialized with the user's token. + workspace_auth - A workspace auth client initialized with the user's token. """ if not user_id or not user_id.strip(): raise ValueError("user_id is required") if not token or not token.strip(): raise ValueError("token is required") - if not cfg: - raise ValueError("cfg is required") - self._user_id = user_id - self._token = token - - # Do a check that the url actually points to the workspace? - # Also maybe consider passing in the workspace url rather than the dict, but the ClientSet - # below will need lots of params so a dict makes sense there, maybe keep the apis similar? - # TODO the client throws a 'X is not a valid url' error if the url isn't valid, improve - # by catching & rethrowing with a more clear message that the config is wrong - ws_url = cfg.get("workspace-url") # may want to make the keys constants? - if not ws_url or not ws_url.strip(): - raise ValueError("missing workspace-url in configuration") - self._workspace = Workspace(ws_url, token=token) - self._workspace_auth = WorkspaceAuth(user_id, self._workspace) - - # create_autospec can't mock instance variables, so we add some boilerplate - # Could make these properties but then they return MagicMocks which don't update with API - # changes - - def user_id(self): - return self._user_id - - def token(self): - return self._token - - def workspace(self): - return self._workspace - - def workspace_auth(self): - return self._workspace_auth + if not workspace: + raise ValueError("workspace is required") + if not workspace_auth: + raise ValueError("workspace_auth is required") + self.user_id = user_id + self.token = token + self.workspace = workspace + self.workspace_auth = workspace_auth + + +def get_user_client_set(cfg: Dict[str, str], user_id: str, token: str): + """ + Create the client set from a configuration dictionary. + + cfg - the configuration dictionary + user_id - the ID of the user to be used to initialize the client set. + token - the token of the user to be used to initialize the client set. Note that the set + trusts that the token actually belongs to the user ID, and currently does not + independently check the validity of the user ID. + + Expected keys in config: + workspace-url - the URL of the kbase workspace service + """ + if not cfg: + raise ValueError("cfg is required") + # Do a check that the url actually points to the workspace? + # Also maybe consider passing in the workspace url rather than the dict, but the ClientSet + # below will need lots of params so a dict makes sense there, maybe keep the apis similar? + # TODO the client throws a 'X is not a valid url' error if the url isn't valid, improve + # by catching & rethrowing with a more clear message that the config is wrong + ws_url = cfg.get("workspace-url") # may want to make the keys constants? + if not ws_url or not ws_url.strip(): + raise ValueError("missing workspace-url in configuration") + workspace = Workspace(ws_url, token=token) + workspace_auth = WorkspaceAuth(user_id, workspace) + return UserClientSet(user_id, token, workspace, workspace_auth) class ClientSet: diff --git a/test/tests_for_auth/ee2_admin_mode_test.py b/test/tests_for_auth/ee2_admin_mode_test.py index d7d03d72a..b2c3edc6d 100644 --- a/test/tests_for_auth/ee2_admin_mode_test.py +++ b/test/tests_for_auth/ee2_admin_mode_test.py @@ -10,6 +10,7 @@ from mock import patch from installed_clients.CatalogClient import Catalog +from installed_clients.WorkspaceClient import Workspace from lib.execution_engine2.authorization.roles import AdminAuthUtil from lib.execution_engine2.authorization.workspaceauth import WorkspaceAuth from execution_engine2.sdk.EE2Constants import ADMIN_READ_ROLE, ADMIN_WRITE_ROLE @@ -17,7 +18,7 @@ from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner from lib.execution_engine2.utils.Condor import Condor from lib.execution_engine2.utils.CondorTuples import SubmissionInfo -from execution_engine2.utils.clients import UserClientSet +from execution_engine2.utils.clients import get_user_client_set, UserClientSet from test.utils_shared.test_utils import ( get_sample_job_params, get_sample_condor_info, @@ -45,7 +46,7 @@ def setUpClass(cls): cls.token = "token" cls.method_runner = SDKMethodRunner( - cls.cfg, UserClientSet(cls.cfg, cls.user_id, cls.token) + cls.cfg, get_user_client_set(cls.cfg, cls.user_id, cls.token) ) def setUp(self) -> None: @@ -89,7 +90,7 @@ def tearDown(self) -> None: def getRunner(self, user_clients=None) -> SDKMethodRunner: # Initialize these clients from None if not user_clients: - user_clients = UserClientSet(self.cfg, self.user_id, self.token) + user_clients = get_user_client_set(self.cfg, self.user_id, self.token) runner = SDKMethodRunner(self.cfg, user_clients) # type : SDKMethodRunner runner.get_jobs_status() runner.get_runjob() @@ -108,22 +109,23 @@ def get_runner_with_condor(self) -> SDKMethodRunner: # TODO How do you test ADMIN_MODE without increasing too much coverage - def get_mocks(self) -> (UserClientSet, WorkspaceAuth): - # add workspace client mock if needed - ucs = create_autospec(UserClientSet, instance=True, spec_set=True) + def get_mocks( + self, user_id=None, token="fake_token" + ) -> (UserClientSet, Workspace, WorkspaceAuth): + user_id = user_id if user_id else self.user_id + ws = create_autospec(Workspace, instance=True, spec_set=True) wsa = create_autospec(WorkspaceAuth, instance=True, spec_set=True) - ucs.workspace_auth.return_value = wsa - return ucs, wsa + ucs = UserClientSet(user_id, token, ws, wsa) + return ucs, ws, wsa @patch.object(Catalog, "get_module_version", return_value="module.version") @patch.object(AdminAuthUtil, "_fetch_user_roles") def test_regular_user(self, aau, catalog): # Regular User lowly_user = "Access Denied: You are not an administrator" - ucs, wsa = self.get_mocks() - ucs.user_id.return_value = self.user_id - wsa.can_write.return_value = True - runner = self.getRunner(ucs) + user_client_set, _, ws_auth = self.get_mocks() + ws_auth.can_write.return_value = True + runner = self.getRunner(user_client_set) aau.return_value = ["RegularJoe"] method_1 = "module_name.function_name" job_params_1 = get_sample_job_params(method=method_1, wsid=self.ws_id) @@ -140,7 +142,7 @@ def test_regular_user(self, aau, catalog): job_id = runner.run_job(params=job_params_1, as_admin=False) self.assertTrue(bson.objectid.ObjectId.is_valid(job_id)) - wsa.can_write.assert_called_once_with(self.ws_id) + ws_auth.can_write.assert_called_once_with(self.ws_id) # RUNJOB BUT ATTEMPT TO BE AN ADMIN with self.assertRaisesRegexp( diff --git a/test/tests_for_sdkmr/ee2_SDKMethodRunner_EE2Logs_test.py b/test/tests_for_sdkmr/ee2_SDKMethodRunner_EE2Logs_test.py index eb83c60b9..9a9b68d55 100644 --- a/test/tests_for_sdkmr/ee2_SDKMethodRunner_EE2Logs_test.py +++ b/test/tests_for_sdkmr/ee2_SDKMethodRunner_EE2Logs_test.py @@ -10,7 +10,7 @@ from lib.execution_engine2.db.MongoUtil import MongoUtil from lib.execution_engine2.db.models.models import Job, JobLog from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner -from execution_engine2.utils.clients import UserClientSet +from execution_engine2.utils.clients import get_user_client_set from test.utils_shared.test_utils import ( bootstrap, run_job_adapter, @@ -36,7 +36,7 @@ def setUpClass(cls): cls.token = "token" cls.method_runner = SDKMethodRunner( - cls.cfg, UserClientSet(cls.cfg, cls.user_id, cls.token) + cls.cfg, get_user_client_set(cls.cfg, cls.user_id, cls.token) ) cls.mongo_util = MongoUtil(cls.cfg) cls.mongo_helper = MongoTestHelper(cls.cfg) diff --git a/test/tests_for_sdkmr/ee2_SDKMethodRunner_test.py b/test/tests_for_sdkmr/ee2_SDKMethodRunner_test.py index 220cb1780..c63310142 100644 --- a/test/tests_for_sdkmr/ee2_SDKMethodRunner_test.py +++ b/test/tests_for_sdkmr/ee2_SDKMethodRunner_test.py @@ -23,7 +23,7 @@ from lib.execution_engine2.exceptions import InvalidStatusTransitionException from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner from lib.execution_engine2.utils.CondorTuples import SubmissionInfo, CondorResources -from execution_engine2.utils.clients import UserClientSet +from execution_engine2.utils.clients import get_user_client_set from test.tests_for_sdkmr.ee2_SDKMethodRunner_test_utils import ee2_sdkmr_test_helper from test.utils_shared.test_utils import ( bootstrap, @@ -64,7 +64,7 @@ def setUpClass(cls): cls.token = "token" cls.method_runner = SDKMethodRunner( - cls.cfg, UserClientSet(cls.cfg, cls.user_id, cls.token) + cls.cfg, get_user_client_set(cls.cfg, cls.user_id, cls.token) ) cls.mongo_util = MongoUtil(cls.cfg) cls.mongo_helper = MongoTestHelper(cls.cfg) @@ -670,7 +670,8 @@ def test_check_job_global_perm(self, rq_mock): # now test with a different user other_method_runner = SDKMethodRunner( - self.cfg, UserClientSet(self.cfg, "some_other_user", "other_token") + self.cfg, + get_user_client_set(self.cfg, "some_other_user", "other_token"), ) job_states = other_method_runner.get_jobs_status().check_workspace_jobs( self.ws_id diff --git a/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Runjob_test.py b/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Runjob_test.py index c3d95024c..33fc9389d 100644 --- a/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Runjob_test.py +++ b/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Runjob_test.py @@ -13,7 +13,7 @@ from lib.execution_engine2.db.models.models import Job from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner from lib.execution_engine2.utils.CondorTuples import SubmissionInfo, CondorResources -from execution_engine2.utils.clients import UserClientSet +from execution_engine2.utils.clients import get_user_client_set from test.utils_shared.test_utils import ( bootstrap, get_example_job, @@ -51,7 +51,7 @@ def setUpClass(cls): cls.token = "token" cls.method_runner = SDKMethodRunner( - cls.cfg, UserClientSet(cls.cfg, cls.user_id, cls.token) + cls.cfg, get_user_client_set(cls.cfg, cls.user_id, cls.token) ) cls.mongo_util = MongoUtil(cls.cfg) diff --git a/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Status_test.py b/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Status_test.py index 4b2a4defb..3ffc5e19f 100644 --- a/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Status_test.py +++ b/test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Status_test.py @@ -13,7 +13,7 @@ from lib.execution_engine2.db.models.models import Job from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner from lib.execution_engine2.utils.CondorTuples import SubmissionInfo, CondorResources -from execution_engine2.utils.clients import UserClientSet +from execution_engine2.utils.clients import get_user_client_set from test.tests_for_sdkmr.ee2_SDKMethodRunner_test_utils import ee2_sdkmr_test_helper from test.utils_shared.test_utils import bootstrap, get_example_job @@ -46,7 +46,7 @@ def setUpClass(cls): cls.token = "token" cls.method_runner = SDKMethodRunner( - cls.cfg, UserClientSet(cls.cfg, cls.user_id, cls.token) + cls.cfg, get_user_client_set(cls.cfg, cls.user_id, cls.token) ) cls.cr = CondorResources( request_cpus="1", diff --git a/test/tests_for_sdkmr/ee2_load_test.py b/test/tests_for_sdkmr/ee2_load_test.py index 031f985bb..0fa577495 100644 --- a/test/tests_for_sdkmr/ee2_load_test.py +++ b/test/tests_for_sdkmr/ee2_load_test.py @@ -17,7 +17,7 @@ from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner from lib.execution_engine2.utils.Condor import Condor from lib.execution_engine2.utils.CondorTuples import SubmissionInfo -from execution_engine2.utils.clients import UserClientSet +from execution_engine2.utils.clients import get_user_client_set from test.utils_shared.test_utils import ( bootstrap, get_sample_job_params, @@ -44,7 +44,7 @@ def setUpClass(cls): cls.ctx = {"token": cls.token, "user_id": cls.user_id} cls.impl = execution_engine2(cls.cfg) cls.method_runner = SDKMethodRunner( - cls.cfg, UserClientSet(cls.cfg, cls.user_id, cls.token) + cls.cfg, get_user_client_set(cls.cfg, cls.user_id, cls.token) ) cls.mongo_util = MongoUtil(cls.cfg) cls.mongo_helper = MongoTestHelper(cls.cfg) diff --git a/test/tests_for_utils/clients_test.py b/test/tests_for_utils/clients_test.py index 07437d31f..3e109e73e 100644 --- a/test/tests_for_utils/clients_test.py +++ b/test/tests_for_utils/clients_test.py @@ -2,34 +2,42 @@ # That code is tested in integration tests. from pytest import raises +from unittest.mock import create_autospec -from execution_engine2.utils.clients import UserClientSet +from execution_engine2.authorization.workspaceauth import WorkspaceAuth +from execution_engine2.utils.clients import UserClientSet, get_user_client_set from utils_shared.test_utils import assert_exception_correct +from installed_clients.WorkspaceClient import Workspace -def test_user_client_set_init_fail(): +def test_get_user_client_set_fail(): ws_err = "missing workspace-url in configuration" - user_client_set_init_fail(None, "foo", "bar", ValueError("cfg is required")) - user_client_set_init_fail({}, "foo", "bar", ValueError("cfg is required")) - user_client_set_init_fail({"a": "b"}, "foo", "bar", ValueError(ws_err)) - user_client_set_init_fail({"workspace-url": None}, "foo", "bar", ValueError(ws_err)) - user_client_set_init_fail( + get_user_client_set_fail(None, "foo", "bar", ValueError("cfg is required")) + get_user_client_set_fail({}, "foo", "bar", ValueError("cfg is required")) + get_user_client_set_fail({"a": "b"}, "foo", "bar", ValueError(ws_err)) + get_user_client_set_fail({"workspace-url": None}, "foo", "bar", ValueError(ws_err)) + get_user_client_set_fail( {"workspace-url": " \t "}, "foo", "bar", ValueError(ws_err) ) - user_client_set_init_fail( + get_user_client_set_fail( {"workspace-url": "https://ws.com"}, None, "bar", ValueError("user_id is required"), ) - user_client_set_init_fail( + get_user_client_set_fail( {"workspace-url": "https://ws.com"}, " \t ", "bar", ValueError("user_id is required"), ) - user_client_set_init_fail({"a": "b"}, "foo", None, ValueError("token is required")) - user_client_set_init_fail( + get_user_client_set_fail( + {"workspace-url": "https://ws.com"}, + "foo", + None, + ValueError("token is required"), + ) + get_user_client_set_fail( {"workspace-url": "https://ws.com"}, "foo", " \t ", @@ -37,7 +45,26 @@ def test_user_client_set_init_fail(): ) -def user_client_set_init_fail(cfg, user, token, expected): +def get_user_client_set_fail(cfg, user, token, expected): + with raises(Exception) as e: + get_user_client_set(cfg, user, token) + assert_exception_correct(e.value, expected) + + +def test_user_client_set_init_fail(): + ws = create_autospec(Workspace, spec_set=True, instance=True) + wsa = WorkspaceAuth("u", ws) + user_client_set_init_fail(None, "t", ws, wsa, ValueError("user_id is required")) + user_client_set_init_fail(" \t ", "t", ws, wsa, ValueError("user_id is required")) + user_client_set_init_fail("u", None, ws, wsa, ValueError("token is required")) + user_client_set_init_fail("u", " \t ", ws, wsa, ValueError("token is required")) + user_client_set_init_fail("u", "t", None, wsa, ValueError("workspace is required")) + user_client_set_init_fail( + "u", "t", ws, None, ValueError("workspace_auth is required") + ) + + +def user_client_set_init_fail(user, token, ws_client, ws_auth, expected): with raises(Exception) as e: - UserClientSet(cfg, user, token) + UserClientSet(user, token, ws_client, ws_auth) assert_exception_correct(e.value, expected) From 6781f1779ead75a364f5e27ae026907567cd8287 Mon Sep 17 00:00:00 2001 From: Gavin Date: Wed, 3 Feb 2021 13:43:59 -0800 Subject: [PATCH 10/12] Move helper function out of Impl file Keeping as much code as possible out of the Impl file is good practice --- .../execution_engine2Impl.py | 54 +++++++++---------- lib/execution_engine2/utils/APIHelpers.py | 31 +++++++++++ 2 files changed, 55 insertions(+), 30 deletions(-) create mode 100644 lib/execution_engine2/utils/APIHelpers.py diff --git a/lib/execution_engine2/execution_engine2Impl.py b/lib/execution_engine2/execution_engine2Impl.py index eba1fb4c3..caecb561a 100644 --- a/lib/execution_engine2/execution_engine2Impl.py +++ b/lib/execution_engine2/execution_engine2Impl.py @@ -6,7 +6,7 @@ from lib.execution_engine2.db.MongoUtil import MongoUtil from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner -from execution_engine2.utils.clients import get_user_client_set, UserClientSet +from execution_engine2.utils.APIHelpers import GenerateFromConfig #END_HEADER @@ -42,10 +42,6 @@ class execution_engine2: ADMIN_ROLES_CACHE_SIZE = 500 ADMIN_ROLES_CACHE_EXPIRE_TIME = 300 # seconds - - def get_user_clients(self, ctx) -> UserClientSet: - return get_user_client_set(self.config, ctx['user_id'], ctx['token']) - #END_CLASS_HEADER # config contains contents of config file in a hash or None if it couldn't @@ -64,9 +60,7 @@ def __init__(self, config): maxsize=self.ADMIN_ROLES_CACHE_SIZE, ttl=self.ADMIN_ROLES_CACHE_EXPIRE_TIME ) self.mongo_util = MongoUtil(config) - - - + self.gen_cfg = GenerateFromConfig(config) #END_CONSTRUCTOR pass @@ -233,7 +227,7 @@ def run_job(self, ctx, params): #BEGIN run_job mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util ) @@ -305,7 +299,7 @@ def run_job_batch(self, ctx, params, batch_params): #BEGIN run_job_batch mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util ) @@ -334,7 +328,7 @@ def abandon_children(self, ctx, params): #BEGIN abandon_children mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util ) @@ -417,7 +411,7 @@ def run_job_concierge(self, ctx, params, concierge_params): #BEGIN run_job_concierge mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), mongo_util=self.mongo_util ) job_id = mr.run_job_concierge(params=params,concierge_params=concierge_params) @@ -487,7 +481,7 @@ def get_job_params(self, ctx, params): #BEGIN get_job_params mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -516,7 +510,7 @@ def update_job_status(self, ctx, params): #BEGIN update_job_status mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -554,7 +548,7 @@ def add_job_logs(self, ctx, params, lines): #BEGIN add_job_logs mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -605,7 +599,7 @@ def get_job_logs(self, ctx, params): mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -646,7 +640,7 @@ def finish_job(self, ctx, params): #BEGIN finish_job mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -675,7 +669,7 @@ def start_job(self, ctx, params): #BEGIN start_job mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -788,7 +782,7 @@ def check_job(self, ctx, params): #BEGIN check_job mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), mongo_util=self.mongo_util ) job_state = mr.check_job( @@ -995,7 +989,7 @@ def check_job_batch(self, ctx, params): #BEGIN check_job_batch mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), mongo_util=self.mongo_util ) returnVal = mr.check_job_batch( @@ -1114,7 +1108,7 @@ def check_jobs(self, ctx, params): #BEGIN check_jobs mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), mongo_util=self.mongo_util ) returnVal = mr.check_jobs( @@ -1235,7 +1229,7 @@ def check_workspace_jobs(self, ctx, params): # return variables are: returnVal #BEGIN check_workspace_jobs mr = SDKMethodRunner(self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), mongo_util=self.mongo_util) returnVal = mr.check_workspace_jobs( params.get("workspace_id"), @@ -1268,7 +1262,7 @@ def cancel_job(self, ctx, params): #BEGIN cancel_job mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -1307,7 +1301,7 @@ def check_job_canceled(self, ctx, params): #BEGIN check_job_canceled mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), mongo_util=self.mongo_util ) result = mr.check_job_canceled(job_id=params["job_id"], as_admin=params.get('as_admin')) @@ -1334,7 +1328,7 @@ def get_job_status(self, ctx, params): #BEGIN get_job_status mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), job_permission_cache=self.job_permission_cache, admin_permissions_cache=self.admin_permissions_cache, mongo_util=self.mongo_util @@ -1462,7 +1456,7 @@ def check_jobs_date_range_for_user(self, ctx, params): #BEGIN check_jobs_date_range_for_user mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), mongo_util=self.mongo_util ) returnVal = mr.check_jobs_date_range_for_user( @@ -1598,7 +1592,7 @@ def check_jobs_date_range_for_all(self, ctx, params): #BEGIN check_jobs_date_range_for_all mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), mongo_util=self.mongo_util ) returnVal = mr.check_jobs_date_range_for_user( @@ -1633,7 +1627,7 @@ def handle_held_job(self, ctx, cluster_id): #BEGIN handle_held_job mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.gen_cfg.get_user_clients(ctx), mongo_util=self.mongo_util ) returnVal = mr.handle_held_job(cluster_id=cluster_id) @@ -1656,7 +1650,7 @@ def is_admin(self, ctx): #BEGIN is_admin mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.get_cfg.get_user_clients(ctx), mongo_util=self.mongo_util ) returnVal = mr.check_is_admin() @@ -1682,7 +1676,7 @@ def get_admin_permission(self, ctx): #BEGIN get_admin_permission mr = SDKMethodRunner( self.config, - user_clients=self.get_user_clients(ctx), + user_clients=self.get_cfg.get_user_clients(ctx), mongo_util=self.mongo_util ) returnVal = mr.get_admin_permission() diff --git a/lib/execution_engine2/utils/APIHelpers.py b/lib/execution_engine2/utils/APIHelpers.py new file mode 100644 index 000000000..4e15cc386 --- /dev/null +++ b/lib/execution_engine2/utils/APIHelpers.py @@ -0,0 +1,31 @@ +""" +Contains classes and fuctions for use with the EE2 SDK API class (e.g. the *Impl.py file). +""" + +from typing import Dict +from execution_engine2.utils.clients import UserClientSet, get_user_client_set + + +# this class is only tested as part of integration tests. +class GenerateFromConfig: + """ + Utility methods to generate constructs from the service configuration. + """ + + def __init__(self, cfg: Dict[str, str]): + """ + Create an instance from a configuration. + + cfg - the configuration. + """ + self.cfg = cfg + + def get_user_clients(self, ctx) -> UserClientSet: + """ + Create a user client set from an SDK context object. + + ctx - the context object. This is passed in to SDK methods in the *Impl.py file. It is + expected that the context object contains the user_id and token keys, and this method + will fail with a KeyError if it does not. + """ + return get_user_client_set(self.cfg, ctx['user_id'], ctx['token']) From 2321e2b8971e99b9630b645a277493cbd958f06f Mon Sep 17 00:00:00 2001 From: Gavin Date: Wed, 3 Feb 2021 13:52:28 -0800 Subject: [PATCH 11/12] Run black (again?) I'm looking at my console right now, and I see a black run with no issues, and then the current black run with this problem. I don't understand why black didn't catch this the first time. --- lib/execution_engine2/utils/APIHelpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/execution_engine2/utils/APIHelpers.py b/lib/execution_engine2/utils/APIHelpers.py index 4e15cc386..65e66ed74 100644 --- a/lib/execution_engine2/utils/APIHelpers.py +++ b/lib/execution_engine2/utils/APIHelpers.py @@ -28,4 +28,4 @@ def get_user_clients(self, ctx) -> UserClientSet: expected that the context object contains the user_id and token keys, and this method will fail with a KeyError if it does not. """ - return get_user_client_set(self.cfg, ctx['user_id'], ctx['token']) + return get_user_client_set(self.cfg, ctx["user_id"], ctx["token"]) From 0e610a3aab177ecfaa754a0ef6a149d7ffe6b10e Mon Sep 17 00:00:00 2001 From: Gavin Date: Wed, 3 Feb 2021 15:08:09 -0800 Subject: [PATCH 12/12] Remove TODO It's needed in a couple of places, most notably EE2RunJob --- lib/execution_engine2/utils/clients.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/execution_engine2/utils/clients.py b/lib/execution_engine2/utils/clients.py index d123d2ad0..97aa13fb3 100644 --- a/lib/execution_engine2/utils/clients.py +++ b/lib/execution_engine2/utils/clients.py @@ -26,7 +26,7 @@ class UserClientSet: def __init__( self, user_id: str, - token: str, # TODO is this needed? + token: str, workspace: Workspace, workspace_auth: WorkspaceAuth, ):