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.

Sign up for GitHub

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

Jump to bottom

chore: revert task obfuscation lint failures #8406

Merged
merged 2 commits into from
Nov 13, 2023
Merged

chore: revert task obfuscation lint failures #8406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17886bc
noop
amandavialva01 Nov 13, 2023
e6bdf74
revert: fix det slot for no-permission RBAC users
amandavialva01 Nov 13, 2023
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
24 changes: 9 additions & 15 deletions harness/determined/cli/agent.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@
from determined.common.api import authentication, bindings
from determined.common.declarative_argparse import Arg, Cmd, Group

NO_PERMISSIONS = "NO PERMISSIONS"


def local_id(address: str) -> str:
return os.path.basename(address)
Expand All @@ -28,7 +26,7 @@ def list_agents(args: argparse.Namespace) -> None:
agents = [
collections.OrderedDict(
[
("id", local_id(a.id) if a.id else NO_PERMISSIONS),
("id", local_id(a.id)),
("version", a.version),
("registered_time", render.format_time(a.registeredTime)),
("num_slots", len(a.slots) if a.slots is not None else ""),
Expand Down Expand Up @@ -66,17 +64,16 @@ def list_agents(args: argparse.Namespace) -> None:

@authentication.required
def list_slots(args: argparse.Namespace) -> None:
task_res = bindings.get_GetTasks(cli.setup_session(args))
task_res = api.get(args.master, "tasks")
resp = bindings.get_GetAgents(cli.setup_session(args))

allocations = task_res.allocationIdToSummary if task_res.allocationIdToSummary else {}
allocations = task_res.json()

c_names = {
r.containerId: {"name": a.name, "allocation_id": a.allocationId if a.name else ""}
r["container_id"]: {"name": a["name"], "allocation_id": a["allocation_id"]}
for a in allocations.values()
for r in a.resources
if a.resources
if r.containerId
for r in a["resources"]
if r["container_id"]
}

def device_type_string(deviceType: typing.Optional[bindings.devicev1Type]) -> str:
Expand All @@ -95,8 +92,7 @@ def get_task_name(containers: Dict[str, Any], slot: bindings.v1Slot) -> str:
container_id = slot.container.id

if slot.container and container_id in containers:
name = str(containers[container_id]["name"])
return name if name else NO_PERMISSIONS
return str(containers[container_id]["name"])

if slot.container and (
"determined-master-deployment" in container_id
Expand All @@ -112,7 +108,7 @@ def get_task_name(containers: Dict[str, Any], slot: bindings.v1Slot) -> str:
slots = [
collections.OrderedDict(
[
("agent_id", local_id(agent.id) if agent.id else local_id(NO_PERMISSIONS)),
("agent_id", local_id(agent.id)),
(
"resource_pools",
", ".join(agent.resourcePools) if agent.resourcePools is not None else "",
Expand All @@ -123,9 +119,7 @@ def get_task_name(containers: Dict[str, Any], slot: bindings.v1Slot) -> str:
(
"allocation_id",
c_names[slot.container.id]["allocation_id"]
if slot.container
and slot.container.id in c_names
and c_names[slot.container.id]["name"]
if slot.container and slot.container.id in c_names
else ("OCCUPIED" if slot.container else "FREE"),
),
("task_name", get_task_name(c_names, slot)),
Expand Down
8 changes: 3 additions & 5 deletions harness/determined/cli/task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@
from determined.common.api.bindings import v1AllocationSummary
from determined.common.declarative_argparse import Arg, Cmd, Group

NO_PERMISSIONS = "NO PERMISSIONS"


def render_tasks(args: Namespace, tasks: Dict[str, v1AllocationSummary]) -> None:
"""Render tasks for JSON, tabulate or csv output.
Expand Down Expand Up @@ -47,9 +45,9 @@ def agent_info(t: v1AllocationSummary) -> Union[str, List[str]]:
]
values = [
[
task.taskId if task.name else NO_PERMISSIONS,
task.allocationId if task.name else NO_PERMISSIONS,
task.name if task.name else NO_PERMISSIONS,
task.taskId,
task.allocationId,
task.name,
task.slotsNeeded,
render.format_time(task.registeredTime),
agent_info(task),
Expand Down
14 changes: 3 additions & 11 deletions master/internal/api_tasks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,20 +534,12 @@ func (a *apiServer) GetTasks(
err = expauth.AuthZProvider.Get().CanGetExperiment(ctx, *curUser, exp)
}
if authz.IsPermissionDenied(err) {
obfuscatedSummary, err := authz.ObfuscateNTSCTask(allocationSummary)
if err != nil {
return nil, err
}
if !isExp {
pbAllocationIDToSummary[string(authz.HiddenString)] = obfuscatedSummary.Proto()
} else {
pbAllocationIDToSummary[string(allocationID)] = allocationSummary.Proto()
}
continue
} else if err != nil {
return nil, err
} else {
pbAllocationIDToSummary[string(allocationID)] = allocationSummary.Proto()
}

pbAllocationIDToSummary[string(allocationID)] = allocationSummary.Proto()
}

return &apiv1.GetTasksResponse{AllocationIdToSummary: pbAllocationIDToSummary}, nil
Expand Down
182 changes: 11 additions & 171 deletions master/internal/api_tasks_intg_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ import (
"context"
"encoding/json"
"fmt"
"math/rand"
"slices"
"testing"
"time"

Expand All @@ -29,15 +27,11 @@ import (
"github.com/determined-ai/determined/master/internal/sproto"
taskPkg "github.com/determined-ai/determined/master/internal/task"
"github.com/determined-ai/determined/master/pkg/actor"
"github.com/determined-ai/determined/master/pkg/aproto"
"github.com/determined-ai/determined/master/pkg/cproto"
"github.com/determined-ai/determined/master/pkg/device"
"github.com/determined-ai/determined/master/pkg/model"
"github.com/determined-ai/determined/master/pkg/ptrs"
"github.com/determined-ai/determined/master/pkg/schemas/expconf"
"github.com/determined-ai/determined/proto/pkg/apiv1"
"github.com/determined-ai/determined/proto/pkg/checkpointv1"
"github.com/determined-ai/determined/proto/pkg/devicev1"
"github.com/determined-ai/determined/proto/pkg/logv1"
"github.com/determined-ai/determined/proto/pkg/taskv1"
)
Expand Down Expand Up @@ -113,99 +107,9 @@ func TestPostTaskLogs(t *testing.T) {
}
}

func validateResponseSummary(t *testing.T, expectedTaskSummary *taskv1.AllocationSummary,
taskSummary *taskv1.AllocationSummary,
) {
require.Equal(t, expectedTaskSummary.TaskId, taskSummary.TaskId)
require.Equal(t, expectedTaskSummary.AllocationId, taskSummary.AllocationId)
require.Equal(t, expectedTaskSummary.Name, taskSummary.Name)

require.Equal(t, len(expectedTaskSummary.Resources), len(taskSummary.Resources))

for i, r := range taskSummary.Resources {
expectedResource := expectedTaskSummary.Resources[i]
require.Equal(t, expectedResource.ResourcesId, r.ResourcesId)
require.Equal(t, expectedResource.AllocationId, r.AllocationId)
var contID string = *r.ContainerId
var expectedContID string = *expectedResource.ContainerId
require.Equal(t, string(expectedContID), string(contID))
require.Equal(t, len(expectedResource.AgentDevices), len(r.AgentDevices))
require.ElementsMatch(t, maps.Keys(expectedResource.AgentDevices),
maps.Keys(r.AgentDevices))
for devName, devs := range expectedResource.AgentDevices {
for ind, dev := range devs.Devices {
agentDevice := r.AgentDevices[devName].Devices[ind]
require.Equal(t, dev.Brand, agentDevice.Brand)
require.Equal(t, dev.Uuid, agentDevice.Uuid)
}
}
}

require.Equal(t, len(expectedTaskSummary.ProxyPorts), len(taskSummary.ProxyPorts))
for ind, pp := range taskSummary.ProxyPorts {
expProxyPortConfig := expectedTaskSummary.ProxyPorts[ind]
require.Equal(t, expProxyPortConfig.ServiceId, pp.ServiceId)
require.Equal(t, int(expProxyPortConfig.Port), int(pp.Port))
require.Equal(t, expProxyPortConfig.ProxyTcp, pp.ProxyTcp)
require.Equal(t, expProxyPortConfig.Unauthenticated, pp.Unauthenticated)
}
}

func CheckObfuscatedTask(t *testing.T, taskSummary *taskv1.AllocationSummary, id string,
allocs map[model.AllocationID]sproto.AllocationSummary, permissions []string,
) error {
hasPermissions := slices.Contains(permissions, id)
allocSummary := allocs[model.AllocationID(id)]
if hasPermissions {
validateResponseSummary(t, allocSummary.Proto(), taskSummary)
} else {
// Create expected obfuscated task summary.
obfuscatedSummary := &taskv1.AllocationSummary{}
obfuscatedSummary.TaskId = authz2.HiddenString
obfuscatedSummary.AllocationId = authz2.HiddenString
obfuscatedSummary.Name = ""

for _, r := range taskSummary.Resources {
resource := &taskv1.ResourcesSummary{}
resource.ResourcesId = authz2.HiddenString
resource.AllocationId = authz2.HiddenString
var contID string = string(authz2.HiddenString)
resource.ContainerId = &contID
agentDevice := make(map[string]*taskv1.ResourcesSummary_Devices)
for _, devs := range r.AgentDevices {
obfuscatedDevs := &taskv1.ResourcesSummary_Devices{}
obfuscatedDevices := make([]*devicev1.Device, len(devs.Devices))
for ind := range devs.Devices {
obfuscatedDev := &devicev1.Device{
Brand: authz2.HiddenString,
Uuid: authz2.HiddenString,
}
obfuscatedDevices[ind] = obfuscatedDev
}
obfuscatedDevs.Devices = obfuscatedDevices
agentDevice[authz2.HiddenString] = obfuscatedDevs
}
resource.AgentDevices = agentDevice
obfuscatedSummary.Resources = append(obfuscatedSummary.Resources, resource)
}
proxyPortConfs := make([]*taskv1.ProxyPortConfig, len(taskSummary.ProxyPorts))
for ind := range taskSummary.ProxyPorts {
ppConf := &taskv1.ProxyPortConfig{}
ppConf.ServiceId = authz2.HiddenString
ppConf.Port = authz2.HiddenInt
ppConf.ProxyTcp = authz2.HiddenBool
ppConf.Unauthenticated = authz2.HiddenBool
proxyPortConfs[ind] = ppConf
}
obfuscatedSummary.ProxyPorts = proxyPortConfs
validateResponseSummary(t, obfuscatedSummary, taskSummary)
}
return nil
}

func mockNotebookWithWorkspaceID(
ctx context.Context, api *apiServer, t *testing.T, workspaceID int,
) (model.TaskID, model.AllocationID) {
) model.TaskID {
nb := &model.Task{
TaskID: model.NewTaskID(),
TaskType: model.TaskTypeNotebook,
Expand Down Expand Up @@ -238,57 +142,7 @@ func mockNotebookWithWorkspaceID(
}).Exec(ctx)
require.NoError(t, err)

return nb.TaskID, allocationID
}

func getRandomString() string {
randomUUID := uuid.New()
return randomUUID.String()
}

func mockAllocationSummary(taskID model.TaskID,
allocID model.AllocationID,
) sproto.AllocationSummary {
grs := getRandomString
summary := sproto.AllocationSummary{
TaskID: taskID, AllocationID: allocID, Name: grs(),
}

// Since mockAllocationSummary is used in conjunction with testing obfuscated tasks and
// obfuscating a boolean just sets it to false, we set defaultBool to true so that we can
// test whether or not the value gets obscured.
defaultBool := true
pPortConf := sproto.ProxyPortConfig{
ServiceID: grs(),
Port: 0, ProxyTCP: defaultBool, Unauthenticated: defaultBool,
}
pPortConfs := []*sproto.ProxyPortConfig{&pPortConf}

resID := sproto.ResourcesID(grs())
restype := sproto.ResourcesType(grs())
agentDevices := make(map[aproto.ID][]device.Device)
aPID := aproto.ID(grs())
var devs []device.Device

// Same as above; since obfuscating an int just sets it to -1, we set devID to any
// non-negative int so that we can test whether or not the value gets obscured.
devID := rand.Int()
dev := device.Device{
ID: device.ID(devID), Brand: grs(), UUID: grs(), Type: device.Type(grs()),
}
devs = append(devs, dev)
agentDevices[aPID] = devs
resContID := cproto.ID(grs())
resourcesSummary := sproto.ResourcesSummary{
ResourcesID: resID, ResourcesType: restype, AllocationID: allocID,
AgentDevices: agentDevices, ContainerID: &resContID,
}
resourcesSummaries := []sproto.ResourcesSummary{resourcesSummary}

summary.Resources = resourcesSummaries
summary.ProxyPorts = pPortConfs

return summary
return nb.TaskID
}

func TestGetTasksAuthZ(t *testing.T) {
Expand All @@ -314,47 +168,33 @@ func TestGetTasksAuthZ(t *testing.T) {
return e.ID == cantAccessTrial.ExperimentID
})).Return(authz2.PermissionDeniedError{}).Once()

canAccessNotebookID, canAccessAllocationID := mockNotebookWithWorkspaceID(ctx, api, t, -100)
canAccessNotebookID := mockNotebookWithWorkspaceID(ctx, api, t, -100)
authZNSC.On("CanGetNSC", mock.Anything, mock.Anything, model.AccessScopeID(-100)).
Return(nil).Once()

cantAccessNotebookID, cantAccessAllocationID := mockNotebookWithWorkspaceID(ctx, api, t, -101)
cantAccessNotebookID := mockNotebookWithWorkspaceID(ctx, api, t, -101)
authZNSC.On("CanGetNSC", mock.Anything, mock.Anything, model.AccessScopeID(-101)).
Return(authz2.PermissionDeniedError{}).Once()

summaryAccess := mockAllocationSummary(canAccessNotebookID, canAccessAllocationID)
summaryNoAccess := mockAllocationSummary(cantAccessNotebookID, cantAccessAllocationID)

allocations = map[model.AllocationID]sproto.AllocationSummary{
"alloc0": {
TaskID: expCanAccessTask.TaskID,
},
"alloc1": {
TaskID: expCantAccessTask.TaskID,
},
"alloc2": summaryAccess,
"alloc3": summaryNoAccess,
"alloc2": {
TaskID: canAccessNotebookID,
},
"alloc3": {
TaskID: cantAccessNotebookID,
},
}

NTSCTasks := make(map[string]int)
NTSCTasks["alloc2"] = 0
NTSCTasks[authz2.HiddenString] = 0

resp, err := api.GetTasks(ctx, &apiv1.GetTasksRequest{})
require.NoError(t, err)

require.ElementsMatch(t, []string{"alloc0", "alloc1", "alloc2", authz2.HiddenString},
maps.Keys(resp.AllocationIdToSummary))

// Check that NTSC tasks that are accessed by user with no permissions are obfuscated.
permissions := []string{"alloc2"}
for id, summary := range resp.AllocationIdToSummary {
_, contains := NTSCTasks[id]
if contains {
err := CheckObfuscatedTask(t, summary, id, allocations, permissions)
require.NoError(t, err)
}
}
require.ElementsMatch(t, []string{"alloc0", "alloc2"}, maps.Keys(resp.AllocationIdToSummary))
}

func TestPostTaskLogsLogPattern(t *testing.T) {
Expand Down
Loading
Loading