Conversation
* [WIP] feat: add custom authentication for multi-tenancy * feat: add custom authentication for multi-tenancy * test: adapt integration tests to work-in-progress authentication setup * style: fix typos * test: restore accidentally removed import * style: sort imports or skip linting * fix: switch from peer relation to juju secret * test: update unit tests * fix: reset unit status to active when bucket exists * fix: reset unit status to active after bucket created * ci: print debugging information * Revert "ci: print debugging information" This reverts commit b12e607. * fix: provision auth database via data platform libs * fix: use separate endpoints for the two relational databases * refactor: set the auth database URI via env var as well * style: fix line breaks * refactor: make relation status messages precise * test: fix typos * test: fix typos * test: fix typos * fix: reuse same database (same URI) for both backend store and auth database * fix: reuse same database (same URI) for both backend store and auth database * fix: reuse same database (same URI) for both backend store and auth database * fix: use correct parameter name in jinja template * fix: change logging level * ci: allow for debugging * fix: have proxy mode on by default * fix: restore proxy mode off by default * fix: restore the real problematic situation * fix: debug with a single uvicorn worker * fix: exclude jobs unsuitable to tracking server * fix: correct typo * fix: switch from mysql to postgresql for auth tables not to undergo usptream bugs * style: do not break line * test: test minor upgrades instead of major ones * test: propagate header also in ingress-reachability tests * style: break line * test: fix test file name * test: fix typos * test: add integration tests for user identity and tenant * test: fix typo * test: rename minor-upgrade file * test: fix user role fetching * test: add test case for no grants to newly seen identity * style: fix spelling of variable name * fix: switch from PostgreSQL to MySQL in latest/edge release bundle * feat: stop the tracking server when the backend store is removed * test: increase coverage * style: fix comment grammar * style: fix comment grammar (again)
* feat: allow user aliasing * test: cover new charm logic * style: add newlines * test: add integration tests for identity aliasing * test: fix typo * refactor: make status messages shorter and redirect to logs * refactor: restart the workload on config changes * style: remove newlines * style: make code coherent * style: fix spelling
Add Istio authorization policies for MLflow 3 multi-tenancy and restrict workload access to the following Identities: * the Waypoint in the platform namespace * the Ingress gateway(s) * the Unit related on the `metrics-endpoint` relation (only for the metrics port)
a032e5c to
d2f0f95
Compare
* feat: add mlflow_client provider logic * chore: update dependencies * chore: switch from charm library to Python package for istio beacon * Revert "chore: switch from charm library to Python package for istio beacon" This reverts commit 605f32d. * chore: fix charm library manually * chore: bump chisme * style: make code maintainable * style: make code maintainable * style: make code maintainable * style: make code maintainable * style: fix conflicts among linters * style: fix noqa syntax * style: fix noqa syntax * chore: switch from service-mesh charm lib to dpcharmlibs python package * chore: fix dependencies * test: adjust provider according to requirer * test: cover granting via data-integrator * style: break lines properly * style: break lines properly * test: update missing test file too * test: fix typo * test: fix typo * test: fix typos * refactor: make grant-reconcile logic clearer * test: exclude URL schema for MinIO * refactor: make grant-reconcile script clearer * style: fix comma * test: fix import * test: fix import * style: fix line break * test: fix typos * docs: improve mlflow-client reconciliation docstring * docs: improve mlflow-client reconciliation docstring * test: fix typos * refactor: make grant-reconcile script clearer * refactor: make grant-reconcile script clearer * refactor: make grant-reconcile script clearer * docs: fix docstring typo * refactor: make grant-reconcile script clearer * text: deploy data-integrator from the requirer's pull request * chore: sort our dependencies after rebasing with conflicts * chore: sort our imports after rebasing with conflicts * chore: sort our imports after rebasing with conflicts * style: fix line break * test: refactor test names * test: fix awaiting * test: fix typos * test: assert native grants * style: add newline * test: fix typos * test: reintroduce acidentally removed line * fix: set backend store URI explicitly in workspace store utility * test: fix search endpoint path * test: refactor constant names * test: test relation removal only at the very end * test: add missing query parameter * test: do not add relation that is already in place * test: retry for live grant changes to take effect * test: debug * test: fix role assertion * test: set target workspaces in requests * test: fix role assertion * test: fix typo * docs: make comment less verbose * fix: ensure old permissions for existing roles are removed * test: cover role update in same workspace * test: poll instead of waiting for port-forwarding * test: cast prot to int * test: switch to tenacy for port-forwarding retries * test: add missing import * docs: add paragraph on why script necessary
d2f0f95 to
634d424
Compare
deusebio
left a comment
There was a problem hiding this comment.
The business logic looks fine and neat! I only have a comment on the tests, providing a suggestion of a pattern that we have used in Spark when spawning new pods and executing things on them (which is up to you if you like the pattern and you want to adopt it), but at the very least I would try avoiding a bit the repetition between test_charm_ambient and test_charm_s3 and factor out common methods. It is not super-critical, but I feel it would reduce the number of code lines to be reviews and changes in various PRs
Apart from that , all good!
| return "".join(choices(ascii_lowercase, k=length)) | ||
|
|
||
| @staticmethod | ||
| def _run_script_from_pod(namespace: str, script: str) -> str: |
There was a problem hiding this comment.
suggestion I'm thinking that maybe one could create a fixture to span and teardown the pod, and the use the pod to make curl requests as one pleases during test.
here is an example and how it is used here
There was a problem hiding this comment.
good idea, updated in ff3a36c, I extracted this one to helpers.py and we can continue by moving things there when needed
| experiment_name = f"{TEST_EXPERIMENT_NAME}-{self.generate_random_string(6)}" | ||
| logs_result = None | ||
| tracking_uri = f"http://{CHARM_NAME}.{ops_test.model_name}.svc.cluster.local:{mlflow_port}" | ||
| curl_script = ( |
There was a problem hiding this comment.
suggestion I would really try to break this one up a bit, and get a "exec" => "output" with single API calls. It is not a biggie but this may connect with the previous point to create a Pod, allow to exec commands and retrieve their outputs more programatically
| return "".join(choices(ascii_lowercase, k=length)) | ||
|
|
||
| @staticmethod | ||
| def _run_script_from_pod(namespace: str, script: str) -> str: |
There was a problem hiding this comment.
todo this seems a bit of repetition, it is probably sane to abstract this in either helpers.py
There was a problem hiding this comment.
as discussed in the call, we'll be deprecating other integration test files in an upcoming PR
| ) | ||
|
|
||
| @pytest.mark.abort_on_fail | ||
| async def test_configure_profile_identity_alias( |
There was a problem hiding this comment.
todo also this seems a bit of repetition. Could this be abstracted away in a single place?
There was a problem hiding this comment.
as discussed in the call, we'll be deprecating other integration test files in an upcoming PR
| tracking_uri = f"http://{CHARM_NAME}.{ops_test.model_name}.svc.cluster.local:{mlflow_port}" | ||
| workspace_header = f"{UPSTREAM_WORKSPACE_HEADER_NAME}: {WORKSPACE_WITH_ADMIN_ACCESS_FINAL}" | ||
|
|
||
| curl_pod.curl( |
There was a problem hiding this comment.
praise I really really like this a lot more! Well done!
Closes #457
Add an Istio
TrafficExtensionto derive MLflow user identity, this is for the use case when reaching MLflow from inside the user/profile namespace.