Skip to content
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

destination-s3: Use airbyte/java-connector-base:2.0.0 #51536

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alafanechere
Copy link
Contributor

What

Make the connector use our official non-root base image for java connectors

@alafanechere alafanechere requested a review from a team as a code owner January 14, 2025 10:53
Copy link

vercel bot commented Jan 14, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 17, 2025 8:46pm

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/destination/s3 labels Jan 14, 2025
Copy link
Contributor Author

alafanechere commented Jan 14, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@alafanechere alafanechere force-pushed the augustin/01-14-destination-s3_Use_airbyte/java-connector-base_2.0.0 branch from 86d5656 to 57787f0 Compare January 14, 2025 10:53
@edgao
Copy link
Contributor

edgao commented Jan 15, 2025

resolve: process "sh /tmp/finalize_build.sh" did not complete successfully: exit code: 1
Stdout:
Running destination-s3 docker custom steps...
x86_64
Stderr:
Error: This command has to be run with superuser privileges (under the root user on most systems).

... the dest-s3 build is running yum install, which I guess needs sudo :/ is there some way to run the build as root, but run the actual container as non-root?

@edgao
Copy link
Contributor

edgao commented Jan 15, 2025

oh right, this is in the finalize script, not even the docker build itself. @alafanechere do we have a way to run that script as root?

@alafanechere
Copy link
Contributor Author

oh right, this is in the finalize script, not even the docker build itself. @alafanechere do we have a way to run that script as root?

You're right. It should be run as root at build time then. It'll require an airbyte-ci change. I'll do that.

@alafanechere alafanechere force-pushed the augustin/01-14-destination-s3_Use_airbyte/java-connector-base_2.0.0 branch from 57787f0 to 0b45b8d Compare January 16, 2025 11:30
@alafanechere alafanechere requested a review from a team as a code owner January 16, 2025 11:30
@alafanechere alafanechere changed the base branch from master to augustin/01-16-airbyte-ci_run_finalize_build_script_as_root January 16, 2025 11:31
@alafanechere alafanechere force-pushed the augustin/01-16-airbyte-ci_run_finalize_build_script_as_root branch from 5912cc3 to 1921b9d Compare January 16, 2025 11:36
Base automatically changed from augustin/01-16-airbyte-ci_run_finalize_build_script_as_root to master January 16, 2025 12:03
@alafanechere alafanechere force-pushed the augustin/01-14-destination-s3_Use_airbyte/java-connector-base_2.0.0 branch from 0b45b8d to d3a43b1 Compare January 16, 2025 12:04
@alafanechere
Copy link
Contributor Author

@edgao airbyte-ci now runs the finalize script as root. The build is passing 👍
Unfortunately the integration tests are not...
I'm seeing errors like:

Caused by: software.amazon.awssdk.crt.CrtRuntimeException: software.amazon.awssdk.crt.CrtRuntimeException: Invalid directory: /tmp UNKNOWN(-1) UNKNOWN(-1)

I'm surprised because it should be allowed to write to /tmp according to our base image definition. . But when I shell in the built image I can't indeed write to this path. I'm investigating now.

@alafanechere
Copy link
Contributor Author

I think I understand what's happenning:

  • When we run finalize as route some files get written to /tmp, and the owner is root. Which supersedes the original ownership of this dir to the airbyte user.
    I'm going to change airbyte-ci to give /tmp ownership back to airbyte

@alafanechere alafanechere force-pushed the augustin/01-14-destination-s3_Use_airbyte/java-connector-base_2.0.0 branch from d3a43b1 to 9a6ea21 Compare January 16, 2025 12:20
@alafanechere alafanechere changed the base branch from master to augustin/01-16-airbyte-ci_give_back_ownership_of_/tmp_to_the_original_user_when_running_finalize_in_build January 16, 2025 12:20
@alafanechere
Copy link
Contributor Author

alafanechere commented Jan 16, 2025

@edgao so to summarize I made two changes to airbyte-ci:

  • Run finalize as root - which could change the ownership of /tmp
  • When finalize is done give back ownership of /tmp to airbyte/

I confirmed that build is passing and when shelling into the built container I'm able to read and write to /tmp.
But we still have this error:

Caused by: software.amazon.awssdk.crt.CrtRuntimeException: software.amazon.awssdk.crt.CrtRuntimeException: Invalid directory: /tmp UNKNOWN(-1) UNKNOWN(-1)

I think that it's the same problem we have with other python connectors we're trying to make non root.
I've probably identified the root cause: we mount a volume at test time to /tmp and the owner of this volume is root. I'm on it.

@alafanechere alafanechere force-pushed the augustin/01-16-airbyte-ci_give_back_ownership_of_/tmp_to_the_original_user_when_running_finalize_in_build branch from d43cc5e to 65594e3 Compare January 16, 2025 13:00
Base automatically changed from augustin/01-16-airbyte-ci_give_back_ownership_of_/tmp_to_the_original_user_when_running_finalize_in_build to master January 16, 2025 13:01
@alafanechere alafanechere force-pushed the augustin/01-14-destination-s3_Use_airbyte/java-connector-base_2.0.0 branch 2 times, most recently from 16facc0 to 8de50bd Compare January 16, 2025 15:51
@alafanechere alafanechere changed the base branch from master to augustin/01-16-airbyte-ci_mount_/tmp_with_the_current_user_as_owner January 16, 2025 15:51
@alafanechere alafanechere force-pushed the augustin/01-16-airbyte-ci_mount_/tmp_with_the_current_user_as_owner branch from a2a0c17 to 63313ab Compare January 16, 2025 16:06
@alafanechere alafanechere force-pushed the augustin/01-14-destination-s3_Use_airbyte/java-connector-base_2.0.0 branch from 8de50bd to e7663df Compare January 16, 2025 16:06
@alafanechere alafanechere force-pushed the augustin/01-16-airbyte-ci_mount_/tmp_with_the_current_user_as_owner branch 2 times, most recently from 846b12f to a0c9414 Compare January 17, 2025 12:03
@alafanechere alafanechere force-pushed the augustin/01-14-destination-s3_Use_airbyte/java-connector-base_2.0.0 branch from e7663df to 2a95267 Compare January 17, 2025 12:03
@alafanechere alafanechere force-pushed the augustin/01-16-airbyte-ci_mount_/tmp_with_the_current_user_as_owner branch from 3580a7a to 86aca90 Compare January 17, 2025 14:57
@alafanechere alafanechere force-pushed the augustin/01-14-destination-s3_Use_airbyte/java-connector-base_2.0.0 branch from 2a95267 to 82add22 Compare January 17, 2025 17:21
@alafanechere alafanechere force-pushed the augustin/01-16-airbyte-ci_mount_/tmp_with_the_current_user_as_owner branch 3 times, most recently from dc96eaa to 4660e5a Compare January 17, 2025 18:29
@alafanechere alafanechere force-pushed the augustin/01-14-destination-s3_Use_airbyte/java-connector-base_2.0.0 branch from 82add22 to fc863c5 Compare January 17, 2025 18:29
@alafanechere alafanechere force-pushed the augustin/01-16-airbyte-ci_mount_/tmp_with_the_current_user_as_owner branch from 4660e5a to 518aeed Compare January 17, 2025 20:39
@alafanechere alafanechere force-pushed the augustin/01-14-destination-s3_Use_airbyte/java-connector-base_2.0.0 branch from fc863c5 to 43b8b43 Compare January 17, 2025 20:39
Base automatically changed from augustin/01-16-airbyte-ci_mount_/tmp_with_the_current_user_as_owner to master January 17, 2025 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation base-image-migration connectors/destination/s3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants