Skip to content

feat: add "runas_host_user" param to get_source() #614

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

feat: add "runas_host_user" param to get_source() #614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c660095
feat: add "runas_host_user" param to get_source()
nicob3y Feb 13, 2025
414ec3d
refacto: "runas_host_user" required a POSIX system to work
nicob3y Mar 19, 2025
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
10 changes: 10 additions & 0 deletions airbyte/_executors/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
# Copyright (c) 2023 Airbyte, Inc., all rights reserved.
from __future__ import annotations

import os
import tempfile
import logging
from pathlib import Path
from typing import TYPE_CHECKING, Literal, cast

Expand All @@ -20,6 +22,7 @@
from airbyte.sources.registry import ConnectorMetadata, InstallType, get_connector_metadata
from airbyte.version import get_version

logger = logging.getLogger("airbyte")

if TYPE_CHECKING:
from airbyte._executors.base import Executor
Expand Down Expand Up @@ -123,6 +126,7 @@ def get_connector_executor( # noqa: PLR0912, PLR0913, PLR0915 # Too many branch
local_executable: Path | str | None = None,
docker_image: bool | str | None = None,
use_host_network: bool = False,
runas_host_user: bool = False,
source_manifest: bool | dict | Path | str | None = None,
install_if_missing: bool = True,
install_root: Path | None = None,
Expand Down Expand Up @@ -236,6 +240,12 @@ def get_connector_executor( # noqa: PLR0912, PLR0913, PLR0915 # Too many branch
if use_host_network is True:
docker_cmd.extend(["--network", "host"])

if runas_host_user is True:
try:
docker_cmd.extend(["--user", f"{os.getuid()}:{os.getgid()}"])
except:
logger.info("The 'runas_host_user' parameter is only supported on POSIX systems.")

Comment on lines +243 to +248
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid bare except and consider reporting non-POSIX limitation more visibly.

The implementation of the runas_host_user parameter makes sense, but there are a couple of issues:

  1. The bare except clause is flagged by linters and should be replaced with specific exception types.
  2. The behavior silently falls back without the --user flag on non-POSIX systems.

What do you think about this implementation?

if runas_host_user is True:
    try:
        docker_cmd.extend(["--user", f"{os.getuid()}:{os.getgid()}"])
-    except:
+    except AttributeError:
        logger.info("The 'runas_host_user' parameter is only supported on POSIX systems.")
+        logger.warning("Continuing without host user permissions. File operations may fail.")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if runas_host_user is True:
try:
docker_cmd.extend(["--user", f"{os.getuid()}:{os.getgid()}"])
except:
logger.info("The 'runas_host_user' parameter is only supported on POSIX systems.")
if runas_host_user is True:
try:
docker_cmd.extend(["--user", f"{os.getuid()}:{os.getgid()}"])
except AttributeError:
logger.info("The 'runas_host_user' parameter is only supported on POSIX systems.")
logger.warning("Continuing without host user permissions. File operations may fail.")
🧰 Tools
🪛 Ruff (0.8.2)

246-246: Do not use bare except

(E722)

🪛 GitHub Actions: Run Linters

[error] 246-246: Ruff: Do not use bare except.

docker_cmd.extend([docker_image])

return DockerExecutor(
Expand Down
5 changes: 5 additions & 0 deletions airbyte/destinations/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ def get_destination( # noqa: PLR0913 # Too many arguments
local_executable: Path | str | None = None,
docker_image: str | bool | None = None,
use_host_network: bool = False,
runas_host_user: bool = False,
install_if_missing: bool = True,
) -> Destination:
"""Get a connector by name and version.
Expand Down Expand Up @@ -56,6 +57,9 @@ def get_destination( # noqa: PLR0913 # Too many arguments
the host network. This is useful for connectors that need to access resources on
the host machine, such as a local database. This parameter is ignored when
`docker_image` is not set.
runas_host_user: If set, along with docker_image, the connector will be executed using the
host's user UID/GID. This is useful to handle container privileges (such as files
permissions). This parameter is ignored when `docker_image` is not set.
install_if_missing: Whether to install the connector if it is not available locally. This
parameter is ignored when local_executable is set.
"""
Expand All @@ -70,6 +74,7 @@ def get_destination( # noqa: PLR0913 # Too many arguments
local_executable=local_executable,
docker_image=docker_image,
use_host_network=use_host_network,
runas_host_user=runas_host_user,
install_if_missing=install_if_missing,
),
)
Expand Down
5 changes: 5 additions & 0 deletions airbyte/sources/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ def get_source( # noqa: PLR0913 # Too many arguments
local_executable: Path | str | None = None,
docker_image: bool | str | None = None,
use_host_network: bool = False,
runas_host_user: bool = False,
source_manifest: bool | dict | Path | str | None = None,
install_if_missing: bool = True,
install_root: Path | None = None,
Expand Down Expand Up @@ -95,6 +96,9 @@ def get_source( # noqa: PLR0913 # Too many arguments
the host network. This is useful for connectors that need to access resources on
the host machine, such as a local database. This parameter is ignored when
`docker_image` is not set.
runas_host_user: If set, along with docker_image, the connector will be executed using the
host's user UID/GID. This is useful to handle container privileges (such as files
permissions). This parameter is ignored when `docker_image` is not set.
source_manifest: If set, the connector will be executed based on a declarative YAML
source definition. This input can be `True` to attempt to auto-download a YAML spec,
`dict` to accept a Python dictionary as the manifest, `Path` to pull a manifest from
Expand All @@ -116,6 +120,7 @@ def get_source( # noqa: PLR0913 # Too many arguments
local_executable=local_executable,
docker_image=docker_image,
use_host_network=use_host_network,
runas_host_user=runas_host_user,
source_manifest=source_manifest,
install_if_missing=install_if_missing,
install_root=install_root,
Expand Down
Loading