Skip to content

fix: add --user flag to Docker container tests to allow config migration writes #639

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

devin-ai-integration[bot]
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Jul 3, 2025

Fix Docker container test permission errors for config migration

Summary

Resolves PermissionError: [Errno 13] Permission denied: '/secrets/config.json' that occurs when connectors attempt to migrate and write back config during Docker container tests. The issue happens because config files are mounted as read-only volumes, but ManifestDeclarativeSource._migrate_and_transform_config() needs write access to persist migrated configurations.

Root cause: Temporary config files are created with read-only permissions (0o444) and mounted as Docker volumes. When the container process (running as a different user) tries to write migrated config back to /secrets/config.json, it fails due to permission mismatch.

Solution: Added --user flag with host UID/GID (f"{os.getuid()}:{os.getgid()}") to all Docker run commands in container tests, ensuring the container process has the same file permissions as the host user who created the temp files.

Review & Testing Checklist for Human

  • Test with real connector migration: Run Docker container tests with a connector that performs config migration to verify the PermissionError is resolved
  • Verify CI compatibility: Ensure the fix works in GitHub Actions CI environment, not just locally
  • Test non-migration scenarios: Confirm existing functionality isn't broken for connectors without config migration
  • Cross-platform verification: Test on different OS if applicable (the os.getuid()/getgid() calls might behave differently)

Recommended test plan:

  1. Find a connector with config migration logic (like the one that triggered the original error)
  2. Run the Docker container tests locally and in CI
  3. Verify both migration and non-migration test scenarios pass
  4. Check that no new permission-related errors are introduced

Diagram

%%{ init : { "theme" : "default" }}%%
graph TD
    subgraph "Test Flow"
        A["scenario.py<br/>with_temp_config_file()"]
        B["docker_base.py<br/>Docker run commands"]
        C["Container Process<br/>/secrets/config.json"]
        D["manifest_declarative_source.py<br/>_migrate_and_transform_config()"]
    end
    
    A -->|"Creates read-only<br/>temp config (0o444)"| B
    B -->|"Mounts as volume<br/>--user UID:GID"| C
    C -->|"Reads config"| D
    D -->|"Writes migrated config<br/>(now has permissions)"| C
    
    B:::major-edit
    A:::context
    C:::context
    D:::context
    
    subgraph Legend
        L1["Major Edit"]:::major-edit
        L2["Minor Edit"]:::minor-edit
        L3["Context/No Edit"]:::context
    end
    
    classDef major-edit fill:#90EE90
    classDef minor-edit fill:#87CEEB  
    classDef context fill:#FFFFFF
Loading

Notes

  • This fix follows the standard Docker pattern for resolving permission issues when mounting host files
  • The change affects three test methods: test_docker_image_build_and_check, and two commands in test_docker_image_build_and_read (discover and read)
  • No existing patterns were found in PyAirbyte or connector acceptance tests for handling config write scenarios - they assume read-only access
  • Important: I was unable to test this locally since Docker container tests require specific connector images and scenarios. Human testing is critical to verify the fix works correctly.

Link to Devin run: https://app.devin.ai/sessions/145920cef86e4143bca1763d0cd2285a
Requested by: @aaronsteers

…ion writes

- Fixes PermissionError when config migration tries to write to /secrets/config.json
- Adds --user flag with host UID/GID to all Docker run commands in container tests
- Allows ManifestDeclarativeSource._migrate_and_transform_config() to write back migrated configs

Co-Authored-By: AJ Steers <[email protected]>
Copy link
Contributor Author

Original prompt from AJ Steers:

