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

Python base image: create airbyte user #36544

Merged

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Mar 27, 2024

We want to make our python connector image use a non root user.
This PR cuts a new base image version which:

  • Creates an airbyte user
  • Make it the owner of /airbyte
  • Make it able to read the cache dir and the path to the nltk data

Copy link

vercel bot commented Mar 27, 2024

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

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Nov 22, 2024 8:45am

Copy link
Contributor Author

alafanechere commented Mar 27, 2024

Comment on lines 25 to 27
field
for field in list(container._ctx.selections)
if isinstance(field, dagger.client._core.Field) and field.type_name == "Container"
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is due to a change in the latest dagger version

@alafanechere alafanechere force-pushed the augustin/03-27-Python_base_image_create_airbyte_user branch from 1e27a25 to 95e918c Compare March 27, 2024 13:51
@alafanechere alafanechere changed the base branch from ng/base_images/rootless to master March 27, 2024 13:51
@alafanechere alafanechere marked this pull request as ready for review March 27, 2024 13:59
@alafanechere alafanechere requested a review from a team as a code owner March 27, 2024 13:59
Copy link
Contributor

@erohmensing erohmensing left a comment

Choose a reason for hiding this comment

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

I'll let someone else like Angel approve the functionality as I'm not comfortable saying I know all the pieces that need to change to make this change. Reviewing from a "update to base image side" and looks good

@@ -19,6 +19,10 @@ class AirbyteConnectorBaseImage(ABC):
Please do not declare any Dagger with_exec instruction in this class as in the abstract class context we have no guarantee about the underlying system used in the base image.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this concern still apply? I assume that the commands we're using are so basic as to not be concerned, but the messaging is confusing

