-
Notifications
You must be signed in to change notification settings - Fork 157
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
feat: Implement Image Rescanning using Harbor Webhook API #3116
Open
jopemachine
wants to merge
105
commits into
topic/11-13-feat_implement_management_api_for_controlling_harbor_per-project_quota
Choose a base branch
from
topic/11-19-feat_implement_image_rescan_based_on_harbor_webhook
base: topic/11-13-feat_implement_management_api_for_controlling_harbor_per-project_quota
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+133
−2
Open
Changes from all commits
Commits
Show all changes
105 commits
Select commit
Hold shift + click to select a range
68f1c7d
feat: Implement `AssociationContainerRegistriesGroupsRow`
jopemachine 42d8700
fix: import statement
jopemachine c13d35a
fix: Rename column `container_registry_id` -> `registry_id`
jopemachine 7b6e0a7
fix: Remove useless constructor
jopemachine 355c743
chore: Add news fragment
jopemachine 6798a40
fix: Remove foreign keys and Add `relationship`
jopemachine 0327378
fix: Add association table relationship
jopemachine 1672425
fix: Add unique key to combination of registry_id and group_id
jopemachine 3ff07f0
fix: alembic multiple heads
jopemachine 54633ef
feat: Implement `AssociateContainerRegistryWithGroup`, `DisassociateC…
jopemachine a1eeb78
fix: import statement
jopemachine 2cb3e10
fix: import statements
jopemachine 57d90d5
chore: Add news fragment
jopemachine 5ca9520
feat: Add APIs in client sdk
jopemachine f8c3efc
fix: Remove useless constructor
jopemachine ee8d5b6
chore: Update comment
jopemachine 6b1ed6a
chore: Update comment
jopemachine b393601
feat: Implement REST API for container_registry
jopemachine e31157f
fix: Add extra_fixtures
jopemachine f7f0dbb
feat: Add `test_associate_container_registry_with_group`
jopemachine 9afb595
refactor: Remove common fixture
jopemachine 73f7fe6
feat: Add `test_disassociate_container_registry_with_group`
jopemachine 1fc2501
chore: Reorder argument
jopemachine 1c6e907
feat: Add REST API tests and improve exception handling
jopemachine 086108f
feat: Implement image RBAC project scope
jopemachine 115cb22
fix: Wrong impl of `_in_project_scope`
jopemachine 7ce3b03
feat: Implement `build_ctx_in_project_scope` using `AssociationContai…
jopemachine 3f3f264
fix: Wrong check of boolean column
jopemachine 7573ca2
fix: Rename column `container_registry_id` -> `registry_id`
jopemachine 71d40b9
fix: Error message
jopemachine 44bb1eb
chore: Add news fragment
jopemachine 608ecfc
chore: Fix import statements
jopemachine c624f76
fix: Improve `_in_project_scope` using relationship
jopemachine e4ab6be
feat: Implement per-project images API based on RBAC
jopemachine 93c4705
fix: Add missing `scope_id` to `image_node` GQL API
jopemachine 09bcaf6
fix: Merge
jopemachine 4090dc3
fix: Broken CI
jopemachine 6208077
feat: Implement management API for controlling Harbor per-project Quota
jopemachine f022cb8
chore: Add news fragment
jopemachine e68ffbb
fix: Disable user quota mutation
jopemachine 744b156
fix: Rename variables
jopemachine 68e1cc8
feat: Add `registry_quota` to GroupNode
jopemachine d6dc89e
fix: Update `UpdateQuota` mutation
jopemachine 6436452
fix: Update `resolve_registry_quota`
jopemachine f9832e1
fix: Update `resolve_registry_quota`
jopemachine 5df1dbd
fix: Only authorized groups view the quota
jopemachine 73a87d4
feat: Add CreateQuota, DeleteQuota mutation
jopemachine ae071f4
chore: Rename function
jopemachine c748102
fix: Add exception handling for each operation
jopemachine 0028071
chore: Update schema
jopemachine f056d5b
chore: Update error msg
jopemachine edb300b
chore: Rename variable
jopemachine a1e04f7
fix: Remove useless strenum
jopemachine 7f408c3
refactor: `mutate_harbor_project_quota`
jopemachine 4c2fe1e
refactor: Add read operation handling for code reuse
jopemachine 45e0768
feat: Add SDK for registry quota mutations
jopemachine 67cfe83
fix: Broken CI
jopemachine 87361b1
feat: Implement REST API
jopemachine b220b43
fix: Wrong exception handling
jopemachine 9bcccc1
chore: Update comment
jopemachine 87f1bfd
chore: Rename types
jopemachine 2b376bf
chore: update GraphQL schema dump
jopemachine 1934e38
chore: Rename news fragment
jopemachine ab47a01
fix: Use `BigInt`
jopemachine 2e47d0f
chore: update GraphQL schema dump
jopemachine 4268c5a
refactor: Add `HarborQuotaManager` *(Reflect feedback)
jopemachine bfbbf9f
fix: Use BigInt
jopemachine 2069b9b
chore: self -> cls
jopemachine cea1f50
chore: update GraphQL schema dump
jopemachine f14abef
fix: Improve exception handling
jopemachine 19fcf40
feat: Add `test_harbor_read_project_quota`
jopemachine 173d4e3
chore: `mock-group` -> `mock_group`
jopemachine 07277d0
fix: Disjoint `FIXTURES_FOR_HARBOR_CRUD_TEST` from `test_harbor_read_…
jopemachine 9482647
chore: Add registry type
jopemachine c429139
feat: Add create GQL mutation test case
jopemachine 80e5514
feat: Add update, delete GQL mutation test cases
jopemachine bc60ad7
chore: fix typo
jopemachine 0bf3ff5
feat: Add REST API `test_harbor_read_project_quota` test
jopemachine 9759d1a
chore: Rename variables
jopemachine f7a516b
fix: Add `test_harbor_update_project_quota` test
jopemachine 6b69815
chore: Hoist the variable
jopemachine 50ef00e
feat: Add `test_harbor_delete_project_quota` test
jopemachine b561d7f
feat: Add `test_harbor_create_project_quota` test
jopemachine 8779af5
fix: Change the `test_harbor_read_project_quota` test location
jopemachine 841167c
fix: Change test code location
jopemachine 858fccf
fix: Reuse `FIXTURES_FOR_HARBOR_CRUD_TEST`
jopemachine 1e909ea
fix: Broken CI
jopemachine c601c27
refactor: Add `test_case` parametrize annotation
jopemachine 2e83bad
chore: Remove unused variable
jopemachine 628d41a
refactor: `test_group` test cases using `test_case`
jopemachine 7049305
feat: Implement image rescan based on Harbor webhook
jopemachine e2e5294
fix: Bypass middlewares in webhook handler
jopemachine 27f4149
feat: Add `PUSH_ARTIFACT` event handler
jopemachine 43cac42
chore: Add news fragment
jopemachine 555fc94
fix: Update `harbor_webhook_handler`
jopemachine c11a0e5
fix: Improve exception handling logic
jopemachine 35926ce
fix: Add valid type checker
jopemachine 4edef6a
fix: Add `AUTH_MIDDLEWARE_ALLOW_LIST`
jopemachine a1de07c
chore: Update news fragment
jopemachine 6466542
chore: Remove useless comment
jopemachine 2d009a9
fix: Inject `auth_middleware_allowlist` to app in `build_root_app()`
jopemachine 49b5ccf
fix: Change log level to debug
jopemachine e2ebacc
refactor: `harbor_webhook_handler` *(Reflect feedback)
jopemachine f4a60c4
fix: Improve exception handling
jopemachine cb2fd90
Merge branch 'topic/11-13-feat_implement_management_api_for_controlli…
jopemachine File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Implement Image Rescanning using Harbor Webhook API. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,34 @@ | ||
from __future__ import annotations | ||
|
||
import logging | ||
from typing import TYPE_CHECKING, Any, Iterable, Tuple | ||
from typing import TYPE_CHECKING, Any, Iterable, Optional, Tuple | ||
|
||
import aiohttp_cors | ||
import sqlalchemy as sa | ||
import trafaret as t | ||
from aiohttp import web | ||
from sqlalchemy.exc import IntegrityError | ||
from sqlalchemy.ext.asyncio import AsyncSession | ||
|
||
from ai.backend.common import validators as tx | ||
from ai.backend.logging import BraceStyleAdapter | ||
from ai.backend.manager.api.exceptions import ( | ||
ContainerRegistryWebhookAuthorizationFailed, | ||
InternalServerError, | ||
) | ||
from ai.backend.manager.container_registry.harbor import HarborRegistry_v2 | ||
from ai.backend.manager.models.association_container_registries_groups import ( | ||
AssociationContainerRegistriesGroupsRow, | ||
) | ||
from ai.backend.manager.models.container_registry import ContainerRegistryRow, ContainerRegistryType | ||
|
||
from .exceptions import ContainerRegistryNotFound, GenericBadRequest | ||
|
||
if TYPE_CHECKING: | ||
from .context import RootContext | ||
|
||
from .auth import superadmin_required | ||
from .manager import READ_ALLOWED, server_status_required | ||
from .manager import ALL_ALLOWED, READ_ALLOWED, server_status_required | ||
from .types import CORSOptions, WebMiddleware | ||
from .utils import check_api_params | ||
|
||
|
@@ -84,13 +91,112 @@ async def disassociate_with_group(request: web.Request, params: Any) -> web.Resp | |
return web.json_response({}) | ||
|
||
|
||
async def _get_registry_row_matching_url( | ||
db_sess: AsyncSession, registry_url: str, project: str | ||
) -> ContainerRegistryRow: | ||
query = sa.select(ContainerRegistryRow).where( | ||
(ContainerRegistryRow.type == ContainerRegistryType.HARBOR2) | ||
& (ContainerRegistryRow.url.like(f"%{registry_url}%")) | ||
& (ContainerRegistryRow.project == project) | ||
) | ||
result = await db_sess.execute(query) | ||
return result.scalars().one_or_none() | ||
|
||
|
||
def _is_authorized_harbor_webhook_request( | ||
auth_header: Optional[str], registry_row: ContainerRegistryRow | ||
) -> bool: | ||
if auth_header: | ||
extra = registry_row.extra or {} | ||
return extra.get("webhook_auth_header") == auth_header | ||
return True | ||
|
||
|
||
async def _handle_harbor_webhook_event( | ||
root_ctx: RootContext, | ||
event_type: str, | ||
registry_row: ContainerRegistryRow, | ||
project: str, | ||
img_name: str, | ||
tag: str, | ||
) -> None: | ||
match event_type: | ||
# Perform image rescan only for events that require it. | ||
case "PUSH_ARTIFACT": | ||
await _handle_push_artifact_event(root_ctx, registry_row, project, img_name, tag) | ||
case _: | ||
log.debug( | ||
'Ignore harbor webhook event: "{}". Recommended to modify the webhook config to not subscribe to this event type.', | ||
event_type, | ||
) | ||
|
||
|
||
async def _handle_push_artifact_event( | ||
root_ctx: RootContext, registry_row: ContainerRegistryRow, project: str, img_name: str, tag: str | ||
) -> None: | ||
scanner = HarborRegistry_v2(root_ctx.db, registry_row.registry_name, registry_row) | ||
await scanner.scan_single_ref(f"{project}/{img_name}:{tag}") | ||
|
||
|
||
@server_status_required(ALL_ALLOWED) | ||
@check_api_params( | ||
t.Dict({ | ||
"type": t.String, | ||
"event_data": t.Dict({ | ||
"resources": t.List( | ||
t.Dict({ | ||
"resource_url": t.String, | ||
"tag": t.String, | ||
}).allow_extra("*") | ||
), | ||
"repository": t.Dict({ | ||
"namespace": t.String, | ||
"name": t.String, | ||
}).allow_extra("*"), | ||
}).allow_extra("*"), | ||
}).allow_extra("*") | ||
) | ||
async def harbor_webhook_handler(request: web.Request, params: Any) -> web.Response: | ||
jopemachine marked this conversation as resolved.
Show resolved
Hide resolved
|
||
auth_header = request.headers.get("Authorization", None) | ||
event_type = params["type"] | ||
resources = params["event_data"]["resources"] | ||
project = params["event_data"]["repository"]["namespace"] | ||
img_name = params["event_data"]["repository"]["name"] | ||
log.info("HARBOR_WEBHOOK_HANDLER (event_type:{})", event_type) | ||
|
||
root_ctx: RootContext = request.app["_root.context"] | ||
async with root_ctx.db.begin_session() as db_sess: | ||
for resource in resources: | ||
resource_url = resource["resource_url"] | ||
registry_url = resource_url.split("/")[0] | ||
|
||
registry_row = await _get_registry_row_matching_url(db_sess, registry_url, project) | ||
if not registry_row: | ||
raise InternalServerError( | ||
extra_msg=f"Harbor webhook triggered, but the matching container registry row not found! (registry_url: {registry_url}, project: {project})", | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: Add new exception (represent There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't it get handled? |
||
|
||
if not _is_authorized_harbor_webhook_request(auth_header, registry_row): | ||
raise ContainerRegistryWebhookAuthorizationFailed( | ||
extra_msg=f"Unauthorized webhook request (registry: {registry_row.registry_name}, project: {project})", | ||
) | ||
|
||
await _handle_harbor_webhook_event( | ||
root_ctx, event_type, registry_row, project, img_name, resource["tag"] | ||
) | ||
|
||
return web.json_response({}) | ||
|
||
|
||
def create_app( | ||
default_cors_options: CORSOptions, | ||
) -> Tuple[web.Application, Iterable[WebMiddleware]]: | ||
app = web.Application() | ||
app["api_versions"] = (1, 2, 3, 4, 5) | ||
app["prefix"] = "container-registries" | ||
cors = aiohttp_cors.setup(app, defaults=default_cors_options) | ||
|
||
cors.add(app.router.add_route("POST", "/webhook/harbor", harbor_webhook_handler)) | ||
cors.add(app.router.add_route("POST", "/associate-with-group", associate_with_group)) | ||
cors.add(app.router.add_route("POST", "/disassociate-with-group", disassociate_with_group)) | ||
return app, [] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a lot of other code accessing the
RootContext
field stored inrequest.app[“_root.context”]
, is there a reason why you stored it directly in the app here?I'd like to know what separates the cases that access app directly from those that access RootContext. @achimnol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RootContext
contains mutable configurations such aslocal_config
andshared_config
, as well as many objects that are frequently accessed throughout the codebase.So I thought it might be better to inject hardcoded values into the
app
instead.(But this is just my personal opinion.)