🎨 `docker-api-proxy` always requires authentication (⚠️devops) #7586

GitHK · 2025-04-25T11:05:22Z

What do these changes do?

Related issue/s

How to test

⚠️ Dev-ops checklist

No ENV changes or I properly updated ENV (read the instruction)
merge this MR before merging to MASTER
merge this MR during the release to STAGING
merge this MR during the release to PROD

codecov · 2025-04-25T11:07:04Z

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.46%. Comparing base (cb79d90) to head (edbf301).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7586      +/-   ##
==========================================
- Coverage   87.69%   87.46%   -0.24%     
==========================================
  Files        1774     1730      -44     
  Lines       68426    66084    -2342     
  Branches     1125     1123       -2     
==========================================
- Hits        60006    57799    -2207     
+ Misses       8113     7978     -135     
  Partials      307      307

Flag	Coverage Δ
integrationtests	`65.09% <ø> (-0.04%)`	⬇️
unittests	`86.62% <50.00%> (-0.26%)`	⬇️

Components	Coverage Δ
api	`∅ <ø> (∅)`
pkg_aws_library	`93.91% <ø> (ø)`
pkg_dask_task_models_library	`97.10% <ø> (ø)`
pkg_models_library	`92.72% <ø> (ø)`
pkg_notifications_library	`85.26% <ø> (ø)`
pkg_postgres_database	`88.18% <ø> (ø)`
pkg_service_integration	`69.92% <ø> (ø)`
pkg_service_library	`73.01% <0.00%> (+0.06%)`	⬆️
pkg_settings_library	`90.90% <100.00%> (ø)`
pkg_simcore_sdk	`85.72% <ø> (+0.05%)`	⬆️
agent	`96.46% <ø> (ø)`
api_server	`91.27% <ø> (ø)`
autoscaling	`∅ <ø> (∅)`
catalog	`92.64% <ø> (ø)`
clusters_keeper	`99.25% <ø> (ø)`
dask_sidecar	`91.29% <ø> (ø)`
datcore_adapter	`98.12% <ø> (ø)`
director	`76.80% <ø> (ø)`
director_v2	`91.38% <ø> (ø)`
dynamic_scheduler	`97.40% <ø> (ø)`
dynamic_sidecar	`90.15% <ø> (ø)`
efs_guardian	`89.79% <ø> (ø)`
invitations	`93.28% <ø> (ø)`
payments	`92.66% <ø> (ø)`
resource_usage_tracker	`89.12% <ø> (-0.11%)`	⬇️
storage	`87.56% <ø> (-0.11%)`	⬇️
webclient	`∅ <ø> (∅)`
webserver	`86.08% <ø> (+0.02%)`	⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb79d90...edbf301. Read the comment docs.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…ic-auth-to-docker-api-proxy

sonarqubecloud · 2025-04-25T11:27:28Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

services/docker-api-proxy/docker/boot.sh

mrnicegyu11

thx!

pcrespov

Thx. Left some comments.

pcrespov · 2025-04-25T12:49:36Z

packages/pytest-simcore/src/pytest_simcore/docker_api_proxy.py

@@ -22,7 +22,13 @@
 async def _wait_till_docker_api_proxy_is_responsive(
    settings: DockerApiProxysettings,
 ) -> None:
-    async with ClientSession(timeout=ClientTimeout(1, 1, 1, 1, 1)) as client:
+    async with ClientSession(
+        timeout=ClientTimeout(1, 1, 1, 1, 1),


MINOR: i would add keyword arguments to know what these 1,1,1,1 refers to

Yep, this is very unclear. what is 1? a second a millisecond? what is the point of defining all of them? since this is aiohttp, you can also define only 1 argument instead of having this super complicated thing. one of the timeout is getting a client from the pool, etc etc...
thought: Would it not make sense to create 1 client session for waiting instead of re-creating each time?

pcrespov · 2025-04-25T12:50:39Z

packages/service-library/src/servicelib/fastapi/docker.py

-                        login=settings.DOCKER_API_PROXY_USER,
-                        password=settings.DOCKER_API_PROXY_PASSWORD.get_secret_value(),
-                    )
+        session = await exit_stack.enter_async_context(


where is this session used?

pcrespov · 2025-04-25T12:53:49Z

services/docker-api-proxy/docker/Caddyfile

@@ -0,0 +1,11 @@
+:8888 {
+    handle {
+        basicauth {


So this container has two apps? One reverse-proxy (caddy) that auths, and the actual python app that access docker api.

Q: does this service need special tuning of resources ( e.g. 1-2 CPUs? )

pcrespov · 2025-04-25T12:54:39Z

services/docker-api-proxy/tests/integration/test_docker_api_proxy.py

pcrespov · 2025-04-25T12:56:14Z

services/docker-api-proxy/tests/integration/test_docker_api_proxy.py

+    async with setup_docker_client(envs) as working_docker:
+        with pytest.raises(aiodocker.exceptions.DockerError) as exc:
+            await working_docker.system.info()
+        assert exc.value.status == 401


MINOR: use status.HTTP_*** constants instead. They are more readable

assert exec.value.status == status.HTTP_40_UNAUTHORIZED

sanderegg

thanks!

sanderegg · 2025-04-27T19:27:10Z

packages/pytest-simcore/src/pytest_simcore/docker_api_proxy.py

@@ -22,7 +22,13 @@
 async def _wait_till_docker_api_proxy_is_responsive(
    settings: DockerApiProxysettings,
 ) -> None:
-    async with ClientSession(timeout=ClientTimeout(1, 1, 1, 1, 1)) as client:
+    async with ClientSession(
+        timeout=ClientTimeout(1, 1, 1, 1, 1),


Yep, this is very unclear. what is 1? a second a millisecond? what is the point of defining all of them? since this is aiohttp, you can also define only 1 argument instead of having this super complicated thing. one of the timeout is getting a client from the pool, etc etc...
thought: Would it not make sense to create 1 client session for waiting instead of re-creating each time?

sanderegg · 2025-04-27T19:28:20Z

packages/settings-library/src/settings_library/docker_api_proxy.py

-    DOCKER_API_PROXY_USER: str | None = None
-    DOCKER_API_PROXY_PASSWORD: SecretStr | None = None
+    DOCKER_API_PROXY_USER: str
+    DOCKER_API_PROXY_PASSWORD: SecretStr


so this was usable before right? Why do you then need 3 different MRs for the ops part?

sanderegg · 2025-04-27T19:30:06Z

services/docker-api-proxy/Dockerfile

@@ -29,10 +29,11 @@ HEALTHCHECK \
  --start-period=20s \
  --start-interval=1s \
  --retries=5 \
-  CMD curl http://localhost:8888/version || exit 1
+  CMD curl --fail-with-body -u ${DOCKER_API_PROXY_USER}:${DOCKER_API_PROXY_PASSWORD} http://localhost:8888/version


is --user not available since it is alpine?

service always requires credentials now

4ac92b3

GitHK self-assigned this Apr 25, 2025

GitHK added this to the Pauwel Kwak milestone Apr 25, 2025

GitHK changed the title ~~🎨 docker-api-proxy always requires authentication now~~ 🎨 docker-api-proxy always requires authentication now (⚠️devops) Apr 25, 2025

Andrei Neagu added 2 commits April 25, 2025 13:23

Merge remote-tracking branch 'upstream/master' into pr-osparc-add-bas…

9e4e4f6

…ic-auth-to-docker-api-proxy

fixed broken tests

edbf301

GitHK changed the title ~~🎨 docker-api-proxy always requires authentication now (⚠️devops)~~ 🎨 docker-api-proxy always requires authentication (⚠️devops) Apr 25, 2025

GitHK marked this pull request as ready for review April 25, 2025 11:31

GitHK requested review from sanderegg, mrnicegyu11, YuryHrytsuk and pcrespov as code owners April 25, 2025 11:31

GitHK requested review from giancarloromeo, matusdrobuliak66 and bisgaard-itis April 25, 2025 11:31

giancarloromeo approved these changes Apr 25, 2025

View reviewed changes

mrnicegyu11 reviewed Apr 25, 2025

View reviewed changes

services/docker-api-proxy/docker/boot.sh Show resolved Hide resolved

mrnicegyu11 approved these changes Apr 25, 2025

View reviewed changes

pcrespov approved these changes Apr 25, 2025

View reviewed changes

sanderegg approved these changes Apr 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎨 `docker-api-proxy` always requires authentication (⚠️devops) #7586

🎨 `docker-api-proxy` always requires authentication (⚠️devops) #7586

GitHK commented Apr 25, 2025 •

edited

Loading

codecov bot commented Apr 25, 2025 •

edited

Loading

sonarqubecloud bot commented Apr 25, 2025

mrnicegyu11 left a comment

pcrespov left a comment

pcrespov Apr 25, 2025

sanderegg Apr 27, 2025

pcrespov Apr 25, 2025

pcrespov Apr 25, 2025

pcrespov Apr 25, 2025

pcrespov Apr 25, 2025

sanderegg left a comment

sanderegg Apr 27, 2025

sanderegg Apr 27, 2025

sanderegg Apr 27, 2025

🎨 docker-api-proxy always requires authentication (⚠️devops) #7586

Are you sure you want to change the base?

🎨 docker-api-proxy always requires authentication (⚠️devops) #7586

Conversation

GitHK commented Apr 25, 2025 • edited Loading

What do these changes do?

Related issue/s

How to test

⚠️ Dev-ops checklist

codecov bot commented Apr 25, 2025 • edited Loading

Codecov Report

sonarqubecloud bot commented Apr 25, 2025

Quality Gate passed

mrnicegyu11 left a comment

Choose a reason for hiding this comment

pcrespov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderegg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

🎨 `docker-api-proxy` always requires authentication (⚠️devops) #7586

🎨 `docker-api-proxy` always requires authentication (⚠️devops) #7586

GitHK commented Apr 25, 2025 •

edited

Loading

codecov bot commented Apr 25, 2025 •

edited

Loading