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

add high-assurance placeholder option to endpoint configuration #1720

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
33 changes: 25 additions & 8 deletions compute_endpoint/globus_compute_endpoint/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,13 @@ def version_command():
default=False,
help="Configure endpoint as multi-user capable",
)
@click.option(
"--high-assurance",
is_flag=True,
default=False,
hidden=True, # Until HA features are complete
help="Configure endpoint as high assurance capable",
)
@click.option(
"--display-name",
help="A human readable display name for the endpoint, if desired",
Expand Down Expand Up @@ -352,7 +359,7 @@ def version_command():
"--auth-timeout",
help=(
"How old (in seconds) a login session can be and still be compliant. "
"Makes the auth policy high assurance"
"If set, the auth policy will be high assurance"
),
type=click.IntRange(min=0),
default=None,
Expand All @@ -366,6 +373,7 @@ def configure_endpoint(
name: str,
endpoint_config: str | None,
multi_user: bool,
high_assurance: bool,
display_name: str | None,
auth_policy: str | None,
auth_policy_project_id: str | None,
Expand All @@ -389,20 +397,28 @@ def configure_endpoint(
if multi_user and not _has_multi_user:
raise ClickException("multi-user endpoints are not supported on this system")

if (
create_policy = (
auth_policy_project_id is not None
or auth_policy_display_name != _AUTH_POLICY_DEFAULT_NAME
or auth_policy_description != _AUTH_POLICY_DEFAULT_DESC
or allowed_domains is not None
or excluded_domains is not None
or auth_timeout is not None
):
if auth_policy:
raise ClickException(
"Cannot specify an existing auth policy and "
"create a new one at the same time"
)
)

if create_policy and auth_policy:
raise ClickException(
"Cannot specify an existing auth policy and "
"create a new one at the same time"
)
elif (
(not create_policy and not bool(auth_policy)) or not subscription_id
) and high_assurance:
raise ClickException(
"high-assurance(HA) endpoints require both a HA policy and "
"a HA subscription id"
)
elif create_policy:
app = get_globus_app_with_scopes()
ac = ComputeAuthClient(app=app)

Expand Down Expand Up @@ -433,6 +449,7 @@ def configure_endpoint(
ep_dir,
endpoint_config,
multi_user,
high_assurance,
display_name,
auth_policy,
subscription_id,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ def __init__(
self,
*,
multi_user: bool = False,
high_assurance: bool = False,
display_name: str | None = None,
allowed_functions: t.Iterable[UUID_LIKE_T] | None = None,
authentication_policy: UUID_LIKE_T | None = None,
Expand All @@ -75,6 +76,7 @@ def __init__(
self.display_name = display_name
self.debug = debug is True
self.multi_user = multi_user is True
self.high_assurance = high_assurance is True

# Connection info and tuning
self.amqp_port = amqp_port
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

config = UserEndpointConfig(
display_name=None, # If None, defaults to the endpoint name
high_assurance=False,
executors=[
Comment on lines 6 to 8
Copy link
Contributor

Choose a reason for hiding this comment

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

If it defaults to False, per config.py, why include this at all in the default_config.py? Especially since this is yet a hidden feature (per "in-development!") given cli.py?

GlobusComputeEngine(
provider=LocalProvider(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ def inner(cls, model: t.Optional[BaseModel]):

class BaseConfigModel(BaseModel):
multi_user: t.Optional[bool]
high_assurance: t.Optional[bool]
Comment on lines 45 to +47
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, need to rebase on main, we just updated this per the 2.32.1 bug.

display_name: t.Optional[str]
allowed_functions: t.Optional[t.List[uuid.UUID]]
authentication_policy: t.Optional[uuid.UUID]
Expand Down
9 changes: 9 additions & 0 deletions compute_endpoint/globus_compute_endpoint/endpoint/endpoint.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ def update_config_file(
original_path: pathlib.Path,
target_path: pathlib.Path,
multi_user: bool,
high_assurance: bool,
display_name: str | None,
auth_policy: str | None,
subscription_id: str | None,
Expand All @@ -89,6 +90,9 @@ def update_config_file(
if auth_policy:
config_dict["authentication_policy"] = auth_policy

if high_assurance:
config_dict["high_assurance"] = high_assurance

if multi_user:
config_dict["multi_user"] = multi_user
config_dict.pop("engine", None)
Expand All @@ -109,6 +113,7 @@ def init_endpoint_dir(
endpoint_dir: pathlib.Path,
endpoint_config: pathlib.Path | None = None,
multi_user=False,
high_assurance=False,
display_name: str | None = None,
auth_policy: str | None = None,
subscription_id: str | None = None,
Expand Down Expand Up @@ -143,6 +148,7 @@ def init_endpoint_dir(
endpoint_config,
config_target_path,
multi_user,
high_assurance,
display_name,
auth_policy,
subscription_id,
Expand Down Expand Up @@ -187,6 +193,7 @@ def configure_endpoint(
conf_dir: pathlib.Path,
endpoint_config: str | None,
multi_user: bool = False,
high_assurance: bool = False,
display_name: str | None = None,
auth_policy: str | None = None,
subscription_id: str | None = None,
Expand All @@ -202,6 +209,7 @@ def configure_endpoint(
conf_dir,
templ_conf_path,
multi_user,
high_assurance,
display_name,
auth_policy,
subscription_id,
Expand Down Expand Up @@ -428,6 +436,7 @@ def start_endpoint(
allowed_functions=endpoint_config.allowed_functions,
auth_policy=endpoint_config.authentication_policy,
subscription_id=endpoint_config.subscription_id,
high_assurance=endpoint_config.high_assurance,
)

except GlobusAPIError as e:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ def __init__(
auth_policy=config.authentication_policy,
subscription_id=config.subscription_id,
public=config.public,
high_assurance=config.high_assurance,
)

# Mostly to appease mypy, but also a useful text if it ever
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,24 +60,33 @@ def test_double_configure(self):
with pytest.raises(Exception, match="ConfigExists"):
manager.configure_endpoint(config_dir, None)

@pytest.mark.parametrize("ha", [None, True, False])
@pytest.mark.parametrize("mu", [None, True, False])
def test_configure_multi_user_existing_config(self, mu):
def test_configure_multi_user_ha_existing_config(self, ha, mu):
manager = Endpoint()
config_dir = pathlib.Path("/some/path/mock_endpoint")
config_file = Endpoint._config_file_path(config_dir)
config_copy = str(config_dir.parent / "config2.yaml")

# First, make an entry with multi_user
manager.configure_endpoint(config_dir, None, multi_user=True)
# First, make an entry with multi_user/ha
manager.configure_endpoint(
config_dir, None, multi_user=True, high_assurance=False
)
shutil.move(config_file, config_copy)
shutil.rmtree(config_dir)

# Then, modify it with new setting
manager.configure_endpoint(config_dir, config_copy, multi_user=mu)
# Then, modify it with new settings
manager.configure_endpoint(
config_dir, config_copy, multi_user=mu, high_assurance=ha
)

with open(config_file) as f:
config_dict = yaml.safe_load(f)
assert "multi_user" in config_dict
if ha is True:
assert "high_assurance" in config_dict
else:
assert "high_assurance" not in config_dict
Comment on lines +86 to +89
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be reduced to:

assert ("high_assurance" in config_dict) is (ha is True)


os.remove(config_copy)

Expand Down
2 changes: 2 additions & 0 deletions compute_endpoint/tests/unit/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ def pytest_configure(config):
"local_compute_services": True,
"environment": str,
"multi_user": False,
"high_assurance": False,
"executors": None,
}

Expand All @@ -55,6 +56,7 @@ def pytest_configure(config):
"local_compute_services": True,
"environment": str,
"multi_user": True,
"high_assurance": True,
}


Expand Down
98 changes: 98 additions & 0 deletions compute_endpoint/tests/unit/test_cli_behavior.py
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,37 @@ def test_start_ep_display_name_in_config(
assert conf_dict["display_name"] == display_name


@pytest.mark.parametrize(
"ha_test_info",
[
["ep0", None],
["ep1", False],
["ep2", True],
],
)
def test_start_ep_high_assurance_in_config(
run_line, mock_command_ensure, make_endpoint_dir, ha_test_info
):
ep_name, is_ha = ha_test_info

conf = mock_command_ensure.endpoint_config_dir / ep_name / "config.yaml"
configure_arg = ""
auth_policy = str(uuid.uuid4())
sub_id = str(uuid.uuid4())
if is_ha is not None:
configure_arg = (
f" --subscription-id {sub_id} --auth-policy "
f"{auth_policy} --high-assurance"
)
run_line(f"configure {ep_name}{configure_arg}")

with open(conf) as f:
conf_dict = yaml.safe_load(f)

if is_ha is True:
assert conf_dict["high_assurance"] == is_ha


def test_configure_ep_auth_policy_in_config(
run_line, mock_command_ensure, make_endpoint_dir
):
Expand Down Expand Up @@ -1045,6 +1076,73 @@ def test_configure_ep_auth_policy_timeout_sets_ha(
)


@pytest.mark.parametrize(
("is_ha", "policy_id", "auth_desc", "allowed_domains", "sub_id", "exc_text"),
(
(
[True, "pid", None, None, "sub_id", None],
[True, "pid", None, None, "sub_id", None],
[True, "pid", "desc", "globus.org", "sub_id", "Cannot specify an existing"],
[True, "pid", "desc", None, "sub_id", "Cannot specify an existing"],
[True, None, None, None, "sub_id", "require both a HA policy and a HA sub"],
[True, "pid", None, None, None, "require both a HA policy and a HA sub"],
[
True,
None,
"auth_desc",
"globus.org",
None,
"require both a HA policy and a HA sub",
],
[False, None, "auth_desc", "globus.org", None, None],
[False, "pid", None, None, None, None],
)
),
)
def test_configure_ha_ep_requirements(
mocker,
run_line,
mock_cli_state,
make_endpoint_dir,
ep_name,
mock_app,
mock_auth_client,
is_ha,
policy_id,
auth_desc,
allowed_domains,
sub_id,
exc_text,
):
mock_auth_client.create_policy.return_value = {"policy": {"id": "foo"}}
mock_auth_client.get_projects.return_value = []
mocker.patch(f"{_MOCK_BASE}create_or_choose_auth_project")

mock_ep, _ = mock_cli_state

args = ["configure"]
if is_ha:
args.append("--high-assurance")
if policy_id:
args.append(f"--auth-policy {policy_id}")
if auth_desc:
args.append(f"--auth-policy-description {auth_desc}")
if allowed_domains:
args.append(f"--allowed-domains {allowed_domains}")
if sub_id:
args.append(f"--subscription-id {sub_id}")

args.append("ep_name")

line = " ".join(args)
if exc_text:
res = run_line(line, assert_exit_code=1)
assert exc_text in res.stderr
else:
run_line(line)
assert mock_ep.configure_endpoint.called


@pytest.mark.parametrize(
("delete_cmd", "use_uuid", "exit_code", "delete_done"),
[
Expand Down
4 changes: 2 additions & 2 deletions compute_endpoint/tests/unit/test_endpoint_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,8 @@ def test_userconfig_repr_nondefault_kwargs(

repr_c = repr(UserEndpointConfig(**kwds))

if kw == "multi_user":
assert f"{kw}={repr(val)}" not in repr_c, "Multi user *off* by default"
if kw in ["multi_user", "high_assurance"]:
assert f"{kw}={repr(val)}" not in repr_c, "Multi-user and HA *off* by default"
else:
assert f"{kw}={repr(val)}" in repr_c

Expand Down
1 change: 1 addition & 0 deletions compute_endpoint/tests/unit/test_endpointmanager_unit.py
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,7 @@ def test_sends_data_during_registration(
"endpoint_id",
"metadata",
"multi_user",
"high_assurance",
"display_name",
"allowed_functions",
"auth_policy",
Expand Down
4 changes: 4 additions & 0 deletions compute_sdk/globus_compute_sdk/sdk/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,7 @@ def register_endpoint(
auth_policy: UUID_LIKE_T | None = None,
subscription_id: UUID_LIKE_T | None = None,
public: bool | None = None,
high_assurance: bool | None = None,
):
"""Register an endpoint with the Globus Compute service.

Expand All @@ -437,6 +438,8 @@ def register_endpoint(
Endpoint metadata
multi_user : bool | None
Whether the endpoint supports multiple users
high_assurance : bool | None
Whether the endpoint should be high assurance capable
display_name : str | None
The display name of the endpoint
allowed_functions: list[str | UUID] | None
Expand Down Expand Up @@ -467,6 +470,7 @@ def register_endpoint(
auth_policy=auth_policy,
subscription_id=subscription_id,
public=public,
high_assurance=high_assurance,
)
return r.data

Expand Down
Loading
Loading