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

fix: only wrapping ENTRYPOINT, if present #112

Merged
merged 20 commits into from Jan 4, 2024
Merged

fix: only wrapping ENTRYPOINT, if present #112

merged 20 commits into from Jan 4, 2024

Conversation

miki725
Copy link
Contributor

@miki725 miki725 commented Dec 20, 2023

Issue

images with custom ENTRYPOINT from base image would break such as default AWS lambda images

Description

Previously we were wrapping either CMD/ENTRYPOINT however that is incorrect behavior as it can change semantics of the image if the base image has any custom ENTRYPOINT

For example official AWS lambda images have this problem and their ENTRYPOINT requires specific CMD shape
and therefore we cannot adjust CMD without guaranteeing container behavior does not change.


In addition PR includes:

  • switches cloud metadata to be json object vs json string
  • reporting all container/task/stats metadata for ecs vs only container metadata which is limiting. task for example includes more information about task such as task arn/version/etc
  • adding minimal lambda metadata collection. not much we can do there as lambda doesnt have endpoint aside from a few env vars but using those we can get some basic data such as aws account id, role arn, etc
  • extracting some lambda/ecs metadata to dedicated chalk keys such as _AWS_REGION
  • fixes various places which were assuming resolvePath can actually resolve which is not true in lambda for example which resulted in segfaults. the reason is that lambda runs with user 993, not default container user which and as that user does not have home path anything which resolves ~ blows up
  • small other things such as adding exception message when plugin blows up which is very helpful in debugging
  • added priority field to sink configs to allow to prioritize some sinks higher in the processing order
  • adds sink retries as we occasionally were seeing read timeouts. as its network related seems like a good idea to add timeouts there for safety

Testing

➜ make tests args="test_docker.py::test_base_image --logs --pdb"

Requires/TODOs

@miki725 miki725 requested a review from viega as a code owner December 20, 2023 16:12
@miki725 miki725 marked this pull request as draft December 21, 2023 03:59
@miki725 miki725 marked this pull request as ready for review December 22, 2023 05:13
@miki725 miki725 force-pushed the ms/cmd branch 2 times, most recently from 8270b06 to df62496 Compare December 27, 2023 17:37
previously we were wrapping either CMD/ENTRYPOINT however that is
incorrect behavior as it can change semantics of the image
if the base image has any custom ENTRYPOINT

For example official AWS lambda images have this problem
and their ENTRYPOINT requires specific CMD shape
and therefore we cannot adjust CMD without guaranteeing
container behavior does not change.
In some cases chalk can run for uid which does not have a home
directory such as in AWS lambda in which case chalk fails to resolve
all paths which have ~ in them.

This fixes that by ignoring those paths when resolvePath fails
and in some other cases like for temp files also checks that
tmp folder is writable.
this now includes all task containers metadata as well as stats (if present)
this allows to prioritize some sinks higher in the reporting order
@miki725 miki725 force-pushed the ms/cmd branch 6 times, most recently from f3ea90f to fba1ec4 Compare January 3, 2024 18:54
otherwise we get `HUP` signal handling indicating there is an error
which is incorrect as HUP is sent as part of TTY handling logic
limiting first year to 2023, even though some files were committed in 2022
this allows tests not be exclusive as otherwise there are conflicts
between tests
at chalk time image is not built yet so we cannot run external tools
on it yet
Copy link
Contributor

@viega viega left a comment

Choose a reason for hiding this comment

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

Okay. Discussed comments off-line, and have been addressed.

@miki725 miki725 merged commit 0b96fdd into main Jan 4, 2024
2 checks passed
@miki725 miki725 deleted the ms/cmd branch January 4, 2024 16:56
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.

None yet

2 participants