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

support projects #428

Merged
merged 9 commits into from
Mar 11, 2023
Merged
Show file tree
Hide file tree
Changes from 8 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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ htmlcov/
.nox/
.coverage
.coverage.*
.coverage-*
.cache
nosetests.xml
coverage.xml
Expand Down
74 changes: 46 additions & 28 deletions platform_secrets/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,57 +106,65 @@ def _secret_cluster_uri(self) -> str:
def _get_org_secrets_uri(self, org_name: str) -> str:
return f"{self._secret_cluster_uri}/{org_name}"

def _get_user_secrets_uri(self, user: User, org_name: Optional[str]) -> str:
def _get_secrets_uri(self, project_name: str, org_name: Optional[str]) -> str:
if org_name is None:
base = self._secret_cluster_uri
else:
base = self._get_org_secrets_uri(org_name)
return f"{base}/{user.name}"
return f"{base}/{project_name}"

def _get_user_secrets_read_perm(
self, user: User, org_name: Optional[str]
) -> Permission:
return Permission(self._get_user_secrets_uri(user, org_name), "read")
def _get_secret_uri(self, secret: Secret) -> str:
base = self._get_secrets_uri(secret.project_name, secret.org_name)
if secret.owner == secret.project_name:
return f"{base}/{secret.key}"
return base

def _get_secret_read_perm(self, secret: Secret) -> Permission:
return Permission(self._get_secret_uri(secret), "read")

def _get_user_secrets_write_perm(
self, user: User, org_name: Optional[str]
def _get_secrets_write_perm(
self, project_name: str, org_name: Optional[str]
) -> Permission:
return Permission(self._get_user_secrets_uri(user, org_name), "write")
return Permission(
self._get_secrets_uri(project_name, org_name),
"write",
)

def _convert_secret_to_payload(self, secret: Secret) -> dict[str, Optional[str]]:
return {"key": secret.key, "owner": secret.owner, "org_name": secret.org_name}
return {
"key": secret.key,
"org_name": secret.org_name,
"project_name": secret.project_name,
# NOTE: We store all user/project keys in one k8s secret.
# Project k8s secret can contain keys from multiple users, so
# there is no single owner of k8s secret, we loose owner when we work
# with project secrets.
# For backward compatibility leave `owner` field. It will
# contain username for secrets without project.
"owner": secret.project_name,
}

def _check_secret_read_perm(
self, secret: Secret, tree: ClientSubTreeViewRoot
) -> bool:
node = tree.sub_tree
if node.can_read():
return True
parts = secret.owner.split("/") + [secret.key]
if secret.org_name:
parts = [secret.org_name] + parts
try:
for part in parts:
if node.can_read():
return True
node = node.children[part]
return node.can_read()
except KeyError:
return False
return tree.allows(self._get_secret_read_perm(secret))

async def handle_post(self, request: Request) -> Response:
user = await self._get_untrusted_user(request)
payload = await request.json()
payload = secret_request_validator.check(payload)
org_name = payload.get("org_name")
project_name = payload.get("project_name", user.name)
await check_permissions(
request, [self._get_user_secrets_write_perm(user, org_name)]
request,
[self._get_secrets_write_perm(project_name, org_name)],
)
secret = Secret(
key=payload["key"],
value=payload["value"],
owner=user.name,
org_name=org_name,
project_name=project_name,
)
await self._service.add_secret(secret)
resp_payload = self._convert_secret_to_payload(secret)
Expand All @@ -165,12 +173,16 @@ async def handle_post(self, request: Request) -> Response:

async def handle_get_all(self, request: Request) -> Response:
username = await check_authorized(request)
org_name = request.query.get("org_name")
project_name = request.query.get("project_name")
tree = await self._auth_client.get_permissions_tree(
username, self._secret_cluster_uri
)
secrets = [
secret
for secret in await self._service.get_all_secrets()
for secret in await self._service.get_all_secrets(
org_name=org_name, project_name=project_name
)
if self._check_secret_read_perm(secret, tree)
]
resp_payload = [self._convert_secret_to_payload(secret) for secret in secrets]
Expand All @@ -180,12 +192,18 @@ async def handle_get_all(self, request: Request) -> Response:
async def handle_delete(self, request: Request) -> Response:
user = await self._get_untrusted_user(request)
org_name = request.query.get("org_name")
project_name = request.query.get("project_name") or user.name
await check_permissions(
request, [self._get_user_secrets_write_perm(user, org_name)]
request,
[self._get_secrets_write_perm(project_name, org_name)],
)
secret_key = request.match_info["key"]
secret_key = secret_key_validator.check(secret_key)
secret = Secret(key=secret_key, owner=user.name, org_name=org_name)
secret = Secret(
key=secret_key,
org_name=org_name,
project_name=project_name,
)
try:
await self._service.remove_secret(secret)
except SecretNotFound as exc:
Expand Down
9 changes: 7 additions & 2 deletions platform_secrets/kube_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from urllib.parse import urlsplit

import aiohttp
from yarl import URL

from .config import KubeClientAuthType

Expand Down Expand Up @@ -239,8 +240,12 @@ async def get_secret(
self._cleanup_secret_payload(payload)
return payload

async def list_secrets(self) -> list[dict[str, Any]]:
url = self._generate_all_secrets_url()
async def list_secrets(
self, label_selector: Optional[str] = None
) -> list[dict[str, Any]]:
url = URL(self._generate_all_secrets_url())
if label_selector:
url = url.with_query(labelSelector=label_selector)
payload = await self._request(method="GET", url=url)
self._raise_for_status(payload)
items = payload.get("items", [])
Expand Down
74 changes: 53 additions & 21 deletions platform_secrets/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

logger = logging.getLogger()

USER_LABEL = "platform.neuromation.io/user"
PROJECT_LABEL = "platform.neuromation.io/project"
SECRET_API_ORG_LABEL = "platform.neuromation.io/secret-api-org-name"


Expand All @@ -24,7 +26,8 @@ def create(cls, secret_key: str) -> "SecretNotFound":
@dataclass(frozen=True)
class Secret:
key: str
owner: str
project_name: str
owner: Optional[str] = None
value: str = field(repr=False, default="")
org_name: Optional[str] = None

Expand All @@ -33,17 +36,15 @@ class Service:
def __init__(self, kube_client: KubeClient) -> None:
self._kube_client = kube_client

def _get_kube_secret_name(self, owner: str, org_name: Optional[str]) -> str:
path = owner
def _get_kube_secret_name(self, project_name: str, org_name: Optional[str]) -> str:
path = project_name
if org_name:
path = f"{org_name}/{path}"
return f"user--{path.replace('/', '--')}--secrets"

def _get_org_owner_from_secret_name(
self,
secret_name: str,
org_name: Optional[str],
) -> tuple[Optional[str], Optional[str]]:
def _get_owner_from_secret_name(
self, secret_name: str, org_name: Optional[str]
) -> Optional[str]:
match = re.fullmatch(r"user--(?P<user_name>.*)--secrets", secret_name)
if match:
path: str = match.group("user_name").replace("--", "/")
Expand All @@ -52,11 +53,11 @@ def _get_org_owner_from_secret_name(
_, username = path.split("/", 1)
else:
username = path
return username, org_name
return None, None
return username
return None

async def add_secret(self, secret: Secret) -> None:
secret_name = self._get_kube_secret_name(secret.owner, secret.org_name)
secret_name = self._get_kube_secret_name(secret.project_name, secret.org_name)
try:
try:
await self._kube_client.add_secret_key(
Expand All @@ -66,6 +67,10 @@ async def add_secret(self, secret: Secret) -> None:
labels = {}
if secret.org_name:
labels[SECRET_API_ORG_LABEL] = secret.org_name
if secret.project_name == secret.owner:
labels[USER_LABEL] = secret.owner.replace("/", "--")
Copy link
Contributor

@YevheniiSemendiak YevheniiSemendiak Mar 10, 2023

Choose a reason for hiding this comment

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

Unclear how we use this label later, maybe you could elaborate? (maybe, the plan was to utilize it in get_all_secrets instead of _get_owner_from_secret_name)

Copy link
Contributor

Choose a reason for hiding this comment

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

why wasn't this comment addressed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, didn't notice your comment. Yes, I think the plan was to utilize it in get_all_secrets first but then I decided to refuse from this approach. This should be removed, will do.

else:
labels[PROJECT_LABEL] = secret.project_name
await self._kube_client.create_secret(
secret_name, {secret.key: secret.value}, labels=labels
)
Expand All @@ -74,30 +79,57 @@ async def add_secret(self, secret: Secret) -> None:
raise ValueError(f"Secret key {secret.key!r} or its value not valid")

async def remove_secret(self, secret: Secret) -> None:
secret_name = self._get_kube_secret_name(secret.owner, secret.org_name)
secret_name = self._get_kube_secret_name(secret.project_name, secret.org_name)
try:
await self._kube_client.remove_secret_key(secret_name, secret.key)
except (ResourceNotFound, ResourceInvalid):
raise SecretNotFound.create(secret.key)

async def get_all_secrets(self, with_values: bool = False) -> list[Secret]:
payload = await self._kube_client.list_secrets()
async def get_all_secrets(
self,
with_values: bool = False,
org_name: Optional[str] = None,
project_name: Optional[str] = None,
) -> list[Secret]:
label_selectors = []
if org_name:
label_selectors += [f"{SECRET_API_ORG_LABEL}={org_name}"]
if project_name:
label_selectors += [f"{PROJECT_LABEL}={project_name}"]
Comment on lines +97 to +98
Copy link
Contributor

Choose a reason for hiding this comment

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

here we no longer respect USER_LABEL which we set for secrets belonging to projects named after owners.
we should have migrated to PROJECT_LABEL entirely as now these legacy and new labels cause confusion.

Copy link
Contributor Author

@zubenkoivan zubenkoivan Apr 26, 2023

Choose a reason for hiding this comment

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

I see, will remove project label selector and switch to filtering secrets in memory based on owner or project

label_selector = ",".join(label_selectors) if label_selectors else None
payload = await self._kube_client.list_secrets(label_selector)
result = []
for item in payload:
owner, org_name = self._get_org_owner_from_secret_name(
item["metadata"]["name"],
org_name=item["metadata"].get("labels", {}).get(SECRET_API_ORG_LABEL),
)
if not owner:
labels = item["metadata"].get("labels", {})
owner = None
org_name = labels.get(SECRET_API_ORG_LABEL)
project_name = labels.get(PROJECT_LABEL)
if not project_name:
owner = self._get_owner_from_secret_name(
item["metadata"]["name"], org_name
)
project_name = owner
if not project_name:
continue
if with_values:
result += [
Secret(key=key, value=value, owner=owner, org_name=org_name)
Secret(
key=key,
value=value,
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: we probably could do value=value if with_values else "" end remove if-else

owner=owner,
project_name=project_name,
org_name=org_name,
)
for key, value in item.get("data", {}).items()
]
else:
result += [
Secret(key=key, owner=owner, org_name=org_name)
Secret(
key=key,
owner=owner,
project_name=project_name,
org_name=org_name,
)
for key in item.get("data", {}).keys()
]
return result
8 changes: 7 additions & 1 deletion platform_secrets/validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,15 @@ def check_base64(value: str) -> str:
"value": secret_value_validator,
t.Key("org_name", optional=True): t.String(min_length=1, max_length=253)
| t.Null(),
t.Key("project_name", optional=True): t.String(min_length=1, max_length=253),
}
)
secret_response_validator = t.Dict(
{"key": t.String, "owner": t.String, "org_name": t.String() | t.Null()}
{
"key": t.String,
"owner": t.String,
"org_name": t.String() | t.Null(),
"project_name": t.String,
}
)
secret_list_response_validator = t.List(secret_response_validator)
4 changes: 4 additions & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ dev =
max-line-length = 88
ignore = N801,N802,N803,E252,W503,E133,E203,F541

[isort]
profile = black
combine_as_imports = True

[tool:pytest]
testpaths = tests
asyncio_mode = auto
Expand Down
Loading