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

refactor(organization): switch to one-bucket-per-org policy #336

Merged
merged 88 commits into from
Aug 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
88 commits
Select commit Hold shift + click to select a range
d2eb60e
feat: first commit for Site table creation
Jun 17, 2024
d69085c
fix: error when launching backend
Jun 17, 2024
f4ec529
fix: sites tests
Jun 18, 2024
0f4026e
fix mypy
Jun 18, 2024
ceb525e
fix tests
Jun 18, 2024
b698b2c
fix: add site_id in user table
Jun 18, 2024
a4caaaa
feat: implement new security behavior using site_id
Jun 18, 2024
79a9867
feat : add CRUD endpoints for the site path + tests
Jun 19, 2024
40e6889
refactor: Site -> Organization
Jun 19, 2024
1495e21
feat: refactor security behavior
Jun 19, 2024
87d3ef9
feat: fix tests
Jun 21, 2024
ecf5ad2
fix e2e tests
Jun 21, 2024
0e647d8
fix client tests
Jun 21, 2024
65dec07
fix "role" column
Jun 21, 2024
ea9089f
fix: remove useless security in case of create_detection
Jun 24, 2024
76d46b7
fix: resolve first comments
Jul 3, 2024
926b0c3
fix error in detection endpoint
Jul 3, 2024
147804f
take feedback into account
Jul 11, 2024
185b2de
feat: add crud function to avoid for loop
Jul 11, 2024
1877874
feedback PR
Jul 11, 2024
4dda307
fix lint
Jul 12, 2024
12e4258
Merge branch 'main' into rs/add-site-table
Jul 13, 2024
10f9f7e
fix linting
Jul 13, 2024
0f15ca2
feat: add acknowledged endpoint
Jul 4, 2024
6c6fd43
feat: no need to have warning level of error
Jul 4, 2024
ecdf13c
feat: use is_wildfire instead of a new boolean
Jul 8, 2024
52b6abb
refactor fetch_all crud
Jul 11, 2024
522fc2b
fix: rm Exception
Jul 12, 2024
77473ec
fix mypy
Jul 12, 2024
357ae53
feat: start implemnting new payload from_date
Jul 13, 2024
9cb48cc
fir error date -> datetime
Jul 13, 2024
d1f3ac1
fix unlabeled endpoints
Jul 14, 2024
fef57e0
feat: add localization in Detection table
Jul 9, 2024
cd2d224
fix: forgot update client function for bbox
Jul 9, 2024
16cde9c
fix: many errors in localization usage
Jul 9, 2024
f0caa1f
feat: rm Exception as e
Jul 12, 2024
67944fd
refactor: rm Optional
Jul 12, 2024
e82a80a
fix: style
Jul 13, 2024
cd43ed7
feat: add regexp
Jul 13, 2024
9137247
localization can be equal to []
Jul 13, 2024
57e6aca
fix error after rebase
Jul 14, 2024
58d922f
fix: many errors in localization usage
Jul 9, 2024
22b83a6
refactor: rm Optional
Jul 12, 2024
34df116
fix: style
Jul 13, 2024
a8859b0
feat: add regexp
Jul 13, 2024
bc5abfc
fix: many errors in localization usage
Jul 9, 2024
3eb3e85
feat: add localization in Detection table
Jul 9, 2024
a7bf55e
fix: many errors in localization usage
Jul 9, 2024
f6d6a41
feat: use a bucket per organiztion
Jun 26, 2024
09bd16a
feat: update diagramme
Jun 28, 2024
47dc24b
fix: some errors
Jul 1, 2024
d0ca4b3
feat: use id instead of name for bucket_name
Jul 3, 2024
09a66ef
fix miss function params
Jul 4, 2024
10f4feb
feat: automatic creation of the S3 bucket
Jul 4, 2024
cf6c234
fix the diagramm
Jul 4, 2024
f53ab50
fix: linting
Jul 9, 2024
1c5d9cf
fix after merge
Jul 11, 2024
ccbcc15
fix: don't create bucket in Dection, better in Organization
Jul 11, 2024
ef99973
fix error when using ADMIN scope
Jul 12, 2024
5e7b366
fix lint
Jul 12, 2024
77dda00
use global var for admin name
Jul 12, 2024
b971477
some fixes after rebase
Jul 13, 2024
8b5a382
fix test storage
Jul 14, 2024
6c641b2
Merge branch 'main' into rs/add-acknowledged
Jul 15, 2024
be6574e
Add localization in Detection table (#342)
RonanMorgan Jul 19, 2024
4fc04d4
Send url with detection (#346)
RonanMorgan Jul 19, 2024
2613e1a
Merge branch 'rs/add-acknowledged' into rs/use-organization-bucket
Jul 19, 2024
02c884b
clean up comments
Jul 19, 2024
2d3cde3
feat: refactor fecth_all function
Jul 19, 2024
c2f7b91
clean up comments
Jul 19, 2024
5da3d23
fix typing
Jul 19, 2024
4acd511
Merge branch 'rs/add-acknowledged' into rs/use-organization-bucket
Jul 19, 2024
348c603
fix error in delete bucket
Jul 19, 2024
72d7a43
Merge branch 'main' into rs/use-organization-bucket
frgfm Aug 23, 2024
c2a95ca
revert(organization): revert unnecessary changes
frgfm Aug 23, 2024
aea99d2
fix(detections): fix unlabeled detections
frgfm Aug 23, 2024
dc4a6f0
fix(organizations): clean org creation
frgfm Aug 23, 2024
565b48b
revert(migration): remove alembic op
frgfm Aug 23, 2024
37cbddb
refactor(storage): update S3 management
frgfm Aug 23, 2024
50dbed5
refactor(storage): update s3
frgfm Aug 23, 2024
09568e0
style(detections): remove unused import
frgfm Aug 23, 2024
e36535e
style(mypy): fix typing
frgfm Aug 23, 2024
6adbd6a
fix(storage): fix async issues
frgfm Aug 24, 2024
4cc6e51
test(storage): fix async troubles
frgfm Aug 24, 2024
fedb8bf
fix(detections): fix interaction with s3
frgfm Aug 24, 2024
d15191c
test(conftest): update config
frgfm Aug 24, 2024
76ea0c3
test(conftest): fixing init
frgfm Aug 24, 2024
bd709cc
fix(docker): fix docker orchestration
frgfm Aug 24, 2024
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: 0 additions & 1 deletion .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ S3_ACCESS_KEY='na'
S3_SECRET_KEY='na'
S3_REGION='us-east-1'
S3_ENDPOINT_URL='http://localstack:4566'
S3_BUCKET_NAME=bucket

# Initialization
SUPERADMIN_LOGIN='pyroadmin'
Expand Down
1 change: 0 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,6 @@ None :)
- `S3_REGION`: your S3 bucket is geographically identified by its location's region
- `S3_ENDPOINT_URL`: the URL providing a S3 endpoint by your cloud provider
- `S3_PROXY_URL`: the url of the proxy to hide the real s3 url behind, do not use proxy if ""
- `S3_BUCKET_NAME`: the name of the storage bucket

#### Production-only values
- `ACME_EMAIL`: the email linked to your certificate for HTTPS
Expand Down
2 changes: 1 addition & 1 deletion client/pyroclient/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
#################
# DETECTIONS
#################
"detections-create": "/detections",
"detections-create": "/detections/",
"detections-label": "/detections/{det_id}/label",
"detections-fetch": "/detections",
"detections-fetch-unl": "/detections/unlabeled/fromdate",
Expand Down
5 changes: 2 additions & 3 deletions docker-compose.dev.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ services:
volumes:
- ./scripts/localstack:/etc/localstack/init/ready.d
healthcheck:
test: ["CMD-SHELL", "awslocal --endpoint-url=http://localhost:4566 s3 ls s3://bucket"]
test: ["CMD-SHELL", "awslocal --endpoint-url=http://localhost:4566 s3 ls s3://admin"]
interval: 10s
timeout: 5s
retries: 10
Expand All @@ -46,12 +46,11 @@ services:
- POSTGRES_URL=postgresql+asyncpg://dummy_pg_user:dummy_pg_pwd@db/dummy_pg_db
- SUPERADMIN_LOGIN=superadmin_login
- SUPERADMIN_PWD=superadmin_pwd
- SUPERADMIN_ORG=superadmin_org
- SUPERADMIN_ORG=admin
- JWT_SECRET=${JWT_SECRET}
- SUPPORT_EMAIL=${SUPPORT_EMAIL}
- DEBUG=true
- SQLALCHEMY_SILENCE_UBER_WARNING=1
- S3_BUCKET_NAME=bucket
- S3_ENDPOINT_URL=http://localstack:4566
- S3_ACCESS_KEY=fake
- S3_SECRET_KEY=fake
Expand Down
3 changes: 1 addition & 2 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ services:
- ./scripts/localstack:/etc/localstack/init/ready.d
- localstack_data:/tmp/localstack
healthcheck:
test: ["CMD-SHELL", "awslocal --endpoint-url=http://localhost:4566 s3 ls s3://bucket"]
test: ["CMD-SHELL", "awslocal --endpoint-url=http://localhost:4566 s3 ls s3://admin"]
interval: 10s
timeout: 5s
retries: 10
Expand All @@ -58,7 +58,6 @@ services:
- DEBUG=true
- PROMETHEUS_ENABLED=true
- SQLALCHEMY_SILENCE_UBER_WARNING=1
- S3_BUCKET_NAME=${S3_BUCKET_NAME:-bucket}
- S3_ENDPOINT_URL=${S3_ENDPOINT_URL:-http://localstack:4566}
- S3_ACCESS_KEY=${S3_ACCESS_KEY:-na}
- S3_SECRET_KEY=${S3_SECRET_KEY:-na}
Expand Down
1 change: 0 additions & 1 deletion scripts/dbdiagram.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

Enum "userrole" {
"admin"
"agent"
Expand Down
4 changes: 2 additions & 2 deletions scripts/localstack/setup-s3.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/usr/bin/env bash
awslocal s3 mb s3://bucket
awslocal s3 mb s3://admin
echo -n "" > my_file
awslocal s3 cp my_file s3://bucket/my_file
awslocal s3 cp my_file s3://admin/my_file
38 changes: 24 additions & 14 deletions src/app/api/api_v1/endpoints/detections.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
# This program is licensed under the Apache License 2.0.
# See LICENSE or go to <https://www.apache.org/licenses/LICENSE-2.0> for full license details.

import asyncio
import hashlib
import logging
from datetime import datetime
from mimetypes import guess_extension
from typing import List, cast
Expand All @@ -25,7 +25,7 @@
DetectionWithUrl,
)
from app.schemas.login import TokenPayload
from app.services.storage import s3_bucket
from app.services.storage import s3_service
from app.services.telemetry import telemetry_client

router = APIRouter()
Expand Down Expand Up @@ -67,15 +67,19 @@
bucket_key = f"{token_payload.sub}-{datetime.utcnow().strftime('%Y%m%d%H%M%S')}-{sha_hash[:8]}{extension}"
# Reset byte position of the file (cf. https://fastapi.tiangolo.com/tutorial/request-files/#uploadfile)
await file.seek(0)
# Failed upload
if not (await s3_bucket.upload_file(bucket_key, file.file)): # type: ignore[arg-type]
bucket_name = s3_service.resolve_bucket_name(token_payload.organization_id)
bucket = s3_service.get_bucket(bucket_name)
# Upload the file
if not bucket.upload_file(bucket_key, file.file): # type: ignore[arg-type]
raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Failed upload")
logging.info(f"File uploaded to bucket {bucket_name} with key {bucket_key}.")

# Data integrity check
file_meta = await s3_bucket.get_file_metadata(bucket_key)
file_meta = bucket.get_file_metadata(bucket_key)
# Corrupted file
if md5_hash != file_meta["ETag"].replace('"', ""):
# Delete the corrupted upload
await s3_bucket.delete_file(bucket_key)
bucket.delete_file(bucket_key)

Check warning on line 82 in src/app/api/api_v1/endpoints/detections.py

View check run for this annotation

Codecov / codecov/patch

src/app/api/api_v1/endpoints/detections.py#L82

Added line #L82 was not covered by tests
# Raise the exception
raise HTTPException(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
Expand Down Expand Up @@ -119,13 +123,16 @@
detection = cast(Detection, await detections.get(detection_id, strict=True))

if UserRole.ADMIN in token_payload.scopes:
return DetectionUrl(url=await s3_bucket.get_public_url(detection.bucket_key))
camera = cast(Camera, await cameras.get(detection.camera_id, strict=True))
bucket = s3_service.get_bucket(s3_service.resolve_bucket_name(camera.organization_id))
return DetectionUrl(url=bucket.get_public_url(detection.bucket_key))

camera = cast(Camera, await cameras.get(detection.camera_id, strict=True))
if token_payload.organization_id != camera.organization_id:
raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Access forbidden.")
# Check in bucket
return DetectionUrl(url=await s3_bucket.get_public_url(detection.bucket_key))
bucket = s3_service.get_bucket(s3_service.resolve_bucket_name(camera.organization_id))
return DetectionUrl(url=bucket.get_public_url(detection.bucket_key))


@router.get("/", status_code=status.HTTP_200_OK, summary="Fetch all the detections")
Expand Down Expand Up @@ -153,8 +160,10 @@
) -> List[DetectionWithUrl]:
telemetry_client.capture(token_payload.sub, event="unacknowledged-fetch")

async def get_url(detection: Detection) -> str:
return await s3_bucket.get_public_url(detection.bucket_key)
bucket = s3_service.get_bucket(s3_service.resolve_bucket_name(token_payload.organization_id))

def get_url(detection: Detection) -> str:
return bucket.get_public_url(detection.bucket_key)

if UserRole.ADMIN in token_payload.scopes:
all_unck_detections = await detections.fetch_all(
Expand All @@ -168,9 +177,7 @@
inequality_pair=("created_at", ">=", from_date),
)

# Launch all get_url calls in parallel
url_tasks = [get_url(detection) for detection in all_unck_detections]
urls = await asyncio.gather(*url_tasks)
urls = (get_url(detection) for detection in all_unck_detections)

return [DetectionWithUrl(**detection.model_dump(), url=url) for detection, url in zip(all_unck_detections, urls)]

Expand Down Expand Up @@ -199,9 +206,12 @@
async def delete_detection(
detection_id: int = Path(..., gt=0),
detections: DetectionCRUD = Depends(get_detection_crud),
cameras: CameraCRUD = Depends(get_camera_crud),
token_payload: TokenPayload = Security(get_jwt, scopes=[UserRole.ADMIN]),
) -> None:
telemetry_client.capture(token_payload.sub, event="detections-deletion", properties={"detection_id": detection_id})
detection = cast(Detection, await detections.get(detection_id, strict=True))
await s3_bucket.delete_file(detection.bucket_key)
camera = cast(Camera, await cameras.get(detection.camera_id, strict=True))
bucket = s3_service.get_bucket(s3_service.resolve_bucket_name(camera.organization_id))
bucket.delete_file(detection.bucket_key)
await detections.delete(detection_id)
14 changes: 12 additions & 2 deletions src/app/api/api_v1/endpoints/organizations.py
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we also delete the bucket when the org is deleted? it was the core motivation I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

btw are we sure we want that ? maybe we will need the images for the ML part @MateoLostanlen

Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,14 @@

from typing import List, cast

from fastapi import APIRouter, Depends, Path, Security, status
from fastapi import APIRouter, Depends, HTTPException, Path, Security, status

from app.api.dependencies import get_jwt, get_organization_crud
from app.crud import OrganizationCRUD
from app.models import Organization, UserRole
from app.schemas.login import TokenPayload
from app.schemas.organizations import OrganizationCreate
from app.services.storage import s3_service
from app.services.telemetry import telemetry_client

router = APIRouter()
Expand All @@ -27,7 +28,13 @@
telemetry_client.capture(
token_payload.sub, event="organization-create", properties={"organization_name": payload.name}
)
return await organizations.create(payload)
organization = await organizations.create(payload)
Copy link
Member

Choose a reason for hiding this comment

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

Mmmh I was about to suggest we do this after making sure the S3 ops haven't failed, but we need the org ID for the bucket 🤔

Copy link
Collaborator Author

@RonanMorgan RonanMorgan Jul 19, 2024

Choose a reason for hiding this comment

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

I can raise an Error if create_bucket send false ?

bucket_name = s3_service.resolve_bucket_name(organization.id)
if not s3_service.create_bucket(bucket_name):

Check warning on line 33 in src/app/api/api_v1/endpoints/organizations.py

View check run for this annotation

Codecov / codecov/patch

src/app/api/api_v1/endpoints/organizations.py#L32-L33

Added lines #L32 - L33 were not covered by tests
# Delete the organization if the bucket creation failed
await organizations.delete(organization.id)
raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Failed to create bucket")
return organization

Check warning on line 37 in src/app/api/api_v1/endpoints/organizations.py

View check run for this annotation

Codecov / codecov/patch

src/app/api/api_v1/endpoints/organizations.py#L35-L37

Added lines #L35 - L37 were not covered by tests


@router.get(
Expand Down Expand Up @@ -62,4 +69,7 @@
telemetry_client.capture(
token_payload.sub, event="organizations-deletion", properties={"organization_id": organization_id}
)
bucket_name = s3_service.resolve_bucket_name(organization_id)
if not (await s3_service.delete_bucket(bucket_name)):
raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Failed to create bucket")

Check warning on line 74 in src/app/api/api_v1/endpoints/organizations.py

View check run for this annotation

Codecov / codecov/patch

src/app/api/api_v1/endpoints/organizations.py#L74

Added line #L74 was not covered by tests
await organizations.delete(organization_id)
1 change: 0 additions & 1 deletion src/app/core/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ def sqlachmey_uri(cls, v: str) -> str:
)

# Storage
S3_BUCKET_NAME: str = os.environ["S3_BUCKET_NAME"]
S3_ACCESS_KEY: str = os.environ["S3_ACCESS_KEY"]
S3_SECRET_KEY: str = os.environ["S3_SECRET_KEY"]
S3_REGION: str = os.environ["S3_REGION"]
Expand Down
4 changes: 4 additions & 0 deletions src/app/db.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from app.core.config import settings
from app.core.security import hash_password
from app.models import Organization, User, UserRole
from app.services.storage import s3_service

__all__ = ["get_session", "init_db"]

Expand All @@ -34,6 +35,7 @@
async with AsyncSession(engine) as session:
logger.info("Initializing PostgreSQL database...")

# Create the superadmin organization
statement = select(Organization).where(Organization.name == settings.SUPERADMIN_ORG) # type: ignore[var-annotated]
results = await session.execute(statement=statement)
organization = results.scalar_one_or_none()
Expand All @@ -45,6 +47,8 @@
organization_id = new_orga.id
else:
organization_id = organization.id
# Create the bucket
s3_service.create_bucket(s3_service.resolve_bucket_name(organization_id))

Check warning on line 51 in src/app/db.py

View check run for this annotation

Codecov / codecov/patch

src/app/db.py#L51

Added line #L51 was not covered by tests

# Check if admin exists
statement = select(User).where(User.login == settings.SUPERADMIN_LOGIN)
Expand Down
Loading