Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: cli is not a library! #7891

Merged
merged 1 commit into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions e2e_tests/tests/api_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,24 @@ def create_test_user(
return authentication.Credentials(user.username, password)


def assign_user_role(session: api.Session, user: str, role: str, workspace: Optional[str]) -> None:
user_assign = api.create_user_assignment_request(
session, user=user, role=role, workspace=workspace
)
req = bindings.v1AssignRolesRequest(userRoleAssignments=user_assign, groupRoleAssignments=[])
bindings.post_AssignRoles(session, body=req)


def assign_group_role(
session: api.Session, group: str, role: str, workspace: Optional[str]
) -> None:
group_assign = api.create_group_assignment_request(
session, group=group, role=role, workspace=workspace
)
req = bindings.v1AssignRolesRequest(userRoleAssignments=[], groupRoleAssignments=group_assign)
bindings.post_AssignRoles(session, body=req)


def configure_token_store(credentials: authentication.Credentials) -> None:
"""Authenticate the user for CLI usage with the given credentials."""
token_store = authentication.TokenStore(conf.make_master_url())
Expand Down
26 changes: 16 additions & 10 deletions e2e_tests/tests/cluster/test_logging.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import pytest

from determined.cli import command
from determined.common import api
from determined.common.api import authentication, bindings, certs
from tests import config as conf
Expand Down Expand Up @@ -67,10 +66,10 @@ def test_trial_logs() -> None:
@pytest.mark.parametrize(
"task_type,task_config,log_regex",
[
(command.TaskTypeCommand, {"entrypoint": ["echo", "hello"]}, re.compile("^.*hello.*$")),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because the CLI is not a library. It should never be imported from outside the cli/ directory.

Importing it for a few static strings is a violation of the application-nature of the CLI, with no particular benefit. I don't think these strings are going to change in the next 10 years.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't check where the command namespace was coming from. makes sense.

(command.TaskTypeNotebook, {}, re.compile("^.*Jupyter Server .* is running.*$")),
(command.TaskTypeShell, {}, re.compile("^.*Server listening on.*$")),
(command.TaskTypeTensorBoard, {}, re.compile("^.*TensorBoard .* at .*$")),
("command", {"entrypoint": ["echo", "hello"]}, re.compile("^.*hello.*$")),
("notebook", {}, re.compile("^.*Jupyter Server .* is running.*$")),
("shell", {}, re.compile("^.*Server listening on.*$")),
("tensorboard", {}, re.compile("^.*TensorBoard .* at .*$")),
],
)
def test_task_logs(task_type: str, task_config: Dict[str, Any], log_regex: Any) -> None:
Expand All @@ -82,21 +81,21 @@ def test_task_logs(task_type: str, task_config: Dict[str, Any], log_regex: Any)
rps = bindings.get_GetResourcePools(session)
assert rps.resourcePools and len(rps.resourcePools) > 0, "missing resource pool"

if task_type == command.TaskTypeTensorBoard:
if task_type == "tensorboard":
exp_id = exp.run_basic_test(
conf.fixtures_path("no_op/single.yaml"),
conf.fixtures_path("no_op"),
1,
)
treq = bindings.v1LaunchTensorboardRequest(config=task_config, experimentIds=[exp_id])
task_id = bindings.post_LaunchTensorboard(session, body=treq).tensorboard.id
elif task_type == command.TaskTypeNotebook:
elif task_type == "notebook":
nreq = bindings.v1LaunchNotebookRequest(config=task_config)
task_id = bindings.post_LaunchNotebook(session, body=nreq).notebook.id
elif task_type == command.TaskTypeCommand:
elif task_type == "command":
creq = bindings.v1LaunchCommandRequest(config=task_config)
task_id = bindings.post_LaunchCommand(session, body=creq).command.id
elif task_type == command.TaskTypeShell:
elif task_type == "shell":
sreq = bindings.v1LaunchShellRequest(config=task_config)
task_id = bindings.post_LaunchShell(session, body=sreq).shell.id
else:
Expand Down Expand Up @@ -139,7 +138,14 @@ def do_check_logs() -> None:
raise

finally:
command._kill(master_url, task_type, task_id)
if task_type == "tensorboard":
bindings.post_KillTensorboard(session, tensorboardId=task_id)
elif task_type == "notebook":
bindings.post_KillNotebook(session, notebookId=task_id)
elif task_type == "command":
bindings.post_KillCommand(session, commandId=task_id)
elif task_type == "shell":
bindings.post_KillShell(session, shellId=task_id)


def check_logs(
Expand Down
14 changes: 6 additions & 8 deletions e2e_tests/tests/cluster/test_model_registry_rbac.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

import pytest

from determined import cli
from determined.common import util
from determined.common.api import Session, authentication, bindings, errors
from determined.common.api.bindings import experimentv1State
Expand All @@ -12,7 +11,7 @@
from determined.experimental import client as _client
from tests import api_utils
from tests import config as conf
from tests import experiment, utils
from tests import experiment
from tests.cluster.test_rbac import create_workspaces_with_users
from tests.cluster.test_users import log_in_user_cli, logged_in_user

Expand Down Expand Up @@ -352,12 +351,11 @@ def test_model_rbac_deletes() -> None:
add_password=True,
user=bindings.v1User(username=get_random_string(), active=True, admin=False),
)
api_utils.configure_token_store(conf.ADMIN_CREDENTIALS)
cli.rbac.assign_role(
utils.CliArgsMock(
username_to_assign=cluster_admin_creds.username,
role_name="ClusterAdmin",
)
api_utils.assign_user_role(
session=editor_session,
user=cluster_admin_creds.username,
role="ClusterAdmin",
workspace=None,
)
cluster_admin_session = api_utils.determined_test_session(cluster_admin_creds)

Expand Down
66 changes: 31 additions & 35 deletions e2e_tests/tests/cluster/test_rbac.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,14 @@

import pytest

from determined import cli
from determined.cli.user_groups import group_name_to_group_id, usernames_to_user_ids
from determined.common import api
from determined.common.api import authentication, bindings, errors
from tests import api_utils
from tests import config as conf
from tests import utils
from tests.api_utils import configure_token_store, create_test_user, determined_test_session
from tests.cluster.test_workspace_org import setup_workspaces

from .test_groups import det_cmd, det_cmd_expect_error, det_cmd_json
from .test_users import get_random_string, logged_in_user
from .test_users import logged_in_user


def roles_not_implemented() -> bool:
Expand All @@ -25,7 +21,7 @@ def rbac_disabled() -> bool:
if roles_not_implemented():
return True
try:
return not bindings.get_GetMaster(determined_test_session()).rbacEnabled
return not bindings.get_GetMaster(api_utils.determined_test_session()).rbacEnabled
except (errors.APIException, errors.MasterNotFoundException):
return True

Expand All @@ -34,7 +30,7 @@ def strict_q_control_disabled() -> bool:
if roles_not_implemented() or rbac_disabled():
return True
try:
return not bindings.get_GetMaster(determined_test_session()).strictJobQueueControl
return not bindings.get_GetMaster(api_utils.determined_test_session()).strictJobQueueControl
except (errors.APIException, errors.MasterNotFoundException):
return True

Expand All @@ -61,18 +57,18 @@ def create_users_with_gloabl_roles(user_roles: List[List[str]]) -> List[authenti
user_roles: list of roles to assign to each user, one entry per user.
"""
user_creds: List[authentication.Credentials] = []
with logged_in_user(conf.ADMIN_CREDENTIALS):
for roles in user_roles:
user = bindings.v1User(username=api_utils.get_random_string(), admin=False, active=True)
creds = api_utils.create_test_user(True, user=user)
for role in roles:
cli.rbac.assign_role(
utils.CliArgsMock(
username_to_assign=creds.username,
role_name=role,
)
)
user_creds.append(creds)
sess = api_utils.determined_test_session(admin=True)
for roles in user_roles:
user = bindings.v1User(username=api_utils.get_random_string(), admin=False, active=True)
creds = api_utils.create_test_user(True, user=user)
for role in roles:
api_utils.assign_user_role(
session=sess,
user=creds.username,
role=role,
workspace=None,
)
user_creds.append(creds)
return user_creds


Expand Down Expand Up @@ -104,20 +100,20 @@ def create_workspaces_with_users(
]
]
"""
configure_token_store(conf.ADMIN_CREDENTIALS)
sess = api_utils.determined_test_session(admin=True)
api_utils.configure_token_store(conf.ADMIN_CREDENTIALS)
rid_to_creds: Dict[int, authentication.Credentials] = {}
with setup_workspaces(count=len(assignments_list)) as workspaces:
for workspace, user_list in zip(workspaces, assignments_list):
for rid, roles in user_list:
if rid not in rid_to_creds:
rid_to_creds[rid] = create_test_user()
rid_to_creds[rid] = api_utils.create_test_user()
for role in roles:
cli.rbac.assign_role(
utils.CliArgsMock(
username_to_assign=rid_to_creds[rid].username,
workspace_name=workspace.name,
role_name=role,
)
api_utils.assign_user_role(
session=sess,
user=rid_to_creds[rid].username,
role=role,
workspace=workspace.name,
)
yield workspaces, rid_to_creds

Expand Down Expand Up @@ -152,7 +148,7 @@ def test_rbac_permission_assignment() -> None:
assert json_out["roles"] == []
assert json_out["assignments"] == []

group_name = get_random_string()
group_name = api_utils.get_random_string()
with logged_in_user(conf.ADMIN_CREDENTIALS):
# Assign user to role directly.
det_cmd(
Expand Down Expand Up @@ -378,7 +374,7 @@ def test_rbac_permission_assignment_errors() -> None:
)

test_user_creds = api_utils.create_test_user()
group_name = get_random_string()
group_name = api_utils.get_random_string()
det_cmd(["user-group", "create", group_name], check=True)
det_cmd(["rbac", "assign-role", "Viewer", "--group-name-to-assign", group_name], check=True)
det_cmd(
Expand Down Expand Up @@ -461,7 +457,7 @@ def test_rbac_list_roles() -> None:
# Set up group/user to test with.
api_utils.configure_token_store(conf.ADMIN_CREDENTIALS)
test_user_creds = api_utils.create_test_user()
group_name = get_random_string()
group_name = api_utils.get_random_string()
det_cmd(
["user-group", "create", group_name, "--add-user", test_user_creds.username], check=True
)
Expand Down Expand Up @@ -560,7 +556,7 @@ def test_rbac_describe_role() -> None:
# Role is assigned to our group and user.
api_utils.configure_token_store(conf.ADMIN_CREDENTIALS)
test_user_creds = api_utils.create_test_user()
group_name = get_random_string()
group_name = api_utils.get_random_string()

det_cmd(["user-group", "create", group_name], check=True)
det_cmd(["rbac", "assign-role", "Viewer", "--group-name-to-assign", group_name], check=True)
Expand All @@ -578,8 +574,8 @@ def test_rbac_describe_role() -> None:
)

sess = api_utils.determined_test_session(conf.ADMIN_CREDENTIALS)
user_id = usernames_to_user_ids(sess, [test_user_creds.username])[0]
group_id = group_name_to_group_id(sess, group_name)
user_id = api.usernames_to_user_ids(sess, [test_user_creds.username])[0]
group_id = api.group_name_to_group_id(sess, group_name)

det_cmd(
["rbac", "assign-role", "Viewer", "--username-to-assign", test_user_creds.username],
Expand Down Expand Up @@ -624,8 +620,8 @@ def test_rbac_describe_role() -> None:
@pytest.mark.skipif(roles_not_implemented(), reason="ee is required for this test")
def test_group_access() -> None:
# create relevant workspace and project, with group having access
group_name = get_random_string()
workspace_name = get_random_string()
group_name = api_utils.get_random_string()
workspace_name = api_utils.get_random_string()
with logged_in_user(conf.ADMIN_CREDENTIALS):
det_cmd(["workspace", "create", workspace_name], check=True)
det_cmd(["user-group", "create", group_name], check=True)
Expand Down
47 changes: 19 additions & 28 deletions e2e_tests/tests/cluster/test_rbac_misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,8 @@

import pytest

from determined import cli
from determined.common.api import authentication, bindings, errors
from tests import config as conf
from tests import utils
from tests.api_utils import (
configure_token_store,
create_test_user,
determined_test_session,
get_random_string,
)
from tests import api_utils
from tests.cluster.test_rbac import create_workspaces_with_users, rbac_disabled


Expand All @@ -28,26 +20,23 @@ def test_cluster_admin_only_calls() -> None:
],
]
) as (_, creds):
u_admin_role = create_test_user(
u_admin_role = api_utils.create_test_user(
add_password=True,
user=bindings.v1User(username=get_random_string(), active=True, admin=False),
user=bindings.v1User(username=api_utils.get_random_string(), active=True, admin=False),
)
configure_token_store(conf.ADMIN_CREDENTIALS)
cli.rbac.assign_role(
utils.CliArgsMock(
username_to_assign=u_admin_role.username,
role_name="ClusterAdmin",
)
session = api_utils.determined_test_session(admin=True)
api_utils.assign_user_role(
session=session, user=u_admin_role.username, role="ClusterAdmin", workspace=None
)

# normal determined admins without ClusterAdmin role.
u_det_admin = create_test_user(
u_det_admin = api_utils.create_test_user(
add_password=True,
user=bindings.v1User(username=get_random_string(), active=True, admin=True),
user=bindings.v1User(username=api_utils.get_random_string(), active=True, admin=True),
)

def get_agent_slot_ids(creds: authentication.Credentials) -> Tuple[str, str]:
session = determined_test_session(creds)
session = api_utils.determined_test_session(creds)
agents = sorted(bindings.get_GetAgents(session).agents, key=lambda a: a.id)
assert len(agents) > 0
agent = agents[0]
Expand All @@ -58,31 +47,31 @@ def get_agent_slot_ids(creds: authentication.Credentials) -> Tuple[str, str]:
return agent.id, slot_id

def enable_agent(creds: authentication.Credentials) -> None:
session = determined_test_session(creds)
session = api_utils.determined_test_session(creds)
agent_id, _ = get_agent_slot_ids(creds)
bindings.post_EnableAgent(session, agentId=agent_id)

def disable_agent(creds: authentication.Credentials) -> None:
session = determined_test_session(creds)
session = api_utils.determined_test_session(creds)
agent_id, _ = get_agent_slot_ids(creds)
bindings.post_DisableAgent(
session, agentId=agent_id, body=bindings.v1DisableAgentRequest(agentId=agent_id)
)

def enable_slot(creds: authentication.Credentials) -> None:
session = determined_test_session(creds)
session = api_utils.determined_test_session(creds)
agent_id, slot_id = get_agent_slot_ids(creds)
bindings.post_EnableSlot(session, agentId=agent_id, slotId=slot_id)

def disable_slot(creds: authentication.Credentials) -> None:
session = determined_test_session(creds)
session = api_utils.determined_test_session(creds)
agent_id, slot_id = get_agent_slot_ids(creds)
bindings.post_DisableSlot(
session, agentId=agent_id, slotId=slot_id, body=bindings.v1DisableSlotRequest()
)

def get_master_logs(creds: authentication.Credentials) -> None:
logs = list(bindings.get_MasterLogs(determined_test_session(creds), limit=2))
logs = list(bindings.get_MasterLogs(api_utils.determined_test_session(creds), limit=2))
assert len(logs) == 2

def get_allocations_raw(creds: authentication.Credentials) -> None:
Expand All @@ -91,7 +80,9 @@ def get_allocations_raw(creds: authentication.Credentials) -> None:
start_str = start.strftime(EXPECTED_TIME_FMT)
end_str = (start + datetime.timedelta(seconds=1)).strftime(EXPECTED_TIME_FMT)
entries = bindings.get_ResourceAllocationRaw(
determined_test_session(creds), timestampAfter=start_str, timestampBefore=end_str
api_utils.determined_test_session(creds),
timestampAfter=start_str,
timestampBefore=end_str,
).resourceEntries
assert isinstance(entries, list)

Expand All @@ -100,7 +91,7 @@ def get_allocations_aggregated(creds: authentication.Credentials) -> None:
start = datetime.datetime.now()
end = start + datetime.timedelta(seconds=1)
entries = bindings.get_ResourceAllocationAggregated(
determined_test_session(creds),
api_utils.determined_test_session(creds),
# fmt: off
period=bindings.v1ResourceAllocationAggregationPeriod\
.DAILY,
Expand All @@ -117,7 +108,7 @@ def get_allocations_raw_echo(creds: authentication.Credentials) -> None:
end_str = (start + datetime.timedelta(seconds=1)).strftime(EXPECTED_TIME_FMT)
url = "/resources/allocation/raw"
params = {"timestamp_after": start_str, "timestamp_before": end_str}
session = determined_test_session(creds)
session = api_utils.determined_test_session(creds)
response = session.get(url, params=params)
assert response.status_code == 200

Expand Down
Loading