♻️ connecting docker-api-proxy to services which require docker swarm socket - #8773
♻️ connecting docker-api-proxy to services which require docker swarm socket#8773GitHK wants to merge 30 commits into
Conversation
GitHK
changed the title
GitHK
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8773 +/- ##
==========================================
- Coverage 89.30% 89.06% -0.24%
==========================================
Files 1823 1783 -40
Lines 72257 69679 -2578
Branches 911 911
==========================================
- Hits 64526 62058 -2468
+ Misses 7503 7394 -109
+ Partials 228 227 -1
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
@GitHK since this is a critical security issue, I'd like to check this actually. But as you said the PR is very noisy. Can you maybe point me to the key changes? or explain them, w.r.t. the implications for the mounted docker socket, admin rights etc. thanks |
This is a followup of the two following PRs:
From a security point of view it's a "drop-in" replacement of the docker socket, with an HTTP authentication layer so that not all services can access it. The PR simply replaces the docker socket with the docker-api-proxy endpoint (which requires credentials by default). The replacement is done only for osparc services that require swarm level access (typically create and remove services and overlay networks). |
sanderegg
left a comment
sanderegg
left a comment
There was a problem hiding this comment.
Much better thanks for the new changes. Nevertheless I am still not ok with the changes in the autoscaling and give you the following proposal (which we can discuss tomorrow):
- instead of calling this a remote docker client -> setup_docker_client
- then if there is no settings, then initiate the docker client as usual, so that it is not necessary to have that additional moving piece for unit tests
- then it also becomes get_docker_client instead of get_remote_docker_client
- Also change DockerApiProxySettings to DockerClientSettings
- also I do not think yo need mock_setup_remote_docker_client all over the place since these can just be None while testing, which will simplify the testing even more, unless you specifically want to test it (which I would recommend to do in service lib and not in every single service)
| exit_stack = AsyncExitStack() | ||
|
|
||
| async def on_startup() -> None: | ||
| app.state.remote_docker_client = await exit_stack.enter_async_context(aiodocker.Docker()) |
There was a problem hiding this comment.
1 question: did you check what happens if the connection to the docker engine is broken. does this client reconnects?
There was a problem hiding this comment.
It's based on HTTP not TCP, so there is no such issue. If the connection comes back this will work as expcted
| yield {} | ||
|
|
||
|
|
||
| def setup_remote_docker_client(app: FastAPI, settings: DockerApiProxysettings) -> None: |
There was a problem hiding this comment.
should this not be using the new lifespan mechanisms?
There was a problem hiding this comment.
this should not be a "remote_docker_client" but the "docker_client".
and if there is no settings it should create the default client with the unix socket.
There was a problem hiding this comment.
There is a lifespan, for services which use it. In this situation it is required since not all of them use it
There was a problem hiding this comment.
The intent of the name is to avoid any possible confusion that this is a docker client that uses the local docker socket.
I want to keep this very obvious since mixing them is bad. Especially since we only have 1 machine for development and not multiple node machines.
This is client is intended to use docker swarm API and runs only on maser nodes.
If you are on a worker and need to list the containers, this will not work, since the connection will point to a docker master.
If you don't like the name, I suggest to make it even more obvious. Something like swarm_master_node_docker_client. It has to be obvious.
There was a problem hiding this comment.
For the reasons above, this should never user the local docker socket
|
|
||
| @pytest.fixture() | ||
| def mock_env( | ||
| mock_setup_remote_docker_client: Callable[[str], None], |
There was a problem hiding this comment.
I do not think this is necessary for unit tests. this should just be disabled.
|
|
||
| # pylint: disable=too-many-branches | ||
| async def _create_docker_service_params( | ||
| async def _create_docker_service_params( # noqa: C901, PLR0912, PLR0913, PLR0915 |
There was a problem hiding this comment.
Those are lint rule suppressions (typically for flake8 / Ruff):
- C901 — Function is too complex (cyclomatic complexity too high)
- PLR0912 — Too many branches in a function
- PLR0913 — Too many arguments in a function/method
- PLR0915 — Too many statements in a function
So:
# noqa: C901, PLR0912, PLR0913, PLR0915
tells the linter to ignore complexity-related warnings for that line or function — basically “yes, this >function is big/complex, and that’s intentional.”
I think we should be careful ignoring these!! IMO now the RoI is even greater provided that we can pair-program with copilot on these issues
| async def on_shutdown() -> None: | ||
| await exit_stack.aclose() | ||
|
|
||
| app.add_event_handler("startup", on_startup) |
There was a problem hiding this comment.
why not using lifespan events in the fast app instead of these deprecate events https://fastapi.tiangolo.com/advanced/events/
if new code keeps the deprecated events ... we will never get the code up to date. Right now we have a nasty melange of all of these events :-(
There was a problem hiding this comment.
Because not all services support them. And we cannot mix them easily. Either the entire service is lifespan based or setup based.
|
|
||
|
|
||
| async def update_scheduler_data_label(scheduler_data: SchedulerData) -> None: | ||
| client = get_remote_docker_client(app) |
There was a problem hiding this comment.
THOUGHT: Basically, the new proxy client only requires replacing
async with docker_client() as client:
#
client.services. # same Docker client API
#with
client = get_remote_docker_client(app)
#
client.services. # same Docker client API
#I’m wondering whether using a context manager instead of a simple getter (get_remote_docker_client) would have kept the calling code almost unchanged while also making the client’s usage scope explicit. That scope information could then be used for logging start/stop events, timing, or other cross-cutting concerns, e.g.
async with remote_docker_client_ctx(app) as client:
#
client.services. # same Docker client API
#There was a problem hiding this comment.
We have a mix of styles with this client. And I only wanted to do one. Since a single instance apparently works well. I wanted to unify the pattern and avid client creation on the fly.
|
6 participants
What do these changes do?
NOTE I have recreated the PR since the previous one was way out of scope. (@pcrespov @sanderegg please bare with me and review it one more time 🙏 )
This PR is very noisy and it replaces the local docker client, used to access the swarm API in the with the
docker-api-proxyservice in the following services:autoscaling(optional when the image is started inside the computational cluster)director(mandatory)director-v2(mandatory)Related issue/s
How to test
Dev-ops