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

Dev #214

Merged
merged 6 commits into from
Jan 8, 2024
Merged

Dev #214

Show file tree
Hide file tree
Changes from all 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
11 changes: 11 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
# v0.8.4

### Features
- [x] Add basic config in helm chart and CLI for Istio.
- [x] Add worker route to get pieces repository.


### Fixes
- [x] Add classic authorization to default route for get pieces repositories.


# v0.8.3

### Features
Expand Down
4 changes: 2 additions & 2 deletions helm/domino/Chart.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ apiVersion: v2
name: domino
description: A Helm chart for Domino
type: application
version: 0.1.9
appVersion: 0.1.9
version: 0.1.10
appVersion: 0.1.10
home: https://github.com/Tauffer-Consulting/domino
sources:
- https://github.com/Tauffer-Consulting/domino
3 changes: 3 additions & 0 deletions helm/domino/templates/jobs/domino-migrations.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ spec:
parallelism: 1
backoffLimit: 4
template:
metadata:
annotations:
sidecar.istio.io/inject: "false"
spec:
restartPolicy: OnFailure
containers:
Expand Down
36 changes: 34 additions & 2 deletions rest/routers/piece_repository_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,7 @@ def get_piece_repository_release_data(
status.HTTP_500_INTERNAL_SERVER_ERROR: {'model': SomethingWrongError},
status.HTTP_403_FORBIDDEN: {'model': ForbiddenError},
},
# TODO - I commented this to make it easier to test, but we should solve the auth service
# dependencies=[Depends(auth_service.workspace_access_authorizer)]
dependencies=[Depends(auth_service.workspace_access_authorizer)]
)
def get_pieces_repositories(
workspace_id: int,
Expand All @@ -142,6 +141,39 @@ def get_pieces_repositories(
raise HTTPException(status_code=e.status_code, detail=e.message)


@router.get(
path="/worker",
status_code=status.HTTP_200_OK,
responses={
status.HTTP_200_OK: {'model': GetWorkspaceRepositoriesResponse},
status.HTTP_500_INTERNAL_SERVER_ERROR: {'model': SomethingWrongError},
status.HTTP_403_FORBIDDEN: {'model': ForbiddenError},
},
)
def get_pieces_repositories_worker(
workspace_id: int,
page: Optional[int] = 0,
page_size: Optional[int] = 100,
filters: ListRepositoryFilters = Depends(),
) -> GetWorkspaceRepositoriesResponse:
"""
Get pieces repositories for workspace.
This endpoint is used by the worker to get the repositories to be processed.
Is the same endpoint as the one above, but without the auth service.
The authorization is done by our service mesh Authorization Policy.
"""
try:
response = piece_repository_service.get_pieces_repositories(
workspace_id=workspace_id,
page=page,
page_size=page_size,
filters=filters
)
return response
except (BaseException, ForbiddenException) as e:
raise HTTPException(status_code=e.status_code, detail=e.message)


@router.delete(
path="/{piece_repository_id}",
status_code=status.HTTP_204_NO_CONTENT,
Expand Down
12 changes: 7 additions & 5 deletions rest/routers/secret_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def update_repository_secret(
raise HTTPException(status_code=e.status_code, detail=e.message)

@router.get(
path='/{piece_name}',
path='/{piece_name}/secrets-values', # using sufix /secrets-values only because istio does not support wildcards in paths
status_code=200,
responses={
status.HTTP_200_OK: {'model': List[GetSecretsByPieceResponse]},
Expand All @@ -79,13 +79,15 @@ def update_repository_secret(
},
include_in_schema=False
)
#@auth_service.authorize_repository_workspace_access # TODO authorize only worker
def get_piece_secrets(
piece_repository_id: int,
piece_name: str, # TODO check what is better to use. query or path ?
#auth_context: AuthorizationContextData = Depends(auth_service.auth_wrapper)
piece_name: str,
) -> List[GetSecretsByPieceResponse]:
"""Get secrets for a specific Piece from an piece repository, in a specific workspace"""
"""
Get secrets values for a specific Piece from an piece repository, in a specific workspace
This endpoint is not using authorization service because it is used by airflow to get secrets values
In production this endpoint should be blocked from external access using security strategies like authorization policies.
"""
try:
response = secret_service.get_piece_secrets(
piece_repository_id=piece_repository_id,
Expand Down
4 changes: 2 additions & 2 deletions rest/tests/secret/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def patch_piece_secret(client: ApiTestClient, authorization_token: Dict, piece_r
@pytest.fixture(scope="function")
def get_secrets_by_piece_name(request, client: ApiTestClient, authorization_token: Dict, piece_repository: PieceRepository):
return client.get(
f"/pieces-repositories/{piece_repository.id}/secrets/{request.param['piece_name']}",
f"/pieces-repositories/{piece_repository.id}/secrets/{request.param['piece_name']}/secrets-values",
headers={"Authorization": authorization_token["header"]}
)

Expand Down Expand Up @@ -72,5 +72,5 @@ def get_repository_secrets_mock_response():
# )
]
return mock_response


9 changes: 4 additions & 5 deletions rest/tests/secret/test_secret_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"tests.secret.fixtures"
]
@pytest.mark.usefixtures("register", "login", "add_piece_repository", "teardown_piece_repository")
class TestSecretRouter:
class TestSecretRouter:
@staticmethod
def test_get_repository_secrets(get_repository_secrets: Response, get_repository_secrets_mock_response: List):
mock_response = get_repository_secrets_mock_response
Expand All @@ -26,18 +26,18 @@ def test_get_repository_secrets(get_repository_secrets: Response, get_repository
secret_content = json.loads(secret.model_dump_json())
mock_response_secrets_names.append(secret_content.get("name"))
mock_response_secrets_names.sort()

assert response.status_code == 200

mock_response_content = json.loads(mock_response[0].model_dump_json())
assert content[0].keys() == mock_response_content.keys()
assert response_secrets_names == mock_response_secrets_names

@staticmethod
def test_patch_piece_secret(patch_piece_secret: Response):
response = patch_piece_secret
assert response.status_code == 204

@staticmethod
@pytest.mark.parametrize("get_secrets_by_piece_name", [{"piece_name": "SimpleLogPiece"}], indirect=True)
def test_get_secrets_by_piece_name(get_secrets_by_piece_name: Response):
Expand All @@ -55,4 +55,3 @@ def test_get_secrets_by_piece_name(get_secrets_by_piece_name: Response):
assert response.status_code == 200
assert content[0] == mock_response_content


2 changes: 1 addition & 1 deletion src/domino/VERSION
Original file line number Diff line number Diff line change
@@ -1 +1 @@
0.8.3
0.8.4
18 changes: 17 additions & 1 deletion src/domino/cli/utils/platform.py
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,22 @@ def create_platform(install_airflow: bool = True, use_gpu: bool = False) -> None
"sshKeySecret": "airflow-ssh-secret"
},
},
"migrateDatabaseJob": {
"jobAnnotations": {
"sidecar.istio.io/inject": "false"
},
"annotations": {
"sidecar.istio.io/inject": "false"
},
},
"createUserJob": {
"jobAnnotations": {
"sidecar.istio.io/inject": "false"
},
"annotations": {
"sidecar.istio.io/inject": "false"
},
},
**workers,
**scheduler,
}
Expand All @@ -417,7 +433,7 @@ def create_platform(install_airflow: bool = True, use_gpu: bool = False) -> None
"-f", str(fp.name),
"airflow",
"apache-airflow/airflow",
"--version", " 1.9.0",
"--version", " 1.11.0",
]
subprocess.run(commands)

Expand Down
14 changes: 3 additions & 11 deletions src/domino/client/domino_backend_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,30 +31,22 @@ def health_check(self) -> requests.Response:
return response

def get_piece_secrets(self, piece_repository_id: int, piece_name: str) -> requests.Response:
resource = f"/pieces-repositories/{piece_repository_id}/secrets/{piece_name}"
response = self.request(
method='get',
resource=resource
)
return response

def get_piece_repository(self, piece_repository_id: int) -> requests.Response:
resource = f"/pieces-repositories/{piece_repository_id}"
resource = f"/pieces-repositories/{piece_repository_id}/secrets/{piece_name}/secrets-values"
response = self.request(
method='get',
resource=resource
)
return response

def get_piece_repositories_from_workspace_id(self, params: dict) -> requests.Response:
resource = "/pieces-repositories"
resource = "/pieces-repositories/worker"
response = self.request(
method='get',
resource=resource,
params=params
)
return response

def check_create_airflow_connection(self, conn_id: str, conn_type: str):
"""
This should check if a specific Airflow connection exists and create it if it doesn't.
Expand Down
4 changes: 3 additions & 1 deletion src/domino/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ def _set_operator(self) -> BaseOperator:
# - https://www.astronomer.io/guides/templating/
# - good example: https://github.com/apache/airflow/blob/main/tests/system/providers/cncf/kubernetes/example_kubernetes.py
# - commands HAVE to go in a list object: https://stackoverflow.com/a/55149915/11483674

return DominoKubernetesPodOperator(
dag_id=self.dag_id,
task_id=self.task_id,
Expand All @@ -115,11 +116,12 @@ def _set_operator(self) -> BaseOperator:
workflow_shared_storage=self.workflow_shared_storage,
container_resources=self.container_resources,
# ----------------- Kubernetes -----------------
namespace='default', # TODO - separate namespace by User or Workspace?
namespace='default',
image=self.piece.get("source_image"),
image_pull_policy='IfNotPresent',
name=f"airflow-worker-pod-{self.task_id}",
startup_timeout_seconds=600,
annotations={"sidecar.istio.io/inject": "false"}, # TODO - remove this when istio is working with airflow k8s pod
# cmds=["/bin/bash"],
# arguments=["-c", "sleep 120;"],
cmds=["domino"],
Expand Down
Loading