SYSTEM:
=== BEGIN THREAD HISTORY (in #dev-extensibility) ===
Patrick Nilan (U063M9WHWTU): <@U05AKF1BCC9> cc <@U03APJQ895J>

I am getting the following error when running <https://github.com/airbytehq/airbyte/actions/runs/16036799150/job/45250861132?pr=62480|integration tests via CI checks> when the source attempts to migrate the config and save the migrated config back to its file path:

&gt;  PermissionError: [Errno 13] Permission denied: \'/secrets/config.json\'


Patrick Nilan (U063M9WHWTU): Related to this test: <https://github.com/airbytehq/airbyte-python-cdk/blob/a562875fafc8b0c6bb658dd2be20f7475cc5dd0b/airbyte_cdk/test/standard_tests/docker_base.py#L206>

<https://github.com/airbytehq/airbyte-python-cdk/blob/a562875fafc8b0c6bb658dd2be20f7475cc5dd0b/airbyte_cdk/test/standard_tests/docker_base.py|docker_base.py>

Patrick Nilan (U063M9WHWTU): I wonder if there is a way to make the path writeable?

AJ Steers (U05AKF1BCC9): Ok, this indicates the config.json is read only. Let me look into it. I've not seen this before but it makes sense.

Patrick Nilan (U063M9WHWTU): I should be able to get around this by "manually migrating" the config in GSM

AJ Steers (U05AKF1BCC9): Let me make sure I understand...

Should config updates be sent via CONTROL messages, rather than updating the existing config file?

Patrick Nilan (U063M9WHWTU): It does both

Patrick Nilan (U063M9WHWTU): Here is the method in `ManifestDeclarativeSource`: <https://github.com/airbytehq/airbyte-python-cdk/blob/a562875fafc8b0c6bb658dd2be20f7475cc5dd0b/airbyte_cdk/sources/declarative/manifest_declarative_source.py#L217|https://github.com/airbytehq/airbyte-python-cdk/blob/a562875fafc8b0c6bb658dd2be20f[…]/airbyte_cdk/sources/declarative/manifest_declarative_source.py>

<https://github.com/airbytehq/airbyte-python-cdk/blob/a562875fafc8b0c6bb658dd2be20f7475cc5dd0b/airbyte_cdk/sources/declarative/manifest_declarative_source.py|manifest_declarative_source.py>

AJ Steers (U05AKF1BCC9): Pardon my ignorance on this... It sounds like this has always been the case, updating the file as well as sending the control message? (Sounds like yes.)

And is it fine that the local file update will be ignored since the run is ephemeral? (I think yes.)

Assuming both of those, it sounds like we need to update the container test to give a writeable file and not a read-only file.

Patrick Nilan (U063M9WHWTU): Yes to all of the above!

Patrick Nilan (U063M9WHWTU): Writing back down to a file path is mostly for the persisting changes locally (I think)

Patrick Nilan (U063M9WHWTU): I think we could also skip passing in the config_path

AJ Steers (U05AKF1BCC9): Kk. Thanks for confirming.

Patrick Nilan (U063M9WHWTU): since config path only enables writing down:

ATTACHMENT:"https://app.devin.ai/attachments/65ad5c83-c46f-46f7-aaee-e7bacde8ade9/Screenshot+2025-07-02+at+4.08.52%3FPM.png"

AJ Steers (U05AKF1BCC9): The user in the container is "airbyte" and that user won't necessarily exist on the host, we probably need to either give global read/write access to the config file, or else invoke the docker container with a "run as user" override to be the host user that created the file.

AJ Steers (U05AKF1BCC9): I'm going to have to log off soon and I only have tomorrow before I'm ooo for a week. Calling in some help from the bots :robot_face: ...

AJ Steers (U05AKF1BCC9): @Devin - can you look into this and make a suggestion? The code in question is the image test command in the Python CDK cli. You can look at the old connector acceptance tests in the Airbyte repo, and/or the PyAirbyte repo to see what is being done elsewhere to tolerate writes back to the config file.
=== END THREAD HISTORY ===

The latest message is the one right above that tagged you.

Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@github-actions github-actions bot added bug Something isn't working security labels Jul 3, 2025
Copy link

github-actions bot commented Jul 3, 2025

PyTest Results (Fast)

3 685 tests  ±0   3 674 ✅ ±0   6m 17s ⏱️ -1s
    1 suites ±0      11 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit b6c33bf. ± Comparison against base commit 8659a21.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jul 3, 2025

PyTest Results (Full)

3 688 tests   3 677 ✅  18m 2s ⏱️
    1 suites     11 💤
    1 files        0 ❌

Results for commit b6c33bf.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Fixes permission errors in Docker container tests by running containers with the same UID/GID as the host, allowing config migration to write back to mounted files.

  • Imported os to retrieve host user IDs.
  • Added --user f"{os.getuid()}:{os.getgid()}" to three Docker run commands in tests.
Comments suppressed due to low confidence (1)

airbyte_cdk/test/standard_tests/docker_base.py:212

  • Consider handling platforms where os.getuid()/os.getgid() may not be available (e.g., Windows) by falling back to default user or skipping the flag when these functions are not defined.
                    "--user",

@aaronsteers aaronsteers closed this Jul 3, 2025
@aaronsteers aaronsteers reopened this Jul 3, 2025
Copy link

github-actions bot commented Jul 3, 2025

👋 Greetings, Airbyte Team Member!

Here are some helpful tips and reminders for your convenience.

Testing This CDK Version

You can test this version of the CDK using the following:

# Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@devin/1751498158-fix-docker-config-write-permissions#egg=airbyte-python-cdk[dev]' --help

# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch devin/1751498158-fix-docker-config-write-permissions

Helpful Resources

PR Slash Commands

Airbyte Maintainers can execute the following slash commands on your PR:

  • /autofix - Fixes most formatting and linting issues
  • /poetry-lock - Updates poetry.lock file
  • /test - Runs connector tests with the updated CDK
  • /poe <command> - Runs any poe command in the CDK environment

📝 Edit this welcome message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant