Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

Only inject/append Entrypoint if it's not already there #2447

Merged
merged 4 commits into from
Oct 8, 2021

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Oct 7, 2021

Currently the waypoint-entrypoint binary is injected into the container and appended to the front of its Entrypoint definition (as long as disable_entrypoint is false).

In the case of using docker-pull in the build phase, if the image we pull already has the entrypoint included, this will result in a chain of entrypoints as well as additional layers in the image (see #2381).

In this PR, if a container already includes an entry for /waypoint-entrypoint in its Entrypoint definition, then don't inject it again. We assume that if it is already included in the Entrypoint definition, that the binary also exists in the container.

Fixes #2381

@catsby catsby added core pr/no-changelog No automatic changelog entry required for this pull request entrypoint Things related to the entrypoint binary run inside a deployment labels Oct 7, 2021
@catsby catsby requested a review from evanphx October 7, 2021 21:42
if entryPoint == "/waypoint-entrypoint" {
found = true
s.Logger.Debug("waypoint-entrypoint found, not injecting")
break
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intentionally didn't use the new containsEntrypoint function here to avoid including a new package (which at one point gave me a circular dependency issue, but that may be resolved)

@catsby catsby removed the pr/no-changelog No automatic changelog entry required for this pull request label Oct 7, 2021
@catsby catsby requested a review from a team October 7, 2021 21:46
Copy link
Contributor

@evanphx evanphx left a comment

Choose a reason for hiding this comment

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

Just an easy simplification.

internal/pkg/epinject/epinject.go Outdated Show resolved Hide resolved
internal/pkg/epinject/ociregistry/server.go Outdated Show resolved Hide resolved
Copy link
Contributor

@evanphx evanphx left a comment

Choose a reason for hiding this comment

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

Just need to check the lengths too!

internal/pkg/epinject/epinject.go Outdated Show resolved Hide resolved
internal/pkg/epinject/ociregistry/server.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

I actually thought of this back in 0.1 days but thought that the string comparison would be too weird. I guess the pros outweigh the cons and the likelihood someone names their binary waypoint-entrypoint is very low :)

internal/pkg/epinject/epinject.go Outdated Show resolved Hide resolved
@catsby catsby merged commit 813a37f into main Oct 8, 2021
@catsby catsby deleted the single-entrypoint-append branch October 8, 2021 16:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
core entrypoint Things related to the entrypoint binary run inside a deployment plugin/docker plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ECS multiple waypoint-entrypoint and odd behaviors
3 participants