@@ -67,7 +71,7 @@ def with_file_based_connector_dependencies(container: dagger.Container) -> dagge
- nltk data
"""
container = with_tesseract_and_poppler(container)
container = container.with_exec(["mkdir", self.nltk_data_path], skip_entrypoint=True).with_directory(
container = container.with_exec(["mkdir", "-p", "755", self.nltk_data_path], skip_entrypoint=True).with_directory(
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: as we in the base class are doing the setup of the airbyte user, I think(?) that all new directories in the python base or other future bases have to be made with these permissions. Maybe in the base class we could define a method that we can use in the subsclasses to create based on the permissions required from the base.

@@ -99,3 +99,74 @@ async def check_socat_version(container: dagger.Container, expected_socat_versio
raise errors.SanityCheckError(f"unexpected socat version: {version_number}")
else:
raise errors.SanityCheckError(f"Could not find the socat version in the version output: {socat_version_line}")


async def check_user_exists(container: dagger.Container, user: str):
Copy link
Contributor

Choose a reason for hiding this comment

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

Lots of sanity checks 🙏🏻

Comment on lines +122 to +134
await base_sanity_checks.check_user_can_read_dir(container, self.USER, self.AIRBYTE_DIR_PATH)
await base_sanity_checks.check_user_can_read_dir(container, self.USER, self.nltk_data_path)
await base_sanity_checks.check_user_can_read_dir(container, self.USER, self.CACHE_DIR_PATH)
await base_sanity_checks.check_user_can_write_dir(container, self.USER, self.AIRBYTE_DIR_PATH)
await base_sanity_checks.check_user_cant_write_dir(container, self.USER, self.CACHE_DIR_PATH)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: For a second I thought 124 and 126 were contradicting. I think this is easier to parse when organized by file:

Suggested change
await base_sanity_checks.check_user_can_read_dir(container, self.USER, self.AIRBYTE_DIR_PATH)
await base_sanity_checks.check_user_can_read_dir(container, self.USER, self.nltk_data_path)
await base_sanity_checks.check_user_can_read_dir(container, self.USER, self.CACHE_DIR_PATH)
await base_sanity_checks.check_user_can_write_dir(container, self.USER, self.AIRBYTE_DIR_PATH)
await base_sanity_checks.check_user_cant_write_dir(container, self.USER, self.CACHE_DIR_PATH)
await base_sanity_checks.check_user_can_read_dir(container, self.USER, self.AIRBYTE_DIR_PATH)
await base_sanity_checks.check_user_can_write_dir(container, self.USER, self.AIRBYTE_DIR_PATH)
await base_sanity_checks.check_user_can_read_dir(container, self.USER, self.nltk_data_path)
await base_sanity_checks.check_user_can_read_dir(container, self.USER, self.CACHE_DIR_PATH)
await base_sanity_checks.check_user_cant_write_dir(container, self.USER, self.CACHE_DIR_PATH)

.with_exec(["ln", "-snf", "/usr/share/zoneinfo/Etc/UTC", "/etc/localtime"], skip_entrypoint=True)
# Install socat 1.7.4.4
.with_exec(["sh", "-c", "apt update && apt-get install -y socat=1.7.4.4-2"], skip_entrypoint=True)
.with_exec(["adduser", "--system", "--group", "--no-create-home", self.USER], skip_entrypoint=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.with_exec(["adduser", "--system", "--group", "--no-create-home", self.USER], skip_entrypoint=True)
.with_exec(["adduser", "-u", "1000", "--system", "--group", "--no-create-home", self.USER], skip_entrypoint=True)

Copy link
Contributor

Choose a reason for hiding this comment

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

We'd like to make the uid/gid consistent with the platform images (where uid=1000(airbyte) gid=1000(airbyte))

@alafanechere alafanechere force-pushed the augustin/03-27-Python_base_image_create_airbyte_user branch from 95e918c to 13f5b18 Compare March 28, 2024 08:33
@alafanechere alafanechere force-pushed the augustin/03-27-Python_base_image_create_airbyte_user branch from 13f5b18 to 1eef8aa Compare June 6, 2024 12:30
@alafanechere alafanechere force-pushed the augustin/03-27-Python_base_image_create_airbyte_user branch from 1eef8aa to c16bb07 Compare June 7, 2024 17:13
@alafanechere alafanechere force-pushed the augustin/03-27-Python_base_image_create_airbyte_user branch 2 times, most recently from c402324 to c12d318 Compare June 26, 2024 04:24
@alafanechere alafanechere force-pushed the augustin/03-27-Python_base_image_create_airbyte_user branch from c12d318 to c1d53e3 Compare November 20, 2024 13:41
@alafanechere alafanechere requested a review from a team as a code owner November 20, 2024 13:41
@alafanechere alafanechere force-pushed the augustin/03-27-Python_base_image_create_airbyte_user branch from c1d53e3 to f7a67a8 Compare November 20, 2024 14:46
@alafanechere alafanechere changed the base branch from master to augustin/11-20-base-images_prompt_for_bump_type_with_optional_pre-releases November 20, 2024 14:47
@alafanechere alafanechere force-pushed the augustin/11-20-base-images_prompt_for_bump_type_with_optional_pre-releases branch from 705d03f to ea3234a Compare November 22, 2024 08:02
@alafanechere alafanechere force-pushed the augustin/03-27-Python_base_image_create_airbyte_user branch from a2280fd to b924361 Compare November 22, 2024 08:02
@alafanechere alafanechere force-pushed the augustin/11-20-base-images_prompt_for_bump_type_with_optional_pre-releases branch from ea3234a to a78d22e Compare November 22, 2024 08:04
@alafanechere alafanechere force-pushed the augustin/03-27-Python_base_image_create_airbyte_user branch 2 times, most recently from 94a4b4f to 17ef12f Compare November 22, 2024 08:10
Copy link
Contributor Author

alafanechere commented Nov 22, 2024

Merge activity

  • Nov 22, 3:42 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Nov 22, 3:45 AM EST: Graphite rebased this pull request as part of a merge.
  • Nov 22, 3:46 AM EST: A user merged this pull request with Graphite.

@alafanechere alafanechere changed the base branch from augustin/11-20-base-images_prompt_for_bump_type_with_optional_pre-releases to graphite-base/36544 November 22, 2024 08:42
@alafanechere alafanechere changed the base branch from graphite-base/36544 to master November 22, 2024 08:42
@alafanechere alafanechere force-pushed the augustin/03-27-Python_base_image_create_airbyte_user branch from 17ef12f to d116914 Compare November 22, 2024 08:44
@alafanechere alafanechere merged commit ab6cd25 into master Nov 22, 2024
24 of 27 checks passed
@alafanechere alafanechere deleted the augustin/03-27-Python_base_image_create_airbyte_user branch November 22, 2024 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants