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

feature: add git service token management #1136

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
daf7d1d
Initializing git_tokens table
studioego Mar 7, 2023
8620616
make git token manage API
studioego Mar 13, 2023
06bfccd
fix select uuid on SQLAlchemy
studioego Mar 15, 2023
8437067
Merge remote-tracking branch 'origin' into feature/add-git-token-mana…
studioego Mar 15, 2023
9aa48cd
Refactor get token list
studioego Mar 16, 2023
112e0a5
modify to insert git token data
studioego Mar 20, 2023
7cc63f6
add git token management queries
studioego Mar 22, 2023
b756272
Merge remote-tracking branch 'origin/main' into feature/add-git-token…
studioego Apr 4, 2023
49e6237
Merge remote-tracking branch 'origin/main' into feature/add-git-token…
studioego Apr 10, 2023
a5cde53
Merge remote-tracking branch 'origin' into feature/add-git-token-mana…
studioego Jun 27, 2023
0bc7571
Merge remote-tracking branch 'origin' into feature/add-git-token-mana…
studioego Jun 29, 2023
46634af
when update git_token, fill modified_at date
studioego Aug 29, 2023
8b470a0
Add news fragment
studioego Aug 30, 2023
a44a495
Merge remote-tracking branch 'origin' into feature/add-git-token-mana…
studioego Aug 30, 2023
f40a623
fix alembic's multiple heads
studioego Aug 30, 2023
a80ede2
make a new alembic migration file
studioego Aug 30, 2023
51ac4c4
Merge remote-tracking branch 'origin' into feature/add-git-token-mana…
studioego Aug 30, 2023
b6018ac
update alembic heads
studioego Aug 30, 2023
fb98861
fix the broken alembic heads
studioego Aug 30, 2023
d7e52c2
Merge branch 'main' into feature/add-git-token-management
achimnol Sep 22, 2023
f3e4c84
Merge branch 'main' into feature/add-git-token-management
Yaminyam Oct 21, 2023
0420a63
Merge branch 'main' into feature/add-git-token-management
Yaminyam Feb 22, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changes/1136.feature.md
@@ -0,0 +1 @@
Add git services(GitHub & Gitlab) token management
47 changes: 47 additions & 0 deletions src/ai/backend/manager/api/userconfig.py
@@ -1,8 +1,10 @@
import json
import logging
import re
from typing import TYPE_CHECKING, Any, Tuple

import aiohttp_cors
import sqlalchemy as sa
import trafaret as t
from aiohttp import web

Expand All @@ -11,6 +13,8 @@

from ..models import (
MAXIMUM_DOTFILE_SIZE,
git_token,
git_tokens,
Comment on lines +16 to +17
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
git_token,
git_tokens,
insert_update_git_tokens,
git_tokens,

We'd like to import classes or functions, not modules.

keypairs,
query_accessible_vfolders,
query_bootstrap_script,
Expand Down Expand Up @@ -235,6 +239,47 @@ async def get_bootstrap_script(request: web.Request) -> web.Response:
return web.json_response(script)


@auth_required
@server_status_required(READ_ALLOWED)
async def get_git_tokens(request: web.Request) -> web.Response:
root_ctx: RootContext = request.app["_root.context"]
user_uuid = request["user"]["uuid"]

async with root_ctx.db.begin_readonly() as conn:
query = sa.select([git_tokens.c.domain, git_tokens.c.token]).where(
git_tokens.c.user_id == user_uuid
)
Comment on lines +246 to +251
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any use case of getting other user's git tokens?
For example, admins would like to get other user's tokens.

query_result = await conn.execute(query)
rows = query_result.fetchall()
result = [{"domain": row["domain"], "token": row["token"]} for row in rows]
return web.json_response(result, status=200)


@auth_required
@check_api_params(
t.Dict(
{
t.Key("params"): t.String,
},
)
)
async def update_git_tokens(request: web.Request, params: Any) -> web.Response:
resp = []
root_ctx: RootContext = request.app["_root.context"]
user_uuid = request["user"]["uuid"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.
Please let me know if there is any use case of getting or updating other user's git token,


# get token list then
token_list = params["params"]
Comment on lines +262 to +272
Copy link
Contributor

@fregataa fregataa Aug 30, 2023

Choose a reason for hiding this comment

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

Suggested change
t.Key("params"): t.String,
},
)
)
async def update_git_tokens(request: web.Request, params: Any) -> web.Response:
resp = []
root_ctx: RootContext = request.app["_root.context"]
user_uuid = request["user"]["uuid"]
# get token list then
token_list = params["params"]
t.Key("tokens"): t.Mapping(t.String, t.String),
},
)
)
async def update_git_tokens(request: web.Request, params: Any) -> web.Response:
resp = []
root_ctx: RootContext = request.app["_root.context"]
user_uuid = request["user"]["uuid"]
# get token dictionary then
tokens: dict[str, str] = params["tokens"]

First, the name of parameter is too vague. Let's change it.
If we successfully change the parameter type, then let's update git_token.insert_update_git_tokens()!

async with root_ctx.db.begin() as conn:
await git_token.insert_update_git_tokens(conn, user_uuid, token_list)

token_list_json = json.loads(token_list)
for key, value in token_list_json.items():
resp.append({"domain": key, "token": value})

return web.json_response(resp)


async def init(app: web.Application) -> None:
pass

Expand All @@ -258,5 +303,7 @@ def create_app(
cors.add(app.router.add_route("DELETE", "/dotfiles", delete))
cors.add(app.router.add_route("POST", "/bootstrap-script", update_bootstrap_script))
cors.add(app.router.add_route("GET", "/bootstrap-script", get_bootstrap_script))
cors.add(app.router.add_route("GET", "/git-tokens", get_git_tokens))
cors.add(app.router.add_route("PATCH", "/git-tokens", update_git_tokens))

return app, []
3 changes: 3 additions & 0 deletions src/ai/backend/manager/models/__init__.py
Expand Up @@ -4,6 +4,7 @@
from . import dotfile as _dotfile
from . import endpoint as _endpoint
from . import error_logs as _errorlogs
from . import git_token as _gtoken
from . import group as _group
from . import image as _image
from . import kernel as _kernel
Expand Down Expand Up @@ -42,6 +43,7 @@
*_sessiontemplate.__all__,
*_storage.__all__,
*_errorlogs.__all__,
*_gtoken.__all__,
)

from .acl import * # noqa
Expand All @@ -50,6 +52,7 @@
from .dotfile import * # noqa
from .endpoint import * # noqa
from .error_logs import * # noqa
from .git_token import * # noqa
from .group import * # noqa
from .image import * # noqa
from .kernel import * # noqa
Expand Down
@@ -0,0 +1,46 @@
"""add git_token table

Revision ID: 3d31a989dd0a
Revises: 02535458c0b3
Create Date: 2023-08-31 01:04:48.109184

"""
import sqlalchemy as sa
from alembic import op

from ai.backend.manager.models.base import GUID

# revision identifiers, used by Alembic.
revision = "3d31a989dd0a"
down_revision = "02535458c0b3"
branch_labels = None
depends_on = None


def upgrade():
op.create_table(
"git_tokens",
sa.Column("user_id", GUID(), nullable=False, index=True),
sa.Column("domain", sa.String(length=200), nullable=False, index=True),
sa.Column("token", sa.String(length=200)),
sa.Column("created_at", sa.DateTime(timezone=True), server_default=sa.func.now()),
sa.Column(
"modified_at",
sa.DateTime(timezone=True),
server_default=sa.text("now()"),
nullable=True,
),
sa.ForeignKeyConstraint(
["user_id"],
["users.uuid"],
name=op.f("fk_git_tokens_user_id_users"),
onupdate="CASCADE",
ondelete="CASCADE",
),
sa.UniqueConstraint("user_id", "domain", name="uq_git_tokens_user_id_domain"),
)
# Create a default github and gitlab tokens


def downgrade():
op.drop_table("git_tokens")
93 changes: 93 additions & 0 deletions src/ai/backend/manager/models/git_token.py
@@ -0,0 +1,93 @@
from __future__ import annotations

import datetime
import json
import logging
import uuid
from typing import Sequence

import graphene
import sqlalchemy as sa
from graphene.types.datetime import DateTime as GQLDateTime
from sqlalchemy.dialects.postgresql import insert
from sqlalchemy.ext.asyncio import AsyncConnection as SAConnection
from sqlalchemy.sql import text

from ai.backend.common.logging import BraceStyleAdapter

from .base import GUID, Base, metadata

log = BraceStyleAdapter(logging.getLogger(__spec__.name)) # type: ignore[name-defined]


__all__: Sequence[str] = (
"git_tokens",
"GitToken",
"GitTokenRow",
"insert_update_git_tokens",
)

git_tokens = sa.Table(
"git_tokens",
metadata,
sa.Column(
"user_id",
GUID,
sa.ForeignKey("users.uuid", onupdate="CASCADE", ondelete="CASCADE"),
index=True,
nullable=False,
primary_key=True,
),
Comment on lines +33 to +40
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have a unique id field other than the user_id field.

Comment on lines +33 to +40
Copy link
Contributor

Choose a reason for hiding this comment

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

Is git token unique for every user?

sa.Column("domain", sa.String(length=200), unique=True, primary_key=True, nullable=False),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is git token unique within each domain?

Copy link
Contributor

Choose a reason for hiding this comment

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

And domains table has name field which is a primary key.
How about defining this field as a ForeignKey?

sa.Column("token", sa.String(length=200)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a question!
How long git token usually is??

sa.Column("created_at", sa.DateTime(timezone=True), server_default=sa.func.now()),
sa.Column(
"modified_at",
sa.DateTime(timezone=True),
server_default=sa.func.now(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this field should be nullable and default value should be null if we think of a token row without any modification

onupdate=sa.func.current_timestamp(),
),
)
Comment on lines +30 to +50
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we decided to use ORM table, please define the table in ORM
This is an example code of our Session table.



async def insert_update_git_tokens(
conn: SAConnection, user_uuid: uuid.UUID, token_list_str: str
) -> None:
token_list = json.loads(token_list_str)
domain_list = []
Comment on lines +56 to +57
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable is actually of type dict, but it has list in its name, which confuses me into thinking it is of type list.
let's rename them and add type hint!

Suggested change
token_list = json.loads(token_list_str)
domain_list = []
tokens: dict[str, Any] = json.loads(token_list_str)
domain_list: list[str] = []

# check domain_list
for key, value in token_list.items():
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for key, value in token_list.items():
for domain_name, token in token_list.items():

Let's use more clear name!

domain_list.append(key)
# delete some non-exist key on domain list
delete_stmt = sa.delete(git_tokens).where(
sa.and_(git_tokens.c.user_id == user_uuid), sa.not_(git_tokens.c.domain.in_(domain_list))
)
await conn.execute(delete_stmt)

for key, value in token_list.items():
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for key, value in token_list.items():
for domain_name, token in token_list.items():

Same here

data = {
"user_id": user_uuid,
"domain": key,
"token": value,
"modified_at": datetime.datetime.now(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we do not need to fill this part because of the field's onupdate function.

}
query = insert(git_tokens).values(data)
query = query.on_conflict_do_update(
index_elements=["user_id", "domain"],
set_=dict(token=query.excluded.token, modified_at=text("NOW()")),
)
await conn.execute(query)

await conn.commit()


class GitToken(graphene.ObjectType):
user_id = graphene.UUID()
domain = graphene.String()
token = graphene.String()
created_at = GQLDateTime()
modified_at = GQLDateTime()


class GitTokenRow(Base):
__table__ = git_tokens
Comment on lines +92 to +93
Copy link
Contributor

Choose a reason for hiding this comment

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

If we define the table in ORM, we can remove this overriding part.