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

feat: area/promtail: Add support to install wget on promtail docker image to support docker healthcheck #11711

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

Sheikh-Abubaker
Copy link
Contributor

@Sheikh-Abubaker Sheikh-Abubaker commented Jan 18, 2024

What this PR does / why we need it:
Users will now be able to make http request on the promtail API to do the healthcheck.
Which issue(s) this PR fixes:
Fixes #11590

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@Sheikh-Abubaker Sheikh-Abubaker requested a review from a team as a code owner January 18, 2024 20:58
@Sheikh-Abubaker Sheikh-Abubaker changed the title Added support to install wget on promtail docker image area/promtail: Added support to install wget on promtail docker image Jan 19, 2024
@cstyan
Copy link
Contributor

cstyan commented Jan 19, 2024

Can you elaborate on what your end goal is here? It's not entirely clear to me. Promtail already serves all of it's endpoints on an HTTP port.

We also have some users asking for images with even less dependencies, like a scratch image with only our binaries. I think we're more likely to go towards that direction than installing more packages.

@Sheikh-Abubaker
Copy link
Contributor Author

Sheikh-Abubaker commented Jan 19, 2024

Can you elaborate on what your end goal is here? It's not entirely clear to me. Promtail already serves all of it's endpoints on an HTTP port.

@cstyan As you can see the issue #11590 it says there @efficks wants to enable healthcheck on the grafana/promtail docker image but wget or curl is not installed on the image.

To verify the above I went on to check it on my system and found out wget is not installed in grafana/promtail docker image:

sheikhabubaker@DESKTOP-TFVM3PT:~$ docker pull grafana/promtail
Using default tag: latest
latest: Pulling from grafana/promtail
Digest: sha256:c9f886511f40d2cfce44fcbcf650d632c7c73b98a96ee15f4e72ed52e68e7cdc
Status: Image is up to date for grafana/promtail:latest
docker.io/grafana/promtail:latest

What's Next?
  View a summary of image vulnerabilities and recommendations → docker scout quickview grafana/promtail
sheikhabubaker@DESKTOP-TFVM3PT:~$ docker run -it --rm --name promtail-cont --entrypoint /bin/sh grafana/promtail:latest
# wget --version
/bin/sh: 1: wget: not found
#

To install wget onto the grafana/promtail docker image I appended wget to package installing stage into Dockerfile.

@cstyan
Copy link
Contributor

cstyan commented Feb 20, 2024

I commented on the related issue, no response from the original issue reporter yet: #11590 (comment)

@Sheikh-Abubaker Sheikh-Abubaker changed the title area/promtail: Added support to install wget on promtail docker image feat:area/promtail: Added support to install wget on promtail docker image Feb 22, 2024
@Sheikh-Abubaker Sheikh-Abubaker changed the title feat:area/promtail: Added support to install wget on promtail docker image feat: area/promtail: Added support to install wget on promtail docker image Feb 27, 2024
@cstyan
Copy link
Contributor

cstyan commented Apr 19, 2024

@Sheikh-Abubaker okay, we can move forward with adding wget to the image. I'm not sure what's happened with the CLA check, you could try squashing all the commits into one but it might actually just be easiest to create a new branch locally off of main and force push to the branch this PR is open for.

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>
@Sheikh-Abubaker
Copy link
Contributor Author

@cstyan done!!

Copy link
Contributor

@cstyan cstyan left a comment

Choose a reason for hiding this comment

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

thanks for your patience @Sheikh-Abubaker

@cstyan cstyan changed the title feat: area/promtail: Added support to install wget on promtail docker image feat: area/promtail: Added support to install wget on promtail docker image to support docker healthcheck Apr 19, 2024
@cstyan cstyan merged commit ffe684c into grafana:main Apr 19, 2024
14 checks passed
@Sheikh-Abubaker
Copy link
Contributor Author

thanks for your patience @Sheikh-Abubaker

@cstyan you're welcome, thanks for your support too 😀!!

@Sheikh-Abubaker Sheikh-Abubaker changed the title feat: area/promtail: Added support to install wget on promtail docker image to support docker healthcheck feat: area/promtail: Add support to install wget on promtail docker image to support docker healthcheck May 3, 2024
chaudum added a commit that referenced this pull request Nov 25, 2024
The package has been added to the Docker image with PR #11711 with the
intention to support the Docker healthcheck.

However, to reduce the attack surface of our Docker images, we want to
keep them as slim as possible. The current version of Promtail (3.3.0)
for example contains a wget version with vulnerability
[CVE-2024-38428](https://security-tracker.debian.org/tracker/CVE-2024-38428).

The healthcheck can be achieved by other means, e.g.

1. Extend the `grafana/promtail` base image and add `wget` using `apt
   install wget`
   #11590 (comment)
2. Use low-level `/dev/tcp/127.0.0.1:9080` to establish a connection and
   check the exit code
   #11590 (comment)

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
@chaudum
Copy link
Contributor

chaudum commented Nov 25, 2024

@Sheikh-Abubaker We would like to remove wget from the image again to reduce vulnerability attack surface, see #15101
The health check can be achieved in other ways without wget as well. I wanted to inform you, because the removal may be controversial :)

@Sheikh-Abubaker
Copy link
Contributor Author

@chaudum No worries!

chaudum added a commit that referenced this pull request Nov 27, 2024
The package has been added to the Docker image with PR #11711 with the intention to support the Docker healthcheck.

However, to reduce the attack surface of our Docker images, we want to keep them as slim as possible. The current version of Promtail (3.3.0) for example contains a wget version with vulnerability [CVE-2024-38428](https://security-tracker.debian.org/tracker/CVE-2024-38428).

The healthcheck can be achieved by other means, e.g.

1. Extend the `grafana/promtail` base image and add `wget` using `apt install wget`
   #11590 (comment)
3. Use low-level `/dev/tcp/127.0.0.1:9080` to establish a connection and check the exit code
   #11590 (comment)

Original discussion about adding wget #11590
This may break someone's Docker compose installation, when they require on the `wget` powered health check.


Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
loki-gh-app bot pushed a commit that referenced this pull request Nov 27, 2024
The package has been added to the Docker image with PR #11711 with the intention to support the Docker healthcheck.

However, to reduce the attack surface of our Docker images, we want to keep them as slim as possible. The current version of Promtail (3.3.0) for example contains a wget version with vulnerability [CVE-2024-38428](https://security-tracker.debian.org/tracker/CVE-2024-38428).

The healthcheck can be achieved by other means, e.g.

1. Extend the `grafana/promtail` base image and add `wget` using `apt install wget`
   #11590 (comment)
3. Use low-level `/dev/tcp/127.0.0.1:9080` to establish a connection and check the exit code
   #11590 (comment)

Original discussion about adding wget #11590
This may break someone's Docker compose installation, when they require on the `wget` powered health check.

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
(cherry picked from commit 2eea546)
loki-gh-app bot pushed a commit that referenced this pull request Nov 27, 2024
The package has been added to the Docker image with PR #11711 with the intention to support the Docker healthcheck.

However, to reduce the attack surface of our Docker images, we want to keep them as slim as possible. The current version of Promtail (3.3.0) for example contains a wget version with vulnerability [CVE-2024-38428](https://security-tracker.debian.org/tracker/CVE-2024-38428).

The healthcheck can be achieved by other means, e.g.

1. Extend the `grafana/promtail` base image and add `wget` using `apt install wget`
   #11590 (comment)
3. Use low-level `/dev/tcp/127.0.0.1:9080` to establish a connection and check the exit code
   #11590 (comment)

Original discussion about adding wget #11590
This may break someone's Docker compose installation, when they require on the `wget` powered health check.

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
(cherry picked from commit 2eea546)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add wget to promtail Docker image
3 